-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixup setup.py + add pre-commit #142
base: 2.0
Are you sure you want to change the base?
Conversation
@@ -139,3 +139,10 @@ checkpoints/ | |||
experiments/ | |||
wandb/ | |||
raylib/ | |||
|
|||
c_gae.c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These files are all generated when you run pip install --editable .
-- seems like they should be in the .gitignore
- id: check-yaml | ||
- id: check-added-large-files | ||
|
||
# TODO: Add ruff + ruff format for nice linting and consistency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add pre-commit to make it easier to contribute to the repo -- TODO: standardize how to install this? Should we add a Makefile?
|
||
## Contributions | ||
|
||
We're always looking for new contributors! When first starting out, don't forget to run `pre-commit install` before committing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
README still could benefit from more details and context, it's bumpy coming in as a new user, but this is a nice start
@@ -1,3 +1,9 @@ | |||
[build-system] | |||
requires = ["setuptools", "wheel", "Cython", "numpy"] | |||
build-backend = "setuptools.build_meta" | |||
|
|||
[tool.ruff] | |||
line-length = 130 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start standardizing the formatting a bit
author_email="[email protected]", | ||
url="https://github.com/PufferAI/PufferLib", | ||
keywords=["Puffer", "AI", "RL", "Reinforcement Learning"], | ||
include_dirs=[numpy.get_include(), RAYLIB_INCLUDE_DIR], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was previously hardcoded, and was a bug causing breakage on MacOS.
# The extension “.so” is typically in pufferlib/ocean/..., | ||
# and “raylib/lib” is (maybe) two directories up from ocean/<env>. | ||
# So @loader_path/../../raylib/lib is common. | ||
rpath_arg = '-Wl,-rpath,@loader_path/../../raylib/lib' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into a variety of linkage issues here, this was the best I could come up with. Happy to hear any better ideas or solutions to make this cleaner across platforms.
# See https://pre-commit.com for more information | ||
# See https://pre-commit.com/hooks.html for more hooks | ||
|
||
repos: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Should we add pre-commit
to requirements.txt or something? Not sure how this repo handles dev dependencies, a bit confusing...
No description provided.