[RFC] making compute CLI command usable inside Python #245

luizirber · 2017-05-20T22:49:18Z

I'm opening this PR for requesting comments about how to properly support calling CLI commands from the Python API, without being too verbose.

~~This is similar to the design I'm going for in sourmash-utils.~~ (Old PR comments, now this is adapted to #785 ideas)

I'll add inline comments with details, but the rationale is:
We focus on exposing the CLI, but if I want to calculate a signature using the Python API there is quite a bit of boilerplate involved:

import screed
minhashes = []
for g in filenames:
    E = sourmash.MinHash(n=500, ksize=31)
    for record in screed.open(g):
        E.add_sequence(record.sequence, True)
    minhashes.append(E)

what I want to do:

from sourmash.command_compute import compute
sigs = compute(filenames)

(or something similar).

The idea of this PoC is to make arguments explicit in the function signature. The CLI exposes commands and options from subparsers, and function arguments are 'fake-positional': only use kwargs, but leave the required ones (like filenames in the compute(filenames) example) in the beginning to avoid having to define explicitly all the keywords.

Checklist

Is it mergeable?
make test Did it pass the tests?
make coverage Is the new code covered?
Did it change the command-line interface? Only additions are allowed
without a major version increment. Changing file formats also requires a
major version number increment.
Was a spellchecker run on the source code and documentation after
changes were made?

luizirber · 2017-05-20T22:50:49Z

sourmash_lib/__main__.py

+            commands[cmd].add_args(subp)
+            subp.set_defaults(cmd=commands[cmd])
+
+    args = parser.parse_args()


subparsers for all commands must be added before calling this. That's why I'm going for some sort of command.add_args function for each command.

luizirber · 2017-05-20T22:51:35Z

sourmash_lib/__main__.py

+    cmd = args.cmd
+    args_dict = vars(args)
+    args_dict.pop('cmd')
+    cmd(**args_dict)


Dirty trick with vars to convert the args (a Namespace object?) to a dictionary that can be expanded for the function call.

luizirber · 2017-05-20T22:53:52Z

sourmash_lib/commands.py

+def compute(filenames=None, check_sequence=False, ksizes=(21,), dna=True, singleton=False,
+            email='', scaled=10000, force=False, output=None, num_hashes=500, protein=False,
+            name_from_first=False, seed=42, input_is_protein=False, merge=None,
+            track_abundance=False):


Possible issue: keeping defaults here and in the argparse code the same. Probably leave the add_args function before the command definition, so at least they are close to each other...

click uses decorators for this:

import click @click.command() @click.option('--count', default=1, help='Number of greetings.') @click.option('--name', prompt='Your name', help='The person to greet.') def hello(count, name): """Simple program that greets NAME for a total of COUNT times.""" for x in range(count): click.echo('Hello %s!' % name) if __name__ == '__main__': hello()

luizirber · 2017-05-20T22:54:36Z

sourmash_lib/commands.py

        notify('WARNING: input is protein, turning off DNA hashing')
-        args.dna = False
-        args.protein = True
+        dna = False


A lot of changes in this diff is just removing args. from the arguments...

luizirber · 2017-05-20T22:55:29Z

sourmash_lib/commands.py

+                        help='set resolution of signature size')
+    parser.add_argument('--seed', type=int, help='hash seed',
+                        default=sourmash_lib.DEFAULT_SEED)
+compute.add_args = compute_add_args


Dirty, horrible hack. Sorry. Will think about something better than this later 😄

luizirber · 2017-05-20T23:36:50Z

I talked with @standage yesterday and he has another approach for kevlar, where you build a list of CLI-like arguments and then parse with argparse, passing the resulting args object into the main function: https://github.com/dib-lab/kevlar/blob/3130636d5b3822502708084e547b63b671fb2cc2/kevlar/tests/test_dump.py#L36
(and he is also using subparsers:
https://github.com/dib-lab/kevlar/blob/3130636d5b3822502708084e547b63b671fb2cc2/kevlar/cli/__init__.py#L53 )

luizirber · 2017-05-20T23:37:11Z

I talked with @betatim today, and he has a similar setup for osf-cli:
https://github.com/dib-lab/osf-cli/blob/b9c750511595cd6b372eae70965a53055fe8933a/osfclient/__main__.py#L23

ctb · 2017-05-21T03:28:16Z

What's old is new!

https://github.com/ctb/twill/blob/master/twill/commands.py

luizirber · 2017-07-13T18:57:03Z

For future ref: https://realpython.com/blog/python/comparing-python-command-line-parsing-libraries-argparse-docopt-click/#commands

luizirber · 2018-08-22T22:54:33Z

I was talking with @taylorreiter and it seems like this is a good PR to start going more into sourmash code, so I updated it to the current master.

After this PR was created there were code changes (for search and gather) where @ctb took some code away from sourmash/commands.py and into function that are easier to call in Python. The issue of exposing the CLI commands as Python functions is that they take A LOT of arguments, but I think it is still good to have them available.

luizirber · 2018-08-22T22:57:30Z

sourmash/__main__.py

+    else:
+        cmd = commands.get(args.command)
+        return cmd(sys.argv[2:])
+


Once all commands are converted we can remove this if block and keep only the first part (the else is the current implementation for subcommands)

luizirber · 2018-08-22T22:58:31Z

sourmash/__main__.py

+    from sourmash_utils.__main__ import main as utils_main
+    UTILS_AVAILABLE = True
+except ImportError:
+    UTILS_AVAILABLE = False


This is here because the original PR was against #236 and I changed it to be against master to have Travis run the tests...

ctb · 2019-08-24T15:40:50Z

I updated in this PR in #722, which resolves the conflicts. Still have some kind of bug in there but it's safe to merge.

codecov · 2019-11-02T01:01:32Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f6ad66c). Click here to learn what that means.
The diff coverage is 96%.

@@            Coverage Diff            @@
##             master     #245   +/-   ##
=========================================
  Coverage          ?   89.47%           
=========================================
  Files             ?       29           
  Lines             ?     4570           
  Branches          ?       46           
=========================================
  Hits              ?     4089           
  Misses            ?      478           
  Partials          ?        3

