Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add conflict markers to the lock file #9370

Merged
merged 5 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
189 changes: 187 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,184 @@ 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,
}

// Do nothing if there are no declared conflicts. Without any declared
// conflicts, we know we have no conflict markers and thus nothing to
// simplify by determining which extras are activated at different points
// in the dependency graph.
if conflicts.is_empty() {
return;
}

// 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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't the propagated value set order dependent? Below, the green and the red items from A and C are propagated through, but B -> D happens before E -> D, so B -> D isn't propagated. Numbers indicate the order of edge traversals.

PXL_20241209_130608932~2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be easier if we started at the conflicting nodes and propagated knowledge about the "side" of the conflict through from them? E.g.

[tool.uv]
conflicts = [
    [
      { extra = "extra1" },
      { extra = "extra2" },
    ],
]

we could start at project[extra1] and project[extra2] respectively, mark every node with (project, extra1) or (project, extra2) respectively and then simplify the outgoing edges of all nodes that only have either marker. This might make the world-knowledge easier to move in? (Caveat: That's from reading code only)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fixed in 8a5ce95

I opted to fix this by revisiting nodes when the set of extras gets updated.

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) {
let mut change = false;
for set in sets.clone() {
let existing = activated.entry(child_edge.target()).or_default();
// This is doing a linear scan for testing membership, which
// is non-ideal. But it's not actually clear that there's a
// strictly better alternative without a real workload being
// slow because of this. Namely, we are checking whether the
// _set_ being inserted is equivalent to an existing set. So
// instead of, say, `Vec<FxHashSet<ConflictItem>>`, we could
// have `BTreeSet<BTreeSet<ConflictItem>>`. But this in turn
// makes mutating the elements in each set (done above) more
// difficult and likely require more allocations.
//
// So if this does result in a perf slowdown on some real
// work-load, I think the first step would be to re-examine
// whether we're doing more work than we need to be doing. If
// we aren't, then we might want a more purpose-built data
// structure for this.
if !existing.contains(&set) {
existing.push(set);
change = true;
}
}
if seen.insert(child_edge.target()) || change {
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::{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 @@ -119,7 +119,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 @@ -172,7 +172,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
Loading
Loading