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

cache poisoning via automated wheels #3025

Closed
mmerickel opened this issue Aug 12, 2015 · 29 comments
Closed

cache poisoning via automated wheels #3025

mmerickel opened this issue Aug 12, 2015 · 29 comments

Comments

@mmerickel
Copy link

@mmerickel mmerickel commented Aug 12, 2015

A lot of projects distributed as an sdist have custom runtime code in setup.py that depends on the exact environment being run. It appears as if when the wheel is built and put into the wheel cache this is not done "per runtime" (at least by python version). This is especially evident when running tests via tox which uses multiple python versions and pip.

For example, installing mako on py33 and then on py32 will install markupsafe into py32 (which isn't supported). Mako's setup.py properly avoids markupsafe on 3.2 but since the wheel was originally built on 3.3 the wheel now depends on markupsafe.

The issue here is that mako is not distributed as a wheel and thus does not have a setup.cfg or anything broadcasting the proper metadata and yet pip assumes it can just build the wheel once and use it everywhere. This should be considered a cache poisoning bug in the wheel cache.

@dstufft

This comment has been minimized.

Copy link
Member

@dstufft dstufft commented Aug 12, 2015

/cc @rbtcollins

Off hand, I don't think it's unreasonable to reduce our cache hit rate a bit in order to work in more situations. The crushing weight of legacy is a pain. This is only really a factor for pure python wheels, since wheels that have compiled code will be version specific anyways, which are going to be fairly quick to build anyways.

@rbtcollins

This comment has been minimized.

Copy link
Contributor

@rbtcollins rbtcollins commented Aug 12, 2015

One case doesn't make a trend.

Mako's sdist tarball has a setup.cfg in it containing:

[wheel]
universal = 1

I think we have to trust that folk configuring universal wheels intend to do so.

if this is a general systemic problem then sure, we could pass an option to disable the building of universal wheels for the wheels we put in the cache. But I'm not convinced it is so far.

@rbtcollins

This comment has been minimized.

Copy link
Contributor

@rbtcollins rbtcollins commented Aug 12, 2015

Oh, I should mention that while I don't think its super common, its actually pathological and something we can't defend against: https://bitbucket.org/runeh/anyjson/src/0026a68c035696bdc8db8628e364139eba9a8ba8/setup.py?at=default#setup.py-57

I think we should push the 'get it right by default' logic, whatever that is, down to wheel, since pips goal is to consume binaries where possible, and folk building bad wheels and uploading them to pypi is as much an issue as wheels in the cache. (I've done that, for instance).

@rbtcollins

This comment has been minimized.

Copy link
Contributor

@rbtcollins rbtcollins commented Aug 12, 2015

So perhaps looking at it this way.

Some % of packages apply logic to determine dependencies at build time and export that as unconditional install time requirements. Sometimes that logic is coupled to Python version, sometimes not.

Most of those packages cannot be built into wheels today:

  • Some % of them will claim, as Mako does, that they can build universal wheels. This is bogus :).
  • Some % will not make that claim and will only consult Python major version, thus being compatible with wheels default behaviour.
  • The rest won't make that claim but will consult e.g. the Python minor version, or the set of importable packages, or pkg_resources or $anything. These are completely incompatible with being built into wheels.

There's no oracle I can think of to determine which category a given package falls into (wheel metadata is valid, wheel metadata is invalid but explicitly chosen that way by the package, wheel metadata is invalid due to accident).

Our logic to inject setuptools to get the bdist_wheel command is pragmatically fine: a distutils package doesn't have machine expressed dependencies anyway.

So this builds down to the following questions IMO:

A: How do we handle packages that build wheels dependent on more than what their wheel by default expresses, but which can be expressed by wheel without changing egg-info metadata.

B: How do we handle packages that build wheels dependent on more than Python major.minor version [and don't express it via egg-info metadata].

@dstufft on IRC suggested passing a more restrictive wheel tag (e.g. cp26 on CPython 2.6).

That would fix A) as I understand it, but not B).

I'm personally more worried by B than A, since A can be easily fixed by the package author, but B cannot. For instance, anyjson requires ecosystem changes - an alternatives mechanism for dependencies.

@dstufft

This comment has been minimized.

Copy link
Member

@dstufft dstufft commented Aug 12, 2015

Right, but we can't really solve B currently, we can't fix things for anyjson right now until we make more ecosystem changes. I don't think it's going to be possible for us to really solve this for every single scenario, but I think that using a more restrictive wheel tag will at least reduce the cases that we have problems in. I should note that another thing people do is different packages for different interpreters and different packages for different operating systems.

For different interpreters, the more restrictive tag will also fix these cases since the more restrictive tag includes the interpreter baked into it as well. We won't solve the case where people use different dependencies on different OSs, but I don't think it's reasonable to assume that the cache can be shared between OSs.

@rbtcollins

This comment has been minimized.

Copy link
Contributor

@rbtcollins rbtcollins commented Aug 12, 2015

Another option would be to not solve attempt to solve this systematically and instead solve it case by case.

The --no-binary option exists to let folk opt out of wheels on a per package basis - its the escape clause.

Folk should use --no-binary anyjson today, but they don't know that they should. I'm using anyjson as a specific example since its not fixable any other way [today]. Publishing a known-problematic list of packages would let this be socialised. As I suggested on IRC we could even publish that on a REST API for programmatic consumption.

I would like to provide some signal to package authors like Mako - where things can be done better - that they should move to conditional dependencies rather than computed dependencies.

@mmerickel

This comment has been minimized.

Copy link
Author

@mmerickel mmerickel commented Aug 12, 2015

Would this be an issue if mako did not have the wheel directive in setup.cfg ? Presumably then the cache would contain platform tags instead of being cached as universal? If that's the case then this is arguably a misconfiguration in mako to support universal wheels without the proper metadata section in setup.cfg.

@dstufft

This comment has been minimized.

Copy link
Member

@dstufft dstufft commented Aug 12, 2015

It would still be a problem, it would just have a py2 and a py3 wheel instead of a py2.py3 wheel.

@rbtcollins

This comment has been minimized.

@rbtcollins

This comment has been minimized.

Copy link
Contributor

@rbtcollins rbtcollins commented Aug 12, 2015

It is a bug in Mako. But its a bug we expect lots of packages to have because legacy. bdists had the same issues for years but few folk used them. pip's wheel cache is massively increasing the surface areas of packagers' packages interacting with wheels.

@mmerickel

This comment has been minimized.

Copy link
Author

@mmerickel mmerickel commented Aug 12, 2015

There should be a way (such as [wheel] universal=1) that a package broadcasts support for being built with a wheel... if it doesn't then maybe still try to build it but with the most specific platform tags possible, or don't cache it at all.

@mmerickel

This comment has been minimized.

Copy link
Author

@mmerickel mmerickel commented Sep 23, 2015

Is there any progress on this? This is a serious regression as far as any use cases regarding tox so I'm surprised it isn't being fixed sooner. I basically can't use tox without disabling the download cache anymore.

@rbtcollins

This comment has been minimized.

Copy link
Contributor

@rbtcollins rbtcollins commented Sep 23, 2015

There's a proposed fix for mako. The restrictions @dstufft proposed here haven't been coded up yet - and won't fix mako anyhow (because py2/py3 is not specific enough)

@mgedmin

This comment has been minimized.

Copy link

@mgedmin mgedmin commented Oct 15, 2015

pip's wheel caching pushed me to submit fixes for at least two packages to use environment markers instead of dynamically computing install_requires in setup.py.

If I had to submit a patch for a 3rd package, I would have to research the marker syntax from scratch again, because it's underdocumented.

I guess my point is: the best way to fix this is to improve documentation.

@mmerickel

This comment has been minimized.

Copy link
Author

@mmerickel mmerickel commented Nov 3, 2015

Why is this listed as an enhancement and not a bug? I guess I'm confused on pypa's point of view here.

@mmerickel

This comment has been minimized.

Copy link
Author

@mmerickel mmerickel commented Nov 3, 2015

Webtest yesterday was asked to release a wheel as the developers did so... but of course they did it incorrectly because the setup.py has 2.6-specific dependencies. They ended up just removing the wheel entirely. The frustrating part is that this doesn't fix the problem of course because this cache poisoning bug still creates a broken wheel for me (a py2 wheel that is broken on py26) because it was built on py27 and cached for all of py2.

@rbtcollins

This comment has been minimized.

Copy link
Contributor

@rbtcollins rbtcollins commented Nov 3, 2015

@mmerickel well just removing the wheel doesn't fix it for everyone either because its still in HTTP caches even without autobuilt wheels. They need to either a) block building wheels or b) automatically tag the wheel as being python minor version specific ( I don't think you can, but that would be needed) or c) use the dist_requires entry in setup.cfg to make the wheel deps conditional even while setup.py is generating dynamic deps.

@mmerickel

This comment has been minimized.

Copy link
Author

@mmerickel mmerickel commented Nov 3, 2015

@rbtcollins yes I understand the issue at hand which is why I'm so confused by the response I've seen in the thread. pip is used by the vast majority of the python community, and with mostly packages released way before pip 7 was released.. How can this not be considered a regression versus an enhancement? Pip broke the world as far as I can tell as the only suggestions you've made have been a) every user ever stops building wheels, b) not supported or c) every package author ever updates their setup.py. You can fallback on semantic versioning and telling people to pin their version of pip but really... who does that with pip?

@dstufft

This comment has been minimized.

Copy link
Member

@dstufft dstufft commented Nov 3, 2015

I think it is a bug not an enhancement.

Also the HTTP cache doesn't matter past 10 minutes or so since even though the file is cached for basically forever the PEP 503 simple page is not and when it's not listed there anymore pip won't ever use it.

Sent from my iPhone

On Nov 3, 2015, at 2:56 PM, Michael Merickel notifications@github.com wrote:

@rbtcollins yes I understand the issue at hand which is why I'm so confused by the response I've seen in the thread. pip is used by the vast majority of the python community, and with mostly packages released way before pip 7 was released.. How can this not be considered a regression versus an enhancement? Pip broke the world as far as I can tell as the only suggestions you've made have been a) every user ever stops building wheels, b) not supported or c) every package author ever updates their setup.py. You can fallback on semantic versioning and telling people to pin their version of pip but really... who does that with pip?


Reply to this email directly or view it on GitHub.

@bertjwregeer

This comment has been minimized.

Copy link

@bertjwregeer bertjwregeer commented Nov 3, 2015

Currently I find myself deleting the pip cache on every single tox (each environment being run separately by hand due to this issue) run because of this broken behaviour. As a work-around, is there a way to set the cache location for every single run so that I can in my tox.ini set this up as an environment variable that is changed depending on the Python version pip is being run with?

@dstufft

This comment has been minimized.

Copy link
Member

@dstufft dstufft commented Nov 3, 2015

--cache-dir on the CLI, PIP_CACHE_DIR in the environment, or configuration files.

@mmerickel

This comment has been minimized.

Copy link
Author

@mmerickel mmerickel commented Nov 4, 2015

Alternative solution - Why doesn't pip use a different cache dir per environment by default versus messing with the actual wheels? Or at least per python version? This just seems obvious to me as a way to keep pip working while the community has time to come up with a better way to declare dependencies? There are so many packages out there with runtime-defined dependencies in their setup.py that this just seems like it has to be done... It's entirely possible that certain packages can never be fully described declaratively and then pip still needs a way to properly install these packages. Client-side opt-outs are by far the worst way to go here from UX, reliability and reproducibility perspectives.

@rbtcollins

This comment has been minimized.

Copy link
Contributor

@rbtcollins rbtcollins commented Nov 4, 2015

@mmerickel it is a regression; there's no doubt about that. I was offering a workaround given the status quo, not justifying that it should be the way it is. I also agree that there may be packages which can never describe their dependencies fully using markers - but those packages should also not ever build wheels: wheels don't have room for dynamic dependencies.

As far as using per-version caches: the packages where the cache matters the most - numpy and it's consuming packages - are ABI compatible with multiple versions of Python3 (it was one of the major efforts done in Python3, a stable ABI) - and personally I'd be loathe to give up that optimisation.

@mmerickel

This comment has been minimized.

Copy link
Author

@mmerickel mmerickel commented Nov 4, 2015

but those packages should also not ever build wheels: wheels don't have room for dynamic dependencies.

Pip automatically builds wheels and caches them, right? This isn't like the packages opted-into this behavior. That is my concern here.

I'm very familiar with the benefits of wheels for numpy but I also don't see a problem with building it once per python I use on my system (which for most people will not be many pythons) to ensure that things always work for other packages like they did in pip < 7 (at least so much as I noticed them working).

@rbtcollins

This comment has been minimized.

Copy link
Contributor

@rbtcollins rbtcollins commented Nov 4, 2015

Yes, the bdist_wheel command Just Works(tm) - except when it doesn't (like here), merely by virtue of the wheel package being installed. Perhaps that would be a better way forward: have wheel require opt-in by packages. I've seen many many issues with 'pip wheel' building poor wheels as well in the past.

@xavfernandez

This comment has been minimized.

Copy link
Contributor

@xavfernandez xavfernandez commented Nov 4, 2015

Well, if I'm not mistaken, bdist_wheel already tags wheel by the python version interpreter by default, i.e. if you build a wheel with python2.7 it will build a wheel tagged 2.7.

If the wheel is tagged otherwise, it means the package's setup.cfg stated otherwise and the bug would be on the package side...

Of course, there is still the issue of binary wheels built with an incompatible ABI but it is not specific to the wheel cache. This part could be temporarily addressed by a pip cache command to easily cleanup the wheel cache (cf #3146).

@mgedmin

This comment has been minimized.

Copy link

@mgedmin mgedmin commented Nov 4, 2015

Well, if I'm not mistaken, bdist_wheel already tags wheel by the python version interpreter by default, i.e. if you build a wheel with python2.7 it will build a wheel tagged 2.7.

Only if your wheel has C extension modules. Pure-python wheels get tagged py2.

@dstufft dstufft added the type: bug label Nov 4, 2015
@dstufft

This comment has been minimized.

Copy link
Member

@dstufft dstufft commented Nov 4, 2015

See #3225

@pfmoore

This comment has been minimized.

Copy link
Member

@pfmoore pfmoore commented Nov 4, 2015

Only if your wheel has C extension modules. Pure-python wheels get tagged py2.

Indeed, this was to counteract the fact that projects typically didn't specify the Python tag correctly, so "compatible with the current major version" is a better guess than only with the minor version. But that's a sensible default for interactive use only, and @dstufft's approach of explicitly setting the tag for autobuilt wheels is a good one.

@dstufft dstufft closed this in #3225 Nov 4, 2015
mgedmin added a commit to mgedmin/certbot that referenced this issue Dec 3, 2015
(I think this is a bad idea because of
pypa/pip#3025, but letsencrypt maintainers
insist, so *shrug*.  Also the same problem exists for the versioned
'mock' dependency, so I'm not introducing a new one here.)
@lock lock bot added the S: auto-locked label Jun 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.