Skip to content

Commit

Permalink
Respect the user's upper-bound in requires-python (#6824)
Browse files Browse the repository at this point in the history
## Summary

We now respect the user-provided upper-bound in for `requires-python`.
So, if the user has `requires-python = "==3.11.*"`, we won't explore
forks that have `python_version >= '3.12'`, for example.

However, we continue to _only_ compare the lower bounds when assessing
whether a dependency is compatible with a given Python range.

Closes #6150.
  • Loading branch information
charliermarsh authored Aug 29, 2024
1 parent a17c1e8 commit 34d7450
Show file tree
Hide file tree
Showing 10 changed files with 308 additions and 77 deletions.
6 changes: 6 additions & 0 deletions crates/uv-pubgrub/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,15 @@ pub enum PubGrubSpecifierError {
pub struct PubGrubSpecifier(Range<Version>);

impl PubGrubSpecifier {
/// Returns an iterator over the bounds of the [`PubGrubSpecifier`].
pub fn iter(&self) -> impl Iterator<Item = (&Bound<Version>, &Bound<Version>)> {
self.0.iter()
}

/// Return the bounding [`Range`] of the [`PubGrubSpecifier`].
pub fn bounding_range(&self) -> Option<(Bound<&Version>, Bound<&Version>)> {
self.0.bounding_range()
}
}

impl From<Range<Version>> for PubGrubSpecifier {
Expand Down
4 changes: 3 additions & 1 deletion crates/uv-resolver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ pub use preferences::{Preference, PreferenceError, Preferences};
pub use prerelease::PrereleaseMode;
pub use pubgrub::{PubGrubSpecifier, PubGrubSpecifierError};
pub use python_requirement::PythonRequirement;
pub use requires_python::{RequiresPython, RequiresPythonBound, RequiresPythonError};
pub use requires_python::{
RequiresPython, RequiresPythonBound, RequiresPythonError, RequiresPythonRange,
};
pub use resolution::{AnnotationStyle, DisplayResolutionGraph, ResolutionGraph};
pub use resolution_mode::ResolutionMode;
pub use resolver::{
Expand Down
34 changes: 25 additions & 9 deletions crates/uv-resolver/src/marker.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
use pep508_rs::{MarkerTree, MarkerTreeKind, MarkerValueVersion};

use crate::requires_python::RequiresPythonRange;
use crate::RequiresPythonBound;
use pep440_rs::Version;
use pep508_rs::{MarkerTree, MarkerTreeKind, MarkerValueVersion};
use pubgrub::Range;

/// Returns the minimum Python version that can satisfy the [`MarkerTree`], if it's constrained.
pub(crate) fn requires_python(tree: &MarkerTree) -> Option<RequiresPythonBound> {
fn collect_python_markers(tree: &MarkerTree, markers: &mut Vec<RequiresPythonBound>) {
/// Returns the bounding Python versions that can satisfy the [`MarkerTree`], if it's constrained.
pub(crate) fn requires_python(tree: &MarkerTree) -> Option<RequiresPythonRange> {
fn collect_python_markers(tree: &MarkerTree, markers: &mut Vec<Range<Version>>) {
match tree.kind() {
MarkerTreeKind::True | MarkerTreeKind::False => {}
MarkerTreeKind::Version(marker) => match marker.key() {
MarkerValueVersion::PythonVersion | MarkerValueVersion::PythonFullVersion => {
for (range, tree) in marker.edges() {
if !tree.is_false() {
// Extract the lower bound.
let (lower, _) = range.iter().next().unwrap();
markers.push(RequiresPythonBound::new(lower.clone()));
markers.push(range.clone());
}
}
}
Expand Down Expand Up @@ -48,5 +48,21 @@ pub(crate) fn requires_python(tree: &MarkerTree) -> Option<RequiresPythonBound>

let mut markers = Vec::new();
collect_python_markers(tree, &mut markers);
markers.into_iter().min()

// Take the union of all Python version markers.
let range = markers
.into_iter()
.fold(None, |acc: Option<Range<Version>>, range| {
Some(match acc {
Some(acc) => acc.union(&range),
None => range.clone(),
})
})?;

let (lower, upper) = range.bounding_range()?;

Some(RequiresPythonRange::new(
RequiresPythonBound::new(lower.cloned()),
RequiresPythonBound::new(upper.cloned()),
))
}
4 changes: 2 additions & 2 deletions crates/uv-resolver/src/python_requirement.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use pep440_rs::{Version, VersionSpecifiers};
use uv_python::{Interpreter, PythonVersion};

use crate::{RequiresPython, RequiresPythonBound};
use crate::{RequiresPython, RequiresPythonRange};

#[derive(Debug, Clone, Eq, PartialEq)]
pub struct PythonRequirement {
Expand Down Expand Up @@ -49,7 +49,7 @@ 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<Self> {
pub fn narrow(&self, target: &RequiresPythonRange) -> Option<Self> {
let Some(PythonTarget::RequiresPython(requires_python)) = self.target.as_ref() else {
return None;
};
Expand Down
160 changes: 111 additions & 49 deletions crates/uv-resolver/src/requires_python.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,8 @@ pub struct RequiresPython {
/// For a workspace, it's the union of all `requires-python` values in the workspace. If no
/// bound was provided by the user, it's greater equal the current Python version.
specifiers: VersionSpecifiers,
/// The lower bound from the `specifiers` field, i.e. greater or greater equal the lowest
/// version allowed by `specifiers`.
bound: RequiresPythonBound,
/// The lower and upper bounds of `specifiers`.
range: RequiresPythonRange,
}

impl RequiresPython {
Expand All @@ -44,22 +43,26 @@ impl RequiresPython {
specifiers: VersionSpecifiers::from(VersionSpecifier::greater_than_equal_version(
version.clone(),
)),
bound: RequiresPythonBound(Bound::Included(version)),
range: RequiresPythonRange(
RequiresPythonBound::new(Bound::Included(version.clone())),
RequiresPythonBound::new(Bound::Unbounded),
),
}
}

/// Returns a [`RequiresPython`] from a version specifier.
pub fn from_specifiers(specifiers: &VersionSpecifiers) -> Result<Self, RequiresPythonError> {
let bound = RequiresPythonBound(
let (lower_bound, upper_bound) =
crate::pubgrub::PubGrubSpecifier::from_release_specifiers(specifiers)?
.iter()
.next()
.map(|(lower, _)| lower.clone())
.unwrap_or(Bound::Unbounded),
);
.bounding_range()
.map(|(lower_bound, upper_bound)| (lower_bound.cloned(), upper_bound.cloned()))
.unwrap_or((Bound::Unbounded, Bound::Unbounded));
Ok(Self {
specifiers: specifiers.clone(),
bound,
range: RequiresPythonRange(
RequiresPythonBound(lower_bound),
RequiresPythonBound(upper_bound),
),
})
}

Expand All @@ -85,31 +88,36 @@ impl RequiresPython {
return Ok(None);
};

// Extract the lower bound.
let bound = RequiresPythonBound(
range
.iter()
.next()
.map(|(lower, _)| lower.clone())
.unwrap_or(Bound::Unbounded),
);
// Extract the bounds.
let (lower_bound, upper_bound) = range
.bounding_range()
.map(|(lower_bound, upper_bound)| (lower_bound.cloned(), upper_bound.cloned()))
.unwrap_or((Bound::Unbounded, Bound::Unbounded));

// Convert back to PEP 440 specifiers.
let specifiers = range
.iter()
.flat_map(VersionSpecifier::from_release_only_bounds)
.collect();

Ok(Some(Self { specifiers, bound }))
Ok(Some(Self {
specifiers,
range: RequiresPythonRange(
RequiresPythonBound(lower_bound),
RequiresPythonBound(upper_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<Self> {
if target > &self.bound {
pub fn narrow(&self, range: &RequiresPythonRange) -> Option<Self> {
if *range == self.range {
None
} else if range.0 >= self.range.0 && range.1 <= self.range.1 {
Some(Self {
specifiers: VersionSpecifiers::from(VersionSpecifier::from_lower_bound(target)?),
bound: target.clone(),
specifiers: self.specifiers.clone(),
range: range.clone(),
})
} else {
None
Expand Down Expand Up @@ -142,7 +150,7 @@ impl RequiresPython {
// `>=3.7`.
//
// That is: `version_lower` should be less than or equal to `requires_python_lower`.
match (target, self.bound.as_ref()) {
match (target, self.range.lower().as_ref()) {
(Bound::Included(target_lower), Bound::Included(requires_python_lower)) => {
target_lower <= requires_python_lower
}
Expand Down Expand Up @@ -170,17 +178,12 @@ impl RequiresPython {

/// Returns `true` if the `Requires-Python` specifier is unbounded.
pub fn is_unbounded(&self) -> bool {
self.bound.as_ref() == Bound::Unbounded
}

/// Returns the [`RequiresPythonBound`] for the `Requires-Python` specifier.
pub fn bound(&self) -> &RequiresPythonBound {
&self.bound
self.range.lower().as_ref() == Bound::Unbounded
}

/// Returns the [`RequiresPythonBound`] truncated to the major and minor version.
pub fn bound_major_minor(&self) -> RequiresPythonBound {
match self.bound.as_ref() {
match self.range.lower().as_ref() {
// Ex) `>=3.10.1` -> `>=3.10`
Bound::Included(version) => RequiresPythonBound(Bound::Included(Version::new(
version.release().iter().take(2),
Expand All @@ -195,6 +198,11 @@ impl RequiresPython {
}
}

/// Returns the [`Range`] bounding the `Requires-Python` specifier.
pub fn range(&self) -> &RequiresPythonRange {
&self.range
}

/// Returns `false` if the wheel's tags state it can't be used in the given Python version
/// range.
///
Expand Down Expand Up @@ -279,15 +287,76 @@ impl serde::Serialize for RequiresPython {
impl<'de> serde::Deserialize<'de> for RequiresPython {
fn deserialize<D: serde::Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
let specifiers = VersionSpecifiers::deserialize(deserializer)?;
let bound = RequiresPythonBound(
let (lower_bound, upper_bound) =
crate::pubgrub::PubGrubSpecifier::from_release_specifiers(&specifiers)
.map_err(serde::de::Error::custom)?
.iter()
.next()
.map(|(lower, _)| lower.clone())
.unwrap_or(Bound::Unbounded),
);
Ok(Self { specifiers, bound })
.bounding_range()
.map(|(lower_bound, upper_bound)| (lower_bound.cloned(), upper_bound.cloned()))
.unwrap_or((Bound::Unbounded, Bound::Unbounded));
Ok(Self {
specifiers,
range: RequiresPythonRange(
RequiresPythonBound(lower_bound),
RequiresPythonBound(upper_bound),
),
})
}
}

#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct RequiresPythonRange(RequiresPythonBound, RequiresPythonBound);

impl RequiresPythonRange {
/// Initialize a [`RequiresPythonRange`] with the given bounds.
pub fn new(lower: RequiresPythonBound, upper: RequiresPythonBound) -> Self {
Self(lower, upper)
}

/// Returns the lower bound.
pub fn lower(&self) -> &RequiresPythonBound {
&self.0
}

/// Returns the upper bound.
pub fn upper(&self) -> &RequiresPythonBound {
&self.1
}
}

impl Default for RequiresPythonRange {
fn default() -> Self {
Self(
RequiresPythonBound(Bound::Unbounded),
RequiresPythonBound(Bound::Unbounded),
)
}
}

impl From<RequiresPythonRange> for Range<Version> {
fn from(value: RequiresPythonRange) -> Self {
match (value.0.as_ref(), value.1.as_ref()) {
(Bound::Included(lower), Bound::Included(upper)) => {
Range::from_range_bounds(lower.clone()..=upper.clone())
}
(Bound::Included(lower), Bound::Excluded(upper)) => {
Range::from_range_bounds(lower.clone()..upper.clone())
}
(Bound::Excluded(lower), Bound::Included(upper)) => {
Range::strictly_higher_than(lower.clone())
.intersection(&Range::lower_than(upper.clone()))
}
(Bound::Excluded(lower), Bound::Excluded(upper)) => {
Range::strictly_higher_than(lower.clone())
.intersection(&Range::strictly_lower_than(upper.clone()))
}
(Bound::Unbounded, Bound::Unbounded) => Range::full(),
(Bound::Unbounded, Bound::Included(upper)) => Range::lower_than(upper.clone()),
(Bound::Unbounded, Bound::Excluded(upper)) => Range::strictly_lower_than(upper.clone()),
(Bound::Included(lower), Bound::Unbounded) => Range::higher_than(lower.clone()),
(Bound::Excluded(lower), Bound::Unbounded) => {
Range::strictly_higher_than(lower.clone())
}
}
}
}

Expand All @@ -301,6 +370,9 @@ impl Default for RequiresPythonBound {
}

impl RequiresPythonBound {
/// Initialize a [`RequiresPythonBound`] with the given bound.
///
/// These bounds use release-only semantics when comparing versions.
pub fn new(bound: Bound<Version>) -> Self {
Self(match bound {
Bound::Included(version) => Bound::Included(version.only_release()),
Expand All @@ -310,16 +382,6 @@ impl RequiresPythonBound {
}
}

impl From<RequiresPythonBound> for Range<Version> {
fn from(value: RequiresPythonBound) -> Self {
match value.0 {
Bound::Included(version) => Range::higher_than(version),
Bound::Excluded(version) => Range::strictly_higher_than(version),
Bound::Unbounded => Range::full(),
}
}
}

impl Deref for RequiresPythonBound {
type Target = Bound<Version>;

Expand Down
8 changes: 4 additions & 4 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1486,7 +1486,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
// supported by the root, skip it.
let requirement = if let Some(requires_python) = python_requirement.target().and_then(|target| target.as_requires_python()).filter(|_| !requirement.marker.is_true()) {
let marker = requirement.marker.clone().simplify_python_versions(
Range::from(requires_python.bound().clone()),
Range::from(requires_python.range().clone()),
);

if marker.is_false() {
Expand Down Expand Up @@ -1552,7 +1552,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
// supported by the root, skip it.
let constraint = if let Some(requires_python) = python_requirement.target().and_then(|target| target.as_requires_python()).filter(|_| !constraint.marker.is_true()) {
let mut marker = constraint.marker.clone().simplify_python_versions(
Range::from(requires_python.bound().clone()),
Range::from(requires_python.range().clone()),
);
marker.and(requirement.marker.clone());

Expand Down Expand Up @@ -2813,7 +2813,7 @@ impl Ord for Fork {
let self_bound = marker::requires_python(&self.markers).unwrap_or_default();
let other_bound = marker::requires_python(&other.markers).unwrap_or_default();

other_bound.cmp(&self_bound).then_with(|| {
other_bound.lower().cmp(self_bound.lower()).then_with(|| {
// If there's no difference, prioritize forks with upper bounds. We'd prefer to solve
// `numpy <= 2` before solving `numpy >= 1`, since the resolution produced by the former
// might work for the latter, but the inverse is unlikely to be true due to maximum
Expand Down Expand Up @@ -2855,7 +2855,7 @@ fn simplify_python(marker: MarkerTree, python_requirement: &PythonRequirement) -
.target()
.and_then(|target| target.as_requires_python())
{
marker.simplify_python_versions(Range::from(requires_python.bound().clone()))
marker.simplify_python_versions(Range::from(requires_python.range().clone()))
} else {
marker
}
Expand Down
2 changes: 1 addition & 1 deletion crates/uv/src/commands/project/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,7 @@ impl ValidatedLock {
// Requires-Python bound in the workspace, we should have the necessary wheels to perform
// a locked resolution.
if let Some(locked) = lock.requires_python() {
if locked.bound() != requires_python.bound() {
if locked.range() != requires_python.range() {
// On the other hand, if the bound in the lockfile is stricter, meaning the
// bound has since been weakened, we have to perform a clean resolution to ensure
// we fetch the necessary wheels.
Expand Down
Loading

0 comments on commit 34d7450

Please sign in to comment.