Skip to content
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.

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

Return and track affected and culprit on conflicts #298

Merged
merged 4 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
15 changes: 12 additions & 3 deletions examples/caching_dependency_provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@

use std::cell::RefCell;

use pubgrub::{resolve, Dependencies, DependencyProvider, OfflineDependencyProvider, Ranges};
use pubgrub::{
resolve, Dependencies, DependencyProvider, OfflineDependencyProvider,
PackageResolutionStatistics, Ranges,
};

type NumVS = Ranges<u32>;

Expand Down Expand Up @@ -57,8 +60,14 @@ impl<DP: DependencyProvider<M = String>> DependencyProvider for CachingDependenc

type Priority = DP::Priority;

fn prioritize(&self, package: &DP::P, ranges: &DP::VS) -> Self::Priority {
self.remote_dependencies.prioritize(package, ranges)
fn prioritize(
&self,
package: &Self::P,
range: &Self::VS,
package_statistics: &PackageResolutionStatistics,
) -> Self::Priority {
self.remote_dependencies
.prioritize(package, range, package_statistics)
}

type Err = DP::Err;
Expand Down
10 changes: 8 additions & 2 deletions src/internal/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,16 @@ impl<DP: DependencyProvider> State<DP> {

/// Unit propagation is the core mechanism of the solving algorithm.
/// CF <https://github.com/dart-lang/pub/blob/master/doc/solver.md#unit-propagation>
///
/// For each package with a satisfied incompatibility, returns the package and the root cause
/// incompatibility.
#[cold]
#[allow(clippy::type_complexity)] // Type definitions don't support impl trait.
pub(crate) fn unit_propagation(
&mut self,
package: Id<DP::P>,
) -> Result<(), NoSolutionError<DP>> {
) -> Result<Vec<(Id<DP::P>, IncompDpId<DP>)>, NoSolutionError<DP>> {
let mut root_causes = Vec::new();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: like unit_propagation_buffer I bet this Vec ends up expensive to allocate.

Copy link
Member Author

@konstin konstin Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it for a smallvec (it's used a lot less than unit_propagation_buffer)

self.unit_propagation_buffer.clear();
self.unit_propagation_buffer.push(package);
while let Some(current_package) = self.unit_propagation_buffer.pop() {
Expand Down Expand Up @@ -168,6 +173,7 @@ impl<DP: DependencyProvider> State<DP> {
.map_err(|terminal_incompat_id| {
self.build_derivation_tree(terminal_incompat_id)
})?;
root_causes.push((package, root_cause));
self.unit_propagation_buffer.clear();
self.unit_propagation_buffer.push(package_almost);
// Add to the partial solution with incompat as cause.
Expand All @@ -183,7 +189,7 @@ impl<DP: DependencyProvider> State<DP> {
}
}
// If there are no more changed packages, unit propagation is done.
Ok(())
Ok(root_causes)
}

/// Return the root cause or the terminal incompatibility.
Expand Down
67 changes: 43 additions & 24 deletions src/internal/partial_solution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ impl<DP: DependencyProvider> PartialSolution<DP> {
#[cold]
pub(crate) fn pick_highest_priority_pkg(
&mut self,
prioritizer: impl Fn(Id<DP::P>, &DP::VS) -> DP::Priority,
mut prioritizer: impl FnMut(Id<DP::P>, &DP::VS) -> DP::Priority,
) -> Option<Id<DP::P>> {
let check_all = self.changed_this_decision_level
== self.current_decision_level.0.saturating_sub(1) as usize;
Expand Down Expand Up @@ -304,7 +304,21 @@ impl<DP: DependencyProvider> PartialSolution<DP> {
.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
)
}
})
}
Expand Down Expand Up @@ -362,34 +376,39 @@ impl<DP: DependencyProvider> PartialSolution<DP> {
version: DP::V,
new_incompatibilities: std::ops::Range<IncompId<DP::P, DP::VS, DP::M>>,
store: &Arena<Incompatibility<DP::P, DP::VS, DP::M>>,
) {
) -> Option<IncompId<DP::P, DP::VS, DP::M>> {
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: IncompId<DP::P, DP::VS, DP::M>| {
store[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) = Id::range_to_iter(new_incompatibilities)
.find(|incompat| relation(*incompat) == Relation::Satisfied)
{
log::info!(
"rejecting decision {package:?} @ {version} because its dependencies conflict"
);
Some(satisfied)
} 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<DP::P, DP::VS, DP::M>| {
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!("adding decision: {package:?} @ {version}");
self.add_decision(package, version);
None
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
//! and [SemanticVersion] for versions.
//! This may be done quite easily by implementing the three following functions.
//! ```
//! # use pubgrub::{DependencyProvider, Dependencies, SemanticVersion, Ranges, DependencyConstraints, Map};
//! # use pubgrub::{DependencyProvider, Dependencies, SemanticVersion, Ranges, DependencyConstraints, Map, PackageResolutionStatistics};
//! # use std::error::Error;
//! # use std::borrow::Borrow;
//! # use std::convert::Infallible;
Expand All @@ -86,7 +86,7 @@
//! }
//!
//! type Priority = usize;
//! fn prioritize(&self, package: &String, range: &SemVS) -> Self::Priority {
//! fn prioritize(&self, package: &String, range: &SemVS, conflicts_counts: &PackageResolutionStatistics) -> Self::Priority {
//! unimplemented!()
//! }
//!
Expand Down Expand Up @@ -227,7 +227,7 @@ pub use report::{
DefaultStringReportFormatter, DefaultStringReporter, DerivationTree, Derived, External,
ReportFormatter, Reporter,
};
pub use solver::{resolve, Dependencies, DependencyProvider};
pub use solver::{resolve, Dependencies, DependencyProvider, PackageResolutionStatistics};
pub use term::Term;
pub use type_aliases::{DependencyConstraints, Map, SelectedDependencies, Set};
pub use version::{SemanticVersion, VersionParseError};
Expand Down
23 changes: 16 additions & 7 deletions src/provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::cmp::Reverse;
use std::collections::BTreeMap;
use std::convert::Infallible;

use crate::solver::PackageResolutionStatistics;
use crate::{Dependencies, DependencyConstraints, DependencyProvider, Map, Package, VersionSet};

/// A basic implementation of [DependencyProvider].
Expand Down Expand Up @@ -92,15 +93,23 @@ impl<P: Package, VS: VersionSet> DependencyProvider for OfflineDependencyProvide
.and_then(|versions| versions.keys().rev().find(|v| range.contains(v)).cloned()))
}

type Priority = Reverse<usize>;
type Priority = (u32, Reverse<usize>);

#[inline]
fn prioritize(&self, package: &P, range: &VS) -> Self::Priority {
Reverse(
self.dependencies
.get(package)
.map(|versions| versions.keys().filter(|v| range.contains(v)).count())
.unwrap_or(0),
fn prioritize(
&self,
package: &Self::P,
range: &Self::VS,
package_statistics: &PackageResolutionStatistics,
) -> Self::Priority {
(
package_statistics.conflict_count(),
Reverse(
self.dependencies
.get(package)
.map(|versions| versions.keys().filter(|v| range.contains(v)).count())
.unwrap_or(0),
),
)
}

Expand Down
76 changes: 70 additions & 6 deletions src/solver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,31 @@ use log::{debug, info};
use crate::internal::{Id, Incompatibility, State};
use crate::{DependencyConstraints, Map, Package, PubGrubError, SelectedDependencies, VersionSet};

/// Statistics on how often a package conflicted with other packages.
#[derive(Debug, Default, Clone)]
pub struct PackageResolutionStatistics {
unit_propagation_affected: u32,
unit_propagation_culprit: u32,
dependencies_affected: u32,
dependencies_culprit: u32,
Copy link
Contributor

@x-hgg-x x-hgg-x Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dependencies_culprit is never set ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

}

impl PackageResolutionStatistics {
/// The number of conflicts this package was involved in.
///
/// Processing packages with a high conflict count earlier usually speeds up resolution.
///
/// Whenever a package is part of the root cause incompatibility of a conflict, we increase its
/// count by one. Since the structure of the incompatibilities may change, this count too may
/// change in the future.
pub fn conflict_count(&self) -> u32 {
self.unit_propagation_affected
+ self.unit_propagation_culprit
+ self.dependencies_affected
+ self.dependencies_culprit
}
}

/// Main function of the library.
/// Finds a set of packages satisfying dependency bounds for a given package + version pair.
#[cold]
Expand All @@ -77,6 +102,7 @@ pub fn resolve<DP: DependencyProvider>(
version: impl Into<DP::V>,
) -> Result<SelectedDependencies<DP>, PubGrubError<DP>> {
let mut state: State<DP> = State::init(package.clone(), version.into());
let mut conflict_tracker: Map<Id<DP::P>, PackageResolutionStatistics> = Map::default();
let mut added_dependencies: Map<Id<DP::P>, Set<DP::V>> = Map::default();
let mut next = state.root_package;
loop {
Expand All @@ -88,7 +114,22 @@ pub fn resolve<DP: DependencyProvider>(
"unit_propagation: {:?} = '{}'",
&next, state.package_store[next]
);
state.unit_propagation(next)?;
let root_causes = state.unit_propagation(next)?;
for (affected, incompat) in root_causes {
conflict_tracker
.entry(affected)
.or_default()
.unit_propagation_affected += 1;
for (conflict_package, _) in state.incompatibility_store[incompat].iter() {
if conflict_package == affected {
continue;
}
conflict_tracker
.entry(conflict_package)
.or_default()
.unit_propagation_culprit += 1;
}
}

debug!(
"Partial solution after unit propagation: {}",
Expand All @@ -97,7 +138,11 @@ pub fn resolve<DP: DependencyProvider>(

let Some(highest_priority_pkg) =
state.partial_solution.pick_highest_priority_pkg(|p, r| {
dependency_provider.prioritize(&state.package_store[p], r)
dependency_provider.prioritize(
&state.package_store[p],
r,
conflict_tracker.entry(p).or_default(),
)
})
else {
return Ok(state
Expand Down Expand Up @@ -174,9 +219,23 @@ pub fn resolve<DP: DependencyProvider>(
let dep_incompats =
state.add_incompatibility_from_dependencies(p, v.clone(), dependencies);

state
.partial_solution
.add_version(p, v, dep_incompats, &state.incompatibility_store);
if let Some(conflict) = state.partial_solution.add_version(
p,
v,
dep_incompats,
&state.incompatibility_store,
) {
conflict_tracker.entry(p).or_default().dependencies_affected += 1;
for (incompat_package, _) in state.incompatibility_store[conflict].iter() {
if incompat_package == p {
continue;
}
conflict_tracker
.entry(incompat_package)
.or_default()
.unit_propagation_culprit += 1;
}
}
} else {
// `dep_incompats` are already in `incompatibilities` so we know there are not satisfied
// terms and can add the decision directly.
Expand Down Expand Up @@ -254,7 +313,12 @@ pub trait DependencyProvider {
///
/// Note: the resolver may call this even when the range has not changed,
/// if it is more efficient for the resolvers internal data structures.
fn prioritize(&self, package: &Self::P, range: &Self::VS) -> Self::Priority;
fn prioritize(
&self,
package: &Self::P,
range: &Self::VS,
package_conflicts_counts: &PackageResolutionStatistics,
) -> 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.
///
Expand Down
22 changes: 16 additions & 6 deletions tests/proptest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ use proptest::string::string_regex;

use pubgrub::{
resolve, DefaultStringReporter, Dependencies, DependencyProvider, DerivationTree, External,
OfflineDependencyProvider, Package, PubGrubError, Ranges, Reporter, SelectedDependencies,
VersionSet,
OfflineDependencyProvider, Package, PackageResolutionStatistics, PubGrubError, Ranges,
Reporter, SelectedDependencies, VersionSet,
};

use crate::sat_dependency_provider::SatResolve;
Expand Down Expand Up @@ -49,8 +49,13 @@ impl<P: Package, VS: VersionSet> DependencyProvider for OldestVersionsDependency

type Priority = <OfflineDependencyProvider<P, VS> as DependencyProvider>::Priority;

fn prioritize(&self, package: &P, range: &VS) -> Self::Priority {
self.0.prioritize(package, range)
fn prioritize(
&self,
package: &Self::P,
range: &Self::VS,
package_statistics: &PackageResolutionStatistics,
) -> Self::Priority {
self.0.prioritize(package, range, package_statistics)
}

type Err = Infallible;
Expand Down Expand Up @@ -104,8 +109,13 @@ impl<DP: DependencyProvider> DependencyProvider for TimeoutDependencyProvider<DP

type Priority = DP::Priority;

fn prioritize(&self, package: &DP::P, range: &DP::VS) -> Self::Priority {
self.dp.prioritize(package, range)
fn prioritize(
&self,
package: &Self::P,
range: &Self::VS,
package_statistics: &PackageResolutionStatistics,
) -> Self::Priority {
self.dp.prioritize(package, range, package_statistics)
}

type Err = DP::Err;
Expand Down
Loading