Skip to content

Commit

Permalink
uv-resolver: include conflict markers in fork markers (#10818)
Browse files Browse the repository at this point in the history 
When support for conflicting extras/groups was initially added, I
stopped short of including the conflict markers in uv's "fork markers"
in the lock file. That is, the fork markers are markers that indicate
the different splits uv took during resolution, which we record, I
believe, to avoid spurious updates to the lock file as a result of
using them as preferences.

One interesting result of omitting the conflict markers from the fork
markers is that sometimes this would result in duplicate markers. In
response, I wrote a function that stripped off the conflict markers and
deduplicated the remainder. My thinking at the time was that it wasn't
clear whether we needed to keep conflict markers around.

It looks like #10783 demonstrates a case where we do, seemingly, need
them. Namely, it's a case where after stripping conflict markers, you
don't end up with duplicate markers, but you do end up with overlapping
markers. Overlapping fork markers are bad juju for the same reason that
overlapping resolver forks are bad juju: you can end up with multiple
versions of the same package in the same environment.

I don't know how to fix overlapping markers without just including the
conflict markers. So that's what this PR does. Because of this, there
will be some churn in lock files, but this only applies to projects that
define conflicting extras.

This PR includes a regression test from #10783. I also manually tried
the original reproduction in #10772 (where adding `numpy<2` caused `uv
sync` to fail), and things worked.

Fixes #10772, Fixes #10783
  • Loading branch information
@BurntSushi
BurntSushi authored Jan 21, 2025
1 parent 6a5e5b3 commit 9552c0a
Show file tree
Hide file tree
Showing 5 changed files with 729 additions and 64 deletions.
74 changes: 31 additions & 43 deletions crates/uv-resolver/src/lock/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -665,8 +665,7 @@ impl Lock {

if !self.fork_markers.is_empty() {
let fork_markers = each_element_on_its_line_array(
deduplicated_simplified_pep508_markers(&self.fork_markers, &self.requires_python)
.into_iter(),
simplified_universal_markers(&self.fork_markers, &self.requires_python).into_iter(),
);
if !fork_markers.is_empty() {
doc.insert("resolution-markers", value(fork_markers));
Expand Down Expand Up @@ -1636,11 +1635,7 @@ impl TryFrom<LockWire> for Lock {
.fork_markers
.into_iter()
.map(|simplified_marker| simplified_marker.into_marker(&wire.requires_python))
// TODO(ag): Consider whether this should also deserialize a conflict marker.
// We currently aren't serializing. Dropping it completely is likely to be wrong.
.map(|complexified_marker| {
UniversalMarker::new(complexified_marker, ConflictMarker::TRUE)
})
.map(UniversalMarker::from_combined)
.collect();
let lock = Lock::new(
wire.version,
Expand Down Expand Up @@ -2262,8 +2257,7 @@ impl Package {

if !self.fork_markers.is_empty() {
let fork_markers = each_element_on_its_line_array(
deduplicated_simplified_pep508_markers(&self.fork_markers, requires_python)
.into_iter(),
simplified_universal_markers(&self.fork_markers, requires_python).into_iter(),
);
if !fork_markers.is_empty() {
table.insert("resolution-markers", value(fork_markers));
Expand Down Expand Up @@ -2585,11 +2579,7 @@ impl PackageWire {
.fork_markers
.into_iter()
.map(|simplified_marker| simplified_marker.into_marker(requires_python))
// TODO(ag): Consider whether this should also deserialize a conflict marker.
// We currently aren't serializing. Dropping it completely is likely to be wrong.
.map(|complexified_marker| {
UniversalMarker::new(complexified_marker, ConflictMarker::TRUE)
})
.map(UniversalMarker::from_combined)
.collect(),
dependencies: unwire_deps(self.dependencies)?,
optional_dependencies: self
Expand Down Expand Up @@ -4898,42 +4888,40 @@ fn each_element_on_its_line_array(elements: impl Iterator<Item = impl Into<Value

/// Returns the simplified string-ified version of each marker given.
///
/// If a marker is a duplicate of a previous marker or is always true after
/// simplification, then it is omitted from the `Vec` returned. (And indeed,
/// the `Vec` returned may be empty.)
fn deduplicated_simplified_pep508_markers(
/// Note that the marker strings returned will include conflict markers if they
/// are present.
fn simplified_universal_markers(
markers: &[UniversalMarker],
requires_python: &RequiresPython,
) -> Vec<String> {
// NOTE(ag): It's possible that `resolution-markers` should actually
// include conflicting marker info. In which case, we should serialize
// the entire `UniversalMarker` (taking care to still make the PEP 508
// simplified). At present, we don't include that info. And as a result,
// this can lead to duplicate markers, since each represents a fork with
// the same PEP 508 marker but a different conflict marker. We strip the
// conflict marker, which can leave duplicate PEP 508 markers.
//
// So if we did include the conflict marker, then we wouldn't need to do
// deduplication.
//
// Why don't we include conflict markers though? At present, it's just
// not clear that they are necessary. So by the principle of being
// conservative, we don't write them. In particular, I believe the original
// reason for `resolution-markers` is to prevent non-deterministic locking.
// But it's not clear that that can occur for conflict markers.
let mut simplified = vec![];
// Deduplicate without changing order.
let mut pep508_only = vec![];
let mut seen = FxHashSet::default();
for marker in markers {
let simplified_marker = SimplifiedMarkerTree::new(requires_python, marker.pep508());
let Some(simplified_string) = simplified_marker.try_to_string() else {
continue;
};
if seen.insert(simplified_string.clone()) {
simplified.push(simplified_string);
let simplified =
SimplifiedMarkerTree::new(requires_python, marker.pep508()).as_simplified_marker_tree();
if seen.insert(simplified) {
pep508_only.push(simplified);
}
}
simplified
let any_overlap = pep508_only
.iter()
.tuple_combinations()
.any(|(&marker1, &marker2)| !marker1.is_disjoint(marker2));
let markers = if !any_overlap {
pep508_only
} else {
markers
.iter()
.map(|marker| {
SimplifiedMarkerTree::new(requires_python, marker.combined())
.as_simplified_marker_tree()
})
.collect()
};
markers
.into_iter()
.filter_map(MarkerTree::try_to_string)
.collect()
}

#[cfg(test)]
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-resolver/src/universal_marker.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ impl UniversalMarker {

/// Returns the internal marker that combines both the PEP 508
/// and conflict marker.
pub(crate) fn combined(self) -> MarkerTree {
pub fn combined(self) -> MarkerTree {
self.marker
}

Expand Down
7 changes: 1 addition & 6 deletions crates/uv/src/commands/project/lock.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -637,15 +637,10 @@ async fn do_lock(
let resolver_env = ResolverEnvironment::universal(
forks_lock
.map(|lock| {
// TODO(ag): Consider whether we should be capturing
// conflicting extras/groups for every fork. If
// we did, then we'd be able to use them here,
// which would in turn flow into construction of
// `ResolverEnvironment`.
lock.fork_markers()
.iter()
.copied()
.map(UniversalMarker::pep508)
.map(UniversalMarker::combined)
.collect()
})
.unwrap_or_else(|| {
Expand Down
19 changes: 11 additions & 8 deletions crates/uv/tests/it/lock.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22900,10 +22900,11 @@ fn lock_pytorch_cpu() -> Result<()> {
version = 1
requires-python = ">=3.12.[X]"
resolution-markers = [
"platform_machine != 'aarch64' or sys_platform != 'linux'",
"platform_machine == 'aarch64' and sys_platform == 'linux'",
"(platform_machine != 'aarch64' and sys_platform == 'linux') or (sys_platform != 'darwin' and sys_platform != 'linux')",
"(platform_machine == 'aarch64' and sys_platform == 'linux') or sys_platform == 'darwin'",
"(platform_machine != 'aarch64' and extra != 'extra-7-project-cpu' and extra == 'extra-7-project-cu124') or (sys_platform != 'linux' and extra != 'extra-7-project-cpu' and extra == 'extra-7-project-cu124')",
"platform_machine == 'aarch64' and sys_platform == 'linux' and extra != 'extra-7-project-cpu' and extra == 'extra-7-project-cu124'",
"(platform_machine != 'aarch64' and sys_platform == 'linux' and extra == 'extra-7-project-cpu' and extra != 'extra-7-project-cu124') or (sys_platform != 'darwin' and sys_platform != 'linux' and extra == 'extra-7-project-cpu' and extra != 'extra-7-project-cu124')",
"(platform_machine == 'aarch64' and sys_platform == 'linux' and extra == 'extra-7-project-cpu' and extra != 'extra-7-project-cu124') or (sys_platform == 'darwin' and extra == 'extra-7-project-cpu' and extra != 'extra-7-project-cu124')",
"extra != 'extra-7-project-cpu' and extra != 'extra-7-project-cu124'",
]
conflicts = [[
{ package = "project", extra = "cpu" },
Expand Down Expand Up @@ -23547,10 +23548,12 @@ fn lock_pytorch_preferences() -> Result<()> {
version = 1
requires-python = ">=3.10.0"
resolution-markers = [
"sys_platform != 'darwin'",
"sys_platform == 'darwin'",
"(platform_machine != 'aarch64' and sys_platform == 'linux') or (sys_platform != 'darwin' and sys_platform != 'linux')",
"platform_machine == 'aarch64' and sys_platform == 'linux'",
"sys_platform != 'darwin' and extra != 'extra-7-project-cpu' and extra == 'extra-7-project-cu118'",
"sys_platform == 'darwin' and extra != 'extra-7-project-cpu' and extra == 'extra-7-project-cu118'",
"(platform_machine != 'aarch64' and sys_platform == 'linux' and extra == 'extra-7-project-cpu' and extra != 'extra-7-project-cu118') or (sys_platform != 'darwin' and sys_platform != 'linux' and extra == 'extra-7-project-cpu' and extra != 'extra-7-project-cu118')",
"platform_machine == 'aarch64' and sys_platform == 'linux' and extra == 'extra-7-project-cpu' and extra != 'extra-7-project-cu118'",
"sys_platform == 'darwin' and extra == 'extra-7-project-cpu' and extra != 'extra-7-project-cu118'",
"extra != 'extra-7-project-cpu' and extra != 'extra-7-project-cu118'",
]
conflicts = [[
{ package = "project", extra = "cpu" },
Expand Down
Loading

0 comments on commit 9552c0a

Please sign in to comment.