Skip to content

Commit

Permalink
uv-resolver: propagate markers to sibling dependencies in forks
Browse files Browse the repository at this point in the history
When a fork occurs, we divide not just the dependencies that
provoked a fork into distinct groups, but we also add the
corresponding sibling dependencies to each fork. Previously,
while we track markers on the fork itself, the individual
dependencies that had markers only corresponded to markers
written from the dependency specification.

This meant that the sibling dependencies that got added to
each fork would not themselves have markers attached to them.
This in turn meant they would not have markers associated with
them in the lock file.

In many cases, this is actually okay, because the resolver will
pick a version that is "universal" across all forks in most
cases. But in some cases, this just simply isn't possible as
the marker expressions in the fork can and do influence resolution.
In which case, it is possible for the same package with different
versions to show up in the lock file unconditionally. Which is a
big no-no.

So in this commit, after we determine the forks, we intersect the
markers on each fork with each of its dependencies.

This does seem to balloon the marker expressions in some cases.
I plucked one low hanging fruit to avoid doing `x and x` in
trivial cases. (And this eliminated a portion of the snapshot
diffs.) But some pretty gnarly diffs remain.

This commit also fixes another bug: previously, when we created a fork
to capture the "remaining" universe of an incomplete set of markers, we
left out dependencies that should be included in that fork. We rectify
that here.

Fixes #5086

Partially addresses #4732
  • Loading branch information
BurntSushi committed Jul 26, 2024
1 parent 412780f commit 77b0052
Show file tree
Hide file tree
Showing 6 changed files with 133 additions and 23 deletions.
5 changes: 5 additions & 0 deletions crates/uv-resolver/src/pubgrub/dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use pubgrub::range::Range;
use tracing::warn;

use pep440_rs::{Version, VersionSpecifiers};
use pep508_rs::MarkerTree;
use pypi_types::{
ParsedArchiveUrl, ParsedDirectoryUrl, ParsedGitUrl, ParsedPathUrl, ParsedUrl, Requirement,
RequirementSource, VerbatimParsedUrl,
Expand All @@ -29,6 +30,10 @@ pub(crate) struct PubGrubDependency {
}

impl PubGrubDependency {
pub(crate) fn and_markers(&mut self, marker: &MarkerTree) {
self.package.and_markers(marker);
}

pub(crate) fn from_requirement<'a>(
requirement: &'a Requirement,
source_name: Option<&'a PackageName>,
Expand Down
75 changes: 75 additions & 0 deletions crates/uv-resolver/src/pubgrub/package.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::mem;
use std::ops::Deref;
use std::sync::Arc;

Expand Down Expand Up @@ -42,6 +43,9 @@ pub enum PubGrubPackageInner {
/// A Python version.
Python(PubGrubPython),
/// A Python package.
///
/// Note that it is guaranteed that `extra` and `dev` are never both
/// `Some`. That is, if one is `Some` then the other must be `None`.
Package {
name: PackageName,
extra: Option<ExtraName>,
Expand Down Expand Up @@ -117,6 +121,77 @@ impl PubGrubPackage {
}
}

/// Modifies this package in place such that it is associated with the
/// given markers by intersecting them any pre-existing markers.
///
/// That is, this causes the package to only be applicable to marker
/// environments corresponding to the intersection of what it previously
/// matched and the given markers.
///
/// This is useful when one needs to propagate markers from a fork to each
/// of its constituent dependencies. This is necessary because within a
/// fork, the resolver makes decisions based on the markers that created
/// that fork. While in many cases these decisions are universal, some may
/// not be! And so the markers from the fork must be propagated out to the
/// individual packages.
pub(crate) fn and_markers(&mut self, fork_marker: &MarkerTree) {
// We do a little dance here to pluck out as much existing
// information from this package as we can to avoid allocs.
// It is possible that the `Arc::make_mut` will do a deep
// clone here, thereby defeating our efforts, but I think it
// is likely that in most cases it will not. This is because
// this routine should be called almost immediately after a
// `PubGrubPackage` is created and _before_ it is passed to
// PubGrub itself.
match Arc::make_mut(&mut self.0) {
PubGrubPackageInner::Root(_) | PubGrubPackageInner::Python(_) => {}
// In this case, we may or may not already have a marker.
// If we don't, then this implies we will, and thus we may
// need to switch `Package` to some other representation.
PubGrubPackageInner::Package {
ref mut name,
ref mut extra,
ref dev,
ref mut marker,
} => {
// This case *should* never happen, I believe, because
// a `Package` with a non-None `dev` can only happen as
// a result of a `Dev` package, which we should have
// processed already by the time we get this package.
if dev.is_some() {
return;
}

let mut and = fork_marker.clone();
if let Some(marker) = marker.take() {
and.and(marker);
}
*self = PubGrubPackage::from_package(mem::take(name), extra.take(), Some(and));
}
// These cases are easy, because we can just modify the marker
// in place.
PubGrubPackageInner::Extra { ref mut marker, .. } => {
let mut and = fork_marker.clone();
if let Some(marker) = marker.take() {
and.and(marker);
}
*marker = Some(and);
}
PubGrubPackageInner::Dev { ref mut marker, .. } => {
let mut and = fork_marker.clone();
if let Some(marker) = marker.take() {
and.and(marker);
}
*marker = Some(and);
}
PubGrubPackageInner::Marker { ref mut marker, .. } => {
let mut and = fork_marker.clone();
and.and(mem::replace(marker, MarkerTree::And(vec![])));
*marker = and;
}
}
}

/// Returns the name of this PubGrub package, if it has one.
pub(crate) fn name(&self) -> Option<&PackageName> {
match &**self {
Expand Down
38 changes: 34 additions & 4 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2735,10 +2735,22 @@ impl Dependencies {
let mut new_forks: Vec<Fork> = vec![];
if let Some(markers) = fork_groups.remaining_universe() {
trace!("Adding split to cover possibly incomplete markers: {markers}");
new_forks.push(Fork {
dependencies: vec![],
markers,
});
let mut new_forks_for_remaining_universe = forks.clone();
for fork in &mut new_forks_for_remaining_universe {
fork.markers.and(markers.clone());
fork.dependencies.retain(|dep| {
let Some(dep_marker) = dep.package.marker() else {
return true;
};
// After we constrain the markers on an existing
// fork, we should ensure that any existing
// dependencies that are no longer possible in this
// fork are removed. This mirrors the check we do in
// `add_nonfork_package`.
!crate::marker::is_disjoint(&fork.markers, dep_marker)
});
}
new_forks.extend(new_forks_for_remaining_universe);
}
for group in fork_groups.forks {
let mut new_forks_for_group = forks.clone();
Expand All @@ -2752,6 +2764,9 @@ impl Dependencies {
forks = new_forks;
diverging_packages.push(name.clone());
}
for fork in &mut forks {
fork.propagate_markers();
}
ForkedDependencies::Forked {
forks,
diverging_packages,
Expand Down Expand Up @@ -2879,6 +2894,21 @@ impl Fork {
.map_or(true, |pkg_marker| !is_disjoint(pkg_marker, fork_marker))
});
}

/// This attaches the marker in this fork to each of its dependencies.
///
/// In effect, this "propagates" the markers to each individual dependency
/// that was spawned as the result of a fork. While in many cases the
/// markers will be combined when multiple forks choose the same version of
/// a dependency, in some cases, the version chosen can be specific to a
/// particular set of marker environments. In this case, the dependencies
/// will be platform specific and thus require marker expressions to appear
/// in the lock file.
fn propagate_markers(&mut self) {
for dependency in &mut self.dependencies {
dependency.and_markers(&self.markers);
}
}
}

/// Intermediate state that represents a *possible* grouping of forks
Expand Down
6 changes: 3 additions & 3 deletions crates/uv/tests/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2901,9 +2901,9 @@ fn lock_python_version_marker_complement() -> Result<()> {
version = "0.1.0"
source = { editable = "." }
dependencies = [
{ name = "attrs" },
{ name = "iniconfig" },
{ name = "typing-extensions", marker = "python_full_version <= '3.10' or python_full_version > '3.10'" },
{ name = "attrs", marker = "(python_full_version <= '3.10' and python_version == '3.10') or (python_full_version <= '3.10' and python_version < '3.10') or (python_full_version <= '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version == '3.10') or (python_full_version > '3.10' and python_version < '3.10') or (python_full_version > '3.10' and python_version > '3.10')" },
{ name = "iniconfig", marker = "(python_full_version <= '3.10' and python_version == '3.10') or (python_full_version <= '3.10' and python_version < '3.10') or (python_full_version <= '3.10' and python_version < '3.10' and python_version > '3.10') or (python_full_version <= '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version == '3.10') or (python_full_version > '3.10' and python_version < '3.10') or (python_full_version > '3.10' and python_version < '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version > '3.10')" },
{ name = "typing-extensions", marker = "(python_full_version <= '3.10' and python_version == '3.10') or (python_full_version <= '3.10' and python_version < '3.10') or (python_full_version <= '3.10' and python_version < '3.10' and python_version > '3.10') or (python_full_version <= '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version == '3.10') or (python_full_version > '3.10' and python_version < '3.10') or (python_full_version > '3.10' and python_version < '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version > '3.10')" },
]
[[distribution]]
Expand Down
8 changes: 4 additions & 4 deletions crates/uv/tests/lock_scenarios.rs
Original file line number Diff line number Diff line change
Expand Up @@ -723,7 +723,7 @@ fn fork_incomplete_markers() -> Result<()> {
dependencies = [
{ name = "package-a", version = "1.0.0", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "python_version < '3.10'" },
{ name = "package-a", version = "2.0.0", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "python_version >= '3.11'" },
{ name = "package-b" },
{ name = "package-b", marker = "python_version < '3.10' or python_version >= '3.11' or (python_version < '3.11' and python_version >= '3.10')" },
]
"###
);
Expand Down Expand Up @@ -1804,7 +1804,7 @@ fn fork_marker_limited_inherit() -> Result<()> {
dependencies = [
{ name = "package-a", version = "1.0.0", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "sys_platform == 'darwin'" },
{ name = "package-a", version = "2.0.0", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "sys_platform == 'linux'" },
{ name = "package-b" },
{ name = "package-b", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or (sys_platform != 'darwin' and sys_platform != 'linux')" },
]
"###
);
Expand Down Expand Up @@ -1919,7 +1919,7 @@ fn fork_marker_selection() -> Result<()> {
version = "0.1.0"
source = { editable = "." }
dependencies = [
{ name = "package-a" },
{ name = "package-a", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or (sys_platform != 'darwin' and sys_platform != 'linux')" },
{ name = "package-b", version = "1.0.0", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "sys_platform == 'darwin'" },
{ name = "package-b", version = "2.0.0", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "sys_platform == 'linux'" },
]
Expand Down Expand Up @@ -2059,7 +2059,7 @@ fn fork_marker_track() -> Result<()> {
version = "0.1.0"
source = { editable = "." }
dependencies = [
{ name = "package-a" },
{ name = "package-a", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or (sys_platform != 'darwin' and sys_platform != 'linux')" },
{ name = "package-b", version = "2.7", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "sys_platform == 'darwin'" },
{ name = "package-b", version = "2.8", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "sys_platform == 'linux'" },
]
Expand Down
24 changes: 12 additions & 12 deletions crates/uv/tests/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7298,7 +7298,7 @@ fn universal_nested_overlapping_local_requirement() -> Result<()> {
# uv pip compile --cache-dir [CACHE_DIR] requirements.in --universal
cmake==3.28.4 ; platform_machine == 'x86_64' and platform_system == 'Linux'
# via triton
. ; os_name == 'Linux'
. ; (os_name == 'Linux' and platform_machine == 'x86_64') or (os_name == 'Linux' and platform_machine != 'x86_64')
# via -r requirements.in
filelock==3.13.1
# via
Expand Down Expand Up @@ -7766,7 +7766,7 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> {
# uv pip compile --cache-dir [CACHE_DIR] requirements.in -p 3.8 --universal
alabaster==0.7.13
# via sphinx
astroid==3.1.0
astroid==3.1.0 ; python_version < '3.11' or python_version >= '3.12'
# via pylint
babel==2.14.0
# via sphinx
Expand All @@ -7778,7 +7778,7 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> {
# via
# pylint
# sphinx
dill==0.3.8
dill==0.3.8 ; python_version < '3.11' or python_version >= '3.12'
# via pylint
docutils==0.20.1
# via sphinx
Expand All @@ -7788,17 +7788,17 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> {
# via sphinx
importlib-metadata==7.1.0 ; python_version < '3.10'
# via sphinx
isort==5.13.2
isort==5.13.2 ; python_version < '3.11' or python_version >= '3.12'
# via pylint
jinja2==3.1.3
# via sphinx
markupsafe==2.1.5
# via jinja2
mccabe==0.7.0
mccabe==0.7.0 ; python_version < '3.11' or python_version >= '3.12'
# via pylint
packaging==24.0
# via sphinx
platformdirs==4.2.0
platformdirs==4.2.0 ; python_version < '3.11' or python_version >= '3.12'
# via pylint
pygments==2.17.2
# via sphinx
Expand Down Expand Up @@ -7826,7 +7826,7 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> {
# via sphinx
tomli==2.0.1 ; python_version < '3.11'
# via pylint
tomlkit==0.12.4
tomlkit==0.12.4 ; python_version < '3.11' or python_version >= '3.12'
# via pylint
typing-extensions==4.10.0 ; python_version < '3.11'
# via
Expand Down Expand Up @@ -7916,7 +7916,7 @@ fn universal_marker_propagation() -> Result<()> {
# via requests
filelock==3.13.1
# via torch
fsspec==2024.3.1
fsspec==2024.3.1 ; platform_machine != 'x86_64'
# via torch
idna==3.6
# via requests
Expand All @@ -7936,17 +7936,17 @@ fn universal_marker_propagation() -> Result<()> {
# via torchvision
sympy==1.12
# via torch
torch==2.0.0
torch==2.0.0 ; platform_machine == 'x86_64'
# via
# -r requirements.in
# torchvision
torch==2.2.0
torch==2.2.0 ; platform_machine != 'x86_64'
# via
# -r requirements.in
# torchvision
torchvision==0.15.1+rocm5.4.2
torchvision==0.15.1+rocm5.4.2 ; platform_machine == 'x86_64'
# via -r requirements.in
torchvision==0.17.0+rocm5.7
torchvision==0.17.0+rocm5.7 ; platform_machine != 'x86_64'
# via -r requirements.in
typing-extensions==4.10.0
# via torch
Expand Down

0 comments on commit 77b0052

Please sign in to comment.