Skip to content
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

Merged
merged 2 commits into from
Mar 16, 2024

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Mar 13, 2024

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 allow 1.2.3+foo, but ==1.2.3+foo should not allow 1.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...

  • The user provides requirements like torch==2.0.0+cu118 and torchvision==0.15.1+cu118.
  • We then match those exact versions.
  • We then look at the requirements of torchvision==0.15.1+cu118, which includes torch==2.0.0.
  • Under PEP 440, this is fine, because torch @ 2.0.0+cu118 should be compatible with torch==2.0.0.
  • In our model, though, it's not, because these are different versions. If we change our comparison logic in various places to allow this, we risk breaking some fundamental assumptions of PubGrub around version continuity.
  • Thus, we fail to resolve, because we can't accept both torch @ 2.0.0 and torch @ 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:

  • When users specify local versions upfront, we keep track of them. So, above, we'd take note of torch and torchvision.
  • When we convert the dependencies of a package to PubGrub ranges, we check if the requirement matches torch or torchvision. If it's an==, we check if it matches (in the above example) for torch==2.0.0. If so, we change the requirement to torch==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, 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.

Closes #1855.

Closes #2080.

Closes #2328.

@charliermarsh
Copy link
Member Author

No need to review yet. I need to work on the tests.

@charliermarsh charliermarsh force-pushed the charlie/local branch 2 times, most recently from dcc133d to f267e59 Compare March 13, 2024 21:13
Downloaded 2 packages in [TIME]
Installed 2 packages in [TIME]
+ albatross==1.0.0
+ bluebird==2.0.0+foo
Copy link
Member Author

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.)

@charliermarsh charliermarsh force-pushed the charlie/local branch 4 times, most recently from b218e59 to b4625f3 Compare March 14, 2024 01:06
expected["satisfiable"] = True
expected["packages"] = [
{
"name": "local-greater-than-a",
Copy link
Member Author

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.

@charliermarsh charliermarsh force-pushed the charlie/local branch 2 times, most recently from afa8ecb to 0fe844c Compare March 14, 2024 01:10
@charliermarsh charliermarsh added bug Something isn't working compatibility Compatibility with another interface or tool enhancement New feature or request and removed compatibility Compatibility with another interface or tool labels Mar 14, 2024
@charliermarsh charliermarsh marked this pull request as ready for review March 14, 2024 01:13
})?;
// 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) {
Copy link
Member Author

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.

@charliermarsh
Copy link
Member Author

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.

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)]
Copy link
Member

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.

Copy link
Member Author

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 😂

Copy link
Member

@zanieb zanieb left a 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?

Copy link
Member

@BurntSushi BurntSushi left a 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.

crates/pep440-rs/src/version_specifier.rs Outdated Show resolved Hide resolved
crates/uv-resolver/src/resolver/locals.rs Show resolved Hide resolved
Reverts

Check-in updated tests
@charliermarsh charliermarsh merged commit 5a95f50 into main Mar 16, 2024
30 checks passed
@charliermarsh charliermarsh deleted the charlie/local branch March 16, 2024 14:24
@charliermarsh
Copy link
Member Author

BTW pip install torch==2.1.0 --index-url https://download.pytorch.org/whl/cpu does work, because there's a version in there without a local tag!

@BurntSushi
Copy link
Member

BTW pip install torch==2.1.0 --index-url https://download.pytorch.org/whl/cpu does work, because there's a version in there without a local tag!

Interesting. It doesn't work for me:

$ uv pip install torch==2.1.0 --index-url https://download.pytorch.org/whl/cpu
  x No solution found when resolving dependencies:
  `-> Because torch==2.1.0 is unusable because no wheels are available with a matching Python ABI and you require torch==2.1.0, we can conclude that the requirements are unsatisfiable.

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
3 participants