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

Remove use of entrypoints #442

Merged

Conversation

s-t-e-v-e-n-k
Copy link
Contributor

Since Python 3.8, the standard library has included functionality to query entry points directly using importlib.metadata. Since the API has changed for the better with Python 3.10, we need to support both ways of using it.

@s-t-e-v-e-n-k s-t-e-v-e-n-k force-pushed the use-importlib-metadata branch 2 times, most recently from 263ad55 to dfb1607 Compare June 14, 2023 11:39
@joshmoore
Copy link
Member

joshmoore commented Jun 14, 2023

Thanks, @s-t-e-v-e-n-k. I've triggered the workflows.

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #442 (af20af8) into main (d667b31) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #442   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           56        56           
  Lines         2243      2242    -1     
=========================================
- Hits          2243      2242    -1     
Files Changed Coverage Δ
numcodecs/tests/test_entrypoints.py 100.00% <ø> (ø)
numcodecs/registry.py 100.00% <100.00%> (ø)

joshmoore added a commit to conda-forge/numcodecs-feedstock that referenced this pull request Jul 27, 2023
Initially this PR will fail until a version with zarr-developers/numcodecs#442 has been released.
@joshmoore
Copy link
Member

Makes sense to me. I've also opened (a currently failing) PR to remove entrypoint from the conda-forge package: conda-forge/numcodecs-feedstock#95

@jakirkham
Copy link
Member

cc @martindurant

@martindurant
Copy link
Member

I don't know anything about `.select, but I trust you. We actually have tests exercising the entrypoint mechanism, right?

@s-t-e-v-e-n-k
Copy link
Contributor Author

There are tests, but they are marked as xfail.

@joshmoore
Copy link
Member

Good catch, @s-t-e-v-e-n-k. Can you un-xfail them here and we'll address whatever issues there are in the wheels build?

@s-t-e-v-e-n-k s-t-e-v-e-n-k force-pushed the use-importlib-metadata branch 2 times, most recently from 3227eff to 2b9bff9 Compare July 28, 2023 02:58
@s-t-e-v-e-n-k
Copy link
Contributor Author

I have rebased and removed the xfail.

@joshmoore
Copy link
Member

Re-launched workflows.

@jakirkham
Copy link
Member

Looks like the entrypoint test is failing with wheels

@joshmoore
Copy link
Member

      @pytest.fixture()
      def set_path():
          sys.path.append(here)
          numcodecs.registry.run_entrypoints()
          yield
          sys.path.remove(here)
          numcodecs.registry.run_entrypoints()
  >       numcodecs.registry.codec_registry.pop("test")
  E       KeyError: 'test'

This may be similar to the setup needed for conda-forge testing: i.e. it's not enough to just sys.path, but the test must actually be installed due to differing runtime environments.

@s-t-e-v-e-n-k
Copy link
Contributor Author

This may be similar to the setup needed for conda-forge testing: i.e. it's not enough to just sys.path, but the test must actually be installed due to differing runtime environments.

I've been wondering about that, if we want to test this piece meal by mocking, and also doing an integration test by doing ... something like what the sys.path manipulation is trying to accomplish.

@martindurant
Copy link
Member

You should need both the package and a distinfo on the path, like intake does: https://github.com/intake/intake/tree/master/intake/source/tests/plugin_searchpath

@s-t-e-v-e-n-k
Copy link
Contributor Author

I fell down an importlib.metadata vs entrypoints rabbit hole, but I nailed it down to the wheel not including the needed files for the tests to pass. I'm not certain why MANIFEST.in is not helping us here.

Since Python 3.8, the standard library has included functionality to
query entry points directly using importlib.metadata. Since the API has
changed for the better with Python 3.10, we need to support both ways of
using it.
@jakirkham
Copy link
Member

RTD failures are due to an upstream issue ( readthedocs/readthedocs.org#10601 ). Mentioned these instances there

@jakirkham
Copy link
Member

Looks like it may have been a GH issue. Restarted RTD and it appears to have passed. Also enabled GHA jobs to run

@@ -20,7 +19,6 @@ def set_path():
numcodecs.registry.codec_registry.pop("test")


@pytest.mark.xfail(reason="FIXME: not working in wheels build")
Copy link
Member

Choose a reason for hiding this comment

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

Hurrah! 🎉

Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

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

Thanks for all the hard work, @s-t-e-v-e-n-k! Merging lest other RTD issues crop up, but if there are any concerns from anyone, just let us know before the upcoming release.

@joshmoore joshmoore merged commit cb15543 into zarr-developers:main Aug 11, 2023
23 checks passed
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.

4 participants