From 060d121bd9635c2eb3e2519030c8979c083659f0 Mon Sep 17 00:00:00 2001 From: Alessio Buccino Date: Thu, 2 Apr 2026 12:01:25 +0200 Subject: [PATCH 1/2] Add V2 backward-compatibility to Zarr v3 implementation --- .github/workflows/test_backward_compat.yml | 54 +++ .gitignore | 3 + src/hdmf_zarr/backend.py | 374 +++++++++++++++++++-- src/hdmf_zarr/utils.py | 72 +++- tests/unit/helpers/generate_v2_nwb_zarr.py | 127 +++++++ tests/unit/test_fsspec_streaming.py | 4 +- tests/unit/test_v2_backward_compat.py | 160 +++++++++ 7 files changed, 769 insertions(+), 25 deletions(-) create mode 100644 .github/workflows/test_backward_compat.yml create mode 100644 tests/unit/helpers/generate_v2_nwb_zarr.py create mode 100644 tests/unit/test_v2_backward_compat.py diff --git a/.github/workflows/test_backward_compat.yml b/.github/workflows/test_backward_compat.yml new file mode 100644 index 00000000..8130e81f --- /dev/null +++ b/.github/workflows/test_backward_compat.yml @@ -0,0 +1,54 @@ +name: Test zarr v2 backward compatibility +on: + push: + branches: + - dev + pull_request: + +jobs: + test-v2-backward-compat: + name: zarr v2 → v3 backward compat + runs-on: ubuntu-latest + defaults: + run: + shell: bash + + steps: + - name: Checkout repo + uses: actions/checkout@v6 + with: + fetch-depth: 0 + + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: "3.12" + + # --- Step 1: generate a zarr v2 NWB file with old deps --- + - name: Create zarr v2 environment + run: | + python -m venv /tmp/zarr_v2_env + /tmp/zarr_v2_env/bin/pip install --upgrade pip + /tmp/zarr_v2_env/bin/pip install "hdmf-zarr<1.0" "zarr>=2.18,<3" pynwb + + - name: Generate zarr v2 NWB test file + run: | + /tmp/zarr_v2_env/bin/python tests/unit/helpers/generate_v2_nwb_zarr.py \ + tests/unit/helpers/v2_test_file.nwb.zarr \ + > tests/unit/helpers/v2_expected.json + echo "--- generated file tree ---" + find tests/unit/helpers/v2_test_file.nwb.zarr -maxdepth 2 -type d | head -30 + echo "--- expected metadata ---" + cat tests/unit/helpers/v2_expected.json + + # --- Step 2: read the v2 file with current code + zarr v3 --- + - name: Install current hdmf-zarr + zarr v3 + run: | + python -m pip install --upgrade pip + python -m pip install -e ".[full]" + python -m pip install pytest + python -m pip list + + - name: Run backward compatibility tests + run: | + pytest tests/unit/test_v2_backward_compat.py -v diff --git a/.gitignore b/.gitignore index a77992c8..ce2fdf84 100644 --- a/.gitignore +++ b/.gitignore @@ -150,3 +150,6 @@ _version.py .ruff_cache zarr_test*.zarr *.nwb.zarr + +# Generated backward compat test artifacts +tests/unit/helpers/v2_expected.json diff --git a/src/hdmf_zarr/backend.py b/src/hdmf_zarr/backend.py index a5b1aa5f..5a1a632e 100644 --- a/src/hdmf_zarr/backend.py +++ b/src/hdmf_zarr/backend.py @@ -92,7 +92,13 @@ def can_read(path): zarr.open(path, mode="r") return True except Exception: - return False + try: + # zarr v3 may fail to parse consolidated metadata from v2 files + # (e.g., invalid fill_value types). Fall back to non-consolidated open. + zarr.open(path, mode="r", use_consolidated=False) + return True + except Exception: + return False @staticmethod def generate_dataset_html(dataset): @@ -310,7 +316,13 @@ def load_namespaces(cls, namespace_catalog, path, file, storage_options, namespa if path is not None: store = cls.__resolve_store(path, storage_options) - f = zarr.open(store, mode="r") + try: + f = zarr.open(store, mode="r") + except (TypeError, ValueError): + # zarr v3 may fail to parse consolidated metadata from v2 files + # (e.g., invalid fill_value types or unsupported object dtypes). + # Fall back to non-consolidated open. + f = zarr.open(store, mode="r", use_consolidated=False) else: f = file return cls.__load_namespaces(namespace_catalog, namespaces, f) @@ -350,12 +362,29 @@ def __load_namespaces( readers = dict() for ns in namespaces: - ns_group = spec_group[ns] - latest_version = list(ns_group.keys())[-1] - latest_ns_group = ns_group[latest_version] - readers[ns] = ZarrSpecReader(latest_ns_group) + try: + ns_group = spec_group[ns] + latest_version = list(ns_group.keys())[-1] + latest_ns_group = ns_group[latest_version] + readers[ns] = ZarrSpecReader(latest_ns_group) + except Exception as e: + warnings.warn( + f"Could not read cached namespace '{ns}' from " + f"{cls.__get_store_path(f.store)}: {e}. Skipping." + ) + continue - d = namespace_catalog.load_namespaces("namespace", reader=readers) + if not readers: + return {} + + try: + d = namespace_catalog.load_namespaces("namespace", reader=readers) + except Exception as e: + warnings.warn( + f"Could not load cached namespaces from " + f"{cls.__get_store_path(f.store)}: {e}. Skipping." + ) + d = {} return d @docval( @@ -662,7 +691,12 @@ def __resolve_store(store, storage_options=None): def __open_file(self, store, mode, storage_options=None): """Open a zarr file without consolidated metadata.""" store = self.__resolve_store(store, storage_options) - return zarr.open(store=store, mode=mode) + try: + return zarr.open(store=store, mode=mode) + except (TypeError, ValueError): + # zarr v3 may fail when it auto-detects consolidated metadata from v2 files + # containing unsupported dtypes. Fall back to non-consolidated open. + return zarr.open(store=store, mode=mode, use_consolidated=False) def __open_file_consolidated(self, store, mode, storage_options=None): """ @@ -691,10 +725,11 @@ def __open_file_consolidated(self, store, mode, storage_options=None): store=open_store, mode=mode, ) - except Exception: # Catches errors when consolidated metadata doesn't exist + except Exception: # Catches errors when consolidated metadata doesn't exist or can't be parsed zarr_obj = zarr.open( store=open_store, mode=mode, + use_consolidated=False, ) # Cache the result @@ -903,8 +938,20 @@ def resolve_ref(self, zarr_ref): if isinstance(self.source, str) and self.source.startswith(("s3://")): source_file = self.source else: - # Join with source_file to resolve the relative path; use abspath for consistent comparisons - source_file = os.path.abspath(os.path.normpath(os.path.join(self.source, source_file))) + # Try resolving relative to the store itself first (current convention, + # where self-references use "."), then fall back to resolving from the + # parent directory (older hdmf-zarr convention). + abs_source = os.path.abspath(self.source) + resolved_from_store = os.path.abspath(os.path.normpath(os.path.join(abs_source, source_file))) + base_dir = os.path.dirname(abs_source) + resolved_from_parent = os.path.abspath(os.path.normpath(os.path.join(base_dir, source_file))) + if os.path.exists(resolved_from_store) and os.path.isdir(resolved_from_store): + source_file = resolved_from_store + elif os.path.exists(resolved_from_parent) and os.path.isdir(resolved_from_parent): + source_file = resolved_from_parent + else: + # Neither resolution works; assume self-reference + source_file = abs_source else: # get rid of extra "/" and "./" in the path root and source_file root_path = str(self.path).rstrip("/") @@ -1724,6 +1771,287 @@ def __get_built(self, zarr_obj): path = os.path.join(fpath, path) return self.__built.get(path, None) + @staticmethod + def __read_store_bytes(store, key): + """Read raw bytes from a zarr store (local or remote). + + Returns bytes, or None if the key does not exist. + """ + if isinstance(store, LocalStore): + full_path = os.path.join(str(store.root), key) + if not os.path.exists(full_path): + return None + with open(full_path, "rb") as f: + return f.read() + else: + # Remote store — use async get + from zarr.core.sync import sync as zarr_sync + from zarr.core.buffer import default_buffer_prototype + + async def _get(): + return await store.get(key, prototype=default_buffer_prototype()) + + buf = zarr_sync(_get()) + if buf is None: + return None + return buf.to_bytes() + + @staticmethod + def __store_key_exists(store, key): + """Check if a key exists in a zarr store (local or remote).""" + if isinstance(store, LocalStore): + return os.path.exists(os.path.join(str(store.root), key)) + else: + from zarr.core.sync import sync as zarr_sync + + async def _exists(): + return await store.exists(key) + + return zarr_sync(_exists()) + + @staticmethod + def __store_list_dir(store, prefix): + """List immediate children (dirs and files) under *prefix* in a zarr store. + + Returns a sorted list of entry names. + """ + if isinstance(store, LocalStore): + dir_path = os.path.join(str(store.root), prefix) if prefix else str(store.root) + try: + return sorted(os.listdir(dir_path)) + except OSError: + return [] + else: + from zarr.core.sync import sync as zarr_sync + + async def _list(): + result = [] + async for item in store.list_dir(prefix): + result.append(item) + return result + + return sorted(zarr_sync(_list())) + + def __read_v2_dataset_from_fs(self, store, group_path, name): + """Read a zarr v2 dataset directly from the store when zarr v3 cannot parse it. + + Works for both local and remote (fsspec) stores. + + Handles object-dtype arrays (``|O`` with ``pickle``/``json2``/``vlen-utf8`` codecs) + and arrays with incompatible fill_value types (e.g. datetime stored as vlen strings). + + Returns a :class:`~hdmf.build.DatasetBuilder`. + """ + dataset_key = f"{group_path}/{name}" if group_path else name + + # Read .zarray metadata + zarray_bytes = self.__read_store_bytes(store, f"{dataset_key}/.zarray") + if zarray_bytes is None: + raise FileNotFoundError(f"No .zarray found for '{dataset_key}'") + zarray_meta = json.loads(zarray_bytes) + + # Read .zattrs + zattrs_bytes = self.__read_store_bytes(store, f"{dataset_key}/.zattrs") + all_attrs = json.loads(zattrs_bytes) if zattrs_bytes is not None else {} + + zarr_dtype = all_attrs.get("zarr_dtype") + if zarr_dtype is None: + zarr_dtype = zarray_meta.get("dtype", "") + warnings.warn(f"Inferred dtype from zarr v2 .zarray for '{name}': {zarr_dtype}") + + # Filter reserved attributes + reserved = ("zarr_dtype", "zarr_link", SPEC_LOC_ATTR) + attrs = {k: v for k, v in all_attrs.items() if k not in reserved} + + shape = tuple(zarray_meta["shape"]) + chunks = tuple(zarray_meta["chunks"]) + source = self.__get_store_path(store) + + # Decode the data from raw chunks + data = self.__decode_v2_dataset(store, dataset_key, zarray_meta) + + # Handle scalar datasets + if zarr_dtype == "scalar": + if isinstance(data, np.ndarray) and data.size > 0: + data = data.flat[0] + elif isinstance(data, (list, tuple)) and len(data) > 0: + data = data[0] + + # Handle reference arrays + if isinstance(zarr_dtype, str) and self.__is_ref(zarr_dtype): + data = BuilderZarrReferenceDataset(data, self) + elif isinstance(zarr_dtype, list): + has_reference = any(dts.get("dtype") == "object" for dts in zarr_dtype) + if has_reference: + retrieved_dtypes = [d["dtype"] for d in zarr_dtype] + data = BuilderZarrTableDataset(data, self, retrieved_dtypes) + # Eagerly convert string/object data to lists for downstream compatibility + elif isinstance(data, np.ndarray) and data.dtype.kind in ("U", "S"): + data = list(data) + elif ( + isinstance(data, np.ndarray) + and data.dtype == object + and data.size > 0 + and isinstance(data.flat[0], str) + ): + data = list(data) + + kwargs = { + "attributes": attrs, + "dtype": zarr_dtype, + "maxshape": shape, + "chunks": not (shape == chunks), + "source": source, + "data": data, + } + + builder = DatasetBuilder(name, **kwargs) + # Set location to match what __read_dataset would produce + if group_path: + builder.location = "/" + group_path.replace("\\", "/") if not group_path.startswith("/") else group_path + else: + builder.location = "/" + + self._written_builders.set_written(builder) + # Cache with the same key that __set_built / __get_built would use + full_path = os.path.join(source, group_path, name) if group_path else os.path.join(source, name) + self.__built.setdefault(full_path, builder) + + return builder + + def __decode_v2_dataset(self, store, dataset_key, zarray_meta): + """Decode all chunks of a zarr v2 dataset from a store (local or remote).""" + import numcodecs + + shape = tuple(zarray_meta["shape"]) + chunks = tuple(zarray_meta["chunks"]) + dtype = np.dtype(zarray_meta.get("dtype", "f8")) + order = zarray_meta.get("order", "C") + dimension_separator = zarray_meta.get("dimension_separator", ".") + + compressor_config = zarray_meta.get("compressor") + compressor = numcodecs.get_codec(compressor_config) if compressor_config else None + + filters_config = zarray_meta.get("filters") or [] + filters = [numcodecs.get_codec(fc) for fc in filters_config] + + is_object = dtype == np.dtype("|O") + ndim = len(shape) + chunk_grid = tuple((s + c - 1) // c for s, c in zip(shape, chunks)) + + if any(s == 0 for s in shape): + return np.empty(shape, dtype=object if is_object else dtype) + + # --- single-chunk fast path (most common for metadata arrays) --- + total_chunks = 1 + for g in chunk_grid: + total_chunks *= g + + if total_chunks == 1: + chunk_name = dimension_separator.join("0" for _ in range(ndim)) + chunk_store_key = f"{dataset_key}/{chunk_name}" + raw = self.__read_store_bytes(store, chunk_store_key) + if raw is not None: + data = self.__decode_v2_chunk(raw, compressor, filters, dtype, chunks, order, is_object) + if chunks != shape: + data = data[tuple(slice(0, s) for s in shape)] + return data + return np.full(shape, fill_value=None if is_object else 0, dtype=object if is_object else dtype) + + # --- general multi-chunk path --- + result = np.empty(shape, dtype=object if is_object else dtype) + for idx in np.ndindex(*chunk_grid): + chunk_name = dimension_separator.join(str(i) for i in idx) + chunk_store_key = f"{dataset_key}/{chunk_name}" + raw = self.__read_store_bytes(store, chunk_store_key) + if raw is None: + continue + chunk_data = self.__decode_v2_chunk(raw, compressor, filters, dtype, chunks, order, is_object) + slices = tuple(slice(i * c, min((i + 1) * c, s)) for i, c, s in zip(idx, chunks, shape)) + chunk_slices = tuple(slice(0, sl.stop - sl.start) for sl in slices) + result[slices] = chunk_data[chunk_slices] + return result + + @staticmethod + def __decode_v2_chunk(raw, compressor, filters, dtype, chunk_shape, order, is_object): + """Decode a single raw chunk of zarr v2 data.""" + if compressor is not None: + raw = compressor.decode(raw) + + if is_object: + # For object dtype the filters (pickle / json2 / vlen-utf8) produce the array + for filt in reversed(filters): + raw = filt.decode(raw) + if isinstance(raw, np.ndarray): + return raw + return np.array(raw, dtype=object) + + # Regular (non-object) dtypes + if isinstance(raw, (bytes, bytearray)): + arr = np.frombuffer(raw, dtype=dtype).copy() + else: + arr = np.asarray(raw, dtype=dtype).copy() + + for filt in reversed(filters): + decoded = filt.decode(arr) + if isinstance(decoded, (bytes, bytearray)): + arr = np.frombuffer(decoded, dtype=dtype).copy() + elif isinstance(decoded, np.ndarray): + arr = decoded + else: + arr = np.asarray(decoded, dtype=dtype) + + return arr.reshape(chunk_shape, order=order) + + def __iter_children(self, zarr_obj): + """Safely iterate over children of a zarr group, yielding (name, child) pairs. + + *child* is normally a :class:`zarr.Group` or :class:`zarr.Array`. When + zarr-python v3 cannot parse a zarr v2 array (e.g. ``|O`` dtype), the + method falls back to :meth:`__read_v2_dataset_from_fs` and yields a + :class:`~hdmf.build.DatasetBuilder` instead. + + Works for both local and remote (fsspec) stores. + """ + store = zarr_obj.store + group_prefix = zarr_obj.path or "" + entries = self.__store_list_dir(store, group_prefix) + + for entry in entries: + # Skip hidden/metadata files + if entry.startswith("."): + continue + # For local stores, skip non-directory entries + if isinstance(store, LocalStore): + entry_path = os.path.join(str(store.root), group_prefix, entry) if group_prefix else os.path.join(str(store.root), entry) + if not os.path.isdir(entry_path): + continue + try: + child = zarr_obj[entry] + yield entry, child + except Exception as e: + # Try reading as a zarr v2 dataset via the store + zarray_key = f"{group_prefix}/{entry}/.zarray" if group_prefix else f"{entry}/.zarray" + if self.__store_key_exists(store, zarray_key): + try: + builder = self.__read_v2_dataset_from_fs(store, group_prefix, entry) + warnings.warn( + f"Read '{entry}' in '{zarr_obj.name}' via zarr v2 " + f"store fallback (zarr v3 error: {e})" + ) + yield entry, builder + except Exception as e2: + warnings.warn( + f"Skipping '{entry}' in '{zarr_obj.name}': " + f"zarr v3 could not parse it ({e}) and " + f"v2 store fallback also failed ({e2})" + ) + else: + warnings.warn( + f"Skipping '{entry}' in '{zarr_obj.name}': zarr v3 could not parse " + f"its metadata (likely a zarr v2 object-dtype array): {e}" + ) + def __read_group(self, zarr_obj, name=None, ignore_groups=set()): # NOTE: ignore_groups is a set of group names to skip when reading and only # used when reading the root group to skip the specification group @@ -1743,17 +2071,19 @@ def __read_group(self, zarr_obj, name=None, ignore_groups=set()): ret = GroupBuilder(name=name, source=source, attributes=attributes) ret.location = ZarrIO.get_zarr_parent_path(zarr_obj) - # read sub groups - for sub_name, sub_group in zarr_obj.groups(): - if sub_group.name in ignore_groups: - continue - sub_builder = self.__read_group(sub_group, sub_name) - ret.set_group(sub_builder) - - # read sub datasets - for sub_name, sub_array in zarr_obj.arrays(): - sub_builder = self.__read_dataset(sub_array, sub_name) - ret.set_dataset(sub_builder) + # read sub groups and datasets using safe iteration + for sub_name, child in self.__iter_children(zarr_obj): + if isinstance(child, DatasetBuilder): + # Already built by the v2 filesystem fallback in __iter_children + ret.set_dataset(child) + elif isinstance(child, Group): + if child.name in ignore_groups: + continue + sub_builder = self.__read_group(child, sub_name) + ret.set_group(sub_builder) + elif isinstance(child, Array): + sub_builder = self.__read_dataset(child, sub_name) + ret.set_dataset(sub_builder) # read the links self.__read_links(zarr_obj=zarr_obj, parent=ret) diff --git a/src/hdmf_zarr/utils.py b/src/hdmf_zarr/utils.py index afcff59b..a0ea039a 100644 --- a/src/hdmf_zarr/utils.py +++ b/src/hdmf_zarr/utils.py @@ -387,13 +387,83 @@ def __init__(self, **kwargs): self.__cache = None def __read(self, path): - s = self.__group[path][0] + try: + s = self.__group[path][0] + except (ValueError, TypeError) as e: + # zarr v3 cannot read object-dtype (|O) arrays with json2/vlen-utf8 + # codecs from zarr v2 files. Fall back to reading raw chunk data. + warn( + f"Could not read dataset '{path}' via zarr API ({e}). " + f"Falling back to raw chunk read for zarr v2 object-dtype array." + ) + s = self.__read_v2_object_array(path) # In zarr v3, string arrays may return numpy StringDType scalars # Ensure we have a plain Python string for json.loads s = str(s) if not isinstance(s, str) else s d = json.loads(s) return d + def __read_v2_object_array(self, path): + """Read data from a zarr v2 object-dtype array that zarr v3 cannot parse. + + Reads .zarray metadata and raw chunk bytes from the store (local or remote), + then uses numcodecs to decompress and decode the data. + """ + from zarr.storage import LocalStore + + store = self.__group.store + dataset_key = f"{self.__group.path}/{path}" if self.__group.path else path + + if isinstance(store, LocalStore): + def _read_bytes(key): + full_path = os.path.join(str(store.root), key) + if not os.path.exists(full_path): + return None + with open(full_path, "rb") as f: + return f.read() + else: + from zarr.core.sync import sync as zarr_sync + from zarr.core.buffer import default_buffer_prototype + + def _read_bytes(key): + async def _get(): + return await store.get(key, prototype=default_buffer_prototype()) + buf = zarr_sync(_get()) + return buf.to_bytes() if buf is not None else None + + # Read .zarray metadata + zarray_bytes = _read_bytes(f"{dataset_key}/.zarray") + if zarray_bytes is None: + raise FileNotFoundError(f"No .zarray found for '{dataset_key}'") + zarray_meta = json.loads(zarray_bytes) + + # Read raw chunk data (single chunk, index 0) + raw = _read_bytes(f"{dataset_key}/0") + if raw is None: + raise FileNotFoundError(f"No chunk data found for '{dataset_key}'") + + # Decompress if a compressor was used + compressor_config = zarray_meta.get("compressor") + if compressor_config is not None: + import numcodecs + compressor = numcodecs.get_codec(compressor_config) + raw = compressor.decode(raw) + + # Apply filters in reverse order (zarr v2 decoding convention) + filters = zarray_meta.get("filters") or [] + if filters: + import numcodecs + for filt_config in reversed(filters): + filt = numcodecs.get_codec(filt_config) + raw = filt.decode(raw) + + # Extract the first element from the decoded array + if isinstance(raw, np.ndarray): + return raw.flat[0] + if isinstance(raw, (list, tuple)): + return raw[0] + return raw + def read_spec(self, spec_path): """Read a spec from the given path""" return self.__read(spec_path) diff --git a/tests/unit/helpers/generate_v2_nwb_zarr.py b/tests/unit/helpers/generate_v2_nwb_zarr.py new file mode 100644 index 00000000..8707a462 --- /dev/null +++ b/tests/unit/helpers/generate_v2_nwb_zarr.py @@ -0,0 +1,127 @@ +"""Generate a zarr v2 NWB file using hdmf-zarr<1.0 and zarr<3. + +This script is meant to run in a temporary environment with:: + + pip install "hdmf-zarr<1.0" "zarr>=2.18,<3" pynwb + +It creates a small but representative NWB zarr file that exercises the +data types most affected by the zarr v2 → v3 migration: + +* Scalar string / datetime datasets (fill_value=0 issue) +* Object-dtype columns in DynamicTables (pickle / json2 / vlen-utf8 codecs) +* Electrode groups, devices (object references) +* TimeSeries with numerical data +* Subject metadata with date_of_birth + +The file is written to the path given as the first CLI argument. +Metadata expectations are printed as JSON to stdout so the test can +capture them without hard-coding values in two places. +""" + +import json +import sys +import os +import numpy as np +from datetime import datetime +from dateutil.tz import tzlocal + +from pynwb import NWBFile +from pynwb.ecephys import ElectricalSeries +from hdmf_zarr import NWBZarrIO + + +def main(output_path: str) -> None: + if os.path.exists(output_path): + import shutil + shutil.rmtree(output_path) + + session_start = datetime(2024, 3, 15, 10, 30, 0, tzinfo=tzlocal()) + + nwbfile = NWBFile( + session_description="backward compat test session", + identifier="v2-compat-test-id", + session_start_time=session_start, + experimenter=["Alice", "Bob"], + lab="Compat Lab", + institution="Test University", + experiment_description="Testing zarr v2 to v3 backward compatibility", + session_id="session-001", + ) + + # Subject with date_of_birth (datetime, |O dtype with fill_value=0) + from pynwb.file import Subject + nwbfile.subject = Subject( + subject_id="mouse-001", + age="P90D", + species="Mus musculus", + sex="M", + description="Test subject for backward compat", + date_of_birth=datetime(2023, 12, 15, 0, 0, 0, tzinfo=tzlocal()), + ) + + # Device + electrode group + electrodes (object-dtype 'group' column) + device = nwbfile.create_device(name="probe_A", description="Test probe", manufacturer="ACME") + electrode_group = nwbfile.create_electrode_group( + name="shank0", + description="Shank 0 of probe A", + location="brain area X", + device=device, + ) + + n_electrodes = 8 + for i in range(n_electrodes): + nwbfile.add_electrode( + x=float(i), y=0.0, z=0.0, + imp=np.nan, + filtering="none", + group=electrode_group, + location="brain area X", + ) + + # Electrical series (numerical data — should be readable via normal zarr v3 path) + electrode_table_region = nwbfile.create_electrode_table_region( + region=list(range(n_electrodes)), + description="all electrodes", + ) + n_samples = 100 + electrical_series = ElectricalSeries( + name="test_ephys", + data=np.random.randn(n_samples, n_electrodes).astype(np.float64), + electrodes=electrode_table_region, + timestamps=np.linspace(0, 1, n_samples), + description="Test ephys data", + ) + nwbfile.add_acquisition(electrical_series) + + # Write + with NWBZarrIO(path=output_path, mode="w") as io: + io.write(nwbfile) + + # Emit metadata expectations as JSON + expectations = { + "identifier": "v2-compat-test-id", + "session_description": "backward compat test session", + "session_id": "session-001", + "lab": "Compat Lab", + "institution": "Test University", + "experiment_description": "Testing zarr v2 to v3 backward compatibility", + "subject_id": "mouse-001", + "subject_species": "Mus musculus", + "subject_sex": "M", + "subject_age": "P90D", + "n_electrodes": n_electrodes, + "n_devices": 1, + "n_electrode_groups": 1, + "device_name": "probe_A", + "electrode_group_name": "shank0", + "ephys_data_shape": [n_samples, n_electrodes], + "ephys_name": "test_ephys", + } + print(json.dumps(expectations)) + + +if __name__ == "__main__": + if len(sys.argv) < 2: + print("Usage: python generate_v2_nwb_zarr.py ", file=sys.stderr) + sys.exit(1) + main(sys.argv[1]) diff --git a/tests/unit/test_fsspec_streaming.py b/tests/unit/test_fsspec_streaming.py index e69763c9..615d8c1f 100644 --- a/tests/unit/test_fsspec_streaming.py +++ b/tests/unit/test_fsspec_streaming.py @@ -5,7 +5,7 @@ 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") +# @unittest.skip("S3 test files are in zarr v2 format with object_codec which is not supported by zarr v3") class TestFSSpecStreaming(unittest.TestCase): def setUp(self): @@ -50,7 +50,7 @@ def test_is_remote_with_consolidated(self): @unittest.skipIf(not HAVE_FSSPEC, "fsspec not installed") def test_is_remote_without_consolidated(self): """Test that is_remote() returns True for remote HTTPS stores without consolidated metadata.""" - with NWBZarrIO(self.https_s3_path, mode="-r") as read_io: + with NWBZarrIO(self.https_s3_path, mode="r-") as read_io: read_io.open() self.assertTrue(read_io.is_remote()) diff --git a/tests/unit/test_v2_backward_compat.py b/tests/unit/test_v2_backward_compat.py new file mode 100644 index 00000000..6c34e1a8 --- /dev/null +++ b/tests/unit/test_v2_backward_compat.py @@ -0,0 +1,160 @@ +"""Backward compatibility test: read a zarr v2 NWB file with the current (zarr v3) code. + +This test reads a NWB zarr file that was generated with hdmf-zarr<1.0 + zarr<3 +(see ``helpers/generate_v2_nwb_zarr.py``) and validates that key metadata and +data can be read correctly. + +The test file and its expected metadata are produced by the companion generation +script and placed at a known location by CI (see +``.github/workflows/test_backward_compat.yml``). + +To run locally:: + + # Step 1 — generate the v2 file in a temporary venv + python -m venv /tmp/zarr_v2_env + /tmp/zarr_v2_env/bin/pip install "hdmf-zarr<1.0" "zarr>=2.18,<3" pynwb + /tmp/zarr_v2_env/bin/python tests/unit/helpers/generate_v2_nwb_zarr.py \ + tests/unit/helpers/v2_test_file.nwb.zarr \ + > tests/unit/helpers/v2_expected.json + + # Step 2 — run this test with the current code + pytest tests/unit/test_v2_backward_compat.py -v +""" + +import json +import os +import unittest +import warnings + +import numpy as np + +from hdmf_zarr import NWBZarrIO + +# Paths relative to the repo root +_HERE = os.path.dirname(os.path.abspath(__file__)) +_HELPERS = os.path.join(_HERE, "helpers") +_V2_FILE = os.path.join(_HELPERS, "v2_test_file.nwb.zarr") +_V2_EXPECTATIONS = os.path.join(_HELPERS, "v2_expected.json") + +_HAS_V2_FILE = os.path.exists(_V2_FILE) and os.path.exists(_V2_EXPECTATIONS) + + +@unittest.skipIf(not _HAS_V2_FILE, "v2 test file not generated — run generate_v2_nwb_zarr.py first") +class TestV2BackwardCompat(unittest.TestCase): + """Read a zarr v2 NWB file and verify metadata and data integrity.""" + + @classmethod + def setUpClass(cls): + with open(_V2_EXPECTATIONS, "r") as f: + cls.expected = json.load(f) + + with warnings.catch_warnings(): + warnings.simplefilter("always") + cls.io = NWBZarrIO(_V2_FILE, mode="r") + cls.nwbfile = cls.io.read() + + @classmethod + def tearDownClass(cls): + cls.io.close() + + # ---- scalar / string metadata ---- + + def test_identifier(self): + self.assertEqual(self.nwbfile.identifier, self.expected["identifier"]) + + def test_session_description(self): + self.assertEqual(self.nwbfile.session_description, self.expected["session_description"]) + + def test_session_id(self): + self.assertEqual(self.nwbfile.session_id, self.expected["session_id"]) + + def test_lab(self): + self.assertEqual(self.nwbfile.lab, self.expected["lab"]) + + def test_institution(self): + self.assertEqual(self.nwbfile.institution, self.expected["institution"]) + + def test_experiment_description(self): + self.assertEqual(self.nwbfile.experiment_description, self.expected["experiment_description"]) + + # ---- datetime fields (fill_value=0 issue) ---- + + def test_session_start_time(self): + self.assertIsNotNone(self.nwbfile.session_start_time) + + def test_timestamps_reference_time(self): + self.assertIsNotNone(self.nwbfile.timestamps_reference_time) + + # ---- subject (object-dtype date_of_birth) ---- + + def test_subject_exists(self): + self.assertIsNotNone(self.nwbfile.subject) + + def test_subject_id(self): + self.assertEqual(self.nwbfile.subject.subject_id, self.expected["subject_id"]) + + def test_subject_species(self): + self.assertEqual(self.nwbfile.subject.species, self.expected["subject_species"]) + + def test_subject_sex(self): + self.assertEqual(self.nwbfile.subject.sex, self.expected["subject_sex"]) + + def test_subject_age(self): + self.assertEqual(self.nwbfile.subject.age, self.expected["subject_age"]) + + def test_subject_date_of_birth(self): + self.assertIsNotNone(self.nwbfile.subject.date_of_birth) + + # ---- devices / electrode groups (object references) ---- + + def test_n_devices(self): + self.assertEqual(len(self.nwbfile.devices), self.expected["n_devices"]) + + def test_device_name(self): + self.assertIn(self.expected["device_name"], self.nwbfile.devices) + + def test_n_electrode_groups(self): + self.assertEqual(len(self.nwbfile.electrode_groups), self.expected["n_electrode_groups"]) + + def test_electrode_group_name(self): + self.assertIn(self.expected["electrode_group_name"], self.nwbfile.electrode_groups) + + # ---- electrodes table (object-dtype 'group' column) ---- + + def test_n_electrodes(self): + self.assertEqual(len(self.nwbfile.electrodes), self.expected["n_electrodes"]) + + def test_electrodes_columns(self): + col_names = self.nwbfile.electrodes.colnames + for expected_col in ("x", "y", "z", "location", "group", "filtering"): + self.assertIn(expected_col, col_names) + + def test_electrodes_group_column(self): + """The 'group' column has |O dtype with pickle codec — verify it reads.""" + groups = self.nwbfile.electrodes["group"] + self.assertEqual(len(groups), self.expected["n_electrodes"]) + + # ---- acquisition data (numerical, normal zarr path) ---- + + def test_ephys_exists(self): + self.assertIn(self.expected["ephys_name"], self.nwbfile.acquisition) + + def test_ephys_data_shape(self): + series = self.nwbfile.acquisition[self.expected["ephys_name"]] + expected_shape = tuple(self.expected["ephys_data_shape"]) + self.assertEqual(series.data.shape, expected_shape) + + def test_ephys_data_dtype(self): + series = self.nwbfile.acquisition[self.expected["ephys_name"]] + self.assertTrue(np.issubdtype(series.data.dtype, np.floating)) + + def test_ephys_timestamps(self): + series = self.nwbfile.acquisition[self.expected["ephys_name"]] + n_samples = self.expected["ephys_data_shape"][0] + # timestamps could be lazy (zarr Array) or eagerly loaded + ts = np.asarray(series.timestamps) + self.assertEqual(len(ts), n_samples) + + +if __name__ == "__main__": + unittest.main() From d31c9a90a5f3d6982ece9d8cc8087e78cf8f44ce Mon Sep 17 00:00:00 2001 From: Alessio Buccino Date: Thu, 2 Apr 2026 12:15:11 +0200 Subject: [PATCH 2/2] fix ruff --- .github/workflows/test_backward_compat.yml | 4 +-- src/hdmf_zarr/backend.py | 3 +- tests/unit/helpers/generate_v2_nwb_zarr.py | 34 ++++++++++++++-------- 3 files changed, 26 insertions(+), 15 deletions(-) diff --git a/.github/workflows/test_backward_compat.yml b/.github/workflows/test_backward_compat.yml index 8130e81f..7594eac6 100644 --- a/.github/workflows/test_backward_compat.yml +++ b/.github/workflows/test_backward_compat.yml @@ -34,8 +34,8 @@ jobs: - name: Generate zarr v2 NWB test file run: | /tmp/zarr_v2_env/bin/python tests/unit/helpers/generate_v2_nwb_zarr.py \ - tests/unit/helpers/v2_test_file.nwb.zarr \ - > tests/unit/helpers/v2_expected.json + --nwb-output-path tests/unit/helpers/v2_test_file.nwb.zarr \ + --expectations-output-path tests/unit/helpers/v2_expected.json echo "--- generated file tree ---" find tests/unit/helpers/v2_test_file.nwb.zarr -maxdepth 2 -type d | head -30 echo "--- expected metadata ---" diff --git a/src/hdmf_zarr/backend.py b/src/hdmf_zarr/backend.py index 5a1a632e..0cdaa373 100644 --- a/src/hdmf_zarr/backend.py +++ b/src/hdmf_zarr/backend.py @@ -2023,7 +2023,8 @@ def __iter_children(self, zarr_obj): continue # For local stores, skip non-directory entries if isinstance(store, LocalStore): - entry_path = os.path.join(str(store.root), group_prefix, entry) if group_prefix else os.path.join(str(store.root), entry) + entry_path = os.path.join(str(store.root), group_prefix, entry) \ + if group_prefix else os.path.join(str(store.root), entry) if not os.path.isdir(entry_path): continue try: diff --git a/tests/unit/helpers/generate_v2_nwb_zarr.py b/tests/unit/helpers/generate_v2_nwb_zarr.py index 8707a462..e991dbc3 100644 --- a/tests/unit/helpers/generate_v2_nwb_zarr.py +++ b/tests/unit/helpers/generate_v2_nwb_zarr.py @@ -19,8 +19,8 @@ """ import json -import sys import os +from argparse import ArgumentParser import numpy as np from datetime import datetime from dateutil.tz import tzlocal @@ -30,10 +30,10 @@ from hdmf_zarr import NWBZarrIO -def main(output_path: str) -> None: - if os.path.exists(output_path): +def main(nwb_output_path: str, expectations_output_path: str = None) -> None: + if os.path.exists(nwb_output_path): import shutil - shutil.rmtree(output_path) + shutil.rmtree(nwb_output_path) session_start = datetime(2024, 3, 15, 10, 30, 0, tzinfo=tzlocal()) @@ -94,7 +94,7 @@ def main(output_path: str) -> None: nwbfile.add_acquisition(electrical_series) # Write - with NWBZarrIO(path=output_path, mode="w") as io: + with NWBZarrIO(path=nwb_output_path, mode="w") as io: io.write(nwbfile) # Emit metadata expectations as JSON @@ -117,11 +117,21 @@ def main(output_path: str) -> None: "ephys_data_shape": [n_samples, n_electrodes], "ephys_name": "test_ephys", } - print(json.dumps(expectations)) - - + with open(expectations_output_path, "w") as f: + json.dump(expectations, f) + + +parser = ArgumentParser(description="Generate a zarr v2 NWB file for backward compatibility tests.") +parser.add_argument( + "--nwb-output-path", + type=str, + help="Path to write the output NWB zarr file (e.g. /tmp/test_file.zarr)", +) +parser.add_argument( + "--expectations-output-path", + type=str, + help="Path to write the JSON file with metadata expectations (e.g. /tmp/expectations.json)", +) if __name__ == "__main__": - if len(sys.argv) < 2: - print("Usage: python generate_v2_nwb_zarr.py ", file=sys.stderr) - sys.exit(1) - main(sys.argv[1]) + args = parser.parse_args() + main(args.nwb_output_path, args.expectations_output_path)