Grab bag of bug fixes and improvements #419
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Switch from running both Mypy and Pyright to just Pyright. I can't bear
to keep accommodating Mypy's limitations compared to Pyright.
I only introduced Pyright¹ in addition to (as opposed to replacing)
existing Mypy usage because Pyright had functionality Mypy didn't that I
wanted to use. Mypy at the time also supported some things Pyright
didn't at the time, so I used both instead of switching. Over time
though, Pyright became a lot more sophisticated than Mypy, and most of
Mypy's complaints became false-positives or overly-conservative because
of various limitations/choices. I've made comments (in code, in commit
messages, on GitHub issues, etc.) for a while now that yeah, we probably
should drop Mypy at this point, but it was always low-priority. And I
knew I'd get fed up enough at some point to JFDI, and lo and behold,
here we are!
Type ignore comments which were only necessary for Mypy are removed, and
the remaining ignore comments are converted to Pyright-specific syntax
so they can target specific rules.
Mypy-specific workarounds with cast() and TYPE_CHECKING are removed.
The most recent version of Pyright is required for tests as it contains
a fix² to type inference that without which a cast() is still necessary
in one spot due to type inference changing after removal of
Mypy-specific workarounds.
Reverts the following commits wholesale
- "runner.docker: Use os.getuid()/getgid() when present instead of
conditioning on os.name" (23f618e)
- "dev: Work around some mypy limitation when type checking a lambda"
(31d708f)
- "dev: Ignore for mypy's sake some superclass calls"
(d2fe958)
as part of the changes.
Drops from dev dependencies a few types-* packages that were only
necessary for Mypy.
¹ In "dev: Add (optional) type checking with Pyright to our tests"
(e0faaf5)
² <microsoft/pyright#9988>
Allows for tests that require config and other app data and also isolates tests from the local user's personal config and data.
tsibley
force-pushed
the
trs/grab-bag
branch
4 times, most recently
from
40e4a7a to
b3913e1
Compare
…("nextstrain.cli")
The prose name is more meaningful to users than the internal package
name.
…e. bundles Python) This is a very useful bit of information to know, but previously it was only discernible by knowing what to look for (e.g. the Python executable path) in the --verbose output.
…owser locally The previous approach of late-loading of nextstrain.cli.command.view only worked when running an explicit set of tests/*.py files—pytest's discovery process for doctests and pytest_*() functions means it loads all of nextstrain.cli too early otherwise—and most of the time a new tab or two was popped open because the environment modification was too late. This was annoying. The previous approach also predated our devel/pytest wrapper to reliably set env by insisting on a common entrypoint to tests. With that now present, the easiest and more reliable thing is to move the setting of BROWSER there before pytest is ever exec-ed.
Good manners and standard practice. Almost essential for writing readable Cram tests too.
Recently when working on this codebase I've though it's not clear enough that "UserError" doesn't mean an error the user's committed. It means an error intended for the user to see. Describe it as such for the benefit of other developers.
Improves readability, it seems to me, having spent a bit more time going thru --help recently.
1 task
joverlee521
approved these changes
Mar 7, 2025
genehack
approved these changes
Mar 10, 2025
The query parts of such endpoints were not being correctly joined with additional authentication parameters. Noticed in passing. No impacted users that I know of.
…rdcoded /nextstrain Brings this older code up to consistency with newer code and further centralizes mount_point() as the logic for where named volumes end up.
…not our presumed extraction path. This is a nuanced change: normally the two paths are identical, but zipfile.extract() does sometimes munge extraction paths for security or platform compatibility and thus make the two paths different.
3 tasks
tsibley
added a commit
that referenced
this pull request
Mar 11, 2025
Sacra hasn't been a going concern for a long time, and it's been absent from the runtime image for over 4 years. Spotted in review.¹ ¹ <#419 (comment)>
The latest image, nextstrain/base:build-20250304T041009Z, provides a mechanism in the entrypoint to support bundling of overlays in the workdir ZIP archive by way of upwards-traversing archive member paths.¹ For example, an Augur overlay is bundled into the workdir ZIP archive with member paths starting with ../augur/ and ends up overwriting files in the image's /nextstrain/augur/ since the AWS Batch workdir is always /nextstrain/build/. Extending overlay support to AWS Batch has been very low priority and something I thought was unlikely to ever happen. However, in the course of working on AWS Batch support for `nextstrain run`, it turned out to be easiest/most straightforward/most minimal changes to bundle the pathogen source directory with the working analysis directory in the workdir ZIP archive, i.e. as a "pathogen" overlay. This naturally led to supporting overlays more generally, which I've done here. 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. ¹ <nextstrain/docker-base#241>
…and into the shared runner options. These options are used by all containerized runtimes. Place them below the --image option as a) it made the same journey a while back and b) the overlay options are directly relevant to the image in use. A good suggestion by @joverlee521 in review.¹ ¹ <#419 (comment)> Co-authored-by: Jover Lee <joverlee521@gmail.com>
Sacra hasn't been a going concern for a long time, and it's been absent from the runtime image for over 4 years. Spotted in review.¹ ¹ <#419 (comment)>
|
Ok, a couple of updates pushed for review comments since initial reviews. With those, I think the worst wrinkles are ironed out of this, so once I get the all green I'll merge it unless I hear otherwise. |
This was referenced Mar 11, 2025
tsibley
added a commit
to nextstrain/docker-base
that referenced
this pull request
Mar 13, 2025
…a}/ for overlays
Makes these overlays work the same on AWS Batch as they do in local
containers (Docker or Singularity): the overlays fully replace the
image's copy. This addresses a caveat noted the Nextstrain CLI commit
adding AWS Batch overlay support¹ that was further discussed in review.²
ZIP archive directory members always end in "/", so we test only for the
presence of ../{augur,auspice,fauna}/ in the archive. This works
consistently for workdir archives with overlays produced by Nextstrain
CLI, but it isn't foolproof for archives produced other ways. For
example, it is possible for a ../augur/x/y/z path to be present in the
archive without ../augur/ also being present. In that case, nothing
will be deleted. This could be seen as a bug or a feature.
¹ <nextstrain/cli@99168ac>
² <nextstrain/cli#419 (comment)>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All related of varying degrees to my
nextstrain runand workflows-as-programs work in #407. Split out to be reviewed/merged (and probably released) separately. One big grab bag, as no one seemed to mind.Checklist