Replace VirtualiZarr.ZArray with zarr ArrayMetadata #175

ghidalgo3 · 2024-07-03T00:07:48Z

Before going further with these changes, I'd like to discuss if this direction is correct or not. Based on #17, I tried using Zarr-V3's Array definition instead of VirtualiZarr's ZArray definition. Keeping the names straight is hard, ZArray is the current code and Array is zarr's type.

The problems I ran into were:

The set of allowed kwargs for Array are different between v2 and v3 for zarr, but VirtualiZarr.ZArray I believe is first created with the v2 properties and then transformed to v3 format when zarr_v3_array_metadata is called. After the replacement, anywhere VirtualiZarr creates a Array, one must choose v2 or v3 kwargs, or again introduce something that abstracts over the versions. Unless VirtualiZarr chooses to only support Zarr V3 only, which I believe is the intention.
During concatenation, VirtualiZarr checks if the codecs between ZArrays all match, which was easier when ZArray declared the codecs. Now, if you support v2 and v3 then the code checking logic has to branch based on the type of zarr.array.metadata because the internal zarr property names have changed. Probably there will be other code-level incompatibilities from metadata property name differences, I think Is the compressor type right? #94 ran into this too.

This is just from a few hours of trying this out.

If VirtualiZarr needs to support both Zarr V2 and V3 stores, then this change needs to handle all the little differences in Zarr's API between 2 and 3. But if only V3 is supported, this becomes easier and replacing ZArray with Array should not be too difficult.

Thoughts?

Incompatibilities:

ArrayV3Metadata does not declare storage_transformers zarr-python#2009
v2->v3 zarr.meta module name change breaks imports zarr-python#2021

To be filled later:
Closes #xxxx
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

rabernat · 2024-07-03T11:52:48Z

Just wanted to say that the Zarr V3 class behaviors are not set in stone yet. If you have feedback on the V3 API and potential incompatibilities between V2, please report them issues on Zarr-python: https://github.com/zarr-developers/zarr-python/issues

jsignell · 2024-07-03T20:06:15Z

My sense is that in terms of file format both zarr v2 and zarr v3 should be supported but in terms of library dependencies it would be fine to mandate that only zarr-python >= 3 is supported.

TomNicholas · 2024-07-03T20:18:06Z

Thanks for taking the initiative here @ghidalgo3 !

I tried using Zarr-V3's Array definition instead of VirtualiZarr's ZArray definition

I was actually hoping we could use some class from zarr-python that wasn't a fully-featured zarr.Array, instead using something that just represented the metadata for one array. This ArrayV3Metadata class might be what we need? Is that public API?

If VirtualiZarr needs to support both Zarr V2 and V3 stores, then this change needs to handle all the little differences in Zarr's API between 2 and 3. But if only V3 is supported, this becomes easier and replacing ZArray with Array should not be too difficult.

I see two ways to do this:

Make VirtualiZarr internally agnostic to v2 vs v3. Then a ManifestArray can represent either a v2 array or a v3 array, distinguished via its metadata. For example the attributes of a ManifestArray might become
```
from zarr.metadata import ArrayMetadata

class ManifestArray:
    _chunkmanifest: ChunkManifest
    _zarray: ArrayMetadata
```
where ArrayMetadata is an ABC that needs to be a concrete ArrayV2Metadata or ArrayV3Metadata.
VirtualiZarr internally only uses v3 arrays, and whilst we support reading zarr and writing zarr v2 arrays, they are coerced at reading / writing time.

I'm not sure which of these two is the best approach.

I also completely agree with @jsignell - all of this should be supported through only a dependency on zarr>=3.0.0, as this PR adds.

ghidalgo3 · 2024-07-08T19:45:33Z

To minimize the code changes needed in VirtualiZarr, I'll attempt the first option.

ghidalgo3 · 2024-07-09T17:30:44Z

Adding a dependency on zarr==3 causes a problem for Kerchunk. Kerchunk expects to be able to import zarr.meta but in ZarrV3, that code was moved to the v2 directory such that Kerchunk has to do from zarr.v2.meta import encode_fill_value instead of from zarr.meta import encode_fill_value. Then this causes unit tests to fail in VirtualiZarr.

See the reorg that happened here about 3 months ago for ZarrV3: zarr-developers/zarr-python#1809 and some attempts in Kerchunk to support ZarrV3:

Maybe the v2 module name is temporary in Zarr? But until then, any tests that call into Kerchunk are broken :(.

rabernat · 2024-07-09T17:52:41Z

@ghidalgo3 please please open a Zarr issue to track any API incompatibilities between V2 and V3. 🙏

https://github.com/zarr-developers/zarr-python/issues

TomNicholas · 2024-07-09T18:31:57Z

@ghidalgo3 it might be a good idea to have a separate first PR that makes virtualizarr pass every test using zarr v3, and then coming back to this PR once that compatibility can be relied upon. Otherwise this might become a rabbit hole.

d-v-b · 2024-07-09T20:11:44Z

I was actually hoping we could use some class from zarr-python that wasn't a fully-featured zarr.Array, instead using something that just represented the metadata for one array. This ArrayV3Metadata class might be what we need? Is that public API?

That class is public API, but it might be changing soon, and while it is intended for this use case it also has some extra stuff that goes beyond "representing metadata". So it should do what you want, eventually :)

TomNicholas

Great work so far @ghidalgo3 !

TomNicholas · 2024-07-10T01:02:37Z

virtualizarr/manifests/array.py

+        """
+        Individual chunk size by number of elements.
+        """
+        if isinstance(self._zarray.chunk_grid, RegularChunkGrid):


this is a nice check, but I think we could actually perform this check at ManifestArray construction time, because right now a lot of other things will break if the chunk grid is not regular.

TomNicholas · 2024-07-10T01:04:25Z

virtualizarr/manifests/array_api.py

+        match zarray:
+            case ArrayV2Metadata(compressor=compressor, filters=filters):
+                return Codec(compressor=compressor, filters=filters)
+            case ArrayV3Metadata(codecs=codecs):
+                return codecs
+            case _:
+                raise ValueError("Unknown ArrayMetadata type")


Is there a reason to prefer this match...case over a standard if isinstance()...else syntax?

Mainly a style preference coming from languages with discriminated unions and compilers, but if you'd like the whole code base to be consistent I can rewrite it with isinstance.

TomNicholas · 2024-07-10T01:06:54Z

virtualizarr/tests/__init__.py

        chunks=chunks,
-        compressor="zlib",
+        compressor={"id": "zlib"},


Is the presence of "id" a v2 vs v3 difference? because you don't seem to use it in the other tests

In V3, the codecs array is made up of (name, configuration) objects, but in V2 the id field serves as the name. In this particular test, I just chose to convert the ZArray to an ArrayV2Metadata

Actually please ignore any of the changes in tests for now because I will need to edit almost every test to excise ZArray :) The few changes I added were done up until I hit the kerchunk module import issue

TomNicholas · 2024-07-10T01:08:18Z

virtualizarr/zarr.py

 class Codec(BaseModel):
-    compressor: str | None = None
+    """
+    ZarrayV2 codec definition.
+    """
+
+    compressor: str | dict[str, Any] | None = None
+    """
+    If it's a string, it's the compressor ID.
+    If it's a dict, it's the full compressor configuration.
+    """
    filters: list[dict] | None = None

    def __repr__(self) -> str:
        return f"Codec(compressor={self.compressor}, filters={self.filters})"


Should this class be moved upstream?

I'm not sure, because ZarrV3 now has a notion of codecs which is different from VirtualiZarr's notion of codecs. A VirtualiZarr codec is really the combination of filters and compressor from V2 metadata, which are easily transformable to V3 codecs.

Maybe a zarr.ArrayV2Metadata can have a property that projects the compressor and filters into V3 Codecs, @d-v-b thoughts on that? In that case, Codec could even be a property of the ABC ArrayMetadata.

I actually think that codecs: ListWithInternalStructure[Codec] (i.e., what zarr v3 metadata uses) is not the best interface. See a writeup on that point of view here.

But if we ignore that concern for a moment, I'm curious about @TomNicholas's question: what could we change in zarr-python so that you could just import a functionally equivalent class from there (or import a dataclass and wrap with pydantic)? I have a selfish interest in this question since I'm likely going to follow in your footsteps when I implement better codec support over in pydantic-zarr.

Thanks for forcing me to read the Zarr code more closely! I found VirtualiZarr could probably just use create_codec_pipeline to turn V2 or V3 metadata into a BatchedCodecPipeline and then VirtualiZarr can just deal with CodecPipeline as the abstraction over V2 and V3 . Additionally, VirtualiZarr could change its Codec to use V2Filters and V2Compressor from Zarr, or I might get rid of it entirely.

what does Virtualizarr do with the codecs?

All virtualizarr currently needs to do with the codecs is:

Check that any two arrays you ask it to concatenate have the same codecs (see Virtual concatenation of arrays with different codecs or dtypes #5)

Make sure the codecs are recorded in the kerchunk references when they get written out to disk.

…ghidalgo3/VirtualiZarr into guhidalgo/replaceinternalzarray

Try out replacing ZArray

5e12b88

ghidalgo3 temporarily deployed to test-release July 3, 2024 15:01 — with GitHub Actions Inactive

Undo changes

92a7e81

ghidalgo3 mentioned this pull request Jul 9, 2024

v2->v3 zarr.meta module name change breaks imports zarr-developers/zarr-python#2021

Open

TomNicholas added the zarr-python Relevant to zarr-python upstream label Jul 9, 2024

try using zarr array metadata

42b3f3a

ghidalgo3 mentioned this pull request Jul 10, 2024

Add Zarr v3 dependency #182

Draft

9 tasks

TomNicholas reviewed Jul 10, 2024

View reviewed changes

TomNicholas changed the title ~~Replace VirtualiZarr.ZArray with zarr.array.Array~~ Replace VirtualiZarr.ZArray with zarr ArrayMetadata Jul 10, 2024

ghidalgo3 temporarily deployed to test-release July 10, 2024 01:10 — with GitHub Actions Inactive

ghidalgo3 and others added 3 commits July 9, 2024 21:41

Merge branch 'main' into guhidalgo/replaceinternalzarray

c8c9020

more RegularChunkGrid validation to constructor

5fa1dea

Merge branch 'guhidalgo/replaceinternalzarray' of https://github.com/…

cea2214

…ghidalgo3/VirtualiZarr into guhidalgo/replaceinternalzarray

ghidalgo3 temporarily deployed to test-release July 10, 2024 15:22 — with GitHub Actions Inactive

TomNicholas mentioned this pull request Jul 30, 2024

Nit: __post_init__ vs pydantic @model_validator(mode="after") #204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace VirtualiZarr.ZArray with zarr ArrayMetadata #175

Replace VirtualiZarr.ZArray with zarr ArrayMetadata #175

ghidalgo3 commented Jul 3, 2024 •

edited

Loading

rabernat commented Jul 3, 2024

jsignell commented Jul 3, 2024

TomNicholas commented Jul 3, 2024 •

edited

Loading

ghidalgo3 commented Jul 8, 2024

ghidalgo3 commented Jul 9, 2024

rabernat commented Jul 9, 2024

TomNicholas commented Jul 9, 2024

d-v-b commented Jul 9, 2024

TomNicholas left a comment

TomNicholas Jul 10, 2024

TomNicholas Jul 10, 2024

ghidalgo3 Jul 10, 2024

TomNicholas Jul 10, 2024

ghidalgo3 Jul 10, 2024

TomNicholas Jul 10, 2024

ghidalgo3 Jul 10, 2024

d-v-b Jul 10, 2024

ghidalgo3 Jul 10, 2024

d-v-b Jul 10, 2024

TomNicholas Jul 10, 2024

Replace VirtualiZarr.ZArray with zarr ArrayMetadata #175

Are you sure you want to change the base?

Replace VirtualiZarr.ZArray with zarr ArrayMetadata #175

Conversation

ghidalgo3 commented Jul 3, 2024 • edited Loading

rabernat commented Jul 3, 2024

jsignell commented Jul 3, 2024

TomNicholas commented Jul 3, 2024 • edited Loading

ghidalgo3 commented Jul 8, 2024

ghidalgo3 commented Jul 9, 2024

rabernat commented Jul 9, 2024

TomNicholas commented Jul 9, 2024

d-v-b commented Jul 9, 2024

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

Choose a reason for hiding this comment

ghidalgo3 commented Jul 3, 2024 •

edited

Loading

TomNicholas commented Jul 3, 2024 •

edited

Loading