Migrate hdmf-zarr from zarr-python v2 to v3#325
Conversation
Port all code to zarr-python v3 API (zarr>=3.1.0), writing zarr v3 format while maintaining ability to read existing v2 format files. Key changes: - Replace DirectoryStore/NestedDirectoryStore/TempStore with LocalStore - Replace FSStore with FsspecStore - Replace object_codec (numcodecs.Pickle/JSON) with StringDType + JSON serialization - Replace zarr.copy() with custom _copy_array() (not yet implemented in v3) - Deprecate synchronizer parameter (removed in v3) - Replace require_dataset/create_dataset with require_array/create_array - Add Array.__len__ and scalar indexing monkey-patches for v2 compat - Eagerly load StringDType arrays for cross-backend compatibility - Categorize numcodecs.Shuffle as BytesBytesCodec (compressor, not filter) - Fix LocalStore path resolution (store.root vs str() URI) - Decode bytes before writing to StringDType arrays - Update consolidated metadata handling for v3 API - Update all tests and documentation All 454 unit tests pass. Closes #202 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix ruff lint: move monkey-patches after imports (E402), remove unused
imports (tempfile, numcodecs), fix long lines, remove unused variable
- Bump minimum Python to 3.11 (zarr v3.1.0 requires >=3.11)
- Update CI workflows and tox.ini: replace py310 with py311
- Fix gallery examples: replace NestedDirectoryStore with LocalStore,
replace .compressor with .compressors
- Fix read_nwb: use storage_options=None instead of {} to avoid
importing fsspec when not needed
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #325 +/- ##
==========================================
- Coverage 85.72% 84.00% -1.72%
==========================================
Files 5 5
Lines 1261 1388 +127
Branches 239 278 +39
==========================================
+ Hits 1081 1166 +85
- Misses 123 146 +23
- Partials 57 76 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zarr 3.1.0 requires numpy>=1.26 and numcodecs>=0.14. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- zarr.codecs.numcodecs module added in zarr 3.1.3 (not in 3.1.0) - Add warning filters for ZarrUserWarning (numcodecs codecs, consolidated metadata) and UnstableSpecificationWarning in gallery tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
hdmf DtypeConversionWarning triggers when zarr arrays with object dtype are converted during export (e.g., electrode references). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dtype guards - Fix tox wheelinstall by removing invalid `extras = null` (use empty value) - Update broken zarr docs links (DirectoryStore → LocalStore, consolidated metadata URL) - Skip fsspec streaming tests (S3 test files use zarr v2 object_codec) - Bump hdmf minimum to 4.2.0 (fixes dtype=numpy.ndarray in export) - Add np.ndarray dtype guards in __list_fill__ and __scalar_fill__ - Handle object dtype in _copy_array by converting to StringDType Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@bendichter thanks for taking a crack at this. I'll take a look. Do you know if this is compatible with reading existing Zarr v2 files? I think it is fine to drop Zarr v2 support for writing, but it would be great to have a pathway to maintain read for existing v2 data. |
|
@oruebel Thanks for the review! Here's a summary of the changes in the latest commit: Re: zarr v2 read compatibility - zarr-python v3 can transparently read zarr v2 format files as long as they don't use Changes in this commit addressing your review:
|
- Remove synchronizer parameter from ZarrIO and NWBZarrIO entirely - Remove object_codec_class parameter from ZarrIO entirely - Remove _numcodec_to_zarr_v3 compat wrapper - ZarrDataIO now requires zarr v3 codecs (BytesBytesCodec/ArrayArrayCodec) - Update hdf5_to_zarr_filters to produce zarr v3 wrapped codecs directly - Restore removed comment about orphaned containers in zarr_utils.py - Update tests to use zarr.codecs instead of numcodecs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…xamples - Remove unused numcodecs import from utils.py (ruff F401) - Update gallery tutorial to use zarr.codecs.BloscCodec instead of numcodecs.Blosc - Update storage.rst to describe zarr v3 JSON-serialized references Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace hardcoded U256 fixed-length strings in compound dtypes with dynamically computed sizes based on actual data content. This prevents truncation of JSON-serialized references that could exceed 256 chars for deeply nested paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Just to clarify, does Zarr v3 fail to read 1) any v2 file that contains If it is 1) then I think that would mean that no NWB Zarr file would be readable, because of the cached spec. I think in that case we may need some form of a migration tool. If it is 2) then I think the main problem cases are:
|
|
what NWB datasets currently use Zarr v2? |
|
I tested this systematically. It's scenario 2 — zarr v3 fails on individual # These work fine:
root = zarr.open_group(store, mode='r') # opening the group succeeds
root['normal_data'][:] # accessing non-object arrays by name works
root['subgroup'] # accessing subgroups by name works
# These fail:
root['spec_dataset'] # accessing an object_codec array by name fails
root.arrays() # iterating arrays fails if ANY array has object_codec
root.groups() # iterating groups also fails (same underlying iteration)
root.members() # sameSince hdmf-zarr's We have a few options:
I think option 1 is reasonable for this PR since it's already a major release. We could file a follow-up issue for v2 read compatibility. What do you think? |
@bendichter Any of the Allen Institute for Neural Dynamics (AIND) datasets should be Zarr v2. However, I am not sure if they have published those on DANDI. If there are no NWB Zarr files on DANDI, then I think the migration problem may be mainly an AIND-specific issue.
I agree that this doesn't necessarily need to be part of this PR. However, we should have some answer for the migration question for the major release. |
|
@oruebel I know AIND is one of the main groups pushing for Zarr v3, so I think from their perspective they would rather us push forward with this |
At the very least, I think it would be useful to add some discussion in the docs on:
|
| dset = tempIO._file["test_dataset"] | ||
| self.assertEqual(dset["a"].tolist(), data["a"].tolist()) | ||
| self.assertEqual(dset["b"].tolist(), data["b"].tolist()) | ||
| # In zarr v3, field-name indexing on Array is not supported; read all then index |
There was a problem hiding this comment.
May be something to add to the https://hdmf-zarr.readthedocs.io/en/stable/overview.html#known-limitations
There was a problem hiding this comment.
Added to known limitations in 8a0195d — documented both the FixedLengthUTF32 compound dtype limitation and the zarr v3-only write format.
Restore write_link Case 1/2 comments and __setup_chunked_dataset__ docstring. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The dtype is always set from either options["dtype"] (when not None) or data.dtype, so the None fallback is unreachable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The zarr v3 docs moved the storage API page from /api/zarr/storage.html to /user-guide/storage/. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| HAVE_FSSPEC = check_s3fs_ffspec_installed() | ||
|
|
||
|
|
||
| @unittest.skip("S3 test files are in zarr v2 format with object_codec which is not supported by zarr v3") |
There was a problem hiding this comment.
@bendichter we should add a Zarr V3 test dataset to DANDI to test streaming read. Maybe as part of https://dandiarchive.org/dandiset/001083. We should at least file an issue for this.
In addition, as part of this PR, is there a way to check that a file is V2 when we try to access it (probably in ZarrIO.open or ZarrIO.__init__) to check that the file is V2 and if it is V2 raise an error. If reading V2 is not supported then we should raise an error as early as possible to avoid confusion.
hasattr(data, "__len__") duck typing with ndim-based collection detection
hdmf-dev/hdmf#1414
Resolve merge conflicts from dev CI/dependency improvements: - backend.py: Keep v3 FsspecStore-based is_remote() (no ConsolidatedMetadataStore in zarr v3) - tox.ini: Adopt uv --resolution lowest-direct approach with py311 minimum - requirements-min.txt: Accept deletion (replaced by uv resolution) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Zarr 3.1.3 doesn't support Python 3.10, so the required checks will need to be bumped to 3.11 for this PR |
|
note, that it seems that hdmf-zarr restriction of numcodes seems to forbid dandi-cli to come to the bright future of python 3.14: Is there a time line for this PR or should we look into alternative approaches on how to unblock us? (e.g. proposing hdmf-zarr to become an optional dependency, or alike) |
We discussed this on Monday. NeurodataWithoutBorders/nwbinspector#698 should unblock dandi |
|
This PR is tested against also in SpikeInterface (all green!): SpikeInterface/spikeinterface#4260 Also, here is a WIP PR in neuroconv to configure a Zarr v3 backend with new codecs and sharding: catalystneuro/neuroconv#1749 |
| # Eagerly load StringDType arrays so other backends (e.g. HDF5IO) can handle them. | ||
| # Must be after reference checks since references are also stored as StringDType. | ||
| elif isinstance(zarr_obj.dtype, np.dtypes.StringDType) and dtype != "scalar": | ||
| data = list(zarr_obj[:]) |
There was a problem hiding this comment.
Reading any non-scalar string dataset now does data = list(zarr_obj[:]). For a large 1-D text column (e.g. millions of entries) this pulls the whole dataset into a Python list on open, defeating zarr's chunked/lazy access and risking OOM. Worse, for a 2-D string dataset list(zarr_obj[:]) yields a list of row ndarrays, so a consumer doing builder.data[i, j] raises TypeError: list indices must be integers or slices, not tuple, and .shape/.dtype are gone. Consider a lazy decoding wrapper (like the reference-dataset wrappers just above) instead of full materialization.
There was a problem hiding this comment.
Good catch — fixed in b23a49a.
Replaced the eager data = list(zarr_obj[:]) with a lazy ZarrStringDataset wrapper (in zarr_utils.py, mirroring the reference-dataset wrappers just above), so string datasets are no longer materialized on open:
- Lazy — wrapping reads nothing; elements are decoded to native
stronly on access, preserving zarr's chunked access and avoiding the OOM risk on large text columns. - 2-D correctness —
data[i, j]now returns the decoded scalar (previouslyTypeError: list indices must be integers or slices, not tuple), and.shape/.dtype/len()are preserved. - Export — reports an object dtype so HDMF infers
utf8on re-build; Zarr→HDF5 and Zarr→Zarr round-trips verified.
(Root cause of why the eager cast was there: for generic dtype=None specs like VectorData, HDMF's __check_edgecases doesn't recognize np.dtypes.StringDType and falls through to value.dtype.type = python str, which HDF5IO can't write. The wrapper's object dtype steers inference to utf8 instead.)
Added TestZarrStringDataset covering laziness, shape/dtype/len preservation, scalar/slice decoding, and 2-D [i, j] indexing.
There was a problem hiding this comment.
Correction / follow-up (b3dbd30): the lazy ZarrStringDataset wrapper I described above broke export of typed text columns (spec dtype text, e.g. electrodes.location) and failed the plot_convert_nwb_hdf5 gallery job. Root cause: HDMF's convert_dtype only recognizes zarr.Array / ndarray / StrDataset / list / DataChunkIterator for string specs, so a custom wrapper falls through to _unicode(wrapper) and raises Expected unicode or ascii string. A raw zarr StringDType array doesn't work either: its dtype kind is "T", which HDMF's generic (dtype=None) inference doesn't recognize, so it infers python str and HDF5IO can't create the dataset.
I've replaced the wrapper with decoding to a numpy object array of native str on read. That is recognized by HDMF for both typed and generic specs, preserves shape/dtype, and supports N-D data[i, j] indexing (which list(zarr_obj[:]) did not). It is still materialized on open — genuinely lazy access isn't achievable without upstream HDMF support for StringDType (or wrapping as a DataChunkIterator, which is a much larger change and unsuitable as the read representation). Left a code comment to that effect; happy to file an HDMF issue for the upstream fix if you'd like.
| if isinstance(path, Path): | ||
| path = str(path) | ||
| # Convert local paths to absolute for consistent path resolution | ||
| if isinstance(path, str) and not path.startswith(("s3://", "http://", "https://")): |
There was a problem hiding this comment.
Any other fsspec protocol (gcs://, gs://, abfs://, az://) fails the check and gets rewritten to /cwd/gcs:/bucket/file.zarr, so the subsequent FsspecStore.from_url receives a corrupted URL and the open fails or points at the wrong location. A GCS/Azure user passing storage_options cannot open their file.
There was a problem hiding this comment.
Fixed in ed79899. The check now detects any protocol URL generically via "://" not in path instead of an s3/http allowlist, so gcs://, gs://, abfs://, az://, and chained fsspec protocols (e.g. simplecache::s3://) are left untouched while local/relative paths are still made absolute. Added TestPathNormalization covering all these protocols.
| kwargs['fill_value'] = source.fill_value | ||
|
|
||
| dest = dest_group.create_array(**kwargs) | ||
| dest[:] = source[:] |
There was a problem hiding this comment.
The replaced zarr.copy() streamed chunk-by-chunk. During export of a multi-GB dataset (e.g. a large ElectricalSeries), this now decompresses the whole array into one in-memory ndarray before writing, which can OOM. Iterate over chunk regions (for sl in ...: dest[sl] = source[sl]) to bound memory to a single chunk. (zarr v3's zarr.from_array(...) also does this and would copy codecs/fill_value for you.)
There was a problem hiding this comment.
Fixed in ed79899. _copy_array now copies one chunk region at a time (itertools.product over chunk-aligned offsets → dest[region] = source[region]) so peak memory is bounded to a single chunk instead of decompressing the whole array into one ndarray. I kept the explicit copy rather than zarr.from_array since this method already handles the object→StringDType conversion and copies compressors/filters/fill_value/attrs; the chunk loop reuses dest.chunks (== source.chunks). Added TestCopyArray covering multi-chunk numeric, object→StringDType, and scalar (0-d) copies.
Reading a non-scalar string dataset did `data = list(zarr_obj[:])`, which pulled the whole column into memory on open (risking OOM for large text columns) and, for 2-D string datasets, collapsed to a list of row arrays that no longer supported `data[i, j]` indexing or exposed `shape`/`dtype`. Replace this with a lazy `ZarrStringDataset` wrapper (mirroring the reference-dataset wrappers) that decodes elements to native `str` on demand, reports an object dtype so HDMF infers `utf8` on re-build, and preserves `shape`, `dtype`, `__len__`, and N-D indexing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two export/read fixes flagged in review: - Path normalization only excluded s3://, http://, https:// before calling os.path.abspath, so other fsspec protocols (gcs://, gs://, abfs://, az://, chained protocols like simplecache::s3://) were rewritten into a corrupted local absolute path (/cwd/gcs:/bucket/file.zarr), breaking GCS/Azure reads. Detect protocol URLs generically via "://" and leave them untouched. - ZarrIO._copy_array did `dest[:] = source[:]`, decompressing the entire (possibly multi-GB) array into one in-memory ndarray and risking OOM when exporting large datasets. Copy one chunk region at a time to bound peak memory to a single chunk. Adds TestPathNormalization and TestCopyArray regression tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The ZarrStringDataset wrapper introduced earlier broke export of typed text columns (spec dtype 'text', e.g. electrodes.location): HDMF's convert_dtype only recognizes zarr.Array/ndarray/StrDataset/list/DataChunkIterator for string specs, so a custom wrapper falls through to _unicode(wrapper) and raises "Expected unicode or ascii string" (caught by the plot_convert_nwb_hdf5 gallery). A raw zarr StringDType array cannot be used either: numpy's StringDType has kind "T", which HDMF's generic (dtype=None) inference does not recognize, so it infers python `str` and HDF5IO cannot create the dataset. Decode to a numpy object array of native Python str on read. This is recognized by HDMF for both typed and generic specs, preserves shape/dtype, and supports N-D data[i, j] indexing (which list(zarr_obj[:]) did not). Truly lazy access is not achievable here without upstream HDMF support for StringDType. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Migrate hdmf-zarr from zarr-python v2 (
zarr>=2.18.0, <3.0) to zarr-python v3 (zarr>=3.1.3), writing zarr v3 format only while maintaining the ability to read existing v2 format files.Closes #202
Key changes
DirectoryStore/NestedDirectoryStore/TempStorewithLocalStore;FSStorewithFsspecStoreobject_codec=numcodecs.Pickle()/JSON()withStringDType+ JSON serialization for references, specs, and compound datasetszarr.copy()not implemented in v3: Add custom_copy_array()helpersynchronizerparameter with warningrequire_dataset/create_dataset→require_array/create_array__len__(not implemented) and scalar indexing (returns 0-d ndarrays)numcodecs.ShuffleisBytesBytesCodec(compressor), notArrayArrayCodec(filter)store.root.resolve()for filesystem paths (str(LocalStore)returns URI)ConsolidatedMetadataStorewrapper)hdmf>=4.2.0,zarr>=3.1.3,numpy>=1.26.0,numcodecs>=0.14.0, Python>=3.11Files changed
src/hdmf_zarr/backend.pysrc/hdmf_zarr/utils.pysrc/hdmf_zarr/zarr_utils.pysrc/hdmf_zarr/nwb.pypyproject.tomlzarr>=3.1.3,hdmf>=4.2.0, remove numcodecs upper bound, Python >=3.11requirements-min.txttox.iniextras = null→ empty), update Python versionsdocs/source/conf.pydocs/source/index.rstdocs/source/storage.rstdocs/source/integrating_data_stores.rstdocs/gallery/plot_s3_streaming.pytests/unit/base_tests_zarrio.pytests/unit/test_zarrio.pytests/unit/test_io_convert.pytests/unit/test_zarrdataio.pytests/unit/test_compression_info.pytests/unit/test_fsspec_streaming.pyTest plan
pytest tests/)🤖 Generated with Claude Code