Repository navigation
Zarr I/O PR 6: native Zarr saving - #7346
Open
trexfeathers wants to merge 73 commits into
Open
trexfeathers wants to merge 73 commits into
trexfeathers wants to merge 73 commits into
Conversation
The parts of the plan that every task reads: what PR 6 delivers, the constraints copied verbatim from the design document, the scope boundary, how a branch over two unmerged pull requests gets rebased, nine findings that refine the spec, the file structure and the interface the tasks leave behind. Four decisions taken with the user and recorded as findings: grid-mapping writing gets a fifth hook rather than the regularisation Q8 defers (F2); _attribute_handlers moves here, in its own commit (F3); the write-side value conversions get their own module (F4); xarray goes in the requirement files but the lock files are regenerated by hand later (F5). Every zarr-python and xarray claim the write path leans on was re-checked against zarr 3.4.1 and xarray 2026.9.0 and reproduces (F6). Tasks follow in later stages. Review Focus is written last, when there are tasks to pin each line to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: b8474d78-fdfa-43e5-b907-4bc3fe833fb3
Task 1 moves the CF attribute handlers out of the netCDF package, which is what §12.4 assigns to this pull request and what closes cf/saver.py's last two-way import. Task 2 adds the fifth saver hook, so that the 63 grid-mapping parameters reach a store with no netCDF4 object behind it. Both are prerequisites for any Zarr saving at all, and neither mentions Zarr, so they lead and they commit separately. The grid-mapping hook keeps netCDF writing to the same object it writes to today, by returning it. That is what lets this pull request leave the encoding question where Q8 put it, in its own pull request, rather than changing what lands in a netCDF file as a side effect of adding Zarr. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: b8474d78-fdfa-43e5-b907-4bc3fe833fb3
Task 3 adds zarr/_encode.py: JSON-native attribute values, the _FillValue and missing_value encodings, the default fill table and masked-array filling. Pure functions, so the task tests them on plain values with no store in sight. Four findings behind it, each checked against zarr 3.4.1 and xarray 2026.9.0 rather than assumed: - missing_value must be a plain number. Written as base64, like _FillValue, xarray opens the store and masks nothing (F10). - zarr's default fill value is 0 for every dtype, floats included, so a masked cube saved with no fill_value would write zeros and declare nothing. Iris writes netCDF's default fills instead, and always declares them as _FillValue (F11). Decided with the user. - create_variable never sees the data, so the spec's "derive the write grid from the source's chunking" cannot happen there. The invariant is enforced by rechunking at write time instead, with the data variable's chunks defaulted from the cube (F12). - A masked array written straight to a zarr array raises inside numpy.ma, so filling is not an optimisation (F13). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: b8474d78-fdfa-43e5-b907-4bc3fe833fb3
Task 4 opens the four write modes, defends the w- default that makes a clobber deliberate, and puts the version 3 guard in one place. Task 5 turns the two creation calls CFSaver makes into Zarr arrays: a declared dimension is a dict entry, an array carries its dimension names, and a name that already exists is refused rather than overwritten. The keyword table is here too. A netCDF storage keyword raises and names the Zarr replacement where there is one, and says there is none where there is not - which is what stops the translation layer the spec rules out from growing back by accident. The fill value a create gets when the caller names none is netCDFs default for the dtype, and no _FillValue attribute goes with it. Who declares the attribute, and when, is Task 11: it has to happen before the metadata is consolidated, and only the saver knows the data is masked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: b8474d78-fdfa-43e5-b907-4bc3fe833fb3
Task 6 puts each kind of write at the one place it passes through: attributes are converted before zarr sees them, because a value its encoder rejects stays in the mapping; masked values are filled on the way into an array; and finalise consolidates the whole store from the root, with zarr's "not part of the version 3 specification" warning suppressed and explained. Task 7 is the saver. It is thin, which is the point of PR 5: opening a store, one hook saying Zarr holds text natively and needs no trailing character dimension, and the save() users call. Storage keywords are validated before the store is created, so a mistyped one does not leave an empty store behind. Deferred writes, the alignment invariant and the masked _FillValue declaration are Task 8, on the same class. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: b8474d78-fdfa-43e5-b907-4bc3fe833fb3
Task 8 is the one where a mistake loses data rather than raising. The _FillValue a masked source needs is declared in _backend_write, before the metadata is consolidated, because under compute=False the data is written long after. The lazy source is rechunked to the array's write grid - shards when sharded, chunks otherwise - which is what licenses lock=False, and the task carries the §4.5 reproduction as a test. delayed_completion returns a real Delayed that owns the consolidation, and binds the dataset locally rather than through self, because the closure is serialised to every distributed worker. Task 9 registers the saver. The entry is a shim in the zarr package rather than the saver function itself: registration happens the first time any format is saved, and reaching the real save would import zarr-python with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: b8474d78-fdfa-43e5-b907-4bc3fe833fb3
Task 10 saves realistic_4d_w_everything and loads it back, and pins the two format behaviours Iris cannot fix: a deleted chunk reads as fill with no warning, and consolidated metadata goes stale silently. Both fail loudly here if a zarr-python release changes them. Task 11 is the one that justifies two design decisions rather than merely implementing them - xarray opens every store Iris writes, and Iris loads what xarray wrote. It adds xarray to the three requirement files and deliberately leaves the lock files alone. Task 12 replaces the Saving section PR 4 wrote, whose whole content is that Iris cannot save to Zarr. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: b8474d78-fdfa-43e5-b907-4bc3fe833fb3
The Review Focus section is written last, with the spec in front of me and the tasks already down: five input classes a user meets in ordinary work that no task's own tests would have reached - a mesh cube, packed data, int64, a non-ASCII attribute and a dataless cube. Each got a test added to the task that owns the code, rather than being noted and left. The self-review found one real error: Task 10's fixture called realistic_4d_w_everything(lazy=True), and the function's only parameter is w_mesh. Fixed, and the mesh case now uses it deliberately. Task 13 records the seven decisions in the design document, says what PR 6 does and does not close of Q5 and Q8, and sets out what the pull request body has to say - above all that it is stacked on two unmerged branches and that CI has no xarray until the lock files are regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: b8474d78-fdfa-43e5-b907-4bc3fe833fb3
There is no tests/unit/fileformats/cf/saver/ yet - PR 5 left the savers tests under netcdf/saver/, driving the generic machinery through the only subclass there was. Task 2 adds the first test that belongs to CFSaver itself, so it has to create the package marker too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: b8474d78-fdfa-43e5-b907-4bc3fe833fb3
cf/saver.py imported ATTRIBUTE_HANDLERS from the netCDF package, which in turn imported cf/saver.py, surviving only because _attribute_handlers itself imported nothing from iris.fileformats.cf. The handlers serialise the Iris-only attribute types that every CF store needs - STASH, GRIB_PARAM, ukmo__process_flags - so the CF package is their natural home, and moving them there closes that two-way import. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
_add_grid_mapping_to_dataset bound grid_variable to cf_var_grid.variable and made sixty-three plain attribute assignments to it. That object is netCDF4's own, so any store with no netCDF4 variable behind it raised a bare AttributeError for every cube with a coordinate system. Routing those assignments through .attributes instead would change how some of them are typed in a netCDF file (crs_wkt as NC_STRING against grid_mapping_name as NC_CHAR), which Q8 of the design document rules out of this programme. So the fix is a fifth hook, _grid_mapping_writer, that returns the assignment target: the base implementation returns a small writer onto cf_var.attributes, and netCDF's override returns cf_var.variable unchanged, so the netCDF path writes to the identical object it always has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
The native Zarr saver needs a few value conversions before anything can reach a store: attributes into JSON-native types, `_FillValue` and `missing_value` into the encodings xarray expects, a default fill value per dtype, and masked arrays into filled ones. Keeping these as pure functions, mirroring `_decode.py`, lets every rule be tested without a store, ahead of the dataset and saver code that will call them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
ZarrDataset.__init__ accepted mode="r" only. This adds the four write modes zarr-python itself supports, with Iris's own default: "w-" creates and refuses to clobber an existing store, because a Zarr clobber is many non-atomic deletes rather than one atomic rename, so replacing one is opted into via mode="w" instead of assumed. Writing is guarded to version 3 in one place, in _WRITABLE_ZARR_FORMAT, so that version 2 writing can be added later by implementing the three version-specific points of the design document and deleting that guard. Reading keeps working unchanged for either version. _declared_dimensions is added as empty state for the next task to fill, and the old _NO_SAVING constant is retired along with _READ_MODES in favour of _WRITE_MODES and _MODES; the three still unimplemented write members now raise their message inline. An existing test asserted that mode="w" was refused outright; that is superseded by this change and is removed, with its replacement now covered in the new write test module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
A missing closing fence after the _dimension_size helper left every heading from Task 6 onward inside a code block, so the three paragraphs that introduce check_encoding_keywords rendered as Python and the tool that extracts one task's text for an implementer could not find where Task 5 ends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
Zarr has no dimension object, so create_dimension records a name and size and catches a conflicting redeclaration itself, rather than letting two arrays silently disagree. create_variable then writes those names onto a new array, refuses a Zarr storage keyword it does not recognise rather than guessing a netCDF translation, and applies the dtype-dependent _FillValue encoding to both the array's own fill and its attribute. The pre-existing write-surface test covered three members that always refused; two of them now work on a writable dataset, so it is narrowed to the one still unimplemented, finalise, with the read-only refusal of the other two covered by the new tests instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
create_variable always looked a default fill up in the netCDF table when none was given, but dtype=str has no entry there - it asks Zarr for a variable-length string array, which is not a netCDF concept to have a default for. Skip the lookup for that case and omit the fill_value keyword entirely, so zarr-python's own default for the dtype applies, matching what the docstring already promised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
Attributes now convert to a JSON-native form before zarr sees them, on every assignment rather than only at variable creation, so a rejected value never lands in the attributes mapping. _FillValue and missing_value get their own CF encodings, decided by the array's dtype. A write now fills any masked array with its own fill value first, since a Zarr array has no mask of its own and numpy.ma raises deep inside the bytes codec otherwise. finalise() consolidates the whole store's metadata, the step that makes a remote store usable to a reader that fetches one document instead of one per node. The _FillValue pre-encoding in create_variable was a stopgap for when the attributes mapping did not yet encode anything; it is removed now that __setitem__ does the encoding itself, so a fill value is no longer at risk of being encoded twice. test_finalise_refuses pinned the old NotImplementedError; it is superseded by TestFinalise::test_a_read_only_dataset_refuses, which pins the real refusal once finalise does its job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
CFSaver already does the CF work; the Zarr-specific saver only opens a store, says how a string is stored, provides the save() entry point that validates Zarr's own encoding keywords before anything is created, and completes the store with a consolidated metadata document. delayed_completion() wraps the inherited lazy-write delayed with a real Delayed that also runs the consolidation, so both the eager (compute=True) and deferred (compute=False) paths finish through the same mechanism, and a save that never completes never claims to have consolidated. Also refuse, rather than silently rename, a cube whose data-variable name already exists in the group when adding to an existing store: the generic CFSaver renaming is right for two cubes of one call sharing a name, and wrong for a store this call did not just create. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
mode="a" onto a store this saver had already consolidated raised a bare KeyError: ZarrDataset.__init__ opened it with zarr-python's default consolidated-metadata fast path, so the reopened group listed only the arrays present at the last consolidation, never the one the new write was about to add. Pass use_consolidated=False for every write mode; a read mode keeps the fast path, which is what it is for. Saver.write()'s own repeated-name refusal read self._dataset.variables live, which create_variable invalidates on every call, so it also caught a name created moments earlier by another cube of the same save() call - exactly the case its own docstring says must still be renamed. Compare against a snapshot taken once, when the Saver opens the dataset, instead. The refusal's message was duplicated verbatim between saver.py and _dataset.py. Shared it as existing_name_message() in _dataset.py. A scalar cube failed to save: CFSaver writes a whole variable as cf_var[:] = data, and zarr rejects slice(None) on a 0-d array. Zarr array indexing wants Ellipsis or () instead; ZarrDatasetVariable .__setitem__ now makes that substitution for a 0-d array. Corrected delayed_completion's docstring, which claimed the inherited method returns a tuple for a sequence of sources: for one delayed write it returns a plain dask.array.core.Array, never a tuple and never actually a Delayed despite the annotation. The SciTools#7291 reference and the reason for wrapping it both still hold. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
A lazy source handed straight to da.store(..., lock=False) can have its dask chunking subdivide the array's write grid - its shards when sharded, its chunks otherwise - so several concurrent tasks read- modify-write one stored object and lose each other's updates. Rechunk the source to the write grid first, so each task owns whole cells and no cell is touched twice. Declare a _FillValue when the saved data is masked, even when the cube's own attributes name none, so a reader sees the mask rather than plain sentinel values with nothing marking them missing. Reshape delayed_completion's inner function into a module-level one so the Delayed it returns survives a plain pickle round trip, not only cloudpickle, which a distributed scheduler needs regardless. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
Drop the two finding labels from shipped test comments, and correct one that described the netCDF compute=False path wrongly - it returns a dask Array for one delayed write, not always a tuple. Make _tiles_the_grid raise on a chunks/grid rank mismatch rather than silently answering True for dimensions it never examined: the one function whose whole job is to prevent a silent, mismatched write must not default to "safe" for input it did not check. Use np.ma.is_masked instead of np.ma.isMaskedArray to decide whether real data needs a declared fill value: the latter is true of an np.ma array with an all-False mask, which Iris produces routinely, and would invent a missing-value declaration for data whose source never carried one. Lazy data keeps is_lazy_masked_data, since it cannot be inspected without computing it. Rework the "aligned source is left alone" test: chunking it at exactly the write-grid size made the old chunks-equality assertion pass whether or not the early return existed, because dask's own rechunk() is already a no-op in that case. A single oversized chunk forces dask's rechunk to subdivide if asked, so identity is now a check that can fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
An earlier pull request taught find_saver to match the .zarr suffix, but iris.save still had no saver registered for it, so the lookup returned None. Register the zarr package's _save_cubes shim for the extension, keeping the import of zarr-python deferred until a Zarr target is actually saved. The shim's __module__ is "iris.fileformats.zarr", the package, not "iris.fileformats.zarr.saver": iris.save reads __module__ to decide whether a saver takes a whole CubeList at once, so that is the string the frozenset needs, not the module the shim forwards to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
_save_cubes reaches iris.fileformats.zarr.saver with a plain from .saver import save, which bypasses __getattr__ and the _check_zarr_version call it makes on every load. On an installation with zarr missing or too old, iris.load already explained the problem clearly, but iris.save silently wrote a store (when zarr was merely too old) or leaked a bare "No module named 'zarr'" (when it was entirely absent). Repeat the same checks __getattr__ runs, so all three routes into this package give the same clear error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
Everything so far has tested one piece of the Zarr saver in isolation. This is the first test that puts a realistic cube with meshes, packed data, int64 data, non-ASCII attributes and a dataless form through iris.save and iris.load together, and checks what comes back - and pins the two Zarr format behaviours Iris depends on but does not control: a deleted chunk reads back as fill with no warning, and consolidated metadata goes stale until Iris re-consolidates. Two of the five ordinary-input cases fail and are left failing on purpose: packed data round-trips to the wrong values, because nothing in the Zarr saver applies the scale_factor/add_offset transform before writing (netCDF relies on netCDF4-python doing this automatically, and Zarr has no equivalent); and a mesh does not reattach on load, because the Zarr loader (an earlier, already-merged piece of work) does not build MeshXY objects, and iris.mesh.load_meshes explicitly ignores non-netCDF sources. Both are real defects the tests found rather than authoring mistakes, so they are reported rather than papered over. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
The generic CF saver only ever sets scale_factor/add_offset as attributes and streams the unpacked values straight through, because netCDF4-python applies the pack transform itself on assignment once those attributes are set on the variable. Zarr has no such mechanism, so a packed Zarr variable was silently storing the original values rounded and cast into the narrow dtype, unrelated to what the scale_factor/add_offset attributes claimed - correct metadata over wrong data, with no warning on either the write or the read side. ZarrDatasetVariable.__setitem__ is the one place every write passes through, eager and lazy alike, so that is where the pack transform now runs: round((value - add_offset) / scale_factor), narrowed to the array's own dtype, with either attribute absent behaving as its identity. It runs before the existing mask fill rather than after, because the fill value lives in packed (stored) space and the data arrives unpacked - filling first would push an already-filled sentinel through the same transform as real data and lose it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
…nnot yet The Zarr saver writes a complete, CF-UGRID-compliant mesh - a topology variable, a connectivity with its start_index, and data-variable mesh/location attributes - confirmed by reading the store directly. Loading one back does not reattach it: the Zarr loader (already-merged, out of this pull request's scope) never builds a MeshXY, and iris.mesh.load_meshes filters to the netCDF handler only, so neither public entry point recovers a mesh from a store this saver wrote. Replaces the single round-trip test with two: one against the written store, asserting the structure the saver actually delivers, and one strict xfail for the full iris.save/iris.load round trip, so that loader support landing later is caught rather than left silently passing under a stale marker. Also tightens test_several_cubes_round_trip to assert the exact count of three cubes (the two saved plus the orography cube the cube fixture's hybrid-height factory always brings along) and that both saved names are present, rather than a bare count that said nothing about which cubes they were. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
A non-integer packing dtype reached the store silently mishandled: nothing validated the packspec before writing, and pack()'s unconditional rounding mangled a float target instead of merely narrowing it. The netCDF saver already refuses the same input outright, so the Zarr saver now does too, with the same message, checked before the store is created. pack() itself now rounds only for an integer target, matching what netCDF4-python actually does on assignment rather than a rounding version of it applied regardless. Also closes two smaller gaps in the same function: scale_factor=0 now raises naming it, rather than dividing by zero in silence, and the final cast is reconstructed from a block's underlying data and mask rather than calling MaskedArray.astype() directly, which otherwise tries to cast the block's inherited (often very large) floating fill_value into the narrow target dtype as well, and numpy warns about that even though every real value casts cleanly - a warning a lazy masked packed write would otherwise have carried all the way out to the user with no connection to anything they wrote. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
test_a_missing_chunk_reads_back_filled asserted only the shape, which is unchanged whether a missing chunk reads back in silence or with a warning - so it could not have caught a future zarr-python release that started warning about exactly that. It now also asserts no warning was raised, and that the masked count went up against the original cube's, which is the actual "data silently went missing" the test exists to record. test_a_group_round_trips saved the same cube to the root and to a named group, so reading the root would have satisfied every one of its assertions - a loader that ignored group= entirely would have passed. It now saves a differently-named cube into the group and asserts that name comes back, so ignoring group= raises instead. test_packed_data_round_trips checked only that scale_factor was present, not add_offset, and compared at a tolerance loose enough that a systematic one-count bias would have passed. It now checks both attributes and tightens the tolerance to one stored count - loose enough for the quantisation error correct packing is allowed to introduce, tight enough to fail a one-count bias. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
Every Zarr test so far has checked Iris against Iris. This checks it against the rest of the world: xarray opens every store Iris writes, in both directions. It is the only thing that can catch a swap of the base64 _FillValue and the JSON-native missing_value encodings, since Iris's own reader accepts either shape for either attribute. xarray goes into the test section of each Python version's environment file, not the lock files: the human developer regenerates those by hand once the Zarr dependency PR has landed, so until then continuous integration has no xarray and these tests skip there. They run locally, which is where they are needed while the work is being done. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
The cross-reader module proved that a floating _FillValue has to be base64, because xarray fails the whole dataset open on a plain number. It did not yet prove the other half of the same pair: that missing_value has to stay a plain number, because base64 there opens cleanly and silently masks nothing. A unit test on the encoder alone cannot see that failure, since it only checks what the function returns, not what a reader does with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6582234f-93d1-42d0-94ce-b54ee4a252af
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 96841741-ed31-4388-9014-e3f94eb82651
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## brownfield #7346 +/- ##
==============================================
+ Coverage 90.88% 91.04% +0.15%
==============================================
Files 104 106 +2
Lines 26813 27157 +344
Branches 4898 4963 +65
==============================================
+ Hits 24369 24724 +355
+ Misses 1689 1677 -12
- Partials 755 756 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Session-Id: 96841741-ed31-4388-9014-e3f94eb82651
The entry said they would be regenerated by hand once SciTools#7336 landed, and said CI would skip the cross-reader conformance tests until that happened. Both have now been done, so it says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 96841741-ed31-4388-9014-e3f94eb82651
github-actions
Bot
added
the
benchmark_this
Regenerating the lock files to add xarray moved a lot more than xarray - cartopy 0.25 to 0.26 among it - and five `TestTrajectoryWrap` cases plus one shapefile mask test then failed in CI, none of them Zarr. SciTools#7299 had already fixed exactly those on `main`: a tolerance of five decimal places on the path comparison, an adjustment in `iris.plot` for cartopy>=0.26, and the shapefile test. `brownfield` has now taken `main`, so this merge is how they arrive. The lock files are this branch's own, not the merged ones. Both sides had regenerated all three, and a line-by-line merge of two independent solver outputs is not a solve: it left `py312` and `py313` naming two different `libpython` builds apiece. This branch's set was generated today, from requirement files this merge does not touch, and is newer than the one SciTools#7299 carried, so it is taken whole. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 96841741-ed31-4388-9014-e3f94eb82651
Not this branch's code, and not its defect: `.hooks/check_docs_page_metadata.py` came with SciTools#7308 and ruff moved from 0.15.20 to 0.16.6 with the `main` merge `brownfield` has just taken, which flags ISC004 on the implicit concatenation in it. Every pull request targeting `brownfield` meets the same failure. It is fixed here because it is one line and this pull request is otherwise green; fixing it on `brownfield` as well would be no bad thing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 96841741-ed31-4388-9014-e3f94eb82651
stephenworsley
requested changes
Oct 23, 2026
| A cube with an unstructured :class:`~iris.mesh.MeshXY` saves correctly: the | ||
| store carries the full CF-UGRID structure, the same mesh, topology and | ||
| connectivity variables a netCDF save would write, and loading it back gives | ||
| you the mesh again. |
Contributor
There was a problem hiding this comment.
I think this reads a bit weird due to its history of edits. I'd remove "and loading it back gives you the mesh again".
This section is also now connected to a following comment about attributes which is unrelated. This feels like it maybe belongs as a note in the loading section since it is about the behaviour of a loaded cube and this failure to save is not specific to zarr.
Contributor
Author
There was a problem hiding this comment.
Yeah I wondered about that too. Probably a case of a too-narrow edit when rebasing. Will chase up
trexfeathers
commented
Oct 23, 2026
trexfeathers
left a comment
trexfeathers
left a comment
Contributor
Author
There was a problem hiding this comment.
(posted by GPT)
I audited this PR at f8e039b074f97516f98e5e597501cbb870091220 against brownfield at 426d7189342aa3b4db61ebd9e6b2200f97317081. I read the complete 3,044-line specification, including the decision log. Most loading and saving requirements have implementation and test evidence, but the branch does not fully satisfy the specification. No repository files were changed.
Concrete discrepancies
-
[P2, introduced here] A subgroup save with
consolidate_metadata=Falseleaves stale root metadata._completeskips metadata work entirely when consolidation is disabled. Opening the subgroup unconsolidated does not invalidate the root document. I reproduced a root whose defaultgroup_keys()returned[]while an unconsolidated reader returned['sub']. Opening the subgroup directly still worked. This violates the discovery/durability contract and the user guide's explicit promise to remove the document. The existing refusal test adds an array at the root; subsequent root attribute writes invalidate that document, so it misses subgroup saves. Root consolidated metadata needs invalidation when declining consolidation, including subgroup saves. -
[P2, introduced here] The new CF grid-mapping test fails without the optional Zarr dependency.
tests/unit/fileformats/cf/saver/test_grid_mapping_writer.py::TestTheBaseWriterimports the Zarr saver without animportorskipguard. I invoked the test with Zarr imports blocked and obtainedModuleNotFoundError. The guards in the Zarr test packages do not protect a test underfileformats/cf. This violates §6's promise that Iris without Zarr installed acquires no new test failures. -
[P2, introduced here] Integer fill values can disagree between storage and CF metadata.
ZarrDataset.create_variablecasts the storage fill to the variable dtype but writes the original argument as_FillValue. Saving maskedint16data withfill_value=1.5produced storage fill1, attribute_FillValue=1.5, and stored values[2, 1]. §4.5 requires a JSON integer and the same native value in both places. Existing tests exercise representable integer sentinels only. Reject an incompatible sentinel or encode the same validated native value in both places. -
[P2, introduced here] Two dataset capability contracts fail at their boundaries. Reopening a store containing a two-element array on dimension
x, then callingcreate_dimension('x', 3), succeeded and changeddataset.dimensionsto{'x': 3}. The conflict check considers only declarations made during that dataset instance. Separately,supports_dtype(np.dtype('O'))returnedTrue, butcreate_variablerejected that dtype as ambiguous. Both were reproduced. The ordinary saver's disambiguation tests do not exercise existing-array redeclaration; the capability tests do not establish that every affirmative answer can be created. -
[P2, inherited from brownfield] Empty-root diagnostics do not identify deeper data-bearing groups.
zarr.loader._check_has_data_variableslists immediate subgroups. For data undermeasurements/reflectance/r10m, its error suggested onlymeasurements, which also contained no data arrays. This misses §4.6's promise to name groups that contain data. The test uses an immediately data-bearing subgroup. This is an existing programme discrepancy, not a regression introduced by this PR. -
The required shared Zarr write contract is missing.
test_ZarrDataset_contract.pysubclasses onlyCFDatasetReadContract. No Zarr class runsCFDatasetWriteContract. Separate write tests provide substantial coverage, but they do not fulfil the shared-body requirement in §6 and the decision log. -
The specification has unresolved contradictions. §4.4 permits subdividing oversized storage chunks and warns; §6 still requires no subdivision and exact read alignment. Q4 still describes the discarded guard. §4.2 names
storage_settings, while later requirements name the nonexistentencodingmember. Q10 says theNETCDF3_64BITdefect is preserved, while the later decision log and implementation fix it. The progress header and PR table describe PR 4 as under review despite the later history saying it merged. These need reconciliation before an unqualified conformity judgement is possible.
Verification: the focused selection finished with 1,475 passed, 2 skipped. Adding netCDF tests produced 2,243 passed, 23 skipped, one missing-file failure and ten fixture setup errors. All ten setup-error tests passed separately. No Zarr test failed. Local-store reproductions completed outside the sandbox after standalone zarr.open_group() hung inside it. Actual remote access, minimum-version compatibility, real NOAA/EOPF data, benchmarks, documentation builds and a baseline test checkout were not tested. Real-data/benchmark work, the prose sweep and merge-back remain planned programme obligations rather than unexpected PR 6 regressions.
Core interface, attribute and dimension contracts are mostly delivered; capability boundaries and some specification statements remain partial.
Repeated requirements are consolidated here. Superseded decisions are not treated as current obligations; unresolved contradictory statements are retained. “Delivered” means implementation inspection plus relevant test evidence, or a directly checkable structural requirement. It does not mean every possible input was tested.
Evidence paths below are relative to lib/iris/. CF means fileformats/cf and tests/unit/fileformats/cf. Dataset means fileformats/zarr/_dataset.py and tests/unit/fileformats/zarr/dataset. NC means fileformats/netcdf and its unit tests. Load means the Zarr loader, its unit tests and tests/integration/zarr/test_load.py. Save means the Zarr saver, its unit tests and tests/integration/zarr/test_save.py. Decode and Encode mean the respective Zarr modules and test__decode.py/test__encode.py. IO means io/__init__.py, loading/format dispatch and tests/unit/io. Cross-reader means tests/integration/zarr/test_cross_reader.py.
| Requirement | Status | Implementation and test evidence |
|---|---|---|
| CF becomes a package | Delivered | CF _variables, _group, _reader; test___init__ |
| Existing CF public names remain exported | Delivered | Static __all__; export tests |
| Classic and UGRID classifiers remain together | Delivered | CF _variables; classifier tests |
| Generic loader is extracted | Delivered | CF loader; loader tests |
| Generic saver is extracted and remains private | Delivered | _CFSaver; Save inheritance test |
Zarr exposes load_cubes, save, Saver |
Delivered | Package forwarding table; API/dependency tests |
| Zarr imports remain lazy | Delivered | Package forwarding; subprocess import tests |
| Missing Zarr gives clear load/save errors | Delivered | Package message; dependency tests |
| Versions below 3.0.8 are rejected | Delivered | Metadata version guard; simulated old-version tests |
| Actual compatibility with Zarr 3.0.8 | Unverified | Execution used 3.4.1 |
| Dataset and variable ABCs exist | Delivered | CF dataset; ABC/contract tests |
| Variable name, location, format are explicit | Delivered | Both implementations; read contracts |
| Shape, dtype, size, ndim and length are explicit | Delivered | Properties; scalar/read contracts |
| Mode, closed state and context management are explicit | Delivered | Both implementations; lifecycle contracts |
| Attributes are materialised mappings | Delivered | Attribute mappings; contract tests |
| Storage settings are readable | Delivered | storage_settings; dataset/property tests |
| Create-time storage settings remain inspectable | Partial | Chunks/shards/codecs tested; literal encoding API absent |
| Generic code avoids backend implementation type tests | Delivered | CF inspection; test_no_implementation_coupling |
| Generic dtype handling asks capability rather than format | Delivered | _ensure_valid_dtype; NC dtype tests |
| Capability answers are truthful for every input | Partial | Numeric cases pass; object-dtype reproduction contradicts it |
| Scalar creation defaults to no dimensions | Delivered | Dataset/shared/scalar-save tests |
| Unlimited dimensions are supported or explicitly refused | Delivered | NC implementation; Zarr refusal tests |
| Dimension-size conflicts are checked | Partial | New declarations checked; existing-array conflict accepted |
| Sync, finalise and close are separate | Delivered | Dataset implementations; finalisation tests |
| NetCDF finalise is a no-op | Delivered | NC implementation; write contract |
| Write handles work after close | Delivered | NC contract; Zarr deferred/pickle tests |
Library CF attributes use .attributes |
Delivered | Classifiers/load rules inspected; tracking tests |
| Reads track attribute usage | Delivered | TrackedAttributes; unused-attribute tests |
| Separate CF wrappers have independent read sets | Delivered | Per-wrapper views; tracking tests |
| Attribute tracking is a snapshot and loading does not write | Delivered | CF construction; reader isolation tests |
| Deprecated accessor works for both backends | Delivered | CF __getattr__; CF/Zarr reader tests |
| Accessor warns with replacement mapping/key | Delivered | Accessor; warning-message tests |
Missing attributes raise AttributeError |
Delivered | Miss path; missing/default tests |
| Accessor does not cache dynamic instance members | Delivered | Source; repeated-read tests |
| NetCDF-specific fallback survives with warning | Delivered | deprecated_netcdf_member; NC tests |
| Normal load avoids deprecation and recorded problems | Delivered | Silent-load test records warnings and LOAD_PROBLEMS |
| Colliding attribute names remain reachable | Delivered | Mapping-based load/save; collision tests |
| Both Zarr versions read | Delivered | Dataset version handling; V2/V3 load tests |
V3 reads names from dimension_names |
Delivered | _dimension_names; named-dimension tests |
V2 reads names from _ARRAY_DIMENSIONS |
Delivered | _dimension_names; V2 tests |
| Absent names remain anonymous | Delivered | Dataset; anonymous tests |
| Null entries remain anonymous | Delivered | Dataset; partly named tests |
| Scalars have no dimensions | Delivered | Dataset; scalar tests |
| Repeated names within an array are accepted | Delivered | Dataset; repeated-name test |
| Wrong-length names warn and become anonymous | Delivered | Warning path; malformed V2 attribute test |
| V3 malformation rejected before Iris is distinguished | Delivered | zarr-python rejection test; defensive Iris check |
Dimension identity is (name, size) |
Delivered | _DimensionIdentity; identity/spans tests |
| Anonymous dimensions match no other dimensions | Delivered | Identity; anonymous spans tests |
| Coordinate attachment uses the same identity | Delivered | _common_data_dims; repeated-name axis test |
| Conflicting dataset names are omitted with warning | Delivered | _assemble_dimensions; conflict tests |
| Variables retain their own names despite conflicts | Delivered | Dataset variables; conflict integration tests |
| No control forces equal name/size pairs apart | Delivered | API inspection; Q9 limitation |
_ARRAY_DIMENSIONS does not reach cube attributes |
Delivered | Withholding; V2 integration test |
Root _NCProperties does not reach cube attributes |
Delivered | Withholding; NCProperties test |
_NCProperties withholding respects documented node scope |
Partial | Withheld on every selected group, not only root |
| Fixed-width unicode reads normally | Delivered | Both-version dtype tests |
Decoding, cache identity and lazy reading have test evidence; the old read-alignment requirement conflicts with the implemented decision.
| Requirement | Status | Implementation and test evidence |
|---|---|---|
| V3 storage fill alone does not mask | Delivered | Decode precedence; valid-zero tests |
| V2 storage fill supplies fallback masking | Delivered | Decode; V2 fallback tests |
| CF sentinel beats storage fill | Delivered | Decode; differing-sentinel tests |
| Both missing-value attributes contribute masks | Delivered | Decode; combined-sentinel tests |
Vector missing_value masks all valid sentinels |
Delivered | Decode vector tests |
Vector _FillValue warns and is ignored |
Delivered | Decode vector-fill test |
| Floating base64 reads as little-endian float64 | Delivered | Decode NaN/finite tests |
| Numeric floating sentinel also reads | Delivered | Decode JSON-number tests |
| Invalid sentinels warn with variable/value | Delivered | Invalid/base64-width/range tests |
| Masking precedes unpacking | Delivered | mask_and_scale; packed-value masking test |
| Valid-min/max/range mask values | Delivered | Decode range tests |
| Scale/offset unpack data | Delivered | Pure/integration packing tests |
| Structured JSON survives uninterpreted | Delivered | Dataset/CF mappings; structured-attribute tests |
| Structured attributes are not silently coerced for netCDF | Delivered | NC refusal; structured-attribute save test |
| No Zarr read proxy | Delivered | Direct array graph construction inspected |
| Decode is an explicit lazy graph layer | Delivered | read_data; lazy/meta/dtype tests |
| Small reads can bypass chunk policy | Delivered | Small-array policy tests |
| Lazy meta matches decoded dtype/mask | Delivered | Declared-dtype/meta tests |
as_lazy_data takes caller cache identity |
Delivered | Lazy-data layer; cache tests |
| Lazy layer no longer imports netCDF proxy | Delivered | Source; loader/cache tests |
NetCDF keeps repr(proxy) identity |
Delivered | NC read_data; cache-key regression evidence |
| Zarr key includes address and complete metadata | Delivered | cache_key; stability/sensitivity tests |
| Shape/fill changes miss cache | Delivered | Rewritten-store tests |
| Chunks/shards/dtype/codecs/names/attributes affect key | Delivered | Metadata sensitivity parametrisation |
| Shared coordinates reuse cached lazy arrays | Delivered | Cache-population/reuse test |
| Same-metadata content changes remain acknowledged limitation | Delivered | Metadata-only implementation; Q6 disclosure |
| Sizing budgets decoded dtype | Delivered | Dataset sizing tests |
| AS_DASK retains Dask sizing semantics | Delivered | normalize_chunks; Dask-answer test |
| Read unit is inner chunk, not shard | Delivered | chunking; instrumented partial-read test |
| §4.4 allows oversized chunk subdivision and warns | Delivered | CF warning; subdivision test |
| §6 forbids subdivision and demands universal read alignment | Missing | Contradicts §4.4's implemented decision |
| Explicit chunk controls are honoured | Delivered | CF chunk-control tests |
| Oversized warning covers every explicit control | Partial | AS_DASK/pinned controls bypass warning |
| Iris leaves Zarr async config unchanged | Delivered | Source inspection |
| Async/Dask concurrency interaction is documented | Delivered | User guide Chunking section |
Saving and write alignment are substantially delivered; subgroup invalidation, integer sentinels and dataset boundaries remain partial.
| Requirement | Status | Implementation and test evidence |
|---|---|---|
| Write V3 only | Delivered | Dataset guard; write-version tests |
| Unsupported versions fail in dataset-level guard | Delivered | Version guard tests |
| Reading ignores requested creation version | Delivered | Read-ignore-version test |
| Generic saver does not branch on Zarr version | Delivered | CF source inspection |
| Dimension declarations determine shape and array names | Delivered | Dataset creation tests |
| NumPy scalars become JSON-native | Delivered | Encode conversion tests |
| NumPy arrays become lists | Delivered | Encode/Dataset tests |
| ASCII bytes become text | Delivered | Encode/Dataset tests |
| Non-ASCII strings remain text | Delivered | Encode/integration tests |
| Sequences recursively convert NumPy values | Delivered | Encode sequence tests |
| Unsupported attrs raise with name before backend write | Delivered | Encode validation; clean-store refusal test |
| Unknown objects are not stringified | Delivered | Encode refusal tests |
| Attribute tests compare values, not NumPy dtype | Delivered | Save/Cross-reader tests |
| General attributes avoid private base64 envelopes | Delivered | Encode/Cross-reader tests |
Floating _FillValue writes base64 float64 |
Delivered | Encode/Cross-reader masked/finite tests |
| Integer sentinel is integer and matches storage fill | Partial | Proper inputs pass; 1.5 reproduction fails |
missing_value writes ordinary numbers/vectors |
Delivered | Encode/Cross-reader sentinel tests |
| Masked values are filled without separate mask array | Delivered | Assignment; mask round-trip tests |
Masked data declares _FillValue |
Delivered | Saver eager/lazy tests |
| Default numeric fill agrees with netCDF | Delivered | Encode table tests |
| Unsupported default masked dtype is refused with advice | Delivered | Float16 refusal/explicit-fill tests |
| Masked strings are refused | Delivered | Saver masked-string test |
| Strings do not invent character dimension | Delivered | String hook; label tests |
| Scalar cubes save | Delivered | Scalar-save test |
| Zarr applies packing arithmetic | Delivered | Assignment/Encode; round-trip tests |
| Invalid packing dtype/dictionary is refused | Delivered | Prevalidation/refusal tests |
| Packing precedes masked filling | Delivered | Assignment; masked packing tests |
compute=False returns genuine Delayed |
Delivered | Completion wrapper/deferred tests |
| Lazy values wait for computation | Delivered | Deferred timing tests |
One unlocked da.store handles collected lazy writes |
Delivered | CF completion inspected; deferred tests |
| Write tasks tile chunks/shards, allowing truncated last cells | Delivered | Alignment helper/tests |
| Incompatible source is rechunked before write | Delivered | Alignment/sharded-write tests |
| Lazy chunks inherit source unless shards supplied | Delivered | Chunks/shards default tests |
| Whole graph survives plain pickle | Delivered | Module-level completion; pickle test |
| Serial/threaded/distributed graph execution works | Delivered | Save scheduler tests |
| Consolidation follows all lazy writes | Delivered | Delayed dependency; ordering test |
| Finalisation consolidates from whole-store root | Delivered | Dataset/subgroup reconsolidation tests |
| Consolidation failure raises | Delivered | Exception handling/finalisation tests |
Default w- refusal is actionable |
Delivered | Dataset/Saver refusal tests |
Explicit w replaces |
Delivered | Mode tests |
a adds new arrays/groups without data append |
Delivered | Collision/add/refusal tests |
| Existing data-variable collision names array/group | Delivered | Pre-existing-name check/tests |
| Existing array core metadata is not resized/rewritten | Delivered | Creation/collision paths; add/refusal tests |
| Old array objects remain byte-for-byte intact after subgroup add | Unverified | Value evidence; exact byte comparison absent |
| Declining consolidation never leaves stale root document | Partial | Root test passes; subgroup reproduction fails |
| Interrupted saves make no rollback promise | Delivered | Saver/user-guide notes |
| Missing-chunk silent fill is pinned | Delivered | Integration format-behaviour test |
| Stale consolidated discovery is pinned | Delivered | Integration format-behaviour test |
| Dask locks relocate to CF | Delivered | File location; NC lock tests |
| Generic saver holds no backend lock/scheduler | Delivered | Source/write-handle delegation |
| Zarr multi-process coordination remains deferred | Delivered | API/source; present alignment safety |
| Native storage keywords reach arrays | Delivered | Encoding tests |
| NetCDF keywords are refused with replacement/advice | Delivered | Validator/refusal tests |
order and append are refused |
Delivered | Keyword/refusal tests |
| Consolidation defaults on; expected Zarr warning suppressed | Delivered | Defaults/filter/tests |
Dispatch, groups, meshes and deprecation aliases are delivered for covered cases; deep-group discovery and real remote access remain incomplete or unverified.
| Requirement | Status | Implementation and test evidence |
|---|---|---|
| Root group is default | Delivered | Dataset/Load/Save tests |
| Slash-separated group selects one group | Delivered | Group tests |
Group implies a unless explicit mode |
Delivered | Saver mode tests |
| Nested groups are not auto-loaded | Delivered | Dataset/Load tests |
| Empty-group error names data-bearing groups | Partial | Immediate groups only; deep-hierarchy reproduction |
| Local marker, not suffix alone, determines store | Delivered | Registry/ZarrStore tests |
| Remote supported scheme plus suffix determines store | Delivered | Scheme parametrisation |
| File handle rules out ZarrStore | Delivered | File element; basename-collision regression |
| URI expansion preserves remote URLs | Delivered | IO remote tests |
Stores use load_stores; files keep _load_files |
Delivered | Caller inspection; IO tests |
| Mixed file/store globs work | Delivered | _generate_cubes; mixed-glob test |
| Glob expansion happens once | Delivered | Caller trace to private file loader |
| Duplicate matches from different patterns survive | Unverified | Implementation supports it; dedicated assertion not identified |
| Saver detection accepts trailing separator | Delivered | find_saver; separator tests |
| CubeList saver registration uses function identity | Delivered | IO registry/tests |
| Both extension and explicit Zarr saver routes work | Delivered | Wrapper/self-registration; tests |
| Mesh loading reaches Zarr handler | Delivered | UGRID dispatch; load_meshes tests |
| Cubes rebuild mesh coordinates/connectivity | Delivered | Shared helpers; V2/V3 mesh tests |
| Mesh cubes round-trip through save | Delivered | Save integration tests |
| NCZarr fragments keep separate dispatch | Delivered | URI categorisation/format tests |
| S3 docs describe native loading/saving | Delivered | User guide/S3 tutorial |
| Iris adds no credentials/filesystem dependencies | Delivered | Source/requirements inspection |
| Actual remote connectivity/credentials | Unverified | Mocked routing; no network tests |
| Loader aliases are identical objects and warn on use | Delivered | NC forwarding/relocated-name tests |
| Saver constants/map aliases retain names and warnings | Delivered | NC forwarding/alias tests |
| NetCDF import stays quiet | Delivered | Deferred forwarding/import tests |
| Deprecations are marked 3.17 | Delivered | Docstrings inspected |
| Eventual NEP29 removal | Unverified | Future release obligation |
| NetCDF save/load/Saver/proxy/DEBUG stay in place | Delivered | NC exports/API/DEBUG tests |
cf_patch gets raw netCDF objects |
Delivered | Hook override/tests |
| Borrowed netCDF datasets retain ownership/string decoding | Delivered | Wrapping/reader/user-dataset tests |
| Restricted netCDF integer formats narrow safely | Delivered | Capability/dtype tests |
| NETCDF3_64BIT narrows or rejects overflow | Delivered | Restricted-format constant/dtype tests |
Shared Zarr write tests, real fixtures, benchmarks and merge-back work remain outstanding; release and external-process obligations were not verified.
| Testing/programme obligation | Status | Evidence or remaining work |
|---|---|---|
| Shared read contract runs on both backends | Delivered | NC/Zarr read subclasses |
| Shared write contract runs on both backends | Partial | NC runs it; Zarr subclass missing |
| Automated backend-type coupling check | Delivered | CF source check test |
| Pure decode unit tests | Delivered | Decode suite |
| Sharded partial read measures bytes with decode present | Delivered | Dataset sharding test |
| Sharded write corruption case | Delivered | Alignment tests |
| Realistic round trips cover coordinates/bounds/factories/methods/systems | Delivered | Save round-trip suite |
| Bidirectional NaN/finite/integer sentinel conformance | Delivered | Cross-reader parametrisation |
| Xarray conformance covers every supported output form | Partial | Small numeric/attribute cases, no exhaustive matrix |
| Xarray test-only in all three requirement files | Delivered | py312/py313/py314 requirement sections |
| All three locks include Xarray | Delivered | Lock entries inspected |
| V2 synthetic and NCZarr-written fixtures | Delivered | Dataset/integration Load tests |
| Store tests avoid remote fetching | Delivered | Construction/routing tests inspected |
| Missing Zarr adds no test failures | Partial | Package guards; unguarded new CF test |
| NOAA fixture preserves attributes/sharding/licence/DOI | Missing | Synthetic GFS-like fixture only in repository tests |
| ESA cropped fixture preserves hierarchy/codecs/flat and nested metadata | Missing | No contributed EOPF integration fixture/tests |
| Fixtures contributed to iris-test-data | Unverified | External contribution not audited |
| Zarr save ASV benchmark | Missing | No Zarr benchmark source |
| Zarr load/realise ASV variants | Missing | No Zarr variants found |
| Final prose sweep complete | Unverified | No complete main-to-branch prose audit |
| Separate PRs, labels, attribution and courtesy credit | Unverified | GitHub posts/history not audited |
| Historical relocations preserve baseline behaviour | Unverified | Current tests run; historical base execution absent |
| Feature PR bodies explicitly defer fragments | Unverified | Bodies not inspected |
| Merge-back has closing keywords/five credited fragment types | Missing | Deferred merge-back work |
| Docs build against main with warnings as errors | Unverified | No Sphinx build |
| Deferred loader/mesh-dispatch unification issue raised at merge-back | Missing | TODO exists; merge-back obligation remains |
| Superseded questions/status reconciled | Partial | Q4/Q10/API naming/PR status contradictions |
The focused suite passed; broader failures were a missing fixture and collection-dependent setup errors, not failing Zarr assertions.
I traced callers through URI categorisation, glob expansion, format selection, CF classification, coordinate attachment and mesh construction. For saving I traced both selection routes, attribute marshalling, creation, packing/filling, alignment, deferred completion and root finalisation. I inspected brownfield...HEAD changes rather than inferring delivery from commit descriptions or passing tests alone.
| Run | Result |
|---|---|
| Pure Zarr encode/decode | 86 passed |
| Zarr/CF/load-rule/IO units and Zarr integration | 1,475 passed, 2 skipped |
| Same selection plus netCDF units/integration | 2,243 passed, 23 skipped, 1 failed, 10 setup errors |
| String failure and coordinate-system module separately | 10 passed, 1 failed |
Tests used python -m pytest -q -o addopts=-ra, with caches under /tmp. The focused directories were lib/iris/tests/unit/fileformats/zarr, lib/iris/tests/unit/fileformats/cf, lib/iris/tests/unit/fileformats/nc_load_rules, lib/iris/tests/unit/io, and lib/iris/tests/integration/zarr. The broader run added lib/iris/tests/unit/fileformats/netcdf and lib/iris/tests/integration/netcdf.
The remaining failure was TestChunkedCharData.test_load: external fixture NetCDF/testing/chunked_char_data.nc was absent. The ten errors came from the coordinate-system module fixture not yielding when another collected module had already set its temporary-directory attribute. All ten passed separately. Skips included unavailable iris_grib, one FUTURE-context case and unsupported netCDF string combinations. The test-data override pointed to /data/home/martin.yeo/repos/iris-test-data/test_data.
Execution used Python 3.12.14, NumPy 2.5.2, Dask 2026.8.0, Zarr 3.4.1 and Xarray 2026.9.0. Local-store boundary scripts verified subgroup discovery, existing dimension redeclaration, object-dtype capability, deep-group diagnostics and integer fill disagreement. The optional-dependency check directly invoked the new CF test after blocking Zarr imports. An initial pytest-based simulation did not demonstrate the absence condition; the direct blocked-import invocation did. No actual package was removed.
I did not run pre-commit hooks, which can rewrite files, or a documentation build. I did not test remote credentials/connectivity, Zarr 3.0.8, real NOAA/EOPF data, benchmarks or a baseline checkout. No repository files were changed.
Contributor
⏱️ Performance Benchmark Report: 36f1d28Performance shiftsFull benchmark resultsGenerated by GHA run |
Contributor
⏱️ Performance Benchmark Report: 3e6c89dPerformance shiftsFull benchmark resultsGenerated by GHA run |
Contributor
⏱️ Performance Benchmark Report: 989c1baPerformance shiftsFull benchmark resultsGenerated by GHA run |
The new CF grid-mapping test imported the Zarr saver with nothing to skip on, so an Iris without zarr installed gained a test error -- the one thing section 6 of the design says adding Zarr must not do. The guard is per test rather than at the top of the module, because the module's other two tests are netCDF's: skipping those where Zarr is missing would trade a new failure for a silent gap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91b685bc-5a5a-4512-a687-b8bc67b9e3fe
create_dimension checked a new declaration against the declarations made in this session, and not against the arrays already in the group. Reopening a store whose "x" is two long and declaring "x" as three succeeded, leaving dimensions reporting one length for a name two arrays hold at two. Section 4.5's create-once rule means the array that is already there is never resized, so there is no outcome where both sizes are true. It now checks `dimensions`, which is the declarations and the arrays both. The saver already renames its own dimension when it meets a collision; this is what lets it see one. The mapping is updated rather than dropped afterwards, because dropping it reassembled it from every array in the group on the next declaration, over a store that may be remote. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91b685bc-5a5a-4512-a687-b8bc67b9e3fe
Two places where the Zarr dataset said one thing and did another. A fill value was cast to the array's dtype for storage and written to '_FillValue' unconverted, so an int16 array asked for fill_value=1.5 stored 1 and declared 1.5. Iris read that back correctly, because it narrows a declared sentinel to the stored dtype before comparing, which is what hid it; xarray compares against the attribute and would have masked nothing. Section 4.5 asks for one value in both places, so a sentinel the dtype cannot hold is now refused. A floating sentinel may still lose precision, and must: every float32 fill written as a Python float is a float64. supports_dtype answered True for every dtype, including the ones create_variable then refused -- object arrays, and the extended precisions NumPy sizes by platform. Section 4.2 says it answers for what Zarr has a data type for, and now it does. create_variable asks it rather than letting zarr-python raise, so the two cannot disagree again, and the refusal is in Iris's words. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91b685bc-5a5a-4512-a687-b8bc67b9e3fe
Saving into a subgroup with consolidate_metadata=False left the root's consolidated document untouched. A default reader believes that document, and it does not mention the group just written: the cubes were in the store and invisible. The user guide already promised the document was removed, and nothing removed it. The root-level case passes today only by side effect -- writing an array at the root makes zarr-python rewrite the root metadata, and the consolidated section goes with it -- which is why the existing test did not see this. A promise kept by side effect is kept only where the side effect reaches. ZarrDataset gains drop_consolidated_metadata, the other half of finalise, working from the whole store's root as finalise does. The saver's completion now calls one or the other, rather than one or nothing, so it waits for the data under compute=False like the consolidation it replaces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91b685bc-5a5a-4512-a687-b8bc67b9e3fe
Section 6 asks for one test body run against every CFDataset implementation, and the write half had only netCDF's subclass. This module's own docstring said the Zarr half arrives with the saver, which it now has. It passes unchanged: the refusal of an unlimited dimension is the one answer the contract sanctions two of, and Zarr gives the other one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91b685bc-5a5a-4512-a687-b8bc67b9e3fe
The review read the specification as current and found four places where it contradicts itself. Each is a change made in section 4.2 or in the decision log and never carried through to the sections that repeat it. The CFDataset member renamed storage_settings on 2026-10-20 kept its old name, `encoding`, in sections 5 and 6, where it names no member of anything. The rename was never logged at all, which is what let it rot. Section 6 still asked for the read-side alignment guard that was removed the same day: that dask chunks tile the store grid in every case, and that a chunk larger than the target is not subdivided. Both now say what the loader does, and what its tests assert -- including that a pinned CHUNK_CONTROL setting is honoured without a warning, because a caller who pins has already taken the trade. Section 6's skipping rule described one importorskip per Zarr test package, which no longer covers everything after a test outside those packages came to exercise the Zarr backend. Q10 was closed into the decision log on 2026-10-16 but its row was left standing in the open questions, still saying the NETCDF3_64BIT defect was preserved rather than fixed. Removed; a question is closed by moving it, and half a move leaves the table disagreeing with the log. Q4 is closed here. It asked whether the netCDF read path should get the same alignment guard as Zarr, and there is no such guard on either path any more, so the asymmetry it existed to resolve cannot arise. Also records PR 4 as merged, and what this review found in the code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91b685bc-5a5a-4512-a687-b8bc67b9e3fe
The first fix listed the dtypes to exclude, and review found two more it had missed: a structured dtype with an object field, and a zero-length void. Both answered True and then failed in create_variable. A list of exclusions is the wrong shape of answer, and the version floor proves it. zarr-python 3.1 replaced a fixed enum of fifteen data types with an extensible registry, so 3.4 stores a structured dtype and a datetime64 that 3.0.8 -- the oldest Iris accepts -- has no data type for. Any list written against the installed version is wrong on the declared one. So the answer now comes from a table of kinds and item sizes: bool, the integers and unsigneds, floats to 8 bytes, complex to 16, and fixed-length text and bytes of non-zero length. That is the intersection across the versions Iris accepts, which makes a few answers conservative -- datetime64 is False although zarr-python 3.4 would store it. Conservative is the safe direction, because create_variable asks this before it creates anything: a False is a refusal in Iris's words, while a True the store goes back on is the thing section 4.2's rule exists to prevent. Byte order stays out of the answer, although 3.0.8's table is little-endian only. A PP or GRIB load yields big-endian arrays, and a False there would send a big-endian int64 into the saver's narrowing path on every version, including the ones that would have stored it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91b685bc-5a5a-4512-a687-b8bc67b9e3fe
The header said PR 4 was in review and was the next action, while §12.1 recorded it as merged. That is the second time the header has gone stale against §12.1, so it now carries the rule as well as the summary: it is a summary of §12.1 and nothing more, and a reader who stops at the header must not be told something §12 contradicts. §4.2 gains what supports_dtype answers from, and why: a table of what Zarr has rather than a list of what it lacks, the intersection across the zarr-python versions Iris accepts rather than what the newest holds, and byte order left out of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91b685bc-5a5a-4512-a687-b8bc67b9e3fe
stephenworsley
requested changes
Oct 23, 2026
| @@ -662,17 +662,39 @@ def cache_key(self) -> str: | |||
| #: makes that case safe. | |||
| _EXACT_FILL_KINDS = "biu" | |||
Contributor
There was a problem hiding this comment.
This would be clearer as a list rather than a string. Iterating through a string seems like bad code smell to me.
supports_dtype answered True for a big-endian dtype, which zarr-python 3.0.8 has no data type for at all: `>i8` raises KeyError. The last round found this and recorded it as an acknowledged exception rather than fixing it, on the grounds that answering False would be worse. That much was right. supports_dtype is consulted for int64 and unsigned, so a False would have narrowed big-endian int64 data, and create_variable -- which now asks before it creates -- would have refused big-endian floats outright. A PP or GRIB load produces both, so Iris would have stopped saving ordinary cubes. But the choice was never between those two. create_variable now creates in the platform's own byte order whatever order it is handed, and NumPy converts on assignment: the values stored are the caller's and only the layout is the store's. supports_dtype can then answer True for `>i8` on every supported version, because every supported version can keep it. This is what the netCDF saver has always done with the same data -- a `>f4` cube is float32 in the file it writes -- so the Zarr path is matching it rather than adding a second rule. Checked by running it, not by reading zarr-python's source: zarr-python 3.0.8 installed in a throwaway environment, and every dtype the table answers True for created in turn, on 3.0.8 and 3.4.1 both. Every one creates on both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91b685bc-5a5a-4512-a687-b8bc67b9e3fe
trexfeathers
commented
Oct 23, 2026
trexfeathers
left a comment
trexfeathers
left a comment
Contributor
Author
There was a problem hiding this comment.
(posted by GPT)
I am satisfied that findings 1–4, 6 and 7 from my specification audit are addressed at df4f443307dd460f2c14829199189abd5fd488cc. Finding 5 remains deferred to @trexfeathers as agreed. This closes those findings; it is not a claim that the whole programme’s specification is delivered.
The final byte-order change resolves the remaining capability concern. I ran the actual Iris dtype and byte-order tests against Zarr 3.0.8, including the big-endian cube round trip. Array creation normalises byte order and preserves the values.
Two separate test assertions still fail on Zarr 3.0.8: U5 and S5 create successfully, but their reported dtypes are StringDType() and object, while the tests expect the original fixed-width dtypes. Those assertions need reconciliation with the supported minimum version. They do not reopen the big-endian finding.
Validation: 465 regression tests passed on Zarr 3.4.1; minimum-version tests passed except for two string dtype assertions
| Environment | Tests run | Result |
|---|---|---|
| Zarr 3.4.1 | Zarr dataset and saver unit tests, CF dataset unit tests, Zarr save integration tests | 465 passed |
| Zarr 3.0.8 | Dtype capability and byte-order unit tests, big-endian cube round trip | 64 passed, 2 failed; both failures are the string dtype assertions described above |
The specification corrections were checked by inspection. No repository files were edited.
lib/iris/AGENTS.md puts prose that explains a subsystem in the module docstring, holds a comment to three lines, and says a fact a test can pin belongs in that test with one line in the source naming it. Three of the comments added with the dtype work were essays instead: twelve lines on byte order at the assignment, and twenty on the constant. What Zarr can hold and how that varies by zarr-python version is now a fourth trap in the module docstring, beside the three already there. The constants carry a label each, create_variable keeps one line pointing at the docstring and at TestByteOrder, and the docstrings lose the sentences that defended the design rather than stating it. No behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91b685bc-5a5a-4512-a687-b8bc67b9e3fe
docs/AGENTS.md: published prose must never cite an artefact of how the work was done -- a review finding, a task or plan number, a phase. The decision log and the history table had entries turning on "the third round of the same finding" and "the second round", which resolve to nothing for anyone who was not there. Each now states the change and leaves the dates and the pull request numbers to anchor it, as the surrounding entries already do. No design change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91b685bc-5a5a-4512-a687-b8bc67b9e3fe
The mesh paragraph in the saving section claimed that loading a store gives the mesh back. That sentence replaced a caveat saying loading did not yet rebuild the mesh, and outlived the reason it existed: the loading section states the same fact three lines from the top. Removed. The note about a Zarr attribute netCDF cannot hold moves from the mesh subsection, where it has nothing to do with meshes, to the loading section, where the cube it describes comes from. _EXACT_FILL_KINDS and _STORABLE_TEXT_KINDS are frozensets rather than bare strings, matching _TEXT_KINDS in zarr/saver.py. _STORABLE_TEXT_KINDS gains "T", so the two agree on what text is: NumPy's variable-width StringDType creates on zarr-python 3.0.8 and 3.4.1 alike, and supports_dtype had been answering False for it. The dtype contract test asserted that a created array reports the dtype it was given, which holds for the numeric kinds but not the text ones: zarr 3.0.8 reports StringDType() for U5 and object for S5, where 3.4 reports the fixed width. For text it now asserts what both versions promise, that the values written come back. Verified on both: 156 passed in the dataset write tests against zarr 3.0.8, and the Zarr and CF suites pass against 3.4.1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 76a82181-7fe1-4063-9fd7-deb10fb0d658
trexfeathers
commented
Oct 23, 2026
Contributor
Author
There was a problem hiding this comment.
Not relevant to this file specifically, but I wanted somewhere to start a conversation thread:
Testing on this PR has revealed that Iris does not support Zarr down to version 3.0.8 as claimed. The code was written against 3.4.1. 3.0.8 was chosen as the 'floor' due to a known bug with older versions. Options:
- Raise the minimum supported version to the very latest version of Zarr, the one that the code was written against
- Additional work to support 3.0.8 properly
- Here
- Another PR
Contributor
⏱️ Performance Benchmark Report: 91c9407Performance shiftsFull benchmark resultsGenerated by GHA run |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(posted by Claude Opus)
This is the sixth of nine pull requests delivering native Zarr support for Iris, and the subject of #6980:
iris.save(cube, "out.zarr")now writes a Zarr version 3 store, withcompute=Falsereturning a realdask.delayed.Delayed, Zarr's own storage keywords (chunks,shards,compressors,filters,serializer), agroup=keyword,consolidate_metadata, and the CF masking, packing and fill-value handling that zarr-python does not do for free. Part of #6980. If any of that arithmetic is wrong the store still opens cleanly and reads back the wrong numbers rather than raising, so the masking and packing logic inzarr/_encode.pyandZarrDatasetVariable.__setitem__, and the fill-value declaration inzarr/saver.py's_backend_write, are where I would look first.Public API added.
iris.fileformats.zarr.saveandiris.fileformats.zarr.Saver, andiris.savenow reaches them for any target ending.zarr, local or remote, through a new"zarr"entry iniris.io._savers.The base. This work was built on
zarr-io-pr-4-5-base, a branch merging PR 4 (#7336) and PR 5 (#7338) into one tree, because it both edits PR 4'szarr/_dataset.pyand subclasses PR 5's generic saver. Both predecessors have since merged, and this branch is now rebased ontobrownfieldwithgit rebase --onto upstream/brownfield zarr-io-pr-4-5-base zarr-io-pr-6, so the diff below is this pull request's own work and nothing else. An earlier preview of it ran as trexfeathers#100, based on that merge branch so that its diff showed only this work; the review comments there are answered in the commits here.What the rebase changed, over and above replaying the commits. Three things landed in
brownfieldwhile this was being built, and meeting them is a commit of its own at the head of the branch. The generic saver is_CFSaver, renamed and made private at review of #7338, so every reference here follows it.iris.iono longer decides which savers take a wholeCubeListby module name: it holds the saver functions themselves, in_savers.cubelist_savers, soiris.io._zarr_saveis registered there beside the netCDF saver andiris.fileformats.zarr.saveregisters itself at the foot of its own module — the only place it can be named by identity withoutiris.ioimporting zarr to do it, and both routes a caller can take are covered by tests. And UGRID meshes now load from Zarr, added to #7336 on 2026-10-22, which turned the mesh round-trip test from a strictxfailinto an ordinary passing one.No changelog fragment. This programme writes its changelog fragments once, at the merge-back into
main, not on the feature branches that land onbrownfield— the same rule PRs 1 through 5 followed.xarray is a new test dependency.
xarrayis added to the test section ofrequirements/py3{12,13,14}.yml, for the cross-reader conformance tests — xarray opens what Iris writes, and Iris loads what xarray wrote, which is how a private encoding decision gets caught. All three lock files are regenerated to match, so CI runs those tests rather than skipping them. The regeneration is its own commit, and deliberately late: #7336 regenerated all three lock files forzarr, and doing it again while that was outstanding would have guaranteed a conflict and a second slow environment rebuild on the rebase.The first of two changes that touch the netCDF path, and why it does not touch a netCDF file.
_CFSavergains a fifth hook,_grid_mapping_writer, so the CF grid-mapping parameters can reach a non-netCDF store at all — the base implementation writes them through.attributes, which is what Zarr gets.netcdf.saver.Saver's override returnscf_var.variable, the samenetCDF4.Variableobject those parameters have always been assigned onto, so what lands in a netCDF file is unchanged: the override is an identity, not a behaviour change. The ASCII-to-bytes coercion that object still bypasses is Q8 in the design document, unblocked by this hook but not closed by it.The second, and the one to look at hardest: a block moved out of
netcdf.saver.save. Deciding which of a cube's attributes a store can hold globally is cube logic, not storage logic, but it was written insidenetcdf.saver.saveand nothing else could call it — so on the Zarr path every cube global attribute was simply dropped. Withiris.FUTURE.save_split_attrs = True, the genericwritedeliberately writes no globals and leaves them to the caller: netCDF wrote them, Zarr did not, andtitle,institution,history,sourceandreferencesvanished from a Zarr save. In the legacy default mode it is thelocal_keyscomputation that was missing, so aCubeListwhose cubes disagreed about an attribute kept only the last cube's value where netCDF gave each data variable its own. The block is nowcf.saver.marshal_attributes, called by both savers. The extraction is behaviour-preserving — its only edit is that the legacy deprecation message takes the format name, so netCDF passes"netcdf"and gets back the exact string it had — andlib/iris/tests/integration/test_netcdf__loadsaveattrs.pygives 2059 passed before and 2059 passed after, as does the whole netCDF suite. One thing did change, and it is worth saying so nobody discovers it in a traceback: those warnings now reportlib/iris/fileformats/cf/saver.pyas their source rather thanlib/iris/fileformats/netcdf/saver.py. Their text, category and count are unchanged, and nothing in the repository filters on the location.One consequence for a maintainer to rule on. Because the Zarr saver now shares that marshalling, a legacy-mode Zarr save emits the same attribute-handling deprecation netCDF has emitted since 3.8 — consistent with netCDF, and correct in that the legacy mode is deprecated for every format, but it does mean a brand-new format ships warning on its default path, since
save_split_attrsstill defaults toFalse. Suppressing it for Zarr alone would mean a flag on the shared function whose only purpose is to hide a deprecation from one format, which is the divergence the extraction exists to prevent. Left as it is, deliberately, for an owner to decide.Packing needed its own implementation here, not a reuse of netCDF's. The generic saver's
packing=support setsscale_factorandadd_offsetas attributes and streams the data through unchanged, because netCDF4-python applies that transform itself the moment the attributes are set on a variable it is about to receive data for. zarr-python has no such behaviour: left alone,packing=on a Zarr save would have set the attributes correctly and then written physical-unit values straight into a narrower, usually integer, stored dtype — numbers with no relationship to what a reader would reconstruct from them.zarr/_encode.pynow has apackfunction, the write-side mirror of the existing_decode.unpack, andZarrDatasetVariable.__setitem__applies it before the masked fill, because the fill value lives in packed space and packing an already-filled sentinel would send it through the same transform as real data. The Zarr saver also refuses a non-integer packing dtype, matching the check and the messagenetcdf/saver.pyalready has.What this does not do.
mode="a"adds new arrays to an existing store; it does not match an incoming coordinate against one the store already holds, so saving a second cube with its ownlatitudewrites a second array,latitude_0, rather than reusing the first, because deciding when two coordinates are "the same" across unequal values, differing units and different calendars is its own piece of work. And a masked string variable is refused rather than written: the only sentinel a Zarr string array carries is the empty string, which is also a legitimate label, and Iris's own loader does not restore a mask from one — so writing it would lose the mask in silence. The netCDF saver cannot store a masked string coordinate either, so this is parity with it rather than a step back from it.The full suite.
pytest lib/iris/tests/unit lib/iris/tests/integration -q -n auto, withNPY_DISABLE_CPU_FEATURESset as the repository's own instructions ask, on the rebased result: 11485 passed, 59 skipped, nothing failing, 239s.pre-commit run --all-filesis clean.