Skip to content

Conversation

@aplzr
Copy link
Contributor

@aplzr aplzr commented Feb 6, 2023

Reference issue

Fixes #11448.

What does this implement/fix?

mne currently relies on pkg_resources, which is a part of setuptools, without declaring it as a dependency. This can cause issues in environments where setuptools is not present by default (see #11448 for an example).

This PR updates mne to use importlib.resources for Python >= 3.9 or its backport importlib_resources for Python < 3.9 as recommended here. importlib is part of the standard library.

Additional information

The only importlib function this PR uses is importlib.resources.files(), which was added in Python 3.9. I added a dependency to importlib_resources in requirements_base.txt for Python versions < 3.9. Please advise if this is ok. If not I will update the PR accordingly, but it would mean that mne would still rely on pkg_resources for Python < 3.9 (meaning that setuptools should be declared as a dependency then).

pkg_resources was used in two functions, mne/datasets/eegbci/eegbci.py:load_data() and mne/utils/misc.py:_resource_path(). There are currently no unit tests that test load_data(), but manually calling the updated version works for Python 3.7 and 3.11 on my end. All unit tests calling the updated _resource_path() are passing when I run them locally in Python 3.7 and 3.11.

@welcome
Copy link

welcome bot commented Feb 6, 2023

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

@cbrnr
Copy link
Contributor

cbrnr commented Feb 6, 2023

Regarding the use of checksums in eegbci, I wonder if it's really necessary to query a local file with importlib.resources.files. Could we not fetch the original checksum file and check against its contents? I feel like it's probably not worth having this mechanism just for our checksums.

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it's probably not worth having this mechanism just for our checksums.

Agreed but we do make use of files in mne/utils/misc.py already, so why not keep/use it

Changes LGTM, @drammock feel free to merge if you agree this is a reasonable fix. I was never totally sure about the packaging / pkg_resources etc. stuff...

@larsoner larsoner added this to the 1.4 milestone Feb 6, 2023
@aplzr aplzr marked this pull request as ready for review February 6, 2023 19:33
@aplzr aplzr changed the title [WIP] Fix: Use importlib.resources or importlib_resources instead of pkg_resources Fix: Use importlib.resources or importlib_resources instead of pkg_resources Feb 6, 2023
Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding a couple of code comments for future maintainers. Let's see if the CIs are happy...

@drammock drammock enabled auto-merge (squash) February 9, 2023 19:54
@aplzr
Copy link
Contributor Author

aplzr commented Feb 9, 2023

Let's see if the CIs are happy...

No such luck I suppose.

@larsoner
Copy link
Member

larsoner commented Feb 9, 2023

It looks like the failures are an unrelated bug where nipy.org is down, so our docs complain. So I think we're actually okay!

@drammock
Copy link
Member

drammock commented Feb 9, 2023

only failures are the nipy ones, so in it goes! Thanks @aplzr!

@drammock drammock disabled auto-merge February 9, 2023 23:16
@drammock drammock merged commit e12ba0a into mne-tools:main Feb 9, 2023
@welcome
Copy link

welcome bot commented Feb 9, 2023

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

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

Successfully merging this pull request may close these issues.

mne depends on setuptools without declaring it as a dependecy, which can break build backends other than setuptools

5 participants