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

Introduce main `gammapy` command line tool #1235

Merged
merged 14 commits into from Dec 19, 2017

Conversation

Projects
None yet
2 participants
@adonath
Member

adonath commented Dec 8, 2017

This PR introduces a main gammapy command line tool, which allows us to restructure the gammapy scripts into a single gammapy <command> sub-commands in future. This way the structure of the command line tool becomes very similar to git or conda. Currently it only support the gammapy info and gammapy check sub commands.

@adonath adonath self-assigned this Dec 8, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 8, 2017

Member

@adonath - Thanks!

Concerning https://travis-ci.org/gammapy/gammapy/jobs/313611465#L2463 - I think it's right, you're calling main without passing anything. How does conda do it? Do they have a default in def main(args=None)?

Does python -m gammapy --help work for you locally? I think it should fail, no?
It's nice that our conda build catched that. Maybe we should have an explicit new build on travis-ci that executes the command line tool via gammapy and via python -m gammapy, and does a few things like execute info? (if you want, add it here; if not we can add it later)

We had issues in the past (conda build fails) with having setuptools entry points pointing to functions with the same name as the file they are in. You currently have def main in main.py. Can you please rename one of the two, e.g. to cli?

This one is also weird, I don't understand why it occurs just from looking at the diff:
https://travis-ci.org/gammapy/gammapy/jobs/313611463#L2567
Let me know if I should have a look an investigate here.

Member

cdeil commented Dec 8, 2017

@adonath - Thanks!

Concerning https://travis-ci.org/gammapy/gammapy/jobs/313611465#L2463 - I think it's right, you're calling main without passing anything. How does conda do it? Do they have a default in def main(args=None)?

Does python -m gammapy --help work for you locally? I think it should fail, no?
It's nice that our conda build catched that. Maybe we should have an explicit new build on travis-ci that executes the command line tool via gammapy and via python -m gammapy, and does a few things like execute info? (if you want, add it here; if not we can add it later)

We had issues in the past (conda build fails) with having setuptools entry points pointing to functions with the same name as the file they are in. You currently have def main in main.py. Can you please rename one of the two, e.g. to cli?

This one is also weird, I don't understand why it occurs just from looking at the diff:
https://travis-ci.org/gammapy/gammapy/jobs/313611463#L2567
Let me know if I should have a look an investigate here.

Show outdated Hide outdated gammapy/scripts/info.py Outdated
Show outdated Hide outdated gammapy/__main__.py Outdated

@cdeil cdeil modified the milestones: 0.7, 0.8 Dec 8, 2017

@cdeil cdeil modified the milestones: 0.8, 0.7 Dec 17, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 17, 2017

Member

I do see this error:

$ gammapy check 
Traceback (most recent call last):
  File "/Users/deil/Library/Python/3.6/bin/gammapy", line 11, in <module>
    load_entry_point('gammapy==0.7.dev5369', 'console_scripts', 'gammapy')()
  File "/Users/deil/Library/Python/3.6/lib/python/site-packages/gammapy-0.7.dev5369-py3.6-macosx-10.13-x86_64.egg/gammapy/scripts/cli.py", line 63, in main
    exit_code = do_call(args, parser)
  File "/Users/deil/Library/Python/3.6/lib/python/site-packages/gammapy-0.7.dev5369-py3.6-macosx-10.13-x86_64.egg/gammapy/scripts/cli.py", line 13, in do_call
    relative_mod, func_name = args.func.rsplit('.', 1)
AttributeError: 'Namespace' object has no attribute 'func'

I'll have a closer look at this now, trying to add a test and fix for that issue, and generally to understand how this works ...

Member

cdeil commented Dec 17, 2017

I do see this error:

$ gammapy check 
Traceback (most recent call last):
  File "/Users/deil/Library/Python/3.6/bin/gammapy", line 11, in <module>
    load_entry_point('gammapy==0.7.dev5369', 'console_scripts', 'gammapy')()
  File "/Users/deil/Library/Python/3.6/lib/python/site-packages/gammapy-0.7.dev5369-py3.6-macosx-10.13-x86_64.egg/gammapy/scripts/cli.py", line 63, in main
    exit_code = do_call(args, parser)
  File "/Users/deil/Library/Python/3.6/lib/python/site-packages/gammapy-0.7.dev5369-py3.6-macosx-10.13-x86_64.egg/gammapy/scripts/cli.py", line 13, in do_call
    relative_mod, func_name = args.func.rsplit('.', 1)
AttributeError: 'Namespace' object has no attribute 'func'

I'll have a closer look at this now, trying to add a test and fix for that issue, and generally to understand how this works ...

@cdeil cdeil assigned cdeil and unassigned adonath Dec 17, 2017

@cdeil

@adonath - Can we discuss this a bit and try to finish it together?

There's a few things I don't fully understand yet, and that I'd like to simplify if possible.

One problem is the one mentioned in the previous comment: gammapy check errors out in a weird way instead of displaying help. I'm not sure how to fix that in the current scheme!? Do we need a dummy func? Or inject -h like you do for the main gammapy? Or change do_call somehow?

Concerning testing and run_command, I'm not quite sure why it needs to be so complex, i.e. making strings and splitting them again. It should be possible to just pass args to main, no? Also, it's not obvious to me when catching SystemExit is needed and when it isn't. Maybe you or I could try using pytest-console-scripts instead (see #574) and get rid of the custom run_command implementation?

Finally: are you sure that setting the log level works? Would it be easy to add a test that shows that it does?

Show outdated Hide outdated gammapy/utils/testing.py Outdated
Show outdated Hide outdated gammapy/scripts/tests/test_main.py Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 17, 2017

Member

Do we gain anything with the run_command function? Wouldn't it be possible to just call main directly? This is how Astropy does it. Example:
https://github.com/astropy/astropy/blob/8999e21498081852e054947b13aa886a61af4e9c/astropy/io/fits/tests/test_fitsdiff.py#L90
As far as I can see, run_command doesn't do anything useful and it can be removed?

So either call main from the tests, or use script_runner from pytest-console-scripts because it's nice and simple (presumably no catching of SystemExit needed?). Still needs some experimentation, to see how whatever solution we pick reacts concerning the lazy loading and logging.

Member

cdeil commented Dec 17, 2017

Do we gain anything with the run_command function? Wouldn't it be possible to just call main directly? This is how Astropy does it. Example:
https://github.com/astropy/astropy/blob/8999e21498081852e054947b13aa886a61af4e9c/astropy/io/fits/tests/test_fitsdiff.py#L90
As far as I can see, run_command doesn't do anything useful and it can be removed?

So either call main from the tests, or use script_runner from pytest-console-scripts because it's nice and simple (presumably no catching of SystemExit needed?). Still needs some experimentation, to see how whatever solution we pick reacts concerning the lazy loading and logging.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 18, 2017

Member

@adonath - I have played with ways to structure and test cli's here: https://github.com/cdeil/python-cli-examples/

For now I have preliminary implementations using argparse and click, tomorrow morning I plan to add cliff. click is much simpler, and it automatically has the definition or args and the usage of args in the same place. I haven't figured out yet how to do the lazy-loading, but I'm not sure if it's very important for us. It could certainly be done via setuptools entry_points (like cliff does it) using https://github.com/click-contrib/click-plugins, or maybe it's possible to just implement a custom gammapy --help printout in the custom MultiCommand?

Please go as far as you want here in this PR and then assign over to me.
Probably I'll just merge, but maybe I'll continue a bit tomorrow.

Member

cdeil commented Dec 18, 2017

@adonath - I have played with ways to structure and test cli's here: https://github.com/cdeil/python-cli-examples/

For now I have preliminary implementations using argparse and click, tomorrow morning I plan to add cliff. click is much simpler, and it automatically has the definition or args and the usage of args in the same place. I haven't figured out yet how to do the lazy-loading, but I'm not sure if it's very important for us. It could certainly be done via setuptools entry_points (like cliff does it) using https://github.com/click-contrib/click-plugins, or maybe it's possible to just implement a custom gammapy --help printout in the custom MultiCommand?

Please go as far as you want here in this PR and then assign over to me.
Probably I'll just merge, but maybe I'll continue a bit tomorrow.

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Dec 18, 2017

Member

@cdeil I implemented the changes we've discussed this morning and Travis-CI is passing. So I'd like to leave it at that from my side. Assigning to you .

Member

adonath commented Dec 18, 2017

@cdeil I implemented the changes we've discussed this morning and Travis-CI is passing. So I'd like to leave it at that from my side. Assigning to you .

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 19, 2017

Member

@adonath - Thanks!

Currently gammapy check prints the help for the main gammapy, not the check subcommand. Also I don't think the parser should be handed to the functions that execute the task, they should just take args. I'll try to restructure the code and move the parser help print calls outside those task execution functions.

Member

cdeil commented Dec 19, 2017

@adonath - Thanks!

Currently gammapy check prints the help for the main gammapy, not the check subcommand. Also I don't think the parser should be handed to the functions that execute the task, they should just take args. I'll try to restructure the code and move the parser help print calls outside those task execution functions.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 19, 2017

Member

I tried for an hour to make the right sub-command help come up correctly. I didn't find a good solution, the dispatch part to help or the callback function of the argparse-based solution is annoyingly complex. Like I said, I also think maintaining the args definition for all sub-commands in one file, but then the args usage in separate files, is not a good solution.

So for now I'll change Gammapy to use click and this simple pattern for the sub-commands: https://github.com/cdeil/python-cli-examples/blob/master/click/greet/cli.py

This gives up lazy-loading for now. I'm not sure we need it, but if we do or want it, we can get it back (see https://github.com/click-contrib/click-plugins). I've also asked for advice just now here: pallets/click#904

I don't know if click will be the long-term solution, I'm happy to re-write the cli part again in the future if a better solution emerges. But for now I think click is a nice and as-simple as possible solution that any Gammapy dev will be able to read and write, even Python beginners, whereas with argparse every time I look at it I find it puzzling again (especially where subparsers and callback functions are added, and how / when the right help is invoked).

Member

cdeil commented Dec 19, 2017

I tried for an hour to make the right sub-command help come up correctly. I didn't find a good solution, the dispatch part to help or the callback function of the argparse-based solution is annoyingly complex. Like I said, I also think maintaining the args definition for all sub-commands in one file, but then the args usage in separate files, is not a good solution.

So for now I'll change Gammapy to use click and this simple pattern for the sub-commands: https://github.com/cdeil/python-cli-examples/blob/master/click/greet/cli.py

This gives up lazy-loading for now. I'm not sure we need it, but if we do or want it, we can get it back (see https://github.com/click-contrib/click-plugins). I've also asked for advice just now here: pallets/click#904

I don't know if click will be the long-term solution, I'm happy to re-write the cli part again in the future if a better solution emerges. But for now I think click is a nice and as-simple as possible solution that any Gammapy dev will be able to read and write, even Python beginners, whereas with argparse every time I look at it I find it puzzling again (especially where subparsers and callback functions are added, and how / when the right help is invoked).

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 19, 2017

Member

I've changed to click in 306495e .

Member

cdeil commented Dec 19, 2017

I've changed to click in 306495e .

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 19, 2017

Member

travis-ci passed.

I changed one more cli command gammapy image bin to use click, and removed the imports of py files with cli commands from gammapy/scripts/__init__.py in 7585ec1

Merging this now. We will need follow-up PRs to change all existing cli to this scheme, and to update the Gammapy docs to mention the gammapy cli and python -m gammapy very prominently. Probably we should also use https://pypi.python.org/pypi/sphinx-click to document our cli.

@adonath - Thanks!

Member

cdeil commented Dec 19, 2017

travis-ci passed.

I changed one more cli command gammapy image bin to use click, and removed the imports of py files with cli commands from gammapy/scripts/__init__.py in 7585ec1

Merging this now. We will need follow-up PRs to change all existing cli to this scheme, and to update the Gammapy docs to mention the gammapy cli and python -m gammapy very prominently. Probably we should also use https://pypi.python.org/pypi/sphinx-click to document our cli.

@adonath - Thanks!

@cdeil cdeil merged commit 77505df into gammapy:master Dec 19, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment