Better workaround for cache poisoning (see #3025) #7319

sbidoul · 2019-11-09T11:29:38Z

Make sure pip wheel never outputs pure python wheels with a
python implementation tag. Better fix/workaround for #3025 by
using a per-implementation wheel cache instead of caching pure python
wheels with an implementation tag in their name.

Fixes #7296

chrahunt

This approach seems cleaner than setting the python tag explicitly. In both setups we're caching a wheel per interpreter, but here we preserve all the information that a well-behaved wheel build might want to communicate and it will let us remove some workarounds in our current implementation.

src/pip/_internal/cache.py

pradyunsg · 2019-11-10T08:49:23Z

My big concern here is that this is basically invalidating all existing caches, without clearing them out.

chrahunt · 2019-11-10T09:14:52Z

I would consider that as covered by #6956.

pradyunsg · 2019-11-10T09:32:03Z

This change makes it so that all existing cached wheels would no longer be considered when installing/building a wheelhouse etc. Providing functionality to trim the size of your cached wheels (#6956) is different and would not do anything toward addressing that.

I'd rather that we stop spewing out more the specific tags in our 'pip install' wheels, since we're at a point where it's OK to say something along the lines of:

You're doing dynamic things in your setup.py, which isn't a robust approach --things can and will break. Declare your environment-specific details using static metadata that pip can use when making decisions. If that's not workable and you're having issues with our aggressive caching, please disable it with --no-cache-dir.

Basically (1) from #7296 (comment).

sbidoul · 2019-11-10T10:26:18Z

Requested changes done. Unfortunately sys.implementation is not available on python 2 so I copied a function from packaging.tags.

@pradyunsg a possible issue with the more aggressive approach is that #3025 reveals itself for downstream users of broken sdists. Since such broken sdists are probably unmaintained nowadays, the only actionable option for users will be to disable caching or adapt their, say, tox configurations, to use a different cache per python implementation. So I tend to think some lost disk space and a cache rebuild will generate less complaints. I've no strong opinion on this though.

sbidoul · 2019-11-10T10:44:46Z

I added two commits to remove unused code related to python_tag and implementation_tag.

src/pip/_internal/cache.py

xavfernandez · 2019-11-15T22:57:55Z

This PR would mean that pip 20 would have a completely incompatible cache with previous versions.

I think _get_candidates could call a self.get_legacy_path_for_link(link) (with pip 19 computation) if self.get_path_for_link(link) directory does not exist.
It would mean that pip 20 could still use pip<20 cache directories.

sbidoul · 2019-11-16T11:00:31Z

I rebased and added a commit with the new key construction and hashing algorithm.

sbidoul · 2019-11-17T10:47:20Z

I added support for legacy cache entries, following @xavfernandez's suggestion.

The diff is becoming somewhat big, but individual commits should remain easy to review.

src/pip/_internal/cache.py

xavfernandez · 2019-11-17T21:17:27Z

And I think a .removal newsfile should be added to advertise the fact that pip>=20 cache is not retro-compatible with previous version (but that pip>=20 should still be able to take advantage of previous cache).
We might also want to plan for the removal of this fallback ? 6 months later ? Or maybe a full year ?

xavfernandez

I've a last comment. Otherwise, it looks really good 👍

src/pip/_internal/cache.py

tests/unit/test_cache.py

src/pip/_internal/cache.py

chrahunt

A few minor followup comments (only one actionable right now), otherwise this looks good to me.

src/pip/_internal/cache.py

chrahunt · 2019-11-23T23:26:39Z

src/pip/_internal/cache.py

+        path = self.get_path_for_link(link)
+        if os.path.isdir(path):
+            candidates.extend(os.listdir(path))
+        # TODO remove legacy path lookup in pip>=21


We should make an issue to add a deprecation warning around here when this is merged.

What kind of deprecation warning do you have in mind?

I'm wondering the same as I think pip._internal.utils.deprecation.deprecated would be too noisy for such a thing...
Maybe something like

pip/src/pip/_internal/utils/deprecation.py

Lines 101 to 102 in b3aced9

if gone_in is not None and parse(current_version) >= parse(gone_in):

raise PipDeprecationWarning(message)

to make sure we don't forget about it ?

To not forget about things to do in the future, you could maybe use GitHub milestones and assign such TODO issues to future milestones. Now that pip has a regular release cadence it might be easy enough to manage.

src/pip/_internal/utils/misc.py

pradyunsg

I'll try to take a look at this tomorrow.

news/7296.removal

pypa-bot · 2019-12-02T11:06:36Z

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 eligible for code review and hopefully merging!

Make sure ``pip wheel`` never outputs pure python wheels with a python implementation tag. Better fix/workaround for `pypa#3025 <https://github.com/pypa/pip/issues/3025>`_ by using a per-implementation wheel cache instead of caching pure python wheels with an implementation tag in their name. Fixes pypa#7296

Instead of building an URL-ish string that could be complex to describe and reproduce, generate a dictionary that is hashed with a simple algorithm.

Pip 20 changes the cache key format to include the interpreter name. To avoid invalidating all existing caches, we continue using existing cache entries that were computed with the legacy algorithm. This should not regress issue pypa#3025 because wheel cached in such legacy entries should have the python implementation tag set.

Co-Authored-By: Pradyun Gedam <pradyunsg@gmail.com>

sbidoul · 2019-12-02T11:25:01Z

I rebased to resolve a merge conflict.

sbidoul · 2019-12-05T09:51:13Z

@xavfernandez I think all your comments have been handled. Can you update your review?

xavfernandez · 2019-12-05T09:54:19Z

@sbidoul Sorry, this PR was approved in my head ^^

chrahunt · 2019-12-12T05:27:03Z

Ping @pradyunsg, any issues with this approach?

pradyunsg

Two things, that I think we should consider tackling in follow ups prior to 20.0:

add the deprecation notice, for the legacy path usage -- I'm not 100% sure whether we should be printing a not-actionable notice for something like the cache which pip's generating.
switch to ask-for-forgiveness-not-permission paradigm for file system access here.

Neither of these are blocking concerns though, so green tick it is. :)

pradyunsg

LOL, thanks GitHub.

pradyunsg · 2019-12-12T11:28:29Z

Thanks for all your work here @sbidoul! Thanks for the reviews @chrahunt and @xavfernandez!

Thanks again for the ping @chrahunt. :)

sbidoul · 2019-12-12T11:31:51Z

Thanks all!

chrahunt reviewed Nov 10, 2019

View reviewed changes

src/pip/_internal/cache.py Outdated Show resolved Hide resolved

chrahunt mentioned this pull request Nov 10, 2019

pip binary wheel cache is not interpreter specific #6013

Closed

sbidoul force-pushed the cache-per-impl-sbi branch from 2d71b3a to 8ba6b75 Compare November 10, 2019 10:10

xavfernandez mentioned this pull request Nov 11, 2019

python tag issue with pip wheel / pip install / caching #7296

Closed

sbidoul mentioned this pull request Nov 13, 2019

Include subdirectory URL fragments in cache keys #7335

Merged

chrahunt reviewed Nov 14, 2019

View reviewed changes

src/pip/_internal/cache.py Outdated Show resolved Hide resolved

sbidoul force-pushed the cache-per-impl-sbi branch from b3444d7 to 468e282 Compare November 16, 2019 10:59

xavfernandez requested changes Nov 17, 2019

View reviewed changes

src/pip/_internal/cache.py Outdated Show resolved Hide resolved

sbidoul force-pushed the cache-per-impl-sbi branch 2 times, most recently from 600778a to 69cdf75 Compare November 18, 2019 14:12

xavfernandez reviewed Nov 18, 2019

View reviewed changes

src/pip/_internal/cache.py Outdated Show resolved Hide resolved

tests/unit/test_cache.py Show resolved Hide resolved

chrahunt reviewed Nov 18, 2019

View reviewed changes

src/pip/_internal/cache.py Outdated Show resolved Hide resolved

sbidoul force-pushed the cache-per-impl-sbi branch from 2aab2f2 to f3e5d51 Compare November 18, 2019 18:45

chrahunt reviewed Nov 23, 2019

View reviewed changes

chrahunt approved these changes Nov 30, 2019

View reviewed changes

pradyunsg reviewed Nov 30, 2019

View reviewed changes

news/7296.removal Outdated Show resolved Hide resolved

pypa-bot added the needs rebase or merge PR has conflicts with current master label Dec 2, 2019

sbidoul added 3 commits December 2, 2019 12:07

Remove unused pep425tags.implementation_tag

b14b375

Remove unused wheel_builder python_tag argument

e0165e7

sbidoul and others added 6 commits December 2, 2019 12:07

New cache key generation algorithm

c4ef616

Instead of building an URL-ish string that could be complex to describe and reproduce, generate a dictionary that is hashed with a simple algorithm.

Advertise the new cache structure in a news file

bfb7db2

Better support for unicode cache entries

824dca1

Use interpreter_name and _version in cache keys

ab05936

Update news/7296.removal

e3c1ca1

Co-Authored-By: Pradyun Gedam <pradyunsg@gmail.com>

sbidoul force-pushed the cache-per-impl-sbi branch from d457274 to e3c1ca1 Compare December 2, 2019 11:12

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Dec 2, 2019

xavfernandez approved these changes Dec 5, 2019

View reviewed changes

pradyunsg reviewed Dec 12, 2019

View reviewed changes

pradyunsg approved these changes Dec 12, 2019

View reviewed changes

pradyunsg changed the title ~~Better workaround for cache poisoning #3025~~ Better workaround for cache poisoning (#3025, #7319) Dec 12, 2019

pradyunsg merged commit ce5edd4 into pypa:master Dec 12, 2019

pradyunsg changed the title ~~Better workaround for cache poisoning (#3025, #7319)~~ Better workaround for cache poisoning (see #3025) Dec 12, 2019

sbidoul deleted the cache-per-impl-sbi branch December 12, 2019 11:30

sbidoul mentioned this pull request Dec 15, 2019

Fix cache bug with legacy cache entries #7490

Merged

pradyunsg added the type: enhancement Improvements to functionality label Dec 21, 2019

lock bot added the auto-locked Outdated issues that have been locked by automation label Jan 11, 2020

lock bot locked as resolved and limited conversation to collaborators Jan 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better workaround for cache poisoning (see #3025) #7319

Better workaround for cache poisoning (see #3025) #7319

sbidoul commented Nov 9, 2019

chrahunt left a comment

pradyunsg commented Nov 10, 2019 •

edited

Loading

chrahunt commented Nov 10, 2019

pradyunsg commented Nov 10, 2019

sbidoul commented Nov 10, 2019

sbidoul commented Nov 10, 2019

xavfernandez commented Nov 15, 2019

sbidoul commented Nov 16, 2019

sbidoul commented Nov 17, 2019

xavfernandez commented Nov 17, 2019

xavfernandez left a comment

chrahunt left a comment

chrahunt Nov 23, 2019

sbidoul Nov 24, 2019

xavfernandez Nov 24, 2019

sbidoul Dec 12, 2019

pradyunsg left a comment

pypa-bot commented Dec 2, 2019

sbidoul commented Dec 2, 2019

sbidoul commented Dec 5, 2019

xavfernandez commented Dec 5, 2019

chrahunt commented Dec 12, 2019

pradyunsg left a comment

pradyunsg left a comment

pradyunsg commented Dec 12, 2019

sbidoul commented Dec 12, 2019

	if gone_in is not None and parse(current_version) >= parse(gone_in):
	raise PipDeprecationWarning(message)

Better workaround for cache poisoning (see #3025) #7319

Better workaround for cache poisoning (see #3025) #7319

Conversation

sbidoul commented Nov 9, 2019

chrahunt left a comment

Choose a reason for hiding this comment

pradyunsg commented Nov 10, 2019 • edited Loading

chrahunt commented Nov 10, 2019

pradyunsg commented Nov 10, 2019

sbidoul commented Nov 10, 2019

sbidoul commented Nov 10, 2019

xavfernandez commented Nov 15, 2019

sbidoul commented Nov 16, 2019

sbidoul commented Nov 17, 2019

xavfernandez commented Nov 17, 2019

xavfernandez left a comment

Choose a reason for hiding this comment

chrahunt left a comment

Choose a reason for hiding this comment

chrahunt Nov 23, 2019

Choose a reason for hiding this comment

sbidoul Nov 24, 2019

Choose a reason for hiding this comment

xavfernandez Nov 24, 2019

Choose a reason for hiding this comment

sbidoul Dec 12, 2019

Choose a reason for hiding this comment

pradyunsg left a comment

Choose a reason for hiding this comment

pypa-bot commented Dec 2, 2019

sbidoul commented Dec 2, 2019

sbidoul commented Dec 5, 2019

xavfernandez commented Dec 5, 2019

chrahunt commented Dec 12, 2019

pradyunsg left a comment

Choose a reason for hiding this comment

pradyunsg left a comment

Choose a reason for hiding this comment

pradyunsg commented Dec 12, 2019

sbidoul commented Dec 12, 2019

pradyunsg commented Nov 10, 2019 •

edited

Loading