Repository navigation
ENH: Add _soft_import check for sklearn in mne.decoding - #14409
SohamXYZDev wants to merge 1 commit into
Conversation
|
@tsbinns can you look? If not, I can |
|
@larsoner Yeah, can do |
|
@SohamXYZDev Thanks for submitting this PR.
Good point, that does complicate things, but having to At the very least, we could nest the sklearn imports that are being moved to the end between However, I wonder if a cleaner approach is to still try and centralise the check in at the top of For the warnings this would cause, we could either nest the affected imports in @larsoner WDYT about that? |
| class _SoftImportError(RuntimeError, ImportError): | ||
| """Raised when an optional dependency is missing or incompatible.""" | ||
|
|
||
| pass |
There was a problem hiding this comment.
IIUC, the ImportError comes from ._fixes.py, but shouldn't that never be encountered since we are adding the _soft_import check at the top of that file?
There was a problem hiding this comment.
In the minimal / 3.12 CI build (where scikit-learn is not installed), pytest collects doctests using --doctest-modules --doctest-ignore-import-errors.
That pytest flag only ignores errors that inherit from ImportError. Because _soft_import raised a raw RuntimeError, pytest failed during test collection when importing _fixes.py. Making _SoftImportError inherit from both RuntimeError and ImportError preserves MNE's RuntimeError behavior while letting pytest's doctest collector skip it cleanly when scikit-learn is absent.
|
Yeah I think centralizing is better, that way if we ever want to change it's easier |
59ac762 to
17d0141
Compare
|
@tsbinns @larsoner Updated! Rebased on latest
All 1,311 decoding tests pass locally, and the missing-sklearn import checks verified cleanly. |
17d0141 to
0b0bdcd
Compare
| class MNETransformerMixin(TransformerMixin): | ||
| """TransformerMixin plus some helpers.""" |
There was a problem hiding this comment.
Ah, didn't realise this would introduce a circular import conflict. It's preferable if we don't have to move the mixin around.
Perhaps we can have the import check in a new _check.py file to avoid the circular imports. CC @larsoner
There was a problem hiding this comment.
sounds good to me, happy to move the check into a dedicated _check.py and restore the mixin once confirmed
There was a problem hiding this comment.
updated: moved the check into mne/decoding/_check.py and restored the mixin and transformer imports back to transformer.py. nesting checks and tests pass locally.
0b0bdcd to
30b33c4
Compare
Reference issue (if any)
Fixes #14394.
What does this implement/fix?
Adds
_soft_import("sklearn", "decoding")before top-levelsklearnimports across themne.decodingsubmodules (_fixes.py,base.py,ems.py,receptive_field.py,search_light.py,time_delaying_ridge.py,time_frequency.py, andtransformer.py) so that accessingmne.decodingfunctionality withoutscikit-learninstalled raises MNE's informativeRuntimeErrorinstead of a rawModuleNotFoundError.Also updates
mne/tests/test_docstring_parameters.pyto handle theRuntimeErrorfrom_soft_importwhen walkingmne.decodingin minimal environments withoutscikit-learn, and adds test coverage inmne/utils/tests/test_check.py.Additional information
Because
ruff(I001) sorts third-partyfrom sklearn...imports before relativefrom .base import ...imports, placing_soft_importonly inbase.pywould still allowfrom sklearn...in submodules liketransformer.pyorsearch_light.py(as well asems.pyand_fixes.py, which do not import frombase.py) to fail withModuleNotFoundErrorbeforebase.pyis reached. Placing_soft_import("sklearn", "decoding")right before thesklearnimports in each submodule (with# noqa: E402, matchingmne/export/_edf_bdf.pyand_eeglab.py) ensures every entry point inmne.decoding.__all__consistently raises the_soft_importerror while keepingimport mneandimport mne.decodinglazy.I used Gemini 3.8 Flash to help check import ordering across the decoding submodules and draft the unit test, and reviewed and verified all changes locally.