-
Notifications
You must be signed in to change notification settings - Fork 106
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
Replace pkg_resources with importlib.metadata #137
Conversation
pkg_resources is deprecated in favor of importlib.metadata (https://setuptools.readthedocs.io/en/latest/pkg_resources.html). Due to its caching behaviour, using pkg_resources causes errors on google colab (huggingface/transformers#10964), which switching to importlib.metadata solves. For version before 3.8, the importlib-metadata package provides an official backport.
if sys.version_info[:2] >= (3, 8): | ||
import importlib.metadata as importlib_metadata | ||
else: | ||
import importlib_metadata | ||
|
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.
Another option here is to just catch the ImportError (and you wouldn't need to import sys
)
if sys.version_info[:2] >= (3, 8): | |
import importlib.metadata as importlib_metadata | |
else: | |
import importlib_metadata | |
try: | |
import importlib.metadata as importlib_metadata | |
except ImportError: | |
import importlib_metadata |
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.
I'd prefer not to swallow any exceptions
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.
Fair enough! Suggested because it's a fairly common pattern (there's an except ImportError
in this file already).
A totally different (over-the-top) solution to this issue would be to rewrite the packaging to use setup.cfg
and pyproject.toml
files, at which point setup.py
is no longer necessary and the version can be stored as a literal in this file (setup.cfg
can read it). I only learned how to do this yesterday, so I'm mad with power. 😂
edit: although this is a relatively new feature of setuptools
and might not work well in all environments, so maybe it's better to wait on that.
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.
Personally, I'm all for switching everything to PEP 621 and reading metadata through importlib.metadata
, but for now I guess __version__
and importlib_metadata
will have to do ¯\_(ツ)_/¯
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.
This looks good to me. I think the issue we discussed above can be resolved in another PR someday--I think it'd be reasonable to switch to a PEP 621 installer but that might necessitate a version bump, not sure about backwards compatibility.
pkg_resources was causing errors when using our package on google colab due to its internal caching (sacdallago/bio_embeddings#160, sacdallago/bio_embeddings#123), so I've replaced it with importlib.metadata. I've done the same change before for huggingface/transformers (ttps://github.com/huggingface/transformers/issues/10964). Also note that pkg_resources is deprecated in favor of importlib.metadata (https://setuptools.readthedocs.io/en/latest/pkg_resources.html). For python versions older than 3.8, I'm using the official importlib-metadata backport.