-
-
Notifications
You must be signed in to change notification settings - Fork 9.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
Importing requests is fairly slow #3213
Comments
This is almost certainly the result of CFFI. Can you print what libraries you have installed in your environment ( |
This is not linked to cffi (I don't have it installed). I did the test w/ a quasi empty virtualenv:
|
I have a 2012 Macbook pro:, using a slightly different way of measuring the import time:
I also upgraded pip and setuptools to match your environment:
Further
Which is still not ideal, but is probably higher than the simple import of requests due to Python's initialization. |
That said, if you have information about specific things in our |
The main cost is importing urllib3, so I suspect that should be addressed there. |
@sigmavirus24 as @cournape mentions you can't time it like that; try using
So |
Neither of these numbers seem that high to me. |
Yeah. Especially as your average request is going to take a few hundred milliseconds or so. If you reeeeally want to shave off milliseconds, could always back-translate to use the stdlib. 😜 Or you could shorten the name of your command line tool by 1-2 characters. |
Ah, I didn't realize you were working on a CLI tool. Well, it hasn't seemed to be an issue for httpie. |
It is true that as soon as you do even one request on a remote server, the delay will not matter much. But the problem in python is that you pay the import cost whether you actually do a request or not: think doing 80 ms is pretty bad in this case: the rule of thumb for a CLI tool to have noticeable delay is ~100-150 ms. Try using e.g. git with a 200 ms delay, it is a bit annoying. Now, I completely understand that it may not be a focus of the library, especially if it is not that easy to improve. |
I personally think you're using the wrong tool for the job — given that, on the metrics given above, Python itself is taking 120ms alone. Httpie and mercuial are well-loved tools that do their jobs very well, but they are not, nor ever will they be, curl or git. |
Makes sense; you could code up some lazy loading bits. Found this though, which claims to have been excerpted from Apparent origin: https://selenic.com/hg/file/tip/mercurial/demandimport.py |
@kennethreitz python does not take 120 ms to start, unless you are on a seriously broken environment. It is much closer to 20 ms on decently modern hw (< 5 years), i.e. importing requests means 3x the cost of starting python. FWIW, on my 2011 Desktop PC (Debian):
On my 2014 macbook (OS X)
A simple |
Oh, oops, yeah...my environment is a bit wonky because of the |
haha, I thought that number looked high :) |
The "profimp" module shows where the time goes: https://pypi.python.org/pypi/profimp pkg_resources takes up a lot of time in general. Another time suck is the automatic loading of There doesn't appear to be any way of doing this today, if PyOpenSSL is importable. Would a PR allowing an opt-out be likely to be merged? |
@dsully How would you propose to expose the API for an opt-out? |
@Lukasa Unless I'm mistaken, the
Less of an opt-out, more of a not-needed. Now, if there are cases where the pyopenssl SSLContext wrapper is desired for some reason (?) even when the core libraries are sufficient, then I'll make another pass. What do you think? |
@dsully you said:
Where is Requests using pkg_resources? I don't believe either Requests or Urllib3 uses it.
If I remember correctly, only the more recent versions of 2.7 are actually appropriate. I think there were some minor issues with 2.7.9 and 2.7.10. |
Unfortunately, that's not true. The best example is on the system Python on macOS, which provides an SSLContext but which ships OpenSSL 0.9.8zh. This is an almost impossible to use version which is missing a number of vital features, including TLSv1.2 support. This is resolved by PyOpenSSL, which uses the OpenSSL 1.0.2 statically linked inside the cryptography module. This is vital to secure systems. More generally, using PyOpenSSL allows for users to link in a version of OpenSSL that is not constrained by the OpenSSL that their base Python is linked against. |
@sigmavirus24 Indirectly via
Imported via:
|
@Lukasa Right.. forgot about that. I don't have that particular issue on macOS, but most people do. I'll look at coming up with a way to explicitly opt-out then. |
It should be noted that the easiest way to opt-out is to simply not have PyOpenSSL in your environment. If it's present and you need it, you're presumably paying that import cost somewhere anyway. |
Also worth noting that |
@Lukasa - yes and no. Our build environment has real dependency management for modules (https://engineering.linkedin.com/blog/2016/08/introducing--py-gradle--an-open-source-python-plugin-for-gradle), which means just because someone included PyOpenSSL in their dependency tree, doesn't mean that the code you are importing for your upstream uses it. Lots of code gets installed transitively, but not imported. |
@reaperhulk BTW, is slow import of cryptography still an ongoing issue? |
@Lukasa - without moving the location of the pyopenssl loader, currently in |
I'm tentatively open to that, yes. |
That sounds like the real root cause of this. Well that and the possibility that cryptography is still slow to import. Which is an issue for pyca/cryptography.
I don't like that. Let's say someone is using HTTPie and notice it's slow. They're also using requests in a development project. If they export the environment variable to speed it up without understanding the ramifications and they're on an LTS distribution with a terrible |
@sigmavirus24 I hear you there. If environment changes aren't ok, an explicit call? I'd have to move the current injection, since it happens in |
@dsully so I think what we need to balance is: "better security for most people" versus "avoiding performance penalties for a smaller group of people". If I understand your situation correctly, you have recent enough versions of OpenSSL and Python, yes? In that case a version check against the |
Additional data here - if the PyOpenSSL (16.2.0) based SSLContext wrapper is loaded, and in requests Since this is getting far off the tract of this issue, I'll open another one to track what appears to be a bug. |
@sigmavirus24 I think so.. but I don't follow 100%. Do you mean checking the OPENSSL_VERSION to avoid the pyopenssl injection? |
@sigmavirus24 And yes, you are correct - we run Python 2.7.11 and Python 3.5 (soon to be 3.6), both compiled against a non-system shipped OpenSSL.$latest. Our build system (PyGradle) also sets build time |
@sigmavirus24 I think so.. but I don't follow 100%. Do you mean
checking the OPENSSL_VERSION to avoid the pyopenssl injection?
That's exactly what I failed to communicate. :)
…--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
Ok, so:
What is the minimum OpenSSL version for the required functionality? 1.0.1? |
On January 13, 2017 1:40:44 PM CST, Dan Sully ***@***.***> wrote:
Ok, so:
```
import ssl
if ssl.OPENSSL_VERSION_INFO < (1, 0, 1):
try:
from .packages.urllib3.contrib import pyopenssl
pyopenssl.inject_into_urllib3()
except ImportError:
pass
```
What is the minimum OpenSSL version for the required functionality?
1.0.1?
I think 1.0.1 is a good minimum but would rather defer to @Lukasa and @reaperhulk on that.
…--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
This is not a bug, unless the behaviour is inconsistent with the standard library's behaviour when you do that. Passing a custom bundle to verify should never append to the standard CA bundle, it should always replace it.
I am nervous about setting a minimum required OpenSSL version. We could definitely do it, I suppose, but it strikes me as something that is hard to come up with a universal "good" value for. |
@Lukasa - It should be set to whatever pyOpenSSL's minimum is for the functionality that is required. 0.9.8 was dropped in pyOpenSSL 16.1.0: https://pyopenssl.readthedocs.io/en/stable/changelog.html So 0.9.9 would be the bare minimum, but that's hard to recommend. I still say 1.0.1 or perhaps 1.0.0 |
The problem is that "required" is a moving target. We have a definition of "required" that is true today, but any check like this is going to go out of date. |
Well then it should be updated when the requirements change. :) |
Heh, I agree, but speaking as someone with about twice as many maintenance tasks as his job allows, I'm a big believer in reducing the number of things I have to keep track of. ;) |
PR #3817 created. |
[READY] Import the requests module lazily The requests module is slow to import. See https://github.com/kennethreitz/requests/issues/3213. We should lazy load it to improve startup time. We do that by adding two methods to the `BaseRequest` class: one that returns the requests module and another the session object since it depends on the `request-futures` module and `requests-futures` imports `requests`. In addition, we make sure that no requests are sent at startup otherwise there would be no point to lazy load these. These requests would fail anyway since the server can't be ready yet. Here are the improvements on startup time: <table> <tr> <th rowspan="2">Platform</th> <th colspan="2">First run (ms)</th> <th colspan="2">Subsequent runs (ms)</th> </tr> <tr> <td>Before</td> <td>After</td> <td>Before</td> <td>After</td> </tr> <tr> <td>Ubuntu 16.04 64-bit</td> <td>240</td> <td>131</td> <td>173</td> <td>74</td> </tr> <tr> <td>macOS 10.12</td> <td>435</td> <td>315</td> <td>261</td> <td>208</td> </tr> <tr> <td>Windows 10 64-bit</td> <td>894</td> <td>594</td> <td>359</td> <td>247</td> </tr> </table> *Results obtained by running the `prof.py` script from [this branch](https://github.com/micbou/YouCompleteMe/tree/profiling-startup). The difference between first run and subsequent runs is Python bytecode generation (`*.pyc` files).* <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2563) <!-- Reviewable:end -->
So the cryptography portion of this was fixed by @dsully in pyca/cryptography@aa396c0 which makes me believe that this can be closed. For me, on Fedora 25 and a 4th gen x1 carbon I get these results:
If this crops up again, let's open a new issue. |
We are using requests in our projects, and it is working great. Unfortunately, for our CLI tools, using requests is an issue because it is slow to import. E.g. on my 2014 macbook:
Takes close to 90 ms.
Is optimizing import time worth of consideration for the project ?
The text was updated successfully, but these errors were encountered: