Implement improved & consistent argument parsing #785
Conversation
Codecov Report
@@ Coverage Diff @@
## master #785 +/- ##
==========================================
+ Coverage 79.21% 79.36% +0.14%
==========================================
Files 45 83 +38
Lines 6707 7017 +310
Branches 469 469
==========================================
+ Hits 5313 5569 +256
- Misses 1094 1148 +54
Partials 300 300
Continue to review full report at Codecov.
|
|
(do you want contributions to this PR, if I feel so motivated? :)
On Thu, Dec 05, 2019 at 03:50:28PM -0800, Daniel Standage wrote:
standage commented on this pull request.
> @@ -0,0 +1,71 @@
+# Proposed CLI design
+
+This code demonstrates the mechanics of implementing a CLI with multiple levels of subcommand nesting.
+The `cli/` directory here would be placed in the sourmash root directory.
+
+Note that the actual arguments are incomplete for the examples below, and incorrect for all the commands/subcommands not shown below.
+Fixing all of that will be a tedious copy/paste job.
Reiterating: I haven't actually filled in most of the arguments. For the most part, what's in place now are placeholders.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#785 (review)
--
C. Titus Brown, ctbrown@ucdavis.edu
|
I'm not expecting any this early, but any feedback or contributions would be welcome. Pointers and contributions would certainly be welcome as the CLI matures and we need to start updating the test suite to compensate. |
|
Ok, the CLI is fully implemented—as in DesignEach command/subcommand will have a:
|
|
Part of the redesign is to have the SBT subcommands grouped under the Incidentally, |
sourmash/cli/__init__.py
Outdated
| # BEGIN: dirty hacks to simultaneously support new and previous interface | ||
| if hasattr(args, 'subcmd') and args.subcmd == 'import': | ||
| args.subcmd = 'ingest' | ||
| if hasattr(args, 'cmd') and args.cmd == 'sbt_combine': | ||
| args.cmd = 'sbt' | ||
| args.subcmd = 'combine' | ||
| if hasattr(args, 'cmd') and args.cmd == 'index': | ||
| args.cmd = 'sbt' | ||
| args.subcmd = 'index' | ||
| if hasattr(args, 'cmd') and args.cmd == 'categorize': | ||
| args.cmd = 'sbt' | ||
| args.subcmd = 'categorize' | ||
| if hasattr(args, 'cmd') and args.cmd == 'watch': | ||
| args.subcmd = 'sbt' | ||
| args.subcmd = 'watch' | ||
| if hasattr(args, 'subcmd') and args.subcmd == 'compare_csv': | ||
| args.subcmd = 'compare' | ||
| # END: dirty hacks to simultaneously support new and previous interface |
There was a problem hiding this comment.
Part of the dirty hacks mentioned
sourmash/cli/__init__.py
Outdated
| # BEGIN: dirty hacks to simultaneously support new and previous interface | ||
| sbt.categorize.subparser(sub) | ||
| sbt.combine.alt_subparser(sub) | ||
| sbt.index.subparser(sub) | ||
| sbt.watch.subparser(sub) | ||
| # END: dirty hacks to simultaneously support new and previous interface |
There was a problem hiding this comment.
Moar dirty hacks: support both sourmash categorize and sourmash sbt categorize, both sourmash sbt_combine and sourmash sbt combine, and so on.
There was a problem hiding this comment.
categorize, watch and index are potentially index operations, should work with lca too (so I suggest keeping it as a upper level, instead of inside sbt).
There was a problem hiding this comment.
Gotcha. Well, that only leaves the sbt_combine as a candidate for grouping sbt-specific subcommands, so I'll just undo that grouping and revert those 4 subcommands to top-level commands.
sourmash/cli/lca/compare.py
Outdated
| subparser = subparsers.add_parser('compare') | ||
| subparser.add_argument('csv1', help='taxonomy spreadsheet output by classify') | ||
| subparser.add_argument('csv2', help='custom taxonomy spreadsheet') | ||
| subparser.add_argument( | ||
| '-q', '--quiet', action='store_true', | ||
| help='suppress non-error output' | ||
| ) | ||
| subparser.add_argument( | ||
| '-d', '--debug', action='store_true', | ||
| help='output debugging output' | ||
| ) | ||
| subparser.add_argument( | ||
| '-C', '--start-column', metavar='C', default=2, type=int, | ||
| help='column at which taxonomic assignments start; default=2' | ||
| ) | ||
| subparser.add_argument( | ||
| '--tabs', action='store_true', | ||
| help='input spreadsheet is tab-delimited; default is commas' | ||
| ) | ||
| subparser.add_argument( | ||
| '--no-headers', action='store_true', | ||
| help='no headers present in taxonomy spreadsheet' | ||
| ) | ||
| subparser.add_argument('-f', '--force', action='store_true') | ||
|
|
||
| # Dirty hack to simultaneously support new and previous interface | ||
| # If desired, this function can be removed with a major version bump. | ||
| subparser = subparsers.add_parser('compare_csv') | ||
| subparser.add_argument('csv1', help='taxonomy spreadsheet output by classify') | ||
| subparser.add_argument('csv2', help='custom taxonomy spreadsheet') | ||
| subparser.add_argument( | ||
| '-q', '--quiet', action='store_true', | ||
| help='suppress non-error output' | ||
| ) | ||
| subparser.add_argument( | ||
| '-d', '--debug', action='store_true', | ||
| help='output debugging output' | ||
| ) | ||
| subparser.add_argument( | ||
| '-C', '--start-column', metavar='C', default=2, type=int, | ||
| help='column at which taxonomic assignments start; default=2' | ||
| ) | ||
| subparser.add_argument( | ||
| '--tabs', action='store_true', | ||
| help='input spreadsheet is tab-delimited; default is commas' | ||
| ) | ||
| subparser.add_argument( | ||
| '--no-headers', action='store_true', | ||
| help='no headers present in taxonomy spreadsheet' | ||
| ) | ||
| subparser.add_argument('-f', '--force', action='store_true') |
There was a problem hiding this comment.
Another example of the dirty hack scheme: duplicated subparser definitions, presumably to be removed with a major version bump.
|
Short version: intermediate review plz? Long version: Ok, I think I've reached an inflection point. Everything I've done up to this point should be stable for the long term. Minor updates to the CLI on the master branch would have to be recapitulated in this branch, but I expect those types of changes to be infrequent and minimal. But hooking up the new CLI to the command/subcommand implementations is going to require moving around A LOT of code, and doing so in a way that git will have difficulty tracking. I don't think we should start this process until we are confident we can resolve it quickly. Any other PRs that are merged in parallel in the mean time will have to be recapitulated here, and I'm not enthusiastic about coordinating that. So before I proceed here, could you folks provide some feedback on what I've done so far? Any questions or concerns? If y'all support where I'm going with this, I'll be happy to take the lead on the next steps. It'll be tedious, but I don't think it'll take an inordinate amount of time. The last thing I want to do is spend several hours updating the command/subcommand implementations, only to then spend the next few months trying to keep the PR in sync with parallel updates. |
|
By the way, I've copy/pasted some terminal output above, but the best way to see how this is all working might just be to checkout the |
|
This is really great, @standage! I was partially doing this with subparsers in #245, but I totally support moving all the argparsing into I was keeping the |
Does it need to move code? I think we should keep the command code in diff --git a/sourmash/cli/compare.py b/sourmash/cli/compare.py
index e55b925..eaf93b8 100644
--- a/sourmash/cli/compare.py
+++ b/sourmash/cli/compare.py
@@ -36,4 +36,4 @@ def subparser(subparsers):
def main(args):
- print(args)
+ return sourmash.commands.compare(args)UPDATE: OK, I tried this with
Truer wods were never spoken. Keeping PR in sync with parallel updates is horrible =] |
That's true. At that point, I was mixing concerns: having a functional CLI, and then being able to invoke that CLI conveniently from Python/the test suite (a la #245). |
|
The suggestion of moving code referred to the design proposed at the end of this comment. |
ctb
changed the title
|
(note, adjusted title) |
|
@standage sorry for all the changes landing on master =( But... there is a window for including this in |
No worries. As I said earlier, what I've done so far is pretty isolated from the rest of the codebase.
Yep, I had intended to finish this already. But...you know how it goes.
If I'm ever moving too slowly, feel free to push ahead without me. 😀 |
Taking over sounded too aggressive, so I opened #811 (merging into this one) for implementations of all commands (pretty much just calling what already exists). All tests are now working (I added back missing commands, like Some notes:
Future
|
|
Any ideas on how to get nicer usage/help messages? Current ones for |
Yeah, it's a trade off: flexibility or convenience. It is possible to have some middle ground using The output of Maybe with something like this, although it does run a bit long. We could easily build it automatically by recursively traversing the We could potentially even group the commands into sections, reflecting the command/subcommand structure, with "Basic commands" being the default. By the way, are additional |
I started doing one that digged into the argparse internals and... it got ugly fast. =/
Ooooh, this one is NICE!
Yes, I'll be more mindful when adding commands in the future... for now it's better to keep what exist working, and then remove on 4.0 if no other commands show up. |
|
Ok, Command groups have a similar summary. And finally, each operation has its own usage. |
|
is there a way to hide certain operations, and/or prioritize others in the
output?
e.g. compare, compute, search, gather should be in the basic operations,
and maybe index, but not the others.
lca could all be grouped under one, same for sig.
I have a few goals here --
* limit complexity for new users
* keep screen display for basic help < 25 rows
we can probably keep a custom help message here, come to think of it.
|
|
p.s. and wow, this is super cool :)
|
| self.citation = citation | ||
|
|
||
| @classmethod | ||
| def print_citation(cls): |
There was a problem hiding this comment.
I made print_citation a class method, and storing the _citation_printed at the class level let's us assert that it is only printed once along all commands (because all commands are subclasses of SourmashParser).
(the classmethod trick is straight out of snakemake, btw)
|
🎉 Thanks @standage 💯 |
|
yay w00t thanks!!
|
|
Awesome. Thanks for your feedback! |
The sourmash CLI is a bit hairy using a combination of approaches to parse subcommand and subsubcommand arguments. This is my first pass at implementing a consistent approach using vanilla argparse. A lot of copy/paste is still required, and some tests will need to be updated, but the core design I propose is well demonstrated already.
UPDATE: The proposed CLI implementation is complete—that is,
--helpworks for all commands and subcommands. Having the CLI invoke the appropriate methods is pending. Here I propose making a new subcommand group (e.g.sourmash sbt combineinstead ofsourmash sbt_combine,sourmash sbt categorizeinstead ofsourmash categorize, and so on). This is also implemented, along with a few dirty hacks to support the old CLI (dirty hacks are annotated to simplify removal on a major version bump).Closes #783.
make testDid it pass the tests?make coverageIs the new code covered?without a major version increment. Changing file formats also requires a
major version number increment.
changes were made?