Update to zarr 3 and main kerchunk #406

jsignell · 2025-01-30T16:47:41Z

Pulls commits from #378, #405 and #393

UPDATE: Ideas for #376 removed in 42414f0

Tests added
Tests passing
Full type hint coverage
Changes are documented in docs/releases.rst
New functions/methods are listed in api.rst
New functionality has documentation

The remaining failures on upstream can be broken into:

codecs-related:

FAILED virtualizarr/tests/test_codecs.py::TestCodecs::test_manifest_array_zarr_v2_normalized - AssertionError: assert (BytesCodec(e...shuffle': 1})) == (Delta(codec_...shuffle': 1}))
FAILED virtualizarr/tests/test_writers/test_icechunk.py::test_write_loadable_variable - TypeError: Group.create_array() got an unexpected keyword argument 'exists_ok'
FAILED virtualizarr/tests/test_writers/test_zarr.py::test_zarr_v3_metadata_conformance - AssertionError: assert (True and 1 > 1)

probably setting up the in-memory icechunk store wrong - these are new tests that have never worked - removed in 42414f0

FAILED virtualizarr/tests/test_integration.py::TestRoundtrip::test_roundtrip_no_concat[HDFVirtualBackend-roundtrip_as_in_memory_icechunk] - TypeError: Expected a BytesBytesCodec. Got <class 'numcodecs.zarr3.FixedScaleOffset'> instead.
FAILED virtualizarr/tests/test_integration.py::TestRoundtrip::test_kerchunk_roundtrip_concat[False-time_vars0-HDFVirtualBackend-roundtrip_as_in_memory_icechunk] - TypeError: Expected a BytesBytesCodec. Got <class 'numcodecs.zarr3.FixedScaleOffset'> instead.
FAILED virtualizarr/tests/test_integration.py::TestRoundtrip::test_kerchunk_roundtrip_concat[True-time_vars1-HDF5VirtualBackend-roundtrip_as_in_memory_icechunk] - TypeError: Group.create_array() got an unexpected keyword argument 'exists_ok'
FAILED virtualizarr/tests/test_integration.py::TestRoundtrip::test_kerchunk_roundtrip_concat[True-time_vars1-HDFVirtualBackend-roundtrip_as_in_memory_icechunk] - TypeError: Group.create_array() got an unexpected keyword argument 'exists_ok'
FAILED virtualizarr/tests/test_integration.py::TestRoundtrip::test_datetime64_dtype_fill_value[roundtrip_as_in_memory_icechunk] - KeyError: '<M8[ns]'

…uire kerchunk

TomNicholas · 2025-01-30T17:17:42Z

probably setting up the in-memory icechunk store wrong - these are new tests that have never worked

I don't know that this PR can be disentangled from the changes we need to get icechunk working with zarr v3...

jsignell · 2025-01-30T17:21:15Z

I don't know that this PR can be disentangled from the changes we need to get icechunk working with zarr v3...

I can take the new in-memory icechunk stuff and leave the place where it could slot in. Or this can just be used as a reference point. I'm not attached 😄

TomNicholas · 2025-01-30T19:48:32Z

So IIUC we can either:

Merge this now, break main but only for upstream, then fix the remaining failures in follow-up PRs. That's kind of what Planning explicit dependency on Zarr v3 #392 (comment) implied by separating the "pin zarr" and "fix results of pinning zarr" into separate steps.
Just try to fix the codec issues in this PR

maxrjones · 2025-01-30T21:03:46Z

So IIUC we can either:

Merge this now, break main but only for upstream, then fix the remaining failures in follow-up PRs. That's kind of what Planning explicit dependency on Zarr v3 #392 (comment) implied by separating the "pin zarr" and "fix results of pinning zarr" into separate steps.

Just try to fix the codec issues in this PR

I'd slightly prefer if we merged this now because it seems to me like this PR really breaks the stalemate that we were in and would unblock other work. specifically for my work, I'd love to merge it into #407 to get the updates already implemented

pyproject.toml

TomNicholas · 2025-01-30T23:41:30Z

I'd slightly prefer if we merged this now

I'm happy to merge this now if @jsignell is. We want to block everyone else as little as possible.

Ab/fix tests

jsignell · 2025-01-31T13:37:19Z

It looks like @abarciauskas-bgse might have fixed all the tests? 👏🏻

jsignell · 2025-01-31T13:38:25Z

virtualizarr/tests/test_writers/test_zarr.py

@@ -42,7 +42,7 @@ def test_zarr_v3_metadata_conformance(tmpdir, vds_with_manifest_arrays: Dataset)
    assert isinstance(metadata["fill_value"], (bool, int, float, str, list))
    assert (
        isinstance(metadata["codecs"], list)
-        and len(metadata["codecs"]) > 1
+        and len(metadata["codecs"]) == 1


@abarciauskas-bgse did the expectation change so it is correct that there is one codec now?

Yes, this is because of how serializer was broken out of _v3_codec_pipeline so these are 2 separate objects now. Today I will investigate if this is how it should be, but my guess is that this will become a part of the refactor to use the zarr-python array class instead of Virtualizarr's zarr.py

yeah I took a look at #175 and I agree that this is all going away. So it is probably not worth doing too much work around it.

jsignell · 2025-01-31T13:46:14Z

virtualizarr/zarr.py

@@ -198,6 +191,23 @@ def _v3_codec_pipeline(self) -> Any:

        return codec_pipeline

+    def serializer(self) -> Any:


This function could stand to get reviewed and tested. It looks like it was meant to be just a sketch of what a serializer could look like

If this passes the tests we can perhaps merge and deal with this in the follow-up PR that refactors ZArray away entirely?

Yeah exactly.

jsignell · 2025-01-31T18:58:53Z

I just double checked #410 by merging it with this branch and the tests worked! (Although I did find 76dcfb7 🙊 )

jsignell · 2025-01-31T23:21:45Z

I wasn't sure whether or not to add release notes, but feel free to push to this branch or just merge as you see fit.

TomNicholas · 2025-01-31T23:42:27Z

We'll just add release notes in the follow-up that actually changes the kerchunk dependency to a released version

jsignell and others added 6 commits January 30, 2025 09:40

Use open_dataset_kerchunk in roundtrip tests that don't otherwise req…

511f067

…uire kerchunk

Make it clear that integration tests require zarr-python

61e3cff

Add in-memory icechunk tests to existing roundtrip tests

bd95d60

Playing around with icechunk / zarr / xarray upgrade

cf9fc4f

Passing icechunk tests

390efca

Update tests to latest kerchunk

8581ace

jsignell temporarily deployed to test-release January 30, 2025 16:48 — with GitHub Actions Inactive

jsignell mentioned this pull request Jan 30, 2025

Adding to_icechunk as another type of roundtrip tests #393

Closed

5 tasks

This was referenced Jan 30, 2025

Vendor kerchunk fits reader #399

Closed

Add astropy for fits test #403

Closed

Remove icechunk roundtripping

42414f0

TomNicholas added zarr-python Relevant to zarr-python upstream Kerchunk Relating to the kerchunk library / specification itself dependencies Updates a dependency Icechunk 🧊 Relates to Icechunk library / spec v3-migration Required for migration to Zarr-Python 3.0 labels Jan 30, 2025

jsignell temporarily deployed to test-release January 30, 2025 17:24 — with GitHub Actions Inactive

maxrjones reviewed Jan 30, 2025

View reviewed changes

pyproject.toml Show resolved Hide resolved

abarciauskas-bgse mentioned this pull request Jan 30, 2025

Ab/upgrade icechunk #378

Closed

abarciauskas-bgse added 6 commits January 30, 2025 15:41

Fixed some warnings

ff659b9

Fixed codec test

b3ed8ff

Fix warnings in test_backend.py

57be796

Tests passing

deb15ba

Remove obsolete comment

cecc151

Add fill value to fixture

6d06291

abarciauskas-bgse and others added 9 commits January 30, 2025 17:58

Remove obsolete conditional to ds.close()

4a2f294

Reset workflows with --cov

aa55a39

Reset conftest.py fixtures (air encoding)

83379ee

Reset contributiong (--cov) removed

7786421

Remove context manager from readers/common.py

45eb86b

Reset test_backend with ds.dims

0babb65

Reset test_icechunk (air encoding)

dc4ed8b

Fix tests by @abarciauskas-bgse

77b1088

Ab/fix tests

Merge branch 'main' into main-kerchunk

d4dfb6c

jsignell temporarily deployed to test-release January 31, 2025 13:37 — with GitHub Actions Inactive

jsignell commented Jan 31, 2025

View reviewed changes

Fix change that snuck in on zarr-developers#395

76dcfb7

jsignell temporarily deployed to test-release January 31, 2025 18:57 — with GitHub Actions Inactive

TomNicholas approved these changes Jan 31, 2025

View reviewed changes

Merge branch 'main' into main-kerchunk

2d6f4fb

jsignell temporarily deployed to test-release January 31, 2025 23:20 — with GitHub Actions Inactive

TomNicholas merged commit 83a6f10 into zarr-developers:main Jan 31, 2025
10 of 11 checks passed

jsignell deleted the main-kerchunk branch February 3, 2025 14:56

jsignell mentioned this pull request Feb 3, 2025

Bump Xarray to 2025.1.1 and icechunk to 0.1.0a10 in upstream #375

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to zarr 3 and main kerchunk #406

Update to zarr 3 and main kerchunk #406

jsignell commented Jan 30, 2025 •

edited

Loading

TomNicholas commented Jan 30, 2025

jsignell commented Jan 30, 2025

TomNicholas commented Jan 30, 2025

maxrjones commented Jan 30, 2025

TomNicholas commented Jan 30, 2025

jsignell commented Jan 31, 2025

jsignell Jan 31, 2025

abarciauskas-bgse Jan 31, 2025

jsignell Jan 31, 2025

jsignell Jan 31, 2025

TomNicholas Jan 31, 2025

jsignell Jan 31, 2025

jsignell commented Jan 31, 2025

jsignell commented Jan 31, 2025

TomNicholas commented Jan 31, 2025

		@@ -198,6 +191,23 @@ def _v3_codec_pipeline(self) -> Any:

		return codec_pipeline

		def serializer(self) -> Any:

Update to zarr 3 and main kerchunk #406

Update to zarr 3 and main kerchunk #406

Conversation

jsignell commented Jan 30, 2025 • edited Loading

TomNicholas commented Jan 30, 2025

jsignell commented Jan 30, 2025

TomNicholas commented Jan 30, 2025

maxrjones commented Jan 30, 2025

TomNicholas commented Jan 30, 2025

jsignell commented Jan 31, 2025

jsignell Jan 31, 2025

Choose a reason for hiding this comment

abarciauskas-bgse Jan 31, 2025

Choose a reason for hiding this comment

jsignell Jan 31, 2025

Choose a reason for hiding this comment

jsignell Jan 31, 2025

Choose a reason for hiding this comment

TomNicholas Jan 31, 2025

Choose a reason for hiding this comment

jsignell Jan 31, 2025

Choose a reason for hiding this comment

jsignell commented Jan 31, 2025

jsignell commented Jan 31, 2025

TomNicholas commented Jan 31, 2025

jsignell commented Jan 30, 2025 •

edited

Loading