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

Grab bag of bug fixes and improvements #419

Open
wants to merge 13 commits into
base: master
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
3 changes: 3 additions & 0 deletions .cramrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[cram]
shell = bash
indent = 4
6 changes: 3 additions & 3 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@ environment*
/dist/
/nextstrain_cli.egg-info/

# mypy cache
/.mypy_cache/

# OS generated files #
######################
.DS_Store
Expand All @@ -23,3 +20,6 @@ environment*
Icon?
ehthumbs.db
Thumbs.db

# cram test run outputs
/tests/*.cram.err
62 changes: 62 additions & 0 deletions CHANGES.md
Copy link
Member

Choose a reason for hiding this comment

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

Extending overlay support to AWS Batch has been very low priority and something I thought was unlikely to ever happen.

I've wanted this! Awesome that it's happening

One caveat compared to overlays in runtimes with the concept of volume mounts (Docker, Singularity) is that any files in the image that do not exist in the overlaid files will remain present since nothing removes them. This is potentially problematic and will be annoying if run into but most of the time should be a non-issue. It is also solvable if we care to exert the effort and extra code to do so. I don't right now.

This'll be a nightmare to debug if it ever occurs. If I understand the code (I don't spend much time in this codebase) the behaviour of --augur is going to differ if it's run locally in docker (which swaps the entire augur out) vs aws-batch (which overlays any zipped up ../augur contents), right? Since --augur is already special cased I think it'd be sensible to remove the (containers original) augur directory if we see ../augur in the zip file.

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've wanted this! Awesome that it's happening

\o/

This'll be a nightmare to debug if it ever occurs. If I understand […]

Your understanding is correct. I think "nightmare" is overstating it—a stack trace, for example, will implicate a file that shouldn't be there—but yes, it'd certainly be aggravating.

Note that --augur is just one example of these development overlays, although probably the most useful one. There's also --auspice, --fauna, and (ha) --sacra. (I've personally used --augur and --auspice about evenly.)

Additional handling of these overlays in the AWS Batch entrypoint is possible, like I noted, but I don't really want to spend more time on those bits now. The overlay options have other caveats that present tripping hazards too, e.g. for Augur, it's just source code that's overlaid, not dependencies; and for Auspice you need to make sure to build it locally first. So a bit of "here be dragons" isn't unprecedented, and these are (AFAIK) rarely-used options.

Copy link
Member

Choose a reason for hiding this comment

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

Well, if anything comes from this discussion we should at least remove --sacra

Copy link
Member

Choose a reason for hiding this comment

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

As a little background, it's very very rare I run a workflow using the release version of augur + the master/main repo branch, as that's what we have the automated stuff for in my eyes. Almost every time I'm running workflows it's because i've modified the workflow, or modified augur, or (surprisingly often) both. If we can get nextstrain run ... working as I hope so I can farm out a job to AWS with my modified augur code & modified repo and then download the assets as a separate siloed working directory I'd save a whole lot of build time and my laptop would be happier. Is #407 at the point where this can be done / tested out?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good reminder that although I often focus on the expected/anticipated external users of nextstrain run, we have internal uses for it that have different focuses. Internally we care mainly about the workdir isolation and mechanics in combination with offloading the work to AWS Batch, not the pathogen source/version management.

You can totally test out #407 for your purposes!

There is not currently a supported way to use nextstrain setup to make an "editable install" (to use Python/Pip's lingo) of a self-managed local pathogen repo. It's on the list, but I wasn't focusing on it at first. Sounds like it should be soon/next up. In the meantime, a workaround is to do something like this:

$ ln -sv ~/nextstrain/measles ~/.nextstrain/pathogens/measles/local="$(echo -n local | base32)"
$ nextstrain run measles@local …

Copy link
Member

Choose a reason for hiding this comment

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

This is a good reminder that although I often focus on the expected/anticipated external users

Yeah, this'll always be a tension in the group - imaging a "happy path" for "most people" vs the way some of us run things. For this work I don't mind symlinking things.

P.S. base32 not available on MacOS out of the box.

Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,33 @@ development source code and as such may not be routinely kept up to date.

# __NEXT__

## Improvements

* `nextstrain version` now reports itself as "Nextstrain CLI" (instead of
"nextstrain.cli") and indicates if it is a "standalone" installation
(self-contained, bundles Python) or not.
([#419][])

* Colorized and bolded output is disabled when stdout is not attached to a
terminal (e.g. is redirected to a file, piped to another program, etc.) or
when the [`NO_COLOR` environment variable](https://no-color.org) is set to a
non-empty value.
([#419][])

* The readability of `--help` output is improved by the addition of blank lines
between argument/option descriptions.
([#419][])

* AWS Batch builds now support development overlays such as [`--augur`][] and
[`--auspice`][]. To use this functionality, you'll need at least
`nextstrain/base:build-20250304T041009Z` or newer of the Nextstrain Docker
runtime image. Compatibility of the runtime image is checked automatically
when overlays are used with AWS Batch.
([#419][])

[`--augur`]: https://docs.nextstrain.org/projects/cli/en/__NEXT__/commands/build/#cmdoption-nextstrain-build-augur
[`--auspice`]: https://docs.nextstrain.org/projects/cli/en/__NEXT__/commands/build/#cmdoption-nextstrain-build-auspice

## Bug fixes

* Fixed a rare but possible error case in `nextstrain view` and `nextstrain
Expand All @@ -29,6 +56,41 @@ development source code and as such may not be routinely kept up to date.
in a browser (if any).
([#417](https://github.com/nextstrain/cli/pull/417))

* `nextstrain login` now correctly handles OAuth 2.0 authorization endpoint
URLs (which are obtained automatically from OpenID Connect 1.0 metadata
discovery) with existing query parameters in them. This bug likely effected
approximately no one; logging into nextstrain.org was unaffected and all
known users of non-nextstrain.org remotes were also unaffected.
([#419][])

* File timestamps for results files downloaded from an AWS Batch build are now
correctly restored even if the expected extraction path differs from the
actual extraction path due to ZIP security precautions. This bug likely
affected approximately no one.
([#419][])

## Development

* Goodbye, Mypy! We now use only Pyright for type checking. Both type
checkers used to contribute to our development in their own way, but over
time Pyright's become more sophisticated and Mypy's required more workarounds
to appease it than issues it caught. So long, and thanks for all the ~fish~
type checks!
([#419][])

* [Cram](https://bitheap.org/cram/) test files are now supported, with one
example file for now.
([#419][])

* `NEXTSTRAIN_HOME` is now set for tests so as to avoid interference with the
local user's personal config and data.
([#419][])

* Running tests no longer opens a browser. \o/
([#419][])

[#419]: https://github.com/nextstrain/cli/pull/419


# 8.5.4 (1 November 2024)

Expand Down
3 changes: 2 additions & 1 deletion devel/pytest
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ set -euo pipefail
cd "$(dirname "$0")/.."

set -x
NEXTSTRAIN_RST_STRICT=1 pytest "$@"
source tests/env
exec pytest "$@"
34 changes: 21 additions & 13 deletions doc/development.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@ Now test that you can run:

The development environment includes our dev tools (described below):

pytest # runs doctests as well as mypy and flake8
mypy -p nextstrain.cli
pytest # runs doctests as well as cram, pyright, and flake8
cram tests/*.cram
pyright
flake8
make -C doc livehtml

Expand Down Expand Up @@ -138,7 +139,21 @@ Tests are run with [pytest](https://pytest.org). To run everything, use:

./devel/pytest

This includes the type annotation and static analysis checks described below.
This includes Cram and the type annotation and static analysis checks described
below.

## Cram

Tests for the command-line interface use [Cram](https://bitheap.org/cram/) and
are written in `tests/*.cram` files. You can run individual test files, e.g.:

$ cram -v tests/version.cram
tests/version.cram: passed
# Ran 1 tests, 0 skipped, 0 failed.

Cram tests should always start by sourcing `tests/env` so that they run
identically regardless of if they're run directly with `cram` or via
`./devel/pytest`.

## Type annotations and static analysis

Expand All @@ -147,21 +162,15 @@ catch errors earlier and be explicit about the interfaces expected and
provided. Annotation pairs well with the functional approach taken by the
package.

During development you can run static type checks using [mypy][]:

$ mypy nextstrain
# No output is good!

and [pyright][]:
During development you can run static type checks using [pyright][]:

$ npx pyright
...
Found 40 source files
0 errors, 0 warnings, 0 infos
Completed in 2sec

There are also many [editor integrations for mypy][], and Pyright is integrated
into VS Code's Python support.
There are also many [editor integrations for Pyright][].

The [`typing_extensions`][] module should be used for features not yet available
in the the standard `typings` module of supported Python versions.
Expand Down Expand Up @@ -210,9 +219,8 @@ which may be helpful during development; see `--help` for details.
[source distributions]: https://packaging.python.org/en/latest/glossary/#term-Source-Distribution
[built distributions]: https://packaging.python.org/en/latest/glossary/#term-Built-Distribution
[type annotations]: https://www.python.org/dev/peps/pep-0484/
[mypy]: http://mypy-lang.org/
[pyright]: https://github.com/microsoft/pyright
[editor integrations for mypy]: https://github.com/python/mypy#integrations
[editor integrations for Pyright]: https://microsoft.github.io/pyright/#/installation
[`typing_extensions`]: https://pypi.org/project/typing-extensions
[Flake8]: https://flake8.pycqa.org
[post-release version]: https://peps.python.org/pep-0440/#post-releases
Expand Down
50 changes: 0 additions & 50 deletions mypy.ini

This file was deleted.

4 changes: 2 additions & 2 deletions nextstrain/cli/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ def main():
# rest of the codebase. Avoids needing to instruct folks to set
# PYTHONIOENCODING=UTF-8 or use Python's UTF-8 mode (-X utf8 or
# PYTHONUTF8=1).
reconfigure_stdio(sys.stdout) # type: ignore[arg-type]
reconfigure_stdio(sys.stderr) # type: ignore[arg-type]
reconfigure_stdio(sys.stdout) # pyright: ignore[reportArgumentType]
reconfigure_stdio(sys.stderr) # pyright: ignore[reportArgumentType]

return cli.run( argv[1:] )

Expand Down
12 changes: 12 additions & 0 deletions nextstrain/cli/argparse.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ def _split_lines(self, text, width):
# Render to rST here so rST gets control over wrapping/line breaks.
return rst_to_text(text, width).splitlines()

# Emit blank lines between arguments for better readability. It might seem
# simpler to override _join_parts() instead, but that's called from two
# places and we only want to join on newlines for one of those places.
def add_arguments(self, actions):
for i, action in enumerate(actions):
if i != 0:
# Use " \n" to avoid a completely empty line (e.g. "\n") for
# the sake ShowBriefHelp.truncate_help()'s heuristic.
self._add_item(str, [" \n"])

self.add_argument(action)


def register_default_command(parser):
"""
Expand Down
4 changes: 2 additions & 2 deletions nextstrain/cli/authn/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def __new__(cls, origin: Origin) -> 'Session':
cls = CognitoSession
else:
cls = OpenIDSession
return super().__new__(cls) # type: ignore
return super().__new__(cls) # pyright: ignore[reportArgumentType]

def authenticate_with_password(self, username: str, password: str) -> None:
raise NotImplementedError
Expand Down Expand Up @@ -392,7 +392,7 @@ def __enter__(self):
# The endpoint URI MAY include an "application/x-www-form-urlencoded"
# formatted […] query component […], which MUST be retained when
# adding additional query parameters.
auth_url = auth_endpoint._replace(query = auth_endpoint.query + query(auth_params))
auth_url = auth_endpoint._replace(query = auth_endpoint.query + (auth_endpoint.query and "&") + query(auth_params))
assert auth_url.scheme == "https"


Expand Down
31 changes: 26 additions & 5 deletions nextstrain/cli/command/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@
from ..argparse import add_extended_help_flags, AppendOverwriteDefault, SKIP_AUTO_DEFAULT_IN_HELP
from ..debug import debug
from ..errors import UsageError, UserError
from ..runner import docker, singularity
from ..util import byte_quantity, runner_name, warn
from ..runner import docker, singularity, aws_batch
from ..util import byte_quantity, runner_name, split_image_name, warn
from ..volume import NamedVolume


Expand Down Expand Up @@ -306,10 +306,31 @@ def assert_overlay_volumes_support(opts):
"""
overlay_volumes = opts.volumes

if overlay_volumes and opts.__runner__ not in {docker, singularity}:
if not overlay_volumes:
return

if opts.__runner__ not in {docker, singularity, aws_batch}:
raise UserError(f"""
The {runner_name(opts.__runner__)} runtime does not support overlays (e.g. of {overlay_volumes[0].name}).
Use the Docker or Singularity runtimes (via --docker or --singularity) if overlays are necessary.
Use the Docker, Singularity, or AWS Batch runtimes (via --docker,
--singularity, or --aws-batch) if overlays are necessary.
""")

if opts.__runner__ is aws_batch and not docker.image_supports(docker.IMAGE_FEATURE.aws_batch_overlays, opts.image):
raise UserError(f"""
The Nextstrain runtime image version in use

{opts.image}

is too old to support overlays (e.g. of {overlay_volumes[0].name}) with AWS Batch.

If overlays are necessary, please update the runtime image to at
least version

{split_image_name(opts.image)[0]}:{docker.IMAGE_FEATURE.aws_batch_overlays.value}

using `nextstrain update docker`. Alternatively, use the Docker or
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing nextstrain update docker for the aws_batch runner reminded me of #328 😅

Maybe include --image <version> in the error message as an alternative.

Singularity runtime (via --docker or --singularity) instead.
""")


Expand Down Expand Up @@ -395,7 +416,7 @@ def pathogen_volumes(directory: Path) -> Tuple[NamedVolume, NamedVolume]:
debug(f"Using {working_volume.src} as working ({working_volume.name}) volume")

assert build_volume.src <= working_volume.src
assert docker.mount_point(build_volume) <= docker.mount_point(working_volume) # type: ignore[attr-defined] # for mypy
assert docker.mount_point(build_volume) <= docker.mount_point(working_volume)

return build_volume, working_volume

Expand Down
4 changes: 2 additions & 2 deletions nextstrain/cli/command/shell.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def run(opts):
# will always be small so some special-casing seems ok.
# -trs, 5 Jan 2023 (updated from 25 Sept 2018)
for volume in opts.volumes:
print(" %s is from %s" % (docker.mount_point(volume), volume.src.resolve(strict = True))) # type: ignore
print(" %s is from %s" % (docker.mount_point(volume), volume.src.resolve(strict = True)))

print()

Expand Down Expand Up @@ -95,7 +95,7 @@ def run(opts):
opts.volumes.append(NamedVolume("bashrc", bashrc, dir = False, writable = False))

history_volume = NamedVolume("bash_history", history_file, dir = False)
history_file = docker.mount_point(history_volume) # type: ignore[attr-defined] # for mypy
history_file = docker.mount_point(history_volume)
opts.volumes.append(history_volume)

extra_env = {
Expand Down
5 changes: 2 additions & 3 deletions nextstrain/cli/command/version.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@
import sys
from textwrap import indent
from ..__version__ import __version__
from .. import __package__ as __top_package__
from ..runner import all_runners, default_runner
from ..util import runner_name
from ..util import runner_name, standalone_installation

def register_parser(subparser):
parser = subparser.add_parser("version", help = "Show version information")
Expand All @@ -21,7 +20,7 @@ def register_parser(subparser):


def run(opts):
print(__top_package__, __version__)
print("Nextstrain CLI", __version__, "(standalone)" if standalone_installation() else "")

if opts.verbose:
print()
Expand Down
4 changes: 2 additions & 2 deletions nextstrain/cli/command/view.py
Original file line number Diff line number Diff line change
Expand Up @@ -404,8 +404,8 @@ def resolve(host: str, port: str) -> Tuple[str, int]:
ip4 = [a for a in addrs if a.family is AF_INET]
ip6 = [a for a in addrs if a.family is AF_INET6]

return ((ip4[0].sockaddr[0], ip4[0].sockaddr[1]) if ip4 # type: ignore
else (ip6[0].sockaddr[0], ip6[0].sockaddr[1]) if ip6 # type: ignore
return ((ip4[0].sockaddr[0], ip4[0].sockaddr[1]) if ip4 # pyright: ignore[reportReturnType]
else (ip6[0].sockaddr[0], ip6[0].sockaddr[1]) if ip6
else (str(host), int(port)))


Expand Down
4 changes: 4 additions & 0 deletions nextstrain/cli/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ class InternalError(NextstrainCliError):
pass

class UserError(NextstrainCliError):
"""
Error intended for display to the user, e.g. an error aiming to be clear,
friendly, and, if possible, actionable.
"""
def __init__(self, message, *args, **kwargs):
# Remove leading newlines, trailing whitespace, and then indentation
# to better support nicely-formatted """multi-line strings""".
Expand Down
2 changes: 1 addition & 1 deletion nextstrain/cli/gzip.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ def __init__(self, stream: BinaryIO):
def writable(self):
return True

def write(self, data: bytes): # type: ignore[override]
def write(self, data: bytes): # pyright: ignore[reportIncompatibleMethodOverride]
assert self.stream and self.__gunzip
return self.stream.write(self.__gunzip.decompress(data))

Expand Down
Loading
Loading