Skip to content
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

Add an orderly command line tool. #53

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ classifiers = [
"Programming Language :: Python :: Implementation :: PyPy",
]
dependencies = [
"click",
"dataclasses-json",
"importlib_resources",
"jsonschema",
Expand All @@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@plietar plietar May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


[tool.hatch.version]
path = "src/outpack/__about__.py"

Expand Down
4 changes: 4 additions & 0 deletions src/orderly/__main__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import orderly.cli

Check warning on line 1 in src/orderly/__main__.py

View check run for this annotation

Codecov / codecov/patch

src/orderly/__main__.py#L1

Added line #L1 was not covered by tests

if __name__ == "__main__":
orderly.cli.cli()
224 changes: 224 additions & 0 deletions src/orderly/cli/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
import sys
from collections import Counter
from urllib.parse import urlparse

import click

import outpack.search
from orderly.cli.options import (
NumericalParamType,
PropagatingGroup,
with_root,
with_search_options,
)
from orderly.run import orderly_run
from outpack.init import outpack_init
from outpack.location import (
outpack_location_add,
outpack_location_list,
outpack_location_remove,
outpack_location_rename,
)
from outpack.util import format_list


@click.group(cls=PropagatingGroup)
@with_root(propagate=True)
def cli():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""Run 'orderly <command> --help' to see the options available for each subcommand."""
pass


@cli.command()
@click.argument("path")
@click.option(
"--archive",
help="Path to the archive in which packets will be stored.",
type=click.Path(),
)
@click.option(
"--use-file-store",
is_flag=True,
help="Store packets in a content-addressed store.",
)
@click.option(
"--require-complete-tree",
is_flag=True,
help=(
"Require a complete tree. Whenever packets are pulled from remote"
" locations, orderly will ensure all of that packet's dependencies are"
" pulled as well."
),
)
def init(path, archive, use_file_store, require_complete_tree):
"""Initialize at new orderly repository at the specified path."""
kwargs = {}
if archive is not None:
kwargs["path_archive"] = archive
elif use_file_store:
kwargs["path_archive"] = None
else:
# In this case we let outpack_init set the default value for the archive
# path, ie. `archive`.
pass

outpack_init(
path,
use_file_store=use_file_store,
require_complete_tree=require_complete_tree,
**kwargs,
)


@cli.command()
@click.argument("name")
@click.option(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"-p",
"--parameter",
nargs=2,
type=(str, str),
multiple=True,
help="Pass a string parameter to the report",
metavar="NAME VALUE",
)
@click.option(
"-n",
"--numeric-parameter",
nargs=2,
type=(str, NumericalParamType()),
multiple=True,
help="Pass a numerical parameter to the report",
metavar="NAME VALUE",
)
@click.option(
"-b",
"--bool-parameter",
nargs=2,
type=(str, bool),
multiple=True,
help="Pass a boolean parameter to the report",
metavar="NAME VALUE",
)
@with_search_options
@with_root
def run(
name,
root,
parameter,
numeric_parameter,
bool_parameter,
search_options,
):
"""
Run a report.

Parameters to the report may be specified using the -p/-n/-b options,
depending on the parameter's type. These options may be specified multiple
times.
"""
all_parameters = parameter + numeric_parameter + bool_parameter
counts = Counter(key for (key, _) in all_parameters)
duplicates = [key for key, n in counts.items() if n > 1]
if duplicates:
click.echo(
f"Parameters were specified multiple times: {format_list(duplicates)}",
err=True,
)
sys.exit(1)

id = orderly_run(
name,
root=root,
parameters=dict(all_parameters),
search_options=search_options,
)
click.echo(id)


@cli.command()
@click.argument("query")
@with_search_options
@with_root
def search(query, root, search_options):
"""
Search existing packets by query.

By default, the search will only return packets present in the local
repository. The --allow-remote option allows packets that are known locally
but only present remotely to also be returned.
"""
packets = outpack.search.search(query, root=root, options=search_options)
if packets:
for id in packets:
click.echo(id)
else:
click.echo("No packets matching the query were found", err=True)
sys.exit(1)


@cli.group()
@with_root(propagate=True)
def location():
"""
Manage remote locations.

Run 'orderly location <command> --help' to see the options available for each subcommand.
"""
pass


@location.command("list")
@with_root
def location_list(root):
"""List configured remote locations."""
locations = outpack_location_list(root=root)
for name in locations:
click.echo(name)


@location.command("rename")
@click.argument("old")
@click.argument("new")
@with_root
def location_rename(root, old, new):
"""Rename a remote location."""
outpack_location_rename(old, new, root=root)


@location.command("remove")
@click.argument("name")
@with_root
def location_remove(root, name):
"""Remove a remote location."""
outpack_location_remove(name, root=root)


@location.command("add")
@click.argument("name")
@click.argument("location", metavar="(PATH | URL)")
@with_root
def location_add(root, name, location):
"""
Add a new remote location.

The location may be specified as a path, for outpack repositories that are
directly accesible through the file system, or as URLs. Currently only SSH
URLs are supported.
"""
# On windows it could be slightly ambiguous whether "C://foo" is a URL or a
# file path, and we always interpret that as a URL. Users can easily
# disambiguate by using either backslashes or using a single forward slash.
if "://" not in location:
outpack_location_add(name, "path", {"path": location}, root=root)
else:
parts = urlparse(location)

if parts.scheme in {"ssh"}:
outpack_location_add(
name, parts.scheme, {"url": location}, root=root
)
else:
click.echo(
f"Unsupported location protocol: '{parts.scheme}'", err=True
)
sys.exit(1)
131 changes: 131 additions & 0 deletions src/orderly/cli/options.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
import functools

import click

from outpack.search_options import SearchOptions


class NumericalParamType(click.ParamType):
"""
A type for numerical parameters.

It automatically deduces whether the parameter should be interpreted as an
int or a float.
"""

name = "number"

def convert(self, value, param, ctx):
if isinstance(value, float):
return value

Check warning on line 20 in src/orderly/cli/options.py

View check run for this annotation

Codecov / codecov/patch

src/orderly/cli/options.py#L20

Added line #L20 was not covered by tests

try:
return int(value)
except ValueError:
pass

try:
return float(value)
except ValueError:
self.fail(f"{value!r} is not a valid numerical value", param, ctx)

Check warning on line 30 in src/orderly/cli/options.py

View check run for this annotation

Codecov / codecov/patch

src/orderly/cli/options.py#L29-L30

Added lines #L29 - L30 were not covered by tests


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

# to be accepted early in the command syntax. For instance, we want to accept
# `orderly --root /path run foo`. Normally, with click, for this to be accepted
# we would have to define the option on the top-level command.
#
# However there is at least one command (init) which does not take a --root
# option, which would prevent us from doing this. Additionally, if the option
# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# top-level command with a callback that stores the value in a seperate field
# on the context. When the top-level command dispatches to the sub-command, we
# extract these saved arguments and pass them down.
class PropagatingContext(click.Context):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.propagated_options = []


class PropagatingGroup(click.Group):
context_class = PropagatingContext

def resolve_command(self, ctx, args):

# Rewrite the subcommands arguments to include the propagated options.
#
# args[0] is always the name of the subcommand. We pass that first so
# the superclasses' resolve_command method still works, followed by our
# injected arguments, and finally the rest of the original arguments.
extended_args = [args[0], *ctx.propagated_options, *args[1:]]
return super().resolve_command(ctx, extended_args)

@staticmethod
def propagate(ctx, param, value):
if not isinstance(ctx, PropagatingContext):
msg = "Propagating option on non-propagating group"
raise RuntimeError(msg)

Check warning on line 70 in src/orderly/cli/options.py

View check run for this annotation

Codecov / codecov/patch

src/orderly/cli/options.py#L69-L70

Added lines #L69 - L70 were not covered by tests
if len(param.opts) > 1:
msg = "Options with multiple syntaxes is not supported"
raise NotImplementedError(msg)

Check warning on line 73 in src/orderly/cli/options.py

View check run for this annotation

Codecov / codecov/patch

src/orderly/cli/options.py#L72-L73

Added lines #L72 - L73 were not covered by tests
if value is not None:
ctx.propagated_options += [param.opts[0], value]


PropagatingGroup.group_class = PropagatingGroup


def with_root(f=None, *, propagate=False):
if f is None:
# This happens when the function is called as a decorator factory.
# Return the actual decorator and forward the options.
return lambda f: with_root(f, propagate=propagate)

return click.option(
"--root",
help=(
"Path to the orderly repository. If not specified, the current"
" working directory is used."
),
type=click.Path(),
hidden=propagate,
expose_value=not propagate,
callback=PropagatingGroup.propagate if propagate else None,
)(f)


def with_search_options(f):
plietar marked this conversation as resolved.
Show resolved Hide resolved
@click.option(
"--pull-metadata",
is_flag=True,
help="Synchronize metadata from remote locations.",
)
@click.option(
"--allow-remote",
is_flag=True,
help="Search for packets in remote locations.",
)
@click.option(
"--location",
multiple=True,
help=(
"Locations in which packets are searched. This option may be"
" specified multiple times to allow multiple locations. If none are"
" specified, all configured locations are searched."
),
metavar="NAME",
)
@click.pass_context
@functools.wraps(f)
def inner(ctx, allow_remote, pull_metadata, location, *args, **kwargs):
options = SearchOptions(
allow_remote=allow_remote,
pull_metadata=pull_metadata,
location=list(location) if location else None,
)
return ctx.invoke(f, *args, **kwargs, search_options=options)

return inner
Loading
Loading