-
Notifications
You must be signed in to change notification settings - Fork 26
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
update entry_points to use importlib.metadata #84
update entry_points to use importlib.metadata #84
Conversation
use of pkg_resources is discouraged: https://setuptools.pypa.io/en/latest/pkg_resources.html additionally, this should fix potential version conflicts where dependency versions are checked on entry point loading for pkg_resources which can cause tests in CI jobs to fail when testing against a development version
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #84 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 25 25
Lines 3066 3066
======================================
Misses 3066 3066
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@@ -16,6 +16,7 @@ dependencies = [ | |||
'crds>=7.4.1.3', | |||
'astropy>=5.0.4', | |||
'stdatamodels>=0.2.4', | |||
'importlib_metadata>=4.11.4', |
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.
importlib_metadata
is a shim package that allows us to use metadata features that have been added to future versions of Python. In this case I don't think we need it? Both romancal and jwst require Python >= 3.8, and even the provisional implementation of importlib.metadata that was added in 3.8 has all the features we use here.
I recommend:
- Drop this dependency
- Bump the minimum Python version of this package from 3.7 to 3.8
- Import entry_points like this instead:
from importlib.metadata import entry_points
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.
Sounds good. The astropy>=5.0.4
dependency has a python >= 3.8
requirement:
https://github.com/astropy/astropy/blob/326435449ad8d859f1abf36800c3fb88d49c27ea/setup.cfg#L35
and the CI does not test 3.7.
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 take that back, this fails on 3.8 and 3.9. Here is one example:
https://github.com/spacetelescope/stpipe/actions/runs/4344347265/jobs/7587577693#step:7:286
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 returned this to requiring importlib_metadata
which matches the requirement in asdf:
https://github.com/asdf-format/asdf/blob/3c83a49de6ab7455a4cd2bcc4aea65704a582db3/pyproject.toml#L25
importlib.metadata
has issues with duplicate entry points (this created an issue for ASDF which led to the requirement for importlib_metadata
for all current python version, see the PR here: asdf-format/asdf#1260).
It's possible the needs of stpipe might be solved by importlib.metadata
for python 3.10 and greater but given that asdf is a dependency and the issues with the standard library version I'm inclined to stick with adding importlib_metadata
as a requirement.
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.
Yeah the entry point stuff in for importlib_metadata
has been under a lot of active development, and I don't think some of the necessary fixes have been back-ported to importlib.metadata
in all the python versions. So I think right now its safest to use importlib_metadata
until that has stabilized and we figure out which python version(s) has gotten all the necessary changes back-ported.
d9a4370
to
801e55b
Compare
EDIT: Theastropy>=5.0.4
dependency has apython >= 3.8
requirement:https://github.com/astropy/astropy/blob/326435449ad8d859f1abf36800c3fb88d49c27ea/setup.cfg#L35
and the CI does not test 3.7. This PR increases the minimum python version to 3.8 which allows for use of importlib.metadata (see conversation below: #84 (comment))
use of pkg_resources is discouraged:
https://setuptools.pypa.io/en/latest/pkg_resources.html
additionally, this should fix potential version conflicts where dependency versions are checked on entry point loading for pkg_resources which can cause tests in CI jobs to fail when testing against a development version. Here is one example:
jwst pins stdatamodels to <1.2
stdatamodels tests against jwst as part of it's CI
stdatamodels development is currently on 1.2
the CI job installs the development >=1.2 version of stdatamodels and development version of jwst (install is forced to ignore the version conflict)
the CI job runs and all stpipe tests fail, raising
VersionConflict
when the jwst stpipe entry point is loaded due to pkg_resources checking versions at runtime:https://github.com/spacetelescope/stdatamodels/actions/runs/4315086232/jobs/7528977635#step:12:848
Testing locally this runtime version checking does not appear to happen with importlib.metadata.