Skip to content
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

Open
cbrnr opened this issue Feb 20, 2025 · 6 comments
Open

Consider reverting lazy imports #13121

cbrnr opened this issue Feb 20, 2025 · 6 comments

Comments

@cbrnr
Copy link
Contributor

cbrnr commented Feb 20, 2025

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:

  1. Lazy loading is essentially a sophisticated workaround rather than a supported Python feature. The relevant PEP 690 was rejected, and the related discussion suggests it's unlikely to be officially adopted in the future. As a result, the mechanism is not guaranteed to work in all cases and has already shown instances of breaking.
  2. The package relies on .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.
  3. Lazy loading can be useful for CLI tools or packages with many submodules, where startup time is a major concern. However, I think the advantages for MNE-Python are questionable:
    • The import time wasn't particularly long to begin with.
    • While a 50% reduction in relative terms sounds significant, the absolute gain is just 175 milliseconds, which is negligible.
    • We already used nested imports for optional dependencies, and nested imports can also be used to import packages on demand without the need for the non-standard lazy-loader mechanism.
    • Delaying imports until first use might be more disruptive than a slightly longer initial import time.
  4. The 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.

@agramfort
Copy link
Member

agramfort commented Feb 21, 2025 via email

@drammock
Copy link
Member

This comment has the relevant backstory links that @cbrnr forgot to include:

#12388 (comment)

we started with #11838 (but did it a dumb way, without .pyi files). This caused problems with IDE completion hints etc, as discussed in #12059 (that's where all the opinions are aired). The IDE problem was fixed by #12072.

@cbrnr
Copy link
Contributor Author

cbrnr commented Feb 21, 2025

It's not the entire backstory, just some aspect. I think I summarized all important points in my initial post.

@drammock
Copy link
Member

It's not the entire backstory, just some aspect.

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 think I summarized all important points in my initial post.

I disagree. Past discussions / decisions are relevant and important context. I'm surprised that you seem to be saying that you omitted them intentionally.

@cbrnr
Copy link
Contributor Author

cbrnr commented Feb 21, 2025

Of course I'm not saying I omitted something intentionally, come on!

@drammock
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants