Add PubGrub's priority queue (#221)

Pulls in https://github.com/pubgrub-rs/pubgrub/pull/104.

Cargo.lock

@@ -1959,6 +1959,16 @@ dependencies = [
  "termtree",
 ]
 
+[[package]]
+name = "priority-queue"
+version = "1.3.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "fff39edfcaec0d64e8d0da38564fad195d2d51b680940295fcc307366e101e61"
+dependencies = [
+ "autocfg",
+ "indexmap 1.9.3",
+]
+
 [[package]]
 name = "proc-macro-error"
 version = "1.0.4"
@@ -2011,7 +2021,9 @@ dependencies = [
 name = "pubgrub"
 version = "0.2.1"
 dependencies = [
+ "indexmap 2.0.2",
  "log",
+ "priority-queue",
  "rustc-hash",
  "thiserror",
 ]

crates/puffin-resolver/src/pubgrub/mod.rs

@@ -10,6 +10,7 @@ use crate::pubgrub::specifier::PubGrubSpecifier;
 use crate::pubgrub::version::{PubGrubVersion, MAX_VERSION};
 
 pub(crate) mod package;
+pub(crate) mod priority;
 mod specifier;
 pub(crate) mod version;
 

crates/puffin-resolver/src/pubgrub/priority.rs

@@ -0,0 +1,3 @@
+use std::cmp::Reverse;
+
+pub(crate) type PubGrubPriority = Reverse<usize>;

crates/puffin-resolver/src/resolution.rs

@@ -1,8 +1,8 @@
+use std::hash::BuildHasherDefault;
+
 use colored::Colorize;
 use fxhash::FxHashMap;
 use petgraph::visit::EdgeRef;
-use std::hash::BuildHasherDefault;
-
 use pubgrub::range::Range;
 use pubgrub::solver::{Kind, State};
 use pubgrub::type_aliases::SelectedDependencies;
@@ -13,6 +13,7 @@ use puffin_client::File;
 use puffin_package::package_name::PackageName;
 
 use crate::pubgrub::package::PubGrubPackage;
+use crate::pubgrub::priority::PubGrubPriority;
 use crate::pubgrub::version::PubGrubVersion;
 
 /// A package pinned at a specific version.
@@ -95,7 +96,7 @@ impl Graph {
     pub fn from_state(
         selection: &SelectedDependencies<PubGrubPackage, PubGrubVersion>,
         pins: &FxHashMap<PackageName, FxHashMap<Version, File>>,
-        state: &State<PubGrubPackage, Range<PubGrubVersion>>,
+        state: &State<PubGrubPackage, Range<PubGrubVersion>, PubGrubPriority>,
     ) -> Self {
         // TODO(charlie): petgraph is a really heavy and unnecessary dependency here. We should
         // write our own graph, given that our requirements are so simple.

crates/puffin-resolver/src/resolver.rs

@@ -1,6 +1,6 @@
 //! Given a set of requirements, find a set of compatible packages.
 
-use std::borrow::Borrow;
+use std::cmp::Reverse;
 use std::collections::hash_map::Entry;
 use std::collections::BTreeMap;
 use std::future::Future;
@@ -34,6 +34,7 @@ use crate::distribution::{DistributionFile, SdistFile, WheelFile};
 use crate::error::ResolveError;
 use crate::manifest::Manifest;
 use crate::pubgrub::package::PubGrubPackage;
+use crate::pubgrub::priority::PubGrubPriority;
 use crate::pubgrub::version::{PubGrubVersion, MIN_VERSION};
 use crate::pubgrub::{iter_requirements, version_range};
 use crate::resolution::Graph;
@@ -130,32 +131,41 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
             // Run unit propagation.
             state.unit_propagation(next)?;
 
-            // Fetch the list of candidates.
+            // Pre-visit all candidate packages, to allow metadata to be fetched in parallel.
-            let Some(potential_packages) = state.partial_solution.potential_packages() else {
+            self.pre_visit(
-                let Some(selection) = state.partial_solution.extract_solution() else {
+                state.partial_solution.prioritized_packages(),
-                    return Err(PubGrubError::Failure(
+                &mut requested_versions,
-                        "How did we end up with no package to choose but no solution?".into(),
+                request_sink,
-                    )
+            )?;
-                    .into());
-                };
-
-                return Ok(Graph::from_state(&selection, &pins, &state));
-            };
 
             // Choose a package version.
-            let potential_packages = potential_packages.collect::<Vec<_>>();
+            let Some(highest_priority_pkg) = state
+                .partial_solution
+                .pick_highest_priority_pkg(|package, range| self.prioritize(package, range))
+            else {
+                let selection = state.partial_solution.extract_solution();
+                return Ok(Graph::from_state(&selection, &pins, &state));
+            };
+            next = highest_priority_pkg;
+
+            let term_intersection = state
+                .partial_solution
+                .term_intersection_for_package(&next)
+                .ok_or_else(|| {
+                    PubGrubError::Failure("a package was chosen but we don't have a term.".into())
+                })?;
             let decision = self
-                .choose_package_version(
+                .choose_version(
-                    potential_packages,
+                    &next,
+                    term_intersection.unwrap_positive(),
                     &mut pins,
                     &mut requested_versions,
                     request_sink,
                 )
                 .await?;
-            next = decision.0.clone();
 
             // Pick the next compatible version.
-            let version = match decision.1 {
+            let version = match decision {
                 None => {
                     debug!("No compatible version found for: {}", next);
 
@@ -226,19 +236,28 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
         }
     }
 
-    /// Given a set of candidate packages, choose the next package (and version) to add to the
+    #[allow(clippy::unused_self)]
-    /// partial solution.
+    fn prioritize(
-    async fn choose_package_version<T: Borrow<PubGrubPackage>, U: Borrow<Range<PubGrubVersion>>>(
         &self,
-        mut potential_packages: Vec<(T, U)>,
+        _package: &PubGrubPackage,
-        pins: &mut FxHashMap<PackageName, FxHashMap<pep440_rs::Version, File>>,
+        _range: &Range<PubGrubVersion>,
+    ) -> PubGrubPriority {
+        // TODO(charlie): Define a priority function.
+        Reverse(0)
+    }
+
+    /// Visit the set of candidate packages prior to selection. This allows us to fetch metadata for
+    /// all of the packages in parallel.
+    fn pre_visit(
+        &self,
+        packages: impl Iterator<Item = (&'a PubGrubPackage, &'a Range<PubGrubVersion>)>,
         in_flight: &mut FxHashSet<String>,
         request_sink: &futures::channel::mpsc::UnboundedSender<Request>,
-    ) -> Result<(T, Option<PubGrubVersion>), ResolveError> {
+    ) -> Result<(), ResolveError> {
         // Iterate over the potential packages, and fetch file metadata for any of them. These
         // represent our current best guesses for the versions that we _might_ select.
-        for (index, (package, range)) in potential_packages.iter().enumerate() {
+        for (package, range) in packages {
-            let PubGrubPackage::Package(package_name, _) = package.borrow() else {
+            let PubGrubPackage::Package(package_name, _) = package else {
                 continue;
             };
 
@@ -250,13 +269,9 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
 
             // Try to find a compatible version. If there aren't any compatible versions,
             // short-circuit and return `None`.
-            let Some(candidate) = self
+            let Some(candidate) = self.selector.select(package_name, range, version_map) else {
-                .selector
+                // Short-circuit: we couldn't find _any_ compatible versions for a package.
-                .select(package_name, range.borrow(), version_map)
+                return Ok(());
-            else {
-                // Short circuit: we couldn't find _any_ compatible versions for a package.
-                let (package, _range) = potential_packages.swap_remove(index);
-                return Ok((package, None));
             };
 
             // Emit a request to fetch the metadata for this version.
@@ -277,14 +292,21 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
                 }
             }
         }
+        Ok(())
+    }
 
-        // Always choose the first package.
+    /// Given a set of candidate packages, choose the next package (and version) to add to the
-        // TODO(charlie): Devise a better strategy here (for example: always choose the package with
+    /// partial solution.
-        // the fewest versions).
+    async fn choose_version(
-        let (package, range) = potential_packages.swap_remove(0);
+        &self,
-
+        package: &PubGrubPackage,
-        return match package.borrow() {
+        range: &Range<PubGrubVersion>,
-            PubGrubPackage::Root => Ok((package, Some(MIN_VERSION.clone()))),
+        pins: &mut FxHashMap<PackageName, FxHashMap<pep440_rs::Version, File>>,
+        in_flight: &mut FxHashSet<String>,
+        request_sink: &futures::channel::mpsc::UnboundedSender<Request>,
+    ) -> Result<Option<PubGrubVersion>, ResolveError> {
+        return match package {
+            PubGrubPackage::Root => Ok(Some(MIN_VERSION.clone())),
             PubGrubPackage::Package(package_name, _) => {
                 // Wait for the metadata to be available.
                 let entry = self.index.packages.wait(package_name).await.unwrap();
@@ -292,17 +314,13 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
 
                 debug!(
                     "Searching for a compatible version of {} ({})",
-                    package_name,
+                    package_name, range,
-                    range.borrow(),
                 );
 
                 // Find a compatible version.
-                let Some(candidate) =
+                let Some(candidate) = self.selector.select(package_name, range, version_map) else {
-                    self.selector
-                        .select(package_name, range.borrow(), version_map)
-                else {
                     // Short circuit: we couldn't find _any_ compatible versions for a package.
-                    return Ok((package, None));
+                    return Ok(None);
                 };
 
                 debug!(
@@ -340,7 +358,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
                 }
 
                 let version = candidate.version.clone();
-                Ok((package, Some(version)))
+                Ok(Some(version))
             }
         };
     }

scripts/benchmarks/requirements.in

@@ -6,25 +6,3 @@ pygls>=1.0.1
 lsprotocol>=2023.0.0a1
 ruff>=0.0.274
 typing_extensions
-scipy
-numpy
-pandas<2.0.0
-matplotlib>=3.0.0
-scikit-learn
-rich
-textual
-jupyter>=1.0.0,<2.0.0
-transformers[torch]
-django<4.0.0
-sqlalchemy
-psycopg2-binary
-trio<0.20
-trio-websocket
-trio-asyncio
-trio-typing
-trio-protocol
-fastapi
-typer
-pydantic
-uvicorn
-traitlets

vendor/pubgrub/Cargo.toml

@@ -22,6 +22,8 @@ include = ["Cargo.toml", "LICENSE", "README.md", "src/**", "tests/**", "examples
 [dependencies]
 thiserror = "1.0"
 rustc-hash = "1.1.0"
+indexmap = "2.0.2"
+priority-queue = "1.1.1"
 serde = { version = "1.0", features = ["derive"], optional = true }
 log = "0.4.14" # for debug logs in tests
 

vendor/pubgrub/examples/caching_dependency_provider.rs

@@ -32,13 +32,6 @@ impl<P: Package, VS: VersionSet, DP: DependencyProvider<P, VS>>
 impl<P: Package, VS: VersionSet, DP: DependencyProvider<P, VS>> DependencyProvider<P, VS>
     for CachingDependencyProvider<P, VS, DP>
 {
-    fn choose_package_version<T: std::borrow::Borrow<P>, U: std::borrow::Borrow<VS>>(
-        &self,
-        packages: impl Iterator<Item = (T, U)>,
-    ) -> Result<(T, Option<VS::V>), Box<dyn Error + Send + Sync>> {
-        self.remote_dependencies.choose_package_version(packages)
-    }
-
     // Caches dependencies if they were already queried
     fn get_dependencies(
         &self,
@@ -66,6 +59,20 @@ impl<P: Package, VS: VersionSet, DP: DependencyProvider<P, VS>> DependencyProvid
             error @ Err(_) => error,
         }
     }
+
+    fn choose_version(
+        &self,
+        package: &P,
+        range: &VS,
+    ) -> Result<Option<VS::V>, Box<dyn Error + Send + Sync>> {
+        self.remote_dependencies.choose_version(package, range)
+    }
+
+    type Priority = DP::Priority;
+
+    fn prioritize(&self, package: &P, range: &VS) -> Self::Priority {
+        self.remote_dependencies.prioritize(package, range)
+    }
 }
 
 fn main() {

vendor/pubgrub/src/error.rs

@@ -46,7 +46,7 @@ pub enum PubGrubError<P: Package, VS: VersionSet> {
     /// Error arising when the implementer of
     /// [DependencyProvider](crate::solver::DependencyProvider)
     /// returned an error in the method
-    /// [choose_package_version](crate::solver::DependencyProvider::choose_package_version).
+    /// [choose_version](crate::solver::DependencyProvider::choose_version).
     #[error("Decision making failed")]
     ErrorChoosingPackageVersion(Box<dyn std::error::Error + Send + Sync>),
 

vendor/pubgrub/src/internal/core.rs

@@ -20,7 +20,7 @@ use crate::version_set::VersionSet;
 
 /// Current state of the PubGrub algorithm.
 #[derive(Clone)]
-pub struct State<P: Package, VS: VersionSet> {
+pub struct State<P: Package, VS: VersionSet, Priority: Ord + Clone> {
     root_package: P,
     root_version: VS::V,
 
@@ -32,7 +32,7 @@ pub struct State<P: Package, VS: VersionSet> {
 
     /// Partial solution.
     /// TODO: remove pub.
-    pub partial_solution: PartialSolution<P, VS>,
+    pub partial_solution: PartialSolution<P, VS, Priority>,
 
     /// The store is the reference storage for all incompatibilities.
     pub incompatibility_store: Arena<Incompatibility<P, VS>>,
@@ -43,7 +43,7 @@ pub struct State<P: Package, VS: VersionSet> {
     unit_propagation_buffer: SmallVec<P>,
 }
 
-impl<P: Package, VS: VersionSet> State<P, VS> {
+impl<P: Package, VS: VersionSet, Priority: Ord + Clone> State<P, VS, Priority> {
     /// Initialization of PubGrub state.
     pub fn init(root_package: P, root_version: VS::V) -> Self {
         let mut incompatibility_store = Arena::new();

vendor/pubgrub/src/internal/partial_solution.rs

@@ -1,20 +1,26 @@
 // SPDX-License-Identifier: MPL-2.0
 
 //! A Memory acts like a structured partial solution
-//! where terms are regrouped by package in a [Map].
+//! where terms are regrouped by package in a [Map](crate::type_aliases::Map).
 
 use std::fmt::Display;
+use std::hash::BuildHasherDefault;
+
+use priority_queue::PriorityQueue;
+use rustc_hash::FxHasher;
 
 use crate::internal::arena::Arena;
 use crate::internal::incompatibility::{IncompId, Incompatibility, Relation};
 use crate::internal::small_map::SmallMap;
 use crate::package::Package;
 use crate::term::Term;
-use crate::type_aliases::{Map, SelectedDependencies};
+use crate::type_aliases::SelectedDependencies;
 use crate::version_set::VersionSet;
 
 use super::small_vec::SmallVec;
 
+type FnvIndexMap<K, V> = indexmap::IndexMap<K, V, BuildHasherDefault<rustc_hash::FxHasher>>;
+
 #[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)]
 pub struct DecisionLevel(pub u32);
 
@@ -27,13 +33,29 @@ impl DecisionLevel {
 /// The partial solution contains all package assignments,
 /// organized by package and historically ordered.
 #[derive(Clone, Debug)]
-pub struct PartialSolution<P: Package, VS: VersionSet> {
+pub struct PartialSolution<P: Package, VS: VersionSet, Priority: Ord + Clone> {
     next_global_index: u32,
     current_decision_level: DecisionLevel,
-    package_assignments: Map<P, PackageAssignments<P, VS>>,
+    /// `package_assignments` is primarily a HashMap from a package to its
+    /// `PackageAssignments`. But it can also keep the items in an order.
+    ///  We maintain three sections in this order:
+    /// 1. `[..current_decision_level]` Are packages that have had a decision made sorted by the `decision_level`.
+    ///    This makes it very efficient to extract the solution, And to backtrack to a particular decision level.
+    /// 2. `[current_decision_level..changed_this_decision_level]` Are packages that have **not** had there assignments
+    ///    changed since the last time `prioritize` has bean called. Within this range there is no sorting.
+    /// 3. `[changed_this_decision_level..]` Containes all packages that **have** had there assignments changed since
+    ///    the last time `prioritize` has bean called. The inverse is not necessarily true, some packages in the range
+    ///    did not have a change. Within this range there is no sorting.
+    package_assignments: FnvIndexMap<P, PackageAssignments<P, VS>>,
+    /// `prioritized_potential_packages` is primarily a HashMap from a package with no desition and a positive assignment
+    /// to its `Priority`. But, it also maintains a max heap of packages by `Priority` order.
+    prioritized_potential_packages: PriorityQueue<P, Priority, BuildHasherDefault<FxHasher>>,
+    changed_this_decision_level: usize,
 }
 
-impl<P: Package, VS: VersionSet> Display for PartialSolution<P, VS> {
+impl<P: Package, VS: VersionSet, Priority: Ord + Clone> Display
+    for PartialSolution<P, VS, Priority>
+{
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         let mut assignments: Vec<_> = self
             .package_assignments
@@ -120,13 +142,15 @@ pub enum SatisfierSearch<P: Package, VS: VersionSet> {
     },
 }
 
-impl<P: Package, VS: VersionSet> PartialSolution<P, VS> {
+impl<P: Package, VS: VersionSet, Priority: Ord + Clone> PartialSolution<P, VS, Priority> {
     /// Initialize an empty PartialSolution.
     pub fn empty() -> Self {
         Self {
             next_global_index: 0,
             current_decision_level: DecisionLevel(0),
-            package_assignments: Map::default(),
+            package_assignments: FnvIndexMap::default(),
+            prioritized_potential_packages: PriorityQueue::default(),
+            changed_this_decision_level: 0,
         }
     }
 
@@ -141,21 +165,20 @@ impl<P: Package, VS: VersionSet> PartialSolution<P, VS> {
                     AssignmentsIntersection::Decision(_) => panic!("Already existing decision"),
                     // Cannot be called if the versions is not contained in the terms intersection.
                     AssignmentsIntersection::Derivations(term) => {
-                        debug_assert!(
+                        debug_assert!(term.contains(&version))
-                            term.contains(&version),
-                            "{}: {} was expected to be contained in {}",
-                            package,
-                            version,
-                            term,
-                        )
                     }
                 },
             }
+            assert_eq!(
+                self.changed_this_decision_level,
+                self.package_assignments.len()
+            );
         }
+        let new_idx = self.current_decision_level.0 as usize;
         self.current_decision_level = self.current_decision_level.increment();
-        let pa = self
+        let (old_idx, _, pa) = self
             .package_assignments
-            .get_mut(&package)
+            .get_full_mut(&package)
             .expect("Derivations must already exist");
         pa.highest_decision_level = self.current_decision_level;
         pa.assignments_intersection = AssignmentsIntersection::Decision((
@@ -163,6 +186,10 @@ impl<P: Package, VS: VersionSet> PartialSolution<P, VS> {
             version.clone(),
             Term::exact(version),
         ));
+        // Maintain that the beginning of the `package_assignments` Have all decisions in sorted order.
+        if new_idx != old_idx {
+            self.package_assignments.swap_indices(new_idx, old_idx);
+        }
         self.next_global_index += 1;
     }
 
@@ -173,7 +200,7 @@ impl<P: Package, VS: VersionSet> PartialSolution<P, VS> {
         cause: IncompId<P, VS>,
         store: &Arena<Incompatibility<P, VS>>,
     ) {
-        use std::collections::hash_map::Entry;
+        use indexmap::map::Entry;
         let term = store[cause].get(&package).unwrap().negate();
         let dated_derivation = DatedDerivation {
             global_index: self.next_global_index,
@@ -181,8 +208,10 @@ impl<P: Package, VS: VersionSet> PartialSolution<P, VS> {
             cause,
         };
         self.next_global_index += 1;
+        let pa_last_index = self.package_assignments.len().saturating_sub(1);
         match self.package_assignments.entry(package) {
             Entry::Occupied(mut occupied) => {
+                let idx = occupied.index();
                 let pa = occupied.get_mut();
                 pa.highest_decision_level = self.current_decision_level;
                 match &mut pa.assignments_intersection {
@@ -192,11 +221,21 @@ impl<P: Package, VS: VersionSet> PartialSolution<P, VS> {
                     }
                     AssignmentsIntersection::Derivations(t) => {
                         *t = t.intersection(&term);
+                        if t.is_positive() {
+                            // we can use `swap_indices` to make `changed_this_decision_level` only go down by 1
+                            // but the copying is slower then the larger search
+                            self.changed_this_decision_level =
+                                std::cmp::min(self.changed_this_decision_level, idx);
+                        }
                     }
                 }
                 pa.dated_derivations.push(dated_derivation);
             }
             Entry::Vacant(v) => {
+                if term.is_positive() {
+                    self.changed_this_decision_level =
+                        std::cmp::min(self.changed_this_decision_level, pa_last_index);
+                }
                 v.insert(PackageAssignments {
                     smallest_decision_level: self.current_decision_level,
                     highest_decision_level: self.current_decision_level,
@@ -207,43 +246,66 @@ impl<P: Package, VS: VersionSet> PartialSolution<P, VS> {
         }
     }
 
-    /// Extract potential packages for the next iteration of unit propagation.
+    pub fn prioritized_packages(&self) -> impl Iterator<Item = (&P, &VS)> {
-    /// Return `None` if there is no suitable package anymore, which stops the algorithm.
+        let check_all = self.changed_this_decision_level
-    /// A package is a potential pick if there isn't an already
+            == self.current_decision_level.0.saturating_sub(1) as usize;
-    /// selected version (no "decision")
+        let current_decision_level = self.current_decision_level;
-    /// and if it contains at least one positive derivation term
+        self.package_assignments
-    /// in the partial solution.
+            .get_range(self.changed_this_decision_level..)
-    pub fn potential_packages(&self) -> Option<impl Iterator<Item = (&P, &VS)>> {
+            .unwrap()
-        let mut iter = self
-            .package_assignments
             .iter()
+            .filter(move |(_, pa)| {
+                // We only actually need to update the package if its Been changed
+                // since the last time we called prioritize.
+                // Which means it's highest decision level is the current decision level,
+                // or if we backtracked in the mean time.
+                check_all || pa.highest_decision_level == current_decision_level
+            })
             .filter_map(|(p, pa)| pa.assignments_intersection.potential_package_filter(p))
-            .peekable();
+    }
-        if iter.peek().is_some() {
+
-            Some(iter)
+    pub fn pick_highest_priority_pkg(
-        } else {
+        &mut self,
-            None
+        prioritizer: impl Fn(&P, &VS) -> Priority,
-        }
+    ) -> Option<P> {
+        let check_all = self.changed_this_decision_level
+            == self.current_decision_level.0.saturating_sub(1) as usize;
+        let current_decision_level = self.current_decision_level;
+        let prioritized_potential_packages = &mut self.prioritized_potential_packages;
+        self.package_assignments
+            .get_range(self.changed_this_decision_level..)
+            .unwrap()
+            .iter()
+            .filter(|(_, pa)| {
+                // We only actually need to update the package if its Been changed
+                // since the last time we called prioritize.
+                // Which means it's highest decision level is the current decision level,
+                // or if we backtracked in the mean time.
+                check_all || pa.highest_decision_level == current_decision_level
+            })
+            .filter_map(|(p, pa)| pa.assignments_intersection.potential_package_filter(p))
+            .for_each(|(p, r)| {
+                let priority = prioritizer(p, r);
+                prioritized_potential_packages.push(p.clone(), priority);
+            });
+        self.changed_this_decision_level = self.package_assignments.len();
+        prioritized_potential_packages.pop().map(|(p, _)| p)
     }
 
     /// If a partial solution has, for every positive derivation,
     /// a corresponding decision that satisfies that assignment,
     /// it's a total solution and version solving has succeeded.
-    pub fn extract_solution(&self) -> Option<SelectedDependencies<P, VS::V>> {
+    pub fn extract_solution(&self) -> SelectedDependencies<P, VS::V> {
-        let mut solution = Map::default();
+        self.package_assignments
-        for (p, pa) in &self.package_assignments {
+            .iter()
-            match &pa.assignments_intersection {
+            .take(self.current_decision_level.0 as usize)
-                AssignmentsIntersection::Decision((_, v, _)) => {
+            .map(|(p, pa)| match &pa.assignments_intersection {
-                    solution.insert(p.clone(), v.clone());
+                AssignmentsIntersection::Decision((_, v, _)) => (p.clone(), v.clone()),
+                AssignmentsIntersection::Derivations(_) => {
+                    panic!("Derivations in the Decision part")
                 }
-                AssignmentsIntersection::Derivations(term) => {
+            })
-                    if term.is_positive() {
+            .collect()
-                        return None;
-                    }
-                }
-            }
-        }
-        Some(solution)
     }
 
     /// Backtrack the partial solution to a given decision level.
@@ -290,6 +352,9 @@ impl<P: Package, VS: VersionSet> PartialSolution<P, VS> {
                 true
             }
         });
+        // Throw away all stored priority levels, And mark that they all need to be recomputed.
+        self.prioritized_potential_packages.clear();
+        self.changed_this_decision_level = self.current_decision_level.0.saturating_sub(1) as usize;
     }
 
     /// We can add the version to the partial solution as a decision
@@ -386,7 +451,7 @@ impl<P: Package, VS: VersionSet> PartialSolution<P, VS> {
     /// to return a coherent previous_satisfier_level.
     fn find_satisfier(
         incompat: &Incompatibility<P, VS>,
-        package_assignments: &Map<P, PackageAssignments<P, VS>>,
+        package_assignments: &FnvIndexMap<P, PackageAssignments<P, VS>>,
         store: &Arena<Incompatibility<P, VS>>,
     ) -> SmallMap<P, (usize, u32, DecisionLevel)> {
         let mut satisfied = SmallMap::Empty;
@@ -407,7 +472,7 @@ impl<P: Package, VS: VersionSet> PartialSolution<P, VS> {
         incompat: &Incompatibility<P, VS>,
         satisfier_package: &P,
         mut satisfied_map: SmallMap<P, (usize, u32, DecisionLevel)>,
-        package_assignments: &Map<P, PackageAssignments<P, VS>>,
+        package_assignments: &FnvIndexMap<P, PackageAssignments<P, VS>>,
         store: &Arena<Incompatibility<P, VS>>,
     ) -> DecisionLevel {
         // First, let's retrieve the previous derivations and the initial accum_term.

vendor/pubgrub/src/lib.rs

@@ -75,7 +75,7 @@
 //! trait for our own type.
 //! Let's say that we will use [String] for packages,
 //! and [SemanticVersion](version::SemanticVersion) for versions.
-//! This may be done quite easily by implementing the two following functions.
+//! This may be done quite easily by implementing the three following functions.
 //! ```
 //! # use pubgrub::solver::{DependencyProvider, Dependencies};
 //! # use pubgrub::version::SemanticVersion;
@@ -89,7 +89,12 @@
 //! type SemVS = Range<SemanticVersion>;
 //!
 //! impl DependencyProvider<String, SemVS> for MyDependencyProvider {
-//!     fn choose_package_version<T: Borrow<String>, U: Borrow<SemVS>>(&self,packages: impl Iterator<Item=(T, U)>) -> Result<(T, Option<SemanticVersion>), Box<dyn Error + Send + Sync>> {
+//!     fn choose_version(&self, package: &String, range: &SemVS) -> Result<Option<SemanticVersion>, Box<dyn Error + Send + Sync>> {
+//!         unimplemented!()
+//!     }
+//!
+//!     type Priority = usize;
+//!     fn prioritize(&self, package: &String, range: &SemVS) -> Self::Priority {
 //!         unimplemented!()
 //!     }
 //!
@@ -104,18 +109,17 @@
 //! ```
 //!
 //! The first method
-//! [choose_package_version](crate::solver::DependencyProvider::choose_package_version)
+//! [choose_version](crate::solver::DependencyProvider::choose_version)
-//! chooses a package and available version compatible with the provided options.
+//! chooses a version compatible with the provided range for a package.
-//! A helper function
+//! The second method
-//! [choose_package_with_fewest_versions](crate::solver::choose_package_with_fewest_versions)
+//! [prioritize](crate::solver::DependencyProvider::prioritize)
-//! is provided for convenience
+//! in which order different packages should be chosen.
-//! in cases when lists of available versions for packages are easily obtained.
+//! Usually prioritizing packages
-//! The strategy of that helper function consists in choosing the package
+//! with the fewest number of compatible versions speeds up resolution.
-//! with the fewest number of compatible versions to speed up resolution.
 //! But in general you are free to employ whatever strategy suits you best
 //! to pick a package and a version.
 //!
-//! The second method [get_dependencies](crate::solver::DependencyProvider::get_dependencies)
+//! The third method [get_dependencies](crate::solver::DependencyProvider::get_dependencies)
 //! aims at retrieving the dependencies of a given package at a given version.
 //! Returns [None] if dependencies are unknown.
 //!

vendor/pubgrub/src/solver.rs

@@ -68,7 +68,7 @@
 //! to satisfy the dependencies of that package and version pair.
 //! If there is no solution, the reason will be provided as clear as possible.
 
-use std::borrow::Borrow;
+use std::cmp::Reverse;
 use std::collections::{BTreeMap, BTreeSet as Set};
 use std::error::Error;
 
@@ -103,30 +103,27 @@ pub fn resolve<P: Package, VS: VersionSet>(
             state.partial_solution
         );
 
-        let Some(potential_packages) = state.partial_solution.potential_packages() else {
+        let Some(highest_priority_pkg) = state
-            return state.partial_solution.extract_solution().ok_or_else(|| {
+            .partial_solution
-                PubGrubError::Failure(
+            .pick_highest_priority_pkg(|p, r| dependency_provider.prioritize(p, r))
-                    "How did we end up with no package to choose but no solution?".into(),
+        else {
-                )
+            return Ok(state.partial_solution.extract_solution());
-            });
         };
+        next = highest_priority_pkg;
 
-        let decision = dependency_provider
-            .choose_package_version(potential_packages)
-            .map_err(PubGrubError::ErrorChoosingPackageVersion)?;
-        info!("DP chose: {} @ {:?}", decision.0, decision.1);
-
-        next = decision.0.clone();
-
-        // Pick the next compatible version.
         let term_intersection = state
             .partial_solution
             .term_intersection_for_package(&next)
             .ok_or_else(|| {
                 PubGrubError::Failure("a package was chosen but we don't have a term.".into())
             })?;
+        let decision = dependency_provider
+            .choose_version(&next, term_intersection.unwrap_positive())
+            .map_err(PubGrubError::ErrorChoosingPackageVersion)?;
+        info!("DP chose: {} @ {:?}", next, decision);
 
-        let v = match decision.1 {
+        // Pick the next compatible version.
+        let v = match decision {
             None => {
                 let inc = Incompatibility::no_versions(next.clone(), term_intersection.clone());
                 state.add_incompatibility(inc);
@@ -146,51 +143,53 @@ pub fn resolve<P: Package, VS: VersionSet>(
             .or_default()
             .insert(v.clone());
 
-        if !is_new_dependency {
+        if is_new_dependency {
+            // Retrieve that package dependencies.
+            let p = &next;
+            let dependencies = dependency_provider.get_dependencies(p, &v).map_err(|err| {
+                PubGrubError::ErrorRetrievingDependencies {
+                    package: p.clone(),
+                    version: v.clone(),
+                    source: err,
+                }
+            })?;
+
+            let known_dependencies = match dependencies {
+                Dependencies::Unknown => {
+                    state.add_incompatibility(Incompatibility::unavailable_dependencies(
+                        p.clone(),
+                        v.clone(),
+                    ));
+                    continue;
+                }
+                Dependencies::Known(x) if x.contains_key(p) => {
+                    return Err(PubGrubError::SelfDependency {
+                        package: p.clone(),
+                        version: v.clone(),
+                    });
+                }
+                Dependencies::Known(x) => x,
+            };
+
+            // Add that package and version if the dependencies are not problematic.
+            let dep_incompats = state.add_incompatibility_from_dependencies(
+                p.clone(),
+                v.clone(),
+                &known_dependencies,
+            );
+
+            state.partial_solution.add_version(
+                p.clone(),
+                v,
+                dep_incompats,
+                &state.incompatibility_store,
+            );
+        } else {
             // `dep_incompats` are already in `incompatibilities` so we know there are not satisfied
             // terms and can add the decision directly.
             info!("add_decision (not first time): {} @ {}", &next, v);
             state.partial_solution.add_decision(next.clone(), v);
-            continue;
         }
-
-        // Retrieve that package dependencies.
-        let p = &next;
-        let dependencies = dependency_provider.get_dependencies(p, &v).map_err(|err| {
-            PubGrubError::ErrorRetrievingDependencies {
-                package: p.clone(),
-                version: v.clone(),
-                source: err,
-            }
-        })?;
-
-        let known_dependencies = match dependencies {
-            Dependencies::Unknown => {
-                state.add_incompatibility(Incompatibility::unavailable_dependencies(
-                    p.clone(),
-                    v.clone(),
-                ));
-                continue;
-            }
-            Dependencies::Known(x) if x.contains_key(p) => {
-                return Err(PubGrubError::SelfDependency {
-                    package: p.clone(),
-                    version: v.clone(),
-                });
-            }
-            Dependencies::Known(x) => x,
-        };
-
-        // Add that package and version if the dependencies are not problematic.
-        let dep_incompats =
-            state.add_incompatibility_from_dependencies(p.clone(), v.clone(), &known_dependencies);
-
-        state.partial_solution.add_version(
-            p.clone(),
-            v,
-            dep_incompats,
-            &state.incompatibility_store,
-        );
     }
 }
 
@@ -210,11 +209,15 @@ pub trait DependencyProvider<P: Package, VS: VersionSet> {
     /// [Decision making](https://github.com/dart-lang/pub/blob/master/doc/solver.md#decision-making)
     /// is the process of choosing the next package
     /// and version that will be appended to the partial solution.
-    /// Every time such a decision must be made,
-    /// potential valid packages and sets of versions are preselected by the resolver,
-    /// and the dependency provider must choose.
     ///
-    /// The strategy employed to choose such package and version
+    /// Every time such a decision must be made, the resolver looks at all the potential valid
+    /// packages that have changed, and a asks the dependency provider how important each one is.
+    /// For each one it calls `prioritize` with the name of the package and the current set of
+    /// acceptable versions.
+    /// The resolver will then pick the package with the highes priority from all the potential valid
+    /// packages.
+    ///
+    /// The strategy employed to prioritize packages
     /// cannot change the existence of a solution or not,
     /// but can drastically change the performances of the solver,
     /// or the properties of the solution.
@@ -227,16 +230,24 @@ pub trait DependencyProvider<P: Package, VS: VersionSet> {
     /// > since these packages will run out of versions to try more quickly.
     /// > But there's likely room for improvement in these heuristics.
     ///
-    /// A helper function [choose_package_with_fewest_versions] is provided to ease
+    /// Note: the resolver may call this even when the range has not change,
-    /// implementations of this method if you can produce an iterator
+    /// if it is more efficient for the resolveres internal data structures.
-    /// of the available versions in preference order for any package.
+    fn prioritize(&self, package: &P, range: &VS) -> Self::Priority;
+    /// The type returned from `prioritize`. The resolver does not care what type this is
+    /// as long as it can pick a largest one and clone it.
     ///
-    /// Note: the type `T` ensures that this returns an item from the `packages` argument.
+    /// [std::cmp::Reverse] can be useful if you want to pick the package with
-    #[allow(clippy::type_complexity)]
+    /// the fewest versions that match the outstanding constraint.
-    fn choose_package_version<T: Borrow<P>, U: Borrow<VS>>(
+    type Priority: Ord + Clone;
+
+    /// Once the resolver has found the highest `Priority` package from all potential valid
+    /// packages, it needs to know what vertion of that package to use. The most common pattern
+    /// is to select the largest vertion that the range contains.
+    fn choose_version(
         &self,
-        potential_packages: impl Iterator<Item = (T, U)>,
+        package: &P,
-    ) -> Result<(T, Option<VS::V>), Box<dyn Error + Send + Sync>>;
+        range: &VS,
+    ) -> Result<Option<VS::V>, Box<dyn Error + Send + Sync>>;
 
     /// Retrieves the package dependencies.
     /// Return [Dependencies::Unknown] if its dependencies are unknown.
@@ -256,35 +267,6 @@ pub trait DependencyProvider<P: Package, VS: VersionSet> {
     }
 }
 
-/// This is a helper function to make it easy to implement
-/// [DependencyProvider::choose_package_version].
-/// It takes a function `list_available_versions` that takes a package and returns an iterator
-/// of the available versions in preference order.
-/// The helper finds the package from the `packages` argument with the fewest versions from
-/// `list_available_versions` contained in the constraints. Then takes that package and finds the
-/// first version contained in the constraints.
-pub fn choose_package_with_fewest_versions<P: Package, VS: VersionSet, T, U, I, F>(
-    list_available_versions: F,
-    potential_packages: impl Iterator<Item = (T, U)>,
-) -> (T, Option<VS::V>)
-where
-    T: Borrow<P>,
-    U: Borrow<VS>,
-    I: Iterator<Item = VS::V>,
-    F: Fn(&P) -> I,
-{
-    let count_valid = |(p, set): &(T, U)| {
-        list_available_versions(p.borrow())
-            .filter(|v| set.borrow().contains(v))
-            .count()
-    };
-    let (pkg, set) = potential_packages
-        .min_by_key(count_valid)
-        .expect("potential_packages gave us an empty iterator");
-    let version = list_available_versions(pkg.borrow()).find(|v| set.borrow().contains(v));
-    (pkg, version)
-}
-
 /// A basic implementation of [DependencyProvider].
 #[derive(Debug, Clone, Default)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
@@ -354,25 +336,29 @@ impl<P: Package, VS: VersionSet> OfflineDependencyProvider<P, VS> {
 
 /// An implementation of [DependencyProvider] that
 /// contains all dependency information available in memory.
-/// Packages are picked with the fewest versions contained in the constraints first.
+/// Currently packages are picked with the fewest versions contained in the constraints first.
+/// But, that may change in new versions if better heuristics are found.
 /// Versions are picked with the newest versions first.
 impl<P: Package, VS: VersionSet> DependencyProvider<P, VS> for OfflineDependencyProvider<P, VS> {
-    #[allow(clippy::type_complexity)]
+    fn choose_version(
-    fn choose_package_version<T: Borrow<P>, U: Borrow<VS>>(
         &self,
-        potential_packages: impl Iterator<Item = (T, U)>,
+        package: &P,
-    ) -> Result<(T, Option<VS::V>), Box<dyn Error + Send + Sync>> {
+        range: &VS,
-        Ok(choose_package_with_fewest_versions(
+    ) -> Result<Option<VS::V>, Box<dyn Error + Send + Sync>> {
-            |p| {
+        Ok(self
-                self.dependencies
+            .dependencies
-                    .get(p)
+            .get(package)
-                    .into_iter()
+            .and_then(|versions| versions.keys().rev().find(|v| range.contains(v)).cloned()))
-                    .flat_map(|k| k.keys())
+    }
-                    .rev()
+
-                    .cloned()
+    type Priority = Reverse<usize>;
-            },
+    fn prioritize(&self, package: &P, range: &VS) -> Self::Priority {
-            potential_packages,
+        Reverse(
-        ))
+            self.dependencies
+                .get(package)
+                .map(|versions| versions.keys().filter(|v| range.contains(v)).count())
+                .unwrap_or(0),
+        )
     }
 
     fn get_dependencies(

vendor/pubgrub/src/term.rs

@@ -64,7 +64,7 @@ impl<VS: VersionSet> Term<VS> {
 
     /// Unwrap the set contained in a positive term.
     /// Will panic if used on a negative set.
-    pub(crate) fn unwrap_positive(&self) -> &VS {
+    pub fn unwrap_positive(&self) -> &VS {
         match self {
             Self::Positive(set) => set,
             _ => panic!("Negative term cannot unwrap positive set"),

vendor/pubgrub/src/version.rs

@@ -121,7 +121,7 @@ impl SemanticVersion {
 }
 
 /// Error creating [SemanticVersion] from [String].
-#[derive(Error, Debug, PartialEq)]
+#[derive(Error, Debug, PartialEq, Eq)]
 pub enum VersionParseError {
     /// [SemanticVersion] must contain major, minor, patch versions.
     #[error("version {full_version} must contain 3 numbers separated by dot")]

vendor/pubgrub/tests/proptest.rs

@@ -6,10 +6,7 @@ use pubgrub::error::PubGrubError;
 use pubgrub::package::Package;
 use pubgrub::range::Range;
 use pubgrub::report::{DefaultStringReporter, Reporter};
-use pubgrub::solver::{
+use pubgrub::solver::{resolve, Dependencies, DependencyProvider, OfflineDependencyProvider};
-    choose_package_with_fewest_versions, resolve, Dependencies, DependencyProvider,
-    OfflineDependencyProvider,
-};
 use pubgrub::version::{NumberVersion, SemanticVersion};
 use pubgrub::version_set::VersionSet;
 
@@ -32,16 +29,6 @@ struct OldestVersionsDependencyProvider<P: Package, VS: VersionSet>(
 impl<P: Package, VS: VersionSet> DependencyProvider<P, VS>
     for OldestVersionsDependencyProvider<P, VS>
 {
-    fn choose_package_version<T: std::borrow::Borrow<P>, U: std::borrow::Borrow<VS>>(
-        &self,
-        potential_packages: impl Iterator<Item = (T, U)>,
-    ) -> Result<(T, Option<VS::V>), Box<dyn Error + Send + Sync>> {
-        Ok(choose_package_with_fewest_versions(
-            |p| self.0.versions(p).into_iter().flatten().cloned(),
-            potential_packages,
-        ))
-    }
-
     fn get_dependencies(
         &self,
         p: &P,
@@ -49,6 +36,26 @@ impl<P: Package, VS: VersionSet> DependencyProvider<P, VS>
     ) -> Result<Dependencies<P, VS>, Box<dyn Error + Send + Sync>> {
         self.0.get_dependencies(p, v)
     }
+
+    fn choose_version(
+        &self,
+        package: &P,
+        range: &VS,
+    ) -> Result<Option<VS::V>, Box<dyn Error + Send + Sync>> {
+        Ok(self
+            .0
+            .versions(package)
+            .into_iter()
+            .flatten()
+            .find(|&v| range.contains(v))
+            .cloned())
+    }
+
+    type Priority = <OfflineDependencyProvider<P, VS> as DependencyProvider<P, VS>>::Priority;
+
+    fn prioritize(&self, package: &P, range: &VS) -> Self::Priority {
+        self.0.prioritize(package, range)
+    }
 }
 
 /// The same as DP but it has a timeout.
@@ -74,13 +81,6 @@ impl<DP> TimeoutDependencyProvider<DP> {
 impl<P: Package, VS: VersionSet, DP: DependencyProvider<P, VS>> DependencyProvider<P, VS>
     for TimeoutDependencyProvider<DP>
 {
-    fn choose_package_version<T: std::borrow::Borrow<P>, U: std::borrow::Borrow<VS>>(
-        &self,
-        potential_packages: impl Iterator<Item = (T, U)>,
-    ) -> Result<(T, Option<VS::V>), Box<dyn Error + Send + Sync>> {
-        self.dp.choose_package_version(potential_packages)
-    }
-
     fn get_dependencies(
         &self,
         p: &P,
@@ -96,6 +96,20 @@ impl<P: Package, VS: VersionSet, DP: DependencyProvider<P, VS>> DependencyProvid
         self.call_count.set(calls + 1);
         Ok(())
     }
+
+    fn choose_version(
+        &self,
+        package: &P,
+        range: &VS,
+    ) -> Result<Option<VS::V>, Box<dyn Error + Send + Sync>> {
+        self.dp.choose_version(package, range)
+    }
+
+    type Priority = DP::Priority;
+
+    fn prioritize(&self, package: &P, range: &VS) -> Self::Priority {
+        self.dp.prioritize(package, range)
+    }
 }
 
 type NumVS = Range<NumberVersion>;