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

Copy pip's import machinery wholesale #2375

Merged
merged 2 commits into from Jan 9, 2015

Conversation

Projects
None yet
8 participants
@sigmavirus24
Member

sigmavirus24 commented Dec 9, 2014

Fedora and Debian have both recently added symlinks to their distributions of requests so people can do from requests.packages import urllib3 but they seem to still rewrite our import statements. So the following situation is possible:

  • User registers adapter for custom scheme

  • User does the following:

    from requests.packages.urllib3 import poolmanager
    
    poolmanager.pool_classes_by_scheme['glance+https'] = HTTPSConectionPool
  • When they do s.get('glance+https://...') they see a KeyError because the urllib3 we're using in requests is not the one they've imported (according to sys.modules).

Pip works around this with the copied machinery. The tests seem to work just fine for me. I still need to test this with our vendored packages removed and urllib3/chardet installed separately.

/cc @dstufft @eriolv @ralphbean

@sigmavirus24 sigmavirus24 added the Bug label Dec 9, 2014

@Lukasa

This comment has been minimized.

Member

Lukasa commented Dec 9, 2014

I don't really see why this is our problem. Debian/downstream have introduced this bug, why are we the ones fixing it?

@dstufft

This comment has been minimized.

Contributor

dstufft commented Dec 9, 2014

The reason pip did this is primarily because while it annoys me to no end what the downstream re-distributors do I felt that ultimately it was better for the end-users if we made sure this particular bug was fixed. In the end, as much as I wish they wouldn't, downstream is going to unbundle stuff and I think it's wholly better if they all do it in the same way that works more or less transparently than if each one invents it's own way of doing it with it's own caveats.

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Dec 9, 2014

@Lukasa because it's faster if we fix it and provide a little extra flexibility at no little extra cost.

@eriol

This comment has been minimized.

Contributor

eriol commented Dec 9, 2014

Hello,
I can understand how @Lukasa is annoyed by unvendoring made downstream, but I can also understand why this is done. For example, in Debian, Python SSLv3 support was removed and I only had to patch (I forwared my patch upstream of course[¹]) urllib3 to fix all packages depending on urllib3.

So, yes, this is a downstream problem, but as in past (even when this issue arosed on Debian Bug Tracker) I will never add something without asking first if you, upstream developers, are ok with a downstream change.
As I said a lot of time before being the Debian maintainer of requests I'm one of its users: I want requests to be in the best shape in Debian. :)

I agree with @dstufft and @sigmavirus24 but, if you don't agree, I can also replace the currently used patch in Debian with this so, at least, Debian, Ubuntu and pip will use the same code.
IMHO cooperating we will arrive to the best solution for all.

[¹] http://anonscm.debian.org/viewvc/python-modules/packages/python-urllib3/tags/1.9.1-3/debian/patches/06_do-not-make-SSLv3-mandatory.patch?view=markup Yes, next time I will use a PR, fortunately @dstufft forwarded it properly! :)

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Dec 9, 2014

@eriolv question: Since the symlink is in place (it may just be Fedora that has this in place), is there any chance of the imports that import from .packages inside of requests could not be rewritten?

If not, can the imports not be rewritten after we ship this patch? (After we've updated it to give proper attribution to @dstufft and pypa/pip) The crux of this issue is that sys.modules is incorrectly populated and needs to work a certain way for users to not run into surprises like this.

[/Edit - I submitted my comment too soon]
And I appreciate your collaboration @eriolv. That's why I pinged you immediately. I wanted to make you aware of this from the start and get your feedback as well as @Lukasa's and @ralphbean's

@dstufft

This comment has been minimized.

Contributor

dstufft commented Dec 9, 2014

You don't need to rewrite imports with either the symlinks or this solution. The major difference between symlinks and this solution is. With symlinks the import system is going to see urllib3 and requests.packages.urllib3 as two different things. With this it should treat them as the same because it's just aliases the exact same module object in sys.modules. This should mean, though I don't think I tested it, that it doesn't matter if people import from urllib3 or requests.packages.urllib3.

@Lukasa

This comment has been minimized.

Member

Lukasa commented Dec 9, 2014

My snarkiness was mostly the result of me waking up late and being late for work and having not had coffee, apologies all.

In reality I'm +0 on this. I don't like that we have to do it, and the unvendoring zealots have hardened my opinion towards the idea of doing them any favours on any issue whatsoever. (I don't include you in that group @eriolv, you have not displayed any zealotry that I'm aware of 😉 )

However, I acknowledge the Catch-22 of the fact that we will get blamed for the zealots decision to unbundle us breaking their code. For that reason I have no intention of blocking this patch: punishing users is unacceptable.

However, I'd like someone to test the change, as neither @sigmavirus24 nor @dstufft appear to have. Ideally I'd like some form of automated testing for it as well: having that would raise me to +0.5.

@dstufft

This comment has been minimized.

Contributor

dstufft commented Dec 9, 2014

To be clear, I tested it in the context of pip :) I have not tested it in the context of requests. The major difference being that pip's bundled stuff isn't generally imported outside of pip. In pip we also modify the setup.py so there is an environment variable that downstream can set to unbundle things without patching which raises a warning saying that it's an unsupported mechanism for installing pip. We then use this to install pip unbundled and run all of our tests in CI with installed copies of everything. However a difference between pip and requests in this is that pip will only bundle a released copy of something and we also have a vendor.txt which is a file in the requirements.txt file format which lists the exact versions we've bundled. So we can easily install the required things by just doing pip install -r pip/_vendor/vendor.txt.

@dstufft

This comment has been minimized.

Contributor

dstufft commented Dec 9, 2014

To be more clear! I know that this will make it so that import requests.packages.urllib3 will fall back to import urllib3 automatically without doing an import writes. The thing I haven't tested is that you can do something like:

import requests.packages.urllib3``
import urllib3

assert isinstance(urllib3.Timeout, requests.packages.urllib3.Timeout)
@@ -1,3 +1,80 @@
"""

This comment has been minimized.

@untitaker

untitaker Dec 9, 2014

Contributor

This docstring still refers to pip FWIW.

@dstufft

This comment has been minimized.

Contributor

dstufft commented Dec 9, 2014

Oh, and I wrote this in it's entirety, so feel free to license/attribute it however you want. I don't really care.

@Lukasa

This comment has been minimized.

Member

Lukasa commented Dec 9, 2014

@dstufft Indeed, and that's what's important. I want to make sure that if we're doing this we do it right the first time.

Note that it obviously won't do that on vendored systems. =)

@dstufft

This comment has been minimized.

Contributor

dstufft commented Dec 9, 2014

Yea, well really people should just always import from requests.packages and never from the top level packages if they are using things in conjunction with requests, but yea it +1 on testing that it works the way I think it does :D

@ralphbean

This comment has been minimized.

Contributor

ralphbean commented Dec 10, 2014

Fedora and Debian have both recently added symlinks to their distributions of requests so people can do from requests.packages import urllib3 but they seem to still rewrite our import statements

Hey, in Fedora rawhide (the most advanced branch, the "rolling release") we're not rewriting the imports anymore. That patch didn't make it into Fedora 21 though -- just released yesterday.

finally:
# Re-add any additions to sys.meta_path that were made while
# during the import we just did, otherwise things like
# pip._vendor.six.moves will fail.

This comment has been minimized.

@ralphbean

ralphbean Dec 10, 2014

Contributor

This comment, too, references pip. It could be updated to refer to requests instead?

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Dec 10, 2014

Thanks @ralphbean. Will Fedora 21 be able to generate a new build with this change at least?

Also, to everyone commenting on the fact that this file still references pip, I pulled this in and committed it to start a discussion. This will not be merged as is.

@ralphbean

This comment has been minimized.

Contributor

ralphbean commented Dec 10, 2014

Will Fedora 21 be able to generate a new build with this change at least?

Yes, it can. We needed to let it sit frozen in preparation for that initial release.

@kennethreitz

This comment has been minimized.

Member

kennethreitz commented Dec 17, 2014

This is so annoying :)

@kennethreitz

This comment has been minimized.

Member

kennethreitz commented Dec 19, 2014

I dislike everything about this.

Not saying we shouldn't be doing it yet — just documenting my feelings.

@sigmavirus24 sigmavirus24 changed the title from WIP: Copy pip's import machinery wholesale to Copy pip's import machinery wholesale Dec 23, 2014

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Dec 23, 2014

So I did the following:

$ mktmpenv -ppython{{version}}
$ pip install /path/to/requests/  # Where this is the git repo checked out on this branch
$ cd lib/python{{version}}/site-packages/requests/packages/
$ rm -rf chardet urllib3
$ cd - # Back in the temporary virtualenv directory
$ pip install chardet urllib3
$ python

Then at the console I did

>>> import requests
>>> requests.get('https://httpbin.org/get')
<Response [200]>
>>> import sys
>>> sys.modules['requests.packages.urllib3']
<module 'urllib3' from '/Users/ian7708/virtualenv/tmp-421360116860cb19/lib/python2.7/site-packages/urllib3/__init__.pyc'>
>>> import urllib3
>>> sys.modules['requests.packages.urllib3'] is urllib3
True

So we're getting the right module and it has the right name. I'm forgetting though, were there other things we need to check?

@dstufft

This comment has been minimized.

Contributor

dstufft commented Dec 23, 2014

It should hold true but the only thing I can think of is make sure that assert isinstance(urllib3.Timeout, requests.packages.urllib3.Timeout) is True.

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Dec 23, 2014

>>> isinstance(urllib3.Timeout(5), requests.packages.urllib3.Timeout)
True
@dstufft

This comment has been minimized.

Contributor

dstufft commented Dec 23, 2014

It should totally work then.

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Dec 23, 2014

I'm comfortable using this. It'd be great to have access to our CI and set up a job for this but I'm not sure that will happen in a practical period of time

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Dec 24, 2014

@Lukasa thoughts?

@Lukasa

This comment has been minimized.

Member

Lukasa commented Dec 24, 2014

I'm with @kennethreitz. I don't like it, but I think we have to do it.

@dstufft

This comment has been minimized.

Contributor

dstufft commented Dec 24, 2014

To be clear, I don't like it either, but given that downstream is unlikely to stop de-bundling anytime soon I prefer to control the way they do it.

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Dec 24, 2014

I think the only people who will be happy to have this will be our users frankly. Aren't they who matter most?

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Dec 31, 2014

@kennethreitz @Lukasa either of you have time to give this another once over? This will vastly improve user experience when using downstream distributed modules. Believe it, or not, it will also reduce the number of changes downstream re-distributors need to make to requests itself as well. Finally, it will make requests far more stable for our users on those systems who do not have the ability to "Just use pip". I still prefer to use pip myself (because I like to have a better control over what I use) but that isn't a luxury everyone has who wants to (and needs to) use requests. We shouldn't begrudge them for having a different set of constraints than we typically have.

@Lukasa

This comment has been minimized.

Member

Lukasa commented Dec 31, 2014

The code looks good to me, as it did originally, though I'm hardly an expert on Python's import machinery.

I still resent the requirement to add this code. I accept the benefits and acknowledge the need for it, I just refuse to be happy about it. ;) Basically, I feel the same way about this as I do about cleaning the house.

I'm +1 on merging, I'm sure I'll get over the pain. Want @kennethreitz to sign off though.

@kennethreitz

This comment has been minimized.

Member

kennethreitz commented Jan 9, 2015

I don't like this. I'd much rather convince the distro maintainers to stop mangling our code.

@untitaker

This comment has been minimized.

Contributor

untitaker commented Jan 9, 2015

I am currently using requests and requests-toolbelt in a CLI tool (which breaks due to this kind of imports), and since...

  • no clear consensus has been found so far
  • requests/toolbelt#40 doesn't seem to get merged anytime soon
  • I get support requests for this every week

...I am considering monkeypatching sys.modules for my tool to work on older Debian versions. For newer ones, I don't think there's a fix for such issues.

@dstufft

This comment has been minimized.

Contributor

dstufft commented Jan 9, 2015

@kennethreitz you have approximately a zero chance of convincing them to do that. I spent months on it for pip and basically gave up. As far as I can tell your options are either provide the tooling for unbundling to happen in a way that you control, or they are going to do it however each individual distro feels like doing it with varying levels of support for thinks like requests-toolbelt needing to import from urllib3.

Ubuntu 14.04 for instance ships with a completely broken ensurepip and a broken by default venv module, and they knew it was broken at the time, because ensurepip bundles pip and they didn't have a a solution for handling the bundling in a way that they considered acceptable. From what I can tell a lot of distros are perfectly happen shipping broken things as long as it's not broken for their particular use cases. This sucks but I don't think it's going to change.

Personally in pip I decided that grabbing my nose to deal with the stink and getting at least consistent de-bundling was worth the somewhat gross code when the alternative was that end users would suffer.

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Jan 9, 2015

Cool. While we wait for you to do that @kennethreitz our users can continue to suffer.

@kennethreitz

This comment has been minimized.

Member

kennethreitz commented Jan 9, 2015

This is officially @dstufft's decision. :)

@dstufft

This comment has been minimized.

Contributor

dstufft commented Jan 9, 2015

I say merge it. It'll improve the end users life.

kennethreitz added a commit that referenced this pull request Jan 9, 2015

@kennethreitz kennethreitz merged commit a57eacf into requests:master Jan 9, 2015

@kennethreitz

This comment has been minimized.

Member

kennethreitz commented Jan 9, 2015

yay delegation

@dstufft gets a cookie 🍪

@kennethreitz

This comment has been minimized.

Member

kennethreitz commented Jan 9, 2015

and infinite glasses of milk 🐄

@eriol

This comment has been minimized.

Contributor

eriol commented Jan 9, 2015

@kennethreitz et al, many thanks for this! :)

@sigmavirus24 sigmavirus24 deleted the sigmavirus24:fix-imports-when-unbundled branch Jan 9, 2015

@legrostdg

This comment has been minimized.

legrostdg commented Jan 25, 2015

Would it be possible to have a small dot release containing this fix? Thanks!

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Jan 25, 2015

@legrostdg maybe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment