-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Comments
/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. |
One case doesn't make a trend. Mako's sdist tarball has a setup.cfg in it containing:
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. |
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). |
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:
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. |
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. |
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. |
Would this be an issue if mako did not have the |
It would still be a problem, it would just have a |
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. |
There should be a way (such as |
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. |
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) |
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. |
Why is this listed as an enhancement and not a bug? I guess I'm confused on pypa's point of view here. |
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. |
@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. |
@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? |
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
|
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? |
|
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. |
@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. |
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). |
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. |
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 |
Only if your wheel has C extension modules. Pure-python wheels get tagged |
See #3225 |
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. |
(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.)
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 viatox
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.The text was updated successfully, but these errors were encountered: