Closed lmazuel closed 7 years ago
Yeah, we can add that as a release note. That said, pip 9 is a bit newer than I thought we were going to need: @dstufft, is the platform_system
marker not provided before then?
I just bisect, and it works starting 8.1.2 (broken in 8.1.1).
Ok, so that means it works with a version of pip that's a year old. I think I'm ok with this, though naturally the rest of the dev team should express opinions here. Generally, I'm a bit loathe to be tied down by older pips: generally speaking for most people it's easy to upgrade pip, and if it's hard to upgrade pip they're usually using a distro and so should use a virtualenv to install Requests, at which point they should just install a new pip inside the virtualenv.
Any thoughts on that team? @nateprewitt @sigmavirus24 @kennethreitz?
(For those who want the data point, pip 8.1.2 was released one year ago on Thursday).
I just found this issue because the default pip on Travis CI is 6.x (easily fixed with the addition of pip install --upgrade pip
but that'll be in every project until they update the base image)
Hrm. That's a bit hard for Travis CI to justify: the most recent 6.x release of pip is two years old, which is really a bit much!
@Lukasa I'm opening an issue now
I'm pretty sure the justifcation is they're Rubyists and don't think much about pip :)
@dstufft Do you have an opinion on this, btw?
I think everyone should always upgrade to the latest pip! Whether enough people have done that for requests to consider it reasonable to depend on it or not, I dunno.
Ok, so I decided to get some data. I ran this query over the pip BigQuery data to see what pip version was used to install Requests over the past 30 days. Here are our answers, down to 5k downloads (I don't really care about any number lower than that):
Row | details_installer_name | details_installer_version | total_downloads |
---|---|---|---|
1 | pip | 9.0.1 | 5020104 |
2 | pip | 1.5.4 | 1271017 |
3 | pip | 8.1.1 | 860806 |
4 | pip | 8.1.2 | 605871 |
5 | pip | 8.0.3 | 500970 |
6 | pip | 7.1.2 | 471446 |
7 | pip | 6.1.1 | 370775 |
8 | pip | 1.5.6 | 317236 |
9 | pip | 6.0.8 | 153005 |
10 | pip | 6.0.7 | 149881 |
11 | pip | 7.1.0 | 78024 |
12 | pip | 9.0.0 | 41436 |
13 | pip | 1.4.1 | 38936 |
14 | pip | 8.0.2 | 34807 |
15 | pip | 1.5.5 | 30253 |
16 | pip | 1.5 | 28426 |
17 | pip | 7.0.1 | 9441 |
18 | pip | 7.0.3 | 8717 |
19 | pip | 8.1.0 | 7126 |
20 | pip | 1.5.1 | 4979 |
So the versions that support these markers are 8.1.2, 9.0.0, and 9.0.1. Together they account for 5,667,411 of our 10,015,055 pip downloads in that time frame. Put another way, 56.6% of our downloads last month came from pip installs that are compatible with the markers.
For some extra data points, some of the other large numbers are people using the ancient pips their distros provided. For example, pip 1.5.4 is almost entirely going to be Ubuntu Trusty, with their 1,271,017 downloads. I'm inclined not to worry about them too much: if they're installing Requests with the distro-provided pip all they're going do is break their pip. Same note with 1.5.6 (Debian Jessie), which together provide another 17% or so of downloads that are basically beyond us.
This also broke our CI a minute ago: so is Travis gonna fix their env or are projects supposed to do an update within their Travis scripts?
Ok, so right now with 56% of our downloaders already safe and most of the rest one pip install -U pip
away from being ok, I'm pretty tempted to say that we should consider toughing this out. It's going to annoy a whole bunch of people, but if we can drag the ecosystem forward a bit to the point that people can actually meaningfully use environment markers for conditional dependencies that would be one hell of a public service.
@oberstet If it were me, I'd add a pip install -U pip
to the start of my travis scripts. Travis has been known to be slow with their Python environments at the best of time, and that way you guarantee you'll never encounter a problem with a dependency that does this again in the future. Consider it a forward looking defense against this kind of pain happening again.
As a fellow maintainer of OSS projects who wants to use environment markers, <3. It seems like we should also engage with the travis folks to fix this, as that'll reduce a lot of the pain.
@Lukasa, 8.1.1 seems to likely be coming from Ubuntu Xenial too. It seems reasonable for people to be upgrading a version of pip that in some cases is more than 3 years old. I'm on board with waiting this one out as well with the current information. We can't support legacy versions of pip forever.
@Lukasa I'm fine with that. Travis should fix that, instead of myriads of projects, but what the hell: your advice is pragmatic! =)
@oberstet Very much agreed, I'd prefer a world where Travis has some kind of rolling approach to pip versions, and I'm happy to work with them to make that possible if they need outside help. In the meantime, though, we shall do what we can in their absence! :grin:
the rest are one
pip install -U pip
away from being ok
Just to point out, they are also one pip install requests==2.13.0
away from being ok. Seriously, this is going to impact a lot of people who use Travis. Before today I had never even heard of requests, but all of a sudden every unit test for our project is failing because codecov depends on requests. I agree that Travis should install a newer version of pip, but that doesn't mean this is the right way to do it. I would prefer rolling back whatever change requires environment markers until Travis updates pip.
@adamjstewart Yeah, that's absolutely true, and some number of projects will probably do that. All we can do is make a decision about at what point we decide to stop supporting the older releases of pip
: at what level of usage do we give up on them?
I should note that Travis-CI is included in the above numbers, which means even with Travis-CI less than half of our downloads are currently affected. I don't consider that number to be at a level where I want to immediately revert this.
I'm still keeping this open for discussion: I want the community and the other maintainers to weigh in. @nateprewitt and I have had our say, two maintainers have yet to get involved, and there are many more community folk who should feel free to drop in and tell us what they think. All I am saying is that right now I am open to leaving this as-is.
Let's see how many people's computers we can break.
Seeing this issue made me smile. :) I'm :+1: on keeping it how it is, on with progress!
Just to point out, they are also one pip install requests==2.13.0 away from being ok.
To be fair, we ran into this issue when installing a package which depends on an unbounded requests
version but I agree that a popular library like this forcing a pip upgrade
is probably a good thing but will mess up a lot of people's day (like mine!).
This is a common feature of shipping Requests releases: when you have a userbase as large as ours it's basically impossible to avoid breaking someone. This was a little bit bigger than I expected, I admit, but still. :grin:
@blamarvt Yeah, so I should also make something clear: we are sorry about this. We didn't do it on purpose: when we committed the change we thought it had much broader support than it actually did. However, now that we're here we should evaluate whether this was a blessing in disguise.
when you have a userbase as large as ours it's basically impossible to avoid breaking someone
Is "less than half of our users are affected" the bar? When you have a lot of users, half your users is still a lot of users.
@tgamblin I don't think there is a singular bar, but rather a combination of how bad the breakage is, when the breakage occurs, how many people are going to be broken, how bad the remedy is, and whether the new thing is an improvement or just different.
@tgamblin Is it really anything beyond upgrading pip? This had to happen eventually, I think expecting users to run pip install -U pip
at least once a year is a really low bar.
@tgamblin Right now we don't have a defined bar. If we did, this would be a much shorter conversation: either it would meet it, or it would not. Part of the reason we're having this discussion is to try to kick around the idea of how much pain we're willing to inflict for (IMO) positive ecosystem gain. As I've said before, we are not committed to keeping this.
The flip side to this is that if we can bring even 20% of our users forward to a much newer pip, that opens up an enormous chunk of the ecosystem to being able to use these newer Python packaging tools. Python packaging is such a commonly complained about topic, and one reason is that an enormous amount of the newer nicer features cannot actually be used because we have to support older pips. Putting on some ecosystem pressure to upgrade might be a good thing. But it might not. Hence: this thread.
@Lukasa Completely understand. I haven't dug deep into the change but if the pip version could be detected and a better error message emitted it would go a long way to ameliorating your users.
Pain is a part of software engineering, if it was easy no one would pay us. I think this accidental "breakage" is actually a benefit to the ecosystem as a whole.
@blamarvt Yeah, that'd be really nice. Sadly, we've been publishing wheels for a long time, and wheels are preferred by pip and don't run any code, so anyone who just types pip install requests
and can use wheels won't get that handy error message. 😞
I also needed to make sure setuptools was upgraded in order to use sphinx to compile documentation. Got this error when running sphinx-build
(RHEL7):
# Sphinx version: 1.5.5
# Python version: 2.7.5 (CPython)
# Docutils version: 0.13.1 release
# Jinja2 version: 2.9.6
# Last messages:
# Loaded extensions:
Traceback (most recent call last):
File "/root/t/lib/python2.7/site-packages/sphinx/cmdline.py", line 295, in main
opts.warningiserror, opts.tags, opts.verbosity, opts.jobs)
File "/root/t/lib/python2.7/site-packages/sphinx/application.py", line 189, in __init__
self.setup_extension(extension)
File "/root/t/lib/python2.7/site-packages/sphinx/application.py", line 512, in setup_extension
mod = __import__(extension, None, None, ['setup'])
File "/root/t/lib/python2.7/site-packages/sphinx/builders/linkcheck.py", line 34, in <module>
from sphinx.util import encode_uri, requests
File "/root/t/lib/python2.7/site-packages/sphinx/util/requests.py", line 46, in <module>
pkg_resources.require(['requests[security]'])
File "/root/t/lib/python2.7/site-packages/pkg_resources.py", line 728, in require
needed = self.resolve(parse_requirements(requirements))
File "/root/t/lib/python2.7/site-packages/pkg_resources.py", line 631, in resolve
requirements.extend(dist.requires(req.extras)[::-1])
File "/root/t/lib/python2.7/site-packages/pkg_resources.py", line 2489, in requires
dm = self._dep_map
File "/root/t/lib/python2.7/site-packages/pkg_resources.py", line 2700, in _dep_map
self.__dep_map = self._compute_dependencies()
File "/root/t/lib/python2.7/site-packages/pkg_resources.py", line 2733, in _compute_dependencies
common = frozenset(reqs_for_extra(None))
File "/root/t/lib/python2.7/site-packages/pkg_resources.py", line 2730, in reqs_for_extra
if req.marker_fn(override={'extra':extra}):
File "/root/t/lib/python2.7/site-packages/_markerlib/markers.py", line 113, in marker_fn
return eval(compiled_marker, environment)
File "<environment marker>", line 1, in <module>
NameError: name 'platform_system' is not defined
That would be because Sphinx is using pkg_resources.require()
.
MORE DATA!
If we restrict ourselves to the two most recent Requests releases, to try to exclude people who aren't actually tracking requests releases, the data looks like this:
Row | details_installer_name | details_installer_version | total_downloads |
---|---|---|---|
1 | pip | 9.0.1 | 2615739 |
2 | pip | 1.5.4 | 955686 |
3 | pip | 8.1.2 | 415272 |
4 | pip | 8.1.1 | 258218 |
5 | pip | 6.1.1 | 184210 |
6 | pip | 1.5.6 | 170216 |
7 | pip | 6.0.7 | 130783 |
8 | pip | 7.1.2 | 100125 |
9 | pip | 6.0.8 | 66280 |
10 | pip | 7.1.0 | 56674 |
11 | pip | 8.0.2 | 30829 |
12 | pip | 1.5.5 | 30045 |
13 | pip | 1.4.1 | 15066 |
14 | pip | 1.5 | 14546 |
15 | pip | 9.0.0 | 9349 |
16 | pip | 7.0.1 | 9015 |
17 | pip | 1.5.1 | 4947 |
In this variation the versions of pip that support the marker account for 3,040,360 downloads of a total of 5,076,901, or a total of 59.8%. So some portion of our older downloads are downloading older Requests versions, and they will continue to be unaffected.
If you want a CI that doesn't fail on arbitrary upstream changes, pin your dependencies. pip, setuptools etc. are build-time dependencies.
So instead of pip install -U pip
, consider pip install pip==X.Y.Z
. It's your choice which of the two you prefer. Maybe you want to have CI jobs for both.
IMHO we should spread awareness of this rather than supporting super-old stuff forever.
Stable environments -- say, RHEL6/CentOS6 is where a ton of software gets deployed. If you are going to break them, it better be for a good reason, no?
Do you need a newer pip hard enough to push work on thousands of other people? Or can this be solved with a quick one liner?
(This is not an abstract question, I've done releng for many FOSS projects, over many years, and I releng for a project where we just got hit by this. You can google me up if you want to.)
@martin-langhoff Is it a stable environment if you're pip installing the latest Requests? RHEL ships with a copy of Requests in their repositories that they use for system tools, and you really should not be shadowing it with one from pip. On the other hand, if you're using a virtualenv (as you should be in this case), you can just pip install -U pip
in the virtualenv to resolve the issue.
As to whether we need it, it depends on whose needs we consider. The reality is that right now, if you install the socks dependencies on Windows for a version of Requests before 2.14 you get a broken install, because we need the win_inet_pton
package. Naturally, this package is not needed on any other system, and may indeed misbehave quite badly on those systems.
Working around this requires forcing the execution of setup.py
, which means no longer distributing wheels, which means forcing client code to build them and cache them locally. Not so good. There's a balance here: either we leave Windows users broken, or we rely on Windows users having no wheel support (not likely, Windows users often have up-to-date pips), or we break people running old tools. There is no trivial easy answer here, or we'd have taken it.
I might be mistaken, but we are doing the following in @mitmproxy for a while now without any complaints:
extras_require={
':sys_platform == "win32"': [
"pydivert>=2.0.3, <2.1",
],
I think this also works for older pip versions, but I unfortunately cannot check that on mobile right now. This might be acceptable for requests as well?
@mhils If we can get an idea of what pips it'll work for we can make a data-driven decision about how helpful it is. If it brings support up 10% or more I'm probably sold on that being a good approach.
FYI: This broke a @TravisCI build for me this morning. I think their default pip is pretty old. So you can look forward to similar reports from others. I'm adding pip install pip --upgrade
to my build routine as recommended.
@Lukasa This might help: https://gsnedders.github.io/python-marker-test/results.html
That's a good point @mhils , I didn't realize this wasn't the syntax used in requests
2.14.0. I use this in my own package (msrest):
extras_require={
":python_version<'3.4'": ['enum34>=1.0.4'],
}
and I think it's fair to say it's comparable.
I was able to install my package using pip 6.0.0 (I can't go before, I have my own limitation :))
Thanks for the explanation of the difficulties. It was a missing factor in the discussion. The breakage affecting my project is resolved, but I'm hopeful that a fix is forthcoming so as to save others the pain.
Note that lots of folks don't depend on requests
; we don't! But we pip install
a package that through its depchain does. Our needs re stability tradeoffs might not align with the needs of every maintainer in the depchain.
There's a balance here: either we leave Windows users broken, or we rely on Windows users having no wheel support (not likely, Windows users often have up-to-date pips), or we break people running old tools. There is no trivial easy answer here, or we'd have taken it.
This is a far better justification than "only 46% of our users are affected" or "I think this accidental "breakage" is actually a benefit to the ecosystem as a whole."
I will say I suspect this would be a shorter thread if the error message was better. The fix for this is easy (update pip!). But the message you get when you hit the problem doesn't even remotely imply that's what you should do. Per pypa/pip#4469:
Collecting requests>=2.7.9 (from codecov)
Downloading requests-2.14.0-py2.py3-none-any.whl (559kB)
100% |################################| 561kB 801kB/s
Exception:
Traceback (most recent call last):
File "/home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages/pip/basecommand.py", line 232, in main
status = self.run(options, args)
File "/home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages/pip/commands/install.py", line 339, in run
requirement_set.prepare_files(finder)
File "/home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages/pip/req/req_set.py", line 436, in prepare_files
req_to_install.extras):
File "/home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages/pip/_vendor/pkg_resources/__init__.py", line 2496, in requires
dm = self._dep_map
File "/home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages/pip/_vendor/pkg_resources/__init__.py", line 2697, in _dep_map
self.__dep_map = self._compute_dependencies()
File "/home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages/pip/_vendor/pkg_resources/__init__.py", line 2730, in _compute_dependencies
common = frozenset(reqs_for_extra(None))
File "/home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages/pip/_vendor/pkg_resources/__init__.py", line 2727, in reqs_for_extra
if req.marker_fn(override={'extra':extra}):
File "/home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages/pip/_vendor/_markerlib/markers.py", line 113, in marker_fn
return eval(compiled_marker, environment)
File "<environment marker>", line 1, in <module>
NameError: name 'platform_system' is not defined
It would be awesome if that just said "your pip is too old". I'm not intimately familiar with the code here, so I'm not sure whether requests
could print such a message or whether pip
would have to do it (in which case you can't really backport it to an old pip
).
If you want a CI that doesn't fail on arbitrary upstream changes, pin your dependencies. pip, setuptools etc. are build-time dependencies.
As a data point: we pin most of our dependencies to exact version numbers, but we specifically don't pin requests
because we
a) know that you follow semantic versioning, and
b) we trust the requests developers to be responsible and not introduce breaking changes without a major version bump.
I'm fine with upgrading pip
, but this did break our builds.
@tgamblin Requests can't print that message because the error originates from the old version of pip
.
@bjmc Yeah, this is definitely in the realm of "tricky to reason about". This is a problem that exists in a project we don't even explicitly depend on, we just expect to be present. At a certain point semantic versioning gets fuzzy, and I'd say we're in that realm. For example, how old a Linux kernel are we expected to support? It's not ever formally been defined as part of the semver contract in Requests: should it be? The same applies to pip: there's no way for us to programmatically express a pip version requirement so it's hard for us to enforce it as part of our processes or claim we follow semver in that regard.
However, it's been pointed out upthread that we may be able to improve our compatibility with some older pips. If we can do that, we will.
Example with pip 6.1.1 (but same with pip 8.1.1):
So, installing requests 2.14.0 implicitly requires pip >= 9.x. Should be at least in the release note as a disclaimer, or be fixed if it's not on purpose.