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

Simplify graph in get_topological_weights. #10574

Merged
merged 13 commits into from
Nov 20, 2021
Merged
Show file tree
Hide file tree
Changes from 7 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
1 change: 1 addition & 0 deletions news/10557.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Simplify the graph when calculating weights for the installation order.
mauritsvanrees marked this conversation as resolved.
Show resolved Hide resolved
34 changes: 34 additions & 0 deletions src/pip/_internal/resolution/resolvelib/resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,10 @@ def get_installation_order(
"""
assert self._result is not None, "must call resolve() first"

if not req_set.requirements:
# Nothing is left to install, so we do not need an order.
return []

graph = self._result.graph
weights = get_topological_weights(
graph,
Expand Down Expand Up @@ -213,6 +217,30 @@ def get_topological_weights(
path: Set[Optional[str]] = set()
weights: Dict[Optional[str], int] = {}

def simplify_graph() -> None:
leaves = set()
for key in graph:
if key is None:
continue
for _child in graph.iter_children(key):
# This means we have at least one child
break
else:
# No child.
leaves.add(key)
if not leaves:
# We are done simplifying.
return
# Calculate the weight for the leaves.
weight = len(graph) - 1
for leaf in leaves:
weights[leaf] = weight
# Remove the leaves from the copy of the graph, making the copy simpler.
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly misleading comment, as there is no longer a copy of the graph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you are right. You mentioned this earlier, but I misunderstood, thinking your comment was outdated, referring to an older version of my PR.
I have fixed the comment.

for leaf in leaves:
graph.remove(leaf)
# Now that we have a simpler graph, try to simplify it again.
simplify_graph()

def visit(node: Optional[str]) -> None:
if node in path:
# We hit a cycle, so we'll break it here.
Expand All @@ -227,6 +255,12 @@ def visit(node: Optional[str]) -> None:
last_known_parent_count = weights.get(node, 0)
weights[node] = max(last_known_parent_count, len(path))

# Recursively simplify the graph, pruning leaves that have no dependencies.
# This is needed for large graphs (say over 200 packages) because the
# `visit` function is exponentially slower then, taking minutes.
# See https://github.com/pypa/pip/issues/10557
simplify_graph()
# Visit the remaining graph.
# `None` is guaranteed to be the root node by resolvelib.
visit(None)

Expand Down
2 changes: 1 addition & 1 deletion tests/unit/resolution_resolvelib/test_resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ def test_new_resolver_get_installation_order(
("three", "four"),
("four", "five"),
],
{None: 0, "one": 1, "two": 1, "three": 2, "four": 3, "five": 4},
{None: 0, "five": 5, "four": 4, "one": 4, "three": 2, "two": 1},
),
(
"linear",
Expand Down