Revert lazy loading (importing) packages

[cbrnr]

Fixes #13121.

[cbrnr]

@drammock I'm not sure how to remove a core dependency, since the hook generates the list from the latest stable release.

Other than that, I tried to minimize the changes to main. We could think about not nesting the SciPy imports, since this only takes around 50ms or so, and it would make the code a bit easier to read.

I don't know how to handle sklearn, I forget the mechanism that we use to skip the import if the package is missing. Also, how do we handle classes that require sklearn (e.g. CSP)?

[larsoner]

For the hook I think it's easiest probably just to add an exception to the hook code for now. We've hit that problem before and I think that's how we worked around it.

For sklearn we have a harder decision. Without lazy loading mne.decoding we basically have two choices:

1. Require people to import mne.decoding before accessing anything in that namespace. So you could no longer do:

   import mne
   mne.decidng.CSP(...)
   

   You would need to add a import mne.decoding instead (or from mne.decoding import CSP etc.). A lot of other packages are this way, such as sklearn (and SciPy was at some point, might still be).
2. Make sklearn a hard requirement of MNE.

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 mne.decoding to properly subclass sklearn classes (e.g., TransformerMixin, MetaEstimatorMixin, etc.). Before lazy loading we had our own duplicated, half-baked, partially incorrect variants of these subclasses, which we definitely should not go back to. It's a big maintenance burden and constantly breaking in addition to being a bad violation of DRY, not working with sklearn class validation tools, etc.

[cbrnr]

For the hook I think it's easiest probably just to add an exception to the hook code for now. We've hit that problem before and I think that's how we worked around it.

Thanks, I'll try that.

Regarding sklearn, I'd say that adding it as a core dependency would automatically also solve the subclassing issue. Given that sklearn is such a widely used package, I'd vote for 2 here.

[drammock]

While I appreciate your initiative in taking this on @cbrnr, I would have preferred a bit longer discussion period before embarking on a change as large as this one. Notably, nobody has yet offered the arguments in favor of keeping lazy_loader, so although there are 3 other maintainers (so far) concurring with your preference to remove it, I don't really think the discussion has had a chance to reach consensus.

In addition, according to our governance, it is the reponsibility of both the maintainer team and the steering council to work in partnership on any "large-scale changes to the software", which includes changes that affect multiple submodules. So to give the steering council time to discuss this, I'm marking this with "request changes" to block merging until that discussion has occurred. Next steering council meeting is 07 March.

[cbrnr]

That's why this PR is a draft, I thought it would be a good idea to give people an idea of the required changes.

(And just as a side note, I don't remember any longer discussion when lazy loading was introduced. Don't get me wrong, I'm not opposed to a longer discussion at all, just noting that such decisions seem to be a bit arbitrary, at least in my perception.)

[cbrnr]

For the hook I think it's easiest probably just to add an exception to the hook code for now. We've hit that problem before and I think that's how we worked around it.

@larsoner I'm not sure what to do here. I looked at the history of that hook (and the pre-commit config file), but I didn't find anything related to ignoring a change in dependencies.

[cbrnr]

BTW, mne.preprocessing.xdawn also requires sklearn.base.TransformerMixin.

[cbrnr]

If anyone wants to try the current state of this PR, import mne should now work if sklearn is installed.

[cbrnr]

I think I nested all scipy imports, but this didn't influence import time at all (maybe I forgot something).

[cbrnr]

Beyond that I'm not sure how we'd get classes in mne.decoding to properly subclass sklearn classes (e.g., TransformerMixin, MetaEstimatorMixin, etc.). Before lazy loading we had our own duplicated, half-baked, partially incorrect variants of these subclasses, which we definitely should not go back to. It's a big maintenance burden and constantly breaking in addition to being a bad violation of DRY, not working with sklearn class validation tools, etc.

@larsoner I would just put it into a try/except block (implemented like this currently).

I know that there is no consensus on this change at all, but I don't understand the test_import_nesting test, which currently fails. Regardless of whether or not this will go in, I'd appreciate someone explaining it to me. For example, why is it currently complaining:

E   AssertionError: 25 nesting errors:
E   Line 10:   ("mne/datasets/__init__.py", "from . import fieldtrip_cmc", "non-explicit relative import"),
E   Line 11:   ("mne/datasets/__init__.py", "from . import brainstorm", "non-explicit relative import"),
E   Line 12:   ("mne/datasets/__init__.py", "from . import visual_92_categories", "non-explicit relative import"),
E   Line 13:   ("mne/datasets/__init__.py", "from . import kiloword", "non-explicit relative import"),
E   Line 14:   ("mne/datasets/__init__.py", "from . import eegbci", "non-explicit relative import"),
E   Line 15:   ("mne/datasets/__init__.py", "from . import hf_sef", "non-explicit relative import"),
E   Line 16:   ("mne/datasets/__init__.py", "from . import misc", "non-explicit relative import"),
E   Line 17:   ("mne/datasets/__init__.py", "from . import mtrf", "non-explicit relative import"),
E   Line 18:   ("mne/datasets/__init__.py", "from . import sample", "non-explicit relative import"),
E   Line 19:   ("mne/datasets/__init__.py", "from . import somato", "non-explicit relative import"),
E   Line 20:   ("mne/datasets/__init__.py", "from . import multimodal", "non-explicit relative import"),
E   Line 21:   ("mne/datasets/__init__.py", "from . import fnirs_motor", "non-explicit relative import"),
E   Line 22:   ("mne/datasets/__init__.py", "from . import opm", "non-explicit relative import"),
E   Line 23:   ("mne/datasets/__init__.py", "from . import spm_face", "non-explicit relative import"),
E   Line 24:   ("mne/datasets/__init__.py", "from . import testing", "non-explicit relative import"),
E   Line 25:   ("mne/datasets/__init__.py", "from . import _fake", "non-explicit relative import"),
E   Line 26:   ("mne/datasets/__init__.py", "from . import phantom_4dbti", "non-explicit relative import"),
E   Line 27:   ("mne/datasets/__init__.py", "from . import sleep_physionet", "non-explicit relative import"),
E   Line 28:   ("mne/datasets/__init__.py", "from . import limo", "non-explicit relative import"),
E   Line 29:   ("mne/datasets/__init__.py", "from . import refmeg_noise", "non-explicit relative import"),
E   Line 30:   ("mne/datasets/__init__.py", "from . import ssvep", "non-explicit relative import"),
E   Line 31:   ("mne/datasets/__init__.py", "from . import erp_core", "non-explicit relative import"),
E   Line 32:   ("mne/datasets/__init__.py", "from . import epilepsy_ecog", "non-explicit relative import"),
E   Line 33:   ("mne/datasets/__init__.py", "from . import eyelink", "non-explicit relative import"),
E   Line 34:   ("mne/datasets/__init__.py", "from . import ucl_opm_auditory", "non-explicit relative import"),

But removing those imports breaks stuff.

[larsoner]

The test probably needs to be adjusted for this case. It's meant to catch importing classes and functions (which we always want to make explicit like from .<something> import <thing>, but in this case it's catching importing a submodule which should be fine.