Skip to content

Commit cfac007

Browse files
lguerardclaude
andcommitted
fix(ome_zarr): 🐛 guard in-place re-merges and match NGFF layout to zarr version
Four fixes, three of them found by comparing against napari-tmidas. An in-place merge destroys its own input, so running it twice would add each tile's offset to ids that are already global and could land two unrelated objects on the same id. The merge now records its progress on the array: "running" (a previous attempt died mid-relabel, so the data is part local and part global) refuses and asks for a re-segmentation; "done" is a no-op, so a failure *after* the relabel can still be retried cheaply. An earlier retry test passed only by luck. The NGFF metadata layout was wrong for the store being written. NGFF 0.4 is defined over zarr v2 with its keys at the top level; 0.5 is the v3 revision and nests them under "ome" with the version there. patchworks wrote v3 data with 0.4's top-level layout, which matches neither -- a strict 0.5 reader looking under attributes.ome found nothing. The layout now follows the zarr version, and every reader goes through read_ngff_attr, which accepts both, so stores written by earlier versions still load. _default_axes guesses from the number of dimensions, which is unambiguous up to 3-D but silently labels a (t, z, y, x) array "czyx". It now warns. Files never reach this path -- their axes come from the reader's dimension metadata, which is what napari-tmidas does and what patchworks already did. The compressor is pinned rather than inherited from zarr's default (zstd either way today, but the stores should not change silently if that moves). Verified end to end on both merge routes, through the napari plugin's label discovery, and against napari-chunked-regionprops (17/17, measurements matching direct voxel counts). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent ab889be commit cfac007

10 files changed

Lines changed: 350 additions & 36 deletions

File tree

docs/api/plugins/ome_zarr.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,3 +24,15 @@ resolution, matching anisotropic microscopy stacks.
2424
## register_labels
2525

2626
::: patchworks.plugins.ome_zarr.register_labels
27+
28+
## NGFF metadata layout
29+
30+
NGFF 0.4 is defined over zarr v2 and puts its keys at the top level; 0.5 is
31+
the zarr-v3 revision and nests them under `ome`. patchworks writes whichever
32+
matches the store, and reads both.
33+
34+
::: patchworks.plugins.ome_zarr.ngff_version
35+
36+
::: patchworks.plugins.ome_zarr.read_ngff_attr
37+
38+
::: patchworks.plugins.ome_zarr.write_ngff_attrs

docs/guide/merging.md

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,9 +94,19 @@ image that is most of the volume.
9494

9595
When the merge's output *is* its input, this pass rewrites the array in
9696
place. That is safe because the boundary scan (step 3) has already finished,
97-
so nothing still needs the original ids. The trade-off is restartability: a
98-
job killed part-way through leaves a half-relabelled array, where a separate
99-
output store would have left the input intact to redo from.
97+
so nothing still needs the original ids.
98+
99+
Because an in-place merge destroys its own input, it records how far it got
100+
on the array itself, and refuses to guess on a re-run:
101+
102+
| State found | What happens |
103+
| --- | --- |
104+
| nothing recorded | fresh tile-local ids — merge normally |
105+
| `running` | a previous attempt died mid-relabel, so the array is part local and part global. **Refuses**: re-segment to rebuild it. |
106+
| `done` | already merged — a no-op, so a failure *after* the relabel (the pyramid, say) can simply be retried |
107+
108+
Without that, a second pass would add each tile's offset to ids that are
109+
already global, which can land two unrelated objects on the same id.
100110

101111
## Using the merge step standalone
102112

docs/guide/ome_zarr_napari.md

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,23 @@ natively. Pass `write_to="other.zarr"` to instead write a separate
2222
single-resolution label store, or `output_component="cells"` to name the label
2323
image.
2424

25-
The label pyramid is built lazily (`da.to_zarr`, streamed chunk by chunk), so
26-
it stays OOM-safe even for terabyte volumes. Control it with `pyramid_levels`
27-
and `pyramid_downscale`.
25+
The label pyramid is streamed chunk by chunk, so it stays OOM-safe even for
26+
terabyte volumes. Control it with `pyramid_levels` and `pyramid_downscale`.
27+
28+
!!! note "NGFF version"
29+
The metadata layout follows the zarr format being written: **NGFF 0.5**
30+
(keys nested under an `ome` attribute) on zarr v3, **0.4** (keys at the
31+
top level) on v2. Writing v3 data with 0.4's top-level layout — which
32+
earlier versions did — matches neither revision, and a strict 0.5 reader
33+
finds nothing there.
34+
35+
Reading accepts **both** layouts, so stores written by any patchworks
36+
version still load. Use `read_ngff_attr()` if you need to inspect the
37+
metadata yourself rather than indexing `attrs["multiscales"]` directly.
38+
39+
patchworks' own hints (`n_objects`, `sequential_labels`) are not NGFF
40+
keys, so they stay at the top level where a consumer finds them without
41+
knowing the layout.
2842

2943
## Why a pyramid?
3044

@@ -107,6 +121,21 @@ input — see [Cluster usage](snakemake.md).
107121
to_ome_zarr("scan.ims", "scan.zarr", reuse_pyramid=True)
108122
```
109123

124+
### Axis names
125+
126+
For a file, axes come from the reader's own dimension metadata (bioio's
127+
`dims.order`, Imaris, or the `sequence_pattern` groups) — not guessed. Only
128+
non-spatial singleton axes are dropped, so a genuine `z=1` survives.
129+
130+
For a **bare array** there is nothing to read, so axes are inferred from the
131+
number of dimensions (`yx`, `zyx`, `czyx`, `tczyx`). That is unambiguous up to
132+
3-D; from 4-D the leading axis could be channel or time, and patchworks logs a
133+
warning saying what it guessed. Pass `axes=` to settle it:
134+
135+
```python
136+
to_ome_zarr(arr, "out.zarr", axes="tzyx") # not the czyx it would assume
137+
```
138+
110139
### Pixel calibration
111140

112141
The physical voxel size is read from the input — bioio's `physical_pixel_sizes`,

src/patchworks/_distributed.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import numpy as np
1919
import zarr
2020

21+
from ._io import zarr_compressor_kwargs
2122
from ._relabel import relabel_sequential_array
2223

2324
Overlap = Union[int, Sequence[int]]
@@ -116,7 +117,11 @@ def create_stage(
116117
"""
117118
root = zarr.open_group(str(stage_path), mode="w")
118119
root.create_array(
119-
name=component, shape=shape, chunks=tile_shape, dtype=dtype
120+
name=component,
121+
shape=shape,
122+
chunks=tile_shape,
123+
dtype=dtype,
124+
**zarr_compressor_kwargs(),
120125
)
121126
return str(stage_path)
122127

src/patchworks/_io.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,35 @@
1515
_ZARR_V3 = int(zarr.__version__.split(".")[0]) >= 3
1616

1717

18+
_ZARR_V3 = int(zarr.__version__.split(".")[0]) >= 3
19+
20+
21+
def zarr_compressor_kwargs() -> dict:
22+
"""Keyword arguments pinning the compression codec for a new array.
23+
24+
zstd is already zarr v3's default, but relying on a library default means
25+
the stores patchworks writes change silently if that default ever moves.
26+
Labels in particular are highly compressible, so this is worth stating.
27+
28+
Returns
29+
-------
30+
dict
31+
``compressors=``/``compressor=`` as the installed zarr expects, or
32+
empty if the codec cannot be built (then the default applies).
33+
"""
34+
try:
35+
if _ZARR_V3:
36+
from zarr.codecs import ZstdCodec
37+
38+
return {"compressors": (ZstdCodec(level=1),)}
39+
import numcodecs
40+
41+
return {"compressor": numcodecs.Zstd(level=1)}
42+
except Exception: # pragma: no cover - depends on the installed zarr
43+
logger.debug("could not pin a compressor; using zarr's default")
44+
return {}
45+
46+
1847
def load_ome_zarr(
1948
store_path: Union[str, Path],
2049
channel: int | None = 0,

src/patchworks/_merge.py

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import zarr
3535

3636
from ._chunks import cpu_allocation
37+
from ._io import zarr_compressor_kwargs
3738

3839
try:
3940
from tqdm.auto import tqdm as _tqdm
@@ -45,6 +46,12 @@
4546
_ZARR_V3 = int(zarr.__version__.split(".")[0]) >= 3
4647
_LUT_WARN_THRESHOLD = 100_000_000 # warn when max_label > 100 M (LUT > 800 MB)
4748

49+
# Attributes recording how far an in-place merge got. Needed because an
50+
# in-place merge destroys its own input: a second pass over already-global ids
51+
# would add the per-tile offsets again and can collide unrelated objects.
52+
_MERGE_STATE = "patchworks_merge_state"
53+
_MERGE_COUNT = "patchworks_n_objects"
54+
4855
# Per-worker globals set by _init_worker.
4956
# LUT is memory-mapped from disk so it is shared read-only across all workers
5057
# (OS page cache, no per-process copy). Passing the LUT directly via pickle
@@ -335,10 +342,13 @@ def _create_zarr_label_array(
335342
"""
336343
if name in group:
337344
del group[name]
345+
kwargs = zarr_compressor_kwargs()
338346
if _ZARR_V3:
339-
return group.create_array(name, shape=shape, chunks=chunks, dtype=dtype)
347+
return group.create_array(
348+
name, shape=shape, chunks=chunks, dtype=dtype, **kwargs
349+
)
340350
return group.zeros(
341-
name, shape=shape, chunks=chunks, dtype=dtype, overwrite=True
351+
name, shape=shape, chunks=chunks, dtype=dtype, overwrite=True, **kwargs
342352
)
343353

344354

@@ -635,6 +645,29 @@ def zarr_native_merge(
635645
"output_chunks cannot differ from the source chunking when "
636646
"merging in place (the array is rewritten, not recreated)"
637647
)
648+
# Running this twice over the same array would add the per-tile
649+
# offsets to ids that are already global, which can land two unrelated
650+
# objects on the same id. The source is destroyed as we go, so the
651+
# state has to be recorded rather than inferred.
652+
state = arr.attrs.get(_MERGE_STATE)
653+
if state == "running":
654+
raise RuntimeError(
655+
f"{out_path}/{out_component} was left half-merged by an "
656+
"earlier attempt, so its labels are part local and part "
657+
"global and cannot be merged again. Re-run the segmentation "
658+
"for this label (delete the label group and its tiles.json) "
659+
"to rebuild it from scratch."
660+
)
661+
if state == "done":
662+
n = arr.attrs.get(_MERGE_COUNT)
663+
logger.info(
664+
"zarr_native_merge: %s/%s is already merged (%s objects); "
665+
"nothing to do",
666+
out_path,
667+
out_component,
668+
n,
669+
)
670+
return n
638671
logger.info("zarr_native_merge: relabeling in place, no second store")
639672

640673
out_root = zarr.open_group(out_path, mode="a")
@@ -699,6 +732,12 @@ def zarr_native_merge(
699732
np.save(lut_path, lut)
700733
del lut # parent no longer needs it; workers load via mmap
701734

735+
if in_place:
736+
# From here the source is being overwritten. Record that, so a crash
737+
# leaves evidence the array is half-converted rather than looking like
738+
# untouched input to the next attempt.
739+
arr.attrs[_MERGE_STATE] = "running"
740+
702741
try:
703742
if n_w <= 1:
704743
_init_worker(
@@ -731,6 +770,13 @@ def zarr_native_merge(
731770

732771
shutil.rmtree(_lut_dir, ignore_errors=True)
733772

773+
if in_place:
774+
# Every chunk is now global. Recording the count as well makes a retry
775+
# after a *later* failure (the pyramid, say) a cheap no-op instead of
776+
# forcing a full re-segmentation.
777+
arr.attrs[_MERGE_COUNT] = n_objects
778+
arr.attrs[_MERGE_STATE] = "done"
779+
734780
return n_objects
735781

736782

src/patchworks/plugins/napari.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import zarr
3333

3434
from .._io import load_ome_zarr
35+
from .ome_zarr import read_ngff_attr
3536

3637
logger = logging.getLogger(__name__)
3738

@@ -84,7 +85,7 @@ def _has_multiscales(path: Union[str, Path]) -> bool:
8485
True if the group has a ``multiscales`` attribute.
8586
"""
8687
root = zarr.open_group(str(path), mode="r")
87-
return "multiscales" in root.attrs
88+
return read_ngff_attr(root.attrs, "multiscales") is not None
8889

8990

9091
def _multiscale_levels(
@@ -105,7 +106,7 @@ def _multiscale_levels(
105106
One lazy array per resolution level.
106107
"""
107108
root = zarr.open_group(str(path), mode="r")
108-
datasets = root.attrs["multiscales"][0]["datasets"]
109+
datasets = read_ngff_attr(root.attrs, "multiscales")[0]["datasets"]
109110
return [
110111
load_ome_zarr(path, channel=channel, level=i)
111112
for i in range(len(datasets))
@@ -231,7 +232,7 @@ def _inner_label_names(store: Union[str, Path]) -> list[str]:
231232
grp = zarr.open_group(f"{store}/labels", mode="r")
232233
except Exception:
233234
return []
234-
return list(grp.attrs.get("labels", []))
235+
return list(read_ngff_attr(grp.attrs, "labels", []) or [])
235236

236237

237238
def _resolve_labels(

0 commit comments

Comments
 (0)