Split find_matches into generation and sorting #8234

pfmoore · 2020-05-13T16:56:37Z

OK, this refactors find_matches so that the factory is only responsible for generating a list of acceptable matches, and it's the provider's job to sort them. This means that the set of "root" requirements (the ones input by the user) are no longer needed in the factory and can be saved on the provider, restoring the factory to its role of acting as a stateless interface to the finder, preparer, etc.

The code is still pretty messy. I'm fairly sure that this is mainly because the behaviour we're trying to implement is messy, so I don't think it's something we can eliminate totally. But I've been working on this for a while now, and I may well be too close to it to see obvious simplifications, so I'd appreciate any suggestions on that score.

I'm pretty sure that (style aside) this is OK to go, so IMO it's ready for review.

uranusjr · 2020-05-13T19:01:54Z

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

+    # For an ExtrasCandidate, we check the base
+    if isinstance(cand, ExtrasCandidate):
+        cand = cand.base
+    return isinstance(cand, AlreadyInstalledCandidate)


I wonder if it’s better to add an is_installed() on candidates… or even abstract out the keep_installed() function into candidate subclasses.

We can't abstract out keep_installed() out since it depends on the upgrade_strategy.

+1 on adding is_installed() -- although, as a read-only property feels a lot cleaner.

I considered that, but wasn't sure if it was too intrusive (it felt a bit specialised). But as both of you seem OK with the idea, I'll do this.

OK. I made is_installed an attribute on the class, and added an equivalent property to the Candidate class. That seems to work, although mypy failed to catch that I forgot to add the attribute to RequiresPythonCandidate. I understand why (there's a default implementation in Candidate) but is there really no way to say in mypy "every subclass of this class needs to have an attribute is_installed"? Or to say "every object of type Candidate must have an is_instance attribute" and have class attributes count?

I think that’s what Protocol is for, but it’s a pain to use Protocol on comment-style annotations. Python 2 😞

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

uranusjr · 2020-05-13T19:19:34Z

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

@@ -82,6 +81,8 @@ def resolve(self, root_reqs, check_supported_wheels):
                else:
                    constraints[name] = req.specifier
            else:
+                if req.is_direct and req.name:
+                    roots.add(canonicalize_name(req.name))


I wonder if ther would be edge cases where an “unnamed” requirement messes up priority logic later. Would it be a good idea if we resolve all names eagerly here? We have to do it soon for resolvelib anyway.

Honestly, I'm not sure. I'm really cautious about preparing too early, because it feels so easy to end up with another mess like canonical names¹, where we keep going "let's resolve names now, because we don't know if it's a problem". That's why I like the new candidate approach of having a lazy property. Hopefully when we switch over to the project model, projects will do the same.

I'd like to understand why we need to resolve the name of an unnamed requirement here, if we do need to (we're talking about pip install --upgrade /local/dir some other stuff where we need to know that /local/dir is a root so that we know to upgrade it... uh, what?) Let's wait till something breaks, then we can write a test that clarifies why we need to do this, and then do it. At the moment, I'm not even sure what pip install --upgrade /local/dir should mean...

¹ I added that canonicalize_name call there as a result of a test failure, and it was another case of "just canonicalise, it's easier than trying to work out how come it's not already been done" 🙁

uranusjr

The logic looks great! I have only one doubt regarding how we populate roots, all other comments are about the interface.

pradyunsg

The main refactor looks great to me!

A bunch of naming/cleanup suggestions, which I don't feel too strongly about. :)

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

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

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

pradyunsg · 2020-05-13T19:22:22Z

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

+    # For an ExtrasCandidate, we check the base
+    if isinstance(cand, ExtrasCandidate):
+        cand = cand.base
+    return isinstance(cand, AlreadyInstalledCandidate)


We can't abstract out keep_installed() out since it depends on the upgrade_strategy.

+1 on adding is_installed() -- although, as a read-only property feels a lot cleaner.

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

pradyunsg · 2020-05-13T19:45:23Z

Please remind me to file a PR that is the result of running the following on pip:

find ./src/pip/_internal ./tests/unit -name "*.py" -type f -exec sed -i '' 's/\.is_direct/.is_user_provided/' {} \;

InstallRequirement.{is_direct -> is_user_provided}

pradyunsg · 2020-05-13T19:50:41Z

Whoops. I accidentally restarted the Travis CI build here, when I was trying to restart the master Travis CI build. Sorry!

uranusjr · 2020-05-14T12:28:30Z

I wonder if this is relevant to #7678.

pfmoore · 2020-05-14T14:31:03Z

@uranusjr Probably. Also relevant to the discussion we had about test_upgrade_to_same_version_from_url. I think the new resolvelib behaviour (where we merge "all the stuff the resolver has seen" in find_matches) will make these easier to handle, so I propose we defer worrying about them until that resolvelib release is merged.

On that basis, is there anything more you want to see done with this PR before I merge it?

pradyunsg · 2020-05-14T17:09:16Z

I'm happy with this as is. LGTM! :)

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

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

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

pradyunsg · 2020-05-15T10:02:15Z

@pfmoore :)

I'll do the find | sed after this PR is merged.

pfmoore · 2020-05-15T10:12:43Z

Assuming the tests pass, I'll merge this once @uranusjr gives the OK. It's blocking a couple of things, now, so I'd like to get it in.

Split find_matches into generation and sorting

ddbc8fd

pfmoore requested review from uranusjr and pradyunsg May 13, 2020 16:56

pfmoore added the skip news Does not need a NEWS file entry (eg: trivial changes) label May 13, 2020

Remove left-over print from debugging

0db022f

uranusjr reviewed May 13, 2020

View reviewed changes

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

uranusjr reviewed May 13, 2020

View reviewed changes

src/pip/_internal/resolution/resolvelib/provider.py Show resolved Hide resolved

uranusjr reviewed May 13, 2020

View reviewed changes

pradyunsg approved these changes May 13, 2020

View reviewed changes

pradyunsg added C: new resolver type: refactor Refactoring code labels May 13, 2020

Address review comments

9cf1bed

pfmoore requested a review from uranusjr May 14, 2020 14:29

uranusjr reviewed May 14, 2020

View reviewed changes

src/pip/_internal/resolution/resolvelib/candidates.py Show resolved Hide resolved

uranusjr reviewed May 14, 2020

View reviewed changes

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

uranusjr reviewed May 14, 2020

View reviewed changes

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

Type annotations

96b3377

pfmoore mentioned this pull request May 15, 2020

Rename InstallRequirement is_direct to user_specified #8242

Closed

uranusjr approved these changes May 15, 2020

View reviewed changes

pfmoore merged commit d42c448 into pypa:master May 15, 2020

pfmoore deleted the refactor_find_matches branch May 15, 2020 13:03

uranusjr mentioned this pull request May 19, 2020

Refactor new resolver to remove state from factory #8195

Closed

pfmoore mentioned this pull request Jun 4, 2020

New Resolver: Refactor iter_found_candidates #8205

Closed

github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split find_matches into generation and sorting #8234

Split find_matches into generation and sorting #8234

pfmoore commented May 13, 2020

uranusjr May 13, 2020 •

edited

Loading

pradyunsg May 13, 2020

pfmoore May 14, 2020

pfmoore May 14, 2020

uranusjr May 14, 2020

uranusjr May 13, 2020

pfmoore May 14, 2020

uranusjr left a comment •

edited

Loading

pradyunsg left a comment

pradyunsg May 13, 2020

pradyunsg commented May 13, 2020

pradyunsg commented May 13, 2020

uranusjr commented May 14, 2020

pfmoore commented May 14, 2020

pradyunsg commented May 14, 2020

pradyunsg commented May 15, 2020

pfmoore commented May 15, 2020

Split find_matches into generation and sorting #8234

Split find_matches into generation and sorting #8234

Conversation

pfmoore commented May 13, 2020

uranusjr May 13, 2020 • edited Loading

Choose a reason for hiding this comment

pradyunsg May 13, 2020

Choose a reason for hiding this comment

pfmoore May 14, 2020

Choose a reason for hiding this comment

pfmoore May 14, 2020

Choose a reason for hiding this comment

uranusjr May 14, 2020

Choose a reason for hiding this comment

uranusjr May 13, 2020

Choose a reason for hiding this comment

pfmoore May 14, 2020

Choose a reason for hiding this comment

uranusjr left a comment • edited Loading

Choose a reason for hiding this comment

pradyunsg left a comment

Choose a reason for hiding this comment

pradyunsg May 13, 2020

Choose a reason for hiding this comment

pradyunsg commented May 13, 2020

pradyunsg commented May 13, 2020

uranusjr commented May 14, 2020

pfmoore commented May 14, 2020

pradyunsg commented May 14, 2020

pradyunsg commented May 15, 2020

pfmoore commented May 15, 2020

uranusjr May 13, 2020 •

edited

Loading

uranusjr left a comment •

edited

Loading