feat: add table_shard_size_bytes to control the zarr shard size of tables - #1199
feat: add table_shard_size_bytes to control the zarr shard size of tables#1199Tomatokeftes wants to merge 2 commits into
table_shard_size_bytes to control the zarr shard size of tables#1199Conversation
Add a keyword-only `table_shard_size_bytes` to `SpatialData.write`, `SpatialData.write_element` and `write_table` (as `shard_size_bytes`). It is a target size in bytes of uncompressed data for a single zarr shard of every array inside a table group. A table is a heterogeneous tree of zarr arrays of mixed rank, length and dtype that all receive one shared `dataset_kwargs` from anndata, so a flat chunks/shards tuple cannot be honoured: a 2-D `chunks` raises on `obs/_index`, a 1-D `chunks` raises on 2-D `obsm`, any `shards` tuple raises on the `uns` scalars, and `shards` without `chunks` raises on divisibility. A scalar byte budget is the one shape that executes. Nothing is passed into `dataset_kwargs`. Two process globals are scoped around the existing anndata call for the duration of one table write: zarr's `array.target_shard_size_bytes`, and anndata's `zarr_write_format` and `auto_shard_zarr_v3` settings. anndata then injects `shards="auto"` itself, only at the writers where that is safe, and yields to the caller set budget instead of installing its own 1 GB default. zarr derives the shard shape from the chunk shape, so `shard % chunk == 0` and `shard <= array` hold by construction at every rank. `shards` deliberately never reaches `dataset_kwargs`: zarr's `_guess_num_chunks_per_axis_shard` does not terminate on a rank-0 array while a shard budget is set (zarr-developers/zarr-python#4304), and every SpatialData table carries rank-0 string scalars in `uns/spatialdata_attrs`. `zarr_write_format` is overridden alongside the sharding setting because `AnnData.write_zarr` reopens the group with `zarr_format` taken from that setting; leaving it at 2 silently produces a zarr v2 table group and no sharding at all. Both write branches are wrapped, so the semantics are uniform across the supported anndata range with no version-conditional code. The argument is validated up front in `write` and `write_element`, before any element reaches disk, and raises `TableWriteOptionsError` (a `ValueError` subclass) when it is not a positive int, when zarr is older than 3.1.6, when anndata does not support zarr v3 auto-sharding, or when the table format is zarr v2. The zarr and anndata gates are runtime checks, so no dependency pins change. Closes scverse#1178
The skip condition now also covers anndata, so a leg without zarr v3 auto-sharding support skips instead of failing on an AttributeError or on the wrong validation message. Applied to the two tests that push a budget through validation without sharding anything, and reused by the issue scverse#1183 guard, so all the shard tests share one predicate.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1199 +/- ##
=======================================
Coverage 91.89% 91.89%
=======================================
Files 53 53
Lines 7942 7975 +33
=======================================
+ Hits 7298 7329 +31
- Misses 644 646 +2
🚀 New features to boost your workflow:
|
|
|
||
| from spatialdata._io.exceptions import TableWriteOptionsError | ||
|
|
||
| if isinstance(table_shard_size_bytes, bool) or not isinstance(table_shard_size_bytes, int): |
There was a problem hiding this comment.
isinstance(x, bool) and not isinstance(x, int) are redundant. Plus, I don't think we should be checking the types of non-union arguments anyway =)
| settings_obj = getattr(ad, "settings", None) | ||
| if settings_obj is None or not hasattr(settings_obj, "auto_shard_zarr_v3"): | ||
| raise TableWriteOptionsError( | ||
| "`table_shard_size_bytes` requires an anndata that supports zarr v3 auto-sharding, got " |
There was a problem hiding this comment.
I know we're trying to be nice here by catering to many versions of anndata (and zarr!) but I really dislike that we essentially lie to our users on the function signature, only to immediately disappoint by throwing an exception if the versions of zarr and/or anndata aren't what we need. We also completely defeat the type checker's ability to tell if the arguments are good or not.
One way around it would be to name those parameters as something like table_shard_size_bytes_hint (note the "hint" at the end); This makes it clear that they may or may not apply and we can just do nothing if that feature isn't supported.
Alternatively, we could create the type TableShardBudget, with a method like TableShardBudget.try_create(...), which is clearly visibly fallible, and would go through the validation logic in this function. This way if a client fails to get a TableShardBudget, then they can react accordingly (and locally to their code!), and all functions that use the budget don't have to re-validate. And you could also make the TableShardBudget be itself the context manager.
Maybe there is a way to have different signatures depending on what dependencies we have, but that would have strange impacts in our versioning scheme, so I'm skeptical that this could work.
Curious to see what other people think
| def _table_shard_budget(shard_size_bytes: int | None) -> Generator[None, None, None]: | ||
| """Scope a zarr shard budget and anndata's zarr v3 auto-sharding around a single table write. | ||
|
|
||
| Nothing is passed into anndata's `dataset_kwargs`. Instead two process globals are set for the duration of one |
There was a problem hiding this comment.
I think this docstring could be a bit more succint; I find it a bit hard to understand in the context of this PR, and would find it even harder when browsing the code out of context.
| # `write_zarr` in anndata v0.13 and above can only write to zarr v3 | ||
| # solution of passing resolved store directly roughly based on: | ||
| # https://github.com/scverse/anndata/issues/1548#issuecomment-2199801855 | ||
| with _table_shard_budget(shard_size_bytes): |
There was a problem hiding this comment.
I would just like to be absolutely sure that this is the only way to do this. Temporarily setting a global variable is a very dangerous design, even with the context managers (e.g.: how do we even know we're not already inside a context? what happens on multithreaded applications? etc), so if there is any way we could pass these arguments to the a function call, I'd much much much prefer that.
Closes #1178.
Adds a keyword-only
table_shard_size_bytes: int | NonetoSpatialData.writeandSpatialData.write_element, forwarded towrite_tableasshard_size_bytes. It is a target size inbytes of uncompressed data for a single zarr shard of every array inside a table group.
Why a byte budget and not a chunks/shards tuple
This is deliberately not symmetric with #1106. A table is a heterogeneous tree of zarr arrays of
mixed rank, length and dtype that all receive one shared
dataset_kwargsfrom anndata, so no singletuple can be honoured by all of them. Measured on anndata 0.12.16 and zarr 3.2.1 with an ordinary
table:
chunksraises onobs/_indexchunksraises on 2-Dobsmshardstuple raises on theunsscalarsshardswithoutchunksraises on divisibilitychunks=<int>broadcasts, butshards=<int>raisesTypeErrorA
table_write_kwargsmirroringraster_write_kwargswould therefore ship an API whose documentedhappy path cannot execute. A scalar budget avoids that: zarr derives shard = chunk * n per array, so
shard % chunk == 0andshard <= arrayhold by construction at every rank, length and dtype.I offered
raster_shard_size_bytesas a symmetric form on the issue and have since withdrawn it. Themechanism does not carry over:
array.target_shard_size_bytesis read only when zarr is asked for anautomatic shard shape, and nothing injects
shards="auto"on the raster side (shardsdoes not appearin
_io/io_raster.py), so the same construction there would be a silently inert argument. A byte budgetis also the wrong shape for raster, where rank is uniform and
storage_optionsalready carries anexplicit per-level
chunks. #1106'sraster_write_kwargslooks like the right form for that side, sothis PR stays table only.
How it is delivered
Nothing is passed into
dataset_kwargs. Two process globals are scoped around the existing anndatacall, for one table's write:
zarr.config["array.target_shard_size_bytes"]anndata.settings.override(zarr_write_format=3, auto_shard_zarr_v3=True)anndata then injects
shards="auto"itself, only at the four writers where that is safe, and yieldsto the caller-set budget instead of installing its own 1 GB default. The existing
table.write_zarr(...)and
write_adata(group, name, table)calls are unchanged, and the #1183 re-fetch is outside thescoped block, so the encoding attributes are untouched.
To be clear about what this does and does not do: it narrows the global, it does not remove it. Today
a downstream writer has to hold
zarr.configopen across a wholesdata.write; after this it isscoped to one element and restored on exit, including on exceptions. It is still a process global
underneath.
Both write branches are wrapped, so the semantics are uniform across the supported anndata range with
no version-conditional code.
Two things worth flagging
shardsmust never reachdataset_kwargs. zarr's_guess_num_chunks_per_axis_sharddoes notterminate on a rank-0 array while
array.target_shard_size_bytesis set, and every SpatialData tablecarries rank-0 string scalars in
uns/spatialdata_attrs. It is an unbounded pure-Python loop, not anerror, so it would hang the write rather than fail it. Filed upstream as
zarr-developers/zarr-python#4304. Nothing here can reach it, and there is a fast test asserting that.
zarr_write_formathas to be overridden alongside the sharding setting.AnnData.write_zarrreopensthe group with
mode="w"andzarr_format=settings.zarr_write_format, destroying and recreating thegroup spatialdata just made; with that setting left at 2, the argument would be silently inert
(measured: table group format 3 before, 2 after,
X/datashardsNone, no error and no warning).Validation
All errors are
TableWriteOptionsError, a newValueErrorsubclass re-exported from the top level.All four are raised up front in
writeandwrite_element, before anything reaches disk, because_write_elementcreates the element group andwritewrites every preceding element before thetable is reached.
int(boolincluded)array.target_shard_size_bytes, but 3.1.4 and 3.1.5 still size the innerchunk with
max_bytes=1024where 1 MiB was intended (fixed by fix: auto-chunking when auto-sharding 1MiB number zarr-developers/zarr-python#3603),which would put roughly 130k inner chunks in a 128 MiB shard
The zarr and anndata gates are runtime checks, so
zarr>=3.0.0andanndata>=0.9.1are unchanged andno CI leg gains a dependency.
Setting the argument forces
auto_shard_zarr_v3=Truefor the duration of each table write, so itoverrides an explicit
False; there is no value that turns sharding off. The budget is a target, nota bound: below the automatically chosen inner chunk it degenerates to one chunk per shard. Both are
documented.
Tests
tests/io/test_readwrite.py, on a purpose-built 4000 x 2000 CSR table (the shipped_get_tableis8 kB and cannot differentiate any budget):
shards % chunks == 0, and the smaller budgetproduces a strictly smaller shard
regionshardsnever reaches anndata, on both write branches (a fast guard, since the failure mode is ahang)
writeandwrite_element, with nothing writtenRelease notes
Added
table_shard_size_bytestoSpatialData.writeandSpatialData.write_element, to set a targetuncompressed size in bytes for the zarr shards of table arrays.