-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
MAINT: Add nibabel as a required dependency #11565
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
Conversation
This reverts commit d239393.
* upstream/main: MAINT: Temporary regression workaround [circle deploy] (mne-tools#11566)
|
I think after discussion with @agramfort @britta-wstnr @larsoner and myself today, it was agreed to abandon this idea, closing as not planned. Rationale is that adding core deps can be burdensome for users in a tightly managed environment (they may need permission from IT to get |
|
Indeed I'll open another PR soon to add more |
| - NumPy >= 1.20.2 | ||
| - SciPy >= 1.6.3 | ||
| - Matplotlib >= 3.4.0 | ||
| - NiBabel >= 3.2.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we discovered this in HNN. Looks like nibabel is a core dependency now but it's not reflected in the setup.py: https://github.com/mne-tools/mne-python/blob/main/requirements_base.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give a traceback? We ended up not making it a core dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you go: jonescompneurolab/hnn-core#649 (comment)
should we be installing nibabel in our CIs separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, nibabel is an optional dependency not a core one, so you have to install it separately. PR welcome to change the wording of the error message to say something about it being optional and to consider installing it
Line 159 in 079c868
| raise ImportError(f"The {lib} package{extra} is required to {what}, " f"{why}") |
Just the start of a proof-of-concept for #11564, in addition to the lines I removed already in #11557. Would still need:
@requires_nibabelto be removednibabelpart of MAINT: Temporary regression workaround #11566