Skip to content

Commit

Permalink
Remove extra forking
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Jul 1, 2024
1 parent 6149448 commit f4a7ef4
Show file tree
Hide file tree
Showing 7 changed files with 93 additions and 83 deletions.
7 changes: 3 additions & 4 deletions crates/uv-resolver/src/marker.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
#![allow(clippy::enum_glob_use)]

use std::cmp::Ordering;
use std::collections::HashMap;
use std::ops::Bound::{self, *};
use std::ops::RangeBounds;
Expand All @@ -13,7 +12,7 @@ use pep508_rs::{

use crate::pubgrub::PubGrubSpecifier;
use crate::RequiresPythonBound;
use pubgrub::range::{Range as PubGrubRange, Range};
use pubgrub::range::Range as PubGrubRange;

/// 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.
Expand Down Expand Up @@ -92,7 +91,7 @@ pub(crate) fn requires_python_marker(tree: &MarkerTree) -> Option<RequiresPython
let specifier = PubGrubSpecifier::try_from(specifier).ok()?;

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

// Extract the lower bound.
Expand All @@ -105,7 +104,7 @@ pub(crate) fn requires_python_marker(tree: &MarkerTree) -> Option<RequiresPython
MarkerTree::Or(trees) => {
// If all subtrees have a minimum, take the maximum.
let mut version = None;
for tree in trees.iter() {
for tree in trees {
let next = requires_python_marker(tree)?;
version = match version {
Some(version) => Some(std::cmp::max(version, next)),
Expand Down
5 changes: 3 additions & 2 deletions crates/uv-resolver/src/python_requirement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,9 @@ impl PythonRequirement {
}
}

/// Narrow the [`PythonRequirement`] to the given version.
pub fn narrow(&self, target: RequiresPythonBound) -> Option<Self> {
/// 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> {
let Some(PythonTarget::RequiresPython(requires_python)) = self.target.as_ref() else {
return None;
};
Expand Down
47 changes: 27 additions & 20 deletions crates/uv-resolver/src/requires_python.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::cmp::Ordering;
use std::collections::Bound;
use std::ops::Deref;

use itertools::Itertools;
use pubgrub::range::Range;
Expand Down Expand Up @@ -29,16 +30,6 @@ pub struct RequiresPython {
}

impl RequiresPython {
/// Narrow the [`RequiresPython`] to the given version, if it's stricter than the current
/// target.
pub fn narrow(&self, target: RequiresPythonBound) -> Option<Self> {
let target = VersionSpecifiers::from(VersionSpecifier::from_lower_bound(&target.0)?);
Self::union(std::iter::once(&target))
.ok()
.flatten()
.filter(|next| next.bound > self.bound)
}

/// Returns a [`RequiresPython`] to express `>=` equality with the given version.
pub fn greater_than_equal_version(version: Version) -> Self {
Self {
Expand Down Expand Up @@ -89,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<Self> {
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)
Expand Down Expand Up @@ -153,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.0) {
match (target, self.bound.as_ref()) {
(Bound::Included(target_lower), Bound::Included(requires_python_lower)) => {
target_lower.release() <= requires_python_lower.release()
}
Expand All @@ -179,9 +180,9 @@ impl RequiresPython {
&self.specifiers
}

/// Returns the lower [`Bound`] for the `Requires-Python` specifier.
pub fn bound(&self) -> &Bound<Version> {
&self.bound.0
/// 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
Expand All @@ -197,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.0 {
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())
}
};
Expand Down Expand Up @@ -281,6 +280,14 @@ impl RequiresPythonBound {
}
}

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

fn deref(&self) -> &Self::Target {
&self.0
}
}

impl PartialOrd for RequiresPythonBound {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.cmp(other))
Expand All @@ -289,7 +296,7 @@ impl PartialOrd for RequiresPythonBound {

impl Ord for RequiresPythonBound {
fn cmp(&self, other: &Self) -> Ordering {
match (&self.0, &other.0) {
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,
Expand Down
76 changes: 24 additions & 52 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use tracing::{debug, enabled, instrument, trace, warn, Level};
use distribution_types::{
BuiltDist, CompatibleDist, Dist, DistributionMetadata, IncompatibleDist, IncompatibleSource,
IncompatibleWheel, InstalledDist, PythonRequirementKind, RemoteSource, ResolvedDist,
ResolvedDistRef, SourceDist, VersionOrUrlRef, WheelCompatibility,
ResolvedDistRef, SourceDist, VersionOrUrlRef,
};
pub(crate) use locals::Locals;
use pep440_rs::{Version, MIN_VERSION};
Expand Down Expand Up @@ -412,27 +412,6 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag

prefetcher.version_tried(state.next.clone());

// Narrow the Python requirement, if possible.
let (python_requirement, requires_python) = if let Some(python_requirement) = state
.next
.marker()
.and_then(requires_python_marker)
.and_then(|marker| state.python_requirement.narrow(marker))
{
let requires_python = if state.requires_python.is_some() {
python_requirement.to_marker_tree()
} else {
None
};
debug!("requires-python: {:?}", requires_python);
(Cow::Owned(python_requirement), Cow::Owned(requires_python))
} else {
(
Cow::Borrowed(&state.python_requirement),
Cow::Borrowed(&state.requires_python),
)
};

let term_intersection = state
.pubgrub
.partial_solution
Expand All @@ -451,7 +430,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
&mut state.pins,
&preferences,
&state.fork_urls,
&python_requirement,
&state.python_requirement,
visited,
&request_sink,
)?;
Expand Down Expand Up @@ -530,7 +509,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
&version,
&state.fork_urls,
&state.markers,
requires_python.as_ref().as_ref(),
state.requires_python.as_ref(),
)?;
match forked_deps {
ForkedDependencies::Unavailable(reason) => {
Expand Down Expand Up @@ -576,7 +555,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
.map(ToString::to_string)
.join(", ")
);
// assert!(forks.len() >= 2);
assert!(forks.len() >= 2);
// This is a somewhat tortured technique to ensure
// that our resolver state is only cloned as much
// as it needs to be. We basically move the state
Expand All @@ -600,13 +579,12 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
if let Some(python_version) =
requires_python_marker(&forked_state.markers)
{
debug!("Found `requires-python` bound for fork: {python_version:?}");

if let Some(python_requirement) =
forked_state.python_requirement.narrow(python_version)
forked_state.python_requirement.narrow(&python_version)
{
debug!("Narrowed `requires-python` bound to: {python_requirement:?}");

if let Some(target) = python_requirement.target() {
debug!("Narrowed `requires-python` bound to: {target}");
}
forked_state.requires_python =
if forked_state.requires_python.is_some() {
python_requirement.to_marker_tree()
Expand Down Expand Up @@ -1862,8 +1840,20 @@ struct ForkState {
/// that the marker expression that provoked the fork is true), then that
/// dependency is completely ignored.
markers: MarkerTree,
/// The Python requirement for this state.
/// The Python requirement for this fork. Defaults to the Python requirement for
/// the resolution, but may be narrowed if a `python_version` marker is present
/// in a given fork.
///
/// For example, in:
/// ```text
/// numpy >=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<MarkerTree>,
}

Expand Down Expand Up @@ -2434,7 +2424,7 @@ impl Dependencies {
continue;
}
};
// assert!(fork_groups.forks.len() >= 2, "expected definitive fork");
assert!(fork_groups.forks.len() >= 2, "expected definitive fork");
let mut new_forks: Vec<Fork> = vec![];
for group in fork_groups.forks {
let mut new_forks_for_group = forks.clone();
Expand Down Expand Up @@ -2612,18 +2602,7 @@ impl<'a> PossibleForks<'a> {
let PossibleForks::PossiblyForking(ref fork_groups) = *self else {
return false;
};
if fork_groups.forks.len() > 1 {
return true;
}
if fork_groups.forks.iter().any(|fork_groups| {
fork_groups
.packages
.iter()
.any(|(index, markers)| requires_python_marker(markers).is_some())
}) {
return true;
};
false
fork_groups.forks.len() > 1
}

/// Consumes this possible set of forks and converts a "possibly forking"
Expand All @@ -2636,14 +2615,7 @@ impl<'a> PossibleForks<'a> {
let PossibleForks::PossiblyForking(ref fork_groups) = self else {
return self;
};
if fork_groups.forks.len() == 1
&& !fork_groups.forks.iter().any(|fork_groups| {
fork_groups
.packages
.iter()
.any(|(index, markers)| requires_python_marker(markers).is_some())
})
{
if fork_groups.forks.len() == 1 {
self.make_no_forks_possible();
return self;
}
Expand Down
4 changes: 1 addition & 3 deletions crates/uv/src/commands/project/lock.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use std::collections::Bound;

use anstream::eprint;

use distribution_types::UnresolvedRequirementSpecification;
Expand Down Expand Up @@ -127,7 +125,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}`).");
Expand Down
35 changes: 35 additions & 0 deletions crates/uv/tests/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
2 changes: 0 additions & 2 deletions req.in

This file was deleted.

0 comments on commit f4a7ef4

Please sign in to comment.