-
Notifications
You must be signed in to change notification settings - Fork 102
chore/hollow out zarr3 #780
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (89.12%) is below the target coverage (100.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #780 +/- ##
===========================================
- Coverage 99.82% 89.12% -10.70%
===========================================
Files 64 64
Lines 2804 2547 -257
===========================================
- Hits 2799 2270 -529
- Misses 5 277 +272
🚀 New features to boost your workflow:
|
@zarr-developers/python-core-devs please take a look at this. I'm removing the zarr-python-dependent codecs from numcodecs and re-exporting codecs from zarr-python instead. When zarr python 3.1.3 or greater is installed, importing a codec from This solves our long-standing circular dependency issue, but I want to make sure we get this right. |
A summary of this PR:
>>> from numcodecs.zarr3 import Blosc
<stdin>-1:1: DeprecationWarning: The numcodecs.zarr3 module is deprecated and will be removed in a future release of numcodecs. Import Blosc via zarr.codecs.numcodecs.Blosc instead. This requires Zarr Python >= 3.1.3.
I would like to merge this today after the Zarr Python dev meeting, unless someone objects to these changes. In the release after the next one, we can remove |
Thank you for your efforts in resolving the circular dependency issue! I'd rather someone with more numcodecs experience give a formal review. Should Zarr now be an optional dependency for numcodecs? It's currently only listed in dependency-groups as far as I see, which means it's not included as an optional dependency on release distributions |
My feeling is that Zarr should not be listed as an optional dependency, because numcodecs doesn't actually use Zarr for anything. But as we are in a bit of a weird situation, maybe there is some reason to declare Zarr as an optional dep here? Open to ideas. |
Wouldn't an optional dep be the circular dependency we want to get rid of? Thanks @d-v-b for working on this! |
When I add this PR as a dependency in VirtualiZarr, I get this error: ImportError while loading conftest '/Users/max/Documents/Code/zarr-developers/VirtualiZarr/conftest.py'.
conftest.py:17: in <module>
from virtualizarr.manifests import ChunkManifest, ManifestArray
virtualizarr/__init__.py:3: in <module>
from virtualizarr.accessor import (
virtualizarr/accessor.py:17: in <module>
from virtualizarr.manifests import ManifestArray
virtualizarr/manifests/__init__.py:4: in <module>
from virtualizarr.manifests.array import ManifestArray # type: ignore # noqa
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
virtualizarr/manifests/array.py:6: in <module>
from zarr.core.metadata.v3 import ArrayV3Metadata, RegularChunkGrid
.pixi/envs/upstream/lib/python3.13/site-packages/zarr/__init__.py:2: in <module>
from zarr.api.synchronous import (
.pixi/envs/upstream/lib/python3.13/site-packages/zarr/api/synchronous.py:7: in <module>
import zarr.api.asynchronous as async_api
.pixi/envs/upstream/lib/python3.13/site-packages/zarr/api/asynchronous.py:13: in <module>
from zarr.core.array import (
.pixi/envs/upstream/lib/python3.13/site-packages/zarr/core/array.py:30: in <module>
from zarr.codecs._v2 import V2Codec
.pixi/envs/upstream/lib/python3.13/site-packages/zarr/codecs/__init__.py:5: in <module>
from zarr.codecs.crc32c_ import Crc32cCodec
.pixi/envs/upstream/lib/python3.13/site-packages/zarr/codecs/crc32c_.py:8: in <module>
from crc32c import crc32c
E ModuleNotFoundError: No module named 'crc32c' |
makes sense, thanks for explaining |
this PR removes the classes defined in
numcodecs.zarr3
, and instead re-exports those classes fromzarr.codecs._numcodecs
. This PR is a draft until zarr-developers/zarr-python#3376 is merged in zarr-python.closes #773, #768, #778