Skip to content

Conversation

@saimn
Copy link
Contributor

@saimn saimn commented Nov 25, 2020

Following discussions on pkg_resources vs importlib.metadata: pkg_resources is slow (1, 2, 3), which is the reason why importlib.metadata was added to the stdlib in Python 3.8 (with a backport for 3.7).

For instance, importing astropy.modeling:
image

There is a better alternative, so let's use it ?

docs/conf.py Outdated

# The full version, including alpha/beta/rc tags.
release = get_distribution(project).version
release = version(project)
Copy link
Member

Choose a reason for hiding this comment

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

😮 I'm going to use this everywhere now!

@pllim
Copy link
Member

pllim commented Nov 30, 2020

This touches modeling and io.misc.asdf entry points, so maybe one of @nden , @perrygreenfield , @jdavies-st , or @eslavich can have a look?

I guess adding it as a required external dependency for just Python 3.7 is acceptable annoyance if it has clear performance gain.

@nden
Copy link
Contributor

nden commented Mar 8, 2021

Sorry I'm just noticing this now.
Will review shortly.

Copy link
Contributor

@eslavich eslavich left a comment

Choose a reason for hiding this comment

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

This looks great to me, seems like we should adopt importlib.metadata in asdf as well. Thanks for showing us the way!

@nden
Copy link
Contributor

nden commented Mar 19, 2021

@saimn Thanks! Feel free to merge after a rebase.

@embray
Copy link
Member

embray commented Apr 6, 2021

👍 🎊

Python packaging has come a long way since this was last worked on.

@pllim
Copy link
Member

pllim commented Apr 6, 2021

Just to be sure, can you please rebase? All the CI should be green now.

@pllim
Copy link
Member

pllim commented Apr 6, 2021

Also, since this introduces a new required dependency, it needs a change log. Alas, the towncrier check doesn't work right now, so you won't get reminded in the status checks.

@embray
Copy link
Member

embray commented Apr 6, 2021

FWIW the importlib-metadata dependency is just a backport for Python 3.7, but I agree it merits a changelog mention.

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@pllim pllim merged commit 2d8ef1b into astropy:main Apr 7, 2021
@saimn saimn deleted the importlib.metadata branch April 7, 2021 21:05
saimn added a commit to saimn/astropy that referenced this pull request Apr 7, 2021
Ref astropy#11091 (review)
packaging is used in tests and the Sphinx config so we should list it
explicitly as a dependency, even if it's already a dependency for pytest
and Sphinx.
nstarman pushed a commit to nstarman/astropy that referenced this pull request Apr 26, 2021
Ref astropy#11091 (review)
packaging is used in tests and the Sphinx config so we should list it
explicitly as a dependency, even if it's already a dependency for pytest
and Sphinx.
aaryapatil pushed a commit to aaryapatil/astropy that referenced this pull request May 3, 2021
Ref astropy#11091 (review)
packaging is used in tests and the Sphinx config so we should list it
explicitly as a dependency, even if it's already a dependency for pytest
and Sphinx.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants