Add a proxy layer for extras #3100
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
|
I like where this is going |
|
I think the ideas here are sound but need a packse test and more documentation around the concepts. |
charliermarsh
force-pushed
the
charlie/extras
branch
from
6ee5ce6
to
b6e294a
Compare
charliermarsh
force-pushed
the
charlie/ex-test
branch
from
ce6d1b9
to
59e9f57
Compare
charliermarsh
force-pushed
the
charlie/extras
branch
2 times, most recently
from
647afcf
to
e0ea9f6
Compare
zanieb
reviewed
Apr 17, 2024
charliermarsh
force-pushed
the
charlie/extras
branch
from
e0ea9f6
to
54d1ead
Compare
BurntSushi
approved these changes
Apr 18, 2024
| PubGrubPackage::Package(package_name.clone(), Some(extra.clone()), url.clone()), | ||
| Range::singleton(version.clone()), | ||
| ), | ||
| ])), |
|
This looks like a clever lowering. I'd love to talk it over to see if it helps with a similar problem with "features" in cargo. I was planing to use pubgrub-rs/pubgrub#124 for the "features" problem, and would love to compare. |
charliermarsh
force-pushed
the
charlie/ex-test
branch
from
59e9f57
to
1113ac1
Compare
charliermarsh
added a commit
that referenced
this pull request
Apr 19, 2024
## Summary This PR adds a test that currently leads to an error, but should successfully resolve as of #3100. The core idea is that if we have a pinned package, we shouldn't try to build other versions of that package if we have an unconstrained variant with an extra.
charliermarsh
force-pushed
the
charlie/extras
branch
from
54d1ead
to
31eb597
Compare
charliermarsh
added a commit
that referenced
this pull request
Jun 7, 2024
## Summary This PR adds a lowering similar to that seen in #3100, but this time, for markers. Like `PubGrubPackageInner::Extra`, we now have `PubGrubPackageInner::Marker`. The dependencies of the `Marker` are `PubGrubPackageInner::Package` with and without the marker. As an example of why this is useful: assume we have `urllib3>=1.22.0` as a direct dependency. Later, we see `urllib3 ; python_version > '3.7'` as a transitive dependency. As-is, we might (for some reason) pick a very old version of `urllib3` to satisfy `urllib3 ; python_version > '3.7'`, then attempt to fetch its dependencies, which could even involve building a very old version of `urllib3 ; python_version > '3.7'`. Once we fetch the dependencies, we would see that `urllib3` at the same version is _also_ a dependency (because we tack it on). In the new scheme though, as soon as we "choose" the very old version of `urllib3 ; python_version > '3.7'`, we'd then see that `urllib3` (the base package) is also a dependency; so we see a conflict before we even fetch the dependencies of the old variant. With this, I can successfully resolve the case in #4099. Closes #4099.
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.
Given requirements like:
The resolver will (on
main) add a dependency on Black, and then try to use the most recent version of Black to satisfyblack[colorama]. For sake of example, assumeblack==24.0.0is the most recent version. Once the selects this most recent version, it'll fetch the metadata, then return the dependencies forblack==24.0.0with thecoloramaextra enabled. Finally, it will tack onblack==24.0.0(a dependency on the base package). The resolver will then detect a conflict betweenblack==23.1.0andblack==24.0.0, and throw outblack[colorama]==24.0.0, trying to next most-recent version.This is both wasteful and can cause problems, since we're fetching metadata for versions that will never satisfy the resolver. In the
apache-airflow[all]case, I also ran into an issue whereby we were attempting to build very old versions ofapache-airflowdue toapache-airflow[pandas], which in turn led to resolution failures.The solution proposed here is that we create a new proxy package with exactly two dependencies: one on
blackand one ofblack[colorama]. Both of these packages must be at the same version as the proxy package, so the resolver knows much earlier that (in the above example) the extra variant must match23.1.0.