Add .pyi files to help static type checkers deal with lazy imports

[hoechenberger]

Fixes #12047

This is very much work in progress and I only added the few .pyi files I needed to get some immediate work done. I'm not sure if and when I'll have time to complete this for all (sub)modules. Anyone, pleae feel free to help me out here or take over – this PR is really just meant to serve as a demo and starting point. I tested with Pylance and it's working as expected.

FWIW I used the following script to generate the .pyi file content.

cc @drammock

# %%
submodules [...]
submod_attrs = {...}


output = []
for submodule in submodules:
    output.append(f"from . import {submodule} as {submodule}\n")

for mod_name, mod_contents in submod_attrs.items():
    if len(mod_contents) == 1:
        output.append(f"from .{mod_name} import {mod_contents[0]} as {mod_contents[0]}\n")
    else:
        output.append(f"from .{mod_name} import (")
        for c in mod_contents:
            output.append(f"    {c} as {c},")
        output.append(")\n")

print("\n".join(output))

[agramfort]

honestly to me this is too much. Suddenly we added a lot of extra work and complexity for what I consider a very marginal gain.

[cbrnr]

I tend to agree with @agramfort. The real culprit is lazy loading though. To me, a little slower import time is not worth the extra hacks. A typical analysis script runs dozens of seconds, if not much longer, so a few seconds spent on import is negligible.

[hoechenberger]

I agree too. I don't see any advantage for me and most users thanks to lazy imports. Whether my scripts take 1 or 2.5 seconds to start processing is completely irrelevant to my work. And for interactive use, the import is run once at the very beginning and that's it...

[hoechenberger]

In case @agramfort was not talking about lazy imports per se, but only about the changes in this PR:
We don't add additional complexity here, we're basically just moving the lazy import logic from one file to another, nothing more.

I don't like lazy imports. They're an obvious and ugly hack as long as they're not a core language feature ...

[hoechenberger]

To add to the broader discussion about lazy imports, here's the justification as to why PEP 690 – Lazy Imports was rejected. I believe it's an interesting read. The core reason is this:

... But a problem we deem significant when adding lazy imports as a language feature is that it becomes a split in the community over how imports work. A need to test code both ways in both traditional and lazy import setups arises. It creates a divergence between projects who expect and rely upon import time code execution and those who forbid it. It also introduces the possibility of unexpected import related exceptions occurring in code at the time of first use virtually anywhere. Such exceptions could bubble up from transitive dependency first use in unanticipated places. ...

[larsoner]

Indeed good points, I think these were discussed at some point during a dev meeting when we originally considered and made a go/no-go decision on #11780. Since we decided at that time to "go" I think we should stick with it a bit longer. If it gets (or continues to be) too onerous or broken for users or devs then we should eventually revert, but I don't think we've exhausted our options for fixing it yet.

[cbrnr]

Just to be clear, my comments in #11780 were rather skeptical once I realized that the actual implementation is not ideal (as opposed to the general idea of lazy loading, which is great). It seems like the prospect of adding MNE-Python to the list of SPEC supporters was the driving force behind this discussion/decision rather than the actual technical implications.

But OK with me to decide later. Maybe other people can share their thoughts as well here.

[hoechenberger]

I, too, can live with the changes started in this PR. The imports are redundant but at least the imported names are not strings anymore 😅 so that's at least something!

[cbrnr]

I think we should re-evaluate our decision to adopt lazy loading. You can read my general thoughts on the topic here, and here is why I think MNE-Python doesn't need that extra complexity. Before lazy loading, import mne takes about 190ms on average on my machine (tested with MNE 1.4.2). With lazy loading, import mne takes about 90ms (tested with main). Yes, this is more than twice as fast, but yes, we're only taking about an absolute difference of 100ms. This is completely negligible given that it happens only once, and a typical analysis pipeline will take dozens of seconds, if not minutes. And I'm not even factoring in the time in interactive mode where Python is idling, because it is waiting for user input.

[larsoner]

There were factors other than just speed taken into account and discussed when we made that decision

[cbrnr]

There were factors other than just speed taken into account and discussed when we made that decision

Would you mind reiterating them here? I could only find

1. Standardization
2. Un-nesting imports

But maybe more arguments have been discussed in a dev meeting (where I didn't participate).

BTW, I don't think (1) is a good reason because lazy loading should not be a standard (see my comment in the scientific-python repo). And I'd argue that nested imports are less complex (and less prone to unexpected behavior) than lazy loading.

[hoechenberger]

Funnily enough, even the engineer who originally developed the lazy loading implementation we're using is extremely skeptical of the entire approach now. It's causing headaches for developers downstream.

[larsoner]

Would you mind reiterating them here? I could only find

Yes those are the two major ones, and so far the un-nesting is a nice addition for contributors and maintainers alike I think.

is extremely skeptical of the entire approach now. It's causing headaches for developers downstream.

I'm cautiously optimistic that we've solved most of those headaches now, but we'll see! I agree with eventually reverting if stuff continues to pop up after @drammock's latest PR. At some point the cost is too high. But I'd prefer to wait some more months (maybe a year?) and see if our maintenance burden has net gone up and/or package usability has net gone down.

[drammock]

@cbrnr your big complaint seems to be with how the ecosystem presents lazy loading to the community. Given that MNE is a large complex package with many submodules, that complaint alone doesn't seem reason enough for us to abandon lazy loading, so I infer that you also have some other gripes... you say here that the speedup is too small to be worth it, but by now the work is already done so I think that point is now moot. Do you have any other substantive objections to it?

@hoechenberger I think your chief complaint was to do with Pylance, but that is now working for you again, right? Any other substantive complaints?

My personal take is that the "halfway" implementation that we did in #11838 was pretty bad (no code completion and an ugly / hard-to-grok implementation in our __init__ files), but that after switching to the .pyi approach in #12072 both of those problems went away. Notably the __init__.pyi files look like you would expect an __init__ file to look, the __init__.py files have only 2 boilerplate lines related to lazy loading, and (crucially) when adding new imports we don't need to change both files, only the .pyi file.

[cbrnr]

your big complaint seems to be with how the ecosystem presents lazy loading to the community

Yes.

Do you have any other substantive objections to it?

Yes, you are correct in assuming that I don't think it is a good idea. The reasons are mentioned in the rejection of the PEP. It's likely that new corner cases will be discovered. For example, there is a significant conflict between lazy loading and strict type checking discussed here. I also don't like rolling a custom import system with .pyi files and whatnot. Finally, I don't understand why we brush away all arguments made by the Python core developers by saying "nah, we'll just implement it and it'll work".

Anyway, it is probably more work to revert now than to wait and see what happens.

Edit: And yes, I agree that our __init__.pyi now look what you'd expect them to look like.

[hoechenberger]

Hi @drammock, I'm currently on the road and don't have much time for a more elaborate response. But over the course of the past couple of days I read up on lazy loading in Python (including the rejected PEP 690 and the related discussion) and I'm just not convinced this is something we should have unless it becomes part of the stdlib. It also appears that the approach we've adopted now will break downstream projects that use mypy for strict typing.

I'm not saying we should revert at this point, but my gut feeling is not a very positive one.

In any case I'd like acknowledge and to thank you for all the work you've put into this!

[drammock]

It also appears that the approach we've adopted now will break downstream projects that use mypy for strict typing.

If I'm correctly interpreting the discussion that @cbrnr linked to (this one from skimage) then what we've done will not break downstream projects that use mypy for strict typing --- that would happen only if we included a py.typed file (which we currently don't), and the problem described in that skimage PR that motivated adding py.typed doesn't seem to be happening for me with MNE (on current main in vscode using pylance, I see the full function signature & docstring in the popup).

[cbrnr]

That's also how I interpreted it @drammock. However, if we ever want to add type annotations to MNE, we will need py.typed.

[hoechenberger]

Yes, this is my concern. I had locally tested (a while back, before we moved to lazy importing) adding type annotations for a few classes and functions, which greatly improved usability in some cases. It's really something I'd like to see either being added to MNE upstream or, alternatively, to typeshed someday. But it would require adding py.typed.

So I'm a little bit uncertain about future implications here

[drammock]

So I'm a little bit uncertain about future implications here

please read the skimage thread more carefully. The "downstream problems" arise from adding py.typed and not adding (many) type annotations and using the current version of mypy. Mypy has already been patched, so the situtation you're describing is not expected to cause downstream problems as long as we wait until the next mypy release before we add a py.typed file to our repo.

[hoechenberger]

Thanks @drammock, I in fact didn't realize the mypy fix was already around the corner.