use getattr to lazy load aiohttp and trio #15878
Conversation
ghost
added
the
| try: | ||
| from ._aiohttp import AioHttpTransport | ||
| if 'AioHttpTransport' not in __all__: | ||
| __all__.extend([ |
There was a problem hiding this comment.
__all__ is a pre-declaration of what exists, mostly to help IDE and doc to know what they would show. This means that I think this should be available even if the actualy import is not done. But putting this inside the __getattr__, it means dir is now not accurate:
I believe it should be possible to have __all__ correct, with a static list of strings outside of the __getattr__ but inside your if sys.version_info >= (3, 7), in order to get the regular tooling able to list the possible importable types.
There was a problem hiding this comment.
@xiangyan99 I see your latest commit, doesn't fix dir, so read the spec this time and not guessing and there is a __dir__:
https://www.python.org/dev/peps/pep-0562/#specification
We're close :)
There was a problem hiding this comment.
Something like:
if python > 3.7:
...
def __dir__():
return __all__
There was a problem hiding this comment.
You still get the perf improvement right? None of the previous commits breaks anything?
There was a problem hiding this comment.
Yes. We still get the perf improvement with the changes.
There was a problem hiding this comment.
@lmazuel I figured since I wrote a proof-of-concept of this 2.5 years ago with you in mind 😄 https://pypi.org/project/modutil/ (which also touches on __all__ and dir() as you are discovering 😁 ).
And to be clear about __all__, it exists to control from ... import * specifically, but has come to mean for some what the public API is.
There was a problem hiding this comment.
Yes, we (the azure client libraries) are part of the "assume __all__ means what the public API surface area is crowd". Primarily this is because that is what sphinx/autodoc, by default, uses it as well. So it's convenient.
@brettcannon, I'm not aware of any downsides of making this assumption - are there gotchas that we should look out for? I personally don't find the import * as a mechanism to import the "recommended"/most commonly used subset of the full public API surface area for a package/module to be a common (or good) practice.
There was a problem hiding this comment.
import * is most definitely not meant to convey "recommended/commonly used"; it is entirely to make it easier to play with things in the REPL and you can't make that judgement call for people as to what is common (and import * one of the things I would remove from the language if I could).
There was a problem hiding this comment.
Yeah, the "most commonly used" argument is probably the only argument that I had heard from people recommending the use of import *. But I don't think it is a common argument. The use in REPL is a much better argument, but I would agree that, given a time machine, I would still personally support a quick trip back in time and remove the support for it altogether.
|
/azp run python - storage - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - storage - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
* use getattr to lazy load aiohttp and trio
No description provided.