Skip to content

Commit

Permalink
Avoid respecting preferences from other indexes (#10782)
Browse files Browse the repository at this point in the history
## Summary

The fix I shipped in #10690
regressed an important case. If we solve a PyPI branch before a PyTorch
branch, we'll end up respecting the preference, and choosing `2.2.2`
instead of `2.2.2+cpu`.

This PR goes back to ignoring preferences that don't map to the current
index. However, to solve #10383,
we need to special-case `requirements.txt`, which can't provide explicit
indexes. So, if a preference comes from `requirements.txt`, we still
respect it.

Closes #10772.
  • Loading branch information
charliermarsh authored Jan 20, 2025
1 parent 9e6e1e5 commit 8b6383e
Show file tree
Hide file tree
Showing 3 changed files with 551 additions and 57 deletions.
20 changes: 5 additions & 15 deletions crates/uv-resolver/src/candidate_selector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,35 +187,25 @@ impl CandidateSelector {
[] => return None,
[entry] => {
// Filter out preferences that map to a conflicting index.
if index.is_some_and(|index| {
entry
.index()
.is_some_and(|entry_index| entry_index != index)
}) {
if index.is_some_and(|index| !entry.index().matches(index)) {
return None;
};
}
Either::Left(std::iter::once((entry.marker(), entry.pin().version())))
}
[..] => {
type Entries<'a> = SmallVec<[&'a Entry; 3]>;

let mut preferences = preferences.iter().collect::<Entries>();
preferences.retain(|entry| {
// Filter out preferences that map to a conflicting index.
!index.is_some_and(|index| {
entry
.index()
.is_some_and(|entry_index| entry_index != index)
})
});
// Filter out preferences that map to a conflicting index.
preferences.retain(|entry| index.is_none_or(|index| entry.index().matches(index)));
preferences.sort_by_key(|entry| {
let marker = entry.marker();

// Prefer preferences that match the current environment.
let matches_env = env.included_by_marker(marker.pep508());

// Prefer preferences that match the current index.
let matches_index = index == entry.index();
let matches_index = index.is_none_or(|index| entry.index().matches(index));

std::cmp::Reverse((matches_env, matches_index))
});
Expand Down
96 changes: 54 additions & 42 deletions crates/uv-resolver/src/preferences.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::str::FromStr;
use rustc_hash::FxHashMap;
use tracing::trace;

use uv_distribution_types::{IndexUrl, InstalledDist, InstalledMetadata, InstalledVersion, Name};
use uv_distribution_types::IndexUrl;
use uv_normalize::PackageName;
use uv_pep440::{Operator, Version};
use uv_pep508::{MarkerTree, VersionOrUrl};
Expand All @@ -28,7 +28,7 @@ pub struct Preference {
/// The markers on the requirement itself (those after the semicolon).
marker: MarkerTree,
/// The index URL of the package, if any.
index: Option<IndexUrl>,
index: PreferenceIndex,
/// If coming from a package with diverging versions, the markers of the forks this preference
/// is part of, otherwise `None`.
fork_markers: Vec<UniversalMarker>,
Expand Down Expand Up @@ -62,9 +62,10 @@ impl Preference {
name: requirement.name,
version: specifier.version().clone(),
marker: requirement.marker,
// requirements.txt doesn't have fork annotations.
// `requirements.txt` doesn't have fork annotations.
fork_markers: vec![],
index: None,
// `requirements.txt` doesn't allow a requirement to specify an explicit index.
index: PreferenceIndex::Any,
hashes: entry
.hashes
.iter()
Expand All @@ -74,23 +75,6 @@ impl Preference {
}))
}

/// Create a [`Preference`] from an installed distribution.
pub fn from_installed(dist: &InstalledDist) -> Self {
let version = match dist.installed_version() {
InstalledVersion::Version(version) => version,
InstalledVersion::Url(_, version) => version,
};
Self {
name: dist.name().clone(),
version: version.clone(),
marker: MarkerTree::TRUE,
index: None,
// Installed distributions don't have fork annotations.
fork_markers: vec![],
hashes: Vec::new(),
}
}

/// Create a [`Preference`] from a locked distribution.
pub fn from_lock(
package: &crate::lock::Package,
Expand All @@ -103,7 +87,7 @@ impl Preference {
name: package.id.name.clone(),
version: version.clone(),
marker: MarkerTree::TRUE,
index: package.index(install_path)?,
index: PreferenceIndex::from(package.index(install_path)?),
fork_markers: package.fork_markers().to_vec(),
hashes: Vec::new(),
}))
Expand All @@ -120,10 +104,40 @@ impl Preference {
}
}

