Closed w2ll2am closed 1 year ago
Btw, I think your commits are associated with an email address associated to @WilliamBC-SL rather than @w2ll2am. Just a heads up in case you want the PR credited to the latter account; either one is fine with me. You can change this with https://stackoverflow.com/questions/750172/how-do-i-change-the-author-and-committer-name-email-for-multiple-commits
Looks great! Added some comments. One other suggestion is regarding the file structure. Is
_cli
rather thancli
a choice because of naming issue or is it the standard?
I was basing the structure off pip's CLI which stores its main.py
for the CLI in the following directory: pip/_internal/cli/main.py
. My understanding is that the underscore prefix can be used for methods or files that should be for internal use of the module detailed in this article. I'm happy to choose the structure that is the best fit for the project.
Sounds like a good choice, thanks for the info!
Looks almost ready! One more request - update the usage string. Right now it shows this:
> python -m pysr --help
Usage: cli install [OPTIONS]
Install Julia dependencies for PySR
Options:
-p, --project TEXT
-q, --quiet
--precompile
--no-precompile
--help Show this message and exit.
I think this should be, for example (the red texts are deletions, green text additions):
python -m pysr --help
- Usage: cli install [OPTIONS]
+ Usage: pysr install [OPTIONS]
Install Julia dependencies for PySR
Options:
- -p, --project TEXT
+ -p, --project PROJECT_DIRECTORY Install in a specific Julia project (e.g., a local copy of SymbolicRegression.jl).
- -q, --quiet
+ -q, --quiet Disable logging.
- --precompile
+ --precompile Force precompilation of Julia libraries.
- --no-precompile
+ --no-precompile Disable precompilation.
--help Show this message and exit.
Of course! Implemented now. If you want to add any further CLI functionality in the future it should be straight forward to add from now.
Sorry for the delay; I am trying to think about how we could have unit-tests for this besides the install step itself. But I think it is ready to be merged, nice job!
In this PR I changed all of the pysr.install()
lines to use the CLI instead, so all of the deepsource tests rely on python -m pysr install
to work and pass. So the functionality is tested through that, and I've just added a PR to test the --help
messages.
I'm happy for the code to be merged when you are!
Sorry for the long delay! Has been a crazy couple of weeks.
I think the unittests aren't actually running yet, because it needs to be added to the testing CLI: https://github.com/MilesCranmer/PySR/blob/master/pysr/test/__init__.py and https://github.com/MilesCranmer/PySR/blob/master/pysr/test/__main__.py
Could probably add a "cli" option here: https://github.com/MilesCranmer/PySR/blob/60c32792958e9b357d12716b8e614cdc6949a50b/pysr/test/__main__.py#L28
Okay I think I figured out why it’s still not testing. I think it just needs to be added to the GitHub actions; like: python -m pysr cli
to run the CLI tests.
I pushed directly to the branch; hope that's okay with you. Eager to get this working.
Thank you I'm very happy with you making changes! I'm all happy with the PR.
Thanks again for the amazing PR!! I have added you to the contributors list 💯
This PR implements at CLI for the pysr project using the click package and responds to issue-229. The CLI is intended to be used as
python -m pysr install
. Included in this PR is the following:install
command and it's associated optionspython -m pysr install
in place ofpython -c "import pysr; pysr.install()"
Please do leave comments below as verbose as possible, as I'm new to this community.