Replace the remote HDF5 reference translation layer with native HDF5 indexing and direct fsspec range reads - #712
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings remain in HDF5 validation, serialization, decoding, and resource lifecycle handling.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Replaces kerchunk-based remote HDF5 translation with native h5py indexing, fsspec range reads, and direct filter decoding.
Changes:
- Adds versioned index persistence and sharing across arrays and stores.
- Supports direct deflate, shuffle, and Blosc2 decoding with h5py fallback.
- Removes kerchunk dependencies and updates tests and documentation.
File summaries
| File | Description |
|---|---|
tests/test_remote_store.py |
Updates index-sharing tests. |
tests/test_remote_array.py |
Renames reference terminology. |
tests/test_hdf5_source.py |
Adds native reader and persistence coverage. |
tests/test_fsspec.py |
Updates HDF5 index option tests. |
tests/test_b2z_source.py |
Adjusts optional dependency isolation. |
tests/b2view/test_hierarchy.py |
Tests native HDF5 discovery. |
src/blosc2/schunk.py |
Propagates the public hdf5_index API. |
src/blosc2/remote_store.py |
Uses shared native indexes and manages source lifecycle. |
src/blosc2/remote_array.py |
Persists and shares native indexes. |
src/blosc2/hdf5_source.py |
Implements scanning, decoding, validation, and fallback handling. |
pyproject.toml |
Removes kerchunk dependencies. |
plans/remote-hdf5.md |
Documents the native-reader architecture. |
doc/reference/remotestore.rst |
Updates RemoteStore documentation. |
doc/reference/remotearray.rst |
Documents native HDF5 indexing. |
doc/reference/hdf5ndsource.rst |
Documents direct and fallback pipelines. |
doc/guides/remote_arrays.md |
Updates remote HDF5 guidance. |
doc/guides/b2view.rst |
Updates HDF5 browser requirements. |
doc/getting_started/installation.rst |
Updates installation guidance. |
Review details
Suppressed comments (8)
src/blosc2/hdf5_source.py:326
available_datasets()now unconditionally imports fsspec for every.jsonpath. Because fsspec is optional and local native index paths are supported,available_datasets("/local/index.json")fails in an hdf5-only installation even though the same path can be loaded byHDF5NDSource; use the built-inopen()for paths without a URL scheme and reserve fsspec for remote indexes.
if url_str.endswith(".json"):
import fsspec
with fsspec.open(url_str, "r", **(storage_options or {})) as file:
return sorted(validate_hdf5_index(json.load(file))["datasets"])
src/blosc2/hdf5_source.py:423
- After
_open_local()this object owns a live h5py file, but an exception from_init_geometry()or the subsequent identity setup leaves__init__without explicitly closing_file_finalizer. The previous scoped setup closed local files on failed initialization; retaining the handle until garbage collection can leak descriptors and Windows file locks on the existing invalid-dtype/geometry error paths.
if self._local:
self._open_local()
self._metadata = None
shape, physical_chunks, dtype = self.array.shape, self.array.chunks, self.array.dtype
src/blosc2/hdf5_source.py:418
- This condition routes any local source with an explicit
hdf5_indexthrough the remote branch, which callscheck_hdf5_dependencies()and requires fsspec. That breaks the documented local indexed-reader case for users who installed onlyblosc2[hdf5]; keep indexed local paths on the h5py/local path or make the dependency contract explicit.
self._local = (not remote or os.path.isabs(urlpath)) and hdf5_index is None and _filesystem is None
src/blosc2/hdf5_source.py:461
- The URL split searches the entire URL, including its query string, when extracting the dataset. A direct call such as
HDF5NDSource('https://host/file.h5/group?token=x', ...)therefore treatsgroup?token=xas the dataset (or reports a mismatch) instead of preserving the query on the file URL, so signed/query-bearing URLs fail unless callers pre-normalize them.
for ext in (".h5/", ".hdf5/"):
index = lower.find(ext)
if index != -1:
end, embedded = index + len(ext) - 1, urlpath[index + len(ext) :].strip("/")
if dataset is not None and dataset != embedded:
src/blosc2/hdf5_source.py:242
- The legacy-map detector only recognizes keys ending in
/.zarray. A flat legacy reference map for a root dataset uses the key.zarray, so it reaches the genericInvalid HDF5 index formaterror and misses the promised migration guidance. Include the root.zarray/.zgroupforms in this check.
if index.get("format") != HDF5_INDEX_FORMAT:
if "refs" in index or any(str(key).endswith("/.zarray") for key in index):
raise ValueError(
"Legacy HDF5 reference maps are unsupported; omit hdf5_index and rescan the source"
)
src/blosc2/remote_array.py:712
- This new standalone-close path does not update any closed state, while
_check_open()only detects closed store-derived handles. AfterRemoteArray.close(), a local HDF5 source fails unpredictably on the next read, whereas a remote fallback can reopen its HDF5 file; the same API is therefore neither consistently closed nor rejected. Mark standalone handles closed and enforce that in_check_open(), or preserve the documented no-op semantics.
elif isinstance(getattr(self, "src", None), blosc2.HDF5NDSource):
self.src.close()
src/blosc2/remote_array.py:712
- Standalone
RemoteArray.close()callsHDF5NDSource.close()without taking_operation_lock, while__getitem__andfetchserialize source use with that lock. For a local HDF5 source this can close the h5py file while an in-flight read is usingself.array; serialize this close with the same operation lock.
elif isinstance(getattr(self, "src", None), blosc2.HDF5NDSource):
self.src.close()
tests/test_fsspec.py:773
zarris still imported withpytest.importorskip()immediately after the removedkerchunkguard in both HDF5 HTTP tests. Since this reader no longer uses Zarr, those tests silently skip when validating the new h5py/fsspec-only dependency set; remove the remaining Zarr guard so the range-reader coverage runs with the declared dependencies.
h5py = pytest.importorskip("h5py")
pytest.importorskip("zarr")
- Files reviewed: 18/18 changed files
- Comments generated: 7
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Copilot's review also listed 8 suppressed findings. All are addressed in this branch:
|
There was a problem hiding this comment.
🟡 Changes recommended
Critical HDF5 decoding-validation findings and additional lifecycle, serialization, and option-validation issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (8)
src/blosc2/hdf5_source.py:206
- When
_filesystemis omitted,_filesystem_and_path()creates an fsspec filesystem here, but this function only closes the HDF5/file handles. Publicscan_hdf5_index()and the remoteavailable_datasets()path can therefore retain owned HTTP/S3 sessions after every scan; track ownership and close the owned session in afinally, while leaving supplied filesystems open.
fs, path = _filesystem_and_path(urlpath, storage_options, _filesystem)
groups, datasets = {"": {"attrs": {}}}, {}
with fs.open(path, "rb", block_size=1, cache_type="none") as raw:
src/blosc2/hdf5_source.py:664
- The new explicit
close()path does not coordinate with direct reads:_direct_values()usesself._filesystem.cat_file()without the lifecycle lock, whileclose()can close the session and set_filesystemtoNone. A standalone source closed concurrently with a fetch can therefore fail mid-read or dereference the cleared filesystem; add close-versus-read coordination without serializing independent direct fetches unnecessarily.
def close(self):
with self._fallback_lock:
fallback_finalizer = getattr(self, "_fallback_finalizer", None)
if fallback_finalizer is not None:
fallback_finalizer()
self._fallback_h5 = self._fallback_file = None
finalizer = getattr(self, "_file_finalizer", None)
if finalizer is not None:
finalizer()
filesystem = getattr(self, "_filesystem", None)
if filesystem is not None and self._external_filesystem is None:
# fsspec's HTTP and S3 clients keep an async session alive after
# the file objects it feeds are gone.
close = getattr(filesystem, "close_session", None)
session = getattr(filesystem, "_s3creator", None) or getattr(filesystem, "_session", None)
if close is not None and session is not None:
close(filesystem.loop, session)
self._filesystem = None
src/blosc2/hdf5_source.py:157
- This helper gets the default fsspec instance-cached filesystem, but
HDF5NDSource.close()now closes that filesystem's session whenever_external_filesystemisNone. Two standalone sources for the same HTTP/S3 URL can therefore share one cached client and closing either invalidates the other. Create a private filesystem (skip_instance_cache=True) for source-owned transports, or avoid closing shared instances.
return fsspec.core.url_to_fs(urlpath, **(storage_options or {}))
src/blosc2/msgpack_utils.py:86
- The new ndarray msgpack extension has the same nested-object reconstruction problem: object arrays containing ndarray/list elements are inferred as a multidimensional or ragged array before
reshape(shape), so exporting such an HDF5 attribute can fail or change its structure. Rebuild object arrays by assigning each decoded value into annp.empty(shape, dtype=object)result.
if "values" in payload:
return np.array(payload["values"], dtype=object).reshape(shape)
src/blosc2/msgpack_utils.py:77
- The nested payload is encoded with raw
packband nodefaulthook, so object arrays containing supported values such asnp.int64, complex numbers, sets, or nested ndarrays fail withTypeErroreven though the outer msgpack serializer handles them. Use the project extension hook for this nested pack as well.
return ExtType(_BLOSC2_NDARRAY_EXT_CODE, packb(payload, use_bin_type=True))
src/blosc2/remote_array.py:557
RemoteArray.__init__now exposes the publichdf5_indexargument, but the class docstring's parameter list does not document it. DirectRemoteArray(...)users need the accepted native-index format and the legacy-map migration behavior; theopen()docstring alone does not cover this constructor.
hdf5_index=None,
src/blosc2/schunk.py:2235
- Because this is popped before
_validate_non_lazy_fsspec_options,open(..., lazy=False, cache_dir=..., hdf5_index=...)still raises the genericcache_policy and max_cache_bytes require lazy=Trueerror instead of the option-specifichdf5_indexerror expected by the new regression test. With no cache directory, the popped index can also be ignored on the eager path. Reject a non-lazyhdf5_indexexplicitly before the generic cache-policy validation.
hdf5_index = kwargs.pop("hdf5_index", None)
tests/test_hdf5_source.py:362
- These new Blosc2-filter tests are skipped in the repository's default test job:
.github/workflows/build.ymlinstalls only--group test, whilepyproject.toml's test group includesh5pybut nothdf5plugin. As a result, the new direct Blosc2 path (and the fallback plugin path below) can remain unexecuted in CI; add the plugin to the required test environment or run a required plugin-filter job.
plugin = pytest.importorskip("hdf5plugin")
- Files reviewed: 19/19 changed files
- Comments generated: 5
- Review effort level: Lite
|
Second review round: the 5 threaded findings are fixed and resolved. The 8 suppressed findings are also addressed:
|
There was a problem hiding this comment.
🟡 Changes recommended
Critical routing and filesystem-ownership issues, plus additional validation and lifecycle defects, remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (7)
Previously missed (1) — in code that hasn't changed since the last review.
src/blosc2/hdf5_source.py:302
- This validation treats missing dataset fields as defaults (
shape=()andchunks=None) and does not requirefill_value,attrs, or the other fields emitted by the scanner. Consequently an incomplete explicit index can pass validation; for example, a direct sparse entry withoutfill_valuefails later with aKeyError, while a fallback entry withoutshapecan be opened with the wrong geometry. Reject incomplete dataset entries before applying the type checks.
src/blosc2/hdf5_source.py:510
- The source-owned filesystem is created before the remote index load and dataset validation, but those operations are outside the
try/self.close()block starting below. If scanning, loading a malformed index, or rejecting a missing dataset raises, this standalone source leaks its HTTP/S3 session because the constructor never closes the filesystem it created. Put the remote setup under the same cleanup boundary (or close it in an exception path).
self._filesystem, self._path = _filesystem_and_path(urlpath, storage_options, _filesystem)
self._hdf5_index = self._load_or_scan_index(hdf5_index)
self._validate_dataset_presence(dataset)
src/blosc2/hdf5_source.py:663
- The fill-value path runs before the closed-state check below. After closing a sparse direct HDF5 source, requests for an unallocated chunk therefore return its fill value successfully, while requests for allocated chunks raise
RuntimeError; the new close contract is inconsistent for valid sparse datasets. Check that the source is open before looking uprecordand construct/return the fill result while holding the lifecycle lock so close cannot race that path.
record = self._chunk_records.get(offsets)
valid_shape = tuple(item.stop - item.start for item in selection)
if record is None:
return np.full(valid_shape, _from_json_value(self._metadata["fill_value"]), dtype=self.dtype)
src/blosc2/hdf5_source.py:509
- Remote sources do not register a finalizer for their owned filesystem: only local files get
_file_finalizer, and_fallback_finalizercloses HDF5/raw handles but not the fsspec session. A standaloneHDF5NDSourceorRemoteArrayabandoned without an explicitclose()can therefore leave an HTTP/S3 session alive, especially before the first fallback read. Add an ownership-aware filesystem finalizer and detach it inclose().
self._filesystem, self._path = _filesystem_and_path(urlpath, storage_options, _filesystem)
self._hdf5_index = self._load_or_scan_index(hdf5_index)
src/blosc2/hdf5_source.py:756
- This catches every
OSErrorfrom the retained h5py/fsspec reader and rewrites it as a filter/plugin installation failure. HTTP transport errors, permission failures, and corrupt files can therefore be reported as “install hdf5plugin,” hiding the actionable original cause; preserve non-filter errors and add plugin guidance only for filter-registration failures.
with self._fallback_lock:
try:
values = self._open_fallback()[selection]
except OSError as exc:
filters = [item["id"] for item in self._metadata["filters"]]
raise OSError(
f"Cannot decode HDF5 dataset {self.dataset!r} with filters {filters}; "
"install hdf5plugin if the file uses an optional HDF5 filter"
) from exc
src/blosc2/hdf5_source.py:744
- After
HDF5NDSource.close()closes a local file,_localremains true and this branch still dereferencesself.arraywithout a closed-state check. Unlike the remote direct/fallback branches, a closed local source therefore raises h5py's backend-specific invalid-identifier error (and can race a local read) instead of consistently rejecting the operation as closed. Track a closed state and check it before local reads.
if self._local:
values = self.array[selection]
src/blosc2/remote_array.py:729
- This new standalone branch marks every
RemoteArrayas closed, including B2Z, Zarr, and plain fsspec arrays, even though it only needs to close an owned HDF5 source. That regresses the existing documented no-op lifetime for standalone non-HDF5 handles: simply exiting their context now makes later operations raiseRuntimeError. Restrict the closed-state transition here to standalone HDF5 sources and update the public lifecycle documentation to match.
with self._operation_lock:
# Serialize against in-flight reads before closing the source.
self._closed = True
if isinstance(getattr(self, "src", None), blosc2.HDF5NDSource):
self.src.close()
- Files reviewed: 19/19 changed files
- Comments generated: 3
- Review effort level: Lite
|
Third review round: the 3 threaded findings are fixed and resolved. The 7 suppressed findings are also addressed:
|
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved issues remain in local-file dependency handling, index format resolution, sidecar validation, and HDF5 lifecycle documentation.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
src/blosc2/hdf5_source.py:161
scan_hdf5_indexis documented as handling local or remote containers, but it callscheck_hdf5_dependencies()and then_filesystem_and_path()unconditionally imports fsspec. Since thehdf5extra intentionally does not install fsspec and local HDF5 access is advertised as h5py-only, calling the public scanner on a local path fails before it can use h5py. Select local-file handling first, checking only h5py there and retaining fsspec for remote URLs.
src/blosc2/remote_array.py:561hdf5_indexis now a publicRemoteArrayparameter, but this constructor still resolvessource_formatonly from the URL and dataset. ConsequentlyRemoteArray("memory://container", dataset="data", hdf5_index=index)is rejected as a non-HDF5 dataset, while supplying the index with a detected Zarr/Blosc2 URL silently ignores it. Make the constructor's resolver force HDF5 for a non-conflicting index and reject conflicting formats, matching_open_fsspec_url.
src/blosc2/remote_array.py:714
- This changes standalone HDF5
RemoteArraycontext exits to close the source, but the publicopen()notes insrc/blosc2/schunk.pystill say that exiting a standaloneRemoteArraycontext is a no-op. Users following that documentation can reuse a handle afterwithand now receiveRuntimeError; update the API note to distinguish HDF5 handles from other standalone RemoteArrays.
def close(self):
"""Release this handle and any HDF5 file resources it owns.
Closing is idempotent. Standalone HDF5 handles reject further
operations; other standalone handles keep their no-op close behavior.
src/blosc2/remote_array.py:627
- The shared sidecar is described as disposable, but only decompression/JSON syntax errors are suppressed here. A valid JSON object with an invalid format, version, URL binding, or dataset entry is assigned to
hdf5_index;HDF5NDSourcethen raises during validation instead of rescanning, so a damaged sidecar can prevent future opens. Validate the candidate before assigning it (and discard it on validation failure).
if hdf5_index is None and shared_index_path is not None:
# Disposable metadata: an absent or damaged snapshot needs a fresh scan.
with contextlib.suppress(OSError, ValueError, RuntimeError):
hdf5_index = json.loads(blosc2.decompress(shared_index_path.read_bytes()))
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Fourth review round (no inline threads were generated). The 4 suppressed findings are addressed:
|
Remote files are scanned once with h5py to record dataset metadata and allocated chunk ranges. Uncompressed, deflate, shuffle, and Blosc2 pipelines are decoded directly. Other HDF5 filters use a retained h5py fallback, including filters registered by hdf5plugin.
The native index is shared across RemoteStore leaves and persisted in array carriers and disk caches, avoiding repeated metadata scans on warm opens. The public hdf5_index= argument accepts an existing native index; legacy reference maps are rejected with migration guidance.
This also removes the former translation dependency from the HDF5, development, and test dependency groups. Remote HDF5 now requires only h5py and fsspec, plus the appropriate fsspec protocol backend.