Impacted Files	Coverage Δ
sourmash/__main__.py	`92.68% <88.88%> (ø)`
sourmash/command_compute.py	`96.55% <98.24%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6ad66c...61f07ea. Read the comment docs.

luizirber · 2019-11-07T23:07:26Z

@ctb can you take a look at this commit: 193748c

This is adding a ksize_parser function for the args (that converts strings into a ksize list), and also modifying the cli test to also run with the compute function. If it looks OK, I would go modify the other tests (for sourmash compute) to match the modified one.

luizirber · 2019-11-07T23:08:01Z

I'm also starting to feel like this should move from [RFC] to something actually mergeable.

prepare compute test command fix default ksize parsing failing test: if output is a string or path, should we open it as a file?

luizirber · 2020-01-08T18:56:51Z

Revamped this on top of #785, and updated the PR description. Comments @ctb @standage?

ctb

I like everything about this, except for the final command "path" of sourmash.command_compute.compute, which seems ...long. No immediate solution (and happy to merge this!) but let's think about how to simplify that!

luizirber · 2020-01-08T20:22:43Z

I like everything about this, except for the final command "path" of sourmash.command_compute.compute, which seems ...long. No immediate solution (and happy to merge this!) but let's think about how to simplify that!

yeah, the command_compute thing is... long.

What I've been doing in the Rust crate is creating submodules over time. Example: at the moment the src dir is

.
├── cmd.rs
├── errors.rs
├── ffi/
├── from.rs
├── index/
├── lib.rs
├── signature.rs
├── sketch/
└── wasm.rs

ffi/, index/ and sketch/ started as single files (like cmd.rs, signature.rs and wasm.rs are now). As more features were implemented I expand the single file into a submodule, but still using the top-level name (index, for example) to control what to export (for the public API).

On the Python side, it would translate to commands.py becoming a dir module, and importing/exposing items from its submodules.

Currently:

sourmash
├── cli/
├── command_compute.py
├── commands.py
...
└── utils.py

Proposed (like what sourmash.cli is doing now):

sourmash
├── cli/
├── commands/
│   ├── __init__.py
│   └── compute.py
...
└── utils.py

This way we can start with a massive implementation file (like commands.py is now) and keep the API working when we reorganize into a submodule and split the code (because both could be accessed by from sourmash.commands import compute, for example). Doing sourmash/command_compute.py goes against this (and I know you like flat structures =]), but it will also start to get hard to find files if all of them are in sourmash/ (which is similar to the sourmash/sbt*.py situation)

Before merging this, I think it is good to have some guidelines on how to proceed for all commands. From the top of my head:

Required arguments in the CLI -> positional arguments in the commands.py function
- Ideally we could use * in the function signature and make some args keyword-only, but that's not valid in Python 2 =(
Keeping defaults from argparse in sync with defaults in the commands.py function
Commands taking output (like sourmash compute -o out input): how to map out into an argument? Accept strings and opened files (argparse open a file)? (This is the failing test in this PR, for example)
docstrings for each command (Add docstrings to functions in sourmash/commands.py #828)
testing: update all tests calling utils.runscript to also test the API command? That's what I did with test_do_sourmash_compute and test_do_sourmash_compute_output_valid_file in this PR.
- Better: modify utils.runscript to do that, if it is passed an extra arg?

ctb · 2020-01-21T04:11:18Z

@luizirber has thus declared: "#853 should fix the commands-taking-output problem mentioned above." yay w00t!

luizirber · 2023-02-13T04:05:47Z

Still think this is useful, especially considering pyodide #2433 and using it in a browser repl, but lots to update... And so I'm closing it.

luizirber commented May 20, 2017

View reviewed changes

luizirber force-pushed the feature/utils_subcmd branch from 68374f5 to c7a79ef Compare May 21, 2017 19:02

luizirber force-pushed the refactor/subparsers branch from 1420a8a to 579de0a Compare May 21, 2017 20:32

luizirber mentioned this pull request May 24, 2017

Recommend using argparse.add_subparser commands #21

Closed

luizirber mentioned this pull request Jun 30, 2017

How do I use the API to created scaled signature? #289

Closed

luizirber force-pushed the feature/utils_subcmd branch from 6693797 to dae9ea9 Compare August 22, 2018 20:10

luizirber force-pushed the refactor/subparsers branch 4 times, most recently from d8cbe5e to c265796 Compare August 22, 2018 21:52

luizirber changed the base branch from feature/utils_subcmd to master August 22, 2018 22:50

luizirber commented Aug 22, 2018

View reviewed changes

luizirber mentioned this pull request Aug 23, 2019

Complete help text with --help? #706

Closed

ctb mentioned this pull request Aug 24, 2019

[MRG] Update the 'refactor/subparsers' branch from #245. #722

Merged

5 tasks

luizirber pushed a commit that referenced this pull request Aug 24, 2019

Update the 'refactor/subparsers' branch from #245. (#722)

185c3d3

luizirber force-pushed the refactor/subparsers branch 2 times, most recently from 553f734 to d5cda3d Compare October 23, 2019 23:48

luizirber force-pushed the refactor/subparsers branch from d5cda3d to b768a31 Compare November 2, 2019 00:48

luizirber force-pushed the refactor/subparsers branch from b768a31 to da231dd Compare November 7, 2019 21:34

standage mentioned this pull request Dec 5, 2019

No --help for subcommands #783

Closed

luizirber mentioned this pull request Dec 6, 2019

Implement improved & consistent argument parsing #785

Merged

5 tasks

luizirber force-pushed the refactor/subparsers branch from 5449e14 to 1a5b068 Compare January 8, 2020 16:05

making compute usable inside Python

8766e95

prepare compute test command fix default ksize parsing failing test: if output is a string or path, should we open it as a file?

luizirber force-pushed the refactor/subparsers branch from 1a5b068 to 8766e95 Compare January 8, 2020 18:50

keep default as str

f6ad66c

ctb approved these changes Jan 8, 2020

View reviewed changes

luizirber modified the milestones: pre-4.0, 4.0 Jan 14, 2020

This was referenced Jan 18, 2020

Compute oxidation #845

Merged

[MRG] refactor argparse.FileType out of sourmash argument handling. #853

Merged

Merge branch 'master' into refactor/subparsers

61f07ea

luizirber mentioned this pull request Jul 18, 2020

Idea: imports match CLI? #1112

Open

luizirber closed this Feb 13, 2023

luizirber deleted the refactor/subparsers branch February 13, 2023 04:09

luizirber mentioned this pull request Mar 12, 2023

consider refactoring command-line interface code around plugin code #2521

Open

ctb mentioned this pull request Jul 24, 2023

consider adding/testing simple way to create an args object for use with sourmash.commands etc. #2684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] making compute CLI command usable inside Python #245

[RFC] making compute CLI command usable inside Python #245

luizirber commented May 20, 2017 •

edited

Loading

luizirber May 20, 2017

luizirber May 20, 2017

luizirber May 20, 2017

luizirber May 20, 2017

luizirber May 20, 2017

luizirber commented May 20, 2017 •

edited by ctb

Loading

luizirber commented May 20, 2017

ctb commented May 21, 2017

luizirber commented Jul 13, 2017

luizirber commented Aug 22, 2018

luizirber Aug 22, 2018

luizirber Aug 22, 2018

ctb commented Aug 24, 2019

codecov bot commented Nov 2, 2019 •

edited

Loading

luizirber commented Nov 7, 2019

luizirber commented Nov 7, 2019

luizirber commented Jan 8, 2020

ctb left a comment

luizirber commented Jan 8, 2020 •

edited

Loading

ctb commented Jan 21, 2020

luizirber commented Feb 13, 2023

[RFC] making compute CLI command usable inside Python #245

[RFC] making compute CLI command usable inside Python #245

Conversation

luizirber commented May 20, 2017 • edited Loading

Checklist

luizirber May 20, 2017

Choose a reason for hiding this comment

luizirber May 20, 2017

Choose a reason for hiding this comment

luizirber May 20, 2017

Choose a reason for hiding this comment

luizirber May 20, 2017

Choose a reason for hiding this comment

luizirber May 20, 2017

Choose a reason for hiding this comment

luizirber commented May 20, 2017 • edited by ctb Loading

luizirber commented May 20, 2017

ctb commented May 21, 2017

luizirber commented Jul 13, 2017

luizirber commented Aug 22, 2018

luizirber Aug 22, 2018

Choose a reason for hiding this comment

luizirber Aug 22, 2018

Choose a reason for hiding this comment

ctb commented Aug 24, 2019

codecov bot commented Nov 2, 2019 • edited Loading

Codecov Report

luizirber commented Nov 7, 2019

luizirber commented Nov 7, 2019

luizirber commented Jan 8, 2020

ctb left a comment

Choose a reason for hiding this comment

luizirber commented Jan 8, 2020 • edited Loading

ctb commented Jan 21, 2020

luizirber commented Feb 13, 2023

luizirber commented May 20, 2017 •

edited

Loading

luizirber commented May 20, 2017 •

edited by ctb

Loading

codecov bot commented Nov 2, 2019 •

edited

Loading

luizirber commented Jan 8, 2020 •

edited

Loading