Azure / msrest-for-python

The runtime library "msrest" for AutoRest generated Python clients.
MIT License
41 stars 64 forks source link

Delay load requests and oauthlib to improve module load performance f… #25

Closed johanste closed 7 years ago

johanste commented 7 years ago

…or msrest.exceptions. Fixes #23

codecov-io commented 7 years ago

Codecov Report

Merging #25 into master will decrease coverage by 0.03%. The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #25      +/-   ##
=========================================
- Coverage   71.53%   71.5%   -0.04%     
=========================================
  Files           9       9              
  Lines        1191    1193       +2     
=========================================
+ Hits          852     853       +1     
- Misses        339     340       +1
Impacted Files Coverage Δ
msrest/exceptions.py 59.09% <0%> (-1.52%) :arrow_down:
msrest/authentication.py 81.08% <100%> (ø) :arrow_up:
msrest/configuration.py 35.71% <100%> (ø) :arrow_up:
msrest/service_client.py 85.02% <100%> (+0.18%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cf7ec35...9959fb3. Read the comment docs.

brettcannon commented 7 years ago

Do realize that by doing this you MUST make sure you never instantiate any of these classes in a thread as a side-effect of importing, else you will deadlock (common problem in the os module which uses a similar check).

And if you are doing this for performance then you can always use https://docs.python.org/3/library/importlib.html#importlib.util.LazyLoader which will work transparently for all imports.

brettcannon commented 7 years ago

To clarify what I mean by deadlocks from importing, see https://bugs.python.org/issue1590864 (I don't know how much this affects Python 3 as we tweaked the import lock, but this can affect Python 2).

johanste commented 7 years ago

@brettcannon, please correct me if I'm wrong, but using LazyLoader is only an option for Python 3.5+, correct?

brettcannon commented 7 years ago

@johanste Depends on what you mean by "option" 😉 It's in the stdlib starting in Python 3.5, but if importlib2 is tracking upstream recently enough you could use that to use the lazy loader.

johanste commented 7 years ago

@brettcannon, as always, you teach me something new. It looks like importlib2 has not been officially released, so I would personally be a bit hesitant to use that...

brettcannon commented 7 years ago

@johanste what do you mean by "official"? importlib2 is as "official" as anything else on PyPI. 😉

Anyway, all I'm trying to say is what you're doing is basically fine, just if anyone ever reports some weird deadlocking problem then ask if they are using threads and if they are have them instantiate their classes ahead of time and/or don't launch a new thread as a side-effect of importing (and this might only be a Python 2 problem; haven't deal with this problem in years).

Or drop support up to Python 3.5. Whichever is easiest for you. 😁

johanste commented 7 years ago

I was referring to the following comments for the package:

Note: This project is not yet released and the code base may be unstable. Currently the importlib tests all pass, but a few things remain to be done before the initial release.

I'll take a look at using LazyLoader in the az cli since we have quite a few instances where we want to postpone loading of stuff as long as we can... but I'm afraid I cannot drop <3.5 support right now. :)

brettcannon commented 7 years ago

@johanste if you are serious about using importlib2 I can talk to the maintainer as he's a friend of mine.