MAINT: Make nibabel a required dependency

[larsoner]

To me there are a few criteria for adding a new core dep. Ideally (but not always) it would at least:

1. Have an established history/structure
2. Be well maintained (with an expectation of future maintenance)
3. Demonstrate low maintenance burden, ideally as MNE-Python as an optional dep (but could be elsewhere)
4. Be used in a lot of places or at least a few important places in our codebase
5. Be pure-python (to ensure installability everywhere and help ensure that things don't break often)
6. Have a slow change/deprecation/change policy
7. Have limited installation requirements itself

To me nibabel has all of these features. Adding it as a core dependency would simplify a lot of our codebase/shims (no more _import_nibabel, etc.) and reduce Python ecosystem code duplication (really FreeSurfer surface reading should be implemented one place!). So overall I think we should add it as a core dependency.

Any thoughts on this?

FWIW this is spurred by the recent change I made to remove our (duplicated with nibabel) FreeSurfer surface reading code in #11557. It didn't trigger CI failure there for some reason, but now it causes 117 test failures because we read a lot of surfaces!

[cbrnr]

All of your points make sense. However, would it also solve these problems when we add it as an extra dependency? We could create a new extra group in setup.py if it doesn't fit into an existing group.

[larsoner]

However, would it also solve these problems when we add it as an extra dependency?

We could do this but I don't see much advantage given the low maintenance burden for us. I do see having to do the extra bit of pip install mne[something] for every user who wants to read surfaces as a big disadvantage, though. It complicates our install instructions, gives them more to know/understand/get wrong, etc.

[cbrnr]

I always appreciated that pip install mne pulls in only absolutely mandatory dependencies. Sure you can argue that nibabel is small, pure Python, etc. etc., but it still misses the point that it is only needed for specific use cases. Many EEG users will never work with surfaces.

That said, if it makes our life easier I'm fine with adding it even though it doesn't really fit into my mental model of MNE 😄 (maybe my model is wrong).

[larsoner]

I always appreciated that pip install mne pulls in only absolutely mandatory dependencies.

Currently it pulls in jinja2, which I don't think fits this absolute mandatory mental model. We could make this optional, it would just require more shims and "junk" in our codebase, user knowledge for install, etc. To me what/where we use nibabel is more important/prolific than where we use jinja2, but I think it would be a step backward for us to remove jinja2 as a required dep. So as far as the mental model goes, I think you're assigning more weight to the absolute-mandatory aspects (and what qualifies as this) than me at least.

To me the biggest weight (by far) is burden on maintainers and and users as a whole -- if adding a new required dependency is expected to make our life as maintainers and users lives easier on the whole, we should do it.

[cbrnr]

Sure, I just wanted to extrapolate and caution that we don't want to end up in a situation where we suddenly have 100 required dependencies, because they make our life (as both users and maintainers) easier. This can surely be argued, but the downside is of course that every dependency introduces a huge maintenance burden. I know that this is why you have listed these criteria for a package to qualify, and I already said I'm OK either way.

BTW, jinja2 is already a required core dependency.

[drammock]

absolutely mandatory dependencies

Similar to my comment in #11544 (comment) about defining "MNE should work", here we have the same issue of defining what is "absolutely mandatory". For anyone whose work involves working with Freesurfer surfaces (which is a lot of MNE users) being able to read Freesurfer surfaces is mandatory.

Right now lots of things are optional only because we chose to arrange our imports that way and the motivation that @agramfort usually points to is that minimal install should be lightweight for running MNE on servers where no interactive stuff happens. To me, that's a good concern to keep in mind but it's not the only concern (@larsoner's hit the major ones for me: simplifying / removing our import shims, and simplifying our install instructions and the install process for lots of users).

[cbrnr]

Exactly @drammock! I think of PsychoPy, which has always had the opposite problem – they basically make every package mandatory, which means dozens of dependencies. It will work for every use case, but it requires to maintain a huge dependency chain (hell).

[drammock]

it sounds like the three of us are on the same page(ish), or at least OK with adding nibabel as a core dep. Let's give it some time to see if anyone else wants to weigh in.

[larsoner]

it sounds like the three of us are on the same page(ish), or at least OK with adding nibabel as a core dep. Let's give it some time to see if anyone else wants to weigh in.

Sounds good to me!

BTW, jinja2 is already a required core dependency.

Yes this is sort of my point -- we already require jinja2, and I think nibabel fits the top-comment inclusion criteria better than jinja2.

minimal install should be lightweight for running MNE on servers

Before @agramfort can complain about this 🙂 -- the pure Python part in the criteria above should help us a bit here. The nibabel Linux wheel is 3.3 MB. For context, SciPy is 34 MB and NumPy is 16.7 MB. So overall I think it's an acceptable size nowadays.

[agramfort]

Before @agramfort <https://github.com/agramfort> can complain about this 🙂 -- the pure Python part in the criteria above should help us a bit here. The nibabel Linux wheel is 3.3 MB. For context, SciPy is 34 MB and NumPy is 16.7 MB. So overall I think it's an acceptable size nowadays.

I see many usage of mne that only requires raw data processing hence only numpy and scipy (and matplotlib) if you want plotting. But not even qt... If you don't work with anatomical data which many EEG people do, nibabel is not necessary. So I am really not convinced here. To me mne should work fine for all the sensor level data without any extra dependency that assumes you have MRIs.

…

Message ID: ***@***.***>

[larsoner]

So I am really not convinced here. To me mne should work fine for all the sensor level data without any extra dependency that assumes you have MRIs.

But why? What's the cost (to end users) of adding nibabel as a required dependency? I think in practice it shouldn't cause any issues for people...

[cbrnr]

To me it's more a philosophical issue. Maybe we have differing opinions on what core MNE is, but I agree with @agramfort that it is "standard" EEG/MEG processing with sensor data.

Another option to go forward is to "soft-require" a package when it is not necessary for core functionality. For example, I assume nibabel is only used in specific sub-modules. What if we just import it normally there (and make sure that import mne is not affected)? Then we can get rid of all import shims etc. and still have a minimum set of core dependencies. If we want to go this route, I suggest to re-evaluate some of our recently added dependencies. In particular, jinja2 could be handled exactly like I'm suggesting – we don't need it as a core dependency, but people who want to generate reports will need to install it.

BTW, a regular ModuleNotFoundError is informative, so we don't need to do any extra handling (the error clearly states which package is missing).

Finally, people who use our standalone installers will always have everything available. So this is really a problem that affects "non-standard" users (e.g. people who have reasons why they do not use the standalone installers).