Skip to content

Commit

Permalink
uv-resolver: introduce conflict markers
Browse files Browse the repository at this point in the history
This commit adds a new concept to uv: conflict markers. Conflict markers
resolve the ambiguity created in lock files, in some cases, when
conflicting extras/groups are activated. Conflict markers allow us to
make edges in the dependency graph conditional based only on which
extras are and aren't enabled.

Adding conflict markers themselves turned out to be somewhat easy. But
what was hard was simplifying them. A good portion of the change in this
commit is dedicated to simplification. We do two different kinds:

1. We imbibe "world knowledge" into the marker. That is, we take what we
   know about which extras/groups cannot be enabled simultaneously, then
   we simplify the markers based on this.
2. We walk the dependency graph and annotate which extras/groups are
   activated for each node for all possible paths into that node. When
   an extra (e.g., `foo`) is activated (or not activated) for all
   possible such paths, then we can replace `extra == 'foo'` (or
   `extra != 'foo'`) with `true` in any corresponding conflict marker
   expessions.

Without these simplifications, it's very easy for conflict markers to
appear almost everywhere in a lock file (when conflicting extras/groups
are declared). Many of which are trivially true and aren't actually
resolving any ambiguity and thus aren't needed.
  • Loading branch information
BurntSushi committed Dec 6, 2024
1 parent 18a1ead commit 26acf26
Show file tree
Hide file tree
Showing 17 changed files with 795 additions and 126 deletions.
4 changes: 2 additions & 2 deletions crates/uv-pypi-types/src/conflicts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ impl Conflicts {
}

/// Returns an iterator over all sets of conflicting sets.
pub fn iter(&self) -> impl Iterator<Item = &'_ ConflictSet> + '_ {
pub fn iter(&self) -> impl Iterator<Item = &'_ ConflictSet> + Clone + '_ {
self.0.iter()
}

Expand Down Expand Up @@ -75,7 +75,7 @@ impl ConflictSet {
}

/// Returns an iterator over all conflicting items.
pub fn iter(&self) -> impl Iterator<Item = &'_ ConflictItem> + '_ {
pub fn iter(&self) -> impl Iterator<Item = &'_ ConflictItem> + Clone + '_ {
self.0.iter()
}

Expand Down
161 changes: 159 additions & 2 deletions crates/uv-resolver/src/graph_ops.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
use petgraph::graph::NodeIndex;
use petgraph::graph::{EdgeIndex, NodeIndex};
use petgraph::visit::EdgeRef;
use petgraph::{Direction, Graph};
use rustc_hash::{FxBuildHasher, FxHashMap};
use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet};
use std::collections::hash_map::Entry;

use uv_normalize::{ExtraName, GroupName, PackageName};
use uv_pypi_types::{ConflictItem, Conflicts};

use crate::resolution::ResolutionGraphNode;
use crate::universal_marker::UniversalMarker;

/// Determine the markers under which a package is reachable in the dependency tree.
Expand Down Expand Up @@ -79,3 +83,156 @@ pub(crate) fn marker_reachability<T>(

reachability
}

/// Traverse the given dependency graph and propagate activated markers.
///
/// For example, given an edge like `foo[x1] -> bar`, then it is known that
/// `x1` is activated. This in turn can be used to simplify any downstream
/// conflict markers with `extra == "x1"` in them (by replacing `extra == "x1"`
/// with `true`).
pub(crate) fn simplify_conflict_markers(
conflicts: &Conflicts,
graph: &mut Graph<ResolutionGraphNode, UniversalMarker>,
) {
/// An inference about whether a conflicting item is always included or
/// excluded.
///
/// We collect these for each node in the graph after determining which
/// extras/groups are activated for each node. Once we know what's
/// activated, we can infer what must also be *inactivated* based on what's
/// conflicting with it. So for example, if we have a conflict marker like
/// `extra == 'foo' and extra != 'bar'`, and `foo` and `bar` have been
/// declared as conflicting, and we are in a part of the graph where we
/// know `foo` must be activated, then it follows that `extra != 'bar'`
/// must always be true. Because if it were false, it would imply both
/// `foo` and `bar` were activated simultaneously, which uv guarantees
/// won't happen.
///
/// We then use these inferences to simplify the conflict markers.
#[derive(Clone, Debug, Eq, Hash, PartialEq)]
struct Inference {
item: ConflictItem,
included: bool,
}

// The set of activated extras and groups for each node. The ROOT nodes
// don't have any extras/groups activated.
let mut activated: FxHashMap<NodeIndex, Vec<FxHashSet<ConflictItem>>> = FxHashMap::default();

// Collect the root nodes.
//
// Besides the actual virtual root node, virtual dev dependencies packages are also root
// nodes since the edges don't cover dev dependencies.
let mut queue: Vec<_> = graph
.node_indices()
.filter(|node_index| {
graph
.edges_directed(*node_index, Direction::Incoming)
.next()
.is_none()
})
.collect();

let mut seen: FxHashSet<NodeIndex> = FxHashSet::default();
while let Some(parent_index) = queue.pop() {
if let Some((package, extra)) = graph[parent_index].package_extra_names() {
for set in activated
.entry(parent_index)
.or_insert_with(|| vec![FxHashSet::default()])
{
set.insert(ConflictItem::from((package.clone(), extra.clone())));
}
}
if let Some((package, group)) = graph[parent_index].package_group_names() {
for set in activated
.entry(parent_index)
.or_insert_with(|| vec![FxHashSet::default()])
{
set.insert(ConflictItem::from((package.clone(), group.clone())));
}
}
let sets = activated.get(&parent_index).cloned().unwrap_or_default();
for child_edge in graph.edges_directed(parent_index, Direction::Outgoing) {
for set in sets.clone() {
activated.entry(child_edge.target()).or_default().push(set);
}
if seen.insert(child_edge.target()) {
queue.push(child_edge.target());
}
}
}

let mut inferences: FxHashMap<NodeIndex, Vec<FxHashSet<Inference>>> = FxHashMap::default();
for (node_id, sets) in activated {
let mut new_sets = vec![];
for set in sets {
let mut new_set = FxHashSet::default();
for item in set {
for conflict_set in conflicts.iter() {
if !conflict_set.contains(item.package(), item.as_ref().conflict()) {
continue;
}
for conflict_item in conflict_set.iter() {
if conflict_item == &item {
continue;
}
new_set.insert(Inference {
item: conflict_item.clone(),
included: false,
});
}
}
new_set.insert(Inference {
item,
included: true,
});
}
new_sets.push(new_set);
}
inferences.insert(node_id, new_sets);
}

for edge_index in (0..graph.edge_count()).map(EdgeIndex::new) {
let (from_index, _) = graph.edge_endpoints(edge_index).unwrap();
let Some(inference_sets) = inferences.get(&from_index) else {
continue;
};
// If not all possible paths (represented by our inferences)
// satisfy the conflict marker on this edge, then we can't make any
// simplifications. Namely, because it follows that out inferences
// aren't always true. Some of them may sometimes be false.
let all_paths_satisfied = inference_sets.iter().all(|set| {
let extras = set
.iter()
.filter_map(|inf| {
if !inf.included {
return None;
}
Some((inf.item.package().clone(), inf.item.extra()?.clone()))
})
.collect::<Vec<(PackageName, ExtraName)>>();
let groups = set
.iter()
.filter_map(|inf| {
if !inf.included {
return None;
}
Some((inf.item.package().clone(), inf.item.group()?.clone()))
})
.collect::<Vec<(PackageName, GroupName)>>();
graph[edge_index].conflict().evaluate(&extras, &groups)
});
if !all_paths_satisfied {
continue;
}
for set in inference_sets {
for inf in set {
if inf.included {
graph[edge_index].assume_conflict_item(&inf.item);
} else {
graph[edge_index].assume_not_conflict_item(&inf.item);
}
}
}
}
}
2 changes: 1 addition & 1 deletion crates/uv-resolver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub use resolver::{
PackageVersionsResult, Reporter as ResolverReporter, Resolver, ResolverEnvironment,
ResolverProvider, VersionsResponse, WheelMetadataResult,
};
pub use universal_marker::UniversalMarker;
pub use universal_marker::{ConflictMarker, UniversalMarker};
pub use version_map::VersionMap;
pub use yanks::AllowedYanks;

Expand Down
30 changes: 15 additions & 15 deletions crates/uv-resolver/src/lock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub use crate::lock::target::InstallTarget;
pub use crate::lock::tree::TreeDisplay;
use crate::requires_python::SimplifiedMarkerTree;
use crate::resolution::{AnnotatedDist, ResolutionGraphNode};
use crate::universal_marker::UniversalMarker;
use crate::universal_marker::{ConflictMarker, UniversalMarker};
use crate::{
ExcludeNewer, InMemoryIndex, MetadataResponse, PrereleaseMode, RequiresPython, ResolutionMode,
ResolverOutput,
Expand Down Expand Up @@ -63,21 +63,21 @@ static LINUX_MARKERS: LazyLock<UniversalMarker> = LazyLock::new(|| {
"platform_system == 'Linux' and os_name == 'posix' and sys_platform == 'linux'",
)
.unwrap();
UniversalMarker::new(pep508, MarkerTree::TRUE)
UniversalMarker::new(pep508, ConflictMarker::TRUE)
});
static WINDOWS_MARKERS: LazyLock<UniversalMarker> = LazyLock::new(|| {
let pep508 = MarkerTree::from_str(
"platform_system == 'Windows' and os_name == 'nt' and sys_platform == 'win32'",
)
.unwrap();
UniversalMarker::new(pep508, MarkerTree::TRUE)
UniversalMarker::new(pep508, ConflictMarker::TRUE)
});
static MAC_MARKERS: LazyLock<UniversalMarker> = LazyLock::new(|| {
let pep508 = MarkerTree::from_str(
"platform_system == 'Darwin' and os_name == 'posix' and sys_platform == 'darwin'",
)
.unwrap();
UniversalMarker::new(pep508, MarkerTree::TRUE)
UniversalMarker::new(pep508, ConflictMarker::TRUE)
});

#[derive(Clone, Debug, serde::Deserialize)]
Expand Down Expand Up @@ -149,7 +149,7 @@ impl Lock {
resolution
.fork_markers
.iter()
.filter(|fork_markers| !fork_markers.is_disjoint(&dist.marker))
.filter(|fork_markers| !fork_markers.is_disjoint(dist.marker))
.copied()
.collect()
} else {
Expand Down Expand Up @@ -296,16 +296,16 @@ impl Lock {
tag.starts_with(linux_tag) || tag == "linux_armv6l" || tag == "linux_armv7l"
})
}) {
!graph.graph[node_index].marker().is_disjoint(&LINUX_MARKERS)
!graph.graph[node_index].marker().is_disjoint(*LINUX_MARKERS)
} else if platform_tags
.iter()
.all(|tag| windows_tags.contains(&&**tag))
{
!graph.graph[node_index]
.marker()
.is_disjoint(&WINDOWS_MARKERS)
.is_disjoint(*WINDOWS_MARKERS)
} else if platform_tags.iter().all(|tag| tag.starts_with("macosx_")) {
!graph.graph[node_index].marker().is_disjoint(&MAC_MARKERS)
!graph.graph[node_index].marker().is_disjoint(*MAC_MARKERS)
} else {
true
}
Expand Down Expand Up @@ -860,7 +860,7 @@ impl Lock {
|| dist
.fork_markers
.iter()
.any(|marker| marker.evaluate(marker_env, &[]))
.any(|marker| marker.evaluate_no_extras(marker_env))
{
if found_dist.is_some() {
return Err(format!("found multiple packages matching `{name}`"));
Expand Down Expand Up @@ -1449,7 +1449,9 @@ impl TryFrom<LockWire> for Lock {
.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, MarkerTree::TRUE))
.map(|complexified_marker| {
UniversalMarker::new(complexified_marker, ConflictMarker::TRUE)
})
.collect();
let lock = Lock::new(
wire.version,
Expand Down Expand Up @@ -2251,7 +2253,7 @@ impl PackageWire {
// 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, MarkerTree::TRUE)
UniversalMarker::new(complexified_marker, ConflictMarker::TRUE)
})
.collect(),
dependencies: unwire_deps(self.dependencies)?,
Expand Down Expand Up @@ -3541,7 +3543,7 @@ impl Dependency {
complexified_marker: UniversalMarker,
) -> Dependency {
let simplified_marker =
SimplifiedMarkerTree::new(requires_python, complexified_marker.pep508());
SimplifiedMarkerTree::new(requires_python, complexified_marker.combined());
Dependency {
package_id,
extra,
Expand Down Expand Up @@ -3621,7 +3623,6 @@ struct DependencyWire {
extra: BTreeSet<ExtraName>,
#[serde(default)]
marker: SimplifiedMarkerTree,
// FIXME: Add support for representing conflict markers.
}

impl DependencyWire {
Expand All @@ -3635,8 +3636,7 @@ impl DependencyWire {
package_id: self.package_id.unwire(unambiguous_package_ids)?,
extra: self.extra,
simplified_marker: self.marker,
// FIXME: Support reading conflict markers.
complexified_marker: UniversalMarker::new(complexified_marker, MarkerTree::TRUE),
complexified_marker: UniversalMarker::from_combined(complexified_marker),
})
}
}
Expand Down
6 changes: 3 additions & 3 deletions crates/uv-resolver/src/lock/requirements_txt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use uv_pypi_types::{ParsedArchiveUrl, ParsedGitUrl};

use crate::graph_ops::marker_reachability;
use crate::lock::{LockErrorKind, Package, PackageId, Source};
use crate::universal_marker::UniversalMarker;
use crate::universal_marker::{ConflictMarker, UniversalMarker};
use crate::{InstallTarget, LockError};

/// An export of a [`Lock`] that renders in `requirements.txt` format.
Expand Down Expand Up @@ -122,7 +122,7 @@ impl<'lock> RequirementsTxtExport<'lock> {
// `marker_reachability` wants and it (probably) isn't
// worth inventing a new abstraction so that it can accept
// graphs with either `MarkerTree` or `UniversalMarker`.
MarkerTree::TRUE,
ConflictMarker::TRUE,
),
);

Expand Down Expand Up @@ -175,7 +175,7 @@ impl<'lock> RequirementsTxtExport<'lock> {
dep.simplified_marker.as_simplified_marker_tree(),
// See note above for other `UniversalMarker::new` for
// why this is OK.
MarkerTree::TRUE,
ConflictMarker::TRUE,
),
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,7 @@ Ok(
simplified_marker: SimplifiedMarkerTree(
true,
),
complexified_marker: UniversalMarker {
pep508_marker: python_full_version >= '3.12',
conflict_marker: true,
},
complexified_marker: python_full_version >= '3.12',
},
],
optional_dependencies: {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,7 @@ Ok(
simplified_marker: SimplifiedMarkerTree(
true,
),
complexified_marker: UniversalMarker {
pep508_marker: python_full_version >= '3.12',
conflict_marker: true,
},
complexified_marker: python_full_version >= '3.12',
},
],
optional_dependencies: {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,7 @@ Ok(
simplified_marker: SimplifiedMarkerTree(
true,
),
complexified_marker: UniversalMarker {
pep508_marker: python_full_version >= '3.12',
conflict_marker: true,
},
complexified_marker: python_full_version >= '3.12',
},
],
optional_dependencies: {},
Expand Down
Loading

0 comments on commit 26acf26

Please sign in to comment.