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

MAINT: Endorse SPECs #11780

Closed
larsoner opened this issue Jul 6, 2023 · 14 comments
Closed

MAINT: Endorse SPECs #11780

larsoner opened this issue Jul 6, 2023 · 14 comments
Assignees
Milestone

Comments

@larsoner
Copy link
Member

larsoner commented Jul 6, 2023

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.

  • SPEC0: The minimum version one I haven't ready but I assume is close to what we do anyway, and if that's the case we should endorse/officially adopt it
  • SPEC4: Nightly wheels I guess we could do but we're pure Python so seems unnecessary.

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?

@larsoner larsoner added this to the 1.5 milestone Jul 6, 2023
@cbrnr
Copy link
Contributor

cbrnr commented Jul 6, 2023

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
Copy link
Member Author

larsoner commented Jul 6, 2023

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

#11565 (comment)

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
Copy link
Member

drammock commented Jul 6, 2023

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
Copy link

tupui commented Jul 6, 2023

You got that perfectly 😃 Happy to read that 🚀

@cbrnr
Copy link
Contributor

cbrnr commented Jul 6, 2023

@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
Copy link
Member Author

larsoner commented Jul 6, 2023

@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
Copy link
Contributor

cbrnr commented Jul 6, 2023

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
Copy link

stefanv commented Jul 6, 2023

@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
Copy link
Member Author

larsoner commented Jul 6, 2023

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
Copy link
Member

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
Copy link

tupui commented Jul 6, 2023

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

@larsoner larsoner modified the milestones: 1.5, 1.6 Aug 14, 2023
@drammock
Copy link
Member

drammock commented Oct 1, 2023

@larsoner I had forgotten that only "core" projects can endorse SPECs, but I've opened scientific-python/specs#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
Copy link
Member Author

larsoner commented Oct 1, 2023

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

@larsoner larsoner closed this as completed Oct 1, 2023
@stefanv
Copy link

stefanv commented Oct 2, 2023

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

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

6 participants