diff --git a/src/pip/_internal/resolution/resolvelib/resolver.py b/src/pip/_internal/resolution/resolvelib/resolver.py index cba5a496508..5ec4a97b6a7 100644 --- a/src/pip/_internal/resolution/resolvelib/resolver.py +++ b/src/pip/_internal/resolution/resolvelib/resolver.py @@ -15,9 +15,10 @@ from .factory import Factory if MYPY_CHECK_RUNNING: - from typing import Dict, List, Optional, Tuple + from typing import Dict, List, Optional, Set, Tuple from pip._vendor.resolvelib.resolvers import Result + from pip._vendor.resolvelib.structs import Graph from pip._internal.cache import WheelCache from pip._internal.index.package_finder import PackageFinder @@ -114,42 +115,21 @@ def resolve(self, root_reqs, check_supported_wheels): def get_installation_order(self, req_set): # type: (RequirementSet) -> List[InstallRequirement] - """Create a list that orders given requirements for installation. + """Get order for installation of requirements in RequirementSet. - The returned list should contain all requirements in ``req_set``, - so the caller can loop through it and have a requirement installed - before the requiring thing. + The returned list contains a requirement before another that depends on + it. This helps ensure that the environment is kept consistent as they + get installed one-by-one. - The current implementation walks the resolved dependency graph, and - make sure every node has a greater "weight" than all its parents. + The current implementation creates a topological ordering of the + dependency graph, while breaking any cycles in the graph at arbitrary + points. We make no guarantees about where the cycle would be broken, + other than they would be broken. """ assert self._result is not None, "must call resolve() first" - weights = {} # type: Dict[Optional[str], int] graph = self._result.graph - key_count = len(self._result.mapping) + 1 # Packages plus sentinal. - while len(weights) < key_count: - progressed = False - for key in graph: - if key in weights: - continue - parents = list(graph.iter_parents(key)) - if not all(p in weights for p in parents): - continue - if parents: - weight = max(weights[p] for p in parents) + 1 - else: - weight = 0 - weights[key] = weight - progressed = True - - # FIXME: This check will fail if there are unbreakable cycles. - # Implement something to forcifully break them up to continue. - if not progressed: - raise InstallationError( - "Could not determine installation order due to cicular " - "dependency." - ) + weights = get_topological_weights(graph) sorted_items = sorted( req_set.requirements.items(), @@ -159,6 +139,52 @@ def get_installation_order(self, req_set): return [ireq for _, ireq in sorted_items] +def get_topological_weights(graph): + # type: (Graph) -> Dict[Optional[str], int] + """Assign weights to each node based on how "deep" they are. + + This implementation may change at any point in the future without prior + notice. + + We take the length for the longest path to any node from root, ignoring any + paths that contain a single node twice (i.e. cycles). This is done through + a depth-first search through the graph, while keeping track of the path to + the node. + + Cycles in the graph result would result in node being revisited while also + being it's own path. In this case, take no action. This helps ensure we + don't get stuck in a cycle. + + When assigning weight, the longer path (i.e. larger length) is preferred. + """ + path = set() # type: Set[Optional[str]] + weights = {} # type: Dict[Optional[str], int] + + def visit(node): + # type: (Optional[str]) -> None + if node in path: + # We hit a cycle, so we'll break it here. + return + + # Time to visit the children! + path.add(node) + for child in graph.iter_children(node): + visit(child) + path.remove(node) + + last_known_parent_count = weights.get(node, 0) + weights[node] = max(last_known_parent_count, len(path)) + + # `None` is guaranteed to be the root node by resolvelib. + visit(None) + + # Sanity checks + assert weights[None] == 0 + assert len(weights) == len(graph) + + return weights + + def _req_set_item_sorter( item, # type: Tuple[str, InstallRequirement] weights, # type: Dict[Optional[str], int] diff --git a/tests/unit/resolution_resolvelib/test_resolver.py b/tests/unit/resolution_resolvelib/test_resolver.py index aef80f55e39..6ae6bd7936f 100644 --- a/tests/unit/resolution_resolvelib/test_resolver.py +++ b/tests/unit/resolution_resolvelib/test_resolver.py @@ -6,7 +6,10 @@ from pip._internal.req.constructors import install_req_from_line from pip._internal.req.req_set import RequirementSet -from pip._internal.resolution.resolvelib.resolver import Resolver +from pip._internal.resolution.resolvelib.resolver import ( + Resolver, + get_topological_weights, +) @pytest.fixture() @@ -26,6 +29,21 @@ def resolver(preparer, finder): return resolver +def _make_graph(edges): + """Build graph from edge declarations. + """ + + graph = DirectedGraph() + for parent, child in edges: + parent = canonicalize_name(parent) if parent else None + child = canonicalize_name(child) if child else None + for v in (parent, child): + if v not in graph: + graph.add(v) + graph.connect(parent, child) + return graph + + @pytest.mark.parametrize( "edges, ordered_reqs", [ @@ -40,9 +58,9 @@ def resolver(preparer, finder): ( [ (None, "toporequires"), - (None, "toporequire2"), - (None, "toporequire3"), - (None, "toporequire4"), + (None, "toporequires2"), + (None, "toporequires3"), + (None, "toporequires4"), ("toporequires2", "toporequires"), ("toporequires3", "toporequires"), ("toporequires4", "toporequires"), @@ -59,15 +77,7 @@ def resolver(preparer, finder): ], ) def test_new_resolver_get_installation_order(resolver, edges, ordered_reqs): - # Build graph from edge declarations. - graph = DirectedGraph() - for parent, child in edges: - parent = canonicalize_name(parent) if parent else None - child = canonicalize_name(child) if child else None - for v in (parent, child): - if v not in graph: - graph.add(v) - graph.connect(parent, child) + graph = _make_graph(edges) # Mapping values and criteria are not used in test, so we stub them out. mapping = {vertex: None for vertex in graph if vertex is not None} @@ -80,3 +90,147 @@ def test_new_resolver_get_installation_order(resolver, edges, ordered_reqs): ireqs = resolver.get_installation_order(reqset) req_strs = [str(r.req) for r in ireqs] assert req_strs == ordered_reqs + + +@pytest.mark.parametrize( + "name, edges, expected_weights", + [ + ( + # From https://github.com/pypa/pip/pull/8127#discussion_r414564664 + "deep second edge", + [ + (None, "one"), + (None, "two"), + ("one", "five"), + ("two", "three"), + ("three", "four"), + ("four", "five"), + ], + {None: 0, "one": 1, "two": 1, "three": 2, "four": 3, "five": 4}, + ), + ( + "linear", + [ + (None, "one"), + ("one", "two"), + ("two", "three"), + ("three", "four"), + ("four", "five"), + ], + {None: 0, "one": 1, "two": 2, "three": 3, "four": 4, "five": 5}, + ), + ( + "linear AND root -> two", + [ + (None, "one"), + ("one", "two"), + ("two", "three"), + ("three", "four"), + ("four", "five"), + (None, "two"), + ], + {None: 0, "one": 1, "two": 2, "three": 3, "four": 4, "five": 5}, + ), + ( + "linear AND root -> three", + [ + (None, "one"), + ("one", "two"), + ("two", "three"), + ("three", "four"), + ("four", "five"), + (None, "three"), + ], + {None: 0, "one": 1, "two": 2, "three": 3, "four": 4, "five": 5}, + ), + ( + "linear AND root -> four", + [ + (None, "one"), + ("one", "two"), + ("two", "three"), + ("three", "four"), + ("four", "five"), + (None, "four"), + ], + {None: 0, "one": 1, "two": 2, "three": 3, "four": 4, "five": 5}, + ), + ( + "linear AND root -> five", + [ + (None, "one"), + ("one", "two"), + ("two", "three"), + ("three", "four"), + ("four", "five"), + (None, "five"), + ], + {None: 0, "one": 1, "two": 2, "three": 3, "four": 4, "five": 5}, + ), + ( + "linear AND one -> four", + [ + (None, "one"), + ("one", "two"), + ("two", "three"), + ("three", "four"), + ("four", "five"), + ("one", "four"), + ], + {None: 0, "one": 1, "two": 2, "three": 3, "four": 4, "five": 5}, + ), + ( + "linear AND two -> four", + [ + (None, "one"), + ("one", "two"), + ("two", "three"), + ("three", "four"), + ("four", "five"), + ("two", "four"), + ], + {None: 0, "one": 1, "two": 2, "three": 3, "four": 4, "five": 5}, + ), + ( + "linear AND four -> one (cycle)", + [ + (None, "one"), + ("one", "two"), + ("two", "three"), + ("three", "four"), + ("four", "five"), + ("four", "one"), + ], + {None: 0, "one": 1, "two": 2, "three": 3, "four": 4, "five": 5}, + ), + ( + "linear AND four -> two (cycle)", + [ + (None, "one"), + ("one", "two"), + ("two", "three"), + ("three", "four"), + ("four", "five"), + ("four", "two"), + ], + {None: 0, "one": 1, "two": 2, "three": 3, "four": 4, "five": 5}, + ), + ( + "linear AND four -> three (cycle)", + [ + (None, "one"), + ("one", "two"), + ("two", "three"), + ("three", "four"), + ("four", "five"), + ("four", "three"), + ], + {None: 0, "one": 1, "two": 2, "three": 3, "four": 4, "five": 5}, + ), + ], +) +def test_new_resolver_topological_weights(name, edges, expected_weights): + graph = _make_graph(edges) + + weights = get_topological_weights(graph) + assert weights == expected_weights