[Draft] Non-kerchunk backend for HDF5/netcdf4 files.

[sharkinsspatial]

This is a rudimentary initial implementation for #78. The core code is ported directly from kerchunk's hdf backend. I have not ported the bulk of the kerchunk backend's specialized encoding translation logic but I'll try to do so incrementally so that we can build complete test coverage for the many edge cases it currently covers.

[TomNicholas]

This is looking great so far @sharkinsspatial !

kerchunk backend's specialized encoding translation logic

This part I would really like to either factor out, or at a least really understand what it's doing. See #68

[TomNicholas]

Does one need fsspec if reading a local file? Is there any other way to read from S3 without fsspec at all?

[rabernat]

Not with a filesystem-like API. You would have to use boto3 or aiobotocore directly.

This is one of the great virtues of fsspec and is not to be under-valued.

[TomNicholas]

I like this an a way to interface with the code in open_virtual_dataset

[rabernat]

This looks cool @sharkinsspatial!

My opinion is that it doesn't make sense to just forklift the kerchunk code into virtualizarr. What I would love to see is an extremely tight, strictly typed, unit-tested total refactor of the parsing logic. I think you're headed down the right path, but I encourage you to push as far as you can in that direction.

[sharkinsspatial]

@rabernat Fully agree with your take above 👆 👍 . I'm trying to work through this incrementally whenever I can find some spare time. In the spirit of thorough test coverage 🎊 looking through your issue pydata/xarray#7388 and the corresponding PR I'm not sure what the proper incantation of variable encoding configuration is to use blosc with the netcdf4 engine? Do you have an example of this that you can provide?

[sharkinsspatial]

@TomNicholas We're still investigating some failing test associated with inconsistencies around coordinate variable round tripping and a compatibility issue with nodata and CF time encoding. But given that this backend is still considered experimental and requires an explicit opt-in I think this PR is ok to review and merge. These are the test which are currently xfailed and need further investigation

https://github.com/zarr-developers/VirtualiZarr/blob/hdf5_reader/virtualizarr/tests/test_integration.py#L199
https://github.com/zarr-developers/VirtualiZarr/blob/hdf5_reader/virtualizarr/tests/test_readers/test_hdf_integration.py#L14

This test fails with both the kerchunk hdf reader and the experimental reader.
https://github.com/zarr-developers/VirtualiZarr/blob/hdf5_reader/virtualizarr/tests/test_readers/test_hdf_integration.py#L30

[TomNicholas]

Amazing! I'll take a more detailed look later / tomorrow / next week 😅

[TomNicholas]

I think these should be optional no? (numcodecs is still required). That also means they can be removed from the min-deps.yml in CI.

[TomNicholas]

Shouldn't your reader be added here too? Sounds like it could go under hdf rather than hdf5? Or maybe that's too subtle...

[sharkinsspatial]

Since we discussed treating this new reader as experimental while we do some further battle testing, I'm forcing users to explicitly pass the new backend class HDFVirtualBackend to the optional backend argument for open_virtual_dataset to distinguish it from using the kerchunk reader with filetype auto-detection.