From 910d81904c1ef5e48977a8095d1b6049967c9032 Mon Sep 17 00:00:00 2001 From: osobh Date: Sun, 27 Sep 2026 06:40:44 -0500 Subject: [PATCH 1/9] py: remote files (clawhdf5.File(url), File.open_url) through File::storage() The Python bindings could not open a remote file: they parsed through File::as_bytes() in eight places (path lookups, object headers, dataspaces, attributes, group listings, the global heap of variable-length data), which a storage-backed file does not have. - Every object of a File now shares one handle (src/handle.rs) that runs all file access, metadata included, with the GIL released and parses through File::storage() and the clawhdf5_format *_in functions. Local files take the same path (their storage is the mmap). - clawhdf5.File(url) opens any scheme://... through clawhdf5_remote::storage_for_url (read-only; another mode is a ValueError). File.open_url(url, **options) takes the cache and HTTP options (block_size, cache_size, headers, retries, timeout, allow_full_download, max_full_download, require_validator, max_redirects, max_parallel); File.remote_stats gives the block cache's counters. - Default build: plain HTTP only, no C. https (rustls/ring) and s3/gcs/azure (aws-lc-rs) are opt-in features of clawhdf5-py, and ci-test.sh's no-C check now covers the crate. - A failed storage read (network error, file changed on the server) is an OSError, never KeyError/ValueError and never data; `key in group` raises it instead of answering False. Tests: the read-vs-h5py suite runs locally and over HTTP (1 MiB and 1 KiB blocks) against a range-capable http.server in the test process (conftest.RangeServer); test_remote.py covers request counts, cache hits, a server without Range support, a changed file, a server that hangs up, 16 threads, and a spinning thread that keeps running while a read waits on 0.2 s requests. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 30 +++ README.md | 12 + crates/clawhdf5-py/Cargo.toml | 9 + crates/clawhdf5-py/README.md | 35 ++- crates/clawhdf5-py/src/attrs.rs | 42 +-- crates/clawhdf5-py/src/convert.rs | 70 +++-- crates/clawhdf5-py/src/dataset.rs | 144 +++++++---- crates/clawhdf5-py/src/file.rs | 206 ++++++++++++--- crates/clawhdf5-py/src/group.rs | 151 +++++------ crates/clawhdf5-py/src/handle.rs | 130 ++++++++++ crates/clawhdf5-py/src/lib.rs | 7 +- crates/clawhdf5-py/src/node.rs | 165 ++++++------ crates/clawhdf5-py/tests/conftest.py | 135 ++++++++++ crates/clawhdf5-py/tests/test_read_vs_h5py.py | 25 +- crates/clawhdf5-py/tests/test_remote.py | 244 ++++++++++++++++++ docs/design/range-reads.md | 18 +- docs/known-issues.md | 15 +- scripts/ci-test.sh | 7 +- 18 files changed, 1146 insertions(+), 299 deletions(-) create mode 100644 crates/clawhdf5-py/src/handle.rs create mode 100644 crates/clawhdf5-py/tests/test_remote.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 664a831..6b153b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,36 @@ ## Unreleased +### Python bindings: remote files (2026-09-27) +- **`clawhdf5.File(url)`** opens `http://` URLs (and `https://`, `s3://`, + `gs://`, `az://` in a wheel built with the `https`, `s3`, `gcs`, + `azure` features) through `clawhdf5-remote`'s `open_url`: range requests + through the block cache, the whole read API (groups, attributes, every + dataset type and index the local reader handles). A URL is any + `scheme://…`; a remote file is read-only (another mode is a + `ValueError`). **`clawhdf5.File.open_url(url, **options)`** takes + `block_size`, `cache_size`, `headers`, `retries`, `timeout`, + `allow_full_download`, `max_full_download`, `require_validator`, + `max_redirects` and `max_parallel`; `File.remote_stats` gives the block + cache's counters. The default wheel builds plain HTTP only (no C: rustls + needs ring, and the cloud clients aws-lc-rs), and `ci-test.sh`'s no-C + check now covers `clawhdf5-py`. +- **Every read parses through `File::storage()`** instead of + `File::as_bytes()` (path lookups, object headers, dataspaces, attributes, + group listings, variable-length data through the global heap), inside one + shared file handle that releases the GIL for all file access, not only + dataset reads: a read waiting on the network lets other Python threads + run. A failed read of the storage (a network error, a file changed on + the server) is an `OSError`, never a `KeyError`/`ValueError` and never + data; `key in group` raises it instead of answering `False`. +- Tests: the whole read-vs-h5py suite also runs over HTTP (default 1 MiB + blocks and 1 KiB blocks) against a range-capable `http.server` in the + test process, plus `tests/test_remote.py`: request counts of a small + read, cache hits, a server without `Range` support (refused, or a + whole download when allowed), a file changed on the server, a server + that hangs up, 16 threads on one remote file, and a thread that keeps + running while a read waits on 0.2 s requests. + ### Range reads, milestone M3: remote files (2026-09-26) - **New crate `clawhdf5-remote`.** `open_url("http://host/file.h5")` gives a `clawhdf5::File` (through `File::open_storage`) that reads the file by diff --git a/README.md b/README.md index 3301b09..dccee48 100644 --- a/README.md +++ b/README.md @@ -533,8 +533,20 @@ with clawhdf5.File("data.h5", "r") as f: records = f["table"] # compound -> numpy structured array ids = records["id"] # one field + +# A file on a web server: range requests through a block cache, nothing +# downloaded up front; the same read API. The GIL is released while waiting. +with clawhdf5.File("http://data.example.org/run42.h5") as f: + first = f["group/temperatures"][0] +f = clawhdf5.File.open_url("http://data.example.org/run42.h5", block_size=256 * 1024, + headers={"Authorization": "Bearer ..."}) ``` +The default build reads `http://` URLs only; build with +`maturin develop --release --features https` (rustls with ring, which +compiles C) for `https://`, and `--features s3` (or `gcs`, `azure`) for +object-store URLs. + Reads cover integers and IEEE floats of every width in either byte order, `bool`, enums, complex, fixed and variable-length strings, variable-length sequences, opaque, HDF5 array types and compounds; other types (references, diff --git a/crates/clawhdf5-py/Cargo.toml b/crates/clawhdf5-py/Cargo.toml index 3513a62..8246440 100644 --- a/crates/clawhdf5-py/Cargo.toml +++ b/crates/clawhdf5-py/Cargo.toml @@ -17,11 +17,20 @@ crate-type = ["cdylib", "rlib"] [dependencies] clawhdf5_rs = { path = "../clawhdf5", version = "2.7.0", package = "clawhdf5" } clawhdf5-format = { path = "../clawhdf5-format", version = "2.7.0" } +# Remote files (`clawhdf5.File(url)`): plain HTTP by default, which builds +# no C. HTTPS and the object stores are opt-in features below. +clawhdf5-remote = { path = "../clawhdf5-remote", version = "2.7.0" } pyo3 = "0.29" numpy = "0.29" [features] extension-module = ["pyo3/extension-module"] +# https:// URLs (rustls with ring, which compiles C and assembly). +https = ["clawhdf5-remote/https"] +# s3://, gs://, az:// URLs (object_store; its cloud clients build aws-lc-rs, C). +s3 = ["clawhdf5-remote/s3"] +gcs = ["clawhdf5-remote/gcs"] +azure = ["clawhdf5-remote/azure"] [package.metadata.docs.rs] features = [] diff --git a/crates/clawhdf5-py/README.md b/crates/clawhdf5-py/README.md index 95cae12..16e4169 100644 --- a/crates/clawhdf5-py/README.md +++ b/crates/clawhdf5-py/README.md @@ -61,6 +61,36 @@ with clawhdf5.File("data.h5", "r") as f: - Attributes return what h5py returns; `clawhdf5.Empty` stands for a null dataspace (h5py's `Empty`). +## Remote files + +A URL instead of a path reads the file where it is, through +`clawhdf5-remote`: HTTP range requests through a block cache (1 MiB blocks, +64 MiB budget by default), fetching only the blocks a read needs. The whole +read API works the same, and the GIL is released while waiting on the +network. + +```python +f = clawhdf5.File("http://host/data.h5") # default options +f = clawhdf5.File.open_url( + "http://host/data.h5", + block_size=256 * 1024, cache_size=128 << 20, # the block cache + headers={"Authorization": "Bearer ..."}, # sent to this origin only + retries=3, timeout=30.0, max_redirects=5, max_parallel=8, + allow_full_download=False, # a server without Range support: refuse + require_validator=False, # refuse servers without ETag/Last-Modified +) +f.remote_stats # {'requests': ..., 'bytes_fetched': ..., 'hits': ..., ...} +``` + +- The file is pinned when opened (ETag or Last-Modified, and length): if it + changes on the server, reads raise `OSError` instead of mixing versions. + Network failures are `OSError` too. +- Remote files are read-only. +- Schemes: the default build (no C) reads `http://`. `https://` needs + `maturin develop --release --features https` (rustls with ring, which + compiles C); `s3://`, `gs://` and `az://` need the `s3`, `gcs` and + `azure` features (credentials from the environment; aws-lc-rs, C). + ## Writing `clawhdf5.File(path, "w")` with `create_dataset(name, data=array, @@ -76,7 +106,10 @@ pytest crates/clawhdf5-py/tests ``` `tests/test_read_vs_h5py.py` compares every read with h5py on a file h5py -writes. `scripts/ci-test.sh` builds the wheel and runs these in CI. +writes, opened locally and over HTTP (an in-process range server, +`tests/conftest.py`); `tests/test_remote.py` checks remote reads (requests, +failures, the GIL). `scripts/ci-test.sh` builds the wheel and runs these +in CI. ## License diff --git a/crates/clawhdf5-py/src/attrs.rs b/crates/clawhdf5-py/src/attrs.rs index fd3f0b3..615214b 100644 --- a/crates/clawhdf5-py/src/attrs.rs +++ b/crates/clawhdf5-py/src/attrs.rs @@ -8,13 +8,14 @@ use pyo3::prelude::*; use pyo3::types::{PyList, PyTuple}; use crate::convert::{Converter, Elements, resolve_vl}; +use crate::handle::Handle; use crate::{OwnedAttrValue, PyEmpty, attr_value_to_py, node, py_to_attr_value}; /// Backing storage for attributes. enum AttrsInner { /// Attributes of an object in a file opened for reading, sorted by name. Read { - file: Arc, + handle: Arc, attrs: Vec, }, /// Writable attribute list shared with a parent (PyFile or PyGroup). @@ -36,10 +37,15 @@ pub struct PyAttrs { impl PyAttrs { /// The attributes of the object at `addr` (whose path is `path`) in a /// file opened for reading. - pub(crate) fn read(file: Arc, addr: u64, path: &str) -> PyResult { - let attrs = node::attributes(&file, addr, path)?; + pub(crate) fn read( + py: Python<'_>, + handle: Arc, + addr: u64, + path: &str, + ) -> PyResult { + let attrs = handle.with(py, |f| node::attributes(f, addr, path))?; Ok(Self { - inner: AttrsInner::Read { file, attrs }, + inner: AttrsInner::Read { handle, attrs }, }) } @@ -55,8 +61,8 @@ impl PyAttrs { impl PyAttrs { fn __getitem__(&self, py: Python<'_>, key: &str) -> PyResult> { match &self.inner { - AttrsInner::Read { file, attrs } => match attrs.iter().find(|a| a.name == key) { - Some(attr) => Ok(attr_to_py(py, file, attr)?.unbind()), + AttrsInner::Read { handle, attrs } => match attrs.iter().find(|a| a.name == key) { + Some(attr) => Ok(attr_to_py(py, handle, attr)?.unbind()), None => Err(PyKeyError::new_err(format!( "Can't open attribute (can't locate attribute: '{key}')" ))), @@ -146,9 +152,9 @@ impl PyAttrs { /// Return attribute values as a list. fn values(&self, py: Python<'_>) -> PyResult> { let vals: Vec> = match &self.inner { - AttrsInner::Read { file, attrs } => attrs + AttrsInner::Read { handle, attrs } => attrs .iter() - .map(|a| attr_to_py(py, file, a).map(Bound::unbind)) + .map(|a| attr_to_py(py, handle, a).map(Bound::unbind)) .collect::>()?, AttrsInner::Write(store) => store .lock() @@ -167,9 +173,9 @@ impl PyAttrs { /// Return attribute (key, value) pairs as a list of tuples. fn items(&self, py: Python<'_>) -> PyResult> { let pairs: Vec<(String, Py)> = match &self.inner { - AttrsInner::Read { file, attrs } => attrs + AttrsInner::Read { handle, attrs } => attrs .iter() - .map(|a| Ok((a.name.clone(), attr_to_py(py, file, a)?.unbind()))) + .map(|a| Ok((a.name.clone(), attr_to_py(py, handle, a)?.unbind()))) .collect::>()?, AttrsInner::Write(store) => store .lock() @@ -189,12 +195,11 @@ impl PyAttrs { /// An attribute's value as h5py returns it. fn attr_to_py<'py>( py: Python<'py>, - file: &clawhdf5_rs::File, + handle: &Handle, attr: &AttributeMessage, ) -> PyResult> { crate::no_panic(|| { - let sb = file.superblock(); - let conv = Converter::new(py, &attr.datatype, sb.offset_size) + let conv = Converter::new(py, &attr.datatype, handle.offset_size) .map_err(|e| prefix_err(py, &attr.name, e))?; if node::is_null(&attr.dataspace) { return Ok(PyEmpty::new(conv.dtype).into_pyobject(py)?.into_any()); @@ -216,12 +221,11 @@ fn attr_to_py<'py>( ))); } let raw = &attr.raw_data[..want]; - let file_data = file.as_bytes(); - let (osz, lsz, unit) = (sb.offset_size, sb.length_size, conv.vl_unit); - Elements::Vl( - py.detach(|| resolve_vl(file_data, raw, n, osz, lsz, unit)) - .map_err(|e| PyValueError::new_err(format!("attribute {}: {e}", attr.name)))?, - ) + let (osz, lsz, unit) = (handle.offset_size, handle.length_size, conv.vl_unit); + let what = format!("attribute {}", attr.name); + Elements::Vl(handle.with(py, |f| { + resolve_vl(f.storage(), raw, n, osz, lsz, unit).map_err(|e| e.into_py(&what)) + })?) } else { Elements::Bytes(attr.raw_data.clone()) }; diff --git a/crates/clawhdf5-py/src/convert.rs b/crates/clawhdf5-py/src/convert.rs index 4ca7546..c8c3d68 100644 --- a/crates/clawhdf5-py/src/convert.rs +++ b/crates/clawhdf5-py/src/convert.rs @@ -17,6 +17,7 @@ use std::collections::HashMap; use clawhdf5_format::datatype::{CharacterSet, Datatype, DatatypeByteOrder}; use clawhdf5_format::global_heap::GlobalHeapCollection; +use clawhdf5_format::storage::Storage; use numpy::PyArray1; use pyo3::exceptions::{PyTypeError, PyValueError}; use pyo3::prelude::*; @@ -538,16 +539,16 @@ fn object_array<'py>( /// their bytes: each element's stored length times `unit` (1 for strings, /// the base type's size for sequences). Pure Rust, so it runs without the /// GIL. -pub(crate) fn resolve_vl( - file_data: &[u8], +pub(crate) fn resolve_vl( + file: &S, raw: &[u8], count: usize, offset_size: u8, length_size: u8, unit: usize, -) -> Result>, String> { +) -> Result>, VlError> { let refs = clawhdf5_format::vl_data::parse_vl_references(raw, count as u64, offset_size) - .map_err(|e| e.to_string())?; + .map_err(|e| VlError::Invalid(e.to_string()))?; let undefined = match offset_size { 2 => 0xFFFF, 4 => 0xFFFF_FFFF, @@ -558,43 +559,70 @@ pub(crate) fn resolve_vl( for vl in &refs { if vl.collection_address == 0 || vl.collection_address == undefined { if vl.length != 0 { - return Err(format!( + return Err(VlError::Invalid(format!( "variable-length element of length {} has no heap address", vl.length - )); + ))); } out.push(Vec::new()); continue; } let coll = match collections.entry(vl.collection_address) { std::collections::hash_map::Entry::Occupied(e) => e.into_mut(), - std::collections::hash_map::Entry::Vacant(e) => { - let addr = usize::try_from(vl.collection_address) - .map_err(|_| "global heap address out of range".to_string())?; - e.insert( - GlobalHeapCollection::parse(file_data, addr, length_size) - .map_err(|e| e.to_string())?, - ) - } + std::collections::hash_map::Entry::Vacant(e) => e.insert( + GlobalHeapCollection::parse_in(file, vl.collection_address, length_size) + .map_err(VlError::from_format)?, + ), }; - let index = u16::try_from(vl.object_index) - .map_err(|_| format!("global heap object index {} out of range", vl.object_index))?; + let index = u16::try_from(vl.object_index).map_err(|_| { + VlError::Invalid(format!( + "global heap object index {} out of range", + vl.object_index + )) + })?; let obj = coll.get_object(index).ok_or_else(|| { - format!( + VlError::Invalid(format!( "global heap object {index} not found in the collection at {}", vl.collection_address - ) + )) })?; let need = (vl.length as usize) .checked_mul(unit) - .ok_or("variable-length element too long")?; + .ok_or_else(|| VlError::Invalid("variable-length element too long".into()))?; if need > obj.data.len() { - return Err(format!( + return Err(VlError::Invalid(format!( "variable-length element of {need} bytes in a {}-byte heap object", obj.data.len() - )); + ))); } out.push(obj.data[..need].to_vec()); } Ok(out) } + +/// Why variable-length elements could not be resolved. +#[derive(Debug)] +pub(crate) enum VlError { + /// Reading the file failed (a network error on a remote file). + Storage(String), + /// The references or the heap are not valid. + Invalid(String), +} + +impl VlError { + fn from_format(e: clawhdf5_format::error::FormatError) -> Self { + match e { + clawhdf5_format::error::FormatError::Storage(_) => VlError::Storage(e.to_string()), + e => VlError::Invalid(e.to_string()), + } + } + + /// As a Python exception, the message prefixed with `what`: a storage + /// failure is an `OSError`, anything else a `ValueError`. + pub(crate) fn into_py(self, what: &str) -> PyErr { + match self { + VlError::Storage(m) => pyo3::exceptions::PyOSError::new_err(format!("{what}: {m}")), + VlError::Invalid(m) => PyValueError::new_err(format!("{what}: {m}")), + } + } +} diff --git a/crates/clawhdf5-py/src/dataset.rs b/crates/clawhdf5-py/src/dataset.rs index bcfd852..24c9b51 100644 --- a/crates/clawhdf5-py/src/dataset.rs +++ b/crates/clawhdf5-py/src/dataset.rs @@ -6,21 +6,53 @@ //! whole dataset instead); the //! bytes it returns become the numpy array's buffer without a copy (see //! `convert`). All file access and decoding runs with the GIL released, so -//! Python threads reading the same or different datasets run in parallel. +//! Python threads reading the same or different datasets run in parallel, +//! and a remote file's network reads never hold the GIL. use std::sync::Arc; use clawhdf5_format::datatype::Datatype; use clawhdf5_format::object_header::ObjectHeader; +use clawhdf5_rs::File; use pyo3::exceptions::{PyTypeError, PyValueError}; use pyo3::prelude::*; use pyo3::types::{PyList, PyTuple}; use crate::attrs::PyAttrs; -use crate::convert::{Converter, Elements, resolve_vl}; +use crate::convert::{Converter, Elements, VlError, resolve_vl}; +use crate::handle::Handle; use crate::select::{self, Plan}; use crate::{PyEmpty, node, to_py_err}; +/// What opening a dataset reads from the file (without the GIL). +pub(crate) struct DatasetMeta { + /// `None` for a dataset with a null dataspace (h5py's `Empty`). + shape: Option>, + chunks: Option>, + datatype: Datatype, +} + +impl DatasetMeta { + pub(crate) fn load(f: &File, addr: u64, hdr: &ObjectHeader, path: &str) -> PyResult { + let null = node::is_null(&node::dataspace(f, hdr, path)?); + let ds = f.dataset_at(addr).map_err(to_py_err)?; + let shape = if null { + None + } else { + Some(ds.shape().map_err(to_py_err)?) + }; + let datatype = ds.raw_datatype().map_err(to_py_err)?; + let chunks = shape + .as_ref() + .and_then(|s| node::chunk_shape(f, hdr, s.len())); + Ok(Self { + shape, + chunks, + datatype, + }) + } +} + /// A dataset in a file opened for reading. /// /// ```python @@ -30,7 +62,7 @@ use crate::{PyEmpty, node, to_py_err}; /// ``` #[pyclass(name = "Dataset")] pub struct PyDataset { - file: Arc, + handle: Arc, path: String, /// Where the dataset's object header is: reads open it from here rather /// than resolve `path` again. @@ -45,39 +77,24 @@ pub struct PyDataset { } impl PyDataset { - pub(crate) fn open( + pub(crate) fn new( py: Python<'_>, - file: Arc, + handle: Arc, path: String, addr: u64, - hdr: &ObjectHeader, - ) -> PyResult { - crate::no_panic(|| { - let null = node::is_null(&node::dataspace(&file, hdr)?); - let (shape, datatype) = { - let ds = file.dataset_at(addr).map_err(to_py_err)?; - let shape = if null { - None - } else { - Some(ds.shape().map_err(to_py_err)?) - }; - (shape, ds.raw_datatype().map_err(to_py_err)?) - }; - let conv = Converter::new(py, &datatype, file.superblock().offset_size) - .map_err(|e| e.value(py).to_string()); - let chunks = shape - .as_ref() - .and_then(|s| node::chunk_shape(&file, hdr, s.len())); - Ok(Self { - file, - path, - addr, - shape, - chunks, - datatype, - conv, - }) - }) + meta: DatasetMeta, + ) -> Self { + let conv = crate::no_panic(|| Converter::new(py, &meta.datatype, handle.offset_size)) + .map_err(|e| e.value(py).to_string()); + Self { + handle, + path, + addr, + shape: meta.shape, + chunks: meta.chunks, + datatype: meta.datatype, + conv, + } } fn converter(&self) -> PyResult<&Converter> { @@ -103,10 +120,10 @@ impl PyDataset { }; let (reads, list_axis) = plan.reads(dims, chunk_len, elem_size); let read_shape = plan.read_shape(); - let file = &*self.file; + let handle = &*self.handle; let addr = self.addr; // Everything below touches only Rust data: release the GIL. - let read = || -> Result { + let read = |file: &File| -> Result { let ds = file.dataset_at(addr)?; let mut blocks = Vec::with_capacity(reads.len()); for read in reads { @@ -147,7 +164,7 @@ impl PyDataset { let sb = file.superblock(); let n = read_shape.iter().product(); resolve_vl( - file.as_bytes(), + file.storage(), &raw, n, sb.offset_size, @@ -155,12 +172,20 @@ impl PyDataset { unit, ) .map(Elements::Vl) - .map_err(ReadError::Other) + .map_err(ReadError::Vl) }; let data = py .detach(|| { - std::panic::catch_unwind(std::panic::AssertUnwindSafe(read)) - .unwrap_or_else(|p| Err(ReadError::Panic(crate::panic_text(&*p)))) + handle + .with_detached(|f| { + Ok( + std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| read(f))) + .unwrap_or_else(|p| { + Err(ReadError::Panic(crate::panic_text(&*p))) + }), + ) + }) + .unwrap_or_else(|e| Err(ReadError::Py(e))) }) .map_err(|e| e.into_py(&self.path))?; let joined = conv.to_array(py, data, &read_shape, false)?; @@ -183,6 +208,8 @@ impl PyDataset { /// An error from the read closure, turned into a Python error with the GIL. enum ReadError { Lib(clawhdf5_rs::Error), + Vl(VlError), + Py(PyErr), Other(String), Panic(String), } @@ -197,6 +224,8 @@ impl ReadError { fn into_py(self, path: &str) -> PyErr { match self { ReadError::Lib(e) => to_py_err(e), + ReadError::Vl(e) => e.into_py(&node::name(path)), + ReadError::Py(e) => e, ReadError::Other(msg) => PyValueError::new_err(format!("{}: {msg}", node::name(path))), ReadError::Panic(msg) => crate::InternalError::new_err(format!( "{}: clawhdf5 internal error (please report it): {msg}", @@ -252,22 +281,23 @@ impl PyDataset { /// The maximum shape (`None` per unlimited dimension), like h5py. #[getter] fn maxshape<'py>(&self, py: Python<'py>) -> PyResult> { - crate::no_panic(|| { - let Some(shape) = &self.shape else { - return Ok(py.None().into_bound(py)); - }; - let max = self - .file - .dataset_at(self.addr) - .and_then(|ds| ds.max_dimensions()) - .map_err(to_py_err)? - .unwrap_or_else(|| shape.clone()); - let items: Vec> = max - .into_iter() - .map(|d| (d != u64::MAX).then_some(d)) - .collect(); - Ok(PyTuple::new(py, items)?.into_any()) - }) + let Some(shape) = &self.shape else { + return Ok(py.None().into_bound(py)); + }; + let addr = self.addr; + let max = self + .handle + .with(py, |f| { + f.dataset_at(addr) + .and_then(|ds| ds.max_dimensions()) + .map_err(to_py_err) + })? + .unwrap_or_else(|| shape.clone()); + let items: Vec> = max + .into_iter() + .map(|d| (d != u64::MAX).then_some(d)) + .collect(); + Ok(PyTuple::new(py, items)?.into_any()) } /// The dataset's numpy dtype, as h5py reports it. @@ -295,8 +325,8 @@ impl PyDataset { /// The dataset's attributes (read-only, dict-like). #[getter] - fn attrs(&self) -> PyResult { - PyAttrs::read(Arc::clone(&self.file), self.addr, &self.path) + fn attrs(&self, py: Python<'_>) -> PyResult { + PyAttrs::read(py, Arc::clone(&self.handle), self.addr, &self.path) } /// Read with h5py indexing: integers, slices with positive steps, diff --git a/crates/clawhdf5-py/src/file.rs b/crates/clawhdf5-py/src/file.rs index 5f7f51b..48d94c0 100644 --- a/crates/clawhdf5-py/src/file.rs +++ b/crates/clawhdf5-py/src/file.rs @@ -1,13 +1,17 @@ //! PyFile — the main entry point for opening and creating HDF5 files. +use std::collections::HashMap; use std::path::PathBuf; use std::sync::{Arc, Mutex}; +use std::time::Duration; +use pyo3::exceptions::PyValueError; use pyo3::prelude::*; -use pyo3::types::PyList; +use pyo3::types::{PyDict, PyList}; use crate::attrs::PyAttrs; use crate::group::{PyGroup, ReadGroup, WriteGroupState, finalize_write_group}; +use crate::handle::Handle; use crate::{DatasetSpec, OwnedAttrValue, apply_dataset_spec, extract_numpy_data, to_py_err}; /// Internal state for write mode. @@ -23,8 +27,10 @@ struct WriteState { /// Mirrors the h5py.File interface: /// /// ```python -/// # Reading +/// # Reading, a local file or a URL (range requests, nothing downloaded +/// # up front) /// f = clawhdf5.File('data.h5', 'r') +/// f = clawhdf5.File('https://example.org/data.h5') /// ds = f['dataset'] /// f.close() /// @@ -44,27 +50,51 @@ enum FileInner { Write(WriteState), } +/// Whether `s` is a URL (`scheme://…`) rather than a path: the scheme is a +/// letter followed by letters, digits, `+`, `-` or `.` (RFC 3986). +fn is_url(s: &str) -> bool { + let Some((scheme, _)) = s.split_once("://") else { + return false; + }; + let mut chars = scheme.chars(); + chars.next().is_some_and(|c| c.is_ascii_alphabetic()) + && chars.all(|c| c.is_ascii_alphanumeric() || matches!(c, '+' | '-' | '.')) +} + +impl PyFile { + fn from_handle(handle: Arc, filename: String) -> Self { + let root = handle.root; + Self { + inner: Some(FileInner::Read(ReadGroup::new(handle, String::new(), root))), + filename, + } + } +} + #[pymethods] impl PyFile { /// Open or create an HDF5 file. /// /// Parameters: - /// path: file path + /// path: file path, or a URL (`http://`, `https://`, `s3://`, `gs://`, + /// `az://`; which schemes work depends on how the wheel was built) + /// to read the file remotely with default options (see `open_url`) /// mode: 'r' for read (default), 'w' for write #[new] #[pyo3(signature = (path, mode="r"))] fn new(py: Python<'_>, path: &str, mode: &str) -> PyResult { let filename = path.to_string(); - match mode { - "r" => { - let file = py.detach(|| { - crate::no_panic(|| clawhdf5_rs::File::open(path).map_err(to_py_err)) - })?; - Ok(Self { - inner: Some(FileInner::Read(root_group(Arc::new(file)))), - filename, - }) + if is_url(path) { + if mode != "r" { + return Err(PyValueError::new_err(format!( + "remote files are read-only: mode '{mode}' is not supported for a URL" + ))); } + let handle = Handle::open_url(py, path, &clawhdf5_remote::Options::default())?; + return Ok(Self::from_handle(handle, filename)); + } + match mode { + "r" => Ok(Self::from_handle(Handle::open_local(py, path)?, filename)), "w" => Ok(Self { filename, inner: Some(FileInner::Write(WriteState { @@ -74,12 +104,122 @@ impl PyFile { groups: Vec::new(), })), }), - other => Err(PyErr::new::(format!( + other => Err(PyValueError::new_err(format!( "unsupported mode '{other}'; expected 'r' or 'w'" ))), } } + /// Open a remote file for reading, with options. + /// + /// The file is read through a block cache with range requests: opening + /// costs one request (it also fetches the first block), and a read + /// fetches only the blocks it needs. The GIL is released while waiting + /// on the network. + /// + /// Parameters (all optional): + /// block_size: bytes per cached block (default 1 MiB) + /// cache_size: byte budget of the block cache (default 64 MiB) + /// headers: dict of extra HTTP headers (e.g. Authorization), sent only + /// to the URL's own origin + /// retries: retries of a request that failed transiently (default 3) + /// timeout: seconds to connect and receive response headers (default 30) + /// allow_full_download: when the server ignores Range requests, + /// download the whole file once instead of failing (default False) + /// max_full_download: largest file such a download may fetch + /// (default 1 GiB) + /// require_validator: refuse a server that sends neither ETag nor + /// Last-Modified (default False) + /// max_redirects: redirects followed per request (default 5) + /// max_parallel: requests of one read in flight at once (default 8) + #[staticmethod] + #[allow(clippy::too_many_arguments)] + #[pyo3(signature = (url, *, block_size=None, cache_size=None, headers=None, retries=None, + timeout=None, allow_full_download=None, max_full_download=None, + require_validator=None, max_redirects=None, max_parallel=None))] + fn open_url( + py: Python<'_>, + url: &str, + block_size: Option, + cache_size: Option, + headers: Option>, + retries: Option, + timeout: Option, + allow_full_download: Option, + max_full_download: Option, + require_validator: Option, + max_redirects: Option, + max_parallel: Option, + ) -> PyResult { + let mut options = clawhdf5_remote::Options::default(); + if let Some(b) = block_size { + if b == 0 { + return Err(PyValueError::new_err("block_size must be positive")); + } + options.cache.block_size = b; + options.cache.coalesce_gap = b; + // The opening request fetches the first block, not 1 MiB. + options.http.first_request = b; + } + if let Some(c) = cache_size { + options.cache.capacity = c; + } + let http = &mut options.http; + if let Some(h) = headers { + http.headers = h.into_iter().collect(); + } + if let Some(r) = retries { + http.retries = r; + } + if let Some(t) = timeout { + if !(t.is_finite() && t > 0.0) { + return Err(PyValueError::new_err("timeout must be a positive number")); + } + http.timeout = Duration::from_secs_f64(t); + } + if let Some(a) = allow_full_download { + http.allow_full_download = a; + } + if let Some(m) = max_full_download { + http.max_full_download = m; + } + if let Some(v) = require_validator { + http.require_validator = v; + } + if let Some(r) = max_redirects { + http.max_redirects = r; + } + if let Some(p) = max_parallel { + if p == 0 { + return Err(PyValueError::new_err("max_parallel must be positive")); + } + http.max_parallel = p; + } + let handle = Handle::open_url(py, url, &options)?; + Ok(Self::from_handle(handle, url.to_string())) + } + + /// For a remote file, what its block cache has done so far (reads, + /// hits, misses, requests, bytes fetched, ...); `None` for a local file. + #[getter] + fn remote_stats<'py>(&self, py: Python<'py>) -> PyResult>> { + let Some(storage) = self.read_file()?.handle.remote_storage() else { + return Ok(None); + }; + let s = storage.stats(); + let d = PyDict::new(py); + d.set_item("reads", s.reads)?; + d.set_item("hits", s.hits)?; + d.set_item("misses", s.misses)?; + d.set_item("waits", s.waits)?; + d.set_item("requests", s.requests)?; + d.set_item("fetch_calls", s.fetch_calls)?; + d.set_item("bytes_fetched", s.bytes_fetched)?; + d.set_item("evictions", s.evictions)?; + d.set_item("cached_bytes", s.cached_bytes)?; + Ok(Some(d)) + } + /// Close the file. In write mode, this finalizes and writes the file. fn close(&mut self) -> PyResult<()> { let inner = self.inner.take().ok_or_else(|| { @@ -121,7 +261,7 @@ impl PyFile { /// List the names of all children in the root group. fn keys(&self, py: Python<'_>) -> PyResult> { - let names = self.read_file()?.member_names()?; + let names = self.read_file()?.member_names(py)?; Ok(PyList::new(py, names)?.into_any().unbind()) } @@ -139,8 +279,8 @@ impl PyFile { self.keys(py)?.call_method0(py, "__iter__") } - fn __len__(&self) -> PyResult { - Ok(self.read_file()?.member_names()?.len()) + fn __len__(&self, py: Python<'_>) -> PyResult { + Ok(self.read_file()?.member_names(py)?.len()) } /// The root group's name, `/`. @@ -149,7 +289,7 @@ impl PyFile { "/" } - /// The path the file was opened with. + /// The path (or URL) the file was opened with. #[getter] fn filename(&self) -> &str { &self.filename @@ -204,9 +344,9 @@ impl PyFile { /// Attribute access. In read mode, returns attributes of the root group. /// In write mode, returns a writable attrs handle. #[getter] - fn attrs(&self) -> PyResult { + fn attrs(&self, py: Python<'_>) -> PyResult { match self.inner.as_ref() { - Some(FileInner::Read(root)) => root.attrs(), + Some(FileInner::Read(root)) => root.attrs(py), Some(FileInner::Write(state)) => Ok(PyAttrs::from_write(Arc::clone(&state.root_attrs))), None => Err(PyErr::new::( "file is closed", @@ -216,9 +356,10 @@ impl PyFile { fn __repr__(&self) -> String { match &self.inner { - Some(FileInner::Read(root)) => { - format!("", root.file.as_bytes().len()) - } + Some(FileInner::Read(root)) => match root.handle.redacted_url() { + Some(url) => format!(""), + None => format!("", self.filename), + }, Some(FileInner::Write(s)) => { format!("", s.path.display()) } @@ -226,8 +367,8 @@ impl PyFile { } } - fn __contains__(&self, key: &str) -> PyResult { - Ok(self.read_file()?.contains(key)) + fn __contains__(&self, py: Python<'_>, key: &str) -> PyResult { + self.read_file()?.contains(py, key) } } @@ -271,11 +412,6 @@ fn parse_compression( } } -fn root_group(file: Arc) -> ReadGroup { - let root = file.superblock().root_group_address; - ReadGroup::new(file, String::new(), root) -} - /// Build and write the HDF5 file from accumulated write state. fn finalize_write(state: WriteState) -> PyResult<()> { crate::no_panic(|| { @@ -309,6 +445,18 @@ fn finalize_write(state: WriteState) -> PyResult<()> { mod tests { use super::*; + #[test] + fn urls_and_paths() { + assert!(is_url("http://h/f.h5")); + assert!(is_url("s3://bucket/key.h5")); + assert!(is_url("git+https://x")); + assert!(!is_url("data.h5")); + assert!(!is_url("/tmp/a://b.h5")); + assert!(!is_url("dir/x://y")); + assert!(!is_url("1http://x")); + assert!(!is_url("://x")); + } + #[test] fn parse_gzip_compression() { assert_eq!(parse_compression(Some("gzip"), Some(6)).unwrap(), Some(6)); diff --git a/crates/clawhdf5-py/src/group.rs b/crates/clawhdf5-py/src/group.rs index 7585e57..36506b9 100644 --- a/crates/clawhdf5-py/src/group.rs +++ b/crates/clawhdf5-py/src/group.rs @@ -3,11 +3,12 @@ use std::collections::HashMap; use std::sync::{Arc, Mutex, OnceLock}; -use pyo3::exceptions::{PyIOError, PyKeyError, PyValueError}; +use pyo3::exceptions::{PyIOError, PyKeyError, PyOSError, PyValueError}; use pyo3::prelude::*; use pyo3::types::PyList; use crate::attrs::PyAttrs; +use crate::handle::Handle; use crate::{DatasetSpec, OwnedAttrValue, apply_dataset_spec, extract_numpy_data, node}; /// Shared state for a group being written. @@ -34,9 +35,9 @@ enum GroupInner { } impl PyGroup { - pub(crate) fn from_read(file: Arc, path: String, addr: u64) -> Self { + pub(crate) fn from_read(handle: Arc, path: String, addr: u64) -> Self { Self { - inner: GroupInner::Read(ReadGroup::new(file, path, addr)), + inner: GroupInner::Read(ReadGroup::new(handle, path, addr)), } } @@ -60,9 +61,10 @@ impl PyGroup { /// h5py). It keeps its own address and, once listed, its links, so looking /// up a child neither resolves the path from the root nor scans the group's /// links again: visiting every member of a large group is linear, not -/// quadratic. +/// quadratic. (Edits never add or remove links, so these stay valid in a +/// file open for editing.) pub(crate) struct ReadGroup { - pub file: Arc, + pub handle: Arc, pub path: String, pub addr: u64, /// Link name -> object address (soft links resolved), filled on first use. @@ -72,9 +74,9 @@ pub(crate) struct ReadGroup { } impl ReadGroup { - pub(crate) fn new(file: Arc, path: String, addr: u64) -> Self { + pub(crate) fn new(handle: Arc, path: String, addr: u64) -> Self { Self { - file, + handle, path, addr, links: OnceLock::new(), @@ -82,17 +84,14 @@ impl ReadGroup { } } - fn links(&self) -> PyResult<&HashMap> { + fn links(&self, py: Python<'_>) -> PyResult<&HashMap> { if let Some(links) = self.links.get() { return Ok(links); } - let entries = crate::no_panic(|| { - clawhdf5_format::group_v2::resolve_group_children( - self.file.as_bytes(), - self.file.superblock(), - self.addr, - ) - .map_err(|e| PyValueError::new_err(format!("{}: {e}", node::name(&self.path)))) + let (addr, path) = (self.addr, &self.path); + let entries = self.handle.with(py, |f| { + clawhdf5_format::group_v2::resolve_group_children_in(f.storage(), f.superblock(), addr) + .map_err(|e| node::format_err(path, e, PyValueError::new_err)) })?; let map = entries .into_iter() @@ -102,7 +101,7 @@ impl ReadGroup { } /// The path and address of `key` (a name, a relative or an absolute path). - fn locate(&self, key: &str) -> PyResult<(String, u64)> { + fn locate(&self, py: Python<'_>, key: &str) -> PyResult<(String, u64)> { let path = node::join(&self.path, key); let rel = if self.path.is_empty() { Some(path.as_str()) @@ -112,24 +111,24 @@ impl ReadGroup { path.strip_prefix(self.path.as_str()) .and_then(|r| r.strip_prefix('/')) }; - let addr = match rel { - // A direct child: the link table, when it has the name. - Some(name) if !name.is_empty() && !name.contains('/') => { - match self.links()?.get(name) { - Some(&a) => a, - None => node::resolve_from(&self.file, self.addr, name, &path)?, - } - } - Some(rel) => node::resolve_from(&self.file, self.addr, rel, &path)?, - None => node::address(&self.file, &path)?, - }; - Ok((path, addr)) + // A direct child: the link table, when it has the name. + if let Some(name) = rel.filter(|n| !n.is_empty() && !n.contains('/')) + && let Some(&a) = self.links(py)?.get(name) + { + return Ok((path, a)); + } + let addr = self.addr; + let found = self.handle.with(py, |f| match rel { + Some(rel) => node::resolve_from(f, addr, rel, &path), + None => node::address(f, &path), + })?; + Ok((path, found)) } /// `group[key]`. pub(crate) fn get_item(&self, py: Python<'_>, key: &str) -> PyResult> { - let (path, addr) = self.locate(key)?; - node::open(py, &self.file, path, addr) + let (path, addr) = self.locate(py, key)?; + node::open(py, &self.handle, path, addr) } /// `group.get(key, default)`. @@ -148,48 +147,57 @@ impl ReadGroup { } /// Names of the group's datasets and subgroups, sorted (h5py's order). - pub(crate) fn member_names(&self) -> PyResult<&[String]> { + pub(crate) fn member_names(&self, py: Python<'_>) -> PyResult<&[String]> { if let Some(m) = self.members.get() { return Ok(m); } - let mut names = Vec::new(); - for (name, &addr) in self.links()? { - let hdr = node::header_at(&self.file, addr, &node::join(&self.path, name))?; - if matches!( - node::kind(&hdr), - Some(node::Kind::Dataset | node::Kind::Group) - ) { - names.push(name.clone()); + let links = self.links(py)?; + let path = &self.path; + let mut names = self.handle.with(py, |f| { + let mut names = Vec::new(); + for (name, &addr) in links { + if matches!( + node::kind_at(f, addr, &node::join(path, name))?, + Some(node::Kind::Dataset | node::Kind::Group) + ) { + names.push(name.clone()); + } } - } + Ok(names) + })?; names.sort_by(|a, b| a.as_bytes().cmp(b.as_bytes())); Ok(self.members.get_or_init(|| names)) } - pub(crate) fn contains(&self, key: &str) -> bool { - self.locate(key) - .and_then(|(path, addr)| node::header_at(&self.file, addr, &path)) - .ok() - .and_then(|h| node::kind(&h)) - .is_some_and(|k| k != node::Kind::Datatype) + /// `key in group`: whether `key` names a dataset or group. A failed + /// read of the file (a network error) is raised, not `False`. + pub(crate) fn contains(&self, py: Python<'_>, key: &str) -> PyResult { + let found = self + .locate(py, key) + .and_then(|(path, addr)| self.handle.with(py, |f| node::kind_at(f, addr, &path))); + match found { + Ok(kind) => Ok(kind.is_some_and(|k| k != node::Kind::Datatype)), + Err(e) if e.is_instance_of::(py) => Err(e), + Err(_) => Ok(false), + } } pub(crate) fn values(&self, py: Python<'_>) -> PyResult>> { - self.member_names()? + self.member_names(py)? .iter() .map(|n| self.get_item(py, n)) .collect() } pub(crate) fn items(&self, py: Python<'_>) -> PyResult)>> { - self.member_names()? + self.member_names(py)? .iter() .map(|n| Ok((n.clone(), self.get_item(py, n)?))) .collect() } - pub(crate) fn attrs(&self) -> PyResult { - PyAttrs::read(Arc::clone(&self.file), self.addr, &self.path) + pub(crate) fn attrs(&self, py: Python<'_>) -> PyResult { + PyAttrs::read(py, Arc::clone(&self.handle), self.addr, &self.path) } } @@ -210,7 +218,7 @@ impl PyGroup { fn keys(&self, py: Python<'_>) -> PyResult> { match &self.inner { GroupInner::Read(g) => { - let list = PyList::new(py, g.member_names()?)?; + let list = PyList::new(py, g.member_names(py)?)?; Ok(list.into_any().unbind()) } GroupInner::Write(state) => { @@ -236,9 +244,9 @@ impl PyGroup { self.keys(py)?.call_method0(py, "__iter__") } - fn __len__(&self) -> PyResult { + fn __len__(&self, py: Python<'_>) -> PyResult { match &self.inner { - GroupInner::Read(g) => Ok(g.member_names()?.len()), + GroupInner::Read(g) => Ok(g.member_names(py)?.len()), GroupInner::Write(state) => Ok(state.lock().unwrap().datasets.len()), } } @@ -301,9 +309,9 @@ impl PyGroup { /// Attribute access. #[getter] - fn attrs(&self) -> PyResult { + fn attrs(&self, py: Python<'_>) -> PyResult { match &self.inner { - GroupInner::Read(g) => g.attrs(), + GroupInner::Read(g) => g.attrs(py), GroupInner::Write(state) => { let store = Arc::clone(&state.lock().unwrap().attrs); Ok(PyAttrs::from_write(store)) @@ -311,10 +319,10 @@ impl PyGroup { } } - fn __repr__(&self) -> String { + fn __repr__(&self, py: Python<'_>) -> String { match &self.inner { GroupInner::Read(g) => { - let n = g.member_names().map_or(0, |m| m.len()); + let n = g.member_names(py).map_or(0, |m| m.len()); format!("", node::name(&g.path)) } GroupInner::Write(state) => { @@ -324,9 +332,9 @@ impl PyGroup { } } - fn __contains__(&self, key: &str) -> PyResult { + fn __contains__(&self, py: Python<'_>, key: &str) -> PyResult { match &self.inner { - GroupInner::Read(g) => Ok(g.contains(key)), + GroupInner::Read(g) => g.contains(py, key), GroupInner::Write(state) => { let guard = state.lock().unwrap(); Ok(guard.datasets.iter().any(|d| d.name == key)) @@ -357,31 +365,6 @@ pub(crate) fn finalize_write_group( mod tests { use super::*; - #[test] - fn member_names_are_sorted() { - let mut b = clawhdf5_rs::FileBuilder::new(); - b.create_dataset("zeta").with_f64_data(&[1.0]); - b.create_dataset("alpha").with_f64_data(&[1.0]); - let mut g = b.create_group("mid"); - g.create_dataset("x").with_f64_data(&[1.0]); - let finished = g.finish(); - b.add_group(finished); - let bytes = b.finish().unwrap(); - let file = Arc::new(clawhdf5_rs::File::from_bytes(bytes).unwrap()); - let root = file.superblock().root_group_address; - let top = ReadGroup::new(Arc::clone(&file), String::new(), root); - assert_eq!(top.member_names().unwrap(), ["alpha", "mid", "zeta"]); - let (path, addr) = top.locate("mid").unwrap(); - assert_eq!(path, "mid"); - let mid = ReadGroup::new(Arc::clone(&file), path, addr); - assert_eq!(mid.member_names().unwrap(), ["x"]); - assert!(top.contains("mid/x")); - assert!(mid.contains("/alpha")); - assert!(mid.contains("x") && mid.contains("./x")); - assert!(!top.contains("nope")); - assert!(!mid.contains("alpha")); - } - #[test] fn finalize_group() { let state = WriteGroupState { diff --git a/crates/clawhdf5-py/src/handle.rs b/crates/clawhdf5-py/src/handle.rs new file mode 100644 index 0000000..8649f74 --- /dev/null +++ b/crates/clawhdf5-py/src/handle.rs @@ -0,0 +1,130 @@ +//! The open file every object of a `File` shares. +//! +//! Every read goes through [`Handle::with`], which releases the GIL and +//! parses through `File::storage()`, so the same code serves a local file +//! (memory-mapped) and a remote one (`clawhdf5-remote`: range requests +//! through a block cache, so a network read never holds the GIL). +//! +//! Lock discipline (no deadlock with the GIL): the file lock is only taken +//! with the GIL released, and code that holds it never touches Python. + +use std::path::PathBuf; +use std::sync::{Arc, PoisonError, RwLock}; + +use clawhdf5_rs::File; +use pyo3::exceptions::PyOSError; +use pyo3::prelude::*; + +use crate::to_py_err; + +/// Where the file's bytes come from. +pub(crate) enum Source { + /// A local path (memory-mapped). + Local(#[allow(dead_code)] PathBuf), + /// A URL, read through `clawhdf5-remote`'s block cache. + Remote { + url: String, + storage: Arc, + }, +} + +pub(crate) struct Handle { + file: RwLock>, + source: Source, + pub offset_size: u8, + pub length_size: u8, + pub root: u64, +} + +fn closed_after_failed_reopen() -> PyErr { + PyOSError::new_err("the file could not be reopened after an edit; open it again") +} + +impl Handle { + fn new(file: File, source: Source) -> Arc { + let sb = file.superblock(); + let (offset_size, length_size, root) = + (sb.offset_size, sb.length_size, sb.root_group_address); + Arc::new(Self { + file: RwLock::new(Some(file)), + source, + offset_size, + length_size, + root, + }) + } + + /// A local file, read-only. + pub(crate) fn open_local(py: Python<'_>, path: &str) -> PyResult> { + let file = py.detach(|| crate::no_panic(|| File::open(path).map_err(to_py_err)))?; + Ok(Self::new(file, Source::Local(PathBuf::from(path)))) + } + + /// A remote file (`http(s)://`, `s3://`, ...). + pub(crate) fn open_url( + py: Python<'_>, + url: &str, + options: &clawhdf5_remote::Options, + ) -> PyResult> { + let (file, storage) = py.detach(|| { + crate::no_panic(|| { + let storage = clawhdf5_remote::storage_for_url(url, options).map_err(remote_err)?; + let file = File::open_storage(storage.clone()).map_err(to_py_err)?; + Ok((file, storage)) + }) + })?; + Ok(Self::new( + file, + Source::Remote { + url: url.to_string(), + storage, + }, + )) + } + + /// Run `f` on the file with the GIL released (a remote read may wait + /// on the network; other Python threads run meanwhile). `f` must not + /// touch Python. + pub(crate) fn with( + &self, + py: Python<'_>, + f: impl FnOnce(&File) -> PyResult + Send, + ) -> PyResult { + py.detach(|| self.with_detached(f)) + } + + /// [`with`](Self::with) for code that already runs without the GIL. + pub(crate) fn with_detached(&self, f: impl FnOnce(&File) -> PyResult) -> PyResult { + crate::no_panic(|| { + let guard = self.file.read().unwrap_or_else(PoisonError::into_inner); + let file = guard.as_ref().ok_or_else(closed_after_failed_reopen)?; + f(file) + }) + } + + /// The remote file's block cache. + pub(crate) fn remote_storage(&self) -> Option<&clawhdf5_remote::RemoteStorage> { + match &self.source { + Source::Remote { storage, .. } => Some(storage), + Source::Local(_) => None, + } + } + + /// The URL of a remote file, credentials and query values redacted. + pub(crate) fn redacted_url(&self) -> Option { + match &self.source { + Source::Remote { url, .. } => Some(clawhdf5_remote::redact_url(url)), + Source::Local(_) => None, + } + } +} + +/// A `clawhdf5_remote::Error` as a Python exception: the network side +/// (unreachable, a status, no range support, a changed file) is `OSError`, +/// a file that is not HDF5 is what `to_py_err` makes of it. +pub(crate) fn remote_err(e: clawhdf5_remote::Error) -> PyErr { + match e { + clawhdf5_remote::Error::Hdf5(e) => to_py_err(e), + other => PyOSError::new_err(other.to_string()), + } +} diff --git a/crates/clawhdf5-py/src/lib.rs b/crates/clawhdf5-py/src/lib.rs index 4055b82..db89d8d 100644 --- a/crates/clawhdf5-py/src/lib.rs +++ b/crates/clawhdf5-py/src/lib.rs @@ -14,6 +14,7 @@ mod convert; mod dataset; mod file; mod group; +mod handle; mod node; mod select; @@ -63,7 +64,7 @@ fn _panic_for_test() -> PyResult<()> { /// Convert a `clawhdf5_rs::Error` into a `PyErr`. /// /// Maps different error variants to more specific Python exception types: -/// - I/O errors -> `PyIOError` +/// - I/O errors, and failed reads of a remote file -> `PyIOError`/`PyOSError` /// - Format/parsing errors -> `PyValueError` /// - Missing dataset/path errors -> `PyKeyError` /// - Invalid arguments -> `PyValueError` @@ -73,6 +74,10 @@ pub(crate) fn to_py_err(e: clawhdf5_rs::Error) -> PyErr { use clawhdf5_rs::Error; match &e { Error::Io(_) => PyErr::new::(e.to_string()), + // A failed read of the storage: a network error on a remote file. + Error::Format(clawhdf5_format::error::FormatError::Storage(_)) => { + PyErr::new::(e.to_string()) + } Error::Format(_) => PyErr::new::(e.to_string()), Error::NotADataset(_) | Error::MissingMessage(_) => { PyErr::new::(e.to_string()) diff --git a/crates/clawhdf5-py/src/node.rs b/crates/clawhdf5-py/src/node.rs index fa9ede7..35e5dec 100644 --- a/crates/clawhdf5-py/src/node.rs +++ b/crates/clawhdf5-py/src/node.rs @@ -1,16 +1,24 @@ //! Resolving paths to objects in a file opened for reading. +//! +//! Everything here parses through `File::storage()` (the `clawhdf5_format` +//! `*_in` functions), never `File::as_bytes()`, so it works the same on a +//! memory-mapped local file and on a remote one; and it runs inside +//! `Handle::with`, without the GIL. use std::sync::Arc; use clawhdf5_format::attribute::AttributeMessage; use clawhdf5_format::dataspace::{Dataspace, DataspaceType}; +use clawhdf5_format::error::FormatError; use clawhdf5_format::message_type::MessageType; use clawhdf5_format::object_header::ObjectHeader; -use pyo3::exceptions::{PyKeyError, PyTypeError, PyValueError}; +use clawhdf5_rs::File; +use pyo3::exceptions::{PyKeyError, PyOSError, PyTypeError, PyValueError}; use pyo3::prelude::*; -use crate::dataset::PyDataset; +use crate::dataset::{DatasetMeta, PyDataset}; use crate::group::PyGroup; +use crate::handle::Handle; /// Join `key` onto the group path `base` the way h5py does: an absolute key /// starts from the root, a relative one from `base`. Paths are kept without @@ -33,42 +41,46 @@ pub(crate) fn name(path: &str) -> String { format!("/{path}") } +/// A format error met at `path`: a failed read of the storage (a network +/// error on a remote file) is an `OSError`, anything else `other(message)`. +pub(crate) fn format_err(path: &str, e: FormatError, other: fn(String) -> PyErr) -> PyErr { + let msg = format!("{}: {e}", name(path)); + match e { + FormatError::Storage(_) => PyOSError::new_err(msg), + _ => other(msg), + } +} + +fn value_err(msg: String) -> PyErr { + PyValueError::new_err(msg) +} + /// The address of the object at `path`, resolved from the root group. -pub(crate) fn address(file: &clawhdf5_rs::File, path: &str) -> PyResult { +pub(crate) fn address(file: &File, path: &str) -> PyResult { resolve_from(file, file.superblock().root_group_address, path, path) } /// The address of `rel` resolved from the group at `group` (`full` is the /// resulting path, for the error message). -pub(crate) fn resolve_from( - file: &clawhdf5_rs::File, - group: u64, - rel: &str, - full: &str, -) -> PyResult { +pub(crate) fn resolve_from(file: &File, group: u64, rel: &str, full: &str) -> PyResult { if rel.is_empty() { return Ok(group); } - crate::no_panic(|| { - clawhdf5_format::group_v2::resolve_path_from(file.as_bytes(), file.superblock(), group, rel) - .map_err(|e| { - PyKeyError::new_err(format!( - "Unable to open object (object '{}' doesn't exist): {e}", - name(full) - )) - }) - }) + clawhdf5_format::group_v2::resolve_path_from_in(file.storage(), file.superblock(), group, rel) + .map_err(|e| match e { + FormatError::Storage(_) => format_err(full, e, value_err), + e => PyKeyError::new_err(format!( + "Unable to open object (object '{}' doesn't exist): {e}", + name(full) + )), + }) } /// The object header at `addr` (the object at `path`). -pub(crate) fn header_at(file: &clawhdf5_rs::File, addr: u64, path: &str) -> PyResult { - crate::no_panic(|| { - let sb = file.superblock(); - let at = usize::try_from(addr) - .map_err(|_| PyValueError::new_err(format!("{}: address out of range", name(path))))?; - ObjectHeader::parse(file.as_bytes(), at, sb.offset_size, sb.length_size) - .map_err(|e| PyValueError::new_err(format!("{}: {e}", name(path)))) - }) +pub(crate) fn header_at(file: &File, addr: u64, path: &str) -> PyResult { + let sb = file.superblock(); + ObjectHeader::parse_in(file.storage(), addr, sb.offset_size, sb.length_size) + .map_err(|e| format_err(path, e, value_err)) } /// What an object header describes. @@ -96,29 +108,50 @@ pub(crate) fn kind(hdr: &ObjectHeader) -> Option { } } +/// The kind of the object at `addr`, from its header. +pub(crate) fn kind_at(file: &File, addr: u64, path: &str) -> PyResult> { + Ok(kind(&header_at(file, addr, path)?)) +} + +/// What opening an object found, read without the GIL. +enum Found { + Dataset(DatasetMeta), + Group, + Datatype, + Other, +} + /// Open the object at `addr` (whose path is `path`) as a `Dataset` or /// `Group`. Both keep the address, so later reads resolve nothing. pub(crate) fn open( py: Python<'_>, - file: &Arc, + handle: &Arc, path: String, addr: u64, ) -> PyResult> { - let hdr = header_at(file, addr, &path)?; - match kind(&hdr) { - Some(Kind::Dataset) => Ok(PyDataset::open(py, Arc::clone(file), path, addr, &hdr)? + let found = handle.with(py, |f| { + let hdr = header_at(f, addr, &path)?; + Ok(match kind(&hdr) { + Some(Kind::Dataset) => Found::Dataset(DatasetMeta::load(f, addr, &hdr, &path)?), + Some(Kind::Group) => Found::Group, + Some(Kind::Datatype) => Found::Datatype, + None => Found::Other, + }) + })?; + match found { + Found::Dataset(meta) => Ok(PyDataset::new(py, Arc::clone(handle), path, addr, meta) .into_pyobject(py)? .into_any() .unbind()), - Some(Kind::Group) => Ok(PyGroup::from_read(Arc::clone(file), path, addr) + Found::Group => Ok(PyGroup::from_read(Arc::clone(handle), path, addr) .into_pyobject(py)? .into_any() .unbind()), - Some(Kind::Datatype) => Err(PyTypeError::new_err(format!( + Found::Datatype => Err(PyTypeError::new_err(format!( "{}: committed (named) datatypes are not supported by clawhdf5", name(&path) ))), - None => Err(PyValueError::new_err(format!( + Found::Other => Err(PyValueError::new_err(format!( "{}: not a dataset, group or datatype", name(&path) ))), @@ -126,32 +159,26 @@ pub(crate) fn open( } /// The dataspace message of an object header. -pub(crate) fn dataspace(file: &clawhdf5_rs::File, hdr: &ObjectHeader) -> PyResult { - crate::no_panic(|| { - let sb = file.superblock(); - let msg = hdr - .messages - .iter() - .find(|m| m.msg_type == MessageType::Dataspace) - .ok_or_else(|| PyValueError::new_err("object has no dataspace message"))?; - let data = clawhdf5_format::shared_message::message_data( - file.as_bytes(), - msg, - sb.offset_size, - sb.length_size, - ) - .map_err(|e| PyValueError::new_err(e.to_string()))?; - Dataspace::parse(&data, sb.length_size).map_err(|e| PyValueError::new_err(e.to_string())) - }) +pub(crate) fn dataspace(file: &File, hdr: &ObjectHeader, path: &str) -> PyResult { + let sb = file.superblock(); + let msg = hdr + .messages + .iter() + .find(|m| m.msg_type == MessageType::Dataspace) + .ok_or_else(|| PyValueError::new_err("object has no dataspace message"))?; + let data = clawhdf5_format::shared_message::message_data_in( + file.storage(), + msg, + sb.offset_size, + sb.length_size, + ) + .map_err(|e| format_err(path, e, value_err))?; + Dataspace::parse(&data, sb.length_size).map_err(|e| format_err(path, e, value_err)) } /// The chunk shape of a chunked dataset (one entry per dataset dimension), /// or `None` for other layouts or a layout message that does not parse. -pub(crate) fn chunk_shape( - file: &clawhdf5_rs::File, - hdr: &ObjectHeader, - rank: usize, -) -> Option> { +pub(crate) fn chunk_shape(file: &File, hdr: &ObjectHeader, rank: usize) -> Option> { let sb = file.superblock(); let msg = hdr .messages @@ -179,24 +206,18 @@ pub(crate) fn is_null(space: &Dataspace) -> bool { /// The attributes of the object at `addr` (whose path is `path`), sorted by /// name (h5py's order). Attributes whose messages cannot be parsed are left /// out, as the facade's `attrs()` does. -pub(crate) fn attributes( - file: &clawhdf5_rs::File, - addr: u64, - path: &str, -) -> PyResult> { +pub(crate) fn attributes(file: &File, addr: u64, path: &str) -> PyResult> { let hdr = header_at(file, addr, path)?; - crate::no_panic(|| { - let sb = file.superblock(); - let (mut attrs, _errors) = clawhdf5_format::attribute::extract_attributes_tolerant( - file.as_bytes(), - &hdr, - sb.offset_size, - sb.length_size, - ) - .map_err(|e| PyValueError::new_err(format!("{}: {e}", name(path))))?; - attrs.sort_by(|a, b| a.name.as_bytes().cmp(b.name.as_bytes())); - Ok(attrs) - }) + let sb = file.superblock(); + let (mut attrs, _errors) = clawhdf5_format::attribute::extract_attributes_tolerant_in( + file.storage(), + &hdr, + sb.offset_size, + sb.length_size, + ) + .map_err(|e| format_err(path, e, value_err))?; + attrs.sort_by(|a, b| a.name.as_bytes().cmp(b.name.as_bytes())); + Ok(attrs) } #[cfg(test)] diff --git a/crates/clawhdf5-py/tests/conftest.py b/crates/clawhdf5-py/tests/conftest.py index 967f1e0..88113e8 100644 --- a/crates/clawhdf5-py/tests/conftest.py +++ b/crates/clawhdf5-py/tests/conftest.py @@ -1,6 +1,10 @@ """Shared fixtures for the clawhdf5 Python binding tests.""" import os +import re +import threading +import time +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer import pytest @@ -16,3 +20,134 @@ def h5py(): pytest.fail("h5py is required (CLAWHDF5_REQUIRE_INTEROP=1) but not importable") pytest.skip("h5py not installed") return mod + + +# --------------------------------------------------------------------------- +# An HTTP server for remote reads +# --------------------------------------------------------------------------- + +_RANGE = re.compile(r"^bytes=(\d*)-(\d*)$") + + +class RangeServer: + """A static file server on 127.0.0.1, in a thread of this process, that + answers `Range: bytes=a-b` with 206 and `Content-Range` (the way S3 and + common web servers do), sends an ETag and honours `If-Match`. + + - `ranges=False`: ignores `Range` and answers 200 with the whole file, + like a server without range support. + - `down` (set by `close()`): hang up on every request. + - `delay`: seconds to wait before answering each request after the + first `delay_after` ones (a slow network). + - `log`: every request as `(method, path, range header)`. + """ + + def __init__(self, root, ranges=True): + self.root = str(root) + self.ranges = ranges + self.delay = 0.0 + self.delay_after = 0 + self.down = False + self.log = [] + self._lock = threading.Lock() + server = self + + class Handler(BaseHTTPRequestHandler): + protocol_version = "HTTP/1.1" + + def log_message(self, *args): # quiet + pass + + def do_HEAD(self): + self._serve(body=False) + + def do_GET(self): + self._serve(body=True) + + def _serve(self, body): + with server._lock: + server.log.append((self.command, self.path, self.headers.get("Range"))) + n = len(server.log) + if server.delay and n > server.delay_after: + time.sleep(server.delay) + if server.down: + # Hang up without an answer (open keep-alive + # connections outlive shutdown(), so close() sets this). + self.close_connection = True + return + path = os.path.join(server.root, self.path.lstrip("/").split("?")[0]) + if not os.path.isfile(path): + self.send_response(404) + self.send_header("Content-Length", "0") + self.end_headers() + return + with open(path, "rb") as fh: + data = fh.read() + st = os.stat(path) + etag = f'"{st.st_mtime_ns:x}-{st.st_size:x}"' + want = self.headers.get("If-Match") + if want is not None and want != etag and want != "*": + self.send_response(412) + self.send_header("Content-Length", "0") + self.end_headers() + return + rng = self.headers.get("Range") if server.ranges else None + m = _RANGE.match(rng.strip()) if rng else None + if m and (m.group(1) or m.group(2)): + size = len(data) + if m.group(1): + start = int(m.group(1)) + end = int(m.group(2)) if m.group(2) else size - 1 + else: + start = max(0, size - int(m.group(2))) + end = size - 1 + if start >= size: + self.send_response(416) + self.send_header("Content-Range", f"bytes */{size}") + self.send_header("Content-Length", "0") + self.end_headers() + return + end = min(end, size - 1) + part = data[start : end + 1] + self.send_response(206) + self.send_header("Content-Range", f"bytes {start}-{end}/{size}") + else: + part = data + self.send_response(200) + if server.ranges: + self.send_header("Accept-Ranges", "bytes") + self.send_header("ETag", etag) + self.send_header("Content-Length", str(len(part))) + self.send_header("Content-Type", "application/x-hdf5") + self.end_headers() + if body: + try: + self.wfile.write(part) + except (BrokenPipeError, ConnectionResetError): + pass + + self.httpd = ThreadingHTTPServer(("127.0.0.1", 0), Handler) + self.httpd.daemon_threads = True + self.port = self.httpd.server_address[1] + self.thread = threading.Thread(target=self.httpd.serve_forever, daemon=True) + self.thread.start() + + def url(self, name): + return f"http://127.0.0.1:{self.port}/{name}" + + def requests(self): + with self._lock: + return len(self.log) + + def close(self): + self.down = True + self.httpd.shutdown() + self.httpd.server_close() + + +@pytest.fixture +def range_server(tmp_path): + """A range-capable server over `tmp_path`.""" + server = RangeServer(tmp_path) + yield server + server.close() diff --git a/crates/clawhdf5-py/tests/test_read_vs_h5py.py b/crates/clawhdf5-py/tests/test_read_vs_h5py.py index 6a3b6cb..f3132a7 100644 --- a/crates/clawhdf5-py/tests/test_read_vs_h5py.py +++ b/crates/clawhdf5-py/tests/test_read_vs_h5py.py @@ -178,15 +178,32 @@ def _write_fixture(h5py, path): g.attrs["depth"] = np.int8(3) -@pytest.fixture(scope="module") -def pair(h5py, tmp_path_factory): - path = str(tmp_path_factory.mktemp("h5") / "fixture.h5") +@pytest.fixture(scope="module", params=["local", "http", "http-1k-blocks"]) +def pair(request, h5py, tmp_path_factory): + """The fixture file through h5py and through clawhdf5: opened locally, + and over HTTP range requests (a local server in this process) with the + default 1 MiB blocks and with 1 KiB blocks, so every structure is read + through many small ranges.""" + from conftest import RangeServer + + root = tmp_path_factory.mktemp("h5") + path = str(root / "fixture.h5") _write_fixture(h5py, path) theirs = h5py.File(path, "r") - ours = clawhdf5.File(path, "r") + server = None + if request.param == "local": + ours = clawhdf5.File(path, "r") + else: + server = RangeServer(root) + if request.param == "http": + ours = clawhdf5.File(server.url("fixture.h5")) + else: + ours = clawhdf5.File.open_url(server.url("fixture.h5"), block_size=1024) yield ours, theirs, path theirs.close() ours.close() + if server is not None: + server.close() def _all_datasets(h5py, f): diff --git a/crates/clawhdf5-py/tests/test_remote.py b/crates/clawhdf5-py/tests/test_remote.py new file mode 100644 index 0000000..dbb24b8 --- /dev/null +++ b/crates/clawhdf5-py/tests/test_remote.py @@ -0,0 +1,244 @@ +"""Remote files: `clawhdf5.File(url)` / `File.open_url(url, ...)` read over +HTTP range requests (clawhdf5-remote's block cache), against a server in this +process (conftest.RangeServer). Values are compared with h5py reading the +same file locally; the rest checks what the server saw (only the blocks a +read needs are fetched), the failure modes (no range support, a missing +file, a file that changes, a server that goes away: errors, never wrong +data), and that the GIL is released while a read waits on the network.""" + +import os +import sys +import threading +import time + +import numpy as np +import pytest + +import clawhdf5 +from conftest import RangeServer + + +def _write(h5py, path): + rng = np.random.default_rng(7) + with h5py.File(path, "w") as f: + f.create_dataset("contig", data=rng.standard_normal((400, 300))) + f.create_dataset( + "chunked", + data=rng.integers(0, 1000, size=(512, 512), dtype="= 2 + + +def test_a_small_read_fetches_only_its_blocks(h5py, remote_file): + """With 4 KiB blocks, opening and reading one chunk of a 1 MB chunked + dataset costs a handful of requests and a few blocks, not the file.""" + path, url, server = remote_file + size = os.path.getsize(path) + f = clawhdf5.File.open_url(url, block_size=4096) + opened = server.requests() + assert opened == 1, server.log + ds = f["chunked"] + got = ds[0:10, 0:10] + with h5py.File(path, "r") as theirs: + np.testing.assert_array_equal(got, theirs["chunked"][0:10, 0:10]) + stats = f.remote_stats + assert stats["bytes_fetched"] < size / 4, (stats, size) + assert server.requests() - opened <= 12, server.log + # A second read of the same region is served by the cache. + before = server.requests() + ds[0:10, 0:10] + assert server.requests() == before + assert f.remote_stats["hits"] > stats["hits"] + assert clawhdf5.File(str(path), "r").remote_stats is None + + +def test_server_without_range_support(h5py, tmp_path): + """A server that ignores Range answers 200 with the whole file: that is + an OSError by default, and a whole download when allowed.""" + path = tmp_path / "remote.h5" + _write(h5py, str(path)) + server = RangeServer(tmp_path, ranges=False) + try: + url = server.url("remote.h5") + with pytest.raises(OSError, match="range"): + clawhdf5.File(url) + with clawhdf5.File.open_url(url, allow_full_download=True) as ours, h5py.File(path, "r") as theirs: + np.testing.assert_array_equal(ours["chunked"][...], theirs["chunked"][...]) + np.testing.assert_array_equal(ours["contig"][5], theirs["contig"][5]) + with pytest.raises(OSError): + clawhdf5.File.open_url(url, allow_full_download=True, max_full_download=1000) + finally: + server.close() + + +def test_errors_are_oserrors(remote_file): + _, url, server = remote_file + with pytest.raises(OSError, match="404"): + clawhdf5.File(server.url("missing.h5")) + with pytest.raises(ValueError, match="read-only"): + clawhdf5.File(url, "r+") + with pytest.raises(ValueError, match="read-only"): + clawhdf5.File(url, "w") + with pytest.raises(OSError, match="unsupported URL"): + clawhdf5.File("nosuchscheme://x/y.h5") + with pytest.raises(ValueError): + clawhdf5.File.open_url(url, block_size=0) + with pytest.raises(TypeError): + clawhdf5.File.open_url(url, no_such_option=1) + + +def test_object_store_urls_need_their_features(): + """The default wheel has no S3/GCS/Azure clients (aws-lc-rs builds C): + such a URL is an OSError naming the build feature.""" + for url, feature in [("s3://bucket/k.h5", "s3"), ("gs://b/k.h5", "gcs"), ("az://c/k.h5", "azure")]: + try: + clawhdf5.File(url) + except OSError as e: + if "feature" in str(e): + assert f"`{feature}`" in str(e), str(e) + else: + pytest.fail(f"{url} opened") + + +def test_https_needs_the_https_feature(): + """The default wheel has no TLS stack (rustls needs ring, which builds C): + an https URL is an OSError that names the build feature.""" + with pytest.raises(OSError) as e: + clawhdf5.File("https://127.0.0.1:1/x.h5") + msg = str(e.value) + # Built with `--features https` the error is the refused connection. + assert "https" in msg or "connect" in msg.lower() or "refused" in msg.lower(), msg + + +def test_a_changed_file_is_an_error_not_mixed_data(h5py, remote_file): + path, url, _ = remote_file + f = clawhdf5.File.open_url(url, block_size=1024) + first = f["grp/small"][...] + # Rewrite the file with other values: new ETag, same name. + time.sleep(0.01) + with h5py.File(path, "w") as g: + g.create_dataset("contig", data=np.zeros((400, 300))) + with pytest.raises(OSError, match="changed"): + f["contig"][...] + np.testing.assert_array_equal(first, np.arange(10, dtype=" before + assert took >= 0.2, took + assert progress["n"] > 1000, progress + # Held across a 0.2 s request, the spinner would stall that long. + assert progress["worst"] < 0.1, (progress, took) + + +def test_a_clawhdf5_written_file_reads_the_same_remotely(tmp_path, range_server): + path = tmp_path / "ours.h5" + data = np.arange(3000, dtype=" Promise` backed by `fetch` with a diff --git a/docs/known-issues.md b/docs/known-issues.md index e97f602..bf4ae74 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -844,9 +844,9 @@ cache, but: `read_*_zerocopy`) need the file in memory and answer `FormatError::ContiguousStorageRequired` otherwise; `File::as_bytes()` panics for such a file (`File::contiguous_bytes()` is the fallible form). - `LazyFile`, `MmapFile` and the Python and wasm bindings still read a - whole file (`h5rs` reads through `File::storage`, and takes URLs with its - `remote` feature). + `LazyFile`, `MmapFile` and the wasm bindings still read a whole file + (`h5rs` and the Python bindings read through `File::storage`, and take + URLs: `h5rs` with its `remote` feature, Python with `clawhdf5.File(url)`). - The file's length is read once, at open: a growing file (SWMR) is not followed (milestone M5). A remote file is pinned at open, so one that grows is `RemoteError::FileChanged`. @@ -862,9 +862,12 @@ cache, but: **Status:** open (added 2026-09-26, milestone M3 of `docs/design/range-reads.md`). -- **Python and the browser cannot open URLs yet.** `clawhdf5.File` (PyO3) - parses through `File::as_bytes`, which a remote file does not have; the - wasm reader's `openUrl` is milestone M4. +- **The browser cannot open URLs yet**: the wasm reader's `openUrl` is + milestone M4. Python can (`clawhdf5.File(url)`, since 2026-09-27), but + the default wheel reads plain `http://` only: `https://` needs a wheel + built with `--features https` (rustls with ring, which compiles C), and + `s3://`, `gs://`, `az://` the `s3`, `gcs`, `azure` features (aws-lc-rs). + The Python tests run against an in-process `http.server` only. - **The block size is fixed** (1 MiB unless `CacheConfig` says otherwise). The design's policy of using a paged file's page size as the block size is not implemented, and only the first block is read ahead. diff --git a/scripts/ci-test.sh b/scripts/ci-test.sh index 4f678ad..5731358 100755 --- a/scripts/ci-test.sh +++ b/scripts/ci-test.sh @@ -131,14 +131,17 @@ run_step "cargo clippy (h5rs remote)" cargo clippy \ # js-sys (clawhdf5-wasm's bindings to JavaScript) builds no C. # clawhdf5-remote is checked by default (plain HTTP) and with its # object-store feature, and h5rs with URL support (remote); the https -# (ring) and s3/gcs/azure (aws-lc-rs) features build C and are opt-in. +# (ring) and s3/gcs/azure (aws-lc-rs) features build C and are opt-in. The +# Python bindings (clawhdf5-py, remote reads over plain HTTP) are checked too: +# their https/s3/gcs/azure features are opt-in for the same reason. no_c_in_default_build() { local entry crate features found=0 for entry in clawhdf5-format clawhdf5-io clawhdf5-filters clawhdf5 \ clawhdf5-agent clawhdf5-ann clawhdf5-accel clawhdf5-netcdf4 clawhdf5-cli \ clawhdf5-tools \ clawhdf5-wasm \ - clawhdf5-remote clawhdf5-remote:object-store clawhdf5-tools:remote; do + clawhdf5-remote clawhdf5-remote:object-store clawhdf5-tools:remote \ + clawhdf5-py; do crate=${entry%%:*} features=() [ "$entry" != "$crate" ] && features=(--features "${entry#*:}") From 92fb0830e06e18383c91ff674cee5eeff2ae6917 Mon Sep 17 00:00:00 2001 From: osobh Date: Sun, 27 Sep 2026 06:57:36 -0500 Subject: [PATCH 2/9] py: in-place editing (clawhdf5.File(path, 'r+')) through FileEditor clawhdf5.File(path, 'r+') (and 'a' on an existing file) holds a FileEditor, and with it the file's exclusive lock, until close(): - ds[key] = value: h5py's keys and broadcasting (numpy's rules for slices and integers with extra leading 1-axes allowed; the exact shape for an index list, a scalar only where h5py expands it). Arrays are converted as libhdf5 converts them in native byte order (integers saturate, floats truncate toward zero and clip, integers go into h5py's bool enum by value); other values through numpy.asarray(value, dtype=ds.dtype), as h5py does. NaN into an integer dataset is a ValueError instead of libhdf5's arbitrary value. The value preparation is a small Python module compiled into the extension (src/edit_helpers.py). - ds.resize(shape) / ds.resize(n, axis=k) with h5py's argument rules. - attrs[name] = value, attrs.create(name, data, shape, dtype), attrs.modify: numeric, bool, complex, bytes and str data of any shape, with h5py's HDF5 types; str is stored as fixed-length UTF-8 (the editor cannot write variable-length strings). - File.mode, File.flush(), Dataset.chunks. Each edit runs with the GIL released under the file handle's write lock (no read sees a half-written edit), then the file is reopened; datasets and attrs objects re-read their shape and attributes when the handle's edit generation moved. What the editor cannot do is NotImplementedError before anything is written: deleting attributes or objects, creating datasets or groups, compound fields by name, variable-length data, and FileEditor's own limits. Where libhdf5 2.0 (h5py 3.16) converts inconsistently -- its soft conversions in non-native byte order (a float in (-1, 0) becomes the integer minimum, same-size unsigned->signed wraps) and native casts that are undefined in C (half floats into unsigned, float(max) rounded up) -- clawhdf5 saturates as libhdf5's native path does; listed in docs/known-issues.md. Tests (tests/test_edit.py): every edit applied by h5py and by clawhdf5 to copies of the same file and both read back through h5py after each edit, on h5py files (libver earliest, v114, latest) and a clawhdf5 file: a fixed sequence over every chunk index kind, compact/contiguous/gzip layouts and numeric, bool, enum, complex, string and compound types, 16 random sequences of 40 edits, and a numeric conversion matrix; a refused edit must be refused by both and leave the file unchanged. Also dense attributes, locking, objects seeing edits, readers racing a writer, and h5dump (plus h5rs check in ci-test.sh) on every edited file. The read-vs-h5py suite also runs on a file opened 'r+'. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 42 + README.md | 19 +- crates/clawhdf5-py/README.md | 41 +- crates/clawhdf5-py/src/attrs.rs | 207 +++-- crates/clawhdf5-py/src/dataset.rs | 212 ++++- crates/clawhdf5-py/src/edit.rs | 387 +++++++++ crates/clawhdf5-py/src/edit_helpers.py | 175 +++++ crates/clawhdf5-py/src/file.rs | 52 +- crates/clawhdf5-py/src/group.rs | 15 +- crates/clawhdf5-py/src/handle.rs | 118 ++- crates/clawhdf5-py/src/lib.rs | 8 + crates/clawhdf5-py/tests/test_edit.py | 742 ++++++++++++++++++ crates/clawhdf5-py/tests/test_read_vs_h5py.py | 13 +- docs/known-issues.md | 44 ++ scripts/ci-test.sh | 5 +- 15 files changed, 1978 insertions(+), 102 deletions(-) create mode 100644 crates/clawhdf5-py/src/edit.rs create mode 100644 crates/clawhdf5-py/src/edit_helpers.py create mode 100644 crates/clawhdf5-py/tests/test_edit.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b153b2..d37b2cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,48 @@ ## Unreleased +### Python bindings: in-place editing (2026-09-27) +- **`clawhdf5.File(path, 'r+')`** (and `'a'` on an existing file) opens a + file for editing through `clawhdf5::FileEditor`, holding its exclusive + lock until `close()`: + - `ds[key] = value` with h5py's keys (integers, slices with steps, `...`, + one increasing index list) and broadcasting (numpy's rules for slices + and integers, allowing extra leading length-1 axes; the exact shape for + an index list, a scalar only where h5py expands it). A numpy array is + converted to the dataset's dtype as libhdf5 converts it in native byte + order (integers saturate; floats are truncated toward zero and clipped; + integers go into h5py's bool enum by value, as libhdf5 stores them); + other values through `numpy.asarray(value, dtype=ds.dtype)`, as h5py + does. NaN into an integer dataset is a `ValueError`. + - `ds.resize(shape)` / `ds.resize(n, axis=k)` with h5py's argument rules + and errors (`TypeError` for a dataset that is not chunked). + - `obj.attrs[name] = value`, `attrs.create(name, data, shape, dtype)`, + `attrs.modify`: numeric, bool, complex, bytes and `str` data of any + shape, stored with the HDF5 types h5py uses (bool as the `FALSE`/`TRUE` + enum, complex as the `r`/`i` compound), except that `str` becomes + fixed-length UTF-8. + - Every edit is written and synced before it returns, then the file is + reopened: datasets and attrs objects taken earlier see new shapes and + attributes, and reads on other threads wait while an edit is written. + - What the editor cannot do raises `NotImplementedError` and writes + nothing (deleting attributes or objects, creating datasets or groups, + compound fields by name, variable-length data, ...; + `docs/known-issues.md`). +- `File.mode`, `File.flush()` (a no-op), `Dataset.chunks`. +- Tests (`tests/test_edit.py`): each edit applied to two copies of a file, + by h5py and by clawhdf5, and both read back through h5py after every + edit, on files h5py writes with `libver` earliest, v114 and latest and on + a clawhdf5-written one: a fixed sequence over every chunk index kind, + compact/contiguous/gzip layouts and numeric, bool, enum, complex, string + and compound types, and 16 random sequences of 40 edits (writes, + resizes, attributes); where h5py refuses an edit clawhdf5 must refuse it + and leave its file unchanged. A matrix of every numeric source dtype into + every numeric dataset dtype at the edge values, dense attribute storage, + locking, objects seeing each other's edits, readers racing a writer + (never a partly written dataset). Every edited file must pass `h5dump` + and, in `ci-test.sh`, `h5rs check`. The read-vs-h5py suite also runs on + a file opened `'r+'`. + ### Python bindings: remote files (2026-09-27) - **`clawhdf5.File(url)`** opens `http://` URLs (and `https://`, `s3://`, `gs://`, `az://` in a wheel built with the `https`, `s3`, `gcs`, diff --git a/README.md b/README.md index dccee48..6d8f2a6 100644 --- a/README.md +++ b/README.md @@ -542,6 +542,22 @@ f = clawhdf5.File.open_url("http://data.example.org/run42.h5", block_size=256 * headers={"Authorization": "Bearer ..."}) ``` +An existing file opened with `"r+"` is edited in place (through +`clawhdf5::FileEditor`), with h5py's indexing, broadcasting and numeric +conversion; each edit is on disk when the statement returns: + +```python +with clawhdf5.File("data.h5", "r+") as f: + f["group/temperatures"][100:200, ::4] = 0.0 + f["series"].resize(5000, axis=0) # chunked datasets, within maxshape + f["series"][4000:] = new_values + f["group"].attrs["calibrated"] = True +``` + +Creating or deleting datasets, groups and attributes in an existing file is +not supported (`NotImplementedError`); limits are in +[known issues](docs/known-issues.md). + The default build reads `http://` URLs only; build with `maturin develop --release --features https` (rustls with ring, which compiles C) for `https://`, and `--features s3` (or `gcs`, `azure`) for @@ -561,7 +577,8 @@ non-default fill value (`docs/known-issues.md`). An index list is read one group of neighbouring chunks at a time. Writing (`File(path, "w")`, `create_dataset`, `create_group`, `attrs[...] =`) covers `float64`, `float32`, `int64`, `int32` and `uint8` arrays. The tests -in `crates/clawhdf5-py/tests` compare every read with h5py; run them with +in `crates/clawhdf5-py/tests` compare every read and every in-place edit +with h5py; run them with `pip install pytest h5py && pytest crates/clawhdf5-py/tests`. ### Agent Memory diff --git a/crates/clawhdf5-py/README.md b/crates/clawhdf5-py/README.md index 16e4169..50b0d17 100644 --- a/crates/clawhdf5-py/README.md +++ b/crates/clawhdf5-py/README.md @@ -98,6 +98,41 @@ chunks=..., compression="gzip")`, `create_group` and `attrs[...] = ...` writes `float64`, `float32`, `int64`, `int32` and `uint8` arrays; the file is written on `close()`. +## Editing a file in place + +`clawhdf5.File(path, "r+")` (or `"a"` on an existing file) edits the file +where it is, through clawhdf5's `FileEditor`; the file is locked until +`close()`, and every edit is written and synced before the statement +returns. + +```python +with clawhdf5.File("data.h5", "r+") as f: + ds = f["grid"] + ds[10:20, ::2] = 0 # h5py keys and broadcasting + ds[[1, 4, 7], 3] = [1.5, 2.5, 3.5] # one index list: exact shape + f["series"].resize((5000, 3)) # or .resize(5000, axis=0) + f["series"].attrs["units"] = "K" + f.attrs.create("version", 2, dtype="u1") +``` + +- Values: a numpy array is converted to the dataset's dtype as libhdf5 + converts it (integers saturate at the target's limits; floats are + truncated toward zero and clipped); anything else goes through + `numpy.asarray(value, dtype=ds.dtype)`, as in h5py. Writing NaN into an + integer dataset raises `ValueError` (libhdf5 would store an arbitrary + value). A few libhdf5 edge cases differ on purpose; see + `docs/known-issues.md`. +- Shapes: `ds.resize` grows or shrinks chunked datasets within their + `maxshape`, as h5py; datasets and `attrs` objects taken before an edit + see its result. +- Attributes: numeric, bool, complex, bytes and `str` data of any shape. + `str` is stored as a fixed-length UTF-8 string (h5py stores a + variable-length one), so h5py reads it back as `bytes`. +- Not supported (`NotImplementedError`, nothing written): creating or + deleting datasets, groups and attributes, writing compound fields by + name, variable-length data, HDF5 array types, and whatever + `FileEditor` refuses (listed in `docs/known-issues.md`). + ## Tests ```bash @@ -108,8 +143,10 @@ pytest crates/clawhdf5-py/tests `tests/test_read_vs_h5py.py` compares every read with h5py on a file h5py writes, opened locally and over HTTP (an in-process range server, `tests/conftest.py`); `tests/test_remote.py` checks remote reads (requests, -failures, the GIL). `scripts/ci-test.sh` builds the wheel and runs these -in CI. +failures, the GIL); `tests/test_edit.py` applies every edit through h5py and +clawhdf5 to copies of a file and compares them through h5py (and `h5dump`, +and `h5rs check` when `CLAWHDF5_H5RS` names it). `scripts/ci-test.sh` builds +the wheel and runs these in CI. ## License diff --git a/crates/clawhdf5-py/src/attrs.rs b/crates/clawhdf5-py/src/attrs.rs index 615214b..5b3f365 100644 --- a/crates/clawhdf5-py/src/attrs.rs +++ b/crates/clawhdf5-py/src/attrs.rs @@ -1,23 +1,62 @@ //! PyAttrs — dict-like access to HDF5 attributes. -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, PoisonError}; use clawhdf5_format::attribute::AttributeMessage; -use pyo3::exceptions::{PyKeyError, PyTypeError, PyValueError}; +use pyo3::exceptions::{PyKeyError, PyNotImplementedError, PyTypeError, PyValueError}; use pyo3::prelude::*; use pyo3::types::{PyList, PyTuple}; use crate::convert::{Converter, Elements, resolve_vl}; use crate::handle::Handle; -use crate::{OwnedAttrValue, PyEmpty, attr_value_to_py, node, py_to_attr_value}; +use crate::{OwnedAttrValue, PyEmpty, attr_value_to_py, edit, node, py_to_attr_value}; + +/// The attributes of an object in a file opened for reading (or editing). +struct ReadAttrs { + handle: Arc, + addr: u64, + path: String, + /// Sorted by name, with the file generation they were read at: an edit + /// (`attrs[name] = value`, here or through another handle on the same + /// object) makes them re-read. + cache: Mutex<(u64, Arc>)>, +} + +impl ReadAttrs { + fn current(&self, py: Python<'_>) -> PyResult>> { + let generation = self.handle.generation(); + { + let cached = self.cache.lock().unwrap_or_else(PoisonError::into_inner); + if cached.0 == generation { + return Ok(Arc::clone(&cached.1)); + } + } + let (addr, path) = (self.addr, &self.path); + let attrs = Arc::new(self.handle.with(py, |f| node::attributes(f, addr, path))?); + *self.cache.lock().unwrap_or_else(PoisonError::into_inner) = + (generation, Arc::clone(&attrs)); + Ok(attrs) + } + + fn check_writable(&self) -> PyResult<()> { + if self.handle.is_writable() { + return Ok(()); + } + Err(PyErr::new::( + "cannot set attributes on a read-only file (open it with mode 'r+')", + )) + } + + fn set(&self, py: Python<'_>, name: &str, value: clawhdf5_rs::AttrValue) -> PyResult<()> { + self.check_writable()?; + let path = node::name(&self.path); + self.handle.edit(py, |ed| ed.set_attr(&path, name, &value)) + } +} /// Backing storage for attributes. enum AttrsInner { - /// Attributes of an object in a file opened for reading, sorted by name. - Read { - handle: Arc, - attrs: Vec, - }, + Read(ReadAttrs), /// Writable attribute list shared with a parent (PyFile or PyGroup). Write(Arc>>), } @@ -27,8 +66,11 @@ enum AttrsInner { /// In read mode, values are what h5py returns: numpy scalars for scalar /// attributes, numpy arrays otherwise, `str` for variable-length strings, /// `numpy.bytes_` for fixed-length ones, and `Empty` for a null dataspace. -/// In write mode, attributes set here are accumulated and written when -/// the parent file is closed. +/// In a file opened with `'r+'`, `attrs[name] = value` adds or replaces an +/// attribute in the file at once (as h5py stores it, except that `str` +/// values become fixed-length UTF-8 strings). In write mode (`'w'`), +/// attributes set here are accumulated and written when the parent file is +/// closed. #[pyclass(name = "Attrs")] pub struct PyAttrs { inner: AttrsInner, @@ -43,9 +85,15 @@ impl PyAttrs { addr: u64, path: &str, ) -> PyResult { - let attrs = handle.with(py, |f| node::attributes(f, addr, path))?; + let generation = handle.generation(); + let attrs = Arc::new(handle.with(py, |f| node::attributes(f, addr, path))?); Ok(Self { - inner: AttrsInner::Read { handle, attrs }, + inner: AttrsInner::Read(ReadAttrs { + handle, + addr, + path: path.to_string(), + cache: Mutex::new((generation, attrs)), + }), }) } @@ -55,14 +103,48 @@ impl PyAttrs { inner: AttrsInner::Write(store), } } + + fn set_value( + &self, + py: Python<'_>, + key: &str, + value: &Bound<'_, PyAny>, + dtype: Option<&Bound<'_, PyAny>>, + shape: Option<&Bound<'_, PyAny>>, + ) -> PyResult<()> { + match &self.inner { + AttrsInner::Read(r) => { + r.check_writable()?; + let value = edit::attr_value(py, value, dtype, shape)?; + r.set(py, key, value) + } + AttrsInner::Write(store) => { + if dtype.is_some() || shape.is_some() { + return Err(PyNotImplementedError::new_err( + "attrs.create with a dtype or shape is only supported in a file opened \ + with 'r+'", + )); + } + let owned = py_to_attr_value(value)?; + let mut guard = store.lock().unwrap(); + // Replace existing key if present. + if let Some(entry) = guard.iter_mut().find(|(k, _)| k == key) { + entry.1 = owned; + } else { + guard.push((key.to_string(), owned)); + } + Ok(()) + } + } + } } #[pymethods] impl PyAttrs { fn __getitem__(&self, py: Python<'_>, key: &str) -> PyResult> { match &self.inner { - AttrsInner::Read { handle, attrs } => match attrs.iter().find(|a| a.name == key) { - Some(attr) => Ok(attr_to_py(py, handle, attr)?.unbind()), + AttrsInner::Read(r) => match r.current(py)?.iter().find(|a| a.name == key) { + Some(attr) => Ok(attr_to_py(py, &r.handle, attr)?.unbind()), None => Err(PyKeyError::new_err(format!( "Can't open attribute (can't locate attribute: '{key}')" ))), @@ -80,36 +162,63 @@ impl PyAttrs { } } - fn __setitem__(&self, key: &str, value: &Bound<'_, PyAny>) -> PyResult<()> { + /// `attrs[name] = value`. In a file opened with `'r+'` this writes the + /// attribute (numeric, bool, complex, bytes and str data, any shape) + /// into the file before returning; see the class docs. + fn __setitem__(&self, py: Python<'_>, key: &str, value: &Bound<'_, PyAny>) -> PyResult<()> { + self.set_value(py, key, value, None, None) + } + + /// Deleting attributes is not supported in a file (the in-place editor + /// cannot remove them); in write mode it removes a pending attribute. + fn __delitem__(&self, key: &str) -> PyResult<()> { match &self.inner { - AttrsInner::Read { .. } => Err(PyErr::new::( - "cannot set attributes on a read-only file", - )), + AttrsInner::Read(_) => Err(PyNotImplementedError::new_err(format!( + "cannot delete attribute '{key}': deleting attributes is not supported by \ + clawhdf5's in-place editor" + ))), AttrsInner::Write(store) => { - let owned = py_to_attr_value(value)?; let mut guard = store.lock().unwrap(); - // Replace existing key if present. - if let Some(entry) = guard.iter_mut().find(|(k, _)| k == key) { - entry.1 = owned; - } else { - guard.push((key.to_string(), owned)); + let before = guard.len(); + guard.retain(|(k, _)| k != key); + if guard.len() == before { + return Err(PyKeyError::new_err(key.to_string())); } Ok(()) } } } - fn __len__(&self) -> usize { + /// h5py's `attrs.create(name, data, shape=None, dtype=None)`: `data` + /// converted to `dtype` and reshaped to `shape` first. + #[pyo3(signature = (name, data, shape=None, dtype=None))] + fn create( + &self, + py: Python<'_>, + name: &str, + data: &Bound<'_, PyAny>, + shape: Option<&Bound<'_, PyAny>>, + dtype: Option<&Bound<'_, PyAny>>, + ) -> PyResult<()> { + self.set_value(py, name, data, dtype, shape) + } + + /// h5py's `attrs.modify(name, value)`: same as `attrs[name] = value`. + fn modify(&self, py: Python<'_>, name: &str, value: &Bound<'_, PyAny>) -> PyResult<()> { + self.set_value(py, name, value, None, None) + } + + fn __len__(&self, py: Python<'_>) -> PyResult { match &self.inner { - AttrsInner::Read { attrs, .. } => attrs.len(), - AttrsInner::Write(store) => store.lock().unwrap().len(), + AttrsInner::Read(r) => Ok(r.current(py)?.len()), + AttrsInner::Write(store) => Ok(store.lock().unwrap().len()), } } - fn __contains__(&self, key: &str) -> bool { + fn __contains__(&self, py: Python<'_>, key: &str) -> PyResult { match &self.inner { - AttrsInner::Read { attrs, .. } => attrs.iter().any(|a| a.name == key), - AttrsInner::Write(store) => store.lock().unwrap().iter().any(|(k, _)| k == key), + AttrsInner::Read(r) => Ok(r.current(py)?.iter().any(|a| a.name == key)), + AttrsInner::Write(store) => Ok(store.lock().unwrap().iter().any(|(k, _)| k == key)), } } @@ -119,15 +228,17 @@ impl PyAttrs { Ok(iter) } - fn __repr__(&self) -> String { - let n = self.__len__(); - format!("") + fn __repr__(&self, py: Python<'_>) -> String { + match self.__len__(py) { + Ok(n) => format!(""), + Err(_) => "".to_string(), + } } /// The value of `key`, or `default` if there is no such attribute. #[pyo3(signature = (key, default=None))] fn get(&self, py: Python<'_>, key: &str, default: Option>) -> PyResult> { - if self.__contains__(key) { + if self.__contains__(py, key)? { self.__getitem__(py, key) } else { Ok(default.unwrap_or_else(|| py.None())) @@ -137,7 +248,7 @@ impl PyAttrs { /// Return attribute names as a list. fn keys(&self, py: Python<'_>) -> PyResult> { let names: Vec = match &self.inner { - AttrsInner::Read { attrs, .. } => attrs.iter().map(|a| a.name.clone()).collect(), + AttrsInner::Read(r) => r.current(py)?.iter().map(|a| a.name.clone()).collect(), AttrsInner::Write(store) => store .lock() .unwrap() @@ -152,9 +263,10 @@ impl PyAttrs { /// Return attribute values as a list. fn values(&self, py: Python<'_>) -> PyResult> { let vals: Vec> = match &self.inner { - AttrsInner::Read { handle, attrs } => attrs + AttrsInner::Read(r) => r + .current(py)? .iter() - .map(|a| attr_to_py(py, handle, a).map(Bound::unbind)) + .map(|a| attr_to_py(py, &r.handle, a).map(Bound::unbind)) .collect::>()?, AttrsInner::Write(store) => store .lock() @@ -173,9 +285,10 @@ impl PyAttrs { /// Return attribute (key, value) pairs as a list of tuples. fn items(&self, py: Python<'_>) -> PyResult> { let pairs: Vec<(String, Py)> = match &self.inner { - AttrsInner::Read { handle, attrs } => attrs + AttrsInner::Read(r) => r + .current(py)? .iter() - .map(|a| Ok((a.name.clone(), attr_to_py(py, handle, a)?.unbind()))) + .map(|a| Ok((a.name.clone(), attr_to_py(py, &r.handle, a)?.unbind()))) .collect::>()?, AttrsInner::Write(store) => store .lock() @@ -248,19 +361,3 @@ fn prefix_err(py: Python<'_>, name: &str, e: PyErr) -> PyErr { PyValueError::new_err(msg) } } - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn write_attrs_len() { - let store = Arc::new(Mutex::new(Vec::new())); - store - .lock() - .unwrap() - .push(("key".into(), OwnedAttrValue::I64(99))); - let attrs = PyAttrs::from_write(store); - assert_eq!(attrs.__len__(), 1); - } -} diff --git a/crates/clawhdf5-py/src/dataset.rs b/crates/clawhdf5-py/src/dataset.rs index 24c9b51..d837f73 100644 --- a/crates/clawhdf5-py/src/dataset.rs +++ b/crates/clawhdf5-py/src/dataset.rs @@ -9,12 +9,12 @@ //! Python threads reading the same or different datasets run in parallel, //! and a remote file's network reads never hold the GIL. -use std::sync::Arc; +use std::sync::{Arc, Mutex, PoisonError}; use clawhdf5_format::datatype::Datatype; use clawhdf5_format::object_header::ObjectHeader; use clawhdf5_rs::File; -use pyo3::exceptions::{PyTypeError, PyValueError}; +use pyo3::exceptions::{PyNotImplementedError, PyOSError, PyTypeError, PyValueError}; use pyo3::prelude::*; use pyo3::types::{PyList, PyTuple}; @@ -22,7 +22,7 @@ use crate::attrs::PyAttrs; use crate::convert::{Converter, Elements, VlError, resolve_vl}; use crate::handle::Handle; use crate::select::{self, Plan}; -use crate::{PyEmpty, node, to_py_err}; +use crate::{PyEmpty, edit, node, to_py_err}; /// What opening a dataset reads from the file (without the GIL). pub(crate) struct DatasetMeta { @@ -67,8 +67,9 @@ pub struct PyDataset { /// Where the dataset's object header is: reads open it from here rather /// than resolve `path` again. addr: u64, - /// `None` for a dataset with a null dataspace (h5py's `Empty`). - shape: Option>, + /// The shape (`None` for a null dataspace, h5py's `Empty`), with the + /// file generation it was read at: an edit (a resize) may change it. + shape: Mutex<(u64, Option>)>, /// The chunk shape, for a chunked dataset. chunks: Option>, datatype: Datatype, @@ -84,13 +85,14 @@ impl PyDataset { addr: u64, meta: DatasetMeta, ) -> Self { + let generation = handle.generation(); let conv = crate::no_panic(|| Converter::new(py, &meta.datatype, handle.offset_size)) .map_err(|e| e.value(py).to_string()); Self { handle, path, addr, - shape: meta.shape, + shape: Mutex::new((generation, meta.shape)), chunks: meta.chunks, datatype: meta.datatype, conv, @@ -103,10 +105,54 @@ impl PyDataset { .map_err(|msg| PyTypeError::new_err(format!("{}: {msg}", node::name(&self.path)))) } + /// The current shape: the one read at open, or re-read after an edit. + fn dims(&self, py: Python<'_>) -> PyResult>> { + let generation = self.handle.generation(); + { + let cached = self.shape.lock().unwrap_or_else(PoisonError::into_inner); + if cached.0 == generation { + return Ok(cached.1.clone()); + } + } + let addr = self.addr; + let null = self + .shape + .lock() + .unwrap_or_else(PoisonError::into_inner) + .1 + .is_none(); + let shape = if null { + None + } else { + Some(self.handle.with(py, |f| { + f.dataset_at(addr) + .and_then(|ds| ds.shape()) + .map_err(to_py_err) + })?) + }; + *self.shape.lock().unwrap_or_else(PoisonError::into_inner) = (generation, shape.clone()); + Ok(shape) + } + + fn check_writable(&self) -> PyResult<()> { + if self.handle.is_writable() { + Ok(()) + } else { + Err(PyOSError::new_err(format!( + "{}: the file is open read-only; open it with mode 'r+' to change it", + node::name(&self.path) + ))) + } + } + /// Read the selection described by `plan` into a numpy array. - fn read_plan<'py>(&self, py: Python<'py>, plan: &Plan) -> PyResult> { + fn read_plan<'py>( + &self, + py: Python<'py>, + plan: &Plan, + dims: &[u64], + ) -> PyResult> { let conv = self.converter()?; - let dims = self.shape.as_deref().unwrap_or(&[]); let out_shape = plan.out_shape(); let arr = if plan.is_empty() { @@ -272,7 +318,7 @@ impl PyDataset { /// The shape of the dataset (`None` for an empty/null dataspace). #[getter] fn shape<'py>(&self, py: Python<'py>) -> PyResult> { - match &self.shape { + match self.dims(py)? { Some(s) => Ok(PyTuple::new(py, s)?.into_any()), None => Ok(py.None().into_bound(py)), } @@ -281,7 +327,7 @@ impl PyDataset { /// The maximum shape (`None` per unlimited dimension), like h5py. #[getter] fn maxshape<'py>(&self, py: Python<'py>) -> PyResult> { - let Some(shape) = &self.shape else { + let Some(shape) = self.dims(py)? else { return Ok(py.None().into_bound(py)); }; let addr = self.addr; @@ -292,7 +338,7 @@ impl PyDataset { .and_then(|ds| ds.max_dimensions()) .map_err(to_py_err) })? - .unwrap_or_else(|| shape.clone()); + .unwrap_or(shape); let items: Vec> = max .into_iter() .map(|d| (d != u64::MAX).then_some(d)) @@ -300,6 +346,15 @@ impl PyDataset { Ok(PyTuple::new(py, items)?.into_any()) } + /// The chunk shape, or `None` for a dataset that is not chunked. + #[getter] + fn chunks<'py>(&self, py: Python<'py>) -> PyResult> { + match &self.chunks { + Some(c) => Ok(PyTuple::new(py, c)?.into_any()), + None => Ok(py.None().into_bound(py)), + } + } + /// The dataset's numpy dtype, as h5py reports it. #[getter] fn dtype<'py>(&self, py: Python<'py>) -> PyResult> { @@ -307,14 +362,14 @@ impl PyDataset { } #[getter] - fn ndim(&self) -> usize { - self.shape.as_ref().map_or(0, Vec::len) + fn ndim(&self, py: Python<'_>) -> PyResult { + Ok(self.dims(py)?.map_or(0, |s| s.len())) } /// Number of elements (`None` for an empty/null dataspace, as h5py). #[getter] - fn size(&self) -> Option { - self.shape.as_ref().map(|s| s.iter().product()) + fn size(&self, py: Python<'_>) -> PyResult> { + Ok(self.dims(py)?.map(|s| s.iter().product())) } /// The dataset's full name, e.g. `/group/data`. @@ -323,7 +378,8 @@ impl PyDataset { node::name(&self.path) } - /// The dataset's attributes (read-only, dict-like). + /// The dataset's attributes (dict-like; writable in a file opened with + /// `'r+'`). #[getter] fn attrs(&self, py: Python<'_>) -> PyResult { PyAttrs::read(py, Arc::clone(&self.handle), self.addr, &self.path) @@ -338,7 +394,7 @@ impl PyDataset { py: Python<'py>, key: &Bound<'py, PyAny>, ) -> PyResult> { - let Some(dims) = &self.shape else { + let Some(dims) = self.dims(py)? else { let is_empty_tuple = key.cast::().is_ok_and(|t| t.is_empty()); let is_ellipsis = key.is_instance_of::(); if is_empty_tuple || is_ellipsis { @@ -347,8 +403,109 @@ impl PyDataset { } return Err(PyValueError::new_err("Empty datasets cannot be sliced")); }; - let plan = select::parse(key, dims)?; - self.read_plan(py, &plan) + let plan = select::parse(key, &dims)?; + self.read_plan(py, &plan, &dims) + } + + /// Write with h5py indexing (file opened with `'r+'`): `ds[key] = value`. + /// + /// The key is what `ds[key]` reads (without compound field names). The + /// value is converted to the dataset's dtype as h5py converts it (a + /// numpy array as libhdf5 does, clipping out-of-range numbers; anything + /// else through `numpy.asarray(value, dtype=ds.dtype)`), and broadcast + /// to the selection as h5py broadcasts. The edit is written and synced + /// before this returns; what the in-place editor cannot write raises + /// `NotImplementedError` and leaves the file as it was. + fn __setitem__( + &self, + py: Python<'_>, + key: &Bound<'_, PyAny>, + value: &Bound<'_, PyAny>, + ) -> PyResult<()> { + self.check_writable()?; + let Some(dims) = self.dims(py)? else { + return Err(PyNotImplementedError::new_err( + "writing to an empty (null dataspace) dataset is not supported", + )); + }; + let plan = select::parse(key, &dims)?; + if !plan.fields.is_empty() { + return Err(PyNotImplementedError::new_err( + "writing compound fields by name is not supported by clawhdf5's in-place editor; \ + write whole elements", + )); + } + let category = edit::category(&self.datatype)?; + let conv = self.converter()?; + let bytes = edit::dataset_bytes( + py, + value, + conv.dtype.bind(py), + category, + &plan, + self.chunks.as_deref(), + )?; + if plan.is_empty() { + return Ok(()); + } + let sel = edit::selection(&plan, &dims)?; + let path = node::name(&self.path); + self.handle + .edit(py, |ed| ed.write_selection(&path, &sel, &bytes)) + } + + /// Change the dataset's shape (file opened with `'r+'`), as h5py's + /// `Dataset.resize`: `ds.resize((100, 20))`, or `ds.resize(100, axis=0)`. + /// Only chunked datasets, within their maximum shape; new elements read + /// as the fill value. + #[pyo3(signature = (size, axis=None))] + fn resize(&self, py: Python<'_>, size: &Bound<'_, PyAny>, axis: Option) -> PyResult<()> { + self.check_writable()?; + let Some(dims) = self.dims(py)? else { + return Err(PyTypeError::new_err("Empty datasets cannot be resized")); + }; + if self.chunks.is_none() { + return Err(PyTypeError::new_err("Only chunked datasets can be resized")); + } + let shape: Vec = match axis { + Some(axis) => { + let rank = dims.len(); + let a = usize::try_from(axis) + .ok() + .filter(|&a| a < rank) + .ok_or_else(|| { + PyValueError::new_err(format!( + "Invalid axis (0 to {} allowed)", + rank.saturating_sub(1) + )) + })?; + let n: u64 = size.extract().map_err(|_| { + PyTypeError::new_err("Argument must be a single int if axis is specified") + })?; + let mut s = dims.clone(); + s[a] = n; + s + } + // As h5py: without `axis` the size is a sequence (`tuple(size)`). + None => size.extract().map_err(|_| { + PyTypeError::new_err(format!( + "'{}' object is not iterable", + size.get_type() + .name() + .map(|n| n.to_string()) + .unwrap_or_default() + )) + })?, + }; + if shape.len() != dims.len() { + return Err(PyValueError::new_err(format!( + "new shape {shape:?} has {} dimensions, the dataset {}", + shape.len(), + dims.len() + ))); + } + let path = node::name(&self.path); + self.handle.edit(py, |ed| ed.resize(&path, &shape)) } /// `numpy.asarray(ds)` reads the whole dataset. @@ -360,20 +517,20 @@ impl PyDataset { copy: Option, ) -> PyResult> { let _ = copy; // every read is a fresh array - let Some(dims) = &self.shape else { + let Some(dims) = self.dims(py)? else { return Err(PyValueError::new_err("an empty dataset has no array value")); }; let ellipsis = pyo3::types::PyEllipsis::get(py).to_owned().into_any(); - let plan = select::parse(&ellipsis, dims)?; - let arr = self.read_plan(py, &plan)?; + let plan = select::parse(&ellipsis, &dims)?; + let arr = self.read_plan(py, &plan, &dims)?; match dtype { Some(dt) => arr.call_method1("astype", (dt,)), None => Ok(arr), } } - fn __len__(&self) -> PyResult { - match self.shape.as_deref() { + fn __len__(&self, py: Python<'_>) -> PyResult { + match self.dims(py)?.as_deref() { Some([first, ..]) => Ok(*first as usize), _ => Err(PyTypeError::new_err( "Attempt to take len() of scalar dataset", @@ -391,9 +548,10 @@ impl PyDataset { .unwrap_or_default(), Err(_) => format!("{:?}", self.datatype), }; - let shape = match &self.shape { - Some(s) => format!("{s:?}"), - None => "None".to_string(), + let shape = match self.dims(py) { + Ok(Some(s)) => format!("{s:?}"), + Ok(None) => "None".to_string(), + Err(_) => "?".to_string(), }; format!( "", diff --git a/crates/clawhdf5-py/src/edit.rs b/crates/clawhdf5-py/src/edit.rs new file mode 100644 index 0000000..05dd536 --- /dev/null +++ b/crates/clawhdf5-py/src/edit.rs @@ -0,0 +1,387 @@ +//! In-place editing (`clawhdf5.File(path, 'r+')`) through `FileEditor`: +//! turning what Python assigns into the bytes, selections and attribute +//! values the editor takes. +//! +//! Value conversion follows h5py (see `edit_helpers.py`, run inside the +//! extension module); what `FileEditor` cannot do is `NotImplementedError` +//! before anything is written. + +use std::ffi::CString; + +use clawhdf5_format::datatype::{ + CharacterSet, CompoundMember, Datatype, DatatypeByteOrder, EnumMember, StringPadding, +}; +use clawhdf5_format::selection::Selection; +use clawhdf5_rs::AttrValue; +use pyo3::exceptions::{PyNotImplementedError, PyTypeError}; +use pyo3::prelude::*; +use pyo3::sync::PyOnceLock; +use pyo3::types::{PyBytes, PyModule, PyTuple}; + +use crate::select::{Axis, Plan}; + +/// The helper module, compiled once. +pub(crate) fn helpers(py: Python<'_>) -> PyResult<&Bound<'_, PyModule>> { + static HELPERS: PyOnceLock> = PyOnceLock::new(); + let module = HELPERS.get_or_try_init(py, || -> PyResult> { + let code = CString::new(include_str!("edit_helpers.py")) + .map_err(|e| PyTypeError::new_err(e.to_string()))?; + Ok(PyModule::from_code( + py, + &code, + c"clawhdf5/edit_helpers.py", + c"clawhdf5._edit_helpers", + )? + .unbind()) + })?; + Ok(module.bind(py)) +} + +fn not_implemented(what: impl std::fmt::Display) -> PyErr { + PyNotImplementedError::new_err(format!( + "{what} is not supported by clawhdf5's in-place editor" + )) +} + +/// How values for a dataset of type `dt` are converted (a category of +/// `edit_helpers._convert_array`), or why they cannot be written. +pub(crate) fn category(dt: &Datatype) -> PyResult<&'static str> { + match dt { + Datatype::FixedPoint { .. } => Ok("int"), + Datatype::FloatingPoint { .. } => Ok("float"), + Datatype::Enumeration { + base_type, members, .. + } => { + let is_bool = base_type.type_size() == 1 + && members.len() == 2 + && members + .iter() + .any(|m| m.name == "FALSE" && m.value.first() == Some(&0)) + && members + .iter() + .any(|m| m.name == "TRUE" && m.value.first() == Some(&1)); + Ok(if is_bool { "bool" } else { "enum" }) + } + Datatype::String { + padding: StringPadding::NullPad, + .. + } => Ok("string"), + Datatype::String { padding, .. } => Err(not_implemented(format!( + "writing fixed-length strings padded {padding:?} (libhdf5 converts them \ + differently from numpy)" + ))), + Datatype::Compound { size, members } => { + if is_complex(*size, members) { + return Ok("complex"); + } + check_exact(dt)?; + Ok("exact") + } + Datatype::Opaque { .. } => Ok("exact"), + Datatype::Array { .. } => Err(not_implemented("writing HDF5 array-type elements")), + Datatype::VariableLength { .. } => Err(not_implemented("writing variable-length data")), + Datatype::Reference { .. } => Err(not_implemented("writing references")), + Datatype::BitField { .. } => Err(not_implemented("writing bitfields")), + Datatype::Time { .. } => Err(not_implemented("writing time values")), + } +} + +/// h5py's complex numbers: a compound of two identical floats `r`, `i`. +fn is_complex(size: u32, members: &[CompoundMember]) -> bool { + matches!(members, [r, i] if r.name == "r" && i.name == "i" + && r.datatype == i.datatype + && matches!(r.datatype, Datatype::FloatingPoint { size: fs, .. } + if r.byte_offset == 0 && i.byte_offset == u64::from(fs) && size == 2 * fs)) +} + +/// Compound members written byte for byte from the same numpy dtype: fine +/// unless libhdf5 would convert them on the way (strings padded other than +/// with NULs), or the editor cannot write them at all. +fn check_exact(dt: &Datatype) -> PyResult<()> { + match dt { + Datatype::Compound { members, .. } => { + members.iter().try_for_each(|m| check_exact(&m.datatype)) + } + Datatype::Array { base_type, .. } => check_exact(base_type), + Datatype::String { + padding: StringPadding::NullPad, + .. + } + | Datatype::FixedPoint { .. } + | Datatype::FloatingPoint { .. } + | Datatype::Enumeration { .. } + | Datatype::Opaque { .. } + | Datatype::BitField { .. } => Ok(()), + Datatype::String { .. } => Err(not_implemented( + "writing compounds with strings not padded with NULs", + )), + Datatype::VariableLength { .. } => Err(not_implemented( + "writing compounds with variable-length members", + )), + Datatype::Reference { .. } => Err(not_implemented("writing references")), + Datatype::Time { .. } => Err(not_implemented("writing time values")), + } +} + +/// Largest point selection an index-list write builds (one coordinate +/// vector per element). +const MAX_POINTS: usize = 1 << 22; + +/// The selection `plan` writes, whose elements are numbered as the value's +/// (row-major over the selection's shape). +pub(crate) fn selection(plan: &Plan, dims: &[u64]) -> PyResult { + if plan.axes.is_empty() { + return Ok(Selection::All); + } + if plan.list_axis().is_none() { + let (reads, _) = plan.reads(dims, None, 1); + return match <[_; 1]>::try_from(reads) { + Ok([read]) => Ok(read.sel), + Err(_) => Err(PyTypeError::new_err("internal error: several hyperslabs")), + }; + } + // An index list: the points, in the value's order. + let per_axis: Vec> = plan + .axes + .iter() + .map(|a| match a { + Axis::Index(i) => vec![*i], + Axis::Slice { start, step, count } => (0..*count).map(|k| start + k * step).collect(), + Axis::List(v) => v.clone(), + }) + .collect(); + let n = per_axis + .iter() + .try_fold(1usize, |acc, v| acc.checked_mul(v.len())) + .filter(|&n| n <= MAX_POINTS) + .ok_or_else(|| { + not_implemented(format!( + "an index-list write of more than {MAX_POINTS} elements (write it in slices)" + )) + })?; + let mut points = Vec::with_capacity(n); + let mut at = vec![0usize; per_axis.len()]; + for _ in 0..n { + points.push(at.iter().zip(&per_axis).map(|(&i, v)| v[i]).collect()); + for d in (0..at.len()).rev() { + at[d] += 1; + if at[d] < per_axis[d].len() { + break; + } + at[d] = 0; + } + } + Ok(Selection::Points(points)) +} + +/// The bytes to write for `value` under `plan`, in the dataset's dtype. +pub(crate) fn dataset_bytes( + py: Python<'_>, + value: &Bound<'_, PyAny>, + dtype: &Bound<'_, PyAny>, + category: &str, + plan: &Plan, + chunks: Option<&[u64]>, +) -> PyResult> { + let shape = PyTuple::new(py, plan.out_shape())?; + let fancy = plan.list_axis().is_some(); + let chunk_elems = chunks.map_or(0, |c| c.iter().fold(1u64, |a, &d| a.saturating_mul(d))); + let bytes = helpers(py)?.call_method1( + "dataset_values", + (value, dtype, category, shape, fancy, chunk_elems), + )?; + Ok(bytes.cast::()?.as_bytes().to_vec()) +} + +fn ieee_float(size: u32, byte_order: DatatypeByteOrder) -> Option { + let (exponent_location, exponent_size, mantissa_size, exponent_bias) = match size { + 2 => (10, 5, 10, 15), + 4 => (23, 8, 23, 127), + 8 => (52, 11, 52, 1023), + _ => return None, + }; + Some(Datatype::FloatingPoint { + size, + byte_order, + bit_offset: 0, + bit_precision: (size * 8) as u16, + exponent_location, + exponent_size, + mantissa_location: 0, + mantissa_size, + exponent_bias, + }) +} + +/// The HDF5 datatype h5py writes for a numpy dtype string (`'f8'`, `' Option { + let order = match dtype.as_bytes().first()? { + b'<' | b'|' | b'=' => DatatypeByteOrder::LittleEndian, + b'>' => DatatypeByteOrder::BigEndian, + _ => return None, + }; + let kind = dtype.as_bytes().get(1)?; + let size: u32 = dtype.get(2..)?.parse().ok()?; + match kind { + b'b' if size == 1 => Some(Datatype::Enumeration { + size: 1, + base_type: Box::new(Datatype::FixedPoint { + size: 1, + byte_order: DatatypeByteOrder::LittleEndian, + signed: true, + bit_offset: 0, + bit_precision: 8, + }), + members: vec![ + EnumMember { + name: "FALSE".into(), + value: vec![0], + }, + EnumMember { + name: "TRUE".into(), + value: vec![1], + }, + ], + }), + b'i' | b'u' if matches!(size, 1 | 2 | 4 | 8) => Some(Datatype::FixedPoint { + size, + byte_order: order, + signed: *kind == b'i', + bit_offset: 0, + bit_precision: (size * 8) as u16, + }), + b'f' => ieee_float(size, order), + b'c' => { + let part = ieee_float(size / 2, order)?; + Some(Datatype::Compound { + size, + members: vec![ + CompoundMember { + name: "r".into(), + byte_offset: 0, + datatype: part.clone(), + }, + CompoundMember { + name: "i".into(), + byte_offset: u64::from(size / 2), + datatype: part, + }, + ], + }) + } + b'S' if size > 0 => Some(Datatype::String { + size, + padding: StringPadding::NullPad, + charset: CharacterSet::Ascii, + }), + _ => None, + } +} + +/// An attribute value as h5py would store it (`attrs[name] = value`, or +/// `attrs.create(name, data, shape, dtype)`), except that `str` data is +/// stored as fixed-length UTF-8 strings (h5py stores variable-length ones, +/// which the editor cannot write). +pub(crate) fn attr_value( + py: Python<'_>, + value: &Bound<'_, PyAny>, + dtype: Option<&Bound<'_, PyAny>>, + shape: Option<&Bound<'_, PyAny>>, +) -> PyResult { + if value.is_instance_of::() { + return Err(not_implemented( + "writing an empty (null dataspace) attribute", + )); + } + let (kind, dt, dims, data): (String, String, Vec, Vec) = helpers(py)? + .call_method1("attr_value", (value, dtype, shape))? + .extract()?; + let datatype = if kind == "str" { + let size: u32 = dt + .parse() + .map_err(|_| PyTypeError::new_err("bad string size"))?; + Datatype::String { + size, + padding: StringPadding::NullPad, + charset: CharacterSet::Utf8, + } + } else { + datatype_of(&dt).ok_or_else(|| not_implemented(format!("an attribute of dtype {dt}")))? + }; + Ok(AttrValue::Raw { + datatype, + shape: dims, + data, + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn numpy_dtypes_map_to_h5py_types() { + assert!(matches!( + datatype_of("u2"), + Some(Datatype::FixedPoint { + size: 2, + signed: false, + byte_order: DatatypeByteOrder::BigEndian, + .. + }) + )); + assert!(matches!( + datatype_of(" {dst!r}") + + +def _to_int(arr, dtype): + """Integer target: libhdf5's saturating conversion.""" + info = np.iinfo(dtype) + kind = arr.dtype.kind + if kind == "b": + return arr.astype(dtype) + if kind in "iu": + src = np.iinfo(arr.dtype) + lo = max(info.min, src.min) + hi = min(info.max, src.max) + clipped = np.clip(arr, np.array(lo, arr.dtype), np.array(hi, arr.dtype)) + return clipped.astype(dtype) + if kind == "f": + if np.isnan(arr).any(): + raise ValueError( + "cannot write NaN to an integer dataset (libhdf5 would store an arbitrary value)" + ) + t = np.trunc(arr.astype(np.float64)) + # info.max + 1 and info.min are powers of two: exact as floats. + over = t >= float(info.max + 1) + under = t < float(info.min) + out = np.where(over | under, 0.0, t).astype(dtype) + out[over] = info.max + out[under] = info.min + return out + raise _no_path(arr.dtype, dtype) + + +def _convert_array(arr, dtype, category): + kind = arr.dtype.kind + if category in ("int", "enum"): + if arr.dtype == dtype and kind in "iu": + return arr + if category == "enum" and kind not in "iu": + raise _no_path(arr.dtype, dtype) + return _to_int(arr, dtype) + if category == "bool": + if kind == "b": + return arr.astype(dtype) + if kind in "iu": + # h5py's bool is an enum over int8; libhdf5 converts integers + # into it by value (saturating), not to FALSE/TRUE, so 3 is + # stored as 3. Keep those bytes: a view, not a cast. + return _to_int(arr, np.dtype("i1")).view(dtype) + raise _no_path(arr.dtype, dtype) + if category == "float": + if kind not in "biuf": + raise _no_path(arr.dtype, dtype) + with np.errstate(over="ignore", invalid="ignore"): + return arr.astype(dtype) + if category == "complex": + if kind != "c": + raise _no_path(arr.dtype, dtype) + with np.errstate(over="ignore", invalid="ignore"): + return arr.astype(dtype) + if category == "string": + if kind != "S": + raise _no_path(arr.dtype, dtype) + return arr.astype(dtype) + # "exact": compound and opaque types, written only from the same dtype. + if arr.dtype == dtype: + return arr + raise _no_path(arr.dtype, dtype) + + +def _convert_other(value, dtype, category): + if category == "string": + items = np.asarray(value, dtype=object) + if any(isinstance(x, str) for x in items.flat): + meta = dtype.metadata or {} + if meta.get("h5py_encoding") == "utf-8": + enc = [x.encode("utf-8") if isinstance(x, str) else x for x in items.flat] + return np.array(enc, dtype=dtype).reshape(items.shape) + return np.asarray(value, dtype=dtype) + + +def _broadcast(arr, shape, fancy, chunk_elems): + """h5py's broadcasting: numpy's rules against the selection's shape + (extra leading length-1 axes allowed) for slices and integers. For an + index list, the exact shape; a scalar only where h5py expands it to the + whole selection (a chunked dataset whose chunk holds at least as many + elements as the selection).""" + if arr.shape == shape: + return arr + if fancy: + size = int(np.prod(shape)) + if arr.ndim == 0 and ((chunk_elems > 0 and size <= chunk_elems) or len(shape) == 1): + return np.broadcast_to(arr, shape) + raise TypeError("Broadcasting is not supported for complex selections") + if arr.ndim == 0: + return np.broadcast_to(arr, shape) + err = TypeError(f"Can't broadcast {arr.shape} -> {shape}") + src = arr.shape + while len(src) > len(shape) and src[0] == 1: + src = src[1:] + if len(src) > len(shape): + raise err + try: + return np.broadcast_to(arr.reshape(src), shape) + except ValueError: + raise err from None + + +def dataset_values(value, dtype, category, shape, fancy, chunk_elems): + """The bytes to write for `value` under a selection of `shape`, as a + C-ordered array of the dataset's dtype.""" + if isinstance(value, np.ndarray): + arr = _convert_array(value, dtype, category) + else: + arr = _convert_other(value, dtype, category) + arr = _broadcast(arr, tuple(shape), fancy, chunk_elems) + return np.ascontiguousarray(arr, dtype=dtype).tobytes() + + +def attr_value(value, dtype=None, shape=None): + """(kind, dtype string, shape, bytes) for an attribute value, h5py's + `attrs[name] = value` / `attrs.create(name, data, shape, dtype)`: + + - "str": `str` data (h5py would store a variable-length string; this + stores a fixed-length UTF-8 string, which clawhdf5 can write); + the dtype string is the byte length of the longest element; + - "raw": a numeric, bool or bytes array, as numpy lays it out. + """ + if dtype is not None: + arr = np.asarray(value, dtype=dtype, order="C") + else: + arr = np.asarray(value, order="C") + if shape is not None: + arr = arr.reshape(shape) + kind = arr.dtype.kind + if kind == "O": + if arr.size and all(isinstance(x, str) for x in arr.flat): + kind = "U" + elif arr.size and all(isinstance(x, bytes) for x in arr.flat): + arr = arr.astype(bytes) + kind = "S" + else: + raise TypeError( + f"clawhdf5 cannot write an attribute of Python objects ({value!r:.60})" + ) + if kind == "U": + enc = [str(x).encode("utf-8") for x in arr.flat] + size = max([len(b) for b in enc] + [1]) + data = np.array(enc, dtype=f"S{size}").reshape(arr.shape) + return ("str", str(size), arr.shape, data.tobytes()) + if kind in "biufcS": + return ("raw", arr.dtype.str, arr.shape, np.ascontiguousarray(arr).tobytes()) + raise NotImplementedError( + f"clawhdf5 cannot write an attribute of dtype {arr.dtype} in place" + ) diff --git a/crates/clawhdf5-py/src/file.rs b/crates/clawhdf5-py/src/file.rs index 48d94c0..2764e61 100644 --- a/crates/clawhdf5-py/src/file.rs +++ b/crates/clawhdf5-py/src/file.rs @@ -5,7 +5,7 @@ use std::path::PathBuf; use std::sync::{Arc, Mutex}; use std::time::Duration; -use pyo3::exceptions::PyValueError; +use pyo3::exceptions::{PyNotImplementedError, PyValueError}; use pyo3::prelude::*; use pyo3::types::{PyDict, PyList}; @@ -95,6 +95,18 @@ impl PyFile { } match mode { "r" => Ok(Self::from_handle(Handle::open_local(py, path)?, filename)), + "r+" => Ok(Self::from_handle( + Handle::open_editable(py, path)?, + filename, + )), + "a" if std::path::Path::new(path).exists() => Ok(Self::from_handle( + Handle::open_editable(py, path)?, + filename, + )), + "a" => Err(PyNotImplementedError::new_err(format!( + "mode 'a' on {path}, which does not exist: clawhdf5 can only edit an existing \ + file in place; create a new one with mode 'w'" + ))), "w" => Ok(Self { filename, inner: Some(FileInner::Write(WriteState { @@ -105,7 +117,7 @@ impl PyFile { })), }), other => Err(PyValueError::new_err(format!( - "unsupported mode '{other}'; expected 'r' or 'w'" + "unsupported mode '{other}'; expected 'r', 'r+', 'a' or 'w'" ))), } } @@ -226,7 +238,10 @@ impl PyFile { PyErr::new::("file is already closed") })?; match inner { - FileInner::Read(_) => Ok(()), + FileInner::Read(root) => { + root.handle.close(); + Ok(()) + } FileInner::Write(state) => finalize_write(state), } } @@ -289,6 +304,30 @@ impl PyFile { "/" } + /// `'r'` for a file opened read-only (a local file or a URL), `'r+'` + /// for one open for editing or writing, as h5py reports it. + #[getter] + fn mode(&self) -> PyResult<&'static str> { + match &self.inner { + Some(FileInner::Read(root)) if !root.handle.is_writable() => Ok("r"), + Some(_) => Ok("r+"), + None => Err(PyErr::new::( + "file is closed", + )), + } + } + + /// Nothing to do: every edit is written and synced when it is made, and + /// a file opened with 'w' is written on `close()`. + fn flush(&self) {} + + /// Deleting objects is not supported (h5py's `del f[name]`). + fn __delitem__(&self, key: &str) -> PyResult<()> { + Err(PyNotImplementedError::new_err(format!( + "cannot delete '{key}': deleting objects is not supported by clawhdf5" + ))) + } + /// The path (or URL) the file was opened with. #[getter] fn filename(&self) -> &str { @@ -389,6 +428,13 @@ impl PyFile { fn write_state_mut(&mut self) -> PyResult<&mut WriteState> { match &mut self.inner { Some(FileInner::Write(s)) => Ok(s), + Some(FileInner::Read(root)) if root.handle.is_writable() => { + Err(PyNotImplementedError::new_err( + "creating datasets or groups in an existing file is not supported by \ + clawhdf5's in-place editor (mode 'r+' changes values, shapes and \ + attributes)", + )) + } Some(FileInner::Read(_)) => Err(PyErr::new::( "cannot write to a file opened for reading", )), diff --git a/crates/clawhdf5-py/src/group.rs b/crates/clawhdf5-py/src/group.rs index 36506b9..4020f16 100644 --- a/crates/clawhdf5-py/src/group.rs +++ b/crates/clawhdf5-py/src/group.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use std::sync::{Arc, Mutex, OnceLock}; -use pyo3::exceptions::{PyIOError, PyKeyError, PyOSError, PyValueError}; +use pyo3::exceptions::{PyIOError, PyKeyError, PyNotImplementedError, PyOSError, PyValueError}; use pyo3::prelude::*; use pyo3::types::PyList; @@ -301,12 +301,23 @@ impl PyGroup { state.lock().unwrap().datasets.push(spec); Ok(()) } - GroupInner::Read { .. } => Err(PyIOError::new_err( + GroupInner::Read(g) if g.handle.is_writable() => Err(PyNotImplementedError::new_err( + "creating datasets or groups in an existing file is not supported by \ + clawhdf5's in-place editor (mode 'r+' changes values, shapes and attributes)", + )), + GroupInner::Read(_) => Err(PyIOError::new_err( "cannot create datasets on a read-only group", )), } } + /// Deleting objects is not supported (h5py's `del group[name]`). + fn __delitem__(&self, key: &str) -> PyResult<()> { + Err(PyNotImplementedError::new_err(format!( + "cannot delete '{key}': deleting objects is not supported by clawhdf5" + ))) + } + /// Attribute access. #[getter] fn attrs(&self, py: Python<'_>) -> PyResult { diff --git a/crates/clawhdf5-py/src/handle.rs b/crates/clawhdf5-py/src/handle.rs index 8649f74..754de6f 100644 --- a/crates/clawhdf5-py/src/handle.rs +++ b/crates/clawhdf5-py/src/handle.rs @@ -2,25 +2,34 @@ //! //! Every read goes through [`Handle::with`], which releases the GIL and //! parses through `File::storage()`, so the same code serves a local file -//! (memory-mapped) and a remote one (`clawhdf5-remote`: range requests -//! through a block cache, so a network read never holds the GIL). +//! (memory-mapped), a remote one (`clawhdf5-remote`: range requests through +//! a block cache, so a network read never holds the GIL) and a file open +//! for editing. +//! +//! A file opened with `'r+'` also holds a [`FileEditor`]. An edit takes the +//! file's write lock, so no read runs while the file changes underneath it, +//! and reopens the file afterwards: reads after an edit see the new bytes +//! (a grown file, a new dataspace), never a stale mapping or chunk cache. +//! Objects that cache something an edit can change compare +//! [`Handle::generation`] with the value they cached it at. //! //! Lock discipline (no deadlock with the GIL): the file lock is only taken //! with the GIL released, and code that holds it never touches Python. use std::path::PathBuf; -use std::sync::{Arc, PoisonError, RwLock}; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::{Arc, Mutex, PoisonError, RwLock}; -use clawhdf5_rs::File; +use clawhdf5_rs::{File, FileEditor}; use pyo3::exceptions::PyOSError; use pyo3::prelude::*; -use crate::to_py_err; +use crate::{panic_text, to_py_err}; /// Where the file's bytes come from. pub(crate) enum Source { /// A local path (memory-mapped). - Local(#[allow(dead_code)] PathBuf), + Local(PathBuf), /// A URL, read through `clawhdf5-remote`'s block cache. Remote { url: String, @@ -29,8 +38,14 @@ pub(crate) enum Source { } pub(crate) struct Handle { + /// The file as last opened; `None` if reopening it after an edit failed + /// (every read is then an error rather than a read of stale bytes). file: RwLock>, + /// For `'r+'`: the editor, until the file is closed. + editor: Option>>, source: Source, + /// Bumped by every edit. + generation: AtomicU64, pub offset_size: u8, pub length_size: u8, pub root: u64, @@ -41,13 +56,15 @@ fn closed_after_failed_reopen() -> PyErr { } impl Handle { - fn new(file: File, source: Source) -> Arc { + fn new(file: File, source: Source, editor: Option) -> Arc { let sb = file.superblock(); let (offset_size, length_size, root) = (sb.offset_size, sb.length_size, sb.root_group_address); Arc::new(Self { file: RwLock::new(Some(file)), + editor: editor.map(|e| Mutex::new(Some(e))), source, + generation: AtomicU64::new(0), offset_size, length_size, root, @@ -57,7 +74,25 @@ impl Handle { /// A local file, read-only. pub(crate) fn open_local(py: Python<'_>, path: &str) -> PyResult> { let file = py.detach(|| crate::no_panic(|| File::open(path).map_err(to_py_err)))?; - Ok(Self::new(file, Source::Local(PathBuf::from(path)))) + Ok(Self::new(file, Source::Local(PathBuf::from(path)), None)) + } + + /// A local file, open for in-place editing (`'r+'`): the editor takes + /// the file's exclusive lock and checks that it can edit the file, then + /// the file is opened for reading. + pub(crate) fn open_editable(py: Python<'_>, path: &str) -> PyResult> { + let (file, editor) = py.detach(|| { + crate::no_panic(|| { + let editor = FileEditor::open(path).map_err(to_py_err)?; + let file = File::open(path).map_err(to_py_err)?; + Ok((file, editor)) + }) + })?; + Ok(Self::new( + file, + Source::Local(PathBuf::from(path)), + Some(editor), + )) } /// A remote file (`http(s)://`, `s3://`, ...). @@ -79,6 +114,7 @@ impl Handle { url: url.to_string(), storage, }, + None, )) } @@ -102,6 +138,17 @@ impl Handle { }) } + /// Edits so far: objects that cache something an edit can change (a + /// dataset's shape, an object's attributes) re-read it when this moved. + pub(crate) fn generation(&self) -> u64 { + self.generation.load(Ordering::Acquire) + } + + /// Whether the file was opened for editing (`'r+'`), even if closed since. + pub(crate) fn is_writable(&self) -> bool { + self.editor.is_some() + } + /// The remote file's block cache. pub(crate) fn remote_storage(&self) -> Option<&clawhdf5_remote::RemoteStorage> { match &self.source { @@ -117,6 +164,61 @@ impl Handle { Source::Local(_) => None, } } + + /// Release the editor, and with it the file's lock. Objects still + /// open keep reading the file as it was last written; an edit through + /// them is an error. + pub(crate) fn close(&self) { + if let Some(ed) = &self.editor { + ed.lock().unwrap_or_else(PoisonError::into_inner).take(); + } + } + + /// Apply one edit with the GIL released. No read runs while it writes, + /// and the file is reopened afterwards — also after a failed edit, since + /// a commit that failed part-way may have changed the file. + pub(crate) fn edit( + &self, + py: Python<'_>, + f: impl FnOnce(&mut FileEditor) -> Result + Send, + ) -> PyResult { + let Some(editor) = &self.editor else { + return Err(PyOSError::new_err(match self.source { + Source::Remote { .. } => "remote files are read-only", + Source::Local(_) => { + "the file is open read-only; open it with mode 'r+' to change it" + } + })); + }; + let Source::Local(path) = &self.source else { + return Err(PyOSError::new_err("remote files are read-only")); + }; + py.detach(|| { + let mut ed = editor.lock().unwrap_or_else(PoisonError::into_inner); + let ed = ed + .as_mut() + .ok_or_else(|| PyOSError::new_err("the file is closed"))?; + let mut file = self.file.write().unwrap_or_else(PoisonError::into_inner); + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| f(ed))); + // Drop the old mapping and its chunk cache before reopening. + *file = None; + let reopened = std::panic::catch_unwind(|| File::open(path)); + self.generation.fetch_add(1, Ordering::AcqRel); + match reopened { + Ok(Ok(f)) => *file = Some(f), + Ok(Err(e)) => return Err(to_py_err(e)), + Err(_) => return Err(closed_after_failed_reopen()), + } + drop(file); + match result { + Ok(r) => r.map_err(to_py_err), + Err(p) => Err(crate::InternalError::new_err(format!( + "clawhdf5 internal error (please report it): {}", + panic_text(&*p) + ))), + } + }) + } } /// A `clawhdf5_remote::Error` as a Python exception: the network side diff --git a/crates/clawhdf5-py/src/lib.rs b/crates/clawhdf5-py/src/lib.rs index db89d8d..4ae81f7 100644 --- a/crates/clawhdf5-py/src/lib.rs +++ b/crates/clawhdf5-py/src/lib.rs @@ -7,11 +7,19 @@ //! //! with clawhdf5.File('data.h5', 'r') as f: //! data = f['dataset_name'][:] +//! +//! with clawhdf5.File('http://host/data.h5') as f: # range requests +//! block = f['dataset_name'][10:20] +//! +//! with clawhdf5.File('data.h5', 'r+') as f: # in-place edits +//! f['dataset_name'][0] = 1.5 +//! f.attrs['note'] = 'edited' //! ``` mod attrs; mod convert; mod dataset; +mod edit; mod file; mod group; mod handle; diff --git a/crates/clawhdf5-py/tests/test_edit.py b/crates/clawhdf5-py/tests/test_edit.py new file mode 100644 index 0000000..99090e2 --- /dev/null +++ b/crates/clawhdf5-py/tests/test_edit.py @@ -0,0 +1,742 @@ +"""In-place editing: clawhdf5.File(path, 'r+') against h5py. + +Every edit is applied twice, to two copies of the same file: once through +h5py (libhdf5) and once through clawhdf5 (FileEditor). After every edit both +files are read back with h5py and must hold the same shapes, values and +attributes; clawhdf5's own view must agree; when h5py refuses an edit, +clawhdf5 must refuse it too and leave its file as it was. Files are written +by h5py (libver earliest and latest, so every chunk index kind) and by +clawhdf5; `h5dump` must read every result.""" + +import io +import os +import shutil +import subprocess +import threading + +import numpy as np +import pytest + +import clawhdf5 + + +# --------------------------------------------------------------------------- +# Files +# --------------------------------------------------------------------------- + +ENUM = {"RED": 0, "GREEN": 1, "BLUE": 7} + + +def _h5py_file(h5py, path, libver): + rng = np.random.default_rng(1) + with h5py.File(path, "w", libver=libver) as f: + f.create_dataset("i4", data=np.arange(60, dtype="u2", "i8", "f8"] + + +def _quiet(f): + with np.errstate(all="ignore"): + return f() + + +def _libhdf5_undefined(vals, target): + """The values whose conversion to `target` libhdf5 2.0 (h5py 3.16) gets + wrong even in native byte order, where its C casts are undefined + behaviour; clawhdf5 saturates them as libhdf5's range handling + intends (docs/known-issues.md): + + - half floats into unsigned integers: negatives wrap (-1 -> 65535) and + +inf becomes 0; into signed integers, +-inf becomes the minimum; + - a float equal to the integer maximum rounded up in the float's + precision (float32(2**31 - 1) == 2**31 -> int32, float64(2**64 - 1) + -> uint64) becomes the minimum (or 0); + - a double between 65504 and 65520 into a half float becomes infinity + (IEEE rounds it down to 65504, as numpy does). + """ + bad = np.zeros(vals.shape, dtype=bool) + if vals.dtype.kind != "f": + return bad + if target.kind in "iu": + if vals.dtype.itemsize == 2: + bad |= np.isinf(vals) + if target.kind == "u": + bad |= vals <= -1 + top = _quiet(lambda: np.array(np.iinfo(target).max).astype(vals.dtype)) + if float(top) > np.iinfo(target).max: + bad |= vals == top + if target.kind == "f" and target.itemsize == 2 and vals.dtype.itemsize > 2: + bad |= (np.abs(vals) > 65504) & (np.abs(vals) < 65520) + return bad + + +def test_numeric_conversions_match_h5py(h5py, tmp_path): + """Every numeric source dtype into every numeric dataset dtype, with + values at and beyond the targets' limits, as libhdf5 converts them.""" + edge = np.array([0, 1, -1, -0.3, 2.5, -2.5, 3.7, -3.7, 127.9, -128.9, 200.5, 255.5, 256, -129, + 32767.5, 40000, 65504, 70000, -70000, 2**31 - 1, 2**31, -2**31 - 1, + 4e9, 1e15, -1e15, 1e19, 1e300, -1e300, np.inf, -np.inf]) + sources = { + "f8": edge, + "f4": _quiet(lambda: edge.astype(" {target}" + try: + with h5py.File(path_t, "r+") as f: + f["d"][...] = _native_conversion(h5py, vals, f["d"].dtype) + except Exception: # noqa: BLE001 + with clawhdf5.File(path_o, "r+") as f, pytest.raises(Exception): + f["d"][...] = vals + continue + with clawhdf5.File(path_o, "r+") as f: + f["d"][...] = vals + with h5py.File(path_t, "r") as a, h5py.File(path_o, "r") as b: + assert a["d"][...].tobytes() == b["d"][...].tobytes(), ( + f"{what}: h5py {a['d'][...].tolist()} clawhdf5 {b['d'][...].tolist()}" + ) + + +def test_nan_into_an_integer_dataset_is_refused(h5py, tmp_path): + """libhdf5 stores NaN as an arbitrary integer (0, the minimum or 2**63, + depending on the type); clawhdf5 refuses and writes nothing.""" + path = str(tmp_path / "nan.h5") + with h5py.File(path, "w") as f: + f.create_dataset("d", data=np.arange(4, dtype="f4": np.array([1.5, 2.5], dtype=">f4"), + "f2": np.float16(0.5), + "b": True, + "barr": np.array([True, False]), + "c16": np.complex128(1 + 2j), + "bytes": b"raw", + "sarr": np.array([b"a", b"bcd"]), + "str": "héllo", + "strs": ["x", "yz"], + "2d": np.arange(12, dtype="f4"].dtype == np.dtype(">f4") + assert a["f2"].dtype == np.float16 + assert a["b"] is np.True_ or a["b"] == True # noqa: E712 + assert a["barr"].dtype == np.bool_ + assert a["c16"] == 1 + 2j + assert a["bytes"] == b"raw" + assert list(a["sarr"]) == [b"a", b"bcd"] + # str is stored as fixed-length UTF-8: h5py reads bytes. + assert a["str"].decode("utf-8") == "héllo" + assert [x.decode() for x in a["strs"]] == ["x", "yz"] + assert a["2d"].shape == (3, 4) and a["2d"].dtype == np.dtype("/dev/null 2>&1 && "$PYTHON" -c "import pytest" >/dev/null 2>&1; then From 1f7651644bd17ab3113acfcde075c82450cd01b1 Mon Sep 17 00:00:00 2001 From: osobh Date: Sun, 27 Sep 2026 07:40:01 -0500 Subject: [PATCH 3/9] edit: record the maximum before resizing a chunked dataset that has none FileEditor::resize (on main since PR #18, a4c2ace) scrambled the values of a chunked dataset whose dataspace records no maximum dimensions when it shrank it: clawhdf5's writer stores such a dataspace for every chunked dataset created without a maxshape, the maximum is then the current dimensions, and the Fixed Array index linearises chunks by the maximum, so patching only the current dimensions moved every chunk after the first row. h5py, h5dump and our reader all read the wrong values; the dataset could not grow back either. libhdf5 never writes such a dataspace (H5S_set_extent_simple records the maximum, equal to the dimensions when none is given); reading one, H5S_extent_get_dims reports the current dimensions as the maximum and H5S_set_extent checks against none, so its own H5Dset_extent scrambles such a file the same way. The editor now records the maximum libhdf5 would have written (the dimensions the index was built with) before changing the current ones, moving the grown dataspace message in the header when it must. The writer records the maximum of every chunked dataset too, so h5py can resize what clawhdf5 writes (the pinned file hashes of three no-maxshape cases in plugin_filters_interop change by 8 bytes a dimension). Tests: edit_resize_interop.rs (a 2.7.0-written fixture, new FileBuilder files and h5py files through shrinks, zero extents and growth, against a model with our reader and h5py; h5py resizing a FileBuilder file), and in test_edit.py resizes checked against a numpy model, independently of h5py. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 32 ++ crates/clawhdf5-format/src/file_writer.rs | 16 + crates/clawhdf5-py/tests/test_edit.py | 76 +++++ crates/clawhdf5/src/edit/mod.rs | 40 ++- crates/clawhdf5/tests/edit_resize_interop.rs | 287 ++++++++++++++++++ .../fixtures/chunked_no_maxshape_v2_7_0.h5 | Bin 0 -> 5158 bytes .../clawhdf5/tests/plugin_filters_interop.rs | 11 +- docs/known-issues.md | 27 ++ 8 files changed, 482 insertions(+), 7 deletions(-) create mode 100644 crates/clawhdf5/tests/edit_resize_interop.rs create mode 100644 crates/clawhdf5/tests/fixtures/chunked_no_maxshape_v2_7_0.h5 diff --git a/CHANGELOG.md b/CHANGELOG.md index d37b2cf..b34d1db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,38 @@ ## Unreleased +### Correctness: resizing chunked datasets with no recorded maximum (2026-09-27) +- **`FileEditor::resize` scrambled the values of a chunked dataset whose + dataspace records no maximum dimensions when it shrank it** (fixed + 2026-09-27). The editor shipped on main in PR #18 (a4c2ace) and reached + Python as `Dataset.resize` in `'r+'` files; no release has it. clawhdf5's + writer stores such a dataspace for every chunked dataset created without + a `maxshape`, with a Fixed Array (or Single Chunk) chunk index. The + editor patched only the current dimensions, and with no maximum + recorded the maximum is the current dimensions — which is also what the + Fixed Array linearises chunks by — so a shrink moved every chunk after + the first row: h5py, h5dump and our reader all read wrong values + without complaint (20x20, chunks 6x6, resized to 15x15: row 6 read + `0 0 0 0 0 0 120 ...`). After a shrink the dataset could not grow back + either (`3 exceeds the maximum 0`). libhdf5 itself never writes such a + dataspace (`H5S_set_extent_simple` records the maximum, equal to the + dimensions when none is given); reading one, `H5S_extent_get_dims` + reports the current dimensions as the maximum and `H5S_set_extent` + checks against no maximum at all, so libhdf5's own `H5Dset_extent` + scrambles such a file the same way (and lets it grow past its Fixed + Array). The editor now records the maximum libhdf5 would have written + — the dimensions before the first resize, the ones the index was built + with — then changes the current ones (the dataspace message grows by one + length per dimension and moves in the header when it has to). The + dataset then shrinks, grows back to that extent and refuses more, as + one libhdf5 wrote would. The writer (`FileBuilder`) now records the + maximum of every chunked dataset too, as libhdf5 does, so h5py can + resize what it writes (8 more bytes per dimension). Tests: + `crates/clawhdf5/tests/edit_resize_interop.rs` (a 2.7.0-written fixture, + new `FileBuilder` files and h5py files through shrinks, zero extents + and growth, checked against a model with our reader and h5py; h5py + resizing a `FileBuilder` file) and `test_edit.py`'s numpy-model checks. + ### Python bindings: in-place editing (2026-09-27) - **`clawhdf5.File(path, 'r+')`** (and `'a'` on an existing file) opens a file for editing through `clawhdf5::FileEditor`, holding its exclusive diff --git a/crates/clawhdf5-format/src/file_writer.rs b/crates/clawhdf5-format/src/file_writer.rs index 1a1af83..c64e1cf 100644 --- a/crates/clawhdf5-format/src/file_writer.rs +++ b/crates/clawhdf5-format/src/file_writer.rs @@ -130,6 +130,22 @@ pub(crate) fn build_chunked_dataset_oh( fill_message: &[u8], refcount: u32, ) -> Result, FormatError> { + // libhdf5 records every simple dataspace's maximum dimensions (the + // dimensions themselves when none are given, `H5S_set_extent_simple`). + // Without them libhdf5 takes the maximum to be the current dimensions, + // so a resize by libhdf5 (h5py's `Dataset.resize`) would also change + // the maximum a Fixed Array chunk index is laid out by, and move every + // chunk already written. + let recorded; + let ds = if ds.space_type == DataspaceType::Simple && ds.max_dimensions.is_none() { + recorded = Dataspace { + max_dimensions: Some(ds.dimensions.clone()), + ..ds.clone() + }; + &recorded + } else { + ds + }; let mut w = ObjectHeaderWriter::new(); w.add_message_with_flags(MessageType::Datatype, dt.serialize(), 0x01); w.add_message(MessageType::Dataspace, ds.serialize(LENGTH_SIZE)); diff --git a/crates/clawhdf5-py/tests/test_edit.py b/crates/clawhdf5-py/tests/test_edit.py index 99090e2..a2fd83d 100644 --- a/crates/clawhdf5-py/tests/test_edit.py +++ b/crates/clawhdf5-py/tests/test_edit.py @@ -427,13 +427,89 @@ def test_random_edits_match_h5py(h5py, tmp_path, source, seed): key = _random_key(rng, shape) sel_shape, fancy = _selection_shape(key, shape) op = ("set", name, key, _random_value(rng, sel_shape, fancy, dtype)) + before = None + if op[0] == "resize": + with h5py.File(ours_path, "r", locking=False) as o: + before, fill = o[op[1]][()], o[op[1]].fillvalue if edit_both(h5py, theirs, ours_path, ours, op) is not None: refused += 1 + elif before is not None: + # Independently of h5py (which a wrong index layout fools + # the same way): the kept elements keep their values. + want = resized_model(before, op[2], fill) + np.testing.assert_array_equal(ours[op[1]][()], want, err_msg=repr(op)) + with h5py.File(ours_path, "r", locking=False) as o: + np.testing.assert_array_equal(o[op[1]][()], want, err_msg=repr(op)) assert_ours_reads_like_h5py(h5py, ours, ours_path, "end") assert refused < 30 h5dump_reads(ours_path, base) +def resized_model(before, shape, fill): + """`before` resized to `shape` as HDF5 resizes: elements inside both + extents keep their values, the others read as the fill value.""" + out = np.full(shape, fill, dtype=before.dtype) + common = tuple(slice(0, min(a, b)) for a, b in zip(before.shape, shape)) + out[common] = before[common] + return out + + +RESIZES = [(15, 15), (3, 2), (20, 20), (1, 1), (1, 0), (0, 0), (7, 20), (20, 13), (20, 20)] + + +def _check_resizes(h5py, path, name, orig, maxshape=(20, 20), base=None): + """Resize `name` through RESIZES in 'r+', each checked against a numpy + model with clawhdf5 and h5py and the file with `h5rs check`.""" + model = orig + with clawhdf5.File(path, "r+") as f: + ds = f[name] + for shape in RESIZES: + ds.resize(shape) + model = resized_model(model, shape, 0) + np.testing.assert_array_equal(ds[()], model, err_msg=f"{name} {shape}") + with h5py.File(path, "r", locking=False) as t: + np.testing.assert_array_equal(t[name][()], model, err_msg=f"h5py: {name} {shape}") + assert t[name].maxshape == maxshape + h5rs = os.environ.get("CLAWHDF5_H5RS") + if h5rs: # h5dump would wait for the editor's lock + r = subprocess.run([h5rs, "check", "--data", path], capture_output=True, text=True) + assert r.returncode == 0, f"{name} {shape}: " + (r.stdout + r.stderr)[-2000:] + with pytest.raises(ValueError): + ds.resize((maxshape[0] + 1, 20)) + # Written values survive a shrink. + ds[...] = orig + ds.resize((15, 15)) + with h5py.File(path, "r") as t: + np.testing.assert_array_equal(t[name][()], orig[:15, :15]) + h5dump_reads(path, base) + + +@pytest.mark.parametrize("source", SOURCES) +def test_resizes_keep_values(h5py, tmp_path, source): + """Shrinking, zero extents and growing back keep the values a numpy + model keeps, on every source (on clawhdf5's own files a shrink once + moved every chunk: h5py read the same wrong values).""" + _, ours, base = _make(h5py, tmp_path, source) + with clawhdf5.File(ours, "r+") as f: + f["chunk_gzip"].resize((20, 20)) + maxshape = (20, 20) if source == "clawhdf5" else (40, 40) + _check_resizes(h5py, ours, "chunk_gzip", np.arange(400, dtype="u2", "i8", "f8"] diff --git a/crates/clawhdf5/src/edit/mod.rs b/crates/clawhdf5/src/edit/mod.rs index a5a3c1b..feb1d2b 100644 --- a/crates/clawhdf5/src/edit/mod.rs +++ b/crates/clawhdf5/src/edit/mod.rs @@ -877,7 +877,7 @@ impl FileEditor { /// the fill value (`H5D__chunk_prune_by_extent`). pub fn resize(&mut self, path: &str, shape: &[u64]) -> Result<(), Error> { self.edit(|f, img| { - let t = Target::load(f, path)?; + let mut t = Target::load(f, path)?; let dims = t.dims().to_vec(); if shape.len() != dims.len() { return Err(Error::InvalidArgument(format!( @@ -889,7 +889,12 @@ impl FileEditor { if shape == dims.as_slice() { return Ok(()); } - let max = t.ds.max_dimensions.clone().unwrap_or_else(|| dims.clone()); + // No maximum recorded means the current dimensions (see below). + let record_max = t.ds.max_dimensions.is_none(); + let max = + t.ds.max_dimensions + .get_or_insert_with(|| dims.clone()) + .clone(); for d in 0..dims.len() { if shape[d] > max[d] { return Err(Error::InvalidArgument(format!( @@ -928,7 +933,36 @@ impl FileEditor { } put_uint(&mut dims_bytes[d * ls..], n, img.ls); } - hdr.patch(img, i, first, &dims_bytes)?; + if !record_max { + hdr.patch(img, i, first, &dims_bytes)?; + } else { + // No maximum recorded (clawhdf5's writer, for a dataset + // created without a maxshape). libhdf5 never writes such a + // dataspace: `H5S_set_extent_simple` records the maximum, + // equal to the dimensions when none is given. Reading one, + // libhdf5 takes the maximum to be the *current* dimensions + // (`H5S_extent_get_dims`), so changing them would also + // change the maximum the chunk index was built with — the + // Fixed Array linearises chunks by it — and move every + // existing chunk. Record the maximum libhdf5 would have + // written, the dimensions before this resize, so the index + // keeps its layout and the dataset can grow back to them. + let body_len = first + dims.len() * ls; + if body.len() < body_len || body[2] & !0x01 != 0 { + return Err(Error::Unsupported("dataspace message layout".into())); + } + let mut new_body = body[..first].to_vec(); + new_body[2] |= 0x01; + new_body.extend_from_slice(&dims_bytes); + let at = new_body.len(); + new_body.resize(at + dims.len() * ls, 0); + for (d, &n) in dims.iter().enumerate() { + put_uint(&mut new_body[at + d * ls..], n, img.ls); + } + let (flags, corder) = (hdr.msgs[i].flags, hdr.msgs[i].corder); + hdr.delete(img, i)?; + hdr.insert(img, MSG_DATASPACE, flags, &new_body, corder)?; + } let fill = fill_info(img, &hdr)?; hdr.finish(img)?; let expand = shape.iter().zip(&dims).any(|(n, o)| n > o); diff --git a/crates/clawhdf5/tests/edit_resize_interop.rs b/crates/clawhdf5/tests/edit_resize_interop.rs new file mode 100644 index 0000000..83b4dae --- /dev/null +++ b/crates/clawhdf5/tests/edit_resize_interop.rs @@ -0,0 +1,287 @@ +//! `FileEditor::resize` on chunked datasets whose dataspace records no +//! maximum dimensions, as clawhdf5's writer stored a dataset created without +//! a `maxshape` up to 2.7.0 (`fixtures/chunked_no_maxshape_v2_7_0.h5`). libhdf5 never writes such a dataspace (`H5S_set_extent_simple` +//! always records the maximum, equal to the dimensions when none is given), +//! and its Fixed Array chunk index linearises chunks by the maximum +//! dimensions. The editor therefore records the maximum libhdf5 would have +//! written (the dimensions the index was built with) before it changes the +//! current ones, so existing chunks stay where the index put them and the +//! dataset can grow back to its original extent. +//! +//! The writer now records the maximum too, so h5py can resize what it writes. +//! +//! Checked against a model of the expected values, with our reader and with +//! h5py (`CLAWHDF5_PYTHON`; skipped without it unless +//! `CLAWHDF5_REQUIRE_INTEROP=1`), on files clawhdf5 (old and new) and h5py +//! wrote. + +use std::path::{Path, PathBuf}; +use std::process::Command; + +use clawhdf5::{Error, File, FileBuilder, FileEditor}; + +fn python() -> String { + std::env::var("CLAWHDF5_PYTHON").unwrap_or_else(|_| "python3".to_string()) +} + +fn h5py_ok() -> bool { + let ok = Command::new(python()) + .args(["-c", "import h5py, numpy"]) + .output() + .is_ok_and(|o| o.status.success()); + if !ok { + assert!( + !std::env::var("CLAWHDF5_REQUIRE_INTEROP").is_ok_and(|v| v == "1"), + "CLAWHDF5_REQUIRE_INTEROP=1 but h5py/numpy is not available" + ); + eprintln!("SKIP (h5py part): h5py/numpy not available"); + } + ok +} + +fn py(script: &str) -> String { + let o = Command::new(python()) + .args(["-c", script]) + .output() + .expect("run python"); + assert!( + o.status.success(), + "python failed:\n{script}\nSTDERR: {}", + String::from_utf8_lossy(&o.stderr) + ); + String::from_utf8_lossy(&o.stdout).trim().to_string() +} + +/// Row-major values of a 2-D model after resizing `data` (shape `old`) to +/// `new`: kept elements keep their values, new ones are 0 (the fill value). +fn resized(data: &[f32], old: [u64; 2], new: [u64; 2]) -> Vec { + let mut out = vec![0f32; (new[0] * new[1]) as usize]; + for r in 0..old[0].min(new[0]) { + for c in 0..old[1].min(new[1]) { + out[(r * new[1] + c) as usize] = data[(r * old[1] + c) as usize]; + } + } + out +} + +/// Our reader and (when available) h5py read `expect` at `shape`. +fn check(path: &Path, name: &str, shape: [u64; 2], expect: &[f32], with_h5py: bool) { + let f = File::open(path).unwrap(); + let d = f.dataset(name).unwrap(); + assert_eq!(d.shape().unwrap(), shape); + assert_eq!( + d.read_f32().unwrap(), + expect, + "{name}: our reader at {shape:?}" + ); + if with_h5py { + let got = py(&format!( + "import h5py, numpy as np\n\ + with h5py.File({p:?}, 'r') as f:\n\ + \x20 d = f[{name:?}][()]\n\ + print(d.shape, ','.join(repr(float(x)) for x in d.ravel()))", + p = path.to_str().unwrap() + )); + let want = format!( + "({}, {}) {}", + shape[0], + shape[1], + expect + .iter() + .map(|x| format!("{:?}", f64::from(*x))) + .collect::>() + .join(",") + ); + assert_eq!(got, want.trim(), "{name}: h5py at {shape:?}"); + } +} + +/// Resize `name` (20 x 20, values 0..400) through a sequence of shrinks, +/// zero extents and growth back, checking every step. +fn run(path: &Path, name: &str, with_h5py: bool) { + let orig: Vec = (0..400).map(|i| i as f32).collect(); + let mut shape = [20u64, 20]; + let mut data = orig.clone(); + check(path, name, shape, &data, with_h5py); + for next in [ + [15, 15], + [3, 2], + [20, 20], + [1, 1], + [1, 0], + [0, 0], + [7, 20], + [20, 13], + [20, 20], + ] { + let mut ed = FileEditor::open(path).unwrap(); + ed.resize(name, &next).unwrap(); + drop(ed); + data = resized(&data, shape, next); + shape = next; + check(path, name, shape, &data, with_h5py); + } + // The maximum is the extent the dataset was created with. + let mut ed = FileEditor::open(path).unwrap(); + assert!(matches!( + ed.resize(name, &[21, 20]), + Err(Error::InvalidArgument(_)) + )); + drop(ed); + let f = File::open(path).unwrap(); + assert_eq!( + f.dataset(name).unwrap().max_dimensions().unwrap(), + Some(vec![20, 20]) + ); + drop(f); + // A shrink keeps the values it keeps. + let mut ed = FileEditor::open(path).unwrap(); + let vals: Vec = orig.iter().map(|v| v + 0.5).collect(); + ed.write_values(name, &clawhdf5::Selection::All, &vals) + .unwrap(); + ed.resize(name, &[15, 15]).unwrap(); + drop(ed); + check( + path, + name, + [15, 15], + &resized(&vals, [20, 20], [15, 15]), + with_h5py, + ); +} + +fn fixture(dir: &Path, name: &str) -> PathBuf { + let path = dir.join(name); + std::fs::copy( + Path::new(env!("CARGO_MANIFEST_DIR")) + .join("tests/fixtures") + .join(name), + &path, + ) + .unwrap(); + path +} + +/// Files clawhdf5 2.7.0 wrote: a Fixed Array index (Single Chunk for `s`) +/// and a dataspace with no maximum. Shrinking scrambled the values +/// (released in 2.7.0's `FileEditor`, PR #18). +#[test] +fn resize_without_stored_maxshape_keeps_values() { + let with_h5py = h5py_ok(); + for name in ["d", "z"] { + let dir = tempfile::tempdir().unwrap(); + let path = fixture(dir.path(), "chunked_no_maxshape_v2_7_0.h5"); + run(&path, name, with_h5py); + } + // A single-chunk dataset and an empty one keep their extents as maxima. + let dir = tempfile::tempdir().unwrap(); + let path = fixture(dir.path(), "chunked_no_maxshape_v2_7_0.h5"); + let mut ed = FileEditor::open(&path).unwrap(); + ed.resize("s", &[2, 4]).unwrap(); + ed.resize("s", &[3, 4]).unwrap(); + assert!(matches!( + ed.resize("s", &[4, 4]), + Err(Error::InvalidArgument(_)) + )); + assert!(matches!( + ed.resize("e", &[1, 5]), + Err(Error::InvalidArgument(_)) + )); + ed.resize("e", &[0, 3]).unwrap(); + drop(ed); + let f = File::open(&path).unwrap(); + let s = f.dataset("s").unwrap(); + assert_eq!(s.max_dimensions().unwrap(), Some(vec![3, 4])); + let mut want: Vec = (0..12).collect(); + want[8..].fill(0); + assert_eq!(s.read_i32().unwrap(), want); + assert_eq!( + f.dataset("e").unwrap().max_dimensions().unwrap(), + Some(vec![0, 5]) + ); +} + +fn written(path: &Path, deflate: bool) { + let data: Vec = (0..400).map(|i| i as f32).collect(); + let mut b = FileBuilder::new(); + let d = b + .create_dataset("d") + .with_f32_data(&data) + .with_shape(&[20, 20]) + .with_chunks(&[6, 6]); + if deflate { + d.with_deflate(4); + } + b.write(path).unwrap(); +} + +/// clawhdf5's writer now records the maximum, as libhdf5 does. +#[test] +fn resize_file_written_without_maxshape_keeps_values() { + let with_h5py = h5py_ok(); + for deflate in [false, true] { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cw.h5"); + written(&path, deflate); + let f = File::open(&path).unwrap(); + assert_eq!( + f.dataset("d").unwrap().max_dimensions().unwrap(), + Some(vec![20, 20]) + ); + drop(f); + run(&path, "d", with_h5py); + } +} + +/// h5py resizing a file clawhdf5 wrote without a maxshape keeps its values +/// (it scrambled them while the writer recorded no maximum). +#[test] +fn h5py_resizes_what_clawhdf5_writes() { + if !h5py_ok() { + return; + } + for deflate in [false, true] { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cw.h5"); + written(&path, deflate); + let out = py(&format!( + "import h5py, numpy as np\n\ + exp = np.arange(400, dtype='f4').reshape(20, 20)\n\ + with h5py.File({p:?}, 'r+') as f:\n\ + \x20 f['d'].resize((15, 15))\n\ + \x20 ok = np.array_equal(f['d'][()], exp[:15, :15])\n\ + \x20 f['d'].resize((20, 20))\n\ + \x20 back = f['d'][()]\n\ + want = np.zeros((20, 20), 'f4'); want[:15, :15] = exp[:15, :15]\n\ + print(ok and np.array_equal(back, want))", + p = path.to_str().unwrap() + )); + assert_eq!(out, "True"); + let mut want = vec![0f32; 400]; + for r in 0..15 { + for c in 0..15 { + want[r * 20 + c] = (r * 20 + c) as f32; + } + } + check(&path, "d", [20, 20], &want, false); + } +} + +/// h5py's files record the maximum; the same sequence must hold. +#[test] +fn resize_h5py_file_without_maxshape_keeps_values() { + if !h5py_ok() { + return; + } + for libver in ["earliest", "v110", "latest"] { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("hp.h5"); + py(&format!( + "import h5py, numpy as np\n\ + with h5py.File({p:?}, 'w', libver=({libver:?}, 'latest')) as f:\n\ + \x20 f.create_dataset('d', data=np.arange(400, dtype='f4').reshape(20, 20), chunks=(6, 6))", + p = path.to_str().unwrap() + )); + run(&path, "d", true); + } +} diff --git a/crates/clawhdf5/tests/fixtures/chunked_no_maxshape_v2_7_0.h5 b/crates/clawhdf5/tests/fixtures/chunked_no_maxshape_v2_7_0.h5 new file mode 100644 index 0000000000000000000000000000000000000000..7ecdfe788d952d0d0cf915bce5994955af017843 GIT binary patch literal 5158 zcmd7Wdsx&}9>?+T%y5_cCGmoi2_cHID<&u^#6*;UR$AVrz{FdES1L3TQ*<>&@s{AF z)R0w{O$|~o!Zai$GcnWjLGyxSF1focyHV}?H^1MY$n3KZ&+Z?d$Csbq`Tpj6=FA93 zhGj9vXeWn`4hB0rF^}K0;qR$dg0&k9zi=EdEXMe(UdHL&m74j1=5)|2RU1bUo#>J_ zC=-S@f95jHhv8HBluw!#pdNJs1Y^)3pO-}0544EA|P+j)r|HrZgrguQ3-9yS&lozfuZBuy> z>ebQUnN{i&8V!!3bL9;-zYg>M)t!l0YZv$JU$1BXB_8U{h>=5ks>?W(Y%1U=#TP?hN$Ts!S8AHej{vZR}6pD2MY4EheZ!lzI$KyTnc8#< z^;~LOEYv@xcFaQk7PZ?J>i2EqzfK!i;|~6YpU{YZ!7)n23C?hVE8O6VhtUm>pga5! zf_``m{Sk^V#Na7BjiHD|9L8ZhCSW2G@FLnen%L{F5PEgc!vJ@9z!P402<_pIM-hM? z2t-c|z(9m!5C&ri;xP=v@eH2Db9f1fNWvt%jLB&0u%gErHn4>q?BR_L=!j0}3?KAD z5PG8zg7G^2@3-vM7##*SqLT#pndRzCO!WyZVh1r;cxmXk>8IVj8vY3RdAR#MB$lD}jJqg)B zLN=0+Z6stn33-=<6p)ZVl8`+l+C1jbGaqSq6|doSyv2HuB_w1i30Xx#R+Ery60(Ve zyhB1ZlaTjF$PN;+lZ5OgA%!I5PbAtr(&<@%g?IxQtOLm+AP zgyfNsEhJ}MUw0TS{d2{}wcJ|-dMBp$d?%)THcB;+F!@(Bs4AR$LcJaFS6`+^)IA*CebQxbBN zgd8LBz>PBYg;wJ;>Uh9MbYzT?|8pH&wS9U!s}}*+A6^t4Y2@b$^Jk|foqDHKuV%VI z&2a6LMw6DM+6S3*8|}WWnoU}kY9BJw?X~+3+I=lc+o|((K0+!ZT>I>%b4rU(n~<2W z%{@6GC3#d)!qFu@w($Y^$#vcvJ#$8!2?+@rwLj^b**&u?lktdm=w*IB zS=?#0vX-geIkX`oGUAikij}r~1~(NIrKbGYFlEa3b@eyL?tA6PuAmEr;|`^MUp>gy z_Q}wc3Y|D7cga?o8ESuuT}(i7T|&a;S~pjFyJcBV#cT?mQnkOszInIK@B2&ncH323 z!e9Tf%FQviK`((TJH@HZ&HWpL4aTP{Gb&efb`6`{B`bWEqw(;ow%vSuB6qCy80;?j z%k8fgXQQ>1m*={ikt5<_Po4lbwK z`0-7?{O2C+zUXed_WBcEoRoN@HfMFtn)TT=U)FrpKX^{coLTcyo$Q_3t;#x4d7`TN z_*{R!OnN7{O}wYu{QlKmvB{ow$MD`#38hKLPw$-eb>Z0Jg=KyD z*t|B_?}T{)Q@?vi?b3b+=KkHu$;T%zZ~XYR{&8EvCspN!&Aw5+xp?|Fcdqq(e)_qh z)bDo1Rr9@dCEovdi++8GtAD3XkxBFWXXOs?GFESO>GndQRrcoonK6?mS9F)FB~Ihb z4fmz#`Sl^bk)0DhU%z*3@r(<_Gk^QlV4Jc#HCkVCMxH7U-g`+TRpYwOOEPR-nxuGtKI?SwGV(P&*|1s(so_eZ2+ROkJ~Hn}w=< zd=)7}olVtdJxsL%ZKkeL-(nW1y*l;YRBh&K>br5dz`=ZWn+p7OM*F=^WaEKu>e+4n rz0vZ4M`M*oDgU<0@vR))%5iNC<(O8EX60B`j%4K+){bDV?v8&0D@Fg( literal 0 HcmV?d00001 diff --git a/crates/clawhdf5/tests/plugin_filters_interop.rs b/crates/clawhdf5/tests/plugin_filters_interop.rs index f157ff5..9b41b2a 100644 --- a/crates/clawhdf5/tests/plugin_filters_interop.rs +++ b/crates/clawhdf5/tests/plugin_filters_interop.rs @@ -966,7 +966,10 @@ fn skipped_optional_filters_are_masked_as_libhdf5_masks_them() { /// Files whose chunks all compress are written exactly as before optional /// filters could be skipped: every mask is 0 and nothing else changed. The -/// hashes are of the files the writer produced before that change. +/// hashes are of the files the writer produced before that change, except +/// that a chunked dataset without a maxshape now records its maximum +/// dimensions (8 bytes per dimension; `lzf_fixed`, `lzf_single`, and +/// `blosc_fixed`). #[cfg(feature = "lzf")] #[test] fn files_whose_chunks_all_compress_are_unchanged() { @@ -985,7 +988,7 @@ fn files_whose_chunks_all_compress_are_unchanged() { .with_chunks(&[500]) .with_lzf(); }, - (3965, 449169442), + (3973, 452644487), ), ( "lzf_ea_noshuffle", @@ -1017,7 +1020,7 @@ fn files_whose_chunks_all_compress_are_unchanged() { .with_chunks(&[3000]) .with_lzf(); }, - (546, 690805477), + (554, 1394027497), ), ]; #[cfg(feature = "blosc")] @@ -1029,7 +1032,7 @@ fn files_whose_chunks_all_compress_are_unchanged() { .with_chunks(&[1024]) .with_blosc(BloscCodec::Lz4, 5, BloscShuffle::Byte); }, - (2776, 4278611376), + (2784, 1180133244), )); for (name, build, want) in &cases { let mut fb = clawhdf5::FileBuilder::new(); diff --git a/docs/known-issues.md b/docs/known-issues.md index e33b80c..690bb04 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -7,6 +7,33 @@ deleting it. --- +## Shrinking a chunked dataset with no recorded maximum scrambled it + +**Status:** fixed 2026-09-27, before any release (`FileEditor::resize` +shipped on main in PR #18, a4c2ace). Files the writer produced before the +fix still lack the maximum; see *Existing files*. + +clawhdf5's writer stored no maximum dimensions for a chunked dataset +created without a `maxshape` (libhdf5 always stores one, equal to the +dimensions when none is given). With none recorded, the maximum is the +current dimensions (`H5S_extent_get_dims`), and a Fixed Array chunk index +places chunks by the maximum. `FileEditor::resize` changed only the +current dimensions, so a shrink moved every existing chunk and every +reader returned wrong values; after a shrink the dataset could not grow +back. Found by the review of the Python editing work. + +**Fix:** before the first resize of such a dataset the editor records the +maximum libhdf5 would have written (the dimensions the index was built +with); the writer now records it for every chunked dataset. **Test:** +`crates/clawhdf5/tests/edit_resize_interop.rs`. **Existing files:** +datasets written before the fix have no recorded maximum. The fixed +editor handles them; **libhdf5 (h5py `Dataset.resize`, `H5Dset_extent`) +does not** — it scrambles them the same way, and lets them grow past +their Fixed Array. Resize them once with a fixed `FileEditor` (a resize +to the same shape changes nothing; shrink and grow back) before letting +libhdf5 resize them. A dataset already shrunk by the unfixed +editor holds misplaced chunks; rewrite it from a good copy. + ## Fletcher-32 checksums disagreed with libhdf5 on about 1 chunk in 32768 **Status:** fixed 2026-09-26, after v2.7.0. **Every release (v2.1.0 to From bdf584abb2c3307382f0550b0a6985bcb3c0a3ba Mon Sep 17 00:00:00 2001 From: osobh Date: Sun, 27 Sep 2026 07:40:11 -0500 Subject: [PATCH 4/9] format: chunk grids with a zero extent have no chunks (no division by zero) A Fixed or Extensible Array index whose maximum extent (the current one when no maximum is recorded) is 0 along a dimension has a zero stride for every dimension before it; ChunkGrid::offsets divided by it. The unfixed editor made such files by resizing a clawhdf5-written dataset to a zero extent: `h5rs check` panicked and the next resize raised an internal error (12 of the reviewer's random-edit seeds 10..39). Such an index has no slot for any chunk of the dataset; offsets now returns None. Tests: chunk_grid::zero_extent_has_no_chunks; edit_interop's zero_extent_resizes_without_a_recorded_maximum on a file the unfixed editor left (fixture) and on a 2.7.0-written file taken through zero extents with `h5rs check --data` and h5dump at every step; test_edit.py random edits on seeds 10..39 of a clawhdf5-written file. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 16 +++++ crates/clawhdf5-format/src/chunk_grid.rs | 28 ++++++++ crates/clawhdf5-py/tests/test_edit.py | 7 ++ crates/clawhdf5-tools/tests/edit_interop.rs | 65 ++++++++++++++++++ .../fixtures/chunk_zero_extent_no_maxshape.h5 | Bin 0 -> 5158 bytes 5 files changed, 116 insertions(+) create mode 100644 crates/clawhdf5/tests/fixtures/chunk_zero_extent_no_maxshape.h5 diff --git a/CHANGELOG.md b/CHANGELOG.md index b34d1db..5c0e719 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,22 @@ ## Unreleased +### Correctness: zero extents in Fixed/Extensible Array chunk indexes (2026-09-27) +- **A chunked dataset whose maximum (or, with none recorded, current) + extent is 0 along a dimension made the reader divide by zero** (fixed + 2026-09-27): `h5rs check` panicked ("attempt to divide by zero", + `chunk_grid.rs`) and the next `FileEditor::resize` failed with an + internal error. The unfixed editor produced such files by resizing a + clawhdf5-written dataset to a zero extent (12 of 30 extra random-edit + seeds on clawhdf5-written files). Such an index has no slot for any + chunk of the dataset, and `ChunkGrid::offsets` now says so instead of + dividing by the zero stride. Tests: `chunk_grid`'s + `zero_extent_has_no_chunks`, `edit_interop.rs`'s + `zero_extent_resizes_without_a_recorded_maximum` (a file the unfixed + editor left checks clean and resizes on; a 2.7.0-written file through + zero extents checks clean at each step), and `test_edit.py`'s random + edits on seeds 10 to 39 of a clawhdf5-written file. + ### Correctness: resizing chunked datasets with no recorded maximum (2026-09-27) - **`FileEditor::resize` scrambled the values of a chunked dataset whose dataspace records no maximum dimensions when it shrank it** (fixed diff --git a/crates/clawhdf5-format/src/chunk_grid.rs b/crates/clawhdf5-format/src/chunk_grid.rs index 9e03b9d..8957512 100644 --- a/crates/clawhdf5-format/src/chunk_grid.rs +++ b/crates/clawhdf5-format/src/chunk_grid.rs @@ -135,6 +135,12 @@ impl ChunkGrid { let mut rem = index; for p in 0..rank { let d = self.order[p]; + // A zero stride: a later dimension has no chunks (its maximum, + // or with none recorded its current extent, is 0), so no slot of + // the index is a chunk of the dataset. + if self.down[p] == 0 { + return None; + } let scaled = rem / self.down[p]; rem %= self.down[p]; if scaled >= self.cur_chunks[d] { @@ -193,6 +199,28 @@ mod tests { assert_eq!(g.offsets(11), Some(vec![2, 3])); } + #[test] + fn zero_extent_has_no_chunks() { + // No maximum recorded and a zero current dimension: every stride + // before it is 0 (this divided by zero). + let g = ChunkGrid::fixed_array(&[1, 0], None, &[6, 6]).unwrap(); + for i in 0..16 { + assert_eq!(g.offsets(i), None); + } + let g = ChunkGrid::fixed_array(&[0, 0, 3], Some(&[4, 0, 3]), &[2, 2, 3]).unwrap(); + for i in 0..16 { + assert_eq!(g.offsets(i), None); + } + let g = ChunkGrid::extensible_array(&[0, 5], Some(&[u64::MAX, 0]), &[2, 2]).unwrap(); + for i in 0..16 { + assert_eq!(g.offsets(i), None); + } + // A zero last dimension leaves the other strides alone. + let g = ChunkGrid::fixed_array(&[4, 0], Some(&[4, 6]), &[2, 3]).unwrap(); + assert_eq!(g.offsets(0), None); + assert_eq!(g.linear_index(&[1, 1]), 3); + } + #[test] fn rejects_two_unlimited_dims_after_the_first() { assert!(ChunkGrid::fixed_array(&[4, 6], Some(&[u64::MAX, u64::MAX]), &[2, 3]).is_err()); diff --git a/crates/clawhdf5-py/tests/test_edit.py b/crates/clawhdf5-py/tests/test_edit.py index a2fd83d..690bd0f 100644 --- a/crates/clawhdf5-py/tests/test_edit.py +++ b/crates/clawhdf5-py/tests/test_edit.py @@ -445,6 +445,13 @@ def test_random_edits_match_h5py(h5py, tmp_path, source, seed): h5dump_reads(ours_path, base) +@pytest.mark.parametrize("seed", range(10, 40)) +def test_random_edits_on_clawhdf5_files(h5py, tmp_path, seed): + """More random sequences on a clawhdf5-written file, whose resizes to + zero extents once left files `h5rs check` could not read.""" + test_random_edits_match_h5py(h5py, tmp_path, "clawhdf5", seed) + + def resized_model(before, shape, fill): """`before` resized to `shape` as HDF5 resizes: elements inside both extents keep their values, the others read as the fill value.""" diff --git a/crates/clawhdf5-tools/tests/edit_interop.rs b/crates/clawhdf5-tools/tests/edit_interop.rs index 3a37e83..fad815c 100644 --- a/crates/clawhdf5-tools/tests/edit_interop.rs +++ b/crates/clawhdf5-tools/tests/edit_interop.rs @@ -1509,3 +1509,68 @@ fn unencodable_filters_are_unsupported() { "a refused edit changed the file" ); } + +fn fixture(dir: &Path, name: &str) -> std::path::PathBuf { + let path = dir.join(name); + std::fs::copy( + Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../clawhdf5/tests/fixtures") + .join(name), + &path, + ) + .unwrap(); + path +} + +/// Zero extents on chunked datasets with no recorded maximum. The unfixed +/// editor left `chunk_zero_extent_no_maxshape.h5` (a 2.7.0-written file +/// resized to 1x0): a Fixed Array whose maximum, taken from the current +/// dimensions, has no chunks along one dimension, so every stride before it +/// is 0 — `h5rs check` panicked dividing by it and the next resize failed +/// with an internal error. Such a file must check clean and resize on; a +/// 2.7.0-written file taken through zero extents by the fixed editor must +/// check clean at every step and read the fill value where it grew. +#[test] +fn zero_extent_resizes_without_a_recorded_maximum() { + if !tools_ok() { + return; + } + let dir = tmpdir(); + let path = fixture(dir.path(), "chunk_zero_extent_no_maxshape.h5"); + check_tools(&path, true); + let mut ed = FileEditor::open(&path).unwrap(); + ed.resize("d", &[0, 0]).unwrap(); + ed.resize("z", &[0, 0]).unwrap(); + // Their maximum is now what the index was laid out by (1 x 0). + ed.resize("d", &[1, 0]).unwrap(); + assert!(matches!( + ed.resize("d", &[1, 1]), + Err(Error::InvalidArgument(_)) + )); + drop(ed); + check_tools(&path, true); + + let path = fixture(dir.path(), "chunked_no_maxshape_v2_7_0.h5"); + for shape in [[15, 15], [3, 2], [1, 1], [1, 0], [0, 0], [0, 20], [20, 20]] { + let mut ed = FileEditor::open(&path).unwrap(); + ed.resize("d", &shape).unwrap(); + ed.resize("z", &shape).unwrap(); + drop(ed); + check_tools(&path, true); + } + let f = File::open(&path).unwrap(); + for name in ["d", "z"] { + let d = f.dataset(name).unwrap(); + assert_eq!(d.shape().unwrap(), [20, 20]); + assert!(d.read_f32().unwrap().iter().all(|&v| v == 0.0), "{name}"); + } + assert_eq!( + py(&format!( + "import h5py\n\ + with h5py.File({:?}) as f:\n\ + \x20 print(int(abs(f['d'][()]).sum() + abs(f['z'][()]).sum()), f['d'].maxshape)", + path.to_str().unwrap() + )), + "0 (20, 20)" + ); +} diff --git a/crates/clawhdf5/tests/fixtures/chunk_zero_extent_no_maxshape.h5 b/crates/clawhdf5/tests/fixtures/chunk_zero_extent_no_maxshape.h5 new file mode 100644 index 0000000000000000000000000000000000000000..fa1e0efe7d8dee313fb482a5db333a66a857c9b2 GIT binary patch literal 5158 zcmdtm2~ZPf6bJB^i#yyX9%w{CJW!@$2?|Oi#i#_Wg2e-k0)jW7BA!+8K&t|R*9aan zYPF*bEhwmC!UC*Csg#5Pn>(e};!=I!^g@7tF+tjR7{DgC4- z%}qo`M#RNmY&hF86*u;U`De{~3{)ux3u&a#T36#v5YE*R!89 zOKu%LTEG<8*QaH$>vr3l&0sF$FR~?pm8><1YtEEWQzL5nEsh14OeiD)+re&3BtcDN zVuazuLJ|oK48$UnH&W(hk4N(*%(mnHjctFZ`2>heo9IT-y>UU!n7WaeqqTX#JDCeA zMP?+h#0Sj14{m;La0z?Bn_noz{JA#Fi#|aEIx*Y%{?m3Mb{pn0wI7ES`*DcGV!;D; zH?M-;TiLV!!>PcBNDHJOvIH5p!8y1JTBw97s0Mi-A}{a(C8)q37Q<3VfEBP3R>L01 zfK2!avSB}5g3E9fuER~JfX0RoW*P(?2Voru!Mu7B)@vY@SI@=zAms7t zTCDHDJzl-u3?Hya!rUA9v!l-*`?l1xcO+_)^wiT(+pMR49JL}n^{1#k*Hiz1k?WmO zg*{%vukZ@0;ZHF0C6aPApf?1-Ko|sH z!(bQ!V<8m6ARNX)1T;3p=#hW{uh$;yX3(5h?~Ju8bm7%~uvS1HUOf=&AQ;N4M`Ar5 zCh+Qwo!*Uhd987K)EC>a?( zSXI9)1dy65laREZI5j&6xw22W=MlDU|qL!TomPe&Bs< z&SQUM&NXJP^r8DrZW3D-IyyKgwx`(ivZh@tjPKXB)H`72udaG@DyQ|4xVS&6C1N|$?t8#?yx9UJTPf`VpU@eSvDyM*^!u>HRzE=*oNOcksQ^dG4X@f|YYM(OpU%Via3 zd&}Hh^;+2dM zIrHleCPI=5({2%G7Vkk-RaS+>Q(@tX{B@+OaXRYXK#JGz%E z4dEJix#s#pcbTH4=G3~qBXTC+$eH@rJCR}3?r1+@?j@q~MDts`p1R9A{jXm+^o)J( z6<2j`+p%TQTz|zQ3qjom-f^A&D;c*A{r=Q5=3eFb(37>-uk^KW;`P*Ba{bBjf{J|E z%j)Qx9qsMY(yiKcAsQ}~*wsB(-*L^|N?=}lw<=;Ez*KBE*j;nC5^#@XxVs9o%l~in z;Kv)MxO+GENnK)}>nN)CIWqTd?zz#5o+tcuXX*ZJ@pnlg&^;j1J##VuL(tupb+=>_ IFa>`64FI8F{Qv*} literal 0 HcmV?d00001 From 39f25e5d4ee69298448406e58a0b704bf07a0133 Mon Sep 17 00:00:00 2001 From: osobh Date: Sun, 27 Sep 2026 07:47:03 -0500 Subject: [PATCH 5/9] edit: plan every edit from the file the editor holds, not its path FileEditor re-opened its path to plan each edit but wrote through the file it held open, and the Python 'r+' handle re-opened the path after every edit to read. When the path came to name another file between edits (a rename or replacement, or a relative path after os.chdir), an edit was laid out from the other file's metadata and written into the held one, corrupting it, and later reads came from the other file (the review's repro: h5py then reports "invalid dataset size, likely file corruption"). The editor now plans from a mapping of its own file (a clone of the held descriptor, dropped before the edit writes) and canonicalises its path at open. New FileEditor::reader() opens the held file anew for reading, without sharing the editor's flock (a mapping of a cloned descriptor holds the lock until unmapped): through /proc/self/fd on Linux, which follows a renamed file; elsewhere by path, refused on Unix when the path no longer names the held file. The Python handle reads through it and keeps no path; a 'w' file is written at the absolute path it was opened with. Tests: edit_tests.rs edits_go_to_the_file_held_not_the_path; test_edit.py test_relative_path_and_chdir and test_path_replaced_between_edits. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 20 ++++++++ crates/clawhdf5-io/src/mmap.rs | 17 +++++++ crates/clawhdf5-py/src/file.rs | 4 +- crates/clawhdf5-py/src/handle.rs | 38 +++++++------- crates/clawhdf5-py/tests/test_edit.py | 73 +++++++++++++++++++++++++++ crates/clawhdf5/src/edit/mod.rs | 73 ++++++++++++++++++++++++--- crates/clawhdf5/src/reader.rs | 32 ++++++++++++ crates/clawhdf5/tests/edit_tests.rs | 56 ++++++++++++++++++++ docs/known-issues.md | 6 +++ 9 files changed, 292 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c0e719..98046ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,26 @@ ## Unreleased +### Correctness: edits planned from another file after a rename or `chdir` (2026-09-27) +- **`FileEditor` planned each edit by re-opening its path but wrote + through the file it held open** (fixed 2026-09-27; on main since PR #18, + no release). When the path came to name another file between edits — a + rename or replacement, or, for a relative path, a change of working + directory — an edit was laid out from the other file's metadata and + written into the held one, corrupting it (h5py: "invalid dataset size, + likely file corruption"). The Python `'r+'` handle had the same flaw in + its reads: it reopened the path after every edit, so reads came from the + other file. The editor now plans every edit from the file it holds, and + its path is canonicalised at open. New `FileEditor::reader()` opens the + held file anew for reading (on Linux through `/proc/self/fd`, so it + follows a renamed file; elsewhere by the path, refused when the path no + longer names the held file), without sharing the editor's lock; the + Python handle reads through it, and a `'w'` file is written at the + absolute path it was opened with. Tests: `edit_tests.rs`'s + `edits_go_to_the_file_held_not_the_path`; `test_edit.py`'s + `test_relative_path_and_chdir` and `test_path_replaced_between_edits` + (the review's repro). + ### Correctness: zero extents in Fixed/Extensible Array chunk indexes (2026-09-27) - **A chunked dataset whose maximum (or, with none recorded, current) extent is 0 along a dimension made the reader divide by zero** (fixed diff --git a/crates/clawhdf5-io/src/mmap.rs b/crates/clawhdf5-io/src/mmap.rs index 756d5e3..113693a 100644 --- a/crates/clawhdf5-io/src/mmap.rs +++ b/crates/clawhdf5-io/src/mmap.rs @@ -39,6 +39,23 @@ impl MmapReader { Ok(Self { _file: file, mmap }) } + /// Memory-map a file that is already open (for reading). + /// + /// The mapping references `file`'s open file description for as long as + /// it lives, so a `flock` taken through that description (or a + /// `try_clone` of it) is held until the reader is dropped. + /// + /// # Safety + /// + /// The same contract as [`open`](Self::open): the file must not be + /// modified while the mapping is active. + pub fn from_file(file: fs::File) -> io::Result { + // SAFETY: a read-only mapping; the caller keeps the file unmodified + // while it is alive. + let mmap = unsafe { Mmap::map(&file)? }; + Ok(Self { _file: file, mmap }) + } + /// Zero-copy access to the entire file contents. pub fn as_bytes(&self) -> &[u8] { &self.mmap diff --git a/crates/clawhdf5-py/src/file.rs b/crates/clawhdf5-py/src/file.rs index 2764e61..bfb6d57 100644 --- a/crates/clawhdf5-py/src/file.rs +++ b/crates/clawhdf5-py/src/file.rs @@ -110,7 +110,9 @@ impl PyFile { "w" => Ok(Self { filename, inner: Some(FileInner::Write(WriteState { - path: PathBuf::from(path), + // Absolute now: the file is written at close, possibly + // after the working directory changed. + path: std::path::absolute(path).unwrap_or_else(|_| PathBuf::from(path)), root_datasets: Vec::new(), root_attrs: Arc::new(Mutex::new(Vec::new())), groups: Vec::new(), diff --git a/crates/clawhdf5-py/src/handle.rs b/crates/clawhdf5-py/src/handle.rs index 754de6f..4bb8364 100644 --- a/crates/clawhdf5-py/src/handle.rs +++ b/crates/clawhdf5-py/src/handle.rs @@ -8,7 +8,8 @@ //! //! A file opened with `'r+'` also holds a [`FileEditor`]. An edit takes the //! file's write lock, so no read runs while the file changes underneath it, -//! and reopens the file afterwards: reads after an edit see the new bytes +//! and reopens the file afterwards, through the editor's own open file +//! rather than its path: reads after an edit see the new bytes //! (a grown file, a new dataspace), never a stale mapping or chunk cache. //! Objects that cache something an edit can change compare //! [`Handle::generation`] with the value they cached it at. @@ -16,7 +17,6 @@ //! Lock discipline (no deadlock with the GIL): the file lock is only taken //! with the GIL released, and code that holds it never touches Python. -use std::path::PathBuf; use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Arc, Mutex, PoisonError, RwLock}; @@ -28,8 +28,9 @@ use crate::{panic_text, to_py_err}; /// Where the file's bytes come from. pub(crate) enum Source { - /// A local path (memory-mapped). - Local(PathBuf), + /// A local file (memory-mapped). Its path is not kept: nothing reopens + /// it by path (see `open_editable`). + Local, /// A URL, read through `clawhdf5-remote`'s block cache. Remote { url: String, @@ -74,25 +75,23 @@ impl Handle { /// A local file, read-only. pub(crate) fn open_local(py: Python<'_>, path: &str) -> PyResult> { let file = py.detach(|| crate::no_panic(|| File::open(path).map_err(to_py_err)))?; - Ok(Self::new(file, Source::Local(PathBuf::from(path)), None)) + Ok(Self::new(file, Source::Local, None)) } /// A local file, open for in-place editing (`'r+'`): the editor takes /// the file's exclusive lock and checks that it can edit the file, then - /// the file is opened for reading. + /// the file is read through the editor's own file — never by path + /// again, so a later `os.chdir` or a rename or replacement of the path + /// cannot make reads (or the editor's plans) come from another file. pub(crate) fn open_editable(py: Python<'_>, path: &str) -> PyResult> { let (file, editor) = py.detach(|| { crate::no_panic(|| { let editor = FileEditor::open(path).map_err(to_py_err)?; - let file = File::open(path).map_err(to_py_err)?; + let file = editor.reader().map_err(to_py_err)?; Ok((file, editor)) }) })?; - Ok(Self::new( - file, - Source::Local(PathBuf::from(path)), - Some(editor), - )) + Ok(Self::new(file, Source::Local, Some(editor))) } /// A remote file (`http(s)://`, `s3://`, ...). @@ -153,7 +152,7 @@ impl Handle { pub(crate) fn remote_storage(&self) -> Option<&clawhdf5_remote::RemoteStorage> { match &self.source { Source::Remote { storage, .. } => Some(storage), - Source::Local(_) => None, + Source::Local => None, } } @@ -161,7 +160,7 @@ impl Handle { pub(crate) fn redacted_url(&self) -> Option { match &self.source { Source::Remote { url, .. } => Some(clawhdf5_remote::redact_url(url)), - Source::Local(_) => None, + Source::Local => None, } } @@ -185,14 +184,12 @@ impl Handle { let Some(editor) = &self.editor else { return Err(PyOSError::new_err(match self.source { Source::Remote { .. } => "remote files are read-only", - Source::Local(_) => { - "the file is open read-only; open it with mode 'r+' to change it" - } + Source::Local => "the file is open read-only; open it with mode 'r+' to change it", })); }; - let Source::Local(path) = &self.source else { + if matches!(self.source, Source::Remote { .. }) { return Err(PyOSError::new_err("remote files are read-only")); - }; + } py.detach(|| { let mut ed = editor.lock().unwrap_or_else(PoisonError::into_inner); let ed = ed @@ -202,7 +199,8 @@ impl Handle { let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| f(ed))); // Drop the old mapping and its chunk cache before reopening. *file = None; - let reopened = std::panic::catch_unwind(|| File::open(path)); + // Through the editor's file, not the path (see `open_editable`). + let reopened = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| ed.reader())); self.generation.fetch_add(1, Ordering::AcqRel); match reopened { Ok(Ok(f)) => *file = Some(f), diff --git a/crates/clawhdf5-py/tests/test_edit.py b/crates/clawhdf5-py/tests/test_edit.py index 690bd0f..c0b7ba6 100644 --- a/crates/clawhdf5-py/tests/test_edit.py +++ b/crates/clawhdf5-py/tests/test_edit.py @@ -823,3 +823,76 @@ def test_close_releases_the_file(h5py, tmp_path): ds[0] = 1 with h5py.File(path, "r+") as g: g["d"][0] = 9 + + +def _two_files(h5py, tmp_path): + """d1/f.h5 and d2/f.h5: same name, different layouts (the review's + repro).""" + (tmp_path / "d1").mkdir() + (tmp_path / "d2").mkdir() + with h5py.File(tmp_path / "d1" / "f.h5", "w") as f: + f.create_dataset("x", data=np.arange(10, dtype=">(path: P) -> Result { - let path = path.as_ref().to_path_buf(); - let file = OpenOptions::new().read(true).write(true).open(&path)?; + let file = OpenOptions::new() + .read(true) + .write(true) + .open(path.as_ref())?; + let path = std::fs::canonicalize(path.as_ref())?; match file.try_lock() { Ok(()) => {} Err(TryLockError::WouldBlock) => { @@ -768,16 +777,68 @@ impl FileEditor { file, free: FreeList::default(), }; - let f = File::open(&ed.path)?; + let f = ed.plan_reader()?; check_editable(&f)?; Ok(ed) } - /// The file's path. + /// The file's absolute path when it was opened (it may have been + /// renamed since; the editor keeps editing the file it opened). pub fn path(&self) -> &Path { &self.path } + /// A reader over the file this editor holds, as last written: the file + /// opened by [`open`](Self::open), not whatever its path names now. + /// + /// It opens the file anew (read-only), so it does not share the + /// editor's lock and stays usable after the editor is dropped: on Linux + /// through `/proc/self/fd`, which reaches the held file even after its + /// path was renamed or replaced; elsewhere by the path the file had at + /// open, refused with [`Error::Io`] when that path no longer names the + /// held file (on Unix, compared by device and inode; Windows cannot + /// check). With the `mmap` feature the reader maps the file: edits + /// through the editor change the bytes it sees, so take a new reader + /// after each edit rather than reading through an old one while an edit + /// runs. + pub fn reader(&self) -> Result { + let dir = self.path.parent().map(Path::to_path_buf); + File::from_std_file(self.reopen()?, dir) + } + + /// A new read-only open file description of the held file (see + /// [`reader`](Self::reader)). + fn reopen(&self) -> Result { + #[cfg(target_os = "linux")] + { + use std::os::fd::AsRawFd; + let proc = format!("/proc/self/fd/{}", self.file.as_raw_fd()); + if let Ok(f) = std::fs::File::open(proc) { + return Ok(f); + } + } + let f = std::fs::File::open(&self.path)?; + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + let (a, b) = (self.file.metadata()?, f.metadata()?); + if (a.dev(), a.ino()) != (b.dev(), b.ino()) { + return Err(Error::Io(std::io::Error::other(format!( + "{} no longer names the file being edited (renamed or replaced)", + self.path.display() + )))); + } + } + Ok(f) + } + + /// A reader over the held file itself for planning an edit (it shares + /// the file's lock, and is dropped before the edit writes). + fn plan_reader(&self) -> Result { + let dir = self.path.parent().map(Path::to_path_buf); + File::from_std_file(self.file.try_clone()?, dir) + } + /// Bytes earlier edits of this editor freed that later ones can still /// reuse. pub fn reusable_bytes(&self) -> u64 { @@ -794,7 +855,7 @@ impl FileEditor { &mut self, op: impl FnOnce(&File, &mut Image<'_>) -> Result, ) -> Result { - let f = File::open(&self.path)?; + let f = self.plan_reader()?; check_editable(&f)?; let sb = f.superblock().clone(); let user_block = f.user_block_size(); diff --git a/crates/clawhdf5/src/reader.rs b/crates/clawhdf5/src/reader.rs index 1786f17..d362383 100644 --- a/crates/clawhdf5/src/reader.rs +++ b/crates/clawhdf5/src/reader.rs @@ -405,6 +405,38 @@ impl File { } } + /// A reader over an already open file (the file itself, not whatever + /// its path names now): mapped with the `mmap` feature, else read into + /// memory. `base_dir` resolves external Virtual Dataset sources. + pub(crate) fn from_std_file( + file: std::fs::File, + base_dir: Option, + ) -> Result { + #[cfg(feature = "mmap")] + let mut f = { + let reader = clawhdf5_io::MmapReader::from_file(file).map_err(Error::Io)?; + let (data, superblock) = FileData::new(Backing::Mmap(reader))?; + Self { + data, + superblock, + chunk_cache: ChunkCache::new(), + base_dir: None, + vds_resolver: None, + } + }; + #[cfg(not(feature = "mmap"))] + let mut f = { + use std::io::{Read, Seek, SeekFrom}; + let mut file = file; + let mut bytes = Vec::new(); + file.seek(SeekFrom::Start(0)).map_err(Error::Io)?; + file.read_to_end(&mut bytes).map_err(Error::Io)?; + Self::from_bytes(bytes)? + }; + f.base_dir = base_dir; + Ok(f) + } + /// Open an HDF5 file by reading it entirely into memory. /// /// This is the pre-mmap behaviour and is useful when memory-mapping is diff --git a/crates/clawhdf5/tests/edit_tests.rs b/crates/clawhdf5/tests/edit_tests.rs index 9190a2c..1f83d74 100644 --- a/crates/clawhdf5/tests/edit_tests.rs +++ b/crates/clawhdf5/tests/edit_tests.rs @@ -162,3 +162,59 @@ fn shrink_then_grow_reads_fill() { raw[..4].fill(0.5); assert_eq!(f.dataset("raw").unwrap().read_f64().unwrap(), raw); } + +/// The editor plans every edit from the file it holds open, never by +/// re-opening its path: with the path renamed away and another file put in +/// its place, edits go to the held file, planned from its own metadata, +/// and the file now at the path is untouched (planning from it and writing +/// into the held file corrupted the held one). +#[test] +fn edits_go_to_the_file_held_not_the_path() { + let dir = tempfile::tempdir().unwrap(); + let a = dir.path().join("a.h5"); + let b = dir.path().join("b.h5"); + let mut fb = FileBuilder::new(); + fb.create_dataset("x") + .with_i32_data(&[0; 10]) + .with_shape(&[10]); + fb.create_dataset("big") + .with_f64_data(&[1.5; 5000]) + .with_shape(&[5000]); + fb.set_attr("title", AttrValue::String("a".into())); + fb.write(&a).unwrap(); + let mut fb = FileBuilder::new(); + fb.create_dataset("pad") + .with_f64_data(&[2.5; 3000]) + .with_shape(&[3000]); + fb.create_dataset("x") + .with_i32_data(&[500; 10]) + .with_shape(&[10]); + fb.write(&b).unwrap(); + + let mut ed = FileEditor::open(&a).unwrap(); + assert!(ed.path().is_absolute()); + let moved = dir.path().join("moved.h5"); + std::fs::rename(&a, &moved).unwrap(); + std::fs::rename(&b, &a).unwrap(); + let other = std::fs::read(&a).unwrap(); + + ed.write_values("x", &Selection::All, &[7i32; 10]).unwrap(); + let vals: Vec = (0..50).map(f64::from).collect(); + ed.set_attr("/", "note", &AttrValue::F64Array(vals.clone())) + .unwrap(); + // The editor's own reader sees the held file. + let r = ed.reader().unwrap(); + assert_eq!(r.dataset("x").unwrap().read_i32().unwrap(), [7; 10]); + assert!(r.dataset("pad").is_err()); + drop(r); + drop(ed); + + assert!( + std::fs::read(&a).unwrap() == other, + "the file at the path changed" + ); + let f = File::open(&moved).unwrap(); + assert_eq!(f.dataset("x").unwrap().read_i32().unwrap(), [7; 10]); + assert_eq!(f.dataset("big").unwrap().read_f64().unwrap(), [1.5; 5000]); + assert!(matches!(f.root().attr("note").unwrap(), Some(AttrValue::F64Array(v)) if v == vals)); +} diff --git a/docs/known-issues.md b/docs/known-issues.md index 690bb04..871729d 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -143,6 +143,12 @@ space it leaves is too small for its next, larger version. **No journal.** A crash while an edit patches existing structures can leave the file inconsistent; see the `FileEditor` documentation. +**Renamed files outside Linux.** Edits always go to the file the editor +opened. `FileEditor::reader()` (which the Python `'r+'` handle reads +through) reopens it through `/proc/self/fd` on Linux; elsewhere it reopens +the path, and after the path was renamed or replaced it fails (on Unix; +Windows cannot tell and would read whatever the path names). + ## Python in-place editing (`clawhdf5.File(path, 'r+')`) limits **Status:** open (added 2026-09-27). The Python bindings edit through From 173de3e0d20407a358e9af00140f0744f0e68e57 Mon Sep 17 00:00:00 2001 From: osobh Date: Sun, 27 Sep 2026 07:48:04 -0500 Subject: [PATCH 6/9] py: test that an edit releases the GIL A thread appending timestamps in a loop runs while a 1024x1024 gzip dataset is rewritten through 'r+': the largest gap between its stamps during the edit must be under half the edit's duration (an edit holding the GIL stalls it for the whole edit; checked with a GIL-holding regex standing in for the edit: one 0.20 s gap in a 0.21 s call). Edits already ran detached; nothing tested it. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/clawhdf5-py/tests/test_edit.py | 35 +++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/crates/clawhdf5-py/tests/test_edit.py b/crates/clawhdf5-py/tests/test_edit.py index c0b7ba6..ac28607 100644 --- a/crates/clawhdf5-py/tests/test_edit.py +++ b/crates/clawhdf5-py/tests/test_edit.py @@ -896,3 +896,38 @@ def test_path_replaced_between_edits(h5py, tmp_path): from pathlib import Path _held_file_edited(h5py, moved, Path(path), other_bytes) + +def test_an_edit_releases_the_gil(h5py, tmp_path): + """Another Python thread keeps running while a large edit is written: + had the edit held the GIL, the other thread would stall for the whole + edit (Rust code never yields it).""" + import time + path = str(tmp_path / "big.h5") + with h5py.File(path, "w") as f: + f.create_dataset("d", shape=(1024, 1024), dtype=" 0.1, "the edit is too quick to tell" + assert gaps.max() < 0.5 * (t1 - t0), ( + f"the other thread stalled for {gaps.max():.3f} s of a {t1 - t0:.3f} s edit") From b0930708b6edd912750de9acd02f9d824562cea9 Mon Sep 17 00:00:00 2001 From: osobh Date: Sun, 27 Sep 2026 07:48:04 -0500 Subject: [PATCH 7/9] py: boolean-mask keys raise NotImplementedError, not TypeError h5py supports boolean masks for reads and writes; clawhdf5 supports neither, so a mask is an unsupported operation (NotImplementedError, as for every other edit the bindings cannot do), not an invalid key. Tests: test_unsupported_edits_are_clear_errors (1-D, N-D and per-axis mask writes, file unchanged) and test_boolean_masks_are_refused (reads). Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 2 ++ crates/clawhdf5-py/README.md | 5 +++-- crates/clawhdf5-py/src/select.rs | 13 ++++++++----- crates/clawhdf5-py/tests/test_edit.py | 7 +++++++ crates/clawhdf5-py/tests/test_read_vs_h5py.py | 2 +- docs/known-issues.md | 5 +++-- 6 files changed, 24 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 98046ea..f3dd9d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,8 @@ compound fields by name, variable-length data, ...; `docs/known-issues.md`). - `File.mode`, `File.flush()` (a no-op), `Dataset.chunks`. +- Boolean-mask keys (`ds[mask]`, `ds[mask] = v`), which h5py supports, + raise `NotImplementedError` (they raised `TypeError`). - Tests (`tests/test_edit.py`): each edit applied to two copies of a file, by h5py and by clawhdf5, and both read back through h5py after every edit, on files h5py writes with `libver` earliest, v114 and latest and on diff --git a/crates/clawhdf5-py/README.md b/crates/clawhdf5-py/README.md index 50b0d17..06eebec 100644 --- a/crates/clawhdf5-py/README.md +++ b/crates/clawhdf5-py/README.md @@ -43,8 +43,9 @@ with clawhdf5.File("data.h5", "r") as f: Other types raise `TypeError`. - Keys are h5py's: integers, slices with a positive step, `...`, one increasing list of integers, compound field names. Each maps onto a - hyperslab selection. `None`, negative steps and boolean masks are refused - with h5py's errors. + hyperslab selection. `None` and negative steps are refused + with h5py's errors; boolean masks (which h5py supports) raise + `NotImplementedError`, for reads and writes. - What is read from the file: a selection whose bounding box covers at most half the dataset decodes only the chunks (or contiguous rows) the box overlaps. The library decodes the whole dataset for a larger box diff --git a/crates/clawhdf5-py/src/select.rs b/crates/clawhdf5-py/src/select.rs index 7d11f3c..93a1ad1 100644 --- a/crates/clawhdf5-py/src/select.rs +++ b/crates/clawhdf5-py/src/select.rs @@ -7,11 +7,12 @@ //! (negative from the end) drop their axis, slices must have a positive //! step, one `Ellipsis` fills the unmentioned axes, a single increasing list //! of integers may index one axis, and strings name compound fields. -//! Everything else (`None`/`np.newaxis`, boolean masks, several index lists) -//! is refused with the error h5py gives. +//! Everything else (`None`/`np.newaxis`, several index lists) is refused +//! with the error h5py gives; boolean masks, which h5py supports, raise +//! `NotImplementedError`. use clawhdf5_format::selection::Selection; -use pyo3::exceptions::{PyIndexError, PyTypeError, PyValueError}; +use pyo3::exceptions::{PyIndexError, PyNotImplementedError, PyTypeError, PyValueError}; use pyo3::prelude::*; use pyo3::types::{PyEllipsis, PySlice, PyString, PyTuple}; @@ -379,8 +380,10 @@ fn parse_axis(py: Python<'_>, a: &Bound<'_, PyAny>, n: u64) -> PyResult { let arr = np.call_method1("asarray", (a,))?; let kind: String = arr.getattr("dtype")?.getattr("kind")?.extract()?; if kind == "b" { - return Err(PyTypeError::new_err( - "Boolean mask indexing is not supported by clawhdf5", + // h5py supports masks; clawhdf5 does not (yet), for reads or + // writes: an unsupported operation, not a wrong key. + return Err(PyNotImplementedError::new_err( + "boolean mask indexing is not supported by clawhdf5", )); } let ndim: usize = arr.getattr("ndim")?.extract()?; diff --git a/crates/clawhdf5-py/tests/test_edit.py b/crates/clawhdf5-py/tests/test_edit.py index ac28607..a49df60 100644 --- a/crates/clawhdf5-py/tests/test_edit.py +++ b/crates/clawhdf5-py/tests/test_edit.py @@ -643,6 +643,13 @@ def test_unsupported_edits_are_clear_errors(h5py, tmp_path): f["chunk_ext"].resize(3, axis=2) with pytest.raises(TypeError): f["i4"][0] = np.array(["a"] * 10) + # h5py writes through boolean masks; clawhdf5 does not. + with pytest.raises(NotImplementedError, match="mask"): + f["u1"][np.arange(16) % 2 == 0] = 5 + with pytest.raises(NotImplementedError, match="mask"): + f["i4"][f["i4"][()] > 30] = 0 + with pytest.raises(NotImplementedError, match="mask"): + f["i4"][np.ones(6, dtype=bool), 2] = 0 assert snapshot(h5py, path) == before h5dump_reads(path) diff --git a/crates/clawhdf5-py/tests/test_read_vs_h5py.py b/crates/clawhdf5-py/tests/test_read_vs_h5py.py index 4fab9d2..d45dd61 100644 --- a/crates/clawhdf5-py/tests/test_read_vs_h5py.py +++ b/crates/clawhdf5-py/tests/test_read_vs_h5py.py @@ -440,7 +440,7 @@ def test_unsupported_types_are_errors_not_data(pair): def test_boolean_masks_are_refused(pair): ours, _, _ = pair - with pytest.raises(TypeError): + with pytest.raises(NotImplementedError, match="mask"): ours["num/le_i4_1d"][np.ones(37, dtype=bool)] diff --git a/docs/known-issues.md b/docs/known-issues.md index 871729d..cc2015e 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -164,8 +164,9 @@ before anything is written. On top of them: elements, variable-length data, strings padded with spaces or NUL-terminated (libhdf5 converts those differently from numpy; NUL-padded ones, h5py's, are writable), compounds containing such strings, null - dataspaces, and index-list writes of more than 2²² elements (write them - in slices). + dataspaces, index-list writes of more than 2²² elements (write them + in slices), and boolean-mask keys (`ds[mask] = v`, and mask reads; + h5py supports both). - **`str` attributes are fixed-length UTF-8**, where h5py writes variable-length strings: h5py reads them back as `bytes` (`numpy.bytes_`), not `str`. From d09e55e229773d2a6dd965e353de75e7d7e1ab70 Mon Sep 17 00:00:00 2001 From: osobh Date: Sun, 27 Sep 2026 07:51:22 -0500 Subject: [PATCH 8/9] edit: plan from a new description of the held file, not a clone (Linux) Planning from a mapping of a clone of the locked descriptor shared its flock: a process forked by another thread meanwhile (any Command) kept the lock alive for a moment after the editor was dropped, and a test that reopened the file at once saw Error::Locked (once in a full run). On Linux plan through /proc/self/fd (a new open file description of the same file, which still follows a rename); elsewhere keep the clone. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/clawhdf5/src/edit/mod.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/crates/clawhdf5/src/edit/mod.rs b/crates/clawhdf5/src/edit/mod.rs index 66c59ad..455c77a 100644 --- a/crates/clawhdf5/src/edit/mod.rs +++ b/crates/clawhdf5/src/edit/mod.rs @@ -832,10 +832,23 @@ impl FileEditor { Ok(f) } - /// A reader over the held file itself for planning an edit (it shares - /// the file's lock, and is dropped before the edit writes). + /// A reader over the held file for planning an edit (dropped before the + /// edit writes). On Linux a new open file description through + /// `/proc/self/fd`: a mapping of a clone of the held descriptor would + /// share its `flock`, and a process forked meanwhile (any + /// `std::process::Command` on another thread) would briefly keep the + /// lock alive after the editor is dropped. Elsewhere a clone of the + /// held descriptor, which follows the file wherever its path goes. fn plan_reader(&self) -> Result { let dir = self.path.parent().map(Path::to_path_buf); + #[cfg(target_os = "linux")] + { + use std::os::fd::AsRawFd; + let proc = format!("/proc/self/fd/{}", self.file.as_raw_fd()); + if let Ok(f) = std::fs::File::open(proc) { + return File::from_std_file(f, dir); + } + } File::from_std_file(self.file.try_clone()?, dir) } From 18e9be0af7a6a781fe4c394f45bed37a6472bfa3 Mon Sep 17 00:00:00 2001 From: osobh Date: Sun, 27 Sep 2026 07:57:49 -0500 Subject: [PATCH 9/9] py: only well-formed masks are NotImplementedError; others stay TypeError A boolean array of the axis's length, or of the dataset's whole shape, is a mask h5py would apply (NotImplementedError here); one of any other shape (np.array(True), a wrong length) is a key h5py itself refuses with TypeError, which test_errors_match_h5py requires us to match. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/clawhdf5-py/src/select.rs | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/crates/clawhdf5-py/src/select.rs b/crates/clawhdf5-py/src/select.rs index 93a1ad1..88ac358 100644 --- a/crates/clawhdf5-py/src/select.rs +++ b/crates/clawhdf5-py/src/select.rs @@ -255,6 +255,17 @@ pub(crate) fn parse(key: &Bound<'_, PyAny>, dims: &[u64]) -> PyResult { } } + // A mask of the dataset's whole shape (`ds[ds[()] > 0]`). + if let [a] = args.as_slice() { + let np = key.py().import("numpy")?; + if a.is_instance(&np.getattr("ndarray")?)? + && a.getattr("dtype")?.getattr("kind")?.extract::()? == "b" + && a.getattr("ndim")?.extract::()? > 1 + && a.getattr("shape")?.extract::>()? == dims + { + return Err(mask_unsupported()); + } + } if args.iter().any(|a| a.is_none()) { return Err(PyTypeError::new_err( "Indexing with None (or np.newaxis) is not supported", @@ -333,6 +344,10 @@ pub(crate) fn parse(key: &Bound<'_, PyAny>, dims: &[u64]) -> PyResult { }) } +fn mask_unsupported() -> PyErr { + PyNotImplementedError::new_err("boolean mask indexing is not supported by clawhdf5") +} + fn parse_axis(py: Python<'_>, a: &Bound<'_, PyAny>, n: u64) -> PyResult { if a.is_none() { return Err(PyTypeError::new_err( @@ -380,10 +395,15 @@ fn parse_axis(py: Python<'_>, a: &Bound<'_, PyAny>, n: u64) -> PyResult { let arr = np.call_method1("asarray", (a,))?; let kind: String = arr.getattr("dtype")?.getattr("kind")?.extract()?; if kind == "b" { - // h5py supports masks; clawhdf5 does not (yet), for reads or - // writes: an unsupported operation, not a wrong key. - return Err(PyNotImplementedError::new_err( - "boolean mask indexing is not supported by clawhdf5", + // A mask along this axis (h5py supports them; clawhdf5 does + // not, for reads or writes: an unsupported operation). A mask + // of any other shape is a wrong key, as in h5py. + let shape: Vec = arr.getattr("shape")?.extract()?; + if shape == [n] { + return Err(mask_unsupported()); + } + return Err(PyTypeError::new_err( + "Boolean indexing array has incompatible shape", )); } let ndim: usize = arr.getattr("ndim")?.extract()?;