Prefer higher Python lower-bounds when forking (#10007)

## Summary With the advent of `--fork-strategy requires-python` (the default), we actually _want_ to solve higher lower-bound forks before lower lower-bound forks. The former ensures we get the most compatible versions, while the latter ensures we get fewer overall versions. These two strategies match up with `--fork-strategy`, but need to be respected as such. Closes #9998.
astral-sh · Dec 18, 2024 · 4d3c1b3 · 4d3c1b3
1 parent 8f88f98
commit 4d3c1b3
Show file tree

Hide file tree

Showing 9 changed files with 487 additions and 134 deletions.
diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs
@@ -624,7 +624,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
                         }
                     }
                     ForkedDependencies::Forked {
-                        forks,
+                        mut forks,
                         diverging_packages,
                     } => {
                         debug!(
@@ -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,
@@ -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(),
@@ -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
     }
 }
 

diff --git a/crates/uv/tests/it/branching_urls.rs b/crates/uv/tests/it/branching_urls.rs
@@ -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
     "###
@@ -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]
@@ -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]
@@ -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]
@@ -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]

diff --git a/crates/uv/tests/it/lock.rs b/crates/uv/tests/it/lock.rs
@@ -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]
@@ -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 = [
@@ -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]
@@ -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]
@@ -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]
@@ -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]
@@ -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]
@@ -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]
@@ -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]

diff --git a/crates/uv/tests/it/lock_scenarios.rs b/crates/uv/tests/it/lock_scenarios.rs
@@ -1047,9 +1047,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]]
@@ -3117,9 +3117,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]]