From e4cfcf75bec0799eb6603c7477178be2ce35a463 Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Thu, 24 Aug 2023 11:43:24 -0700 Subject: [PATCH 1/3] Add UsageError for when we want to print command usage before the error message Normally argparse does this when it detects a usage error, but this lets us handle other cases it doesn't in a similar way. Similar to how we track which command class the args parse to, we track the command parser the args parse to so we can emit appropriate usage information. For consistency with argparse, the exit status of a usage error is 2 instead of 1, but the distinction is unlikely to be actually useful in practice. --- nextstrain/cli/__init__.py | 11 +++++++++-- nextstrain/cli/argparse.py | 13 ++++++++++++- nextstrain/cli/errors.py | 6 ++++++ 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/nextstrain/cli/__init__.py b/nextstrain/cli/__init__.py index 5eb6c64c..5c1b110d 100644 --- a/nextstrain/cli/__init__.py +++ b/nextstrain/cli/__init__.py @@ -19,7 +19,7 @@ from .argparse import HelpFormatter, register_commands, register_default_command from .command import build, view, deploy, remote, shell, update, setup, check_setup, login, logout, whoami, version, init_shell, authorization, debugger from .debug import DEBUGGING -from .errors import NextstrainCliError +from .errors import NextstrainCliError, UsageError from .util import warn from .__version__ import __version__ # noqa: F401 (for re-export) @@ -36,11 +36,18 @@ def run(args): return opts.__command__.run(opts) except NextstrainCliError as error: + exit_status = 1 + if DEBUGGING: traceback.print_exc() else: + if isinstance(error, UsageError): + warn(opts.__parser__.format_usage()) + exit_status = 2 + warn(error) - return 1 + + return exit_status except AssertionError: traceback.print_exc() diff --git a/nextstrain/cli/argparse.py b/nextstrain/cli/argparse.py index 474e223b..36b068e2 100644 --- a/nextstrain/cli/argparse.py +++ b/nextstrain/cli/argparse.py @@ -78,7 +78,18 @@ def register_commands(parser, commands): for cmd in commands: subparser = cmd.register_parser(subparsers) - subparser.set_defaults( __command__ = cmd ) + + # No need to worry about reference cycles of subparser to itself. + # CPython's GC uses automatic reference counting complemented with + # traversal-based reachability detection that can handle cycles.¹ + # Other implementations may not, but we largely don't care about those, + # and taking a big step back, our processes here just don't live long + # enough for the cycle to be a memory issue (plus it's just one cycle, + # not recreated over and over again in a hot loop). + # -trs, 24 August 2023 + # + # ¹ + subparser.set_defaults( __command__ = cmd, __parser__ = subparser ) # Ensure all subparsers format like the top-level parser subparser.formatter_class = parser.formatter_class diff --git a/nextstrain/cli/errors.py b/nextstrain/cli/errors.py index 482da4d0..93f7ec34 100644 --- a/nextstrain/cli/errors.py +++ b/nextstrain/cli/errors.py @@ -18,3 +18,9 @@ def __init__(self, message, *args, **kwargs): formatted_message = dedent(message.lstrip("\n").rstrip()).format(*args, **kwargs) super().__init__("Error: " + formatted_message) + +class UsageError(UserError): + """ + Prints brief command usage before the error message. + """ + pass From 5a6b303a71e819d2d6e9a3f3cc87b6022e4b8513 Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Thu, 24 Aug 2023 11:50:20 -0700 Subject: [PATCH 2/3] build: Make optional when --attach + --no-download are used This allows usages which just want to check job status/logs to stop passing in a meaningless/unused directory. --- doc/commands/build.rst | 2 +- nextstrain/cli/command/build.py | 27 ++++++++++++++++++--- nextstrain/cli/runner/aws_batch/__init__.py | 22 ++++++++++------- nextstrain/cli/volume.py | 2 +- 4 files changed, 39 insertions(+), 14 deletions(-) diff --git a/doc/commands/build.rst b/doc/commands/build.rst index 0a217bbe..68ab409e 100644 --- a/doc/commands/build.rst +++ b/doc/commands/build.rst @@ -39,7 +39,7 @@ positional arguments .. option:: - Path to pathogen build directory + Path to pathogen build directory. Required, except when the AWS Batch runtime is in use and both --attach and --no-download are given. .. option:: ... diff --git a/nextstrain/cli/command/build.py b/nextstrain/cli/command/build.py index 57483dfc..820031fd 100644 --- a/nextstrain/cli/command/build.py +++ b/nextstrain/cli/command/build.py @@ -20,6 +20,7 @@ from textwrap import dedent from .. import runner from ..argparse import add_extended_help_flags, AppendOverwriteDefault, SKIP_AUTO_DEFAULT_IN_HELP +from ..errors import UsageError, UserError from ..util import byte_quantity, warn from ..volume import store_volume @@ -118,9 +119,12 @@ def register_parser(subparser): # Positional parameters parser.add_argument( "directory", - help = "Path to pathogen build directory", + help = "Path to pathogen build directory. " + "Required, except when the AWS Batch runtime is in use and both --attach and --no-download are given. " + f"{SKIP_AUTO_DEFAULT_IN_HELP}", metavar = "", - action = store_volume("build")) + action = store_volume("build"), + nargs = "?") # Register runner flags and arguments runner.register_runners(parser, exec = ["snakemake", "--printshellcmds", ...]) @@ -129,8 +133,25 @@ def register_parser(subparser): def run(opts): + # We must check this before the conditions under which opts.build is + # optional because otherwise we could pass a missing build dir to a runner + # which ignores opts.attach. + if (opts.attach or opts.detach) and opts.__runner__ is not runner.aws_batch: + raise UserError(f""" + The --attach/--detach options are only supported when using the AWS + Batch runtime. Did you forget to specify --aws-batch? + """) + # Ensure our build dir exists - if not opts.build.src.is_dir(): + if opts.build is None: + if opts.attach and not opts.download: + # Don't require a build directory with --attach + --no-download. + # User just wants to check status and/or logs. + pass + else: + raise UsageError("Path to a pathogen build is required.") + + elif not opts.build.src.is_dir(): warn("Error: Build path \"%s\" does not exist or is not a directory." % opts.build.src) if not opts.build.src.is_absolute(): diff --git a/nextstrain/cli/runner/aws_batch/__init__.py b/nextstrain/cli/runner/aws_batch/__init__.py index 8be04681..f9fa3adc 100644 --- a/nextstrain/cli/runner/aws_batch/__init__.py +++ b/nextstrain/cli/runner/aws_batch/__init__.py @@ -85,7 +85,7 @@ from sys import exit from textwrap import dedent from time import sleep, time -from typing import Iterable +from typing import Iterable, Optional from uuid import uuid4 from ...types import Env, RunnerSetupStatus, RunnerTestResults, RunnerUpdateStatus from ...util import colored, warn @@ -164,13 +164,13 @@ def register_arguments(parser) -> None: def run(opts, argv, working_volume = None, extra_env: Env = {}, cpus: int = None, memory: int = None) -> int: # Unlike other runners, the AWS Bach runner currently *requires* a working - # dir. This is ok as we only provide the AWS Batch runner for commands - # which also require a working dir (e.g. build), whereas other runners also - # work with commands that don't. - # -trs, 28 Feb 2022 - assert working_volume is not None + # dir in most usages. This is ok as we only provide the AWS Batch runner + # for commands which also require a working dir (e.g. build), whereas other + # runners also work with commands that don't. + # -trs, 28 Feb 2022 (updated 24 August 2023) + assert working_volume is not None or (opts.attach and not opts.download) - local_workdir = working_volume.src.resolve(strict = True) + local_workdir = working_volume.src.resolve(strict = True) if working_volume else None if opts.attach: print_stage("Attaching to Nextstrain AWS Batch Job ID:", opts.attach) @@ -191,6 +191,8 @@ def run(opts, argv, working_volume = None, extra_env: Env = {}, cpus: int = None print_stage("Job is %s" % job.status) else: + assert local_workdir is not None + # Generate our own unique run id since we can't know the AWS Batch job id # until we submit it. This run id is used for workdir and run results # storage on S3, in a bucket accessible to both Batch jobs and CLI users. @@ -357,6 +359,8 @@ def handler(sig, frame): # Download results if we didn't stop the job early. if opts.download and not stop_sent and not job.stopped: + assert local_workdir is not None + patterns = opts.download if isinstance(opts.download, list) else None if patterns: @@ -378,7 +382,7 @@ def handler(sig, frame): ) -def detach(job: jobs.JobState, local_workdir: Path) -> int: +def detach(job: jobs.JobState, local_workdir: Optional[Path]) -> int: """ Detach from the specified *job* and print a message about how to re-attach (using the *local_workdir*). @@ -393,7 +397,7 @@ def detach(job: jobs.JobState, local_workdir: Path) -> int: "--attach", shlex.quote(job.id), # Preserve the local workdir, which has been resolved to an absolute path - shlex.quote(str(local_workdir)) + shlex.quote(str(local_workdir) if local_workdir else ".") ]) print(dedent(""" diff --git a/nextstrain/cli/volume.py b/nextstrain/cli/volume.py index 74106207..ab0ac248 100644 --- a/nextstrain/cli/volume.py +++ b/nextstrain/cli/volume.py @@ -34,7 +34,7 @@ class store(argparse.Action): def __call__(self, parser, namespace, values, option_strings = None): # Add the new volume to the list of volumes volumes = getattr(namespace, "volumes", []) - new_volume = NamedVolume(volume_name, Path(values)) + new_volume = NamedVolume(volume_name, Path(values)) if values else None setattr(namespace, "volumes", [*volumes, new_volume]) # Allow the new volume to be found by name on the opts object From 7979f6f9855d35c0af8d53ecbf1707c13189583f Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Thu, 24 Aug 2023 12:46:47 -0700 Subject: [PATCH 3/3] CHANGES: Document new optional build directory --- CHANGES.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 41e30662..a8555c88 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -13,6 +13,15 @@ development source code and as such may not be routinely kept up to date. # __NEXT__ +## Improvements + +* build: Providing a path to a pathogen build directory is no longer required + when the AWS Batch runtime is in use (e.g. with `--aws-batch`) and both the + `--attach` and `--no-download` options are given. This allows usages which + just want to check job status or logs to stop providing a meaningless/unused + directory. + ([#305](https://github.com/nextstrain/cli/pull/305)) + # 7.2.0 (17 August 2023)