Import aliases for Debian

[untitaker]

Alternative to #2375, on the risk of being naive...

[Lukasa]

@sigmavirus24 You own this logic for the most part: how does this look?

[Lukasa]

@sigmavirus24 You own this logic for the most part: how does this look?

[untitaker]

Also I suppose it'd be nice if @dcramer tries this (because of #2558)

[untitaker]

Also I suppose it'd be nice if @dcramer tries this (because of #2558)

[sigmavirus24]

It looks okay but I want to toy with it a bit before we merge it.

[sigmavirus24]

It looks okay but I want to toy with it a bit before we merge it.

[untitaker]

If you're fine with this, I'd like to add a module docstring (or comment) that explains the whole situation before merging.

[untitaker]

If you're fine with this, I'd like to add a module docstring (or comment) that explains the whole situation before merging.

[sigmavirus24]

@untitaker why not just add it while I'm testing it?

[sigmavirus24]

@untitaker why not just add it while I'm testing it?

[untitaker]

I just meant to say that so you don't merge this too soon.

[untitaker]

I just meant to say that so you don't merge this too soon.

[dcramer]

From my POV this seems safe. @mitsuhiko might have better feedback

[dcramer]

From my POV this seems safe. @mitsuhiko might have better feedback

[eriol]

@untitaker thanks for working on this. Just for reference, here is what it's used in Debian at the moment:
https://anonscm.debian.org/viewvc/python-modules/packages/requests/trunk/debian/patches/04_make-requests.packages.urllib3-same-as-urllib3.patch?revision=32576&view=markup

Yes, I'm exporting only urllib3 since importing chardet from requests.packages seems not used, at least no one complained about this.

I choosed to not cherry pick #2375 due the problems emerged: I don't want Debian and Ubuntu users to have a system version of requests not in the best shape: it will give only more problems.

[eriol]

@untitaker thanks for working on this. Just for reference, here is what it's used in Debian at the moment:
https://anonscm.debian.org/viewvc/python-modules/packages/requests/trunk/debian/patches/04_make-requests.packages.urllib3-same-as-urllib3.patch?revision=32576&view=markup

Yes, I'm exporting only urllib3 since importing chardet from requests.packages seems not used, at least no one complained about this.

I choosed to not cherry pick #2375 due the problems emerged: I don't want Debian and Ubuntu users to have a system version of requests not in the best shape: it will give only more problems.

[untitaker]

Seems good @eriol. Would you adopt a solution offered by requests though, to avoid Distro-specific breakage?

BTW @sigmavirus24 this is ready for review.

[untitaker]

Seems good @eriol. Would you adopt a solution offered by requests though, to avoid Distro-specific breakage?

BTW @sigmavirus24 this is ready for review.

[eriol]

@untitaker of course I will like to drop a custom patch! 😄
I had to write more to explain better my opinion, sorry for this. My opinion is that less divergence from upstream is the better, and it's for this reason that I forward all modification non Distro-specific to upstream.
Furthermore the Debian patch is not perfect, see Debian Bug #771349.
So to recap I will be extremely happy to drop 04_make-requests.packages.urllib3-same-as-urllib3.patch in favor of an upstream solution.
Thanks!

[eriol]

@untitaker of course I will like to drop a custom patch! 😄
I had to write more to explain better my opinion, sorry for this. My opinion is that less divergence from upstream is the better, and it's for this reason that I forward all modification non Distro-specific to upstream.
Furthermore the Debian patch is not perfect, see Debian Bug #771349.
So to recap I will be extremely happy to drop 04_make-requests.packages.urllib3-same-as-urllib3.patch in favor of an upstream solution.
Thanks!

[sigmavirus24]

This works well for me. Objections to merging @Lukasa?

[sigmavirus24]

This works well for me. Objections to merging @Lukasa?

[Lukasa]

@sigmavirus24 I'm super reluctant to rush this out, given the way stuff previously went. Can we try to tap people who've had problems/objections in the past and get them to confirm that this works better for them?

[Lukasa]

@sigmavirus24 I'm super reluctant to rush this out, given the way stuff previously went. Can we try to tap people who've had problems/objections in the past and get them to confirm that this works better for them?

[untitaker]

I'll try to reach mitsuhiko

[untitaker]

I'll try to reach mitsuhiko

[sigmavirus24]

@untitaker did you mean @mitsuhiko? I would hope a direct ping would suffice.

---

@Lukasa I would argue this is probably, on the whole, better than our previous meta_path hackery for several reasons:

1. It provides the same functionality

2. It's significantly simpler (simple is better than complex)

3. It allows for all of this to work in the case where requests is vendored without its vendored dependencies (see @untitaker's use of __name__ when adding the alias to sys.modules)

4. It doesn't mess with other meta_path plugins (e.g., PyInstaller, the hack that @dcramer and @mitsuhiko are using, etc.)

5. It falls back in the correct order

6. It's far easier to explain to someone

7. When urllib3 is not vendored, the following works as we'd like it to:

   import urllib3
   from requests.packages import urllib3 as rurllib3
   urllib3 is rurllib3

I have yet to test this with PyInstaller, but the root of the problem there was the fact that our meta_path plugin was in the wrong place relative to the multiple plugins that PyInstaller uses. So with that removed, this should just work. I'm also confident that if @mitsuhiko and @dcramer test this with their code that hacks the meta_path, then they'll not see any problems.

[sigmavirus24]

@untitaker did you mean @mitsuhiko? I would hope a direct ping would suffice.

---

@Lukasa I would argue this is probably, on the whole, better than our previous meta_path hackery for several reasons:

1. It provides the same functionality

2. It's significantly simpler (simple is better than complex)

3. It allows for all of this to work in the case where requests is vendored without its vendored dependencies (see @untitaker's use of __name__ when adding the alias to sys.modules)

4. It doesn't mess with other meta_path plugins (e.g., PyInstaller, the hack that @dcramer and @mitsuhiko are using, etc.)

5. It falls back in the correct order

6. It's far easier to explain to someone

7. When urllib3 is not vendored, the following works as we'd like it to:

   import urllib3
   from requests.packages import urllib3 as rurllib3
   urllib3 is rurllib3

I have yet to test this with PyInstaller, but the root of the problem there was the fact that our meta_path plugin was in the wrong place relative to the multiple plugins that PyInstaller uses. So with that removed, this should just work. I'm also confident that if @mitsuhiko and @dcramer test this with their code that hacks the meta_path, then they'll not see any problems.

[untitaker]

@sigmavirus24

@untitaker did you mean @mitsuhiko? I would hope a direct ping would suffice.

Unfortunately I don't think so. I haven't heard back from him either, perhaps @dcramer can ping him about that?

…

--- @eriol BTW this should also remove the need to rewrite every import statement inside requests itself.

[untitaker]

@sigmavirus24

@untitaker did you mean @mitsuhiko? I would hope a direct ping would suffice.

Unfortunately I don't think so. I haven't heard back from him either, perhaps @dcramer can ping him about that?

…

--- @eriol BTW this should also remove the need to rewrite every import statement inside requests itself.

[Lukasa]

@sigmavirus24 I'm not disputing better at all. =) What I'd like to do is to take all reasonable precautions to reduce the risk of deploying this fix. For example, can @eriol and @ralphbean confirm that their package building functions correctly with this patch?

Basically, rushing helps nobody, and I'd like to try to begin a run of stable requests releases if at all possible. The last run of four-or-five broken releases in a row is bad, and we need to not get in the habit of doing that.

[Lukasa]

@sigmavirus24 I'm not disputing better at all. =) What I'd like to do is to take all reasonable precautions to reduce the risk of deploying this fix. For example, can @eriol and @ralphbean confirm that their package building functions correctly with this patch?

Basically, rushing helps nobody, and I'd like to try to begin a run of stable requests releases if at all possible. The last run of four-or-five broken releases in a row is bad, and we need to not get in the habit of doing that.

[sigmavirus24]

@Lukasa Note that I'm not arguing for this to be merged with a release to follow immediately after. We could even defer this to 3.0 if you want to not rush it (which I don't think we're doing frankly). The current state of affairs only makes end users' lives more difficult at this point.

[sigmavirus24]

@Lukasa Note that I'm not arguing for this to be merged with a release to follow immediately after. We could even defer this to 3.0 if you want to not rush it (which I don't think we're doing frankly). The current state of affairs only makes end users' lives more difficult at this point.

[eriol]

@Lukasa if is it possible I would like to have a bit more of time: right now I'm checking reverse dependencies of requests in Debian to not break stuff, see: https://lists.debian.org/debian-python/2015/05/msg00021.html (@sigmavirus24 thanks for details, I did not have time to reply on the list, but your mail was appreciated 😄).
I should finish during this week, so should be able to cherry pick and test this immediately after.

[eriol]

@Lukasa if is it possible I would like to have a bit more of time: right now I'm checking reverse dependencies of requests in Debian to not break stuff, see: https://lists.debian.org/debian-python/2015/05/msg00021.html (@sigmavirus24 thanks for details, I did not have time to reply on the list, but your mail was appreciated 😄).
I should finish during this week, so should be able to cherry pick and test this immediately after.

[sigmavirus24]

@eriol always happy to help. We don't have a release planned so we don't have a timeline for when this would be merged. Take your time. Don't stress out. We'll be here and so will the patch. =D

[sigmavirus24]

@eriol always happy to help. We don't have a release planned so we don't have a timeline for when this would be merged. Take your time. Don't stress out. We'll be here and so will the patch. =D

[wangshaochen]

How to down the the code?

[wangshaochen]

How to down the the code?

[eriol]

@wangshaochen you can just add ".patch" to PR URL:
https://github.com/kennethreitz/requests/pull/2567.patch

HTH!

[eriol]

@wangshaochen you can just add ".patch" to PR URL:
https://github.com/kennethreitz/requests/pull/2567.patch

HTH!

[wangshaochen]

@eriol Ths! I yet don't know. with command_line?

[wangshaochen]

@eriol Ths! I yet don't know. with command_line?

[untitaker]

It's not clear what you want to do. Could you tell us more?

[untitaker]

It's not clear what you want to do. Could you tell us more?

[wangshaochen]

i'm sorry. this is my first-time see it.

[wangshaochen]

i'm sorry. this is my first-time see it.

[untitaker]

@Lukasa @sigmavirus24 any updates on this?

[untitaker]

@Lukasa @sigmavirus24 any updates on this?

[Lukasa]

We're still hoping to have @eriol and/or @ralphbean take a swing at this code and confirm it's working for them.

[Lukasa]

We're still hoping to have @eriol and/or @ralphbean take a swing at this code and confirm it's working for them.

[eriol]

I waited to upload requests 2.7.0 in unstable (I uploaded into experimental first) to give httpie maintainer the time to update Debian package (we had httpie 0.8).
Unfortunately this week I'm too busy, so I can cherry pick and test only starting from next week.

[eriol]

I waited to upload requests 2.7.0 in unstable (I uploaded into experimental first) to give httpie maintainer the time to update Debian package (we had httpie 0.8).
Unfortunately this week I'm too busy, so I can cherry pick and test only starting from next week.

[ralphbean]

Looks good here!

[ralphbean]

Looks good here!

[eriol]

Sorry for the delay! I have tested it and all seem fine! Many thanks @untitaker!

[eriol]

Sorry for the delay! I have tested it and all seem fine! Many thanks @untitaker!

[Lukasa]

\o/ Alright, I'm happy to cautiously wave this through.

[Lukasa]

\o/ Alright, I'm happy to cautiously wave this through.

[sigmavirus24]

So as for including this in a release...

I could be swayed towards either 2.8.0 or 2.7.1. Reasoning:

Prior to 2.7.0 we had several releases that made this work on every system regardless of whether the dependencies were vendored or not. 2.7.0 broke this. This constitutes a fix for 2.7.0. (That's sort of a weak argument IMO.)

2.8.0 makes sense because this is something that could (but I highly doubt will) break things.

[sigmavirus24]

So as for including this in a release...

I could be swayed towards either 2.8.0 or 2.7.1. Reasoning:

Prior to 2.7.0 we had several releases that made this work on every system regardless of whether the dependencies were vendored or not. 2.7.0 broke this. This constitutes a fix for 2.7.0. (That's sort of a weak argument IMO.)

2.8.0 makes sense because this is something that could (but I highly doubt will) break things.

[Lukasa]

I vote for 2.8.0.

[Lukasa]

I vote for 2.8.0.

[sigmavirus24]

Thought you'd say that.

[sigmavirus24]

Thought you'd say that.

[warsaw]

I've done a fair bit of testing of this patch for Debian and I think it does exactly what we want it to do. @eriol I've been able to remove the 02_user-system-chardet-and-urllib3.patch and 04_make-requests.packages.urllib3-same-as-urllib3.patch quilt patches and replace it with one that essentially does the PR submitted here. AFAICT, it all looks pretty good. I have a diff against the package that makes it all work, and I'd be inclined to upload a 2.7.0-3 with this patch, even if upstream requests puts it off until 2.8. I'll open a Debian bug with additional details.

+1 for this PR from me.

[warsaw]

I've done a fair bit of testing of this patch for Debian and I think it does exactly what we want it to do. @eriol I've been able to remove the 02_user-system-chardet-and-urllib3.patch and 04_make-requests.packages.urllib3-same-as-urllib3.patch quilt patches and replace it with one that essentially does the PR submitted here. AFAICT, it all looks pretty good. I have a diff against the package that makes it all work, and I'd be inclined to upload a 2.7.0-3 with this patch, even if upstream requests puts it off until 2.8. I'll open a Debian bug with additional details.

+1 for this PR from me.

[warsaw]

The Debian bug report with the packaging diff is at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=788383

[warsaw]

The Debian bug report with the packaging diff is at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=788383

[eriol]

@warsaw thanks for the debdiff, I forgot to commit my changes when I tested it. I agree with you, I want to remove Debian patches in favor of this and I was already working on 2.7.0-3. 😄

[eriol]

@warsaw thanks for the debdiff, I forgot to commit my changes when I tested it. I agree with you, I want to remove Debian patches in favor of this and I was already working on 2.7.0-3. 😄

[eriol]

Uploaded yesterday in Debian unstable: https://packages.qa.debian.org/r/requests/news/20150611T073422Z.html

[eriol]

Uploaded yesterday in Debian unstable: https://packages.qa.debian.org/r/requests/news/20150611T073422Z.html

[sigmavirus24]

pip has adopted a similar patch pypa/pip#2925

[sigmavirus24]

pip has adopted a similar patch pypa/pip#2925

[untitaker]

Are there any notable differences that should be incorporated into this PR as
well?

On Tue, Jun 23, 2015 at 07:38:32PM -0700, Ian Cordasco wrote:

pip has adopted a similar patch pypa/pip#2925

---

Reply to this email directly or view it on GitHub:
#2567 (comment)

[untitaker]

Are there any notable differences that should be incorporated into this PR as
well?

On Tue, Jun 23, 2015 at 07:38:32PM -0700, Ian Cordasco wrote:

pip has adopted a similar patch pypa/pip#2925

---

Reply to this email directly or view it on GitHub:
#2567 (comment)

[sigmavirus24]

@untitaker to be honest, there are some differences. @dstufft mentioned that his patch includes something that handles from pip._vendor import requests as well as import pip._vendor.requests but I'm not sure we need that because I think ours just works as it's written. Perhaps @dstufft could explain more about what he found to be necessary in his patch that didn't work with this (assuming he tried this one in pip)

[sigmavirus24]

@untitaker to be honest, there are some differences. @dstufft mentioned that his patch includes something that handles from pip._vendor import requests as well as import pip._vendor.requests but I'm not sure we need that because I think ours just works as it's written. Perhaps @dstufft could explain more about what he found to be necessary in his patch that didn't work with this (assuming he tried this one in pip)

[mitsuhiko]

Since @kennethreitz just pinged me in #2678 i want to give some comments about this. Given all the crappy solutions about this debian bundling thing, this is probably the best so far. My original complaint was about the badly broken import hook. This one comes with it's own insanity.

For a start: if someone imports from requests.packages.urllib3 import submodule and then later someome imports from urllib3 import submodule (with the patch in place), then submodule.__name__ is requests.packages.urllib3.submodule and not urllib3.submodule. This will break pickle in urllib3 as an example. Since you are already down so far the "we don't care about any of this" route I think this is the closest you will get to achieving your goal.

I still argue that what you should be doing is just stop this vendoring. None of this patching business makes any sense.

There be many more dragons.

[mitsuhiko]

Since @kennethreitz just pinged me in #2678 i want to give some comments about this. Given all the crappy solutions about this debian bundling thing, this is probably the best so far. My original complaint was about the badly broken import hook. This one comes with it's own insanity.

For a start: if someone imports from requests.packages.urllib3 import submodule and then later someome imports from urllib3 import submodule (with the patch in place), then submodule.__name__ is requests.packages.urllib3.submodule and not urllib3.submodule. This will break pickle in urllib3 as an example. Since you are already down so far the "we don't care about any of this" route I think this is the closest you will get to achieving your goal.

I still argue that what you should be doing is just stop this vendoring. None of this patching business makes any sense.

There be many more dragons.

[sigmavirus24]

Further entertaining corner behaviour

import requests
print(requests.packages.urllib3.__name__)
# urllib3
from requests.packages.urllib3 import connection
import urllib3
print(urllib3.connection.__name__)
# requests.packages.urllib3.connection

[sigmavirus24]

Further entertaining corner behaviour

import requests
print(requests.packages.urllib3.__name__)
# urllib3
from requests.packages.urllib3 import connection
import urllib3
print(urllib3.connection.__name__)
# requests.packages.urllib3.connection

[untitaker]

As far as I understand the main advantage Kenneth sees in vendoring urllib3 is that a minimal installation can be done by just unpacking a tarball.

Perhaps there is a way to provide this comfort without making urllib3 a subpackage of requests?

On 29 July 2015 17:52:10 CEST, Armin Ronacher notifications@github.com wrote:

Since @kennethreitz just pinged me in #2678 i want to give some
comments about this. Given all the crappy solutions about this debian
bundling thing, this is probably the best so far. My original
complaint was about the badly broken import hook. This one comes with
it's own insanity.

For a start: if someone imports from requests.packages.urllib3 import submodule and then later someome imports from urllib3 import submodule (with the patch in place), then submodule.name is
requests.packages.urllib3.submodule and not urllib3.submodule.
This will break pickle in urllib3 as an example. Since you are already
down so far the "we don't care about any of this" route I think this is
the closest you will get to achieving your goal.

I still argue that what you should be doing is just stop this
vendoring. None of this patching business makes any sense.

There be many more dragons.

---

Reply to this email directly or view it on GitHub:
#2567 (comment)

Sent from my phone. Please excuse my brevity.

[untitaker]

As far as I understand the main advantage Kenneth sees in vendoring urllib3 is that a minimal installation can be done by just unpacking a tarball.

Perhaps there is a way to provide this comfort without making urllib3 a subpackage of requests?

On 29 July 2015 17:52:10 CEST, Armin Ronacher notifications@github.com wrote:

Since @kennethreitz just pinged me in #2678 i want to give some
comments about this. Given all the crappy solutions about this debian
bundling thing, this is probably the best so far. My original
complaint was about the badly broken import hook. This one comes with
it's own insanity.

For a start: if someone imports from requests.packages.urllib3 import submodule and then later someome imports from urllib3 import submodule (with the patch in place), then submodule.name is
requests.packages.urllib3.submodule and not urllib3.submodule.
This will break pickle in urllib3 as an example. Since you are already
down so far the "we don't care about any of this" route I think this is
the closest you will get to achieving your goal.

I still argue that what you should be doing is just stop this
vendoring. None of this patching business makes any sense.

There be many more dragons.

---

Reply to this email directly or view it on GitHub:
#2567 (comment)

Sent from my phone. Please excuse my brevity.

[kennethreitz]

Maybe we could just add a warning for when the package has been removed :)

[kennethreitz]

Maybe we could just add a warning for when the package has been removed :)

[dstufft]

I would be +1 for requests to just depend on urllib3 using the packaging tooling :D

[dstufft]

I would be +1 for requests to just depend on urllib3 using the packaging tooling :D

[untitaker]

I agree.

Perhaps a bundled version could be created for users who don't want to use PIP,
but I don't think doing this in the PyPI tarball warrants the pain.

On Fri, Aug 07, 2015 at 01:43:49PM -0700, Donald Stufft wrote:

I would be +1 for requests to just depend on urllib3 using the packaging tooling :D

---

Reply to this email directly or view it on GitHub:
#2567 (comment)

[untitaker]

I agree.

Perhaps a bundled version could be created for users who don't want to use PIP,
but I don't think doing this in the PyPI tarball warrants the pain.

On Fri, Aug 07, 2015 at 01:43:49PM -0700, Donald Stufft wrote:

I would be +1 for requests to just depend on urllib3 using the packaging tooling :D

---

Reply to this email directly or view it on GitHub:
#2567 (comment)

[sigmavirus24]

I'm not sure a warning will be effective since they're off by default in every Python after 2.6. They already effectively get a warning in the form of an import error.

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[sigmavirus24]

I'm not sure a warning will be effective since they're off by default in every Python after 2.6. They already effectively get a warning in the form of an import error.

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[dstufft]

Also the warning would only happen in the downstream modified requests, and presumably they'd just remove the warning too.

[dstufft]

Also the warning would only happen in the downstream modified requests, and presumably they'd just remove the warning too.

[sigmavirus24]

Not to take one side or the other, but if we were to change our position on vendoring, this would make updating chardet a lot easier. The project is now using enum from the Python 3.x stdlib (or enum34 if on Python 2.x) and there's no easy way for us to vendor that version unless the project doesn't use enum. This prevents us from getting some very real fixes that would benefit our users. Also, constraining urllib3's version in the dependency would be an interesting topic for discussion.

[sigmavirus24]

Not to take one side or the other, but if we were to change our position on vendoring, this would make updating chardet a lot easier. The project is now using enum from the Python 3.x stdlib (or enum34 if on Python 2.x) and there's no easy way for us to vendor that version unless the project doesn't use enum. This prevents us from getting some very real fixes that would benefit our users. Also, constraining urllib3's version in the dependency would be an interesting topic for discussion.

[Lukasa]

Ok, I think there's no longer consensus on this issue.

I propose releasing 2.8.0 without this patch in it in the short term, to unblock our release process. Separately, we can decide whether or not we're changing our approach to this problem.

[Lukasa]

Ok, I think there's no longer consensus on this issue.

I propose releasing 2.8.0 without this patch in it in the short term, to unblock our release process. Separately, we can decide whether or not we're changing our approach to this problem.

[sigmavirus24]

I disagree. I think we have consensus from downstream that this solves things there. The lack of consensus on here is whether requests should even be vendoring its dependencies. I think any solution around this will be measured by what is the least objectionable way of handling this (if we decide to handle it at all). We can probably dig around for less objectionable ways of doing this without unvendoring requests, but I'm not sure there are many (if there are any).

Perhaps the real solution is to not tell people to import things from urllib3. Perhaps anything that's meant for end-users that needs to come from urllib3 should be provided through the toolbelt? Now that we have it, we can handle the complexity of urllib3 versions and importing the right thing there.

Thoughts?

[sigmavirus24]

I disagree. I think we have consensus from downstream that this solves things there. The lack of consensus on here is whether requests should even be vendoring its dependencies. I think any solution around this will be measured by what is the least objectionable way of handling this (if we decide to handle it at all). We can probably dig around for less objectionable ways of doing this without unvendoring requests, but I'm not sure there are many (if there are any).

Perhaps the real solution is to not tell people to import things from urllib3. Perhaps anything that's meant for end-users that needs to come from urllib3 should be provided through the toolbelt? Now that we have it, we can handle the complexity of urllib3 versions and importing the right thing there.

Thoughts?

[dstufft]

Speaking as someone who vendors requests, if requests stopped vendoring urllib3 it would barely affect my ability to vendor requests. It'd take me like 3 seconds to adjust.

[dstufft]

Speaking as someone who vendors requests, if requests stopped vendoring urllib3 it would barely affect my ability to vendor requests. It'd take me like 3 seconds to adjust.

[Lukasa]

I don't think we can tell people not to import from urllib3: there are too many documents etc. around the web that indicate that we expected people to do that. It'll be really painful trying to discourage that behaviour in future, especially as we can't necessarily very easily deprecate/remove it.

I'm mostly interested to see if @kennethreitz is actually open to considering unvendoring. If he's not, this is the best solution we have so far and we should roll with it.

[Lukasa]

I don't think we can tell people not to import from urllib3: there are too many documents etc. around the web that indicate that we expected people to do that. It'll be really painful trying to discourage that behaviour in future, especially as we can't necessarily very easily deprecate/remove it.

I'm mostly interested to see if @kennethreitz is actually open to considering unvendoring. If he's not, this is the best solution we have so far and we should roll with it.

[sigmavirus24]

I'm not sure painful is a sufficient reason to stay providing a better experience for all of our users

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[sigmavirus24]

I'm not sure painful is a sufficient reason to stay providing a better experience for all of our users

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[Lukasa]

I don't think telling people "don't import from urllib3" is a 'better' experience by any measure, just a different one. And I think the fact that it's a direct contradiction of our previous position makes it potentially a bit of a worse experience. But I could be wrong here.

[Lukasa]

I don't think telling people "don't import from urllib3" is a 'better' experience by any measure, just a different one. And I think the fact that it's a direct contradiction of our previous position makes it potentially a bit of a worse experience. But I could be wrong here.

[dstufft]

For whatever it's worth, I think a lot of the end user pain when things get unbundled is caused because requests bundles urllib3 but it's not just a private implementation detail (as much as it may be intended to be). Either it should be just a private implementation detail and users shouldn't be expected to import from it and requests itself will need to encaspulate all of the functionality that end users are expected to need without them needing to touch urllib3, or it should be a real dependency declared in setup.py.

[dstufft]

For whatever it's worth, I think a lot of the end user pain when things get unbundled is caused because requests bundles urllib3 but it's not just a private implementation detail (as much as it may be intended to be). Either it should be just a private implementation detail and users shouldn't be expected to import from it and requests itself will need to encaspulate all of the functionality that end users are expected to need without them needing to touch urllib3, or it should be a real dependency declared in setup.py.

[warsaw]

@dstufft : +1

[warsaw]

@dstufft : +1

[rschoon]

Either it should be just a private implementation detail and users shouldn't be expected to import from it and requests itself will need to encaspulate all of the functionality that end users are expected to need without them needing to touch urllib3, or it should be a real dependency declared in setup.py.

I think that's definitely a piece of the problem. The documentation mentions urllib3 quite a lot, but only mentions that urllib3 is vendored in passing (in the logging example, no less). It's quite easy to get to from urllib3 import Retry when you really ought to be doing from requests.packages.urllib3 import Retry, especially when it says "import urllib3’s Retry class". I think I only knew to use the vendored name because I had heard of the big mess from distros trying to unvendor it.

Except, if you're trying to be clever when unvendoring, the latter isn't right either, since that's not what requests imports it as:

>>> from urllib3 import Retry as Retry1
>>> from requests.packages.urllib3 import Retry as Retry2
>>> from requests.packages.urllib3.util import Retry as Retry3
>>> from requests.adapters import Retry as Retry4
>>> print(id(Retry1), id(Retry2))
38364744 38364744
>>> print(id(Retry3), id(Retry4))
39516104 39516104

In the example, Retry4 will always be the version that requests actually uses, and because the Retry.from_int class method has an isinstance check, the identity matters. I've seen this behavior with the removed VendorAlias() mechanism, but this pull request seems to suffer from it too. I'm not sure I entirely understand the reasoning for Python's behavior here, either:

>>> Retry2.__module__
'urllib3.util.retry'
>>> sys.modules[Retry2.__module__]
<module 'urllib3.util.retry' from '/home/rschoon/tmp/req/env2/lib/python3.4/site-packages/urllib3/util/retry.py'>
>>> Retry3.__module__
'requests.packages.urllib3.util.retry'
>>> sys.modules[Retry3.__module__]
<module 'requests.packages.urllib3.util.retry' from '/home/rschoon/tmp/req/env2/lib/python3.4/site-packages/urllib3/util/retry.py'>

(Fortunately, the import order does not seem to matter.)

Anyway, if unvendoring from requests proper isn't an option (there's surely backward compatibility issues to deal with), I really feel that it'd be a good idea to give parts of urllib3 that are exposed official "requests" names. At a minimum just making the name of the Retry class in the documentation be "requests.adapters.Retry" would improve things.

[rschoon]

Either it should be just a private implementation detail and users shouldn't be expected to import from it and requests itself will need to encaspulate all of the functionality that end users are expected to need without them needing to touch urllib3, or it should be a real dependency declared in setup.py.

I think that's definitely a piece of the problem. The documentation mentions urllib3 quite a lot, but only mentions that urllib3 is vendored in passing (in the logging example, no less). It's quite easy to get to from urllib3 import Retry when you really ought to be doing from requests.packages.urllib3 import Retry, especially when it says "import urllib3’s Retry class". I think I only knew to use the vendored name because I had heard of the big mess from distros trying to unvendor it.

Except, if you're trying to be clever when unvendoring, the latter isn't right either, since that's not what requests imports it as:

>>> from urllib3 import Retry as Retry1
>>> from requests.packages.urllib3 import Retry as Retry2
>>> from requests.packages.urllib3.util import Retry as Retry3
>>> from requests.adapters import Retry as Retry4
>>> print(id(Retry1), id(Retry2))
38364744 38364744
>>> print(id(Retry3), id(Retry4))
39516104 39516104

In the example, Retry4 will always be the version that requests actually uses, and because the Retry.from_int class method has an isinstance check, the identity matters. I've seen this behavior with the removed VendorAlias() mechanism, but this pull request seems to suffer from it too. I'm not sure I entirely understand the reasoning for Python's behavior here, either:

>>> Retry2.__module__
'urllib3.util.retry'
>>> sys.modules[Retry2.__module__]
<module 'urllib3.util.retry' from '/home/rschoon/tmp/req/env2/lib/python3.4/site-packages/urllib3/util/retry.py'>
>>> Retry3.__module__
'requests.packages.urllib3.util.retry'
>>> sys.modules[Retry3.__module__]
<module 'requests.packages.urllib3.util.retry' from '/home/rschoon/tmp/req/env2/lib/python3.4/site-packages/urllib3/util/retry.py'>

(Fortunately, the import order does not seem to matter.)

Anyway, if unvendoring from requests proper isn't an option (there's surely backward compatibility issues to deal with), I really feel that it'd be a good idea to give parts of urllib3 that are exposed official "requests" names. At a minimum just making the name of the Retry class in the documentation be "requests.adapters.Retry" would improve things.

[untitaker]

@rschoon yes, the problems with __name__ have already been mentioned by mitsuhiko before.

I'd just opt for unbundling all those dependencies, and optionally providing a separate tarball that contains all dependencies (all as top-level package each, not inside the requests package).

[untitaker]

@rschoon yes, the problems with __name__ have already been mentioned by mitsuhiko before.

I'd just opt for unbundling all those dependencies, and optionally providing a separate tarball that contains all dependencies (all as top-level package each, not inside the requests package).

[Lukasa]

So folks, the reality of the matter is that unbundling is not going to happen in the short term. Kenneth is currently stepped back a bit from the project but it was always his opinion that was strongest on this matter: until we can get time to sit down and talk about this in depth we're not going to unvendor anything.

The question is whether this change is superior to the status quo. For the moment, please assume unvendoring is off the table: sorry. 😞

[Lukasa]

So folks, the reality of the matter is that unbundling is not going to happen in the short term. Kenneth is currently stepped back a bit from the project but it was always his opinion that was strongest on this matter: until we can get time to sit down and talk about this in depth we're not going to unvendor anything.

The question is whether this change is superior to the status quo. For the moment, please assume unvendoring is off the table: sorry. 😞

[untitaker]

Since this patch already is used in Debian (I think?) and it has no effect in
environments without unbundled dependencies, I would say it doesn't make any
difference at all for existing packages.

Including this hack into requests prevents new unbundled packages from using
broken alternatives, OTOH including it might also encourage unbundling in the
first place.

On Wed, Sep 16, 2015 at 12:59:57AM -0700, Cory Benfield wrote:

So folks, the reality of the matter is that unbundling is not going to happen in the short term. Kenneth is currently stepped back a bit from the project but it was always his opinion that was strongest on this matter: until we can get time to sit down and talk about this in depth we're not going to unvendor anything.

The question is whether this change is superior to the status quo. For the moment, please assume unvendoring is off the table: sorry. 😞

---

Reply to this email directly or view it on GitHub:
#2567 (comment)

[untitaker]

Since this patch already is used in Debian (I think?) and it has no effect in
environments without unbundled dependencies, I would say it doesn't make any
difference at all for existing packages.

Including this hack into requests prevents new unbundled packages from using
broken alternatives, OTOH including it might also encourage unbundling in the
first place.

On Wed, Sep 16, 2015 at 12:59:57AM -0700, Cory Benfield wrote:

So folks, the reality of the matter is that unbundling is not going to happen in the short term. Kenneth is currently stepped back a bit from the project but it was always his opinion that was strongest on this matter: until we can get time to sit down and talk about this in depth we're not going to unvendor anything.

The question is whether this change is superior to the status quo. For the moment, please assume unvendoring is off the table: sorry. 😞

---

Reply to this email directly or view it on GitHub:
#2567 (comment)

[eriol]

@untitaker yes, it's included in Debian (and Ubuntu) since 2.7.0-3. It was uploaded on 11 Jun 2015. I did not receive complains related to it yet.

[eriol]

@untitaker yes, it's included in Debian (and Ubuntu) since 2.7.0-3. It was uploaded on 11 Jun 2015. I did not receive complains related to it yet.

[Lukasa]

So I'm getting the position that we think that, right now, we should merge this as-is, and look to unbundle at some future release.

[Lukasa]

So I'm getting the position that we think that, right now, we should merge this as-is, and look to unbundle at some future release.