Implemented the CLI with Click #309
Conversation
Codecov Report
@@ Coverage Diff @@
## master #309 +/- ##
==========================================
- Coverage 76.13% 73.54% -2.59%
==========================================
Files 43 43
Lines 7076 6323 -753
==========================================
- Hits 5387 4650 -737
+ Misses 1689 1673 -16
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@vishav1771 This is a good start! My initial comments are not too specific, just wanted to start a conversation. Feel free to write comments on this PR with any questions / difficulties you have, and we can help. Once this PR is approved with just the find command, we should start a long-running feature branch called click and merge PRs into that, until we're ready to fully replace the existing CLI.
.circleci/config.yml
Outdated
| name: run tests | ||
| command: | | ||
| python -m pytest tests/ -v | ||
| python -m pytest tests/test_shell.py -v |
There was a problem hiding this comment.
Why did you change the tests command here?
There was a problem hiding this comment.
Just want to check the test_find in test_shell first. So changed it for simplicity. Forgot to revert back
|
|
||
|
|
||
| def main_project(args): | ||
| class MultipleOptionalArgument(click.Option): |
There was a problem hiding this comment.
I'm not sure if I understand the purpose of this class. @vishav1771 Can you offer some explanation for why this exists? It seemed to me that "Multi Value Options" or "Multiple Options" would suffice, but I'm not sure if I understand the purpose. https://click.palletsprojects.com/en/7.x/options/#multi-value-options
There was a problem hiding this comment.
I tried to match the original command. As click does not support nargs=*, +, ?, so I need to do some monkey patching. This class customize the add_to_parser func and set the arguments according to need.
https://stackoverflow.com/questions/48391777/nargs-equivalent-for-options-in-click
The "Multiple Option" was the other option. But I found a few issues:
- For
--sp: I need to divide this command into two commands--all-sp,--spwhere--all-spwill work as--sp(previous api) with no argument and--spwill work as--sp(previous api). - For
--doc-filter: command--doc-filter a 0will be written as--doc-filter a --doc-filter 0which do not make sense. And if I go for tuple of two values then I was not able to create--doc-filter a{"a": {"$exists": true}}
(Solution to this would be to change--doc-filter key:value)
Kindly provide the feedback.
There was a problem hiding this comment.
These are the exact issues I anticipated in attempting an exact replication of the current interface and I'm glad that we are having this discussion.
The click framework intentionally does not support options with variadic arguments, because it creates syntactical ambiguities. This makes sense to me and I'd prefer to deprecated these instead of trying to force such behavior into the new framework.
However, there is development on enabling options with default arguments, which should make it much easier to replicate the existing interface.
@vishav1771 Could you make a suggestion as to how we could revise the find interface to not rely on variadic options, but assuming that flag options are possible?
Then we can identify how we can use the kind of work-arounds you introduced here to enable backwards compatible behavior with deprecation warnings.
There was a problem hiding this comment.
However, there is development on enabling options with default arguments, which should make it much easier to replicate the existing interface.
Taking this into account, I think --sp should not be changed. But we need to change --doc-filter.
We can implement --doc-filter key:value with setting multiple to True. So --doc-filter a 0 b 1 c would look like --doc-filter a:0 --doc-filter b:1 --doc-filter c. In my opinion, arguments to find should be also be changed (it can be implemented as it is) in a similar way. (find a 0 b 1 c to find a:0 b:1 c)
There was a problem hiding this comment.
Taking this into account, I think
--spshould not be changed.
Are you sure about that? Since --sp is currently accepting an arbitrary number of arguments and I don't see that supported even with the patch.
But we need to change
--doc-filter.
We can implement--doc-filter key:valuewith setting multiple to True. So--doc-filter a 0 b 1 cwould look like--doc-filter a:0 --doc-filter b:1 --doc-filter c.
Afaik, it is possible to specify the exact number arguments an option accepts, so while it's true that users would need to use multiple options to create a filter, changing the notation to the key:value syntax would not strictly be necessary, right?
In my opinion, arguments to
findshould be also be changed (it can be implemented as it is) in a similar way. (find a 0 b 1 ctofind a:0 b:1 c)
I think we should be much more careful with considerations concerning changing the API for the arguments. Users are very much used to these now and it would present a huge break in backwards compatibility.
I propose we keep the discussion going and I will also experiment with this a little bit myself.
There was a problem hiding this comment.
Afaik, it is possible to specify the exact number arguments an option accepts, so while it's true that users would need to use multiple options to create a filter, changing the notation to the key:value syntax would not strictly be necessary, right?
It is not strictly necessary. But i think --doc-filter a:0 is much more intuitive than --doc-filter a --doc-filter 0
I think we should be much more careful with considerations concerning changing the API for the arguments. Users are very much used to these now and it would present a huge break in backwards compatibility.
Makes sense. (I just wanted to keep both the filters similar.)
There was a problem hiding this comment.
Are you sure about that? Since --sp is currently accepting an arbitrary number of arguments and I don't see that supported even with the patch.
I think that should be achievable by setting multiple to True(--sp a --sp b). I will try and let you know.
There was a problem hiding this comment.
I tried and was able to implement sp option as --sp, --sp=a --sp=b. Is it okay?
|
|
||
| try: | ||
| for job_id in find_with_filter(args): | ||
| for job_id in find_with_filter(**kwargs): |
There was a problem hiding this comment.
You might find types.SimpleNamespace useful in places that would otherwise require porting between args (which is a namespace) and **kwargs (which is a dictionary). https://docs.python.org/3/library/types.html#types.SimpleNamespace
|
@vishav1771 What is the status on this? Were you able to make any progress? I think it would be important to at least finish the implementation of one command even we decide to put the project on hold after that. |
|
Hi @vishav1771, I had some discussion on this PR with @csadorf and we decided that some of the decisions that are currently being discussed in this PR would be better addressed in a series of smaller, non-breaking changes to the rest of the repository. Once we've made some of those changes, we can come back to this pull request and hopefully it will be easier to make decisions at that point. For now, the first pull request we should complete is #188, which was partially completed a while ago but went stale and was never merged in. The idea is to enable a unified querying system where statepoint and document keys can be specified through a single filter (rather than separate I'm going to assign you to #188 for now, but feel free to comment if you think there's a better path or if you have any other questions or concerns about this. |
|
@mikemhenry @vyasr This is no longer blocked as #332 is merged, correct? |
|
I don't think this is blocked any longer, but I also don't recall whether we made any decisions regarding this item's priority. Did we decide that it would be worth it, or if the effort required for the change would be too substantial to be worth the payoff? Since IIRC it will require at least minor breaking of the CLI, if we do want to push forward with this I think it would need to be aligned with a major release. I don't particularly want to delay 2.0 for this change without a stronger motivation. |
I think the change introduced with #332 deprecates the ambivalent command line pattern that was holding up this effort. I think it would be worth it to at least briefly evaluate how much effort it would take to complete this. It should not be a blocker for 2.0. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Hi @vishav1771, have you had a chance to look at this? |
|
@vishav1771 I assume that we need to assign somebody else for this. Any of the @glotzerlab/signac-committers open to taking this on? |
|
@csadorf I would vote to close this PR. It is stalled and I don’t know of any champions within the developer pool. It can always be done later if we have someone interested! |
|
This effort is put on hold for now. |
6 participants
Description
Implemented the CLI with Click instead of argparse
Motivation and Context
#297
Types of Changes
1The change breaks (or has the potential to break) existing functionality.
Checklist:
If necessary:
Example for a changelog entry:
Fix issue with launching rockets to the moon (#101, #212).