mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.66k stars 1.31k forks source link

MAINT: Endorse SPECs #11780

Closed larsoner closed 11 months ago

larsoner commented 1 year ago

cc @drammock there is https://scientific-python.org/specs/ , I assume you have thoughts/ideas/knowledge from the conference but wanted to get it on our radar.

The interesting one is that might deserve some discussion is SPEC1 lazy loading. The lazy_load package idea seems great. We could maybe get rid of all of our nested imports except maybe matplotlib, and still have the nested import test pass. And import mne would be faster because @verbose creating functions does have a non-zero overhead, which I think accounts for most of this:

$ time python -c "import numpy, scipy"
real    0m0.123s
$ time python -c "import mne"
real    0m0.464s

But this would be another package to add to MNE-Python's dependency list (which has a high adoption cost as discussed before) or we'd have to vendor it, which would make the conda-forge people unhappy. Or we'd have to do some hybrid where we try: import lazy_loader; except Exception: <import our vendored one>.

@agramfort would vendoring a BSD module be problematic from the industry-adoption end?

cbrnr commented 1 year ago

Great idea! SPEC0 seems doable without much effort (in fact, if we drop support as per this spec, we will actually have less maintenance effort). I also like lazy loading, which we do anyway. For me, adding an additional dependency is worth the benefit of a 4x speedup. Although I'm not sure what the high adoption cost you're referring to means.

larsoner commented 1 year ago

I'm not sure what the high adoption cost you're referring to means.

https://github.com/mne-tools/mne-python/pull/11565#issuecomment-1474453591

Hence my question to @agramfort about whether a vendored (BSD-compatible of course) dependency is a workaround. It seems like it should be since we take/adapt BSD-compatible code all the time in releases...

drammock commented 1 year ago

Yes, this is on my radar. FYI the idea behind endorsing SPECs is (IIRC) something like "numpy endorses SPEC0 and their implementation of it is described in NEP29". That particular example gets the chronology backwards (NEP29 came before SPEC0) but the idea is that projects can endorse-and-still-deviate if they have good reason to do so, and they should document on their own sites their endorsement/implementation and any deviations. For example, we might deviate if we needed to retain support for an older Python because, e.g., Freesurfer needed it or something.

cc @stefanv and @tupui in case I got any of the details wrong here

tupui commented 1 year ago

You got that perfectly 😃 Happy to read that 🚀

cbrnr commented 1 year ago

@larsoner are you talking about nibabel? I thought you were talking about the lazy-loader package. The question is if we really need it, because it is yet another layer that makes things more complicated (for devs). Maybe it is sufficient to import (large) modules inside function, like we already do.

larsoner commented 1 year ago

@larsoner are you talking about nibabel? I thought you were talking about the lazy-loader package.

I linked to a thread where the discussion was originally about nibabel, but it's generally applicable to adding any new dependency, including the lazy_loader package that we're discussing here.

The question is if we really need it, because it is yet another layer that makes things more complicated (for devs).

I think in the end it might actually make it less complicated. Basically I think we can change our __init__.pys to use lazy_loader (or whatever), which is a tiny complexity increase for us. But it has three advantages:

  1. SPEC4/ecosystem compliance
  2. Reduced import mne time
  3. No longer need to nest imports to stuff like scipy.linalg, etc. to keep import speed down

I'd expect point (3) to improve/simplify our code/contribution/maintenance requirements more than the __init__.py changes will worsen them.

cbrnr commented 1 year ago

I linked to a thread where the discussion was originally about nibabel, but it's generally applicable to adding any new dependency, including the lazy_loader package that we're discussing here.

I agree, but pure-Python lightweight packages with zero dependencies like lazy_loader shouldn't be a huge burden.

Re using lazy_loader vs. our current nested imports, we could try and see what it looks like. Your points 1 and 2 can be achieved without the package, just to be clear. Re point 3, if scipy already implements lazy import, then we can just import it without using lazy_loader ourselves, no?

stefanv commented 1 year ago

@cbnr Just weighing in here as lazy_loader author: I think it's perfectly fine to use built in mechanisms (like we did for SciPy). The advantage of lazy_loader is that you can define imports as pyi files (so that code type completion still works in editors, which it won't otherwise). We have an environment variable for disabling lazy loading that's useful in testing. And we have informative deferred import errors.

But, none of that is needed; we're just trying to make it easy for libraries to switch to a lazy import strategy, since we think it's a good idea :) In particular: faster imports for users, and the ability to expose submodules without cost so that libraries can be explored interactively in, e.g. IPython.

Whatever route you choose, I'd be happy to review the changes for common gotchas!

larsoner commented 1 year ago

I agree, but pure-Python lightweight packages with zero dependencies like lazy_loader shouldn't be a huge burden.

I don't think that was the consensus/conclusion we came to in #11565 EDIT: #11564. I'll ask @agramfort about vendoring over there as well, so let's move discussion about adding additional dependencies there.

But, none of that is needed; we're just trying to make it easy for libraries to switch to a lazy import strategy, since we think it's a good idea :)

Indeed to me the standardization is nice. Similar to how everyone could have used try/except easily for Python 2-3 transition but six just made things easier and more standard for everyone.

agramfort commented 1 year ago

no strong feeling on my end. is it likely that numpy / scipy / matplotlib adopt lazy_loader so we can just rely on the same set of dependencies ?

tupui commented 1 year ago

SciPy being SciPy, don't really count on it I am afraid 😅 we have a tendency to want to do our own magic 🪄

drammock commented 11 months ago

@larsoner I had forgotten that only "core" projects can endorse SPECs, but I've opened https://github.com/scientific-python/specs/pull/271 to add us to the list of "adopters" of SPEC001 (lazy loading). Depending on how they respond to that one I'll look deeper into what we should do re: SPEC000 and 004

larsoner commented 11 months ago

Okay I think that's probably good enough to close the issue at our end for now, then!

stefanv commented 11 months ago

Just to emphasize one point from above: unless lazy loading happens in .pyi files, type checkers won't know about those functions. See:

https://scientific-python.org/specs/spec-0001/#type-checkers