Fix default entrypoint handling on older Pythons#475
Fix default entrypoint handling on older Pythons#475jakirkham merged 21 commits intozarr-developers:mainfrom
Conversation
|
Thanks Vyas! 🙏 Made some tweaks to handle the release note. Hope that is ok. Please update as needed Also maybe we should test this somehow. Currently we test the presence of a single entrypoint. Maybe we should also test the absence of any entrypoints? numcodecs/numcodecs/tests/test_entrypoints.py Lines 22 to 24 in a5539b5 |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #475 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 56 57 +1
Lines 2242 2261 +19
=========================================
+ Hits 2242 2261 +19
|
numcodecs 0.12 appears to have some breaking changes. I've submitted a fix upstream, but for this release of RAPIDS we'll need to upper bound the version to be safe. xref: zarr-developers/numcodecs#475 Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice)
|
@martindurant , could you please look over this? |
|
Was seeing AVX2 issues on macOS builds, that I thought we just fixed with PR ( #479 ). So toggling PR to ensure latest changes in |
|
OSX failures are still the sporadic: Trying a rebuild on the action itself. Otherwise, 👍 for the PR. |
|
Maybe one more rerun? Looks like attempt 2 passed one of the previously failing jobs but the other failed again. |
|
Have added a fix in |
|
Since the only entrypoint added is coming from the test entrypoint that we load ourselves. Wonder if we can avoid using a subprocess by simply testing there are no entrypoints present (and effectively those code paths work) before trying to add our own entrypoint (to test that we can load entrypoints). What do you think Vyas? |
I'm afraid I don't understand how your suggestion avoids the problem. My concern is that because If you think those cases aren't worth worrying about, though, then I can revert the multiprocessing change. |
|
Ah missed we added tests for the backport package Ok would it be possible to move these into their own test file? Maybe |
|
Done. The test is now in its own module and only runs if |
| sys.path.append(here) | ||
| numcodecs.registry.run_entrypoints() | ||
| cls = numcodecs.registry.get_codec({"id": "test"}) | ||
| assert cls.codec_id == "test" |
There was a problem hiding this comment.
Would it make sense to reuse set_path here somehow? Should cleanup sys.path and the numcodecs.registry after?
There was a problem hiding this comment.
I thought about reusing, but I think it'd be more work than it's worth. We can't use a fixture directly because it's not supposed to affect the actual test function, we need it to affect the helper function that's run in the subprocess and that function isn't a test so pytest won't patch in fixtures. We could try and split it into a helper function like I had initially, but it's not really necessary. Since the function is running in a subprocess we don't need to clean things up, it shouldn't affect the modules in the parent process.
Co-authored-by: jakirkham <jakirkham@gmail.com>
|
Thanks Vyas! 🙏 |
The fallback here for updating a dictionary needs to be a dictionary, not a list.
I haven't added any new tests or anything, but I can if I'm pointed in the right direction. Not sure how this library wants to test these types of edge cases with entry points.
Resolves #478
TODO: