Add an orderly command line tool. #53

plietar · 2024-05-15T12:54:46Z

A lot of the operational APIs in the package were exposed only as functions, following the R practice. It is more common in Python to expose these as a command like tool.

This adds basic commands to create a repository, run a report, search packets and manage locations. More features and bells and whistles can be added later on.

The tool is currently named orderly, following the package's name. This may change in the future, if and when we decide to rename the package.

plietar · 2024-05-15T12:56:24Z

src/orderly/cli/options.py

+            self.fail(f"{value!r} is not a valid numerical value", param, ctx)
+
+
+# Almost all sub-commands accept a --root option, so it would make sense for it


I'm still a bit on the fence about whether this is worth it. In my testing I was using the --root option a lot so this was quite nice. In practice though I'm not sure that many people will even use it.

Git accepts -C, as does make with similar meaning - it's actually really useful in practice, and most users can ignore it

Oh I definitely want to keep the option, the only thing I'm on the fence about is accepting it as orderly --root /foo run ....

It's nice to work with interactively because I can go back it my shell history, remove the subcommand and replace it with another one, without having to type in the root again. But getting it to work involves a few hacks.

Interestingly, git's -C has slightly different semantics than the --root here. It is attached the the top level command, not each individual one (ie. git commit -C path doesn't work), and it works by changing the working directory before doing anything else. I think tar and make might be similar.

What this means is that any additional paths specified as arguments are now relative to the -C directory, not the directory the command was run from. It also means git -C foo init works, if foo already exists as a directory, and git -C foo init bar is equivalent to git init foo/bar (again assuming foo exists).

plietar · 2024-05-15T13:00:16Z

src/orderly/cli/__init__.py

+@click.argument("type", type=click.Choice(["path", "ssh"]), metavar="TYPE")
+@click.argument("location")
+@with_root
+def location_add(root, name, type, location):


At the moment this looks like orderly location add foo path /path/to/repo. We could possibly infer the type from the third argument, similar to what Git does, and it easily extends to an http implementation too.

This is the Git implementation: https://github.com/git/git/blob/83f1add914c6b4682de1e944ec0d1ac043d53d78/connect.c#L1070

It's an open question whether we'll ever have more types where this doesn't work as well, or even if we'll have types that need more than just a string value.

so far we have:

file (reasonably unique, could be disambiguated by file:// of course)

http (there are actually two of these - one for packit and one for outpack server, but they differ only in auth and will start with https:// and http:// respectively)

ssh (could use user@host:path or make that ssh://user@host:path to be totally clear perhaps)

I doubt we'll have many more, and we're largely in control (via the schema) so we could definitely explore this

#54 simplifies the ssh url support, so it is now easy to detect the type from the URL.

The two http patterns are a bit inconvenient. I don't think we want to detect it based on the use of TLS alone (nothing wrong with serving an outpack_server with a TLS proxy, and for development I can imagine wanting an insecure Packit server). Maybe we can use packit+http://?

richfitz

This is great - some comments and suggestions included

richfitz · 2024-05-15T14:10:20Z

src/orderly/cli/__init__.py

+@click.argument("type", type=click.Choice(["path", "ssh"]), metavar="TYPE")
+@click.argument("location")
+@with_root
+def location_add(root, name, type, location):


so far we have:

file (reasonably unique, could be disambiguated by file:// of course)

http (there are actually two of these - one for packit and one for outpack server, but they differ only in auth and will start with https:// and http:// respectively)

ssh (could use user@host:path or make that ssh://user@host:path to be totally clear perhaps)

I doubt we'll have many more, and we're largely in control (via the schema) so we could definitely explore this

richfitz · 2024-05-15T14:14:50Z

src/orderly/cli/options.py

+            self.fail(f"{value!r} is not a valid numerical value", param, ctx)
+
+
+# Almost all sub-commands accept a --root option, so it would make sense for it


Git accepts -C, as does make with similar meaning - it's actually really useful in practice, and most users can ignore it

richfitz · 2024-05-15T14:16:41Z

src/orderly/cli/options.py

+# is defined on the top-level command, we cannot call the tool with the option
+# further down, ie. `orderly run --root /path foo`.
+#
+# To work around this we apply a bit of magic: we add a hidden option on the


Would it be simpler to allow --root to be passed into the handler for init, then throw with something like --root does not make sense for init?

That could work, but then it wouldn't support passing the root argument at the end, eg orderly run foo --root /foo/bar.

tests/orderly/test_cli.py

richfitz · 2024-05-15T14:22:15Z

pyproject.toml

@@ -40,6 +41,9 @@ Documentation = "https://github.com/mrc-ide/outpack#readme"
 Issues = "https://github.com/mrc-ide/outpack/issues"
 Source = "https://github.com/mrc-ide/outpack"

+[project.scripts]
+orderly = "orderly.cli:cli"


Possibly controversial comment here: if this is orderly then we have the situation where the python version of this and an R version can't really be installed side-by-side (we had an R based cli in orderly1 based on docopt; this has not been copied over to orderly2 yet, and might not). I guess this loops us back to the issue with a good name for this package that stands alone but references that it's related to orderly

Yeah definitely something I want to change eventually. We also can't use outpack, because that's what the Rust code uses 😞

I think I've come to think we should go with pyorderly, both as the CLI tool and as the PyPI name and import name (and maybe repo). Avoids any accidental overwriting with the existing orderly on PyPI and is descriptive albeit a bit unimaginative.

I'd prefer to merge this as is and rename everything in one go though.

One day we can have a single unified CLI that can run both R and Python reports and we can call that one orderly

richfitz · 2024-05-15T14:26:43Z

src/orderly/cli/__init__.py

+
+@click.group(cls=PropagatingGroup)
+@with_root(propagate=True)
+def cli():


Can you override the default help slightly?

(outpack) rfitzjoh@wpia-dide300:~/Documents/src/outpack-py$ orderly --help Usage: orderly [OPTIONS] COMMAND [ARGS]... Options: --help Show this message and exit. Commands: init Initialize at new orderly repository in PATH. location Manage remote locations. run Run a report. search Search existing packets by query.

it would be good to get across that orderly init --help would provide more help

src/orderly/cli/__init__.py

src/orderly/cli/options.py

richfitz · 2024-05-15T14:32:01Z

src/orderly/cli/__init__.py

+
+@cli.command()
+@click.argument("name")
+@click.option(


There's a balance here to be struck. What has worked for us in the past for this issue is to run the received parameters through a yaml parser, which does a pretty good job usually. Then you accept everything as strings. Quotes on command lines are always a special sort of pain though

I personally have a pretty strong distaste for yaml's interpretation of literals, especially when things like y or off get converted to booleans.

Alternatively we can reduce the number of flags from 3 to 2 by saying one is for strings only and the other one uses a json/python parser for literals, and can therefore handle booleans, numbers and strings if the user escapes the quotes.

This is what Nix does in a similar context: https://nixos.org/manual/nix/stable/command-ref/nix-shell#opt-arg --arg accepts an arbitrary expression, --argstr turns the argument into a string literal. One the one hand I find this quite elegant, and I like having fewer flags, but I would find it more complicated to explain to a lay user than simply having one flag per type.

There's also the option of doing -p foo int:1234, which is how I did it in a previous life: https://github.com/microsoft/scitt-ccf-ledger/blob/fd811db702cea568cb3e447ee8e92aa1a57aea02/pyscitt/pyscitt/cli/sign_claims.py#L196-L208

I can't really think of other examples of programs off the top of my head that deal with this issue, although I'm sure they must exist.

Strong agree on some of yaml's choices - we usually restrict that a bunch: https://github.com/mrc-ide/orderly2/blob/main/R/util.R#L182-L189 - I don't think anyone has ever written a key n and expected that to map to FALSE

A lot of the operational APIs in the package were exposed only as functions, following the R practice. It is more common in Python to expose these as a command like tool. This adds basic commands to create a repository, run a report, search packets and manage locations. More features and bells and whistles can be added later on. The tool is currently named `orderly`, following the package's name. This may change in the future, if and when we decide to rename the package.

Co-authored-by: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>

codecov · 2024-06-12T15:03:26Z

Codecov Report

Attention: Patch coverage is 95.80153% with 11 lines in your changes missing coverage. Please review.

Project coverage is 98.87%. Comparing base (b25e84b) to head (aecd30c).

Files	Patch %	Lines
src/orderly/cli/options.py	80.00%	7 Missing and 3 partials ⚠️
src/orderly/__main__.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #53      +/-   ##
==========================================
- Coverage   99.07%   98.87%   -0.21%     
==========================================
  Files          51       55       +4     
  Lines        4017     4279     +262     
  Branches      653      721      +68     
==========================================
+ Hits         3980     4231     +251     
- Misses         21       29       +8     
- Partials       16       19       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

plietar commented May 15, 2024

View reviewed changes

plietar requested a review from richfitz May 15, 2024 13:00

richfitz requested changes May 15, 2024

View reviewed changes

plietar force-pushed the mrc-5360-cli branch 3 times, most recently from 2334046 to 92cb342 Compare May 23, 2024 13:16

plietar requested a review from richfitz May 23, 2024 13:24

plietar and others added 3 commits June 12, 2024 14:55

Update src/orderly/cli/options.py

33ef924

Co-authored-by: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>

Code review.

aecd30c

plietar force-pushed the mrc-5360-cli branch from 92cb342 to aecd30c Compare June 12, 2024 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an orderly command line tool. #53

Add an orderly command line tool. #53

plietar commented May 15, 2024

plietar May 15, 2024

richfitz May 15, 2024

plietar May 16, 2024

plietar May 15, 2024

richfitz May 15, 2024

plietar May 23, 2024

richfitz left a comment

richfitz May 15, 2024

richfitz May 15, 2024

richfitz May 15, 2024

plietar May 23, 2024

richfitz May 15, 2024

plietar May 16, 2024

plietar May 16, 2024 •

edited

Loading

richfitz May 15, 2024

richfitz May 15, 2024

plietar May 23, 2024

richfitz May 23, 2024

codecov bot commented Jun 12, 2024 •

edited

Loading

		self.fail(f"{value!r} is not a valid numerical value", param, ctx)


		# Almost all sub-commands accept a --root option, so it would make sense for it

Add an orderly command line tool. #53

Are you sure you want to change the base?

Add an orderly command line tool. #53

Conversation

plietar commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richfitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plietar May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 12, 2024 • edited Loading

Codecov Report

plietar May 16, 2024 •

edited

Loading

codecov bot commented Jun 12, 2024 •

edited

Loading