Skip to content

Commit

Permalink
Filter out markers based on Python requirement (#4912)
Browse files Browse the repository at this point in the history
## Summary

In marker normalization, we now remove any markers that are redundant
with the `requires-python` specifier (i.e., always true for the given
Python requirement).

For example, given `iniconfig ; python_version >= '3.7'`, we can remove
the `python_version >= '3.7'` marker when resolving with
`--python-version 3.8`.

Closes #4852.
  • Loading branch information
charliermarsh committed Jul 9, 2024
1 parent ef120dc commit 72dd34b
Show file tree
Hide file tree
Showing 8 changed files with 146 additions and 48 deletions.
2 changes: 1 addition & 1 deletion crates/uv-resolver/src/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1892,7 +1892,7 @@ impl Dependency {
) -> Dependency {
let distribution_id = DistributionId::from_annotated_dist(annotated_dist);
let extra = annotated_dist.extra.clone();
let marker = marker.cloned().and_then(crate::marker::normalize);
let marker = marker.cloned();
Dependency {
distribution_id,
extra,
Expand Down
108 changes: 75 additions & 33 deletions crates/uv-resolver/src/marker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::collections::HashMap;
use std::ops::Bound::{self, *};
use std::ops::RangeBounds;

use pubgrub::range::Range as PubGrubRange;
use pubgrub::range::{Range as PubGrubRange, Range};

use pep440_rs::{Operator, Version, VersionSpecifier};
use pep508_rs::{
Expand Down Expand Up @@ -82,35 +82,41 @@ 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<RequiresPythonBound> {
match tree {
MarkerTree::Expression(MarkerExpression::Version {
pub(crate) fn python_range(expr: &MarkerExpression) -> Option<Range<Version>> {
match expr {
MarkerExpression::Version {
key: MarkerValueVersion::PythonFullVersion,
specifier,
}) => {
} => {
// Simplify using PEP 440 semantics.
let specifier = PubGrubSpecifier::from_pep440_specifier(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()))
// Convert to PubGrub.
Some(PubGrubRange::from(specifier))
}
MarkerTree::Expression(MarkerExpression::Version {
MarkerExpression::Version {
key: MarkerValueVersion::PythonVersion,
specifier,
}) => {
} => {
// Simplify using release-only semantics, since `python_version` is always `major.minor`.
let specifier = PubGrubSpecifier::from_release_specifier(specifier).ok()?;

// Convert to PubGrub range and perform a union.
let range = PubGrubRange::from(specifier);
let (lower, _) = range.iter().next()?;
// Convert to PubGrub.
Some(PubGrubRange::from(specifier))
}
_ => None,
}
}

/// Returns the minimum Python version that can satisfy the [`MarkerTree`], if it's constrained.
pub(crate) fn requires_python_marker(tree: &MarkerTree) -> Option<RequiresPythonBound> {
match tree {
MarkerTree::Expression(expr) => {
// Extract the supported Python range.
let range = python_range(expr)?;

// Extract the lower bound.
let (lower, _) = range.iter().next()?;
Some(RequiresPythonBound::new(lower.clone()))
}
MarkerTree::And(trees) => {
Expand All @@ -129,42 +135,47 @@ pub(crate) fn requires_python_marker(tree: &MarkerTree) -> Option<RequiresPython
}
min_version
}
MarkerTree::Expression(_) => None,
}
}

/// Normalizes this marker tree.
///
/// This function does a number of operations to normalize a marker tree recursively:
/// - Sort and flatten all nested expressions.
/// - Simplify expressions. This includes combining overlapping version ranges and removing duplicate
/// expressions.
/// - Simplify expressions. This includes combining overlapping version ranges, removing duplicate
/// expressions, and removing redundant expressions.
/// - Normalize the order of version expressions to the form `<version key> <version op> <version>`
/// (i.e. not the reverse).
/// (i.e., not the reverse).
///
/// This is useful in cases where creating conjunctions or disjunctions might occur in a non-deterministic
/// order. This routine will attempt to erase the distinction created by such a construction.
pub(crate) fn normalize(mut tree: MarkerTree) -> Option<MarkerTree> {
pub(crate) fn normalize(
mut tree: MarkerTree,
bound: Option<&RequiresPythonBound>,
) -> Option<MarkerTree> {
// Filter out redundant expressions that show up before and after normalization.
filter_all(&mut tree);
let mut tree = normalize_all(tree)?;
let mut tree = normalize_all(tree, bound)?;
filter_all(&mut tree);
Some(tree)
}

/// Normalize the marker tree recursively.
pub(crate) fn normalize_all(tree: MarkerTree) -> Option<MarkerTree> {
pub(crate) fn normalize_all(
tree: MarkerTree,
bound: Option<&RequiresPythonBound>,
) -> Option<MarkerTree> {
match tree {
MarkerTree::And(trees) => {
let mut reduced = Vec::new();
let mut versions: HashMap<_, Vec<_>> = HashMap::new();

for subtree in trees {
// Simplify nested expressions as much as possible first.
// Normalize nested expressions as much as possible first.
//
// If the expression gets normalized out (e.g., `version < '3.8' and version >= '3.8'`),
// omit it.
let Some(subtree) = normalize_all(subtree) else {
let Some(subtree) = normalize_all(subtree, bound) else {
continue;
};

Expand Down Expand Up @@ -206,11 +217,11 @@ pub(crate) fn normalize_all(tree: MarkerTree) -> Option<MarkerTree> {
let mut versions: HashMap<_, Vec<_>> = HashMap::new();

for subtree in trees {
// Simplify nested expressions as much as possible first.
// Normalize nested expressions as much as possible first.
//
// If the expression gets normalized out (e.g., `version < '3.8' and version >= '3.8'`),
// return `true`.
let subtree = normalize_all(subtree)?;
let subtree = normalize_all(subtree, bound)?;

match subtree {
MarkerTree::And(_) => reduced.push(subtree),
Expand Down Expand Up @@ -260,6 +271,19 @@ pub(crate) fn normalize_all(tree: MarkerTree) -> Option<MarkerTree> {
}
}

// If the marker is redundant given the supported Python range, remove it.
//
// For example, `python_version >= '3.7'` is redundant with `requires-python: '>=3.8'`.
MarkerTree::Expression(expr)
if bound.is_some_and(|bound| {
python_range(&expr).is_some_and(|supported_range| {
Range::from(bound.clone()).subset_of(&supported_range)
})
}) =>
{
None
}

MarkerTree::Expression(ref expr) => {
if let Some((key, range)) = keyed_range(expr) {
// If multiple terms are required to express the range, flatten them into an `Or`
Expand Down Expand Up @@ -681,8 +705,8 @@ mod tests {
#[test]
fn simplify() {
assert_marker_equal(
"python_version == '3.1' or python_version == '3.1'",
"python_version == '3.1'",
"python_version == '3.9' or python_version == '3.9'",
"python_version == '3.9'",
);

assert_marker_equal(
Expand Down Expand Up @@ -827,6 +851,21 @@ mod tests {
);
}

#[test]
fn requires_python() {
assert_normalizes_out("python_version >= '3.8'");
assert_normalizes_out("python_version >= '3.8' or sys_platform == 'win32'");

assert_normalizes_to(
"python_version >= '3.8' and sys_platform == 'win32'",
"sys_platform == 'win32'",
);

assert_normalizes_to("python_version == '3.8'", "python_version == '3.8'");

assert_normalizes_to("python_version <= '3.10'", "python_version <= '3.10'");
}

#[test]
fn extra_disjointness() {
assert!(!is_disjoint("extra == 'a'", "python_version == '1'"));
Expand Down Expand Up @@ -987,8 +1026,9 @@ mod tests {
}

fn assert_marker_equal(one: impl AsRef<str>, two: impl AsRef<str>) {
let bound = RequiresPythonBound::new(Included(Version::new([3, 8])));
let tree1 = MarkerTree::parse_reporter(one.as_ref(), &mut TracingReporter).unwrap();
let tree1 = normalize(tree1).unwrap();
let tree1 = normalize(tree1, Some(&bound)).unwrap();
let tree2 = MarkerTree::parse_reporter(two.as_ref(), &mut TracingReporter).unwrap();
assert_eq!(
tree1.to_string(),
Expand All @@ -999,17 +1039,19 @@ mod tests {
}

fn assert_normalizes_to(before: impl AsRef<str>, after: impl AsRef<str>) {
let bound = RequiresPythonBound::new(Included(Version::new([3, 8])));
let normalized = MarkerTree::parse_reporter(before.as_ref(), &mut TracingReporter)
.unwrap()
.clone();
let normalized = normalize(normalized).unwrap();
let normalized = normalize(normalized, Some(&bound)).unwrap();
assert_eq!(normalized.to_string(), after.as_ref());
}

fn assert_normalizes_out(before: impl AsRef<str>) {
let bound = RequiresPythonBound::new(Included(Version::new([3, 8])));
let normalized = MarkerTree::parse_reporter(before.as_ref(), &mut TracingReporter)
.unwrap()
.clone();
assert!(normalize(normalized).is_none());
assert!(normalize(normalized, Some(&bound)).is_none());
}
}
15 changes: 15 additions & 0 deletions crates/uv-resolver/src/requires_python.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,11 @@ impl RequiresPython {
self.bound.as_ref() == Bound::Unbounded
}

/// Returns the [`RequiresPythonBound`] for the `Requires-Python` specifier.
pub fn bound(&self) -> &RequiresPythonBound {
&self.bound
}

/// Returns this `Requires-Python` specifier as an equivalent marker
/// expression utilizing the `python_version` marker field.
///
Expand Down Expand Up @@ -254,6 +259,16 @@ 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
2 changes: 1 addition & 1 deletion crates/uv-resolver/src/resolution/display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ fn propagate_markers(mut graph: IntermediatePetGraph) -> IntermediatePetGraph {
}

if let DisplayResolutionGraphNode::Dist(node) = &mut graph[index] {
node.markers = marker_tree.and_then(marker::normalize);
node.markers = marker_tree.and_then(|marker| marker::normalize(marker, None));
};
}

Expand Down
12 changes: 11 additions & 1 deletion crates/uv-resolver/src/resolution/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,16 @@ impl ResolutionGraph {
.and_then(PythonTarget::as_requires_python)
.cloned();

// Normalize any markers.
for edge in petgraph.edge_indices() {
if let Some(marker) = petgraph[edge].take() {
petgraph[edge] = crate::marker::normalize(
marker,
requires_python.as_ref().map(RequiresPython::bound),
);
}
}

Ok(Self {
petgraph,
requires_python,
Expand Down Expand Up @@ -318,7 +328,7 @@ impl ResolutionGraph {

/// Return `true` if there are no packages in the graph.
pub fn is_empty(&self) -> bool {
self.dists().any(super::AnnotatedDist::is_base)
self.dists().any(AnnotatedDist::is_base)
}

/// Returns `true` if the graph contains the given package.
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
}

forked_state.markers.and(fork.markers);
forked_state.markers = normalize(forked_state.markers)
forked_state.markers = normalize(forked_state.markers, None)
.unwrap_or(MarkerTree::And(Vec::new()));

// If the fork contains a narrowed Python requirement, apply it.
Expand Down
6 changes: 3 additions & 3 deletions crates/uv/tests/edit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1405,9 +1405,9 @@ fn update() -> Result<()> {
version = "0.1.0"
source = { editable = "." }
dependencies = [
{ name = "requests", marker = "python_version > '3.7'" },
{ name = "requests", extra = "socks", marker = "python_version > '3.7'" },
{ name = "requests", extra = "use-chardet-on-py3", marker = "python_version > '3.7'" },
{ name = "requests" },
{ name = "requests", extra = "socks" },
{ name = "requests", extra = "use-chardet-on-py3" },
]
[[distribution]]
Expand Down
47 changes: 39 additions & 8 deletions crates/uv/tests/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6366,23 +6366,23 @@ fn universal() -> Result<()> {
----- stdout -----
# This file was autogenerated by uv via the following command:
# uv pip compile --cache-dir [CACHE_DIR] requirements.in --universal
attrs==23.2.0 ; python_version > '3.11' or sys_platform == 'win32'
attrs==23.2.0
# via
# outcome
# trio
cffi==1.16.0 ; implementation_name != 'pypy' and os_name == 'nt' and (python_version > '3.11' or sys_platform == 'win32')
cffi==1.16.0 ; implementation_name != 'pypy' and os_name == 'nt'
# via trio
idna==3.6 ; python_version > '3.11' or sys_platform == 'win32'
idna==3.6
# via trio
outcome==1.3.0.post0 ; python_version > '3.11' or sys_platform == 'win32'
outcome==1.3.0.post0
# via trio
pycparser==2.21 ; implementation_name != 'pypy' and os_name == 'nt' and (python_version > '3.11' or sys_platform == 'win32')
pycparser==2.21 ; implementation_name != 'pypy' and os_name == 'nt'
# via cffi
sniffio==1.3.1 ; python_version > '3.11' or sys_platform == 'win32'
sniffio==1.3.1
# via trio
sortedcontainers==2.4.0 ; python_version > '3.11' or sys_platform == 'win32'
sortedcontainers==2.4.0
# via trio
trio==0.25.0 ; python_version > '3.11' or sys_platform == 'win32'
trio==0.25.0
# via -r requirements.in
----- stderr -----
Expand Down Expand Up @@ -6780,6 +6780,37 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> {
Ok(())
}

/// Remove `python_version` markers that are always true.
#[test]
fn universal_unnecessary_python() -> Result<()> {
let context = TestContext::new("3.12");
let requirements_in = context.temp_dir.child("requirements.in");
requirements_in.write_str(indoc::indoc! {r"
iniconfig ; python_version >= '3.7'
"})?;

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
iniconfig==2.0.0
# 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 1 package 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]
Expand Down

0 comments on commit 72dd34b

Please sign in to comment.