From cb920e45163dc8fc85b740af5ffa687d4c1519d4 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 4 Nov 2024 16:22:26 -0500 Subject: [PATCH] Remove all special-casing for local version identifiers --- crates/uv-requirements/src/lookahead.rs | 2 +- crates/uv-resolver/src/manifest.rs | 1 - crates/uv-resolver/src/resolver/locals.rs | 201 ------------------ .../uv-resolver/src/resolver/locals/tests.rs | 130 ----------- crates/uv-resolver/src/resolver/mod.rs | 36 +--- docs/pip/compatibility.md | 29 --- 6 files changed, 3 insertions(+), 396 deletions(-) delete mode 100644 crates/uv-resolver/src/resolver/locals.rs delete mode 100644 crates/uv-resolver/src/resolver/locals/tests.rs diff --git a/crates/uv-requirements/src/lookahead.rs b/crates/uv-requirements/src/lookahead.rs index fd1b252b0e7d..1f6c7cc64fe0 100644 --- a/crates/uv-requirements/src/lookahead.rs +++ b/crates/uv-requirements/src/lookahead.rs @@ -18,7 +18,7 @@ use crate::{required_dist, Error}; /// A resolver for resolving lookahead requirements from direct URLs. /// /// The resolver extends certain privileges to "first-party" requirements. For example, first-party -/// requirements are allowed to contain direct URL references, local version specifiers, and more. +/// requirements are allowed to contain direct URL references. /// /// The lookahead resolver resolves requirements recursively for direct URLs, so that the resolver /// can treat them as first-party dependencies for the purpose of analyzing their specifiers. diff --git a/crates/uv-resolver/src/manifest.rs b/crates/uv-resolver/src/manifest.rs index ed5c517f9107..21fbe1285065 100644 --- a/crates/uv-resolver/src/manifest.rs +++ b/crates/uv-resolver/src/manifest.rs @@ -106,7 +106,6 @@ impl Manifest { /// - Determining which requirements should allow yanked versions. /// - Determining which requirements should allow pre-release versions (e.g., `torch>=2.2.0a1`). /// - Determining which requirements should allow direct URLs (e.g., `torch @ https://...`). - /// - Determining which requirements should allow local version specifiers (e.g., `torch==2.2.0+cpu`). pub fn requirements<'a>( &'a self, env: &'a ResolverEnvironment, diff --git a/crates/uv-resolver/src/resolver/locals.rs b/crates/uv-resolver/src/resolver/locals.rs deleted file mode 100644 index de7a2261953f..000000000000 --- a/crates/uv-resolver/src/resolver/locals.rs +++ /dev/null @@ -1,201 +0,0 @@ -use std::str::FromStr; - -use uv_distribution_filename::{SourceDistFilename, WheelFilename}; -use uv_distribution_types::RemoteSource; -use uv_pep440::{Operator, Version, VersionSpecifier, VersionSpecifierBuildError}; -use uv_pep508::PackageName; -use uv_pypi_types::RequirementSource; - -use crate::resolver::ForkMap; -use crate::{DependencyMode, Manifest, ResolverEnvironment}; - -/// A map of package names to their associated, required local versions across all forks. -#[derive(Debug, Default, Clone)] -pub(crate) struct Locals(ForkMap); - -impl Locals { - /// Determine the set of permitted local versions in the [`Manifest`]. - pub(crate) fn from_manifest( - manifest: &Manifest, - env: &ResolverEnvironment, - dependencies: DependencyMode, - ) -> Self { - let mut locals = ForkMap::default(); - - // Add all direct requirements and constraints. There's no need to look for conflicts, - // since conflicts will be enforced by the solver. - for requirement in manifest.requirements(env, dependencies) { - if let Some(local) = from_source(&requirement.source) { - locals.add(&requirement, local); - } - } - - Self(locals) - } - - /// Return a list of local versions that are compatible with a package in the given fork. - pub(crate) fn get( - &self, - package_name: &PackageName, - env: &ResolverEnvironment, - ) -> Vec<&Version> { - self.0.get(package_name, env) - } - - /// Given a specifier that may include the version _without_ a local segment, return a specifier - /// that includes the local segment from the expected version. - pub(crate) fn map( - local: &Version, - specifier: &VersionSpecifier, - ) -> Result { - match specifier.operator() { - Operator::Equal | Operator::EqualStar => { - // Given `foo==1.0.0`, if the local version is `1.0.0+local`, map to - // `foo==1.0.0+local`. - // - // This has the intended effect of allowing `1.0.0+local`. - if is_compatible(local, specifier.version()) { - VersionSpecifier::from_version(Operator::Equal, local.clone()) - } else { - Ok(specifier.clone()) - } - } - Operator::NotEqual | Operator::NotEqualStar => { - // Given `foo!=1.0.0`, if the local version is `1.0.0+local`, map to - // `foo!=1.0.0+local`. - // - // This has the intended effect of disallowing `1.0.0+local`. - // - // There's no risk of accidentally including `foo @ 1.0.0` in the resolution, since - // we _know_ `foo @ 1.0.0+local` is required and would therefore conflict. - if is_compatible(local, specifier.version()) { - VersionSpecifier::from_version(Operator::NotEqual, local.clone()) - } else { - Ok(specifier.clone()) - } - } - Operator::LessThanEqual => { - // Given `foo<=1.0.0`, if the local version is `1.0.0+local`, map to - // `foo==1.0.0+local`. - // - // This has the intended effect of allowing `1.0.0+local`. - // - // Since `foo==1.0.0+local` is already required, we know that to satisfy - // `foo<=1.0.0`, we _must_ satisfy `foo==1.0.0+local`. We _could_ map to - // `foo<=1.0.0+local`, but local versions are _not_ allowed in exclusive ordered - // specifiers, so introducing `foo<=1.0.0+local` would risk breaking invariants. - if is_compatible(local, specifier.version()) { - VersionSpecifier::from_version(Operator::Equal, local.clone()) - } else { - Ok(specifier.clone()) - } - } - Operator::GreaterThan => { - // Given `foo>1.0.0`, `foo @ 1.0.0+local` is already (correctly) disallowed. - Ok(specifier.clone()) - } - Operator::ExactEqual => { - // Given `foo===1.0.0`, `1.0.0+local` is already (correctly) disallowed. - Ok(specifier.clone()) - } - Operator::TildeEqual => { - // Given `foo~=1.0.0`, `foo~=1.0.0+local` is already (correctly) allowed. - Ok(specifier.clone()) - } - Operator::LessThan => { - // Given `foo<1.0.0`, `1.0.0+local` is already (correctly) disallowed. - Ok(specifier.clone()) - } - Operator::GreaterThanEqual => { - // Given `foo>=1.0.0`, `foo @ 1.0.0+local` is already (correctly) allowed. - Ok(specifier.clone()) - } - } - } -} - -/// Returns `true` if a provided version is compatible with the expected local version. -/// -/// The versions are compatible if they are the same including their local segment, or the -/// same except for the local segment, which is empty in the provided version. -/// -/// For example, if the expected version is `1.0.0+local` and the provided version is `1.0.0+other`, -/// this function will return `false`. -/// -/// If the expected version is `1.0.0+local` and the provided version is `1.0.0`, the function will -/// return `true`. -fn is_compatible(expected: &Version, provided: &Version) -> bool { - // The requirements should be the same, ignoring local segments. - if expected.clone().without_local() != provided.clone().without_local() { - return false; - } - - // If the provided version has a local segment, it should be the same as the expected - // version. - if provided.local().is_empty() { - true - } else { - expected.local() == provided.local() - } -} - -/// If a [`VersionSpecifier`] contains an exact equality specifier for a local version, -/// returns the local version. -pub(crate) fn from_source(source: &RequirementSource) -> Option { - match source { - // Extract all local versions from specifiers that require an exact version (e.g., - // `==1.0.0+local`). - RequirementSource::Registry { - specifier: version, .. - } => version - .iter() - .filter(|specifier| { - matches!(specifier.operator(), Operator::Equal | Operator::ExactEqual) - }) - .filter(|specifier| !specifier.version().local().is_empty()) - .map(|specifier| specifier.version().clone()) - // It's technically possible for there to be multiple local segments here. - // For example, `a==1.0+foo,==1.0+bar`. However, in that case resolution - // will fail later. - .next(), - // Exact a local version from a URL, if it includes a fully-qualified filename (e.g., - // `torch-2.2.1%2Bcu118-cp311-cp311-linux_x86_64.whl`). - RequirementSource::Url { url, .. } => url - .filename() - .ok() - .and_then(|filename| { - if let Ok(filename) = WheelFilename::from_str(&filename) { - Some(filename.version) - } else if let Ok(filename) = - SourceDistFilename::parsed_normalized_filename(&filename) - { - Some(filename.version) - } else { - None - } - }) - .filter(uv_pep440::Version::is_local), - RequirementSource::Git { .. } => None, - RequirementSource::Path { - install_path: path, .. - } => path - .file_name() - .and_then(|filename| { - let filename = filename.to_string_lossy(); - if let Ok(filename) = WheelFilename::from_str(&filename) { - Some(filename.version) - } else if let Ok(filename) = - SourceDistFilename::parsed_normalized_filename(&filename) - { - Some(filename.version) - } else { - None - } - }) - .filter(uv_pep440::Version::is_local), - RequirementSource::Directory { .. } => None, - } -} - -#[cfg(test)] -mod tests; diff --git a/crates/uv-resolver/src/resolver/locals/tests.rs b/crates/uv-resolver/src/resolver/locals/tests.rs deleted file mode 100644 index 7ff7bfe3df80..000000000000 --- a/crates/uv-resolver/src/resolver/locals/tests.rs +++ /dev/null @@ -1,130 +0,0 @@ -use std::str::FromStr; - -use anyhow::Result; -use url::Url; - -use uv_pep440::{Operator, Version, VersionSpecifier, VersionSpecifiers}; -use uv_pep508::VerbatimUrl; -use uv_pypi_types::ParsedUrl; -use uv_pypi_types::RequirementSource; - -use super::{from_source, Locals}; - -#[test] -fn extract_locals() -> Result<()> { - // Extract from a source distribution in a URL. - let url = VerbatimUrl::from_url(Url::parse("https://example.com/foo-1.0.0+local.tar.gz")?); - let source = - RequirementSource::from_parsed_url(ParsedUrl::try_from(url.to_url()).unwrap(), url); - let locals: Vec<_> = from_source(&source).into_iter().collect(); - assert_eq!(locals, vec![Version::from_str("1.0.0+local")?]); - - // Extract from a wheel in a URL. - let url = VerbatimUrl::from_url(Url::parse( - "https://example.com/foo-1.0.0+local-cp39-cp39-linux_x86_64.whl", - )?); - let source = - RequirementSource::from_parsed_url(ParsedUrl::try_from(url.to_url()).unwrap(), url); - let locals: Vec<_> = from_source(&source).into_iter().collect(); - assert_eq!(locals, vec![Version::from_str("1.0.0+local")?]); - - // Don't extract anything if the URL is opaque. - let url = VerbatimUrl::from_url(Url::parse("git+https://example.com/foo/bar")?); - let source = - RequirementSource::from_parsed_url(ParsedUrl::try_from(url.to_url()).unwrap(), url); - let locals: Vec<_> = from_source(&source).into_iter().collect(); - assert!(locals.is_empty()); - - // Extract from `==` specifiers. - let version = VersionSpecifiers::from_iter([ - VersionSpecifier::from_version(Operator::GreaterThan, Version::from_str("1.0.0")?)?, - VersionSpecifier::from_version(Operator::Equal, Version::from_str("1.0.0+local")?)?, - ]); - let source = RequirementSource::Registry { - specifier: version, - index: None, - }; - let locals: Vec<_> = from_source(&source).into_iter().collect(); - assert_eq!(locals, vec![Version::from_str("1.0.0+local")?]); - - // Ignore other specifiers. - let version = VersionSpecifiers::from_iter([VersionSpecifier::from_version( - Operator::NotEqual, - Version::from_str("1.0.0+local")?, - )?]); - let source = RequirementSource::Registry { - specifier: version, - index: None, - }; - let locals: Vec<_> = from_source(&source).into_iter().collect(); - assert!(locals.is_empty()); - - Ok(()) -} - -#[test] -fn map_version() -> Result<()> { - // Given `==1.0.0`, if the local version is `1.0.0+local`, map to `==1.0.0+local`. - let local = Version::from_str("1.0.0+local")?; - let specifier = VersionSpecifier::from_version(Operator::Equal, Version::from_str("1.0.0")?)?; - assert_eq!( - Locals::map(&local, &specifier)?, - VersionSpecifier::from_version(Operator::Equal, Version::from_str("1.0.0+local")?)? - ); - - // Given `!=1.0.0`, if the local version is `1.0.0+local`, map to `!=1.0.0+local`. - let local = Version::from_str("1.0.0+local")?; - let specifier = - VersionSpecifier::from_version(Operator::NotEqual, Version::from_str("1.0.0")?)?; - assert_eq!( - Locals::map(&local, &specifier)?, - VersionSpecifier::from_version(Operator::NotEqual, Version::from_str("1.0.0+local")?)? - ); - - // Given `<=1.0.0`, if the local version is `1.0.0+local`, map to `==1.0.0+local`. - let local = Version::from_str("1.0.0+local")?; - let specifier = - VersionSpecifier::from_version(Operator::LessThanEqual, Version::from_str("1.0.0")?)?; - assert_eq!( - Locals::map(&local, &specifier)?, - VersionSpecifier::from_version(Operator::Equal, Version::from_str("1.0.0+local")?)? - ); - - // Given `>1.0.0`, `1.0.0+local` is already (correctly) disallowed. - let local = Version::from_str("1.0.0+local")?; - let specifier = - VersionSpecifier::from_version(Operator::GreaterThan, Version::from_str("1.0.0")?)?; - assert_eq!( - Locals::map(&local, &specifier)?, - VersionSpecifier::from_version(Operator::GreaterThan, Version::from_str("1.0.0")?)? - ); - - // Given `===1.0.0`, `1.0.0+local` is already (correctly) disallowed. - let local = Version::from_str("1.0.0+local")?; - let specifier = - VersionSpecifier::from_version(Operator::ExactEqual, Version::from_str("1.0.0")?)?; - assert_eq!( - Locals::map(&local, &specifier)?, - VersionSpecifier::from_version(Operator::ExactEqual, Version::from_str("1.0.0")?)? - ); - - // Given `==1.0.0+local`, `1.0.0+local` is already (correctly) allowed. - let local = Version::from_str("1.0.0+local")?; - let specifier = - VersionSpecifier::from_version(Operator::Equal, Version::from_str("1.0.0+local")?)?; - assert_eq!( - Locals::map(&local, &specifier)?, - VersionSpecifier::from_version(Operator::Equal, Version::from_str("1.0.0+local")?)? - ); - - // Given `==1.0.0+other`, `1.0.0+local` is already (correctly) disallowed. - let local = Version::from_str("1.0.0+local")?; - let specifier = - VersionSpecifier::from_version(Operator::Equal, Version::from_str("1.0.0+other")?)?; - assert_eq!( - Locals::map(&local, &specifier)?, - VersionSpecifier::from_version(Operator::Equal, Version::from_str("1.0.0+other")?)? - ); - - Ok(()) -} diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index e3a8edac8b4d..22b65c263093 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -24,7 +24,6 @@ use tracing::{debug, info, instrument, trace, warn, Level}; pub use environment::ResolverEnvironment; pub(crate) use fork_map::{ForkMap, ForkSet}; -use locals::Locals; pub(crate) use urls::Urls; use uv_configuration::{Constraints, Overrides}; use uv_distribution::{ArchiveMetadata, DistributionDatabase}; @@ -81,7 +80,6 @@ mod fork_map; mod groups; mod index; mod indexes; -mod locals; mod provider; mod reporter; mod urls; @@ -105,7 +103,6 @@ struct ResolverState { locations: IndexLocations, exclusions: Exclusions, urls: Urls, - locals: Locals, indexes: Indexes, dependency_mode: DependencyMode, hasher: HashStrategy, @@ -211,7 +208,6 @@ impl selector: CandidateSelector::for_resolution(options, &manifest, &env), dependency_mode: options.dependency_mode, urls: Urls::from_manifest(&manifest, &env, git, options.dependency_mode)?, - locals: Locals::from_manifest(&manifest, &env, options.dependency_mode), indexes: Indexes::from_manifest(&manifest, &env, options.dependency_mode), groups: Groups::from_manifest(&manifest, &env), project: manifest.project, @@ -525,7 +521,6 @@ impl ResolverState ResolverState, version: &Version, urls: &Urls, indexes: &Indexes, - locals: &Locals, mut dependencies: Vec, git: &GitResolver, resolution_strategy: &ResolutionStrategy, @@ -2168,8 +2161,8 @@ impl ForkState { let PubGrubDependency { package, version, - specifier, url, + .. } = dependency; let mut has_url = false; @@ -2183,31 +2176,6 @@ impl ForkState { has_url = true; }; - // If the specifier is an exact version and the user requested a local version for this - // fork that's more precise than the specifier, use the local version instead. - if let Some(specifier) = specifier { - let locals = locals.get(name, &self.env); - - // It's possible that there are multiple matching local versions requested with - // different marker expressions. All of these are potentially compatible until we - // narrow to a specific fork. - for local in locals { - let local = specifier - .iter() - .map(|specifier| { - Locals::map(local, specifier) - .map_err(ResolveError::InvalidVersion) - .map(Ranges::from) - }) - .fold_ok(Range::full(), |range, specifier| { - range.intersection(&specifier) - })?; - - // Add the local version. - *version = version.union(&local); - } - } - // If the package is pinned to an exact index, add it to the fork. for index in indexes.get(name, &self.env) { self.fork_indexes.insert(name, index, &self.env)?; diff --git a/docs/pip/compatibility.md b/docs/pip/compatibility.md index dbfdeafb45c1..fecc39cfcc92 100644 --- a/docs/pip/compatibility.md +++ b/docs/pip/compatibility.md @@ -74,35 +74,6 @@ and are instead focused on behavior for a _single_ version specifier. As such, t questions around the correct and intended behavior for pre-releases in the packaging ecosystem more broadly. -## Local version identifiers - -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 In both uv and `pip`, users can specify multiple package indexes from which to search for the