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.

Sign up for GitHub

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

Jump to bottom

Move wheel cache out of InstallRequirement #7801

Merged
merged 4 commits into from
Mar 31, 2020
Merged

Move wheel cache out of InstallRequirement #7801

merged 4 commits into from
Mar 31, 2020

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Feb 28, 2020 •
edited
Loading

In #7796 it was discussed how the “link is yanked” warning should not live inside the finder, since the new resolver may not actually end up using that found link. It was later discovered InstallRequirement.populate_link() is also not the correct location to log this, since an InstallRequirement with link populated (i.e. turned into a candidate) may still be discarded when the resolver backtracks.

The next idea is to put that logging code into a separate method for the resolver to call. However, the current implementation of populate_link() eagerly checks the link against the wheel cache, and replace the link if that cache hits. So to make the idea possible, the resolver also needs to invoke the cache look-up step.

With that conclusion, this refactoring removed the wheel_cache attribute from InstallRequirement (since it is not used anywhere else), and puts it on Resolver instead. populate_link() (the only usage of the wheel cache) is also moved to the resolver, and will be broken down in a later PR to move the yanked link warning.

@uranusjr uranusjr mentioned this pull request Feb 28, 2020
Closed
@uranusjr uranusjr added the skip news Does not need a NEWS file entry (eg: trivial changes) label Feb 28, 2020
@uranusjr uranusjr force-pushed the yanked-link-refactor branch 3 times, most recently from 8603e9a to d998eaf Compare February 28, 2020 10:23
@uranusjr uranusjr marked this pull request as ready for review February 28, 2020 13:57
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Mar 12, 2020
@uranusjr uranusjr mentioned this pull request Mar 29, 2020
Open
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Mar 30, 2020
@uranusjr
Copy link
Member Author

uranusjr commented Mar 30, 2020 •
edited
Loading

Reviving this since there are some recent discussion around the wheel cache that makes this relevent, e.g. #7815 and its PR #7898. Currently the only use case for InstallRequirement._wheel_cache is InstallRequirement.populate_link(), but works on the new resolver reveals that this method is resolver-dependent, and should be moved out of InstallRequirement. As a concequence, this allows us to to remove the wheel cache reference from InstallRequirement entirely.

cc @sbidoul @pradyunsg

@uranusjr uranusjr force-pushed the yanked-link-refactor branch 2 times, most recently from 95cc048 to 2f436f2 Compare March 30, 2020 09:07
@uranusjr
Copy link
Member Author

p.s. The functional change is much less significant than the diff suggests. Half of them is simply removing the wheel_cache argument from various InstallRequirement constructors.

pradyunsg
pradyunsg reviewed Mar 30, 2020
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

Did a cursory skim; LGTM in principle and implementation as well.

src/pip/_internal/operations/freeze.py Outdated
line,
isolated=isolated,
wheel_cache=wheel_cache,
line, isolated=isolated,
Copy link
Member

Choose a reason for hiding this comment

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

nit: maintain existing style of one-per-line?

Copy link
Member Author

Choose a reason for hiding this comment

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

I’ll revert this if there’s anything else I should change.

src/pip/_internal/resolution/legacy/resolver.py
@@ -256,6 +260,34 @@ def _check_skip_installed(self, req_to_install):
self._set_req_to_reinstall(req_to_install)
return None

def _populate_link(self, req):
Copy link
Member

Choose a reason for hiding this comment

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

Ooh, nice.

My brain is too fixed at thinking of populate_link as a method of InstallRequirement to have thought of doing this.

@sbidoul sbidoul changed the title Move wheel cache out of InstallRequirment Move wheel cache out of InstallRequirement Mar 30, 2020
pradyunsg
pradyunsg reviewed Mar 30, 2020
View reviewed changes
src/pip/_internal/resolution/legacy/resolver.py Outdated
Comment on lines 265 to 274
"""Ensure that if a link can be found for this, that it is found.

Note that req.link may still be None - if Upgrade is False and the
requirement is already installed.

If preparer.require_hashes is True, don't use the wheel cache, because
cached wheels, always built locally, have different hashes than the
files downloaded from the index server and thus throw false hash
mismatches. Furthermore, cached wheels at present have undeterministic
contents due to file modification times.
Copy link
Member

Choose a reason for hiding this comment

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

The docstring could use an update.

"upgrade is false" -> requirement may be upgraded, based on upgrade-strategy

Copy link
Member Author

Choose a reason for hiding this comment

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

This would couple this docstring with the implementation of _is_upgrade_allowed(). Would it be better to simply mention “if _is_upgrade_allowed() is True”?

pradyunsg
pradyunsg approved these changes Mar 30, 2020
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

LGTM other than the minor comments made.

@uranusjr uranusjr requested a review from pradyunsg March 30, 2020 17:31
@sbidoul
Copy link
Member

sbidoul commented Mar 30, 2020

LGTM

@uranusjr I'm not too sure how this influences #7815 though.

@uranusjr
Copy link
Member Author

@sbidoul It’s only relevent to #7815 (comment) on where we should move the cache check to. Since the new resolver does not rely on populate_link, we can move it to around prepare_link_requirement (probably not inside it because of the yanked warning issue) for meaningful logging.

@pradyunsg pradyunsg merged commit 0f7fac3 into pypa:master Mar 31, 2020
@pradyunsg
Copy link
Member

Thanks @uranusjr! ^.^

@sbidoul sbidoul mentioned this pull request Mar 31, 2020
Merged
15 tasks
@uranusjr uranusjr deleted the yanked-link-refactor branch April 1, 2020 09:16
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 5, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xavfernandez xavfernandez xavfernandez approved these changes

@pradyunsg pradyunsg pradyunsg approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@uranusjr @BrownTruck @sbidoul @pradyunsg @xavfernandez @pypa-bot