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

Avoid downloading multiple candidates of a same version #9432

Merged
merged 1 commit into from
Jan 11, 2021

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Jan 5, 2021

Since 41a3008 (#9289), Candidate objects prepare their underlying distribution eagerly on creation, so the error can be caught deterministically to trigger backtracking.

This however has a negative side-effect. Since dist preparation involves metadata validation, a remote distribution must be downloaded on candidate creation. This means that an sdist will be downloaded, validated, and discarded (since its version is known to be incompatible) during backtracking.

This commit moves version deduplication of candidates from indexes to before the Candidate object is created, to avoid unneeded preparation.

Note that we still need another round of deduplication in FoundCandidates to remove duplicated candidates when a distribution is already installed.

News fragment omitted since the commit introducing this issue has not been released.

@uranusjr uranusjr added skip news Does not need a NEWS file entry (eg: trivial changes) type: bugfix labels Jan 5, 2021
@uranusjr uranusjr added this to the 20.3.4 milestone Jan 5, 2021
@uranusjr uranusjr force-pushed the new-resolver-dedup-on-backtracking branch from 05cfa6f to 9c12314 Compare January 5, 2021 22:09
Since 41a3008, Candidate objects prepare their underlying
distribution eagerly on creation, so the error can be caught
deterministically to trigger backtracking.

This however has a negative side-effect. Since dist preparation involves
metadata validation, a remote distribution must be downloaded on
Candidate creation. This means that an sdist will be downloaded,
validated, and discarded (since its version is known to be incompatible)
during backtracking.

This commit moves version deduplication of candidates from indexes to
before the Candidate object is created, to avoid unneeded preparation.

Note that we still need another round of deduplication in FoundCandidates
to remove duplicated candidates when a distribution is already installed.
@uranusjr uranusjr force-pushed the new-resolver-dedup-on-backtracking branch from 9c12314 to 79cbe6b Compare January 6, 2021 09:20
@uranusjr uranusjr changed the title Avoid candidates of the same version are created Avoid downloading multiple candidates of a same version Jan 6, 2021
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.

LGTM

@pradyunsg pradyunsg merged commit c1251a8 into pypa:master Jan 11, 2021
@pradyunsg
Copy link
Member

This one's gonna be a fun one to merge on 20.3.3.

@pradyunsg pradyunsg removed their request for review January 11, 2021 19:31
@pfmoore
Copy link
Member

pfmoore commented Jan 11, 2021

... which reminds me, are you still planning on doing a 20.3.4 release? If so, are you expecting to do it before or after 21.0? I doubt it makes much difference in practice, but I don't want to mess things up for you if I start looking at 21.0 stuff (e.g. vendoring updates, which I should probably do sooner rather than later...)

@pradyunsg
Copy link
Member

Yea... I was figuring that out just now. I'm thinking of cutting it sometime this week.

I'm basically going to be cherry picking the corresponding commits onto 20.3.3, so feel free to do whatever in the master branch. It won't really affect that work.

I do think we'll need to create some sort of tracker for the legacy resolver removal though, so I've made a milestone for that. We'll want to populate that soon enough and let downstream users who're being noisy know that.

Also, I just realized that I'm supposed to have dinner too, so, well, wheeeeeee!

pradyunsg added a commit to pradyunsg/pip that referenced this pull request Jan 23, 2021
…tracking

Avoid downloading multiple candidates of a same version
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 <3275593+pradyunsg@users.noreply.github.com>
Co-authored-by: Stéphane Bidoul <stephane.bidoul@gmail.com>
Co-authored-by: Pradyun Gedam <pradyunsg@users.noreply.github.com>
@uranusjr uranusjr deleted the new-resolver-dedup-on-backtracking branch February 19, 2021 06:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants