PEP 690: Lazy Imports #2569
PEP 690: Lazy Imports #2569
Conversation
|
cc @warsaw |
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
|
Doc builds are still failing. |
Co-authored-by: Carl Meyer <carl@oddbird.net>
Co-authored-by: Carl Meyer <carl@oddbird.net>
Co-authored-by: Carl Meyer <carl@oddbird.net>
pep-9999.rst
Outdated
| .. code-block py | ||
| from importlib import set_eager_imports | ||
|
|
||
| set_eager_imports(["one.mod", "another"]) |
There was a problem hiding this comment.
What about a context manager in importlib so that you could do something like:
with importlib.eager():
# all things inside this with block will be opted out of lazy imports
There was a problem hiding this comment.
I like this idea. @Kronuz was concerned that there might be confusion about whether the effect of with importlib.eager(): is shallow or deep. I.e. does it make all imports eager that occur recursively "within" that block, or just the imports directly in this module? I prefer shallow, because it's less action-at-a-distance; I'd rather know when reading a module whether the imports in it will be shallow or lazy (presuming lazy imports is enabled at all.) I think if we don't suggest the possibility of "deep" effect then the shallow version should be fairly intuitive...
There was a problem hiding this comment.
If the effect is shallow, the context manager can literally be a no-op, since as discussed elsewhere in the PEP, context managers automatically make imports within them eager, for exception-handling reasons.
There was a problem hiding this comment.
I was thinking it would enable deep eager imports because it's hard otherwise to enforce that, but it could be controlled by a flag, e.g.:
with importlib.eager(deep=False)
...
with importlib.eager(deep=True)
...Or shallow as the flag name instead of deep. Either way, maybe shallow eagerness should be the default?
There was a problem hiding this comment.
If the effect is shallow, the context manager can literally be a no-op, since as discussed elsewhere in the PEP, context managers automatically make imports within them eager, for exception-handling reasons.
Oh yeah, right!
There was a problem hiding this comment.
I think that the importlib.set_eager_imports() API currently proposed in the PEP serves the same use case as with importlib.eager(deep=True) does. But I think set_eager_imports() is a more predictable and less error-prone (though arguably less aesthetic) way to do it, since it ensures that certain selected modules will always use only eager imports, regardless of where they are imported from, whereas if you use the deep context manager, you never know whether some other module might first import the same thing without the deep-eager context, so you might be confused why that thing still gets imported with laziness initially..
So my inclination would be to provide importlib.set_eager_imports() but not provide a deep context manager.
There was a problem hiding this comment.
I can see that. The one thing that maybe bothers me is that set_eager_imports() doesn't say to me either deep or shallow imports, and it implies shallow imports because you have to explicitly name some module paths. The other question is whether it's possible to unset eager imports you've specified in this call (or whether you even need to). With the context manager at least, you understand the limited effective scope of the eagerness.
It's totally fine to be opinionated in the PEP and I support either interpretation. (It will likely get bikeshedded anyway 😄 .) The PEP should include a discussion of the options though, perhaps in the rejected ideas section.
pep-9999.rst
Outdated
| foo.Bar() | ||
|
|
||
| In this case, with lazy imports enabled, the import of ``foo`` will not | ||
| actually occur while the addition to ``sys.path`` is present. |
There was a problem hiding this comment.
It's probably important to discuss, as we chatted about, whether it makes sense to separate finder from loader execution. IOW, I believe this PEP is proposing that neither finder nor loader executes when the import statement is lazy. It could run the finder and cache those results, but defer loader execution until eager time. I'm not saying it should, but the PEP should be explicit about this and perhaps discuss it.
There was a problem hiding this comment.
Yep, this is currently discussed below under rejected ideas.
pep-9999.rst
Outdated
| import side effects is the registry pattern, where population of some | ||
| external registry happens implicitly during the importing of modules, often | ||
| via decorators. Instead, the registry should be built via an explicit call | ||
| that perhaps does a discovery process to find decorated functions or classes. |
There was a problem hiding this comment.
Does this break the click use case? The PEP should at least discuss this topic, as CLIs are the primary motivator and click is a very common CLI library.
There was a problem hiding this comment.
Yes, if click subcommands are created in multiple modules, those modules need to be forced to eager import in order to get the click subcommands registered. Will note this specifically.
pep-9999.rst
Outdated
|
|
||
| * Don't use inline imports, unless absolutely necessary. Import cycles should | ||
| no longer be a problem with lazy imports enabled, so there’s no need to add | ||
| complexity or more opcodes in a potentially hot path. |
There was a problem hiding this comment.
Are there any effects on namespace packages?
|
You need to resort your CODEOWNERS entry. A |
|
Also, you probably need to add yourself to
A |
There was a problem hiding this comment.
None of your code blocks are rendering, the proposed fixes them.
You can use https://pep-previews--2569.org.readthedocs.build/pep-0690/ as a live preview.
A
|
@CAM-Gerlach I marked your code-block suggestions as resolved as the A |
|
I prefer it as well, I just didn't want to overcomplicate things for authors and lead to a greater chance of a syntax error as we saw here, yours didn't show up for me inline on the Files pane while I was reviewing. |
|
Just a quick note about the things we chatted about. Update the PEP to discuss:
|
|
@warsaw I think the latest version addresses all your comments! @CAM-Gerlach Might be easier at this point to get it merged, if it looks good enough - a copy-editing pass as separate PR after land would be very welcome! |
It also means that @Kronuz would no longer be considered a "new contributor", and we wouldn't keep having to click the button to run CI! A |
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Agreed! |
|
@CAM-Gerlach I'll leave the final merge to you. |
There was a problem hiding this comment.
LGTM, thanks! Make sure to post the PEP's discussion thread in the Discourse PEPs category (per @warsaw 's recommendation), link the thread in the Discussions-To header and the Post-History (I can help get that started if you like), and an announcement linking to the PEP and thread on Python-Dev. Thanks!
|
Hey @ewdurbin , when I merged this, the site was rebuilt and redeployed and the new PEP is visible in PEP 0, but when I actually visit the PEP page (as linked there), I get a GitHub Pages 404, even if I force (shift) reload clearing cache, as of the time this comment was posted. I'm guessing maybe Fastly caching is caching the 404 page? What's the timeout, and is there any way to avoid that, since it means for some time after new PEPs are published, the links are visible and indexable, but the PEPs themselves are not, which is not ideal.
Or is something else going on? |
|
https://peps.python.org/pep-0690/ is loading fine for me. |
|
I tested in a private window, @JelleZijlstra tested on his computer and @FFY00 tested on his phone, all on the PyCon conference WiFi, and it all resolves to a 404, but when I tested on my phone on LTE, it works. So it seems its some kind of caching layer specific to the conference internet (and not a regional/PoP thing on the Fastly/GHP side). I also loaded it myself just before it actually deployed, which probably triggered the caching; if I'd waited 60 seconds, it likely wouldn't have cached it. So the page is live for everyone but us at PyCon, thanks to my cache poisoning 😆 |
|
Obviously late to the party, but I would love to see the headers from the responses to determine if it was indeed a caching proxy run by the convention center or a Fastly POP thing. |
|
I guess the interesting headers are now gone for that page, but we can repro by looking at any other 404 page? For example, there's Fastly cache headers on https://peps.python.org/pep-9991/ |
|
I want to see the headers on the responses inside the convention center specifically. |
|
Unfortunately I don't think any of us saved the headers. Maybe we'll see it again with another PEP at next year's PyCon :) |
|
Unless someone wants to volunteer to attend the 2022 Climbing Wall Association Summit or 2022 Society of Gastroenterology Nurses and Associates Annual Course? :) |
Since it worked on our phones at the same time and location, would Fastly expose a different POP to mobile network operators vs. the conference's ISP? If not, then it would seem to be a caching layer between the ISP and the local WiFi router, but if so, we should be able to reproduce it next time we add a PEP...
The critical aspect, I think, was me poisoning the cache just before the PEP went live 😆 |
This is the PEP proposal for Lazy Imports in Python