Implement "lazy sequence" to avoid Internet access during candidate search

[uranusjr]

find_matches() is modified to return a special type that implements the sequence protocol (instead of a plain list). This special sequence type tries to use the installed candidate as the first element if possible, and only access indexes when the installed candidate is considered unsatisfactory.

Outdated numbers

Using the reprod from #8664 (comment):

# Old resolver
real	0m2.209s
user	0m1.930s
sys	0m0.173s

# New resolver before patch
real	0m54.665s
user	0m36.713s
sys	0m1.416s

# New resolver with patch
real	0m4.261s
user	0m3.574s
sys	0m0.285s

I kind of feel the lazy sequence implementation should be pushed into resolvelib instead. The find_matches() would be modified to return a Callable[[], Optional[Iterator[Candidate]]] instead. This callable would be called multiple times by the resolver (to replace the need of iterating through the sequence multiple times in the current implementation).

Fix #8023.

[pradyunsg]

You didn't mention you were implementing this! :o (to be fair, neither did I)

A few minor comments inline.

[pradyunsg]

I kind of feel the lazy sequence implementation should be pushed into resolvelib instead. The find_matches() would be modified to return a Callable[[], Optional[Iterator[Candidate]]] instead. This callable would be called multiple times by the resolver (to replace the need of iterating through the sequence multiple times in the current implementation).

I quite like the sound of this. OTOH, it should be possible to do it make this change here now and have resolvelib add this support in a backwards-compatible way after-the-fact, based on feedback on this variant -- we could then flip the switch on this side after that. :)

[uranusjr]

You didn't mention you were implementing this! :o (to be fair, neither did I)

I was trying to do this in resolvelib initially, but found that the implementation needs to change quite a bit for it to work. The boolean check (i.e. whether the list is empty) is currently used to drive backtracking (if it’s empty, we’re hitting a dead end and should backtrack), and switching to an iterator means the resolver needs to change how it backtracks.

I think it’s probably a better plan to include this in pip for a while, to avoid resolvelib blocking pip releases.

[uranusjr]

Not sure how I should do the news fragment. #8905 is already taken by the cache PR.

--

Edit: I can probably use #8023 instead.

[uranusjr]

This plus the caching PR gets us to

real  0m3.660s
user  0m3.341s
sys   0m0.177s

# Old resolver from above.
real  0m2.209s
user  0m1.930s
sys   0m0.173s

The rest is probably due to the extra checks and not avoidable. For context, the new resolver emits 371 “Requirement already satisfied” messages; the older resolver only 163. These should correspond quite directly to how much computation each resolver does.

[uranusjr]

Uh, why is Travis reporting legistimate test errors while Azure claims everything is fine 😞

[pradyunsg]

Don't we only run the new resolver tests on Travis CI?

[pradyunsg]

I'mma add this to 20.3 as a release blocker, since I strongly believe that we should include this change in the main rollout release. Please holler if you disagree. :)

[uranusjr]

Oh, I misred and thought the failures are from test_new_resolver_*. I’ll try to fix things later today.

[uranusjr]

That should do it.

[uranusjr]

I was trying to address the upgrade strategy issue mentioned above, and ended up re-implementing all the iterator stuff. I actually feel this is much better than the previous class-based approach though, both shorter and easier to understand.

[brainwane]

@pradyunsg and I discussed this pull request in today's meeting. To make enough progress on #8664, we either need to merge this into pip, or we need to merge sarugaku/resolvelib#57 into resolvelib, then cut a resolvelib release, update the vendored version in pip, and update pip's implementation. Pradyun will talk with @uranusjr to figure out the best path forward.

[pradyunsg]

LGTM

[graingert]

@uranusjr @pradyunsg there's conflicts