-
Notifications
You must be signed in to change notification settings - Fork 615
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
Add support for PyTorch-style local version semantics #2430
Conversation
No need to review yet. I need to work on the tests. |
dcc133d
to
f267e59
Compare
Downloaded 2 packages in [TIME] | ||
Installed 2 packages in [TIME] | ||
+ albatross==1.0.0 | ||
+ bluebird==2.0.0+foo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, not that impressive, but this is the critical test that changes. (This is the PyTorch resolution case.)
b218e59
to
b4625f3
Compare
scripts/scenarios/generate.py
Outdated
expected["satisfiable"] = True | ||
expected["packages"] = [ | ||
{ | ||
"name": "local-greater-than-a", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one is fixable with tricks similar to what we do for prereleases with "min version" thing.
afa8ecb
to
0fe844c
Compare
})?; | ||
// If the specifier is an exact version, and the user requested a local version that's | ||
// more precise than the specifier, use the local version instead. | ||
let version = if let Some(expected) = locals.get(&requirement.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
locals.get
should be free (the hash map short-circuits if it's empty) unless local dependencies were declared.
What's the right order of operations for packse? These tests all pass locally, but I need to publish the updated index for them to pass on CI. |
6790bbe
to
bf08dc5
Compare
use crate::ResolveError; | ||
|
||
#[derive(Debug)] | ||
pub struct PubGrubDependencies(Vec<(PubGrubPackage, Range<Version>)>); | ||
|
||
impl PubGrubDependencies { | ||
/// Generate a set of `PubGrub` dependencies from a set of requirements. | ||
#[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have this lint on? I feel like we just ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably have fewer arguments 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow this is pretty epic. I'll defer to Andrew for the approval, but this feels reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really clever. I don't think I would have thought of changing the dependency specifications themselves. Nice work.
The thing that still doesn't work is: imagine if there were only local versions of torch available. Like, torch @ 2.0.0 didn't exist, but torch @ 2.0.0+cpu did, and torch @ 2.0.0+gpu did, and so on. pip install torch==2.0.0 would arbitrarily choose one one 2.0.0+cpu or 2.0.0+gpu, and that's correct as per PEP 440 (local version segments should be completely ignored on torch==2.0.0). However, uv would fail to identify a compatible version. I'd probably prefer to fix this, although candidly I think our behavior is ok in practice, and it's never been reported as an issue.
I think this is what #1497 is. Although the issue starts with uv pip install torch==2.1.0+cpu --find-links https://download.pytorch.org/whl/torch_stable.html
(which I think is fixed by this PR), they also list uv pip install torch==2.1.0 --index-url https://download.pytorch.org/whl/cpu
(which I think is not fixed by this PR). It's not clear to me whether the latter is something they specifically want to do or not though.
e8aeb19
to
07bae1c
Compare
07bae1c
to
5a319e0
Compare
BTW |
Interesting. It doesn't work for me:
IIRC, it's because there aren't any wheels for x86-64 Linux without local version segments. But there are wheels without local version segments for aarch64 Linux (and macOS IIRC). |
Summary
This PR adds limited support for PEP 440-compatible local version testing. Our behavior is not comprehensively in-line with the spec. However, it does fix by far the biggest practical limitation, and resolves all the issues that've been raised on uv related to local versions without introducing much complexity into the resolver, so it feels like a good tradeoff for me.
I'll summarize the change here, but for more context, see Andrew's write-up in the linked issue.
Local version identifiers are really tricky because of asymmetry.
==1.2.3
should allow1.2.3+foo
, but==1.2.3+foo
should not allow1.2.3
. It's very hard to map them to PubGrub, because PubGrub doesn't think of things in terms of individual specifiers (unlike the PEP 440 spec) -- it only thinks in terms of ranges.Right now, resolving PyTorch and friends fails, because...
torch==2.0.0+cu118
andtorchvision==0.15.1+cu118
.torchvision==0.15.1+cu118
, which includestorch==2.0.0
.torch @ 2.0.0+cu118
should be compatible withtorch==2.0.0
.torch @ 2.0.0
andtorch @ 2.0.0+cu118
.As compared to the solutions we explored in #1855 (comment), at a high level, this approach differs in that we lie about the dependencies of packages that rely on our local-version-using package, rather than lying about the versions that exist, or the version we're returning, etc.
In short:
torch
andtorchvision
.torch
ortorchvision
. If it's an==
, we check if it matches (in the above example) fortorch==2.0.0
. If so, we change the requirement totorch==2.0.0+cu118
. (If it's==
some other version, we return an incompatibility.)In other words, we selectively override the declared dependencies by making them more specific if a compatible local version was specified upfront.
The net effect here is that the motivating PyTorch resolutions all work. And, in general, transitive local versions work as expected.
The thing that still doesn't work is: imagine if there were only local versions of
torch
available. Like,torch @ 2.0.0
didn't exist, buttorch @ 2.0.0+cpu
did, andtorch @ 2.0.0+gpu
did, and so on.pip install torch==2.0.0
would arbitrarily choose one one2.0.0+cpu
or2.0.0+gpu
, and that's correct as per PEP 440 (local version segments should be completely ignored ontorch==2.0.0
). However, uv would fail to identify a compatible version. I'd probably prefer to fix this, although candidly I think our behavior is ok in practice, and it's never been reported as an issue.Closes #1855.
Closes #2080.
Closes #2328.