Skip to content

Commit

Permalink
Add support for PyTorch-style local version semantics (#2430)
Browse files Browse the repository at this point in the history
## 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](#1855 (comment))
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.
  • Loading branch information
charliermarsh authored Mar 16, 2024
1 parent 62fdd3d commit 5a95f50
Show file tree
Hide file tree
Showing 12 changed files with 389 additions and 72 deletions.
33 changes: 26 additions & 7 deletions PIP_COMPATIBILITY.md
Original file line number Diff line number Diff line change
Expand Up @@ -132,13 +132,32 @@ broadly.

## Local version identifiers

uv does not implement spec-compliant handling of local version identifiers (e.g., `1.0.0+local`).
Though local version identifiers are rare in published packages (and, e.g., disallowed on PyPI),
they're common in the PyTorch ecosystem. uv's incorrect handling of local version identifiers
may lead to resolution failures in some cases.

In the future, uv intends to implement spec-compliant handling of local version identifiers.
For more, see [#1855](https://github.com/astral-sh/uv/issues/1855).
uv does not implement spec-compliant handling of local version identifiers (e.g., `1.2.3+local`).
This is considered a known limitation. Although local version identifiers are rare in published
packages (and, e.g., disallowed on PyPI), they're common in the PyTorch ecosystem, and uv's approach
to local versions _does_ support typical PyTorch workflows to succeed out-of-the-box.

[PEP 440](https://peps.python.org/pep-0440/#version-specifiers) specifies that the local version
segment should typically be ignored when evaluating version specifiers, with a few exceptions.
For example, `foo==1.2.3` should accept `1.2.3+local`, but `foo==1.2.3+local` should _not_ accept
`1.2.3`. These asymmetries are hard to model in a resolution algorithm. As such, uv treats `1.2.3`
and `1.2.3+local` as entirely separate versions, but respects local versions provided as direct
dependencies throughout the resolution, such that if you provide `foo==1.2.3+local` as a direct
dependency, `1.2.3+local` _will_ be accepted for any transitive dependencies that request
`foo==1.2.3`.

To take an example from the PyTorch ecosystem, it's common to specify `torch==2.0.0+cu118` and
`torchvision==0.15.1+cu118` as direct dependencies. `torchvision @ 0.15.1+cu118` declares a
dependency on `torch==2.0.0`. In this case, uv would recognize that `torch==2.0.0+cu118` satisfies
the specifier, since it was provided as a direct dependency.

As compared to pip, the main differences in observed behavior are as follows:

- In general, local versions must be provided as direct dependencies. Resolution may succeed for
transitive dependencies that request a non-local version, but this is not guaranteed.
- If _only_ local versions exist for a package `foo` at a given version (e.g., `1.2.3+local` exists,
but `1.2.3` does not), `uv pip install foo==1.2.3` will fail, while `pip install foo==1.2.3` will
resolve to an arbitrary local version.

## Packages that exist on multiple indexes

Expand Down
5 changes: 4 additions & 1 deletion crates/pep440-rs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ pub use {
LocalSegment, Operator, OperatorParseError, PreRelease, PreReleaseKind, Version,
VersionParseError, VersionPattern, VersionPatternParseError, MIN_VERSION,
},
version_specifier::{VersionSpecifier, VersionSpecifiers, VersionSpecifiersParseError},
version_specifier::{
VersionSpecifier, VersionSpecifierBuildError, VersionSpecifiers,
VersionSpecifiersParseError,
},
};

mod version;
Expand Down
23 changes: 16 additions & 7 deletions crates/pep440-rs/src/version_specifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -342,16 +342,12 @@ impl Serialize for VersionSpecifier {
impl VersionSpecifier {
/// Build from parts, validating that the operator is allowed with that version. The last
/// parameter indicates a trailing `.*`, to differentiate between `1.1.*` and `1.1`
pub fn new(
pub fn from_pattern(
operator: Operator,
version_pattern: VersionPattern,
) -> Result<Self, VersionSpecifierBuildError> {
let star = version_pattern.is_wildcard();
let version = version_pattern.into_version();
// "Local version identifiers are NOT permitted in this version specifier."
if version.is_local() && !operator.is_local_compatible() {
return Err(BuildErrorKind::OperatorLocalCombo { operator, version }.into());
}

// Check if there are star versions and if so, switch operator to star version
let operator = if star {
Expand All @@ -365,6 +361,19 @@ impl VersionSpecifier {
operator
};

Self::from_version(operator, version)
}

/// Create a new version specifier from an operator and a version.
pub fn from_version(
operator: Operator,
version: Version,
) -> Result<Self, VersionSpecifierBuildError> {
// "Local version identifiers are NOT permitted in this version specifier."
if version.is_local() && !operator.is_local_compatible() {
return Err(BuildErrorKind::OperatorLocalCombo { operator, version }.into());
}

if operator == Operator::TildeEqual && version.release().len() < 2 {
return Err(BuildErrorKind::CompatibleRelease.into());
}
Expand Down Expand Up @@ -545,7 +554,7 @@ impl FromStr for VersionSpecifier {
}
let vpat = version.parse().map_err(ParseErrorKind::InvalidVersion)?;
let version_specifier =
Self::new(operator, vpat).map_err(ParseErrorKind::InvalidSpecifier)?;
Self::from_pattern(operator, vpat).map_err(ParseErrorKind::InvalidSpecifier)?;
s.eat_while(|c: char| c.is_whitespace());
if !s.done() {
return Err(ParseErrorKind::InvalidTrailing(s.after().to_string()).into());
Expand Down Expand Up @@ -1664,7 +1673,7 @@ Failed to parse version: Unexpected end of version specifier, expected operator:
let op = Operator::TildeEqual;
let v = Version::new([5]);
let vpat = VersionPattern::verbatim(v);
assert_eq!(err, VersionSpecifier::new(op, vpat).unwrap_err());
assert_eq!(err, VersionSpecifier::from_pattern(op, vpat).unwrap_err());
assert_eq!(
err.to_string(),
"The ~= operator requires at least two segments in the release version"
Expand Down
4 changes: 2 additions & 2 deletions crates/pep508-rs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1198,12 +1198,12 @@ mod tests {
],
version_or_url: Some(VersionOrUrl::VersionSpecifier(
[
VersionSpecifier::new(
VersionSpecifier::from_pattern(
Operator::GreaterThanEqual,
VersionPattern::verbatim(Version::new([2, 8, 1])),
)
.unwrap(),
VersionSpecifier::new(
VersionSpecifier::from_pattern(
Operator::Equal,
VersionPattern::wildcard(Version::new([2, 8])),
)
Expand Down
8 changes: 4 additions & 4 deletions crates/pep508-rs/src/marker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ impl MarkerExpression {
Some(operator) => operator,
};

let specifier = match VersionSpecifier::new(operator, r_vpat) {
let specifier = match VersionSpecifier::from_pattern(operator, r_vpat) {
Ok(specifier) => specifier,
Err(err) => {
reporter(
Expand Down Expand Up @@ -674,7 +674,7 @@ impl MarkerExpression {
Some(operator) => operator,
};

let specifier = match VersionSpecifier::new(
let specifier = match VersionSpecifier::from_pattern(
operator,
VersionPattern::verbatim(r_version.clone()),
) {
Expand Down Expand Up @@ -784,7 +784,7 @@ impl MarkerExpression {
let r_vpat = r_string.parse::<VersionPattern>().ok()?;
let operator = operator.to_pep440_operator()?;
// operator and right hand side make the specifier
let specifier = VersionSpecifier::new(operator, r_vpat).ok()?;
let specifier = VersionSpecifier::from_pattern(operator, r_vpat).ok()?;

let compatible = python_versions
.iter()
Expand All @@ -808,7 +808,7 @@ impl MarkerExpression {
let compatible = python_versions.iter().any(|r_version| {
// operator and right hand side make the specifier and in this case the
// right hand is `python_version` so changes every iteration
match VersionSpecifier::new(
match VersionSpecifier::from_pattern(
operator,
VersionPattern::verbatim(r_version.clone()),
) {
Expand Down
6 changes: 3 additions & 3 deletions crates/uv-resolver/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,6 @@ pub enum ResolveError {
#[error("There are conflicting URLs for package `{0}`:\n- {1}\n- {2}")]
ConflictingUrlsTransitive(PackageName, String, String),

#[error("There are conflicting versions for `{0}`: {1}")]
ConflictingVersions(String, String),

#[error("Package `{0}` attempted to resolve via URL: {1}. URL dependencies must be expressed as direct requirements or constraints. Consider adding `{0} @ {1}` to your dependencies or constraints file.")]
DisallowedUrl(PackageName, String),

Expand Down Expand Up @@ -87,6 +84,9 @@ pub enum ResolveError {
version: Box<Version>,
},

#[error("Attempted to construct an invalid version specifier")]
InvalidVersion(#[from] pep440_rs::VersionSpecifierBuildError),

/// Something unexpected happened.
#[error("{0}")]
Failure(String),
Expand Down
41 changes: 30 additions & 11 deletions crates/uv-resolver/src/pubgrub/dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,23 @@ use crate::constraints::Constraints;
use crate::overrides::Overrides;
use crate::pubgrub::specifier::PubGrubSpecifier;
use crate::pubgrub::PubGrubPackage;
use crate::resolver::Urls;
use crate::resolver::{Locals, Urls};
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)]
pub(crate) fn from_requirements(
requirements: &[Requirement],
constraints: &Constraints,
overrides: &Overrides,
source_name: Option<&PackageName>,
source_extra: Option<&ExtraName>,
urls: &Urls,
locals: &Locals,
env: &MarkerEnvironment,
) -> Result<Self, ResolveError> {
let mut dependencies = Vec::default();
Expand All @@ -42,12 +44,12 @@ impl PubGrubDependencies {
}

// Add the package, plus any extra variants.
for result in std::iter::once(to_pubgrub(requirement, None, urls)).chain(
for result in std::iter::once(to_pubgrub(requirement, None, urls, locals)).chain(
requirement
.extras
.clone()
.into_iter()
.map(|extra| to_pubgrub(requirement, Some(extra), urls)),
.map(|extra| to_pubgrub(requirement, Some(extra), urls, locals)),
) {
let (mut package, version) = result?;

Expand Down Expand Up @@ -76,12 +78,12 @@ impl PubGrubDependencies {
}

// Add the package, plus any extra variants.
for result in std::iter::once(to_pubgrub(constraint, None, urls)).chain(
for result in std::iter::once(to_pubgrub(constraint, None, urls, locals)).chain(
constraint
.extras
.clone()
.into_iter()
.map(|extra| to_pubgrub(constraint, Some(extra), urls)),
.map(|extra| to_pubgrub(constraint, Some(extra), urls, locals)),
) {
let (mut package, version) = result?;

Expand Down Expand Up @@ -128,6 +130,7 @@ fn to_pubgrub(
requirement: &Requirement,
extra: Option<ExtraName>,
urls: &Urls,
locals: &Locals,
) -> Result<(PubGrubPackage, Range<Version>), ResolveError> {
match requirement.version_or_url.as_ref() {
// The requirement has no specifier (e.g., `flask`).
Expand All @@ -138,12 +141,28 @@ fn to_pubgrub(

// The requirement has a specifier (e.g., `flask>=1.0`).
Some(VersionOrUrl::VersionSpecifier(specifiers)) => {
let version = specifiers
.iter()
.map(PubGrubSpecifier::try_from)
.fold_ok(Range::full(), |range, specifier| {
range.intersection(&specifier.into())
})?;
// 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) {
specifiers
.iter()
.map(|specifier| {
Locals::map(expected, specifier)
.map_err(ResolveError::InvalidVersion)
.and_then(|specifier| PubGrubSpecifier::try_from(&specifier))
})
.fold_ok(Range::full(), |range, specifier| {
range.intersection(&specifier.into())
})?
} else {
specifiers
.iter()
.map(PubGrubSpecifier::try_from)
.fold_ok(Range::full(), |range, specifier| {
range.intersection(&specifier.into())
})?
};

Ok((
PubGrubPackage::from_package(requirement.name.clone(), extra, urls),
version,
Expand Down
Loading

0 comments on commit 5a95f50

Please sign in to comment.