Manifest arrays use arrayv3metadata #429

abarciauskas-bgse · 2025-02-06T03:30:02Z

This is still very much a WIP - many tests and implementations still need to be fixed.

A few notes:

It was suggested we remove ZArray completely as a part of this work, as opposed to using a conversion function for ZArrays to ArrayV3Metadata. So we should be able to remove ZArray as a part of this pr.
It was suggested not to use zarr's _parse_chunk_encoding_v3 function since it is a private function and may change, which is why some of that logic is replicated in convert_to_codec_pipeline

Checklist

virtualizarr/manifests/array.py

virtualizarr/manifests/array_api.py

…not happy about this)

virtualizarr/writers/kerchunk.py

virtualizarr/zarr.py

norlandrhagen · 2025-02-12T18:06:29Z

Yes my first suggestion was similar to your third suggestion.

It sounds like we are leaning towards a feature branch (zarr-python-3.0?) but I'll wait for others to chime in. cc @norlandrhagen @ayushnag

That option seems great to me. Thanks for moving this along @abarciauskas-bgse

abarciauskas-bgse · 2025-02-12T23:56:52Z

I'm going to continue to review this tomorrow but the tests are passing and I've done an initial reorganization of the code that was in zarr.py. So if any of @TomNicholas @norlandrhagen @ayushnag @sharkinsspatial @jsignell @mpiannucci want to start to review please go ahead 🙏🏽

I also changed the base to a new branch of main zarr-python-3.0,

TomNicholas

This is absolutely great @abarciauskas-bgse ! Comments are really just minor.

TomNicholas · 2025-02-13T03:23:56Z

ci/upstream.yml

@@ -13,7 +13,7 @@ dependencies:
  - ujson
  - universal_pathlib
  - hdf5plugin
-  - numcodecs
+  - numcodecs>=0.15.1
  - imagecodecs>=2024.6.1


Should list zarr explicitly in all the envs, and in fact in this upstream one we could install it from main.

TomNicholas · 2025-02-13T03:26:57Z

conftest.py

+@pytest.fixture
+def array_v3_metadata():


Could we reimplement this fixture to internally just call the array_v3_metadata_dict fixture below?

TomNicholas · 2025-02-13T03:29:59Z

docs/usage.md

 ```
-ZArray(shape=(2920, 25, 53), chunks=(2920, 25, 53), dtype=int16, compressor=None, filters=None, fill_value=None)
+ArrayV3Metadata(shape=(2920, 25, 53),


The explosion of information in this printed repr is an argument for either an xarray short repr (@keewis is that public API?) or for replacing these markdown files with rendered output of notebooks (see #83).

TomNicholas · 2025-02-13T03:33:07Z

virtualizarr/codecs.py

    from .manifests.array import ManifestArray

+CodecPipeline = Tuple[
+    Union["ArrayArrayCodec", "ArrayBytesCodec", "BytesBytesCodec"], ...


If you prefer since python 3.10 you can use | instead of Union.

TomNicholas · 2025-02-13T03:35:46Z

virtualizarr/codecs.py

+def extract_codecs(
+    codecs: CodecPipeline,
+) -> tuple[
+    tuple[ArrayArrayCodec, ...], ArrayBytesCodec | None, tuple[BytesBytesCodec, ...]


This is a pretty complicated type that I had to stare at to work out what it is. Use TypeAlias with an informative name?

Also is it definitely the right type? Seems weird that this would be valid: ((,), None, (,))

TomNicholas · 2025-02-13T04:15:24Z

virtualizarr/tests/test_codecs.py

-            ],
+    def test_manifest_array_zarr_v3_with_codecs(self, create_manifestarray):
+        """Test get_codecs with ManifestArray using multiple v3 codecs."""
+        test_codecs = [


This could also be a global variable TEST_CODECS, or even a fixture providing multiple sets of test codecs. See also @dcherian's codecs hypothesis strategy that we could use in future (zarr-developers/zarr-python#2822).

TomNicholas · 2025-02-13T04:20:39Z

virtualizarr/tests/test_manifests/test_array.py

-            order="C",
-            shape=(5, 1, 20),
-            zarr_format=2,
+    # FAILING: TypeError: no implementation found for 'numpy.concatenate' on types that implement __array_function__: [<class 'virtualizarr.manifests.array.ManifestArray'>, <class 'numpy.ndarray'>]


This is a bad bug and shouldn't be forgotten - it implies that somehow inside the manifest / ManifestArray concatenation implementation some array is the wrong type.

(This can't be being thrown from the top-level np.concatenate you can see in the test, so it must be being thrown by the lower-level np.concatenate call that is actually used to merge the manifests internally in .array_api.)

TomNicholas · 2025-02-13T04:26:03Z

virtualizarr/translators/kerchunk.py

+        "compressor", "chunks", and "shape".
+    Returns:
+    -------
+    ArrayV3Metadata


Suggested change

ArrayV3Metadata

ArrayV3Metadata

TomNicholas · 2025-02-13T04:27:32Z

virtualizarr/writers/icechunk.py

-        codecs = zarray._v3_codecs()
-
-        # create array if it doesn't already exist
+        # TODO: Should codecs be an argument to zarr's AsyncrGroup.create_array?


You're asking for an upstream change here right?

TomNicholas · 2025-02-13T04:28:55Z

virtualizarr/writers/kerchunk.py

@@ -67,13 +90,65 @@ def remove_file_uri_prefix(path: str):
        return path


+def convert_v3_to_v2_metadata(


Should this (a) not live in the kerchunk-specific writer module, (b) actually live in zarr-python upstream? Or is there no use for it upstream?

abarciauskas-bgse added 10 commits February 4, 2025 14:38

Added zarray_to_v3metadata and test

2a01bfa

Working on manifest array tests

17fd547

Fix test_manifests/test_array#TestConcat tests

e5666ab

Passing TestStack tests and add fixture

5a8cc4c

All test_manifests/test_array tests passing

4c0b616

Compressors should be list

ac2f787

Passing dmrpp tests

5503c60

Merge branch 'main' into manifest-arrays-use-arrayv3metadata

1272051

Passing test_hdf.py tests

1f36755

Start to work on kerchunk tests

7098803

abarciauskas-bgse had a problem deploying to test-release February 6, 2025 03:30 — with GitHub Actions Failure

abarciauskas-bgse mentioned this pull request Feb 6, 2025

ManifestArray should use zarr-python's ArrayV3Metadata #424

Open

TomNicholas reviewed Feb 6, 2025

View reviewed changes

virtualizarr/manifests/array.py Outdated Show resolved Hide resolved

virtualizarr/manifests/array.py Outdated Show resolved Hide resolved

virtualizarr/manifests/array_api.py Outdated Show resolved Hide resolved

TomNicholas added zarr-python Relevant to zarr-python upstream internals labels Feb 6, 2025

Add method to convert array v3 metadata to v2 metadata for kerchunk (…

ce2284c

…not happy about this)

abarciauskas-bgse had a problem deploying to test-release February 7, 2025 01:11 — with GitHub Actions Failure

abarciauskas-bgse commented Feb 7, 2025

View reviewed changes

virtualizarr/writers/kerchunk.py Show resolved Hide resolved

Fix fixtures and mark xfail netcdf3

c9853d5

abarciauskas-bgse had a problem deploying to test-release February 7, 2025 01:20 — with GitHub Actions Failure

Test for convert_v3_to_v2_metadata

209dae3

abarciauskas-bgse had a problem deploying to test-release February 7, 2025 15:19 — with GitHub Actions Failure

Deduplicate fixture for array v3 metadata

e7205ef

abarciauskas-bgse had a problem deploying to test-release February 7, 2025 15:22 — with GitHub Actions Failure

Parse filters and compressors from v3 metdata for v2 metadata

d65e457

abarciauskas-bgse had a problem deploying to test-release February 7, 2025 15:45 — with GitHub Actions Failure

Rewrite extract_codecs

190c20f

abarciauskas-bgse had a problem deploying to test-release February 7, 2025 20:16 — with GitHub Actions Failure

abarciauskas-bgse commented Feb 7, 2025

View reviewed changes

virtualizarr/zarr.py Outdated Show resolved Hide resolved

Refactor convert_to_codec_pipeline

47f5ddd

Min numcodecs version

c808351

abarciauskas-bgse temporarily deployed to test-release February 12, 2025 05:14 — with GitHub Actions Inactive

numcodecs>=0.15.1 in environment and upstream.yml conda env files

bd50167

abarciauskas-bgse temporarily deployed to test-release February 12, 2025 05:17 — with GitHub Actions Inactive

Working on mypy errors

ed97704

abarciauskas-bgse temporarily deployed to test-release February 12, 2025 06:32 — with GitHub Actions Inactive

Fix mypy errors and tests

a3c190e

abarciauskas-bgse temporarily deployed to test-release February 12, 2025 17:44 — with GitHub Actions Inactive

Remove ZArray class

95886b9

abarciauskas-bgse temporarily deployed to test-release February 12, 2025 18:15 — with GitHub Actions Inactive

abarciauskas-bgse added 2 commits February 12, 2025 14:07

Just return metadata's shape

a0f72b2

Create update metadata function

aad511f

abarciauskas-bgse temporarily deployed to test-release February 12, 2025 22:16 — with GitHub Actions Inactive

abarciauskas-bgse added 2 commits February 12, 2025 14:20

Fix typing for update_metadata

b357b04

Check for regular chunk grid in manifest instantiation

08e877a

abarciauskas-bgse temporarily deployed to test-release February 12, 2025 22:35 — with GitHub Actions Inactive

abarciauskas-bgse added 2 commits February 12, 2025 14:46

Remove obsolete codecs code

f040459

Fix chunks function and add docstring

495d660

abarciauskas-bgse temporarily deployed to test-release February 12, 2025 22:52 — with GitHub Actions Inactive

Remove custom zattrs type

a262f0b

abarciauskas-bgse temporarily deployed to test-release February 12, 2025 23:03 — with GitHub Actions Inactive

Move some imports and make update_metadata a private method

bcd68a0

abarciauskas-bgse temporarily deployed to test-release February 12, 2025 23:15 — with GitHub Actions Inactive

Remove zarr.py

f0ce778

abarciauskas-bgse deployed to test-release February 12, 2025 23:50 — with GitHub Actions View deployment

abarciauskas-bgse marked this pull request as ready for review February 12, 2025 23:54

abarciauskas-bgse changed the base branch from main to zarr-python-3.0 February 12, 2025 23:58

TomNicholas reviewed Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manifest arrays use arrayv3metadata #429

Manifest arrays use arrayv3metadata #429

abarciauskas-bgse commented Feb 6, 2025 •

edited

Loading

norlandrhagen commented Feb 12, 2025

abarciauskas-bgse commented Feb 12, 2025 •

edited

Loading

TomNicholas left a comment

TomNicholas Feb 13, 2025

TomNicholas Feb 13, 2025

TomNicholas Feb 13, 2025

TomNicholas Feb 13, 2025

TomNicholas Feb 13, 2025

TomNicholas Feb 13, 2025

TomNicholas Feb 13, 2025

TomNicholas Feb 13, 2025

TomNicholas Feb 13, 2025

TomNicholas Feb 13, 2025

		@@ -67,13 +90,65 @@ def remove_file_uri_prefix(path: str):
		return path


		def convert_v3_to_v2_metadata(

Manifest arrays use arrayv3metadata #429

Are you sure you want to change the base?

Manifest arrays use arrayv3metadata #429

Conversation

abarciauskas-bgse commented Feb 6, 2025 • edited Loading

Checklist

norlandrhagen commented Feb 12, 2025

abarciauskas-bgse commented Feb 12, 2025 • edited Loading

TomNicholas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abarciauskas-bgse commented Feb 6, 2025 •

edited

Loading

abarciauskas-bgse commented Feb 12, 2025 •

edited

Loading