From 5c9e11308199511c9305c7ef0b0e79628eb27d68 Mon Sep 17 00:00:00 2001 From: konstin Date: Thu, 12 Dec 2024 16:12:14 +0100 Subject: [PATCH 1/3] Return incompatibility from conflict Whenever we either discard a version due to its dependencies or perform conflict resolution, we return the last conflict that led to discarding them. In uv, we use this to re-prioritize and backtrack when a package decision accumulated to many conflicts. --- src/internal/core.rs | 37 ++++++++++---- src/internal/incompatibility.rs | 5 +- src/internal/mod.rs | 2 +- src/internal/partial_solution.rs | 86 ++++++++++++++++++++------------ 4 files changed, 85 insertions(+), 45 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index 26a699d9..c3fc7835 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -7,8 +7,8 @@ use std::collections::HashSet as Set; use std::sync::Arc; use crate::internal::{ - Arena, DecisionLevel, HashArena, Id, IncompDpId, Incompatibility, PartialSolution, Relation, - SatisfierSearch, SmallVec, + Arena, DecisionLevel, HashArena, Id, IncompDpId, IncompatIterItem, Incompatibility, + PartialSolution, Relation, SatisfierSearch, SmallVec, }; use crate::{DependencyProvider, DerivationTree, Map, NoSolutionError, VersionSet}; @@ -74,20 +74,24 @@ impl State { } /// Add the dependencies for the current version of the current package as incompatibilities. + /// + /// Returns the incompatibility that caused the current version to be rejected, if it was + /// rejected due to its dependencies. pub fn add_package_version_dependencies( &mut self, package: Id, version: DP::V, dependencies: impl IntoIterator, - ) { + ) -> Option>> { let dep_incompats = self.add_incompatibility_from_dependencies(package, version.clone(), dependencies); - self.partial_solution.add_package_version_incompatibilities( - package, - version.clone(), - dep_incompats, - &self.incompatibility_store, - ) + self.partial_solution + .check_package_version_incompatibilities( + package, + version.clone(), + dep_incompats, + &self.incompatibility_store, + ) } /// Add an incompatibility to the state. @@ -124,8 +128,18 @@ impl State { /// Unit propagation is the core mechanism of the solving algorithm. /// CF + /// + /// Returns the last incompatibility, if there was a conflict. #[cold] - pub fn unit_propagation(&mut self, package: Id) -> Result<(), NoSolutionError> { + #[allow(clippy::type_complexity)] // Type definitions don't support impl trait. + pub fn unit_propagation( + &mut self, + package: Id, + ) -> Result< + Option>>, + NoSolutionError, + > { + let mut last_incompat = None; self.unit_propagation_buffer.clear(); self.unit_propagation_buffer.push(package); while let Some(current_package) = self.unit_propagation_buffer.pop() { @@ -183,6 +197,7 @@ impl State { .map_err(|terminal_incompat_id| { self.build_derivation_tree(terminal_incompat_id) })?; + last_incompat = Some(root_cause); self.unit_propagation_buffer.clear(); self.unit_propagation_buffer.push(package_almost); // Add to the partial solution with incompat as cause. @@ -198,7 +213,7 @@ impl State { } } // If there are no more changed packages, unit propagation is done. - Ok(()) + Ok(last_incompat.map(|incompat| self.incompatibility_store[incompat].iter())) } /// Return the root cause or the terminal incompatibility. diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 75930b18..2b498cdb 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -12,6 +12,9 @@ use crate::{ VersionSet, }; +// An entry in an incompatibility when iterating over one. +pub(crate) type IncompatIterItem<'term, P, VS> = (Id

, &'term Term); + /// An incompatibility is a set of terms for different packages /// that should never be satisfied all together. /// An incompatibility usually originates from a package dependency. @@ -248,7 +251,7 @@ impl Incompatibilit } /// Iterate over packages. - pub(crate) fn iter(&self) -> impl Iterator, &Term)> { + pub(crate) fn iter(&self) -> impl Iterator> { self.package_terms .iter() .map(|(package, term)| (*package, term)) diff --git a/src/internal/mod.rs b/src/internal/mod.rs index eb94bf3f..0dab4336 100644 --- a/src/internal/mod.rs +++ b/src/internal/mod.rs @@ -10,7 +10,7 @@ mod small_map; mod small_vec; pub(crate) use arena::{Arena, HashArena}; -pub(crate) use incompatibility::{IncompDpId, IncompId, Relation}; +pub(crate) use incompatibility::{IncompDpId, IncompId, IncompatIterItem, Relation}; pub(crate) use partial_solution::{DecisionLevel, PartialSolution, SatisfierSearch}; pub(crate) use small_map::SmallMap; pub(crate) use small_vec::SmallVec; diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index d4444d51..ac081f08 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -10,7 +10,8 @@ use priority_queue::PriorityQueue; use rustc_hash::FxHasher; use crate::internal::{ - Arena, HashArena, Id, IncompDpId, IncompId, Incompatibility, Relation, SmallMap, SmallVec, + Arena, HashArena, Id, IncompDpId, IncompId, IncompatIterItem, Incompatibility, Relation, + SmallMap, SmallVec, }; use crate::{DependencyProvider, Package, Term, VersionSet}; @@ -323,7 +324,21 @@ impl PartialSolution { .map(|(&p, pa)| match &pa.assignments_intersection { AssignmentsIntersection::Decision((_, v, _)) => (p, v.clone()), AssignmentsIntersection::Derivations(_) => { - panic!("Derivations in the Decision part") + let mut context = String::new(); + for (id, assignment) in self + .package_assignments + .iter() + .take(self.current_decision_level.0 as usize) + { + context.push_str(&format!( + " * {:?} {:?}\n", + id, assignment.assignments_intersection + )); + } + panic!( + "Derivations in the Decision part. Decision level {}\n{}", + self.current_decision_level.0, context + ) } }) } @@ -370,45 +385,52 @@ impl PartialSolution { self.has_ever_backtracked = true; } - /// We can add the version to the partial solution as a decision - /// if it doesn't produce any conflict with the new incompatibilities. - /// In practice I think it can only produce a conflict if one of the dependencies - /// (which are used to make the new incompatibilities) - /// is already in the partial solution with an incompatible version. - pub(crate) fn add_package_version_incompatibilities( - &mut self, + /// Add a package version as decision if none of its dependencies conflicts with the partial + /// solution. + /// + /// If the resolution never backtracked before, a fast path adds the package version directly + /// without checking dependencies. + /// + /// Returns the incompatibility that caused the current version to be rejected, if a conflict + /// in the dependencies was found. + pub(crate) fn check_package_version_incompatibilities<'a>( + &'a mut self, package: Id, version: DP::V, new_incompatibilities: std::ops::Range>, - store: &Arena>, - ) { + store: &'a Arena>, + ) -> Option> + 'a> { if !self.has_ever_backtracked { - // Nothing has yet gone wrong during this resolution. This call is unlikely to be the first problem. + // Fast path: Nothing has yet gone wrong during this resolution. This call is unlikely to be the first problem. // So let's live with a little bit of risk and add the decision without checking the dependencies. // The worst that can happen is we will have to do a full backtrack which only removes this one decision. log::info!("add_decision: {package:?} @ {version} without checking dependencies"); self.add_decision(package, version); + return None; + } + + // Check if any of the dependencies preclude deciding on this crate version. + let package_term = Term::exact(version.clone()); + let relation = |incompat: &Incompatibility| { + incompat.relation(|p| { + // The current package isn't part of the package assignments yet. + if p == package { + Some(&package_term) + } else { + self.term_intersection_for_package(p) + } + }) + }; + if let Some(satisfied) = store[new_incompatibilities] + .iter() + .find(|incompat| relation(incompat) == Relation::Satisfied) + { + log::info!("not adding {package:?} @ {version} because its dependencies conflict"); + Some(satisfied.iter()) } else { - // Check if any of the new dependencies preclude deciding on this crate version. - let exact = Term::exact(version.clone()); - let not_satisfied = |incompat: &Incompatibility| { - incompat.relation(|p| { - if p == package { - Some(&exact) - } else { - self.term_intersection_for_package(p) - } - }) != Relation::Satisfied - }; - - // Check none of the dependencies (new_incompatibilities) - // would create a conflict (be satisfied). - if store[new_incompatibilities].iter().all(not_satisfied) { - log::info!("add_decision: {package:?} @ {version}"); - self.add_decision(package, version); - } else { - log::info!("not adding {package:?} @ {version} because of its dependencies",); - } + log::info!("add_decision: {package:?} @ {version}"); + self.add_decision(package, version); + None } } From 42722090261944956a57c3234a9cb072428258c4 Mon Sep 17 00:00:00 2001 From: konstin Date: Thu, 12 Dec 2024 13:45:46 +0100 Subject: [PATCH 2/3] Allow backtracking to before a specific package This allows discarding a previously made decision if it turned out to be a bad decision, even if all options with this decisions have not yet been rejected. We allow attempting to backtrack on packages that were not decided yet to avoid the caller from making the duplicative check. --- src/internal/core.rs | 18 +++++++++++++++++- src/internal/partial_solution.rs | 23 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index c3fc7835..11e5bfd6 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -264,7 +264,8 @@ impl State { } } - /// Backtracking. + /// After a conflict occurred, backtrack the partial solution to a given decision level, and add + /// the incompatibility if it was new. fn backtrack( &mut self, incompat: IncompDpId, @@ -280,6 +281,21 @@ impl State { } } + /// Manually backtrack before the given package was selected. + /// + /// This can be used to switch the order of packages if the previous prioritization was bad. + /// + /// Returns the number of the decisions that were backtracked, or `None` if the package was not + /// decided on yet. + pub fn backtrack_package(&mut self, package: Id) -> Option { + let base_decision_level = self.partial_solution.current_decision_level(); + let new_decision_level = self.partial_solution.backtrack_package(package).ok()?; + // Remove contradicted incompatibilities that depend on decisions we just backtracked away. + self.contradicted_incompatibilities + .retain(|_, dl| *dl <= new_decision_level); + Some(base_decision_level.0 - new_decision_level.0) + } + /// Add this incompatibility into the set of all incompatibilities. /// /// PubGrub collapses identical dependencies from adjacent package versions diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index ac081f08..a0d26c3b 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -6,6 +6,7 @@ use std::fmt::{Debug, Display}; use std::hash::BuildHasherDefault; +use log::debug; use priority_queue::PriorityQueue; use rustc_hash::FxHasher; @@ -385,6 +386,28 @@ impl PartialSolution { self.has_ever_backtracked = true; } + /// Backtrack the partial solution before a particular package was selected. + /// + /// This can be used to switch the order of packages if the previous prioritization was bad. + /// + /// Returns the new decision level on success and an error if the package was not decided on + /// yet. + pub(crate) fn backtrack_package(&mut self, package: Id) -> Result { + let Some(decision_level) = self.package_assignments.get_index_of(&package) else { + return Err(()); + }; + let decision_level = DecisionLevel(decision_level as u32); + if decision_level > self.current_decision_level { + return Err(()); + } + debug!( + "Package backtracking ot decision level {}", + decision_level.0 + ); + self.backtrack(decision_level); + Ok(decision_level) + } + /// Add a package version as decision if none of its dependencies conflicts with the partial /// solution. /// From 251e93bd42b9cf4705e633a2a95a137be4087358 Mon Sep 17 00:00:00 2001 From: konstin Date: Thu, 12 Dec 2024 13:42:19 +0100 Subject: [PATCH 3/3] Allow access to undecided packages Allow introspecting which packages are to be decided. uv currently uses this for (trace) logging in the resolver. --- src/internal/partial_solution.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index a0d26c3b..a98f190f 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -212,6 +212,13 @@ impl PartialSolution { self.next_global_index += 1; } + /// The list of package that have not been selected after the last prioritization. + /// + /// This list gets updated by [`Self::pick_highest_priority_pkg`] and cleared by backtracking. + pub fn undecided_packages(&self) -> impl Iterator, &DP::Priority)> { + self.prioritized_potential_packages.iter() + } + /// Add a derivation. pub(crate) fn add_derivation( &mut self,