-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Consider reverting lazy imports #13121
Comments
I lean on the same side here...
… Message ID: ***@***.***>
|
It's not the entire backstory, just some aspect. I think I summarized all important points in my initial post. |
Sure. I wasn't trying to imply that your description wasn't relevant. Just that there are some other things that are also relevant that you didn't include.
I disagree. Past discussions / decisions are relevant and important context. I'm surprised that you seem to be saying that you omitted them intentionally. |
Of course I'm not saying I omitted something intentionally, come on! |
I guess I misunderstood. Saying "I think I summarized all important points in my initial post" implied that you didn't think linking to past discussions was important, so you chose not to do it. Which, again, was surprising to me, especially since it's a frequent request when old decisions get revisited --- I assumed you'd have thought of doing so. |
I agree with @cbrnr I don't see much value in |
I find this concerning and I never liked the fact that we now critically depend upon a hack that didn't pass standardization (via a PEP) – for IMHO very good reasons. If forced to vote, I'd decide against the current lazy loader implementation. That said, so far I'm not running into problems (anymore) so I actually don't mind (for now). |
FWIW I just briefly checked some of the projects that are listed as having "endorsed" SPEC-0001: NumPy, SciPy, xarray. In none of the above could I find traces of the |
I still like the lazy loading stuff but can live with the majority decision here. I guess we'll see what problems/harder other decisions come out in #13124 -- one we've already hit that people should hopefully discuss is how to handle the |
FWIW just looking into one, SciPy uses a lazy loading approach even if it's not via |
Maybe they have similar issues with the package like the ones I raised here? |
Could be, or could be that nobody has taken the time to switch over. Someone would have to look into it and investigate further.
One point missed here I think is that anything we change has consequences for any dowstream package that does
One more point of data here... the original PR that added it had timings that went from ~500 ms to ~160 ms (#2001). Looking a bit now: $ time python -c "import mne"
real 0m0.315s
$ time python -c "import mne; import scipy.linalg"
real 0m0.433s
$ time python -c "import mne; import scipy.linalg; from sklearn.base import BaseEstimator"
real 0m0.943s As someone who uses MNE in other packages that I launch from the CLI a lot (i.e., psychophysics experiments), having it creep up to half a second (or a second!) to |
No objection against nesting the slow imports as we previously did (including |
sklearn isn't as simple, copying from #13124 (comment) so we can discuss in one place : For
I guess I'd vote for (1). It's a bit weird if we do it just for one submodule, but I guess it's justified because it's the only one that requires an optional dependency for its class hierarchy. Beyond that I'm not sure how we'd get classes in |
+100 |
Well, the ship may have already sailed, but I'd like to take the time to enumerate some arguments on both sides of this issue. in favor of keeping lazy loader
against keeping lazy loader(I've kept these in the same order as @cbrnr's original post
response to arguments against keeping it
I can't fathom why this should hold any weight. Lots of things will never make it into the standard library.
This is the only argument I find somewhat convincing. It seems unlikely, but it is possible that Python will someday set stricter rules about
See point (1) in favor of keeping lazy_loader.
Maybe. I'd say it's a bit too early to tell. In scientific-python/lazy-loader#128 Stéfan said "ping me if you don't get it sorted out" and he was never pinged again, so I feel it's unfair to count that as evidence for lack of maintenance. There's also a bit of a built-in backstop, in that |
I think the ideal solution would be to not import everything into the global
I don't want to reiterate my points, they haven't changed, but I do want a consensus solution and I am definitely not forcing anything onto the project. If anyone sees any other options, please let us know! Maybe there is a way to keep the current lazy loader, but get rid of the |
Just one quick response to the point on CI runs: if we switch from
So about 62x speedup, which I think makes arguing about 200ms longer import times a moot point. |
Marginally (?) related but has anyone considered that the metrics we're using here are not really useful anyway? Just comparing |
To me this is a non-starter. Interactive API exploration is something I use a lot with both MNE (where sometimes I forget the exact name of a func, so I
It's true that the import "savings" doesn't just disappear; if you use (side note: this will become a much bigger deal when #13059 lands, because TL;DR: comparing the |
I don't use it at all, I guess we will not be able to come up with a statistic on how many users require that feature or not. In addition, you can still interactively explore if you first |
which is even slower than importing everything eagerly in the first place.
which breaks interactive API exploration. How is that "sufficient" exactly? |
I would like to discuss reverting the changes that introduced the
lazy-loader
package. I have been unhappy with this change basically almost from the start (although I made a positive comment in the initial discussion, but this was before I fully understood how lazy loading was implemented), and I think it is not good for the project's health in the long run for the following reasons:.pyi
files for runtime behavior, which contradicts their intended purpose. Official references, such as PEP 561 and PEP 484, explicitly state that stub files are strictly meant for static type checking and not execution.lazy-loader
mechanism.lazy-loader
package does not appear to be very actively maintained beyond some tooling updates, and there are several unresolved issues such as all packages being loaded at once (issue #131) and eager imports not working as expected (issue #128).Given these concerns, I strongly suggest that we at least reconsider our decision to adopt the
lazy-loader
package. Since this will be a rather large change involving many files and probably a lot of manual work, I'm happy to submit a PR if there is general agreement that this is the right direction. It is also not super urgent, but I think the sooner we address this, the easier it will be to revert the changes.The text was updated successfully, but these errors were encountered: