Skip to content

Prefer higher Python lower-bounds when forking #10007

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
charliermarsh merged 1 commit into from
Dec 18, 2024
Merged

Prefer higher Python lower-bounds when forking #10007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 67 additions & 50 deletions crates/uv-resolver/src/resolver/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
}
}
ForkedDependencies::Forked {
forks,
mut forks,
diverging_packages,
} => {
debug!(
Expand All @@ -633,6 +633,28 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
start.elapsed().as_secs_f32()
);

// Prioritize the forks.
match (self.options.fork_strategy, self.options.resolution_mode) {
(ForkStrategy::Fewest, _) | (_, ResolutionMode::Lowest) => {
// Prefer solving forks with lower Python bounds, since they're more
// likely to produce solutions that work for forks with higher
// Python bounds (whereas the inverse is not true).
forks.sort_by(|a, b| {
a.cmp_requires_python(b)
.reverse()
.then_with(|| a.cmp_upper_bounds(b))
});
}
(ForkStrategy::RequiresPython, _) => {
// Otherwise, prefer solving forks with higher Python bounds, since
// we want to prioritize choosing the latest-compatible package
// version for each Python version.
forks.sort_by(|a, b| {
a.cmp_requires_python(b).then_with(|| a.cmp_upper_bounds(b))
});
}
}

for new_fork_state in self.forks_to_fork_states(
state,
&version,
Expand Down Expand Up @@ -2907,11 +2929,6 @@ impl Dependencies {
} else if forks.len() == 1 {
ForkedDependencies::Unforked(forks.pop().unwrap().dependencies)
} else {
// Prioritize the forks. Prefer solving forks with lower Python
// bounds, since they're more likely to produce solutions that work
// for forks with higher Python bounds (whereas the inverse is not
// true).
forks.sort();
ForkedDependencies::Forked {
forks,
diverging_packages: diverging_packages.into_iter().collect(),
Expand Down Expand Up @@ -3224,57 +3241,57 @@ impl Fork {
});
Some(self)
}
}

impl Eq for Fork {}

impl PartialEq for Fork {
fn eq(&self, other: &Fork) -> bool {
self.dependencies == other.dependencies && self.env == other.env
}
}

impl Ord for Fork {
fn cmp(&self, other: &Self) -> Ordering {
// A higher `requires-python` requirement indicates a _lower-priority_ fork. We'd prefer
// to solve `<3.7` before solving `>=3.7`, since the resolution produced by the former might
// work for the latter, but the inverse is unlikely to be true.
/// Compare forks, preferring forks with g `requires-python` requirements.
fn cmp_requires_python(&self, other: &Self) -> Ordering {
// A higher `requires-python` requirement indicates a _higher-priority_ fork.
//
// This ordering ensures that we prefer choosing the highest version for each fork based on
// its `requires-python` requirement.
//
// The reverse would prefer choosing fewer versions, at the cost of using older package
// versions on newer Python versions. For example, if reversed, we'd prefer to solve `<3.7
// before solving `>=3.7`, since the resolution produced by the former might work for the
// latter, but the inverse is unlikely to be true.
let self_bound = self.env.requires_python().unwrap_or_default();
let other_bound = other.env.requires_python().unwrap_or_default();
self_bound.lower().cmp(other_bound.lower())
}

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
// version selection. (Selecting `numpy==2.0.0` would satisfy both forks, but selecting
// the latest `numpy` would not.)
let self_upper_bounds = self
.dependencies
.iter()
.filter(|dep| {
dep.version
.bounding_range()
.is_some_and(|(_, upper)| !matches!(upper, Bound::Unbounded))
})
.count();
let other_upper_bounds = other
.dependencies
.iter()
.filter(|dep| {
dep.version
.bounding_range()
.is_some_and(|(_, upper)| !matches!(upper, Bound::Unbounded))
})
.count();

self_upper_bounds.cmp(&other_upper_bounds)
})
/// Compare forks, preferring forks with upper bounds.
fn cmp_upper_bounds(&self, other: &Self) -> Ordering {
// 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 version selection. (Selecting `numpy==2.0.0` would satisfy both forks, but
// selecting the latest `numpy` would not.)
let self_upper_bounds = self
.dependencies
.iter()
.filter(|dep| {
dep.version
.bounding_range()
.is_some_and(|(_, upper)| !matches!(upper, Bound::Unbounded))
})
.count();
let other_upper_bounds = other
.dependencies
.iter()
.filter(|dep| {
dep.version
.bounding_range()
.is_some_and(|(_, upper)| !matches!(upper, Bound::Unbounded))
})
.count();

self_upper_bounds.cmp(&other_upper_bounds)
}
}

impl PartialOrd for Fork {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.cmp(other))
impl Eq for Fork {}

impl PartialEq for Fork {
fn eq(&self, other: &Fork) -> bool {
self.dependencies == other.dependencies && self.env == other.env
}
}

Expand Down
10 changes: 5 additions & 5 deletions crates/uv/tests/it/branching_urls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ fn root_package_splits_but_transitive_conflict() -> Result<()> {
----- stdout -----

----- stderr -----
error: Requirements contain conflicting URLs for package `iniconfig` in split `python_full_version < '3.12'`:
error: Requirements contain conflicting URLs for package `iniconfig` in split `python_full_version >= '3.12'`:
- https://files.pythonhosted.org/packages/9b/dd/b3c12c6d707058fa947864b67f0c4e0c39ef8610988d7baea9578f3c48f3/iniconfig-1.1.1-py2.py3-none-any.whl
- https://files.pythonhosted.org/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl
"###
Expand Down Expand Up @@ -207,8 +207,8 @@ fn root_package_splits_transitive_too() -> Result<()> {
version = 1
requires-python = ">=3.11, <3.13"
resolution-markers = [
"python_full_version < '3.12'",
"python_full_version >= '3.12'",
"python_full_version < '3.12'",
]

[options]
Expand Down Expand Up @@ -402,8 +402,8 @@ fn root_package_splits_other_dependencies_too() -> Result<()> {
version = 1
requires-python = ">=3.11, <3.13"
resolution-markers = [
"python_full_version < '3.12'",
"python_full_version >= '3.12'",
"python_full_version < '3.12'",
]

[options]
Expand Down Expand Up @@ -563,8 +563,8 @@ fn branching_between_registry_and_direct_url() -> Result<()> {
version = 1
requires-python = ">=3.11, <3.13"
resolution-markers = [
"python_full_version < '3.12'",
"python_full_version >= '3.12'",
"python_full_version < '3.12'",
]

[options]
Expand Down Expand Up @@ -648,8 +648,8 @@ fn branching_urls_of_different_sources_disjoint() -> Result<()> {
version = 1
requires-python = ">=3.11, <3.13"
resolution-markers = [
"python_full_version < '3.12'",
"python_full_version >= '3.12'",
"python_full_version < '3.12'",
]

[options]
Expand Down
28 changes: 14 additions & 14 deletions crates/uv/tests/it/lock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1860,9 +1860,9 @@ fn lock_conditional_dependency_extra() -> Result<()> {
version = 1
requires-python = ">=3.7"
resolution-markers = [
"python_full_version >= '3.10'",
"python_full_version >= '3.8' and python_full_version < '3.10'",
"python_full_version < '3.8'",
"python_full_version >= '3.10'",
]

[options]
Expand Down Expand Up @@ -2045,8 +2045,8 @@ fn lock_conditional_dependency_extra() -> Result<()> {
version = "2.2.1"
source = { registry = "https://pypi.org/simple" }
resolution-markers = [
"python_full_version >= '3.8' and python_full_version < '3.10'",
"python_full_version >= '3.10'",
"python_full_version >= '3.8' and python_full_version < '3.10'",
]
sdist = { url = "https://files.pythonhosted.org/packages/7a/50/7fd50a27caa0652cd4caf224aa87741ea41d3265ad13f010886167cfcc79/urllib3-2.2.1.tar.gz", hash = "sha256:d0570876c61ab9e520d776c38acbbb5b05a776d3f9ff98a5c8fd5162a444cf19", size = 291020 }
wheels = [
Expand Down Expand Up @@ -2961,8 +2961,8 @@ fn lock_partial_git() -> Result<()> {
version = 1
requires-python = ">=3.10"
resolution-markers = [
"python_full_version < '3.12'",
"python_full_version >= '3.12'",
"python_full_version < '3.12'",
]

[options]
Expand Down Expand Up @@ -5143,10 +5143,10 @@ fn lock_python_version_marker_complement() -> Result<()> {
version = 1
requires-python = ">=3.8"
resolution-markers = [
"python_full_version < '3.10'",
"python_full_version == '3.10'",
"python_full_version > '3.10' and python_full_version < '3.11'",
"python_full_version >= '3.11'",
"python_full_version > '3.10' and python_full_version < '3.11'",
"python_full_version == '3.10'",
"python_full_version < '3.10'",
]

[options]
Expand Down Expand Up @@ -12481,9 +12481,9 @@ fn lock_narrowed_python_version() -> Result<()> {
version = 1
requires-python = ">=3.7"
resolution-markers = [
"python_full_version < '3.9'",
"python_full_version >= '3.9' and python_full_version < '3.11'",
"python_full_version >= '3.11'",
"python_full_version >= '3.9' and python_full_version < '3.11'",
"python_full_version < '3.9'",
]

[options]
Expand Down Expand Up @@ -13216,8 +13216,8 @@ fn lock_non_project_fork() -> Result<()> {
version = 1
requires-python = ">=3.10"
resolution-markers = [
"python_full_version < '3.11'",
"python_full_version >= '3.11'",
"python_full_version < '3.11'",
]

[options]
Expand Down Expand Up @@ -15752,9 +15752,9 @@ fn lock_python_upper_bound() -> Result<()> {
version = 1
requires-python = ">=3.8"
resolution-markers = [
"python_full_version >= '3.13'",
"python_full_version < '3.9'",
"python_full_version >= '3.9' and python_full_version < '3.13'",
"python_full_version < '3.9'",
"python_full_version >= '3.13'",
]

[options]
Expand Down Expand Up @@ -16983,8 +16983,8 @@ fn lock_change_requires_python() -> Result<()> {
version = 1
requires-python = ">=3.12"
resolution-markers = [
"python_full_version < '3.13'",
"python_full_version >= '3.13'",
"python_full_version < '3.13'",
]

[options]
Expand Down Expand Up @@ -17092,9 +17092,9 @@ fn lock_change_requires_python() -> Result<()> {
version = 1
requires-python = ">=3.10"
resolution-markers = [
"python_full_version < '3.12'",
"python_full_version == '3.12.*'",
"python_full_version >= '3.13'",
"python_full_version == '3.12.*'",
"python_full_version < '3.12'",
]

[options]
Expand Down
8 changes: 4 additions & 4 deletions crates/uv/tests/it/lock_scenarios.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,9 +887,9 @@ fn fork_incomplete_markers() -> Result<()> {
version = 1
requires-python = ">=3.8"
resolution-markers = [
"python_full_version < '3.10'",
"python_full_version == '3.10.*'",
"python_full_version >= '3.11'",
"python_full_version == '3.10.*'",
"python_full_version < '3.10'",
]

[[package]]
Expand Down Expand Up @@ -2957,9 +2957,9 @@ fn fork_overlapping_markers_basic() -> Result<()> {
version = 1
requires-python = ">=3.8"
resolution-markers = [
"python_full_version < '3.10'",
"python_full_version == '3.10.*'",
"python_full_version >= '3.11'",
"python_full_version == '3.10.*'",
"python_full_version < '3.10'",
]

[[package]]
Expand Down
Loading