Stable order for virtual packages

uv gives priorities to packages by package name, not by virtual package (`PubGrubPackage`). pubgrub otoh when prioritizing order the virtual packages. When the order of virtual packages changes, uv changes its resolutions and error messages. This means uv was depending on implementation details of pubgrub's prioritization caching. This broke with pubgrub-rs/pubgrub#299, which added a tiebreaker term that made pubgrub's sorting deterministic given a deterministic ordering of allocating the packages (which happens the first time pubgrub sees a package). The new custom tiebreaker decreases the difference to upstream pubgrub.
astral-sh · Dec 19, 2024 · 5b6e41a · 5b6e41a
1 parent ac348ee
commit 5b6e41a
Show file tree

Hide file tree

Showing 7 changed files with 171 additions and 57 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -130,7 +130,8 @@ petgraph = { version = "0.6.5" }
 platform-info = { version = "2.0.3" }
 proc-macro2 = { version = "1.0.86" }
 procfs = { version = "0.17.0", default-features = false, features = ["flate2"] }
-pubgrub = { git = "https://github.com/astral-sh/pubgrub", rev = "05e8d12cea8d72c6d2d017900e478d0abd28fef4" }
+pubgrub = { path = "../pubgrub" }
+#pubgrub = { git = "https://github.com/astral-sh/pubgrub", rev = "05e8d12cea8d72c6d2d017900e478d0abd28fef4" }
 quote = { version = "1.0.37" }
 rayon = { version = "1.10.0" }
 reflink-copy = { version = "0.1.19" }
@@ -175,7 +176,8 @@ unicode-width = { version = "0.1.13" }
 unscanny = { version = "0.1.0" }
 url = { version = "2.5.2", features = ["serde"] }
 urlencoding = { version = "2.1.3" }
-version-ranges = { git = "https://github.com/astral-sh/pubgrub", rev = "05e8d12cea8d72c6d2d017900e478d0abd28fef4" }
+version-ranges = { path = "../pubgrub/version-ranges" }
+#version-ranges = { git = "https://github.com/astral-sh/pubgrub", rev = "05e8d12cea8d72c6d2d017900e478d0abd28fef4" }
 walkdir = { version = "2.5.0" }
 which = { version = "7.0.0", features = ["regex"] }
 windows-registry = { version = "0.3.0" }

diff --git a/crates/uv-resolver/src/dependency_provider.rs b/crates/uv-resolver/src/dependency_provider.rs
@@ -17,7 +17,8 @@ impl DependencyProvider for UvDependencyProvider {
     type V = Version;
     type VS = Range<Version>;
     type M = UnavailableReason;
-    type Priority = Option<PubGrubPriority>;
+    /// Main priority and tiebreak for virtual packages
+    type Priority = (Option<PubGrubPriority>, u32);
     type Err = Infallible;
 
     fn prioritize(

diff --git a/crates/uv-resolver/src/pubgrub/priority.rs b/crates/uv-resolver/src/pubgrub/priority.rs
@@ -20,7 +20,10 @@ use crate::pubgrub::PubGrubPackageInner;
 ///
 /// See: <https://github.com/pypa/pip/blob/ef78c129b1a966dbbbdb8ebfffc43723e89110d1/src/pip/_internal/resolution/resolvelib/provider.py#L120>
 #[derive(Clone, Debug, Default)]
-pub(crate) struct PubGrubPriorities(FxHashMap<PackageName, PubGrubPriority>);
+pub(crate) struct PubGrubPriorities {
+    package_priority: FxHashMap<PackageName, PubGrubPriority>,
+    virtual_package_tiebreaker: FxHashMap<PubGrubPackage, u32>,
+}
 
 impl PubGrubPriorities {
     /// Add a [`PubGrubPackage`] to the priority map.
@@ -30,14 +33,22 @@ impl PubGrubPriorities {
         version: &Range<Version>,
         urls: &ForkUrls,
     ) {
-        let next = self.0.len();
+        if !self.virtual_package_tiebreaker.contains_key(&package) {
+            self.virtual_package_tiebreaker.insert(
+                package.clone(),
+                u32::try_from(self.virtual_package_tiebreaker.len())
+                    .expect("Less than 2**32 packages"),
+            );
+        }
+
+        let next = self.package_priority.len();
         // The root package and Python constraints have no explicit priority, the root package is
         // always first and the Python version (range) is fixed.
         let Some(name) = package.name_no_root() else {
             return;
         };
 
-        match self.0.entry(name.clone()) {
+        match self.package_priority.entry(name.clone()) {
             std::collections::hash_map::Entry::Occupied(mut entry) => {
                 // Preserve the original index.
                 let index = Self::get_index(&entry).unwrap_or(next);
@@ -92,15 +103,21 @@ impl PubGrubPriorities {
     }
 
     /// Return the [`PubGrubPriority`] of the given package, if it exists.
-    pub(crate) fn get(&self, package: &PubGrubPackage) -> Option<PubGrubPriority> {
-        match &**package {
+    pub(crate) fn get(&self, package: &PubGrubPackage) -> (Option<PubGrubPriority>, u32) {
+        let package_priority = match &**package {
             PubGrubPackageInner::Root(_) => Some(PubGrubPriority::Root),
             PubGrubPackageInner::Python(_) => Some(PubGrubPriority::Root),
-            PubGrubPackageInner::Marker { name, .. } => self.0.get(name).copied(),
-            PubGrubPackageInner::Extra { name, .. } => self.0.get(name).copied(),
-            PubGrubPackageInner::Dev { name, .. } => self.0.get(name).copied(),
-            PubGrubPackageInner::Package { name, .. } => self.0.get(name).copied(),
-        }
+            PubGrubPackageInner::Marker { name, .. } => self.package_priority.get(name).copied(),
+            PubGrubPackageInner::Extra { name, .. } => self.package_priority.get(name).copied(),
+            PubGrubPackageInner::Dev { name, .. } => self.package_priority.get(name).copied(),
+            PubGrubPackageInner::Package { name, .. } => self.package_priority.get(name).copied(),
+        };
+        let virtual_package_tiebreaker = self
+            .virtual_package_tiebreaker
+            .get(&package)
+            .copied()
+            .unwrap_or_default();
+        (package_priority, virtual_package_tiebreaker)
     }
 
     /// Mark a package as prioritized by setting it to [`PubGrubPriority::ConflictEarly`], if it
@@ -109,7 +126,7 @@ impl PubGrubPriorities {
     /// Returns whether the priority was changed, i.e., it's the first time we hit this condition
     /// for the package.
     pub(crate) fn mark_conflict_early(&mut self, package: &PubGrubPackage) -> bool {
-        let next = self.0.len();
+        let next = self.package_priority.len();
         let Some(name) = package.name_no_root() else {
             // Not a correctness bug
             if cfg!(debug_assertions) {
@@ -118,7 +135,7 @@ impl PubGrubPriorities {
                 return false;
             }
         };
-        match self.0.entry(name.clone()) {
+        match self.package_priority.entry(name.clone()) {
             std::collections::hash_map::Entry::Occupied(mut entry) => {
                 if matches!(
                     entry.get(),
@@ -144,7 +161,7 @@ impl PubGrubPriorities {
     /// Returns whether the priority was changed, i.e., it's the first time this package was
     /// marked as conflicting above the threshold.
     pub(crate) fn mark_conflict_late(&mut self, package: &PubGrubPackage) -> bool {
-        let next = self.0.len();
+        let next = self.package_priority.len();
         let Some(name) = package.name_no_root() else {
             // Not a correctness bug
             if cfg!(debug_assertions) {
@@ -153,7 +170,7 @@ impl PubGrubPriorities {
                 return false;
             }
         };
-        match self.0.entry(name.clone()) {
+        match self.package_priority.entry(name.clone()) {
             std::collections::hash_map::Entry::Occupied(mut entry) => {
                 // The ConflictEarly` match avoids infinite loops.
                 if matches!(

diff --git a/crates/uv/tests/it/lock_conflict.rs b/crates/uv/tests/it/lock_conflict.rs
@@ -250,8 +250,8 @@ fn extra_basic_three_extras() -> Result<()> {
 
     ----- stderr -----
       × No solution found when resolving dependencies:
-      ╰─▶ Because project[project3] depends on sortedcontainers==2.4.0 and project[extra1] depends on sortedcontainers==2.2.0, we can conclude that project[extra1] and project[project3] are incompatible.
-          And because your project requires project[extra1] and project[project3], we can conclude that your project's requirements are unsatisfiable.
+      ╰─▶ Because project[project3] depends on sortedcontainers==2.4.0 and project[extra2] depends on sortedcontainers==2.3.0, we can conclude that project[extra2] and project[project3] are incompatible.
+          And because your project requires project[extra2] and project[project3], we can conclude that your project's requirements are unsatisfiable.
     "###);
 
     // And now with the same extra configuration, we tell uv about
@@ -538,8 +538,8 @@ fn extra_multiple_not_conflicting2() -> Result<()> {
 
     ----- stderr -----
       × No solution found when resolving dependencies:
-      ╰─▶ Because project[project4] depends on sortedcontainers==2.4.0 and project[extra1] depends on sortedcontainers==2.3.0, we can conclude that project[extra1] and project[project4] are incompatible.
-          And because your project requires project[extra1] and project[project4], we can conclude that your project's requirements are unsatisfiable.
+      ╰─▶ Because project[project4] depends on sortedcontainers==2.4.0 and project[project3] depends on sortedcontainers==2.3.0, we can conclude that project[project3] and project[project4] are incompatible.
+          And because your project requires project[project3] and project[project4], we can conclude that your project's requirements are unsatisfiable.
     "###);
 
     // If we define extra1/extra2 as conflicting and project3/project4
@@ -1289,10 +1289,8 @@ fn extra_nested_across_workspace() -> Result<()> {
 
     ----- stderr -----
       × No solution found when resolving dependencies:
-      ╰─▶ Because dummy[extra2] depends on proxy1[extra2] and only proxy1[extra2]==0.1.0 is available, we can conclude that dummy[extra2] depends on proxy1[extra2]==0.1.0. (1)
-
-          Because proxy1[extra2]==0.1.0 depends on anyio==4.2.0 and proxy1[extra1]==0.1.0 depends on anyio==4.1.0, we can conclude that proxy1[extra1]==0.1.0 and proxy1[extra2]==0.1.0 are incompatible.
-          And because we know from (1) that dummy[extra2] depends on proxy1[extra2]==0.1.0, we can conclude that dummy[extra2] and proxy1[extra1]==0.1.0 are incompatible.
+      ╰─▶ Because dummy[extra2] depends on proxy1[extra2] and only proxy1[extra2]==0.1.0 is available, we can conclude that dummy[extra2] depends on proxy1[extra2]==0.1.0.
+          And because proxy1[extra2]==0.1.0 depends on anyio==4.2.0 and proxy1[extra1]==0.1.0 depends on anyio==4.1.0, we can conclude that proxy1[extra1]==0.1.0 and dummy[extra2] are incompatible.
           And because only proxy1[extra1]==0.1.0 is available and dummysub[extra1] depends on proxy1[extra1], we can conclude that dummysub[extra1] and dummy[extra2] are incompatible.
           And because your workspace requires dummy[extra2] and dummysub[extra1], we can conclude that your workspace's requirements are unsatisfiable.
     "###);
@@ -1795,7 +1793,7 @@ fn mixed() -> Result<()> {
 
     ----- stderr -----
       × No solution found when resolving dependencies:
-      ╰─▶ Because project[extra1] depends on sortedcontainers==2.4.0 and project:group1 depends on sortedcontainers==2.3.0, we can conclude that project:group1 and project[extra1] are incompatible.
+      ╰─▶ Because project:group1 depends on sortedcontainers==2.3.0 and project[extra1] depends on sortedcontainers==2.4.0, we can conclude that project[extra1] and project:group1 are incompatible.
           And because your project requires project[extra1] and project:group1, we can conclude that your project's requirements are unsatisfiable.
     "###);