#[derive(Debug, Clone)]
pub enum PreferenceIndex {
/// The preference should match to any index.
Any,
/// The preference should match to an implicit index.
Implicit,
/// The preference should match to a specific index.
Explicit(IndexUrl),
}

impl PreferenceIndex {
/// Returns `true` if the preference matches the given explicit [`IndexUrl`].
pub(crate) fn matches(&self, index: &IndexUrl) -> bool {
match self {
Self::Any => true,
Self::Implicit => false,
Self::Explicit(preference) => preference == index,
}
}
}

impl From<Option<IndexUrl>> for PreferenceIndex {
fn from(index: Option<IndexUrl>) -> Self {
match index {
Some(index) => Self::Explicit(index),
None => Self::Implicit,
}
}
}

#[derive(Debug, Clone)]
pub(crate) struct Entry {
marker: UniversalMarker,
index: Option<IndexUrl>,
index: PreferenceIndex,
pin: Pin,
}

Expand All @@ -134,8 +148,8 @@ impl Entry {
}

/// Return the [`IndexUrl`] associated with the entry, if any.
pub(crate) fn index(&self) -> Option<&IndexUrl> {
self.index.as_ref()
pub(crate) fn index(&self) -> &PreferenceIndex {
&self.index
}

/// Return the pinned data associated with the entry.
Expand All @@ -162,7 +176,7 @@ impl Preferences {
preferences: impl IntoIterator<Item = Preference>,
env: &ResolverEnvironment,
) -> Self {
let mut slf = Self::default();
let mut map = FxHashMap::<PackageName, Vec<_>>::default();
for preference in preferences {
// Filter non-matching preferences when resolving for an environment.
if let Some(markers) = env.marker_environment() {
Expand All @@ -187,31 +201,29 @@ impl Preferences {

// Flatten the list of markers into individual entries.
if preference.fork_markers.is_empty() {
slf.insert(
preference.name,
preference.index,
UniversalMarker::TRUE,
Pin {
map.entry(preference.name).or_default().push(Entry {
marker: UniversalMarker::TRUE,
index: preference.index,
pin: Pin {
version: preference.version,
hashes: preference.hashes,
},
);
});
} else {
for fork_marker in preference.fork_markers {
slf.insert(
preference.name.clone(),
preference.index.clone(),
fork_marker,
Pin {
map.entry(preference.name.clone()).or_default().push(Entry {
marker: fork_marker,
index: preference.index.clone(),
pin: Pin {
version: preference.version.clone(),
hashes: preference.hashes.clone(),
},
);
});
}
}
}

slf
Self(map)
}

/// Insert a preference at the back.
Expand All @@ -224,7 +236,7 @@ impl Preferences {
) {
self.0.entry(package_name).or_default().push(Entry {
marker: markers,
index,
index: PreferenceIndex::from(index),
pin: pin.into(),
});
}
Expand All @@ -235,15 +247,15 @@ impl Preferences {
) -> impl Iterator<
Item = (
&PackageName,
impl Iterator<Item = (&UniversalMarker, Option<&IndexUrl>, &Version)>,
impl Iterator<Item = (&UniversalMarker, &PreferenceIndex, &Version)>,
),
> {
self.0.iter().map(|(name, preferences)| {
(
name,
preferences
.iter()
.map(|entry| (&entry.marker, entry.index.as_ref(), entry.pin.version())),
.map(|entry| (&entry.marker, &entry.index, entry.pin.version())),
)
})
}
Expand Down
Loading

0 comments on commit 8b6383e

Please sign in to comment.