Skip to content

Read zarr v2 NWB files via a dedicated NWBZarrV2IO - #349

Open
alejoe91 wants to merge 43 commits into
hdmf-dev:zarr-v3-migrationfrom
alejoe91:nwb-zarr-v2-io
Open

Read zarr v2 NWB files via a dedicated NWBZarrV2IO#349
alejoe91 wants to merge 43 commits into
hdmf-dev:zarr-v3-migrationfrom
alejoe91:nwb-zarr-v2-io

Conversation

@alejoe91

@alejoe91 alejoe91 commented May 29, 2026

Copy link
Copy Markdown
Collaborator

That PR baked the zarr v2 fallbacks directly into ZarrIO / NWBZarrIO — so every v3 read went through try / except / use_consolidated=False, every group walk used a chunk-decoding iterator, and ~440 lines of v2-specific code lived in backend.py. It worked but the v3 path paid the complexity tax for files it would never see.

This PR splits the two:

  • ZarrIO and NWBZarrIO go back to their clean v3 shape.
  • A new ZarrV2IO(ZarrIO) (read-only) and NWBZarrV2IO(ZarrV2IO) live in backend_zarrv2.py / nwb_zarrv2.py and contain all the v2 logic — consolidated-metadata fallbacks, raw-chunk decoding for |O arrays, dual path resolution for old-style references, the spec-reader fallback.
  • To make this possible, ZarrIO grew a few protected hooks (_open_file, _open_file_consolidated, _iter_children, _resolve_ref_source, _make_spec_reader, _open_for_namespaces). No behavior change for v3 users; ZarrV2IO overrides them.

Usage

from hdmf_zarr import NWBZarrIO, NWBZarrV2IO

# explicit:
with NWBZarrV2IO("old_v2_file.nwb.zarr", mode="r") as io:
    nwbfile = io.read()

# read_nwb zarrv2
nwbfile = NWBZarrV2IO.read_nwb("old_v2_file.nwb.zarr")

NWBZarrV2IO is read-only by design — writes always produce v3.

The NWBZarrV2IO has helper functions to export/convert to Zarr v3:

io = NWBZarrV2IO("old_v2_file.nwb.zarr", mode="r")

io.export_to_v3(output="new_v3_file.nwb.zarr")

# or directly
NWBZarrV2IO.convert_to_v3(source_path="old_v2_file.nwb.zarr", dest_path="new_v3_file.nwb.zarr")

Examples

Updated plot_s3_streaming.py to use the NWBZarrV2IO reader.

Tests

  • test_zarrv2_backward_compat.py now uses NWBZarrV2IO.
  • The remote S3 v2 fixtures in test_fsspec_streaming.py are switched to NWBZarrV2IO
  • Three test_zarrio.py tests that poked _ZarrIO__* mangled attributes were updated to the new protected names.

@codecov-commenter

codecov-commenter commented May 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 69.98158% with 163 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.31%. Comparing base (b3dbd30) to head (2bf250b).

Files with missing lines Patch % Lines
src/hdmf_zarr/backend_zarrv2.py 63.10% 92 Missing and 29 partials ⚠️
src/hdmf_zarr/backend.py 80.43% 18 Missing and 9 partials ⚠️
src/hdmf_zarr/nwb_zarrv2.py 80.43% 4 Missing and 5 partials ⚠️
src/hdmf_zarr/nwb.py 70.00% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##           zarr-v3-migration     #349      +/-   ##
=====================================================
- Coverage              84.00%   80.31%   -3.70%     
=====================================================
  Files                      5        7       +2     
  Lines                   1388     1854     +466     
  Branches                 278      357      +79     
=====================================================
+ Hits                    1166     1489     +323     
- Misses                   146      252     +106     
- Partials                  76      113      +37     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alejoe91
alejoe91 requested review from bendichter and rly May 29, 2026 14:39
@alejoe91

alejoe91 commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

@bendichter @oruebel let me know what you guys think about this approach

Comment thread src/hdmf_zarr/v2_backend.py Outdated
Comment thread src/hdmf_zarr/backend_zarrv2.py
@oruebel

oruebel commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

That PR baked the zarr v2 fallbacks directly into ZarrIO / NWBZarrIO — so every v3 read went through try / except / use_consolidated=False, every group walk used a chunk-decoding iterator, and ~440 lines of v2-specific code lived in backend.py. It worked but the v3 path paid the complexity tax for files it would never see.

I think the general approach of separating support for V2 files from the V3 backend makes sense. The overall code may be a bit longer right now but it'll make maintenance easier and keep the V3 code cleaner and more focused. I'll take a closer look at the actual code in the next couple of days.

Comment thread .github/workflows/test_backward_compat.yml Outdated
Comment thread src/hdmf_zarr/backend_zarrv2.py
Comment thread src/hdmf_zarr/__init__.py
Comment thread src/hdmf_zarr/backend.py
Comment thread src/hdmf_zarr/backend.py
Comment thread src/hdmf_zarr/backend_zarrv2.py
Comment thread src/hdmf_zarr/backend.py Outdated
Comment thread src/hdmf_zarr/nwb.py Outdated
Comment thread src/hdmf_zarr/nwb_zarrv2.py Outdated
Comment thread src/hdmf_zarr/v2_backend.py Outdated
Comment thread src/hdmf_zarr/v2_backend.py Outdated
@oruebel

oruebel commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Overall the approach looks good. I still need to take a closer look at v2_backend.py and the tests, but added comments to the other files.

@alejoe91

Copy link
Copy Markdown
Collaborator Author

Thanks for the review @oruebel

I'll tackle them in the next days but happy to hear that the approach is fine!

@alejoe91

alejoe91 commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Done. Added test file generated with pynwb 2.8.3 and Zarr v2.18

@oruebel oruebel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alejoe91 . From my end this looks good. @rly I think it would be good if you could also take a look since this is part of the larger Zarr V3 upgrade.

Comment on lines +470 to +471
ask zarr-python v3 to open every child, which raises for zarr v2 arrays
that v3 cannot parse (most commonly object-dtype arrays stored with

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which raises for zarr v2 arrays
that v3 cannot parse (most commonly object-dtype arrays stored with v2-only codecs such as pickle, json2, or vlen-utf8)

vlen-utf8 should work, but more generally this is behavior I would like to fix in zarr-python. We should support gradual levels of support for arrays with unknown data types, so e.g. you could see that the array is there, but reading / writing chunks would be disallowed.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @d-v-b

I think we should keep an eye on this and defer the logic to zarr-python once it's implemented. I opened an issue on our end as a reminder: #358

@alejoe91

Copy link
Copy Markdown
Collaborator Author

@oruebel @rly can we move forward with this?

@oruebel

oruebel commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

@oruebel @rly can we move forward with this?

Looks fine from my end. I leave final review to @rly

rly and others added 2 commits July 18, 2026 21:29
- Rename test_zarrv2_backward_combat.py -> _compat.py (typo).
- Fix broken Sphinx refs: backend_v2 -> backend_zarrv2 (backend.py,
  nwb_zarrv2.py) and nwb_v2/nwbNWBZarrIO -> nwb_zarrv2/nwb (overview.rst).
- Correct the backward-compat test docstring: the fixture is checked in
  (no CI-generated file / nonexistent workflow), with the right
  regeneration command and filenames.
- Raise a clear ValueError pointing at ZarrV2IO/NWBZarrV2IO when a Zarr v2
  file is opened with the Zarr v3 ZarrIO/NWBZarrIO, replacing an opaque
  zarr-python parse error. Covered at the open, load_namespaces, and
  read_builder entry points; suppressed for the v2 backend via a
  _reads_zarr_v2 class flag. Add tests.
- Move the unreleased hdmf-dev#348/hdmf-dev#349 CHANGELOG entries to Upcoming Release.
- Apply black formatting to the new v2 backend modules.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The base ZarrIO error for reading a Zarr v2 file named the NWB-layer classes
(NWBZarrV2IO / convert_to_v3), which a non-NWB ZarrIO user should never be
pointed at. ZarrIO now recommends only ZarrV2IO via an overridable
`_zarr_v2_backend_name` class attribute; NWBZarrIO overrides it and extends the
message with the NWB convert helper. Add a test asserting the base message
stays free of NWB references.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rly

rly commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Thanks. I'm starting to review this now. I made some minor fixes:

  • I renamed the test file (combat → compat)
  • I fixed broken Sphinx refs in backend.py, nwb_zarrv2.py, and overview.rst.
  • I corrected the backward-compat test docstring.
  • I moved the unreleased Fix resolve_ref self-reference resolution over fsspec stores #348 and this PR CHANGELOG entries to Upcoming Release.
  • I reflowed/formatted the new v2 backend modules with black

And I added a more informative error message if a user tries to read a Zarr v2 with the new ZarrIO/NWBZarrIO class:

Failed to read '…' with NWBZarrIO, which reads Zarr v3 files, but this path is a Zarr v2 file. Open it read-only with the NWBZarrV2IO. Or convert it to Zarr v3 with NWBZarrV2IO.convert_to_v3(source_path, dest_path).

rly and others added 2 commits July 22, 2026 12:40
… hours

The Python 3.14 test jobs intermittently deadlock; with no `timeout-minutes` the
hung job runs to GitHub's 6-hour cap before being cancelled, and produces no stack
trace. Every observed hang stalls in the ZarrToHDF5 DynamicTable export-roundtrip
tests, where a zarr v3 read (async `sync()` loop) is interleaved with an HDF5 write.

- Add `pytest-timeout` (test group) and a global `[tool.pytest.ini_options]`
  timeout of 300s with the "thread" method, which dumps every thread's stack and
  kills the process when a test hangs. Works cross-platform and even when the main
  thread is stuck in a C call (e.g. HDF5), which the signal method cannot interrupt.
- Add `timeout-minutes: 45` to the run_tests.yml jobs as a backstop for hangs
  outside a test.

The next time this job hangs it fails in minutes with an actionable traceback.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rly

rly commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

A Zarr to HDF5 test is timing out sporadically due to a tricky deadlocking issue between the HDF5 global lock, Python's cyclic garbage collector, and zarr v3's async→sync execution model. This should be resolved in HDMF: hdmf-dev/hdmf#1547

rly and others added 8 commits July 24, 2026 11:07
Clarify in read_nwb and convert_to_v3 that s3:// paths are read
anonymously, and that credentialed/gs:// sources should construct
NWBZarrV2IO directly with storage_options. Also note that convert_to_v3's
storage_options argument configures the destination store, not the source.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
read_builder passed two arguments to _zarr_v2_read_error_message, which
takes one, so a Zarr v2 file that opened without consolidated metadata but
failed while reading arrays raised a TypeError about argument count instead
of the intended "use NWBZarrV2IO" hint.

Route open() and read_builder() through a shared _raise_if_zarr_v2 helper so
the guard lives in one place, and narrow read_builder's try to the actual
read call rather than wrapping the spec-location setup and return.

Add a regression test that forces the non-consolidated open path (by removing
.zmetadata) so the failure surfaces in read_builder, and asserts the hint.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reformat utils.py, test_zarrio.py, and generate_nwb_zarrv2.py to satisfy the
repo's pinned black (25.9.0). These are formatting-only changes (quote style,
blank line after nested imports, argument wrapping) with no behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The resolve_ref self-reference fix (hdmf-dev#348) shipped in 0.13.0 and is listed
there on dev, but this branch had relocated it to "Upcoming Release (TBD)".
Move it back so the 0.13.0 section matches dev and the Upcoming section lists
only unreleased changes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Remove the unused FsspecStore / FSSPECSTORE_AVAILABLE import block; the
  store helpers dispatch on isinstance(store, LocalStore) and use the async
  store API for everything else, so the symbols were never referenced in code.
- Widen the consolidated-open fallbacks in _open_file and _open_for_namespaces
  from except (TypeError, ValueError) to except Exception, matching the base
  _open_file_consolidated. zarr v3 raises varying types when it fails to parse a
  v2 consolidated block (observed TypeError for a bad object-dtype fill_value,
  ValueError for a missing .zmetadata), so the non-consolidated retry must not be
  gated on a specific exception type.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rly

rly commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

I made a few more minor changes:

rly and others added 3 commits July 24, 2026 12:26
Text about v2 files being on DANDI will become outdated
…orial

The S3 streaming tutorial and the ZarrIO write/export docval docs described
consolidated metadata as a ".zmetadata" file at the root. That is the Zarr v2
form; Zarr v3 (what ZarrIO writes) stores the consolidated block inside the root
zarr.json. Reword the tutorial to cover both formats and fix the three ZarrIO
docstrings to name zarr.json.

Also fix the tutorial's backend auto-detection reference: NWBHDF5IO.read reads
HDF5 only. The backend-agnostic reader is pynwb.read_nwb.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- _copy_array: zarr.copy() is present in zarr v3 but raises NotImplementedError,
  so say "not implemented in zarr v3" rather than "not available".
- The zarr v3 writer ships in hdmf-zarr 0.14, so legacy Zarr v2 files come from
  hdmf-zarr<0.14, not <1.0. Update the ZarrV2IO / NWBZarrV2IO module docstrings
  and the v2 fixture regeneration instructions (docstring + pip install pin) to
  hdmf-zarr<0.14, matching the "(since v0.14)" note in the docs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants