From d9f389a58d748f217dfb35d11b63706ce53a5be7 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 2 Jul 2024 08:23:38 -0400 Subject: [PATCH] Narrow `requires-python` requirement in resolver forks (#4707) ## Summary Given: ```text numpy >=1.26 ; python_version >= '3.9' numpy <1.26 ; python_version < '3.9' ``` When resolving for Python 3.8, we need to narrow the `requires-python` requirement in the top branch of the fork, because `numpy >=1.26` all require Python 3.9 or later -- but we know (in that branch) that we only need to _solve_ for Python 3.9 or later. Closes https://github.com/astral-sh/uv/issues/4669. --- crates/pep440-rs/src/version_specifier.rs | 4 +- crates/uv-resolver/src/lib.rs | 2 +- crates/uv-resolver/src/marker.rs | 40 ++++++- crates/uv-resolver/src/pubgrub/package.rs | 7 +- crates/uv-resolver/src/python_requirement.rs | 15 ++- crates/uv-resolver/src/requires_python.rs | 94 ++++++++++++---- crates/uv-resolver/src/resolver/mod.rs | 108 ++++++++++++++----- crates/uv/src/commands/project/lock.rs | 4 +- crates/uv/tests/pip_compile.rs | 35 ++++++ 9 files changed, 251 insertions(+), 58 deletions(-) diff --git a/crates/pep440-rs/src/version_specifier.rs b/crates/pep440-rs/src/version_specifier.rs index a462fb770745..09f589ab516d 100644 --- a/crates/pep440-rs/src/version_specifier.rs +++ b/crates/pep440-rs/src/version_specifier.rs @@ -436,7 +436,7 @@ impl VersionSpecifier { } /// Returns a version specifier representing the given lower bound. - fn from_lower_bound(bound: &Bound) -> Option { + pub fn from_lower_bound(bound: &Bound) -> Option { match bound { Bound::Included(version) => Some( VersionSpecifier::from_version(Operator::GreaterThanEqual, version.clone()) @@ -450,7 +450,7 @@ impl VersionSpecifier { } /// Returns a version specifier representing the given upper bound. - fn from_upper_bound(bound: &Bound) -> Option { + pub fn from_upper_bound(bound: &Bound) -> Option { match bound { Bound::Included(version) => Some( VersionSpecifier::from_version(Operator::LessThanEqual, version.clone()).unwrap(), diff --git a/crates/uv-resolver/src/lib.rs b/crates/uv-resolver/src/lib.rs index 6c1be8ffe5f4..353305bd1472 100644 --- a/crates/uv-resolver/src/lib.rs +++ b/crates/uv-resolver/src/lib.rs @@ -10,7 +10,7 @@ pub use preferences::{Preference, PreferenceError, Preferences}; pub use prerelease_mode::PreReleaseMode; pub use pubgrub::{PubGrubSpecifier, PubGrubSpecifierError}; pub use python_requirement::PythonRequirement; -pub use requires_python::{RequiresPython, RequiresPythonError}; +pub use requires_python::{RequiresPython, RequiresPythonBound, RequiresPythonError}; pub use resolution::{AnnotationStyle, DisplayResolutionGraph, ResolutionGraph}; pub use resolution_mode::ResolutionMode; pub use resolver::{ diff --git a/crates/uv-resolver/src/marker.rs b/crates/uv-resolver/src/marker.rs index 3c836779d3ec..1012d31449d6 100644 --- a/crates/uv-resolver/src/marker.rs +++ b/crates/uv-resolver/src/marker.rs @@ -4,6 +4,8 @@ use std::collections::HashMap; use std::ops::Bound::{self, *}; use std::ops::RangeBounds; +use pubgrub::range::Range as PubGrubRange; + use pep440_rs::{Operator, Version, VersionSpecifier}; use pep508_rs::{ ExtraName, ExtraOperator, MarkerExpression, MarkerOperator, MarkerTree, MarkerValueString, @@ -11,7 +13,7 @@ use pep508_rs::{ }; use crate::pubgrub::PubGrubSpecifier; -use pubgrub::range::Range as PubGrubRange; +use crate::RequiresPythonBound; /// Returns `true` if there is no environment in which both marker trees can both apply, i.e. /// the expression `first and second` is always false. @@ -80,6 +82,42 @@ fn string_is_disjoint(this: &MarkerExpression, other: &MarkerExpression) -> bool true } +/// Returns the minimum Python version that can satisfy the [`MarkerTree`], if it's constrained. +pub(crate) fn requires_python_marker(tree: &MarkerTree) -> Option { + match tree { + MarkerTree::Expression(MarkerExpression::Version { + key: MarkerValueVersion::PythonFullVersion | MarkerValueVersion::PythonVersion, + specifier, + }) => { + let specifier = PubGrubSpecifier::try_from(specifier).ok()?; + + // Convert to PubGrub range and perform a union. + let range = PubGrubRange::from(specifier); + let (lower, _) = range.iter().next()?; + + // Extract the lower bound. + Some(RequiresPythonBound::new(lower.clone())) + } + MarkerTree::And(trees) => { + // Take the maximum of any nested expressions. + trees.iter().filter_map(requires_python_marker).max() + } + MarkerTree::Or(trees) => { + // If all subtrees have a bound, take the minimum. + let mut min_version = None; + for tree in trees { + let version = requires_python_marker(tree)?; + min_version = match min_version { + Some(min_version) => Some(std::cmp::min(min_version, version)), + None => Some(version), + }; + } + min_version + } + MarkerTree::Expression(_) => None, + } +} + /// Normalizes this marker tree. /// /// This function does a number of operations to normalize a marker tree recursively: diff --git a/crates/uv-resolver/src/pubgrub/package.rs b/crates/uv-resolver/src/pubgrub/package.rs index 350960148325..e677b4d0d4be 100644 --- a/crates/uv-resolver/src/pubgrub/package.rs +++ b/crates/uv-resolver/src/pubgrub/package.rs @@ -1,4 +1,3 @@ -use std::fmt::{Display, Formatter}; use std::ops::Deref; use std::sync::Arc; @@ -17,9 +16,9 @@ impl Deref for PubGrubPackage { } } -impl Display for PubGrubPackage { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - Display::fmt(&self.0, f) +impl std::fmt::Display for PubGrubPackage { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Display::fmt(&self.0, f) } } diff --git a/crates/uv-resolver/src/python_requirement.rs b/crates/uv-resolver/src/python_requirement.rs index bc262f9ff9fe..cf06fdcdda9e 100644 --- a/crates/uv-resolver/src/python_requirement.rs +++ b/crates/uv-resolver/src/python_requirement.rs @@ -2,7 +2,7 @@ use pep440_rs::VersionSpecifiers; use pep508_rs::{MarkerTree, StringVersion}; use uv_toolchain::{Interpreter, PythonVersion}; -use crate::RequiresPython; +use crate::{RequiresPython, RequiresPythonBound}; #[derive(Debug, Clone, Eq, PartialEq)] pub struct PythonRequirement { @@ -49,6 +49,19 @@ impl PythonRequirement { } } + /// Narrow the [`PythonRequirement`] to the given version, if it's stricter (i.e., greater) + /// than the current `Requires-Python` minimum. + pub fn narrow(&self, target: &RequiresPythonBound) -> Option { + let Some(PythonTarget::RequiresPython(requires_python)) = self.target.as_ref() else { + return None; + }; + let requires_python = requires_python.narrow(target)?; + Some(Self { + installed: self.installed.clone(), + target: Some(PythonTarget::RequiresPython(requires_python)), + }) + } + /// Return the installed version of Python. pub fn installed(&self) -> &StringVersion { &self.installed diff --git a/crates/uv-resolver/src/requires_python.rs b/crates/uv-resolver/src/requires_python.rs index 48a4588babe0..43a861b20337 100644 --- a/crates/uv-resolver/src/requires_python.rs +++ b/crates/uv-resolver/src/requires_python.rs @@ -1,4 +1,6 @@ +use std::cmp::Ordering; use std::collections::Bound; +use std::ops::Deref; use itertools::Itertools; use pubgrub::range::Range; @@ -24,7 +26,7 @@ pub enum RequiresPythonError { #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub struct RequiresPython { specifiers: VersionSpecifiers, - bound: Bound, + bound: RequiresPythonBound, } impl RequiresPython { @@ -34,7 +36,7 @@ impl RequiresPython { specifiers: VersionSpecifiers::from(VersionSpecifier::greater_than_equal_version( version.clone(), )), - bound: Bound::Included(version), + bound: RequiresPythonBound(Bound::Included(version)), } } @@ -61,11 +63,13 @@ impl RequiresPython { }; // Extract the lower bound. - let bound = range - .iter() - .next() - .map(|(lower, _)| lower.clone()) - .unwrap_or(Bound::Unbounded); + let bound = RequiresPythonBound( + range + .iter() + .next() + .map(|(lower, _)| lower.clone()) + .unwrap_or(Bound::Unbounded), + ); // Convert back to PEP 440 specifiers. let specifiers = range @@ -76,6 +80,16 @@ impl RequiresPython { Ok(Some(Self { specifiers, bound })) } + /// Narrow the [`RequiresPython`] to the given version, if it's stricter (i.e., greater) than + /// the current target. + pub fn narrow(&self, target: &RequiresPythonBound) -> Option { + let target = VersionSpecifiers::from(VersionSpecifier::from_lower_bound(target)?); + Self::union(std::iter::once(&target)) + .ok() + .flatten() + .filter(|next| next.bound > self.bound) + } + /// Returns `true` if the `Requires-Python` is compatible with the given version. pub fn contains(&self, version: &Version) -> bool { self.specifiers.contains(version) @@ -140,7 +154,7 @@ impl RequiresPython { // Alternatively, we could vary the semantics depending on whether or not the user included // a pre-release in their specifier, enforcing pre-release compatibility only if the user // explicitly requested it. - match (target, &self.bound) { + match (target, self.bound.as_ref()) { (Bound::Included(target_lower), Bound::Included(requires_python_lower)) => { target_lower.release() <= requires_python_lower.release() } @@ -166,9 +180,9 @@ impl RequiresPython { &self.specifiers } - /// Returns the lower [`Bound`] for the `Requires-Python` specifier. - pub fn bound(&self) -> &Bound { - &self.bound + /// Returns `true` if the `Requires-Python` specifier is unbounded. + pub fn is_unbounded(&self) -> bool { + self.bound.as_ref() == Bound::Unbounded } /// Returns this `Requires-Python` specifier as an equivalent marker @@ -184,16 +198,14 @@ impl RequiresPython { /// returns a marker tree that evaluates to `true` for all possible marker /// environments. pub fn to_marker_tree(&self) -> MarkerTree { - let (op, version) = match self.bound { + let (op, version) = match self.bound.as_ref() { // If we see this anywhere, then it implies the marker // tree we would generate would always evaluate to // `true` because every possible Python version would // satisfy it. Bound::Unbounded => return MarkerTree::And(vec![]), - Bound::Excluded(ref version) => { - (Operator::GreaterThan, version.clone().without_local()) - } - Bound::Included(ref version) => { + Bound::Excluded(version) => (Operator::GreaterThan, version.clone().without_local()), + Bound::Included(version) => { (Operator::GreaterThanEqual, version.clone().without_local()) } }; @@ -247,12 +259,50 @@ impl serde::Serialize for RequiresPython { impl<'de> serde::Deserialize<'de> for RequiresPython { fn deserialize>(deserializer: D) -> Result { let specifiers = VersionSpecifiers::deserialize(deserializer)?; - let bound = crate::pubgrub::PubGrubSpecifier::try_from(&specifiers) - .map_err(serde::de::Error::custom)? - .iter() - .next() - .map(|(lower, _)| lower.clone()) - .unwrap_or(Bound::Unbounded); + let bound = RequiresPythonBound( + crate::pubgrub::PubGrubSpecifier::try_from(&specifiers) + .map_err(serde::de::Error::custom)? + .iter() + .next() + .map(|(lower, _)| lower.clone()) + .unwrap_or(Bound::Unbounded), + ); Ok(Self { specifiers, bound }) } } + +#[derive(Debug, Clone, Eq, PartialEq, Hash)] +pub struct RequiresPythonBound(Bound); + +impl RequiresPythonBound { + pub fn new(bound: Bound) -> Self { + Self(bound) + } +} + +impl Deref for RequiresPythonBound { + type Target = Bound; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl PartialOrd for RequiresPythonBound { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for RequiresPythonBound { + fn cmp(&self, other: &Self) -> Ordering { + match (self.as_ref(), other.as_ref()) { + (Bound::Included(a), Bound::Included(b)) => a.cmp(b), + (Bound::Included(_), Bound::Excluded(_)) => Ordering::Less, + (Bound::Excluded(_), Bound::Included(_)) => Ordering::Greater, + (Bound::Excluded(a), Bound::Excluded(b)) => a.cmp(b), + (Bound::Unbounded, _) => Ordering::Less, + (_, Bound::Unbounded) => Ordering::Greater, + } + } +} diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index e1e3fa3b7221..411e190f5a01 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -44,7 +44,7 @@ use crate::dependency_provider::UvDependencyProvider; use crate::error::ResolveError; use crate::fork_urls::ForkUrls; use crate::manifest::Manifest; -use crate::marker::normalize; +use crate::marker::{normalize, requires_python_marker}; use crate::pins::FilePins; use crate::preferences::Preferences; use crate::pubgrub::{ @@ -312,6 +312,14 @@ impl ResolverState, request_sink: Sender, ) -> Result { + debug!( + "Solving with installed Python version: {}", + self.python_requirement.installed() + ); + if let Some(target) = self.python_requirement.target() { + debug!("Solving with target Python version: {}", target); + } + let root = PubGrubPackage::from(PubGrubPackageInner::Root(self.project.clone())); let mut prefetcher = BatchPrefetcher::default(); let state = ForkState { @@ -322,22 +330,20 @@ impl ResolverState ResolverState ResolverState { @@ -562,10 +570,29 @@ impl ResolverState ResolverState, request_sink: &Sender, ) -> Result, ResolveError> { @@ -746,13 +774,14 @@ impl ResolverState { if let Some(url) = package.name().and_then(|name| fork_urls.get(name)) { - self.choose_version_url(name, range, url) + self.choose_version_url(name, range, url, python_requirement) } else { self.choose_version_registry( name, range, package, preferences, + python_requirement, pins, visited, request_sink, @@ -769,6 +798,7 @@ impl ResolverState, url: &VerbatimParsedUrl, + python_requirement: &PythonRequirement, ) -> Result, ResolveError> { debug!( "Searching for a compatible version of {name} @ {} ({range})", @@ -826,8 +856,9 @@ impl ResolverState ResolverState ResolverState, package: &PubGrubPackage, preferences: &Preferences, + python_requirement: &PythonRequirement, pins: &mut FilePins, visited: &mut FxHashSet, request_sink: &Sender, @@ -932,7 +964,7 @@ impl ResolverState ResolverState ResolverState ResolverState ResolverState, ) -> Result { - let result = self.get_dependencies(package, version, fork_urls, markers); + let result = self.get_dependencies(package, version, fork_urls, markers, requires_python); if self.markers.is_some() { return result.map(|deps| match deps { Dependencies::Available(deps) => ForkedDependencies::Unforked(deps), @@ -1053,6 +1086,7 @@ impl ResolverState, ) -> Result { let url = package.name().and_then(|name| fork_urls.get(name)); let dependencies = match &**package { @@ -1065,6 +1099,7 @@ impl ResolverState ResolverState ResolverState, name: Option<&PackageName>, markers: &'a MarkerTree, + requires_python: Option<&'a MarkerTree>, ) -> Vec> { // Start with the requirements for the current extra of the package (for an extra // requirement) or the non-extra (regular) dependencies (if extra is None), plus @@ -1323,7 +1360,12 @@ impl ResolverState>(); // Check if there are recursive self inclusions and we need to go into the expensive branch. @@ -1346,7 +1388,9 @@ impl ResolverState ResolverState + 'parameters, extra: Option<&'parameters ExtraName>, markers: &'parameters MarkerTree, + requires_python: Option<&'parameters MarkerTree>, ) -> impl Iterator> + 'parameters where 'data: 'parameters, @@ -1379,12 +1424,12 @@ impl ResolverState ResolverState=1.26 ; python_version >= "3.9" + /// numpy <1.26 ; python_version < "3.9" + /// ``` + /// + /// The top fork has a narrower Python compatibility range, and thus can find a + /// solution that omits Python 3.8 support. + python_requirement: PythonRequirement, + /// The [`MarkerTree`] corresponding to the [`PythonRequirement`]. + requires_python: Option, } impl ForkState { diff --git a/crates/uv/src/commands/project/lock.rs b/crates/uv/src/commands/project/lock.rs index 5576b879a319..0bed2c04bda3 100644 --- a/crates/uv/src/commands/project/lock.rs +++ b/crates/uv/src/commands/project/lock.rs @@ -1,5 +1,3 @@ -use std::collections::Bound; - use anstream::eprint; use distribution_types::UnresolvedRequirementSpecification; @@ -129,7 +127,7 @@ pub(super) async fn do_lock( let requires_python = find_requires_python(workspace)?; let requires_python = if let Some(requires_python) = requires_python { - if matches!(requires_python.bound(), Bound::Unbounded) { + if requires_python.is_unbounded() { let default = RequiresPython::greater_than_equal_version(interpreter.python_minor_version()); warn_user!("The workspace `requires-python` field does not contain a lower bound: `{requires_python}`. Set a lower bound to indicate the minimum compatible Python version (e.g., `{default}`)."); diff --git a/crates/uv/tests/pip_compile.rs b/crates/uv/tests/pip_compile.rs index 8e2533e6ebfe..cc557a6a6f05 100644 --- a/crates/uv/tests/pip_compile.rs +++ b/crates/uv/tests/pip_compile.rs @@ -6564,6 +6564,41 @@ fn universal_multi_version() -> Result<()> { Ok(()) } +/// Perform a universal resolution that requires narrowing the supported Python range in one of the +/// fork branches. +#[test] +fn universal_requires_python() -> Result<()> { + let context = TestContext::new("3.12"); + let requirements_in = context.temp_dir.child("requirements.in"); + requirements_in.write_str(indoc::indoc! {r" + numpy >=1.26 ; python_version >= '3.9' + numpy <1.26 ; python_version < '3.9' + "})?; + + uv_snapshot!(context.filters(), windows_filters=false, context.pip_compile() + .arg("requirements.in") + .arg("-p") + .arg("3.8") + .arg("--universal"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + # This file was autogenerated by uv via the following command: + # uv pip compile --cache-dir [CACHE_DIR] requirements.in -p 3.8 --universal + numpy==1.24.4 ; python_version < '3.9' + # via -r requirements.in + numpy==1.26.4 ; python_version >= '3.9' + # via -r requirements.in + + ----- stderr ----- + warning: The requested Python version 3.8 is not available; 3.12.[X] will be used to build dependencies instead. + Resolved 2 packages in [TIME] + "### + ); + + Ok(()) +} + /// Resolve a package from a `requirements.in` file, with a `constraints.txt` file pinning one of /// its transitive dependencies to a specific version. #[test]