Managed Conda runtime #218
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
tsibley
force-pushed
the
trs/managed-conda-runtime
branch
from
dc0a6bc to
70b138c
Compare
tsibley
commented
Sep 13, 2022
tsibley
force-pushed
the
trs/managed-conda-runtime
branch
from
70b138c to
30accef
Compare
|
CI failing on Windows due to two errors:
Repushed after fixups. |
tsibley
commented
Sep 14, 2022
tsibley
force-pushed
the
trs/managed-conda-runtime
branch
3 times, most recently
from
c11234a to
4ff7b15
Compare
|
Testing this with: on my M1 MBP. It'd be great to have a list of accepted args for This only shows when you enter something invalid: We can probably get rid of the Seems to work out of the box :) Even on an M1, that's pretty amazing! Well done @tsibley |
|
When trying to run a workflow, I get the following weird error - no idea where this is from. We don't seem to have a |
You're running into the issue with spaces. I'll have a fix soon. Should have put this PR back into draft mode... but thanks for testing it out so far! |
Yes, indeed. Will do!
Those come from the |
tsibley
force-pushed
the
trs/managed-conda-runtime
branch
5 times, most recently
from
eb80ff0 to
5ee825a
Compare
Fixed in latest repush, along with a similar issue for |
tsibley
force-pushed
the
trs/managed-conda-runtime
branch
from
5ee825a to
74f0113
Compare
|
I think this work is ready to be merged. If folks who tested on macOS earlier and helped identify some issues could test again with the latest version in this PR, that'd be great. New folks testing would also be appreciated. Outstanding questions/tasks that would be good to answer definitely/do before this is first released (even if not before this PR is merged):
Other outstanding questions/tasks can (should be) be addressed after merge/release:
|
This isn't such a big deal. It's more common to incorrectly add placeholders to a string that's not already an f-string and forget to add the "f". A few more not-strictly-necessary f-strings in the codebase may help avoid some of that! 🙃
By specifying --rcfile, the user's existing local interactive shell config is ignored. This more closely matches the non-interactive `build` environment and avoids local shell config which may modify PATH, provide aliases, etc.
…and the fallback default isn't what we just set up. This catches a case where the --set-default option probably was intended. Flagged by @jameshadfield during review.
Lets folks see what will be downloaded, where it will go, etc. before they actually do it. A good suggestion by @jameshadfield in review. Diff best viewed with whitespace ignored.
Nice to be crystal clear that what's said to be happening is not _actually_ happening and make this consistent across commands with dry run modes. Providing this indication as an enterable context that doesn't require changing subsequently-called code means that code doesn't have to be muddied with conditional printing of indications. While this wasn't an issue for the existing dry run indications formerly used by the `nextstrain remote` family of commands, it was a greater issue when considering indications for the `nextstrain setup` command. Thus this work was motivated. I used wrapt.ObjectProxy to implement LinePrefixer because a) it'll do a *much* more thorough job of proxying than a basic __getattr__ override and b) wrapt is already in our dep chain anyway. \o/
…mbol This matters when sys.stderr is replaced (perhaps temporarily) by another file object, such as with redirect_stderr() from contextlib or even in our own __main__.py initialization. Such replacement is a common pattern. Doing the module attribute lookup in warn() means it'll be context-dependent, whereas importing the symbol meant we got whatever the value was at import time. Switches to sys.exit() too so we can remove the "from sys import …" statement all together.
A hallmark of shells that we've been lacking! Made a bit easier now with the prior changes to support the managed Conda runtime.
…nd but runnable This catches situations where an executable is installed but doesn't actually work, which, for example, we observed in testing of the managed Conda runtime when a space was in the prefix path. This could also happen other ways, and it's easy enough for us to verify these programs actually run (at least superficially enough for --version to work).
While the per-platform user-specific application data dirs are a nice idea and worked for our (current) standalone installation needs, part of the point of using them was anticipation that we'd also put other app data there, like the managed Conda runtime files. This ideal would keep a clean separation between app data in the platform-intended locations and app config in ~/.nextstrain. However, despite attempted workarounds, the platform-intended location on macOS turned out be unusable for the managed Conda runtime because "~/Library/Application Support" includes spaces.¹ So we ended up using ~/.nextstrain/runtimes for that. Instead of splitting app data across multiple places, let's double down and use ~/.nextstrain for everything. ~/.nextstrain is already used across platforms and is itself bespoke—it really should have been platform-specific dirs like ~/.config or $XDG_CONFIG_HOME—so ISTM there's little point following platform conventions for some things but not others. This commit takes no pains to have the installers consider previous installations into the old paths since the installers are not yet "released" in any sense, e.g. they're not a documented installation method. The only folks who should be impacted are the core team members, and we can manually tidy our machines up as necessary. ¹ #218 (comment)
A single word is simpler and better parallels the name of the Docker runtime, which is a) named after the technology it uses (the "engine") and b) also "managed" but doesn't say so in its name. I'd used "managed Conda" because "Conda" by itself seemed too confusing: we've long recommended that folks use Conda to manually setup an environment suitable for the "native" runner. There's a strong historical association there that we'd be tangling up users in. The simpler "Conda" name does feel like the better name in the long run, though. Our hope is that we can avoid a lot of this confusion by renaming "native" → "ambient" (more accurate anyhow), rephrasing our install docs, and adding an FAQ entry or two. We'll undoubtedly still deal with some. A push for this renaming and votes of confidence were given by @victorlin and @huddlej in Slack.¹ ¹ <https://bedfordlab.slack.com/archives/C01LCTT7JNN/p1664232205090739?thread_ts=1662072691.294419&cid=C01LCTT7JNN>
Broadens the messaging in "setup: Warn the user if there's no default runtime configured…" (171e4ee), again at the good suggestion of @jameshadfield during further review.
…e function Before we were standardizing its command-line arguments, but this now standardizes how it's executed too. That's important now as I want to modify it's execution environment.
…amba This avoids touching the ~/.mamba/ directory in the user's real homedir, which is more than cosmetically important since installed files in user-controlled Conda environments may be linked to the package cache there. We want to be able to invoke `micromamba clean` safely. Note that HOME isn't set when running commands inside this runtime, only during management operations with Micromamba.
Super handy for edit/preview cycles in dev. Replaces a klunkier, Linux-only homegrown alternative.
This was remote only because at the time it pointed to a non-merged branch. The AWS Batch doc page has since been included in the normal Sphinx build.
Many users probably want to use our general Nextstrain installation guide instead.
The biggest impact of this change is on Python 3.7 where we were seeing
CI test failures.¹ Those now pass again. The story is complicated.
Let's start with the facts.
The recent release of importlib-metadata 5.0.0 removed a previously
deprecated entry points API.²
This API is used by flake8.
flake8 4.0.0 onwards declares (more or less):
importlib-metadata <4.3; python_version < "3.8"
…while prior versions, flake8 3.9.2 and earlier, declared:
importlib-metadata; python_version < "3.8"
Note that flake8 3.9.2 and earlier still require in practice the upper
bound on the importlib-metadata version, they just don't express it in
package metadata. (This is what's known as foreshadowing…)
Meanwhile, Sphinx 4.4.0 onwards declares (more or less):
importlib-metadata >=4.4; python_version < '3.10'
Note that on Python 3.7 and earlier, the latest versions of both flake8
and Sphinx have conflicting version requirements for importlib-metadata.
So when Pip on 3.7 goes to install both, it has to resolve the issue by
downgrading either flake8 or Sphinx until it finds a dependency
solution. For whatever reason, it choose to keep the newer versions of
Sphinx and importlib-metadata and downgrade flake8. So it walks back in
flake8 versions until it eventually finds 3.9.2 without the
importlib-metadata version pin. Pip is satisfied, but although Pip
can't know it, flake8 won't work with importlib-metadata 5.0.0.
By pinning flake8 ≥4.0.0, we're hinting to Pip that only those versions
accurately declare their dependencies given the current situation. Pip
ultimately can use this better dependency information solve the
importlib-metadata conflict by walking back to a version of Sphinx
≤4.4.0 (which it turns out, don't use importlib-metadata at all).
The flake8 breakage doesn't arise on Python 3.6 because support for that
Python version was dropped by importlib-metadata ≥4.9.0, so the removed
API in 5.0.0 isn't seen. Without our flake8 pin, Pip still resolves the
conflict by walking flake8 back to 3.9.2 and keeping the latest Sphinx
plus importlib-metadata 4.8.3. While that importlib-metadata version is
declared as unsupported in later version of flake8, it seems to work
fine in practice. ¯\_(ツ)_/¯
On Python ≥3.8, the importlib-metadata conflict doesn't exist,
presumably because flake8 uses the importlib.metadata standard library
instead.
We'll still build our production docs on RTD with the latest version of
Sphinx, because we use Python 3.10 there, and the downgrading of Sphinx
is limited to 3.7.
(If some of this is making your head ring but also ringing some bells,
this isn't the first time importlib-metadata's development choices and
flake8's development choices have had fallout for us.³)
¹ python/importlib_metadata#409 (comment)
² python/importlib_metadata#405
³ "dev: Ignore a deprecation warning generated by flake8's usage of importlib_metadata" (1a04901)
See <ryanfox/sphinx-markdown-tables#38>. With the downgrade of Sphinx on 3.6 and 3.7 caused by "dev: Require flake8 ≥4.0.0" (352b8d7), this incorrect dep declaration was causing issues because Pip thought things were compatible when in reality the docs build didn't work on those Python versions.
tsibley
force-pushed
the
trs/managed-conda-runtime
branch
from
f1bbe00 to
221a2ae
Compare
3 tasks
5 tasks
2 tasks
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.
This uses the Conda/Mamba ecosystem for the runtime environment, but the user doesn't ever have to manage Conda themselves, thus potentially making it the easiest install option when paired with the standalone installer. The Docker runtime is still more isolated and robust, but I think this provides a really nice middle ground between "native" and Docker.
Context from Slack when I first started this work. There's a lot more in this PR because it turns out that while the basics of the runner/runtime itself was pretty straightforward to implement, there was a lot of complexity in how it integrated with the rest of Nextstrain CLI, parts of which previously had the historical luxury of assuming Docker-only. Many improvements were made there, some I'd noted as good-to-do long ago, so that's nice.
Related issue(s)
Related to #217.
Todo
setup --dry-runflag (ref)setupshould warn and offer guidance if no default is configured (ref)setup --helpshould note acceptable<runtime>valuesPATH)Testing
TKTK