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

Change backtracking when packages conflict too much #9843

Merged
merged 6 commits into from
Dec 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 2 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ 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 = "57832d0588fbb7aab824813481104761dc1c7740" }
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" }
Expand Down Expand Up @@ -175,7 +175,7 @@ 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 = "57832d0588fbb7aab824813481104761dc1c7740" }
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" }
Expand Down
14 changes: 9 additions & 5 deletions crates/uv-resolver/src/dependency_provider.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::convert::Infallible;

use pubgrub::{Dependencies, DependencyProvider, Range};
use pubgrub::{Dependencies, DependencyProvider, PackageResolutionStatistics, Range};

use uv_pep440::Version;

Expand All @@ -17,13 +17,17 @@ impl DependencyProvider for UvDependencyProvider {
type V = Version;
type VS = Range<Version>;
type M = UnavailableReason;
type Priority = Option<PubGrubPriority>;
type Err = Infallible;

fn prioritize(&self, _package: &Self::P, _range: &Self::VS) -> Self::Priority {
fn prioritize(
&self,
_package: &Self::P,
_range: &Self::VS,
_stats: &PackageResolutionStatistics,
) -> Self::Priority {
unimplemented!()
}
type Priority = Option<PubGrubPriority>;

type Err = Infallible;

fn choose_version(
&self,
Expand Down
112 changes: 104 additions & 8 deletions crates/uv-resolver/src/pubgrub/priority.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::cmp::Reverse;

use pubgrub::Range;
use rustc_hash::FxHashMap;
use std::cmp::Reverse;
use std::collections::hash_map::OccupiedEntry;

use crate::fork_urls::ForkUrls;
use uv_normalize::PackageName;
Expand Down Expand Up @@ -40,19 +40,22 @@ impl PubGrubPriorities {
match self.0.entry(name.clone()) {
std::collections::hash_map::Entry::Occupied(mut entry) => {
// Preserve the original index.
let index = match entry.get() {
PubGrubPriority::Unspecified(Reverse(index)) => *index,
PubGrubPriority::Singleton(Reverse(index)) => *index,
PubGrubPriority::DirectUrl(Reverse(index)) => *index,
PubGrubPriority::Root => next,
};
let index = Self::get_index(&entry).unwrap_or(next);

// Compute the priority.
let priority = if urls.get(name).is_some() {
PubGrubPriority::DirectUrl(Reverse(index))
} else if version.as_singleton().is_some() {
PubGrubPriority::Singleton(Reverse(index))
} else {
// Keep the conflict-causing packages to avoid loops where we seesaw between
// `Unspecified` and `Conflict*`.
if matches!(
entry.get(),
PubGrubPriority::ConflictEarly(_) | PubGrubPriority::ConflictLate(_)
) {
return;
}
PubGrubPriority::Unspecified(Reverse(index))
};

Expand All @@ -77,6 +80,17 @@ impl PubGrubPriorities {
}
}

fn get_index(entry: &OccupiedEntry<PackageName, PubGrubPriority>) -> Option<usize> {
match entry.get() {
PubGrubPriority::ConflictLate(Reverse(index))
| PubGrubPriority::Unspecified(Reverse(index))
| PubGrubPriority::ConflictEarly(Reverse(index))
| PubGrubPriority::Singleton(Reverse(index))
| PubGrubPriority::DirectUrl(Reverse(index)) => Some(*index),
PubGrubPriority::Root => None,
}
}

/// Return the [`PubGrubPriority`] of the given package, if it exists.
pub(crate) fn get(&self, package: &PubGrubPackage) -> Option<PubGrubPriority> {
match &**package {
Expand All @@ -88,6 +102,79 @@ impl PubGrubPriorities {
PubGrubPackageInner::Package { name, .. } => self.0.get(name).copied(),
}
}

/// Mark a package as prioritized by setting it to [`PubGrubPriority::ConflictEarly`], if it
/// doesn't have a higher priority already.
///
/// 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 Some(name) = package.name_no_root() else {
// Not a correctness bug
if cfg!(debug_assertions) {
panic!("URL packages must not be involved in conflict handling")
} else {
return false;
}
};
match self.0.entry(name.clone()) {
std::collections::hash_map::Entry::Occupied(mut entry) => {
if matches!(
entry.get(),
PubGrubPriority::ConflictEarly(_) | PubGrubPriority::Singleton(_)
) {
// Already in the right category
return false;
};
let index = Self::get_index(&entry).unwrap_or(next);
entry.insert(PubGrubPriority::ConflictEarly(Reverse(index)));
true
}
std::collections::hash_map::Entry::Vacant(entry) => {
entry.insert(PubGrubPriority::ConflictEarly(Reverse(next)));
true
}
}
}

/// Mark a package as prioritized by setting it to [`PubGrubPriority::ConflictLate`], if it
/// doesn't have a higher priority already.
///
/// 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 Some(name) = package.name_no_root() else {
// Not a correctness bug
if cfg!(debug_assertions) {
panic!("URL packages must not be involved in conflict handling")
} else {
return false;
}
};
match self.0.entry(name.clone()) {
std::collections::hash_map::Entry::Occupied(mut entry) => {
// The ConflictEarly` match avoids infinite loops.
if matches!(
entry.get(),
PubGrubPriority::ConflictLate(_)
| PubGrubPriority::ConflictEarly(_)
| PubGrubPriority::Singleton(_)
) {
// Already in the right category
return false;
};
let index = Self::get_index(&entry).unwrap_or(next);
entry.insert(PubGrubPriority::ConflictLate(Reverse(index)));
true
}
std::collections::hash_map::Entry::Vacant(entry) => {
entry.insert(PubGrubPriority::ConflictLate(Reverse(next)));
true
}
}
}
}

#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
Expand All @@ -101,6 +188,15 @@ pub(crate) enum PubGrubPriority {
/// in the dependency graph.
Unspecified(Reverse<usize>),

/// Selected version of this package were often the culprit of rejecting another package, so
/// it's deprioritized behind `ConflictEarly`. It's still the higher than `Unspecified` to
/// conflict before selecting unrelated packages.
ConflictLate(Reverse<usize>),

/// Selected version of this package were often rejected, so it's prioritized over
/// `ConflictLate`.
ConflictEarly(Reverse<usize>),

/// The version range is constrained to a single version (e.g., with the `==` operator).
Singleton(Reverse<usize>),

Expand Down
Loading
Loading