`zarr-python` v3 compatibility #516

mpiannucci · 2024-10-10T17:33:36Z

So far the only tested file type is HDF5. Thats the only module that currently works with new zarr python in some way

martindurant

There's much less change here than I might have thought.

kerchunk/tests/test_hdf.py

kerchunk/hdf.py

martindurant · 2024-10-10T17:49:06Z

kerchunk/hdf.py

@@ -496,6 +516,8 @@ def _translator(
                        if h5obj.fletcher32:
                            logging.info("Discarding fletcher32 checksum")
                            v["size"] -= 4
+                        key =  str.removeprefix(h5obj.name, "/") + "/" + ".".join(map(str, k))


This is the same as what _chunk_key did? Maybe make it a function with a comment saying it's a copy/reimplementation.

By the way, is h5obj.name not actually a string, so you could have done h5obj.name.removeprefix()?

martindurant · 2024-10-10T17:50:41Z

kerchunk/hdf.py

                    shape=h5obj.shape,
                    dtype=dt or h5obj.dtype,
                    chunks=h5obj.chunks or False,
                    fill_value=fill,
-                    compression=None,
+                    compressor=None,


So here, you could reintroduce the compressor

filters = filters[:-1] compressor = filters[-1]

but obviously it depends on whether there are indeed any filters at all.

It would still need back compat, since filters-only datasts definitely exist.

yeah the big issue is that v3 cares about what type of operation it is, and v2w doesnt so moving them around doesnt necessarily fix that bug

So there needs to be a change upstream?

Yes this: zarr-developers/zarr-python#2325

martindurant · 2024-10-10T17:51:36Z

kerchunk/hdf.py

+            for k, v in self.store_dict.items():
+                if isinstance(v, zarr.core.buffer.cpu.Buffer):
+                    key = str.removeprefix(k, "/")
+                    new_keys[key] = v.to_bytes()
+                    keys_to_remove.append(k)
+            for k in keys_to_remove:
+                del self.store_dict[k]


This is the hacky bit and could use some explanations. Even when requesting "v2", zarr makes Buffer objects, and the keys are also wrong?

Yeah so two issues here:

the keys we get from hdf are for example /depth/.zarray when then need to be depth/.zarray

we cant jsonify buffers, which is how the internal MemoryStore in v3 stores its data. So we need to convert the buffers to bytes to be serialized

OK - would appreciate comments on the code saying this.

kerchunk/hdf.py

kerchunk/tests/test_hdf.py

mpiannucci · 2024-10-15T18:37:23Z

Also worth noting... zarr 3 doesn't support numcodecs codecs out of the box. There is a pr to help this zarr-developers/numcodecs#524 (see also here for updated version zarr-developers/numcodecs#597) but it would mean a change to codec names which causes an incompatibility. For the initial icechunk examples we handle this in virtualizarr but long term it probably belongs here to work standalone.

mpiannucci · 2024-10-21T13:18:19Z

When this zarr-developers/zarr-python#2425 goes in it should unblock this to work full with zarr python v3.

We will also need to create both numcodecs and zarr v3 codec versions of all the custom kerchunk codecs so that a given dataset can be loaded in either v2 or v3 contexts (say if you kerchunk a grib file, then want to convert those references to an icechunk store)

martindurant · 2024-10-21T13:20:47Z

create both numcodecs and zarr v3 codec versions of all the custom kerchunk codecs

Is that a subclass in each case, specifying that it is to be bytes->bytes?

mpiannucci · 2024-10-21T13:22:52Z

Sorry the numcodecs versions exists as they are today. Yes the v3 version would be basically subclassing zarr.abc.codec using the numcodecs implementations of the kerchunk codecs.

Although grib decoder should really be bytes to array i think

mpiannucci · 2024-10-23T12:52:00Z

This is required upstream: fsspec/filesystem_spec#1734

mpiannucci · 2024-10-23T13:30:22Z

Also of note: To get this to work with zarr 3, we pass an fsspec ReferenceFilesystem to a zarr RemoteStore. This works fine with remote filesystems (where the data files live) that have async implementations (s3, http, etc) but does not work when the data files are on filesystem with only a sync implementation (local, etc). The tests heavily depend on the local filesystem, and i'm sure many others do as well so it needs to be figured out how the interaction of sync filesystems and the async zarr RemoteStore requirement work out

mpiannucci · 2024-10-23T14:18:24Z

Grib works as long as read in zarr 2 format.

To be used with zarr 3 the codec needs to be ported over to the new abc.Codec class from zarr

martindurant · 2025-01-29T21:47:45Z

OK, only failures now are in test__grib_idx.py , which use datatree. I think this is to do with a leading "/" in the paths...

maxrjones · 2025-01-29T22:03:35Z

OK, only failures now are in test__grib_idx.py , which use datatree. I think this is to do with a leading "/" in the paths...

🎉 exciting progress! DataTree support is not yet supported fully for Zarr-Python 3, see pydata/xarray#9984. I think a conditional xfail would be the way to go here.

pyproject.toml

martindurant · 2025-01-29T22:13:28Z

I don't know how much effort we want to spend finding the deadlock triggered by the HDF5 embed test that is now commented out.

moradology · 2025-01-29T22:13:46Z

I'd been bashing my head against those grib tests all day and couldn't figure out why, even in a debugger and doing things manually, none of the DataTree nodes had any data...

In the process of messing with this I uncovered what may be a FsspecStore bug in its list method: if the path it is given is "", it will replace all "/" with "" and return some very odd results e.g.
Store keys: ['.zattrs', '.zgroup', 'u.zattrs', 'u.zgroup', 'uinstant.zattrs', 'uinstant.zgroup', 'uinstantheightAboveGround.zattrs', 'uinstantheightAboveGround.zgroup', 'uinstantheightAboveGroundheightAboveGround.zarray', 'uinstantheightAboveGroundheightAboveGround.zattrs', 'uinstantheightAboveGroundheightAboveGround0', 'uinstantheightAboveGroundlatitude.zarray', 'uinstantheightAboveGroundlatitude.zattrs', 'uinstantheightAboveGroundlatitude0.0', 'uinstantheightAboveGroundlongitude.zarray', 'uinstantheightAboveGroundlongitude.zattrs', 'uinstantheightAboveGroundlongitude0.0', 'uinstantheightAboveGroundstep.zarray', 'uinstantheightAboveGroundstep.zattrs', 'uinstantheightAboveGroundstep0', 'uinstantheightAboveGroundtime.zarray', 'uinstantheightAboveGroundtime.zattrs', 'uinstantheightAboveGroundtime0', 'uinstantheightAboveGroundu.zarray', 'uinstantheightAboveGroundu.zattrs', 'uinstantheightAboveGroundu0.0.0.0', 'uinstantheightAboveGroundvalid_time.zarray', 'uinstantheightAboveGroundvalid_time.zattrs', 'uinstantheightAboveGroundvalid_time0.0']

moradology · 2025-01-29T22:17:49Z

Might want to add this small update as well, since it better captures the intent of the version check so far as I can tell: mpiannucci#6

martindurant · 2025-01-29T22:24:47Z

In the process of messing with this I uncovered what may be a FsspecStore bug in its list method: if the path it is given is "", it will replace all "/" with "" and return some very odd results

!!! Please file this as an issue. Maybe it helps with the datatree thing too.

TomNicholas · 2025-01-29T22:33:14Z

Great to see this progress!

The preceding / bug was discussed at length in the Xarray meeting earlier today. We will fix it, but if you can xfail it for now then that would suit us in VirtualiZarr.

martindurant · 2025-01-29T23:01:38Z

@TomNicholas , I am skipping that whole test module - all is green

TomNicholas · 2025-01-29T23:10:40Z

Wow! Okay please let us know when you release this then ❤️

moradology · 2025-01-30T15:34:05Z

@martindurant Not a huge deal, but I noticed that this PR didn't make it in: mpiannucci#6

Should I cut it against kerchunk main or nah?

martindurant · 2025-01-30T15:38:28Z

Actually, it looks like that's now dead code. Following this PR, kechunk only works with zarr3, for better or worse.

mpiannucci added 10 commits October 4, 2024 16:37

Save progress for next week

39722e7

Bump zarr python version

d3c7e37

Get some tests working others failing

25d7d14

get through single hdf to zarr

ffe5f9d

Save progress

5aef233

Cleanup, almost working with hdf

b9323d2

Closer...

0f17119

Updating tests

5c8806b

reorganize

80fedcd

Save progress

1f69a0b

martindurant reviewed Oct 10, 2024

View reviewed changes

kerchunk/tests/test_hdf.py Outdated Show resolved Hide resolved

mpiannucci added 5 commits October 10, 2024 15:30

Refactor to clean things up

d556e52

Fix circular import

b27e64c

Iterate

41d6e8e

Change zarr dep

7ade1a6

More conversion

492ddee

mpiannucci mentioned this pull request Oct 12, 2024

Virtual Dataset Workflow Tracking Issue earth-mover/icechunk#197

Closed

5 tasks

Specify zarr version

6e5741c

TomNicholas mentioned this pull request Oct 17, 2024

Make kerchunk dependency entirely optional zarr-developers/VirtualiZarr#258

Closed

mpiannucci added 3 commits October 23, 2024 09:31

Working remote hdf tests

c0316ac

Working grib impl

59bd36c

Add back commented out code

187ced2

TomNicholas mentioned this pull request Jan 29, 2025

Vendor kerchunk netCDF3 reader zarr-developers/VirtualiZarr#397

Merged

7 tasks

martindurant added 7 commits January 29, 2025 15:13

more fix!

9b96d8c

env typo

17478bd

Add HDF4 simple test

2d5033c

fix other runs

9066360

comma

4750f8e

lint

73385f5

CI deps

1a79a5c

maxrjones reviewed Jan 29, 2025

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

skip tests needing datatree

f51604f

martindurant marked this pull request as ready for review January 29, 2025 22:10

This comment was marked as outdated.

Sign in to view

TomNicholas mentioned this pull request Jan 29, 2025

Add astropy for fits test zarr-developers/VirtualiZarr#403

Closed

martindurant merged commit d2b00a9 into fsspec:main Jan 30, 2025
4 checks passed

jsignell mentioned this pull request Jan 30, 2025

Roundtrip dict and json kerchunk tests without depending on kerchunk library zarr-developers/VirtualiZarr#404

Closed

abarciauskas-bgse mentioned this pull request Feb 1, 2025

Replace ZArray with zarr.core.array.ArrayMetadata zarr-developers/VirtualiZarr#411

Closed

sharkinsspatial mentioned this pull request Feb 3, 2025

Updating fill_value logic for HDFVirtualBackend zarr-developers/VirtualiZarr#414

Open

aufdenkampe mentioned this pull request Feb 17, 2025

Explore integration with Icechunk data engine LimnoTech/Xarray-DataAccessor#5

Open

groutr mentioned this pull request Feb 19, 2025

Remove log setup #543

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`zarr-python` v3 compatibility #516

`zarr-python` v3 compatibility #516

mpiannucci commented Oct 10, 2024 •

edited

Loading

martindurant left a comment

martindurant Oct 10, 2024

martindurant Oct 10, 2024

mpiannucci Oct 10, 2024

martindurant Oct 15, 2024

mpiannucci Oct 15, 2024

martindurant Oct 10, 2024

mpiannucci Oct 10, 2024

martindurant Oct 15, 2024

mpiannucci commented Oct 15, 2024 •

edited

Loading

mpiannucci commented Oct 21, 2024 •

edited

Loading

martindurant commented Oct 21, 2024

mpiannucci commented Oct 21, 2024 •

edited

Loading

mpiannucci commented Oct 23, 2024

mpiannucci commented Oct 23, 2024 •

edited

Loading

mpiannucci commented Oct 23, 2024

martindurant commented Jan 29, 2025

maxrjones commented Jan 29, 2025

martindurant commented Jan 29, 2025

moradology commented Jan 29, 2025

moradology commented Jan 29, 2025

martindurant commented Jan 29, 2025

This comment was marked as outdated.

TomNicholas commented Jan 29, 2025

martindurant commented Jan 29, 2025

TomNicholas commented Jan 29, 2025

moradology commented Jan 30, 2025

martindurant commented Jan 30, 2025

zarr-python v3 compatibility #516

zarr-python v3 compatibility #516

Conversation

mpiannucci commented Oct 10, 2024 • edited Loading

martindurant 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

mpiannucci commented Oct 15, 2024 • edited Loading

mpiannucci commented Oct 21, 2024 • edited Loading

martindurant commented Oct 21, 2024

mpiannucci commented Oct 21, 2024 • edited Loading

mpiannucci commented Oct 23, 2024

mpiannucci commented Oct 23, 2024 • edited Loading

mpiannucci commented Oct 23, 2024

martindurant commented Jan 29, 2025

maxrjones commented Jan 29, 2025

martindurant commented Jan 29, 2025

moradology commented Jan 29, 2025

moradology commented Jan 29, 2025

martindurant commented Jan 29, 2025

This comment was marked as outdated.

TomNicholas commented Jan 29, 2025

martindurant commented Jan 29, 2025

TomNicholas commented Jan 29, 2025

moradology commented Jan 30, 2025

martindurant commented Jan 30, 2025

`zarr-python` v3 compatibility #516

`zarr-python` v3 compatibility #516

mpiannucci commented Oct 10, 2024 •

edited

Loading

mpiannucci commented Oct 15, 2024 •

edited

Loading

mpiannucci commented Oct 21, 2024 •

edited

Loading

mpiannucci commented Oct 21, 2024 •

edited

Loading

mpiannucci commented Oct 23, 2024 •

edited

Loading