Simplify graph in get_topological_weights. #10574

mauritsvanrees · 2021-10-12T09:17:09Z

Fixes #10557 where the resolver spends too much time calculating the weights.

Also, do not let get_installation_order calculate these weights at all when there is nothing left to install.

I had to change one test in test_resolver.py because some weights have changed. The test was meant to address this comment and I think the new weights with new order are okay. Translated to that comment, the new order is that B is installed before D, but that should not matter, because A is still installed first, before C, which is the point of the comment.

This is my first PR for pip.
I wonder: should this PR be done in https://github.com/sarugaku/resolvelib first?

Meanwhile, let me share the output of a few runs with extra print statements.

Installing Zope:

$ bin/python -m pip install Zope -c https://dist.plone.org/release/pip-resolver-test/constraints.txt
...
Expected node count: 80
simplified graph to 66
simplified graph to 46
simplified graph to 32
simplified graph to 25
simplified graph to 23
simplified graph to 19
simplified graph to 17
simplified graph to 14
simplified graph to 8
simplified graph to 3
simplified graph to 2
simplified graph to 1

Same with Products.CMFPlone (a much larger superset of Zope):

Expected node count: 238
simplified graph to 209
simplified graph to 169
simplified graph to 151
simplified graph to 144
simplified graph to 140
simplified graph to 127
simplified graph to 116
simplified graph to 110
simplified graph to 103
simplified graph to 95
simplified graph to 91
simplified graph to 89
simplified graph to 79
simplified graph to 70
simplified graph to 65
simplified graph to 49
simplified graph to 40
simplified graph to 36
simplified graph to 35

So 35 packages in the graph is the maximum simplification. The original visit function is called on the remaining graph.

Same with Plone (a slightly larger superset of Products.CMFPlone):

Expected node count: 253
simplified graph to 221
simplified graph to 178
simplified graph to 159
simplified graph to 152
simplified graph to 148
simplified graph to 135
simplified graph to 124
simplified graph to 118
simplified graph to 111
simplified graph to 103
simplified graph to 99
simplified graph to 97
simplified graph to 87
simplified graph to 77
simplified graph to 72
simplified graph to 55
simplified graph to 46
simplified graph to 42
simplified graph to 41

In the last case, extra packages needed to be installed, and this order was calculated:

Installing collected packages: PyJWT, certifi, urllib3, idna, chardet, requests, plone.cachepurging,
plone.rest, Products.CMFPlacefulWorkflow, plone.app.iterate, plone.restapi, plone.app.upgrade,
plone.app.caching, Plone

That seems right. For example certify is installed before requests, and Plone is installed last.

Fixes #10557 where the resolver spends too much time calculating the weights. Also, do not let `get_installation_order` calculate these weights at all when there is nothing left to install.

uranusjr · 2021-10-12T09:40:16Z

I wonder: should this PR be done in sarugaku/resolvelib first?

Why? This does not touch anything in resolvelib. pip/_internal/resolution/resolvelib is not a part of resolvelib, only an interface to it. The resolvelib code is vendored in pip/_vendor/resolvelib, and this PR does not touch it at all.

mauritsvanrees · 2021-10-12T09:43:31Z

Why? This does not touch anything in resolvelib.

Ah, okay, good.

I do see lots of test failures now locally, so it seems I have some more work to do. But the idea can be reviewed.

mauritsvanrees · 2021-10-12T11:14:24Z

Local failures were due to parallel tox runs, I think.
Mypy fixed.

I am not used to mypy and did not have pre-commit installed yet.

mauritsvanrees · 2021-10-12T15:24:27Z

For good measure I tried with slightly older Plone constraints (https://dist.plone.org/release/5.2.5/constraints.txt) where I know there are still cyclic dependencies, and my code worked there as well. You need Python 3.8 or earlier for this.

Since this is my first PR, two GHA workflows are waiting for approval.

I am happy to squash the commits if that is preferred.

src/pip/_internal/resolution/resolvelib/resolver.py

uranusjr

If tests pass (I think they will)

After minor refactoring, the condition was the wrong way around.

'next' gives mypy errors.

mauritsvanrees · 2021-10-25T09:59:23Z

The mypy part of the lint checking fails, see actions:

src/pip/_internal/resolution/resolvelib/resolver.py:225: error: No overload
variant of "next" matches argument types "Iterable[Optional[str]]", "None" 
[call-overload]
                if next(graph.iter_children(key), None) is None:
                   ^
src/pip/_internal/resolution/resolvelib/resolver.py:225: note: Possible overload variant:
src/pip/_internal/resolution/resolvelib/resolver.py:225: note:     def [_T, _VT] next(__i, Iterator[_T], default: _VT) -> Union[_T, _VT]
src/pip/_internal/resolution/resolvelib/resolver.py:225: note:     <1 more non-matching overload not shown>
Found 1 error in 1 file (checked 283 source files)

Any idea how to solve that? I have not worked with mypy yet. I tried without the default=None argument, adding a try/except StopIteration, but that led to a similar error No overload variant of "next" matches argument type "Iterable[Optional[str]]".

I tried adding a method to DirectedGraph in src/pip/_vendor/resolvelib/structs.py:

def is_leaf(self, key):
    return not self._forwards[key]

and then calling graph.is_leaf(key) instead of the next / iter_children line, but that just leads to a different mypy error locally:

"DirectedGraph[Optional[str]]" has no attribute "is_leaf"  [attr-defined]

Okay, I went with the slightly more verbose code that @jbylund suggested, and now mypy is happy.

Can you restart the workflow please?

pfmoore · 2021-10-25T10:07:09Z

FWIW, graph.iter_children(key) probably returns an Iterable, not an Iterator. But next needs an iterator - next(iter(graph.iter_children(key)), None) would probably have worked. But what you've done is fine, too.

jbylund · 2021-10-25T12:58:23Z

I could see a future reader being confused about if there's somehow a copy of the graph due to the comments. My inclination would be to phrase this as an iterative (while can simplify graph) rather than the recursive, but that's just because of my perception that people find that a little easier to reason about. Curious to hear other takes on recursive v iterative here.

DiddiLeija

The general idea looks excellent for me!

mauritsvanrees · 2021-11-16T16:29:34Z

There seems agreement that my PR is fine. There is only the comment from @jbylund who wonders if it would be better to do simplify_graph as a while loop instead of an recursive function call. I could do that and it would look like this:

while True:
    ... call most of my simplify_graph function...
    if not leaves:
        # We are done simplifying. Recursive function has a 'return' here.
        break
    ... call the rest of my simplify_graph function...

If I do this, would it help this PR along towards merging?

Anything else I can do? Should I squash to one commit? Should I rebase on the main branch?

uranusjr · 2021-11-16T16:41:14Z

Converting traversal to a loop is likely a good idea; it’s not terribly likely we’d go over Python’s recursing limit (Python dependencies are generally not that deep; we’re not JavaScript), but you never know.

I can’t really speak to whether that’d help get this merged though. I want more people to take a look at the logic if possible, but if no-one raises any issues this will go in the next release. Note that we don’t expect to release anything until January 2022 anyway so it doesn’t really matter that much if this is merged very soon.

This is easier to wrap your head around.

mauritsvanrees · 2021-11-16T18:12:04Z

Thanks for the info. Okay, I have turned the recursive function into a loop.
Can you restart the workflow checks?

news/10557.bugfix.rst

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

jbylund · 2021-11-17T14:34:19Z

src/pip/_internal/resolution/resolvelib/resolver.py

+        weight = len(graph) - 1
+        for leaf in leaves:
+            weights[leaf] = weight
+        # Remove the leaves from the copy of the graph, making the copy simpler.


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

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.

jbylund

Up to you if you feel like the docstring of get_installation_order could be updated, since I don't think it's a great explanation of what's going on in the method. But looks good, thanks for the contribution.

mauritsvanrees · 2021-11-17T21:40:05Z

Up to you if you feel like the docstring of get_installation_order could be updated, since I don't think it's a great explanation of what's going on in the method.

I have updated the docstring of get_installation_order and get_topological_weights.

But looks good, thanks for the contribution.

Thank you, and thanks for the review!

src/pip/_internal/resolution/resolvelib/resolver.py

pradyunsg

This looks great and is super smart!

pradyunsg · 2021-11-20T13:30:53Z

Thanks @mauritsvanrees! ^.^

Simplify graph in get_topological_weights.

af38392

Fixes #10557 where the resolver spends too much time calculating the weights. Also, do not let `get_installation_order` calculate these weights at all when there is nothing left to install.

mauritsvanrees mentioned this pull request Oct 12, 2021

Resolver spends too much time calculating "weights" to parents #10557

Closed

1 task

mauritsvanrees added 2 commits October 12, 2021 13:11

fix mypy

3983756

Removed outdated comment and no longer needed code.

9a97320

Fixed another mypy warning, about DiGraph having no attr _forwards.

7e53097

I am not used to mypy and did not have pre-commit installed yet.

jbylund reviewed Oct 22, 2021

View reviewed changes

src/pip/_internal/resolution/resolvelib/resolver.py Outdated Show resolved Hide resolved

src/pip/_internal/resolution/resolvelib/resolver.py Outdated Show resolved Hide resolved

uranusjr reviewed Oct 23, 2021

View reviewed changes

src/pip/_internal/resolution/resolvelib/resolver.py Outdated Show resolved Hide resolved

get_topological_weights: minor improvements from reviews.

a6b930b

uranusjr approved these changes Oct 25, 2021

View reviewed changes

mauritsvanrees added 2 commits October 25, 2021 10:48

get_topological_weights: fixed condition.

11ce17e

After minor refactoring, the condition was the wrong way around.

get_topological_weights: iterate instead of calling next.

6145342

'next' gives mypy errors.

DiddiLeija approved these changes Oct 26, 2021

View reviewed changes

Replace the recursive simplify_graph function with a while loop.

b5c0b82

This is easier to wrap your head around.

uranusjr reviewed Nov 17, 2021

View reviewed changes

news/10557.bugfix.rst Outdated Show resolved Hide resolved

mauritsvanrees and others added 2 commits November 17, 2021 10:49

Update news/10557.bugfix.rst

9408cad

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

Fixed typo in news snippet.

d07cc8c

uranusjr added this to the 22.0 milestone Nov 17, 2021

jbylund reviewed Nov 17, 2021

View reviewed changes

Fixed comment that still talked about a copy of the graph.

43d9434

jbylund approved these changes Nov 17, 2021

View reviewed changes

Updated docstring of get_installation_order and get_topological_weights.

39ce7b5

uranusjr reviewed Nov 18, 2021

View reviewed changes

src/pip/_internal/resolution/resolvelib/resolver.py Outdated Show resolved Hide resolved

src/pip/_internal/resolution/resolvelib/resolver.py Outdated Show resolved Hide resolved

src/pip/_internal/resolution/resolvelib/resolver.py Outdated Show resolved Hide resolved

Reflow and fix typo in docstrings

69b71ca

pradyunsg approved these changes Nov 20, 2021

View reviewed changes

pradyunsg merged commit ba979fd into pypa:main Nov 20, 2021

This was referenced Nov 28, 2021

Release 2021-05 follow-up winpython/winpython#1030

Closed

remark: issue on my big requirement.txt file when pip-master-2021_11_28 #10689

Closed

github-actions bot locked as resolved and limited conversation to collaborators Dec 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify graph in get_topological_weights. #10574

Simplify graph in get_topological_weights. #10574

mauritsvanrees commented Oct 12, 2021

uranusjr commented Oct 12, 2021

mauritsvanrees commented Oct 12, 2021

mauritsvanrees commented Oct 12, 2021

mauritsvanrees commented Oct 12, 2021

uranusjr left a comment

mauritsvanrees commented Oct 25, 2021

pfmoore commented Oct 25, 2021

jbylund commented Oct 25, 2021

DiddiLeija left a comment

mauritsvanrees commented Nov 16, 2021 •

edited

Loading

uranusjr commented Nov 16, 2021

mauritsvanrees commented Nov 16, 2021

jbylund Nov 17, 2021

mauritsvanrees Nov 17, 2021

jbylund left a comment

mauritsvanrees commented Nov 17, 2021

pradyunsg left a comment

pradyunsg commented Nov 20, 2021

Simplify graph in get_topological_weights. #10574

Simplify graph in get_topological_weights. #10574

Conversation

mauritsvanrees commented Oct 12, 2021

uranusjr commented Oct 12, 2021

mauritsvanrees commented Oct 12, 2021

mauritsvanrees commented Oct 12, 2021

mauritsvanrees commented Oct 12, 2021

uranusjr left a comment

Choose a reason for hiding this comment

mauritsvanrees commented Oct 25, 2021

pfmoore commented Oct 25, 2021

jbylund commented Oct 25, 2021

DiddiLeija left a comment

Choose a reason for hiding this comment

mauritsvanrees commented Nov 16, 2021 • edited Loading

uranusjr commented Nov 16, 2021

mauritsvanrees commented Nov 16, 2021

jbylund Nov 17, 2021

Choose a reason for hiding this comment

mauritsvanrees Nov 17, 2021

Choose a reason for hiding this comment

jbylund left a comment

Choose a reason for hiding this comment

mauritsvanrees commented Nov 17, 2021

pradyunsg left a comment

Choose a reason for hiding this comment

pradyunsg commented Nov 20, 2021

mauritsvanrees commented Nov 16, 2021 •

edited

Loading