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

Lazily evaluate the candidate sequence to prevent unneeded distribution downloads #9289

Merged
merged 4 commits into from
Dec 27, 2020

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Dec 15, 2020

Fix #9203, fix #9246 (again).

This PR re-applies the reverted #9264, and adds a fix to the issue that caused it to be reverted in 20.3.2.

The experiement to follow up #9288. I’m not quite sure why we didn’t do this previously… But let’s see if this passes tests.

@pradyunsg
Copy link
Member

pradyunsg commented Dec 15, 2020

I feel like a full revert of #9264 + release is a better short-term plan, because we know we'll end up in a "clean" state then.

@uranusjr
Copy link
Member Author

Agreed.

@uranusjr
Copy link
Member Author

Ah, I see. We didn’t do this because this fails to return the installed candidate if its version is not found on the index.

@uranusjr uranusjr force-pushed the new-resolver-lazy-insert branch 2 times, most recently from 6dea6da to 93ec9a2 Compare December 15, 2020 09:59
@uranusjr uranusjr changed the title New resolver incorrectly trys unneeded candidates Lazily evaluate the candidate sequence to prevent unneeded distribution downloads Dec 15, 2020
@uranusjr
Copy link
Member Author

Hopefully this does the trick.

@uranusjr uranusjr marked this pull request as ready for review December 15, 2020 10:01
@uranusjr uranusjr force-pushed the new-resolver-lazy-insert branch 3 times, most recently from 7de21f1 to eee31c0 Compare December 15, 2020 14:26
@uranusjr
Copy link
Member Author

Alright, rebased this to master + re-applying the skip candidate PR. This should be ready to review once 20.3.3 is out.

news/9246.bugfix.rst Outdated Show resolved Hide resolved
Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

Just one suggestion for an explanatory comment, otherwise LGTM!

@pradyunsg
Copy link
Member

I came to merge, but there's conflicts here.

Skip candidate not providing valid metadata

This reverts commit 7165ab8.
When the new resolver needs to upgrade a package, it puts the
already-installed package in the middle of the candidate list obtained
from indexes. But when doing it, the candidate list is eagerly consumed,
causing pip to download all candidates.
@pradyunsg pradyunsg merged commit 2aea6e8 into pypa:master Dec 27, 2020
@pradyunsg pradyunsg added this to the 20.3.4 milestone Dec 27, 2020
@uranusjr uranusjr deleted the new-resolver-lazy-insert branch December 27, 2020 12:07
pradyunsg added a commit to pradyunsg/pip that referenced this pull request Jan 23, 2021
jsirois added a commit to pex-tool/pip that referenced this pull request Feb 1, 2021
* Merge pull request pypa#9289 from uranusjr/new-resolver-lazy-insert

* Merge pull request pypa#9315 from pradyunsg/better-search-errors

* Merge pull request pypa#9369 from uranusjr/resolvelib-054

* Merge pull request pypa#9320 from uranusjr/wheel-check-valid

Verify built wheel contains valid metadata

* Merge pull request pypa#9432 from uranusjr/new-resolver-dedup-on-backtracking

Avoid downloading multiple candidates of a same version

* Revert "Remove on_returncode parameter from call_subprocess"

This reverts commit ab3ee71.

* Revert "Remove show_stdout from run_command args"

This reverts commit 94882fd.

* Revert "Create call_subprocess just for vcs commands"

This reverts commit 8adbc21.

* Revert "Improve check for svn version string"

This reverts commit 1471897.

* Revert "Bubble up SubProcessError to basecommand._main"

This reverts commit e9f738a.

* Additional revert of 7969

Revert additional changes that were made
after 7969 and depended on it.

* add stdout_only to call_subprocess

* vcs: capture subprocess stdout only

* Add test for call_subprocess stdout_only

* Bump for release

* Fix test bitrot.

Setuptools released 52.0.0 which killed easy_install support and thus
caused tests exercising easy_install to fail.

Co-authored-by: Pradyun Gedam <[email protected]>
Co-authored-by: Stéphane Bidoul <[email protected]>
Co-authored-by: Pradyun Gedam <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants