Refactor Resolution type to retain dependency graph
#9106
Conversation
charliermarsh
added
the
internal
charliermarsh
force-pushed
the
charlie/graph
branch
6 times, most recently
from
5d17f84 to
a20e24d
Compare
charliermarsh
force-pushed
the
charlie/out
branch
from
d7148e2 to
b272b62
Compare
charliermarsh
force-pushed
the
charlie/graph
branch
from
a20e24d to
18317fb
Compare
| @@ -166,15 +172,48 @@ impl Diagnostic for ResolutionDiagnostic { | |||
| } | |||
| } | |||
|
|
|||
| /// A node in the resolution, along with whether its been filtered out. | |||
| /// | |||
| /// This is similar to [`ResolutionGraph`], but represents a resolution for a single platform. | |||
There was a problem hiding this comment.
Is "single platform" correct here? I think it might be, "subset of all marker environments" instead.
Although, this is also for a Node... Should this be moved to the docs for Resolution?
| /// dropping any extras and dependency groups from the graph nodes. Instead, each package is | ||
| /// collapsed into a single node, with extras and dependency groups annotating the _edges_, rather | ||
| /// than being represented as separate nodes. This is a more natural representation, but a further | ||
| /// departure from the PubGrub model. |
| /// departure from the PubGrub model. | ||
| /// | ||
| /// For simplicity, this transformation makes the assumption that the resolution only applies to a | ||
| /// single platform, i.e., it shouldn't be called on universal resolutions. |
There was a problem hiding this comment.
Same "single platform" phrasing here... should it also be "subset of all marker environments"? But the last "shouldn't be called" part suggests maybe not...
There was a problem hiding this comment.
And if "single platform" is right, should we have an assertion like assert!(output.fork_markers.is_empty()) to enforce this?
| use uv_pypi_types::{HashDigest, RequirementSource}; | ||
|
|
||
| use crate::{BuiltDist, Diagnostic, Dist, Name, ResolvedDist, SourceDist}; | ||
|
|
||
| /// A set of packages pinned at specific versions. | ||
| #[derive(Debug, Default, Clone)] | ||
| pub struct Resolution { | ||
| packages: BTreeMap<PackageName, ResolvedDist>, | ||
| hashes: BTreeMap<PackageName, Vec<HashDigest>>, | ||
| graph: Graph<Node, Edge, Directed>, |
There was a problem hiding this comment.
Nit: Node, Edge, Directed sounds like they are generic types from petgraph itself.
|
|
||
| // Add an edge from the root. | ||
| let index = inverse[&dist.id]; | ||
| petgraph.add_edge(root, index, Edge::Prod(MarkerTree::TRUE)); |
There was a problem hiding this comment.
I really that we don't have orphan nodes anymore, but aren't those a bit different in that they aren't mandatory, but depend on the user selection? (This may just be a documentation issue)
charliermarsh
force-pushed
the
charlie/out
branch
from
b272b62 to
7c2eb9f
Compare
charliermarsh
force-pushed
the
charlie/graph
branch
from
18317fb to
6efd07c
Compare
charliermarsh
force-pushed
the
charlie/graph
branch
from
6efd07c to
93678b1
Compare
charliermarsh
force-pushed
the
charlie/graph
branch
from
93678b1 to
30691c9
Compare
|
Thank you both! |
Summary
This PR should not contain any user-visible changes, but the goal is to refactor the
Resolutiontype to retain a dependency graph. We want to be able to explain why a given package was excluded on error (see: #8962), which in turn requires that at install time, we can go back and figure out the dependency chain. At present,Resolutionis just a map from package name to distribution; this PR remodels it as a graph in which each node is a package, and the edges contain markers plus extras or dependency groups.