Skip to content
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 lower pin to importlib_metadata for all python versions #1260

Merged

Conversation

braingram
Copy link
Contributor

importlib in the standard library and importlib_metadata prior to 4.11.4 library return duplicate entry points in some cases. This leads to duplication of extensions in entry_points.get_extensions.

remove use of importlib for python >= 3.10 and require use of importlib_metadata >= 4.11.4

fixes issue #1254

Copy link
Contributor

@WilliamJamieson WilliamJamieson left a comment

Choose a reason for hiding this comment

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

This will also need a regression test for the issue #1254

asdf/tests/test_entry_points.py Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@braingram
Copy link
Contributor Author

Do you have something in mind for a regression test?

Should we test for duplicate entry points?

If instead we test for duplicate extensions, what should be considered a duplicate vs an extension that supports the same types (and versions of those types) to override the default behavior (as explained in the docs)?

I'd appreciate any additional details you can provide on this.

@WilliamJamieson
Copy link
Contributor

Do you have something in mind for a regression test?

Should we test for duplicate entry points?

If instead we test for duplicate extensions, what should be considered a duplicate vs an extension that supports the same types (and versions of those types) to override the default behavior (as explained in the docs)?

I'd appreciate any additional details you can provide on this.

In #1254 (comment) you claim:

import asdf
import weldx

af = asdf.AsdfFile()
ctx = af._create_serialization_context()
weldx.asdf.util.get_weldx_extension(ctx)

Produces an error which is then fixed by this PR. These steps should be reducible to a (possibly contrived) minimal reproducer, independent of weldx which reproduces this error. This minimal reproducer can be turned into a regression test.

Currently, we don't know what exactly is causing the problem only that the change in this PR fixes it.

@braingram
Copy link
Contributor Author

Thanks for the explanation.

The issue is a bug in importlib_metadata that occurs (at least in one instance) when the example in #1254 is run with weldx installed and in the weldx source directory. It is similar to python/importlib_metadata#410 which was fixed in python/importlib_metadata#379

This comment mentions that 3.11 may also have issues python/importlib_metadata#410 (comment) so relying on the standard library might not work and will require more testing.

One regression test I could imagine might work to reproduce the issue would involve creating a package, installing it, and producing a matching .egg-info to provide the conditions for the duplicate entry points. This would involve significant test infrastructure for what appears to be a bug in a dependency. Can you think of an alternative? If not, I don't think the complexity required to add this test is worth what it provides.

@WilliamJamieson
Copy link
Contributor

The OpenAstronomy workflow we use can be used to test a package using the installed version, which we should be doing anyways.

If it's too much work I understand, but I prefer having a minimal reproducer for regressions so that we don't have repeat issues.

@braingram
Copy link
Contributor Author

Testing with 3.11 it also returns duplicate entry points

(weldx_3.11) aerum2022:asdf bgraham$ ipython
iPython 3.11.0 (main, Dec  7 2022, 10:03:05) [Clang 14.0.0 (clang-1400.0.29.102)]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.7.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import importlib.metadata

In [2]: importlib.metadata.entry_points(group='asdf_extensions')
Out[2]:
[EntryPoint(name='builtin', value='asdf.extension:BuiltinExtension', group='asdf_extensions'),
 EntryPoint(name='builtin', value='asdf.extension:BuiltinExtension', group='asdf_extensions'),
 EntryPoint(name='astropy', value='astropy.io.misc.asdf.extension:AstropyExtension', group='asdf_extensions'),
 EntryPoint(name='astropy-asdf', value='astropy.io.misc.asdf.extension:AstropyAsdfExtension', group='asdf_extensions')]

We could do one or both of the following:

  1. change the code in asdf to not assume entry points will not be duplicated
  2. require importlib_metadata>=4.11.4 for all python versions

Writing a regression test for 1 would not add significant complexity to the tests (as we already mock entry points in test_entry_points).

@WilliamJamieson are there other options and which do you prefer?

@braingram braingram marked this pull request as ready for review December 9, 2022 20:31
@braingram braingram requested a review from a team as a code owner December 9, 2022 20:31
@WilliamJamieson
Copy link
Contributor

@WilliamJamieson are there other options and which do you prefer?

Hmm... I think we should remove the if wrapper choosing importing importlib_metadata vs importlib.metadata (maybe comment it out?). While doing so we should add a comment that links back to issue #1254 and this PR for details on reproducing the issue (maybe leave #1254 open after this PR is closed?). We can try to re-introduce using the builtin importlib.metadata when future versions of Python are released. This would also require changing the importlib-metadata to be for all python versions.

I think step 1 is a good idea also, though I think that change is outside the scope of this PR. Let's open a separate issue/PR to track doing that separately from this. Once that is done, we should revisit #1254 and the changes from this PR.

@braingram braingram closed this Dec 12, 2022
@braingram braingram reopened this Dec 12, 2022
@braingram
Copy link
Contributor Author

Closing and reopening to trigger CI as previous failures for mac osx test appear unrelated to code changes (it appears to be a network timeout).

The stdlib importlib.metadata returns duplicate distributions
(and entry points) in some circumstances for all python versions
up to and including 3.11.

python/importlib_metadata#410 (comment)

The above issue can create duplicate asdf extensions.

This fixes issue asdf-format#1254

fix changelog typo
@braingram
Copy link
Contributor Author

The remaining CI failure is the codecov/project check failing due to a insignificant change in coverage.

@WilliamJamieson
Copy link
Contributor

The remaining CI failure is the codecov/project check failing due to a insignificant change in coverage.

Coverage changes are not that important (hence why they are not required CI to pass). In reality this change should not effect coverage at all but it does reduce the number of lines in a imperfectly covered file meaning there is a tiny shift in converage.

We really need to set the thresholds better.

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.

None yet

2 participants