Skip to content

Commit

Permalink
Prefer local variants in preference selection (#11546)
Browse files Browse the repository at this point in the history
## Summary

This PR fixes a subtle issue arising from our propagation of
preferences. When we resolve a fork, we take the solution from that fork
and mark all the chosen versions as "preferred" as we move on to the
next fork.

In this specific case, the resolver ended up solving a macOS-specific
fork first, which led us to pick `2.6.0` rather than `2.6.0+cpu`. This
in itself is correct; but when we moved on to the next fork, we
preferred `2.6.0` over `2.6.0+cpu`, despite the fact that `2.6.0` _only_
includes macOS wheel, and that branch was focused on Linux.

Now, in preferences, we prefer local variants (if they exist). If the
local variant ends up not working, we'll presumedly backtrack to the
base version anyway.

Closes #11406.
  • Loading branch information
charliermarsh authored Feb 16, 2025
1 parent 08ad56e commit 47fb59f
Show file tree
Hide file tree
Showing 4 changed files with 448 additions and 5 deletions.
32 changes: 30 additions & 2 deletions crates/uv-resolver/src/candidate_selector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,10 +298,38 @@ impl CandidateSelector {
}

// Check for a remote distribution that matches the preferred version
if let Some(file) = version_maps
if let Some((version_map, file)) = version_maps
.iter()
.find_map(|version_map| version_map.get(version))
.find_map(|version_map| version_map.get(version).map(|dist| (version_map, dist)))
{
// If the preferred version has a local variant, prefer that.
if version_map.local() {
for local in version_map
.versions()
.rev()
.take_while(|local| *local > version)
{
if !local.is_local() {
continue;
}
if local.clone().without_local() != *version {
continue;
}
if !range.contains(local) {
continue;
}
if let Some(dist) = version_map.get(local) {
debug!("Preferring local version `{package_name}` (v{local})");
return Some(Candidate::new(
package_name,
local,
dist,
VersionChoiceKind::Preference,
));
}
}
}

return Some(Candidate::new(
package_name,
version,
Expand Down
21 changes: 19 additions & 2 deletions crates/uv-resolver/src/version_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ impl VersionMap {
build_options: &BuildOptions,
) -> Self {
let mut stable = false;
let mut local = false;
let mut map = BTreeMap::new();
// Create stubs for each entry in simple metadata. The full conversion
// from a `VersionFiles` to a PrioritizedDist for each version
Expand All @@ -62,6 +63,7 @@ impl VersionMap {
let version = rkyv::deserialize::<Version, rkyv::rancor::Error>(&datum.version)
.expect("archived version always deserializes");
stable |= version.is_stable();
local |= version.is_local();
map.insert(
version,
LazyPrioritizedDist::OnlySimple(SimplePrioritizedDist {
Expand Down Expand Up @@ -100,6 +102,7 @@ impl VersionMap {
inner: VersionMapInner::Lazy(VersionMapLazy {
map,
stable,
local,
simple_metadata,
no_binary: build_options.no_binary_package(package_name),
no_build: build_options.no_build_package(package_name),
Expand All @@ -122,7 +125,7 @@ impl VersionMap {
}

/// Return an iterator over the versions in this map.
pub(crate) fn versions(&self) -> impl Iterator<Item = &Version> {
pub(crate) fn versions(&self) -> impl DoubleEndedIterator<Item = &Version> {
match &self.inner {
VersionMapInner::Eager(eager) => either::Either::Left(eager.map.keys()),
VersionMapInner::Lazy(lazy) => either::Either::Right(lazy.map.keys()),
Expand Down Expand Up @@ -225,14 +228,23 @@ impl VersionMap {
VersionMapInner::Lazy(ref map) => map.stable,
}
}

/// Returns `true` if the map contains at least one local version (e.g., `2.6.0+cpu`).
pub(crate) fn local(&self) -> bool {
match self.inner {
VersionMapInner::Eager(ref map) => map.local,
VersionMapInner::Lazy(ref map) => map.local,
}
}
}

impl From<FlatDistributions> for VersionMap {
fn from(flat_index: FlatDistributions) -> Self {
let stable = flat_index.iter().any(|(version, _)| version.is_stable());
let local = flat_index.iter().any(|(version, _)| version.is_local());
let map = flat_index.into();
Self {
inner: VersionMapInner::Eager(VersionMapEager { map, stable }),
inner: VersionMapInner::Eager(VersionMapEager { map, stable, local }),
}
}
}
Expand Down Expand Up @@ -294,6 +306,8 @@ struct VersionMapEager {
map: BTreeMap<Version, PrioritizedDist>,
/// Whether the version map contains at least one stable (non-pre-release) version.
stable: bool,
/// Whether the version map contains at least one local version.
local: bool,
}

/// A map that lazily materializes some prioritized distributions upon access.
Expand All @@ -305,11 +319,14 @@ struct VersionMapEager {
/// avoiding another conversion step into a fully filled out `VersionMap` can
/// provide substantial savings in some cases.
#[derive(Debug)]
#[allow(clippy::struct_excessive_bools)]
struct VersionMapLazy {
/// A map from version to possibly-initialized distribution.
map: BTreeMap<Version, LazyPrioritizedDist>,
/// Whether the version map contains at least one stable (non-pre-release) version.
stable: bool,
/// Whether the version map contains at least one local version.
local: bool,
/// The raw simple metadata from which `PrioritizedDist`s should
/// be constructed.
simple_metadata: OwnedArchive<SimpleMetadata>,
Expand Down
Loading

0 comments on commit 47fb59f

Please sign in to comment.