Adjust deps to avoid incompatible versions of s3fs and aiobotocore #134
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
Affects fresh installs of nextstrain-cli with the latest aiobotocore version, 2.0.0. The latest aiobotocore, 2.0.0, does not fulfill the dep declarations of the latest s3fs, 2021.10.1. As a result, pip fulfills our s3fs dep with 2021.7.0 instead, which while declared to be compatible with aiobotocore>=1.4.1 is not actually compatible in practice. The result is a runtime AttributeError on AioSession during upload to S3 in `nextstrain build --aws-batch`. When our previous deps on s3fs and aiobotocore[boto3] were introduced, s3fs did not yet support its own [boto3] extra. It does now, which means we can let it declare the aiobotocore[boto3] dep itself and not end up with conflicting dep declarations. This results in fresh installs using an older version of aiobotocore (1.4.x), but that's what we want since it works. Good to get the transitive dep out of our declarations too. Still a mess. Dependencies are hard. Resolves #133.
|
Does this change also require updating |
|
Yeah, doing so locally but got distracted by this meeting. ;-) |
tsibley
force-pushed
the
s3fs-aiobotocore-dep-resolution
branch
from
3155219 to
2e5e189
Compare
Between issues with our custom theme (nextstrain-sphinx-theme) going stale and Pipenv locking (which was the source of locking here), I think this ended up causing more trouble than it potentially saved. This may result in RTD build failures we wouldn't see otherwise, but maybe better to know about those sooner than later anyway? We can always start locking again (maybe with the more minimal pip-compile instead of pipenv) if this turns out to be a bad decision. For comparison, this is what we do for Augur's RTD build, and it seems to be ok.
Pipenv was nice at first for being an "all inclusive" tool, but with time (mostly of it spent in Seattle Flu Study projects) it's shown its warts with dependency-updating behaviour that's hard to reason about and slowness or outright failure to resolve dependencies correctly. In the latest indignity after today's change "Adjust deps to avoid incompatible versions of s3fs and aiobotocore" (e9a9aa6), Pipenv was failing to correctly resolve s3fs, aiobotocore, and boto3, "successfully" producing: - a lock file that didn't include boto3 - yet a venv that had boto3 installed - which when snapshot with `pip freeze` produced a requirements file uninstallable by modern pip. I believe this is because Pipenv vendors an old version of pip and uses an entirely different dependency resolution algorithm, but I didn't care to debug it. A plain venv is easier and more predictable to manage with that familiar tool, pip. It's also how our users will install nextstrain-cli, so it's good to live in that same world. This change means our dev envs no longer used locked deps… but that seems fine. It's how CI tests have been forever. If we find we want locking in the future, we can use the much more minimal pip-compile to generate a dev-requirements.txt.
A new maintainer took over and cut a 0.3.0 release that included the previously-unreleased changes we needed. This avoids needing to install an unreleased version from a GitHub URL.
tsibley
force-pushed
the
s3fs-aiobotocore-dep-resolution
branch
from
2e5e189 to
554ec8d
Compare
|
@joverlee521 Oh man, that was a journey I didn't want to take, but this PR should now be passing all CI. orz |
joverlee521
approved these changes
Nov 3, 2021
There was a problem hiding this comment.
Thank you for working through this @tsibley
Pipenv really hasn't helped with our dependency hell (in SFS or here) 🙈
tsibley
added a commit
that referenced
this pull request
Mar 1, 2023
Job submissions which triggered the auto-registration of a new job
definition (i.e. because a new image was needed) were failing with
botocore ≥1.28.0 (released 24 Oct 2022), because a new field that is
invalid for registering new job definitions started being returned with
the existing job definition used as a base. This resulted in errors
from `nextstrain build` like:
Submitting job
Parameter validation failed:
Unknown parameter in input: "containerOrchestrationType", must be one of: jobDefinitionName, type, parameters, schedulingPriority, containerProperties, nodeProperties, retryStrategy, propagateTags, timeout, tags, platformCapabilities, eksProperties
Job submission failed!
So far this bug only manifests when installing Nextstrain CLI from the
Bioconda package (and potentially only with Mamba's dependency solver)
because of interactions between s3fs, botocore, and aiobotocore. With
the Bioconda package (and Mamba), the dependency solution uses a new
botocore and old s3fs, which doesn't use aiobotocore and thus doesn't pin
an older botocore.¹ An old s3fs is allowed because the Bioconda package
doesn't specify a lower bound.
In contrast, our Python package installed by Pip _does_ specify a lower
bound for s3fs², which then requires aiobotocore, which then requires an
older botocore where we don't encounter this bug.
A better approach than this hardcoded list of fields will come in the
following commit.
¹ aiobotocore is a giant monkeypatch of botocore, and thus has very
narrow version bounds on botocore.
² See <https://github.com/nextstrain/cli/blob/e9f11e60/setup.py#L100-L123>
for why.
Related-to: <#108>
Related-to: <#134>
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.
Affects fresh installs of nextstrain-cli with the latest aiobotocore
version, 2.0.0.
The latest aiobotocore, 2.0.0, does not fulfill the dep declarations of
the latest s3fs, 2021.10.1. As a result, pip fulfills our s3fs dep with
2021.7.0 instead, which while declared to be compatible with
aiobotocore>=1.4.1 is not actually compatible in practice. The result
is a runtime AttributeError on AioSession during upload to S3 in
nextstrain build --aws-batch.When our previous deps on s3fs and aiobotocore[boto3] were introduced,
s3fs did not yet support its own [boto3] extra. It does now, which
means we can let it declare the aiobotocore[boto3] dep itself and not
end up with conflicting dep declarations. This results in fresh
installs using an older version of aiobotocore (1.4.x), but that's what
we want since it works. Good to get the transitive dep out of our
declarations too.
Still a mess. Dependencies are hard.
Resolves #133.