Repository navigation
Zarr I/O PR 4: native Zarr loading - #7336
Merged
Merged
Conversation
Zarr loading. Sections through "Findings that refine the spec", which records seventeen points checked against zarr-python 3.4.1 while the plan was written -- including the answer to the spec's open question Q2, and three places the spec needs amending. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91122c91-913f-44c8-841f-f69ed66ac830
Every file created or modified, the order the tasks run in and why, and the signatures of ZarrDataset, ZarrDatasetVariable, the decode functions and the load-chain additions -- so that each task below can be read on its own without guessing a neighbour's names. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91122c91-913f-44c8-841f-f69ed66ac830
Task 1 makes zarr optional and proves Iris imports without it. Task 2 adds format and encoding to the CF dataset interface and splits the shared contract into read and write halves. Task 3 presents a Zarr array through the interface, which is where the dimension contract lands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91122c91-913f-44c8-841f-f69ed66ac830
Task 4 assembles CFDataset.dimensions from the arrays and decides what a name used at two sizes means. Task 5 asks the shared read contract of both Zarr versions, and adds the check that the cf package names no implementation class. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91122c91-913f-44c8-841f-f69ed66ac830
Task 6 implements masking, valid-range masking and unpacking as pure functions, with the version-aware fill-value precedence and both _FillValue encodings. Task 7 is the lazy read: no proxy, chunking rounded up to the store's own, and a cache key that carries the array's metadata. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91122c91-913f-44c8-841f-f69ed66ac830
Task 8 is the one change a netCDF file could notice, so it runs its suite on both backends. Task 9 lets CFReader take an already-open CF dataset of any format. Task 10 builds cubes, and decides what a group holding no data variables says to the user. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91122c91-913f-44c8-841f-f69ed66ac830
Task 11 recognises a store -- by a marker file locally, by scheme and suffix remotely, and never by the suffix alone. It also has to say in code that an NCZarr URL is still NCZarr's, because the store it names is real. Task 12 is the chain: load_stores, the partition after glob expansion, and the two saver-side changes section 4.7 asks for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91122c91-913f-44c8-841f-f69ed66ac830
Task 13 loads realistic stores both ways round -- version 2 through Iris's own NCZarr writer, version 3 through a fixture shaped like the NOAA GFS archive. Task 14 documents loading. Task 15 updates the spec, including closing its open question about fixed-length unicode. Sections 6 and 7 say what done is measured against. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91122c91-913f-44c8-841f-f69ed66ac830
Review Focus now points at the tasks it means. The interface section's _align_chunks and decode_parameters signatures match what the tasks write. Adds F18, which is the gap the review found: this pull request creates the first way to get a nested attribute onto a cube, and the netCDF saver cannot write one -- so Task 10 characterises it and Task 14 documents it. Also adds the version-aware masking tests at the loaded-cube level, the async-concurrency note the spec asks for, and a step to read the generated API pages. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91122c91-913f-44c8-841f-f69ed66ac830
The netCDF contract subclass lives in test_NetCDFDataset__contract.py, not test_NetCDFDataset.py. The sequencing paragraph now uses the task numbers the tasks ended up with, and the file table lists every test module the tasks create. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 91122c91-913f-44c8-841f-f69ed66ac830
The spec review merged as SciTools#7330. Its tree is identical to the spec branch the plan was written against, so every [verified] claim stands unchanged; only the baseline pointer moves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 436a543d-0543-48a7-92b3-f8bef5d37d8a
PR 3 is still in review, so PR 4 is cut from its branch rather than waiting for brownfield. The base branch of a pull request has to live in the repository it is raised against, and zarr-io-pr-3 is on the fork, so the draft is raised fork-internally: its diff is PR 4's work alone instead of PR 4 plus PR 3's ten commits. Task 15 gains a sixth step for when SciTools#7316 merges - rebase onto brownfield with the three-argument form, re-run the two tasks that read PR 3's code, re-compare the full suite against brownfield, raise the real pull request and close the preview. The full-suite baseline moves to zarr-io-pr-3 for the same reason: it is the only comparison that isolates this pull request's effect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 4aeb6748-437c-4241-b20c-c5e1be85150e
The package holds only its docstring for now: iris.fileformats does not import it, so Iris still installs and imports without zarr-python. The test package skips itself when the dependency is absent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
test_optional_dependency.py now checks only what its name says: Iris importing safely without zarr-python. The two tests proving make_store builds both Zarr format versions move to test_stores.py, named for the module they exercise, with their imports at the top where they belong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
format names the concrete format for a message or a diagnostic; encoding reports the storage settings a store actually holds, readable but never interpreted. Splits the shared contract into its read and write halves, so that an implementation arriving in two pieces can be held to the first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
close, closed and the context manager are read-side behaviour per spec section 4.2, not write-side: a read-only implementation must still answer them correctly, and the whole point of splitting the contract is that a read-only Zarr store gets checked against every read-side question. Moving these three tests to the write half, as the first cut did, would let that divergence through unchecked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
The one-line summary copied from the plan's interface sketch ran to 91 characters; nothing in the pre-commit toolchain catches an over-length docstring line (E501 is disabled repo-wide, and W505 needs a max-doc-length the repository does not set). Splitting it into a short summary plus an extended description also fixes it ahead of the Zarr docstrings a later task writes from the same sketch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
The dimension contract is the substance: a Zarr dimension may be unnamed, and an unnamed one is anonymous rather than an error. _ARRAY_DIMENSIONS is storage, so it is withheld from the CF attributes, and fill_value is reported as the storage fact it is rather than as a missing-value flag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
Dimensions are assembled from the arrays, and a name used at two sizes is omitted with a warning rather than merged: Zarr places no restriction on reusing a name across arrays, so a store meaning two things by one name is conforming, and merging them would produce a wrong cube. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
The inconsistent-dimension test checked the warning and the mapping's omission but not that the two arrays still report the name themselves, which is the fact that lets a later (name, size) match tell them apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
Masking, valid-range masking and scale/offset unpacking, as pure functions of values. The two rules worth reading twice: a floating _FillValue is base64 on version 3, which is xarray's convention rather than a defect; and Array.fill_value is a mask source on version 2 only, and only when the variable declares no CF attribute of its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
decode_fill_value narrowed a decoded sentinel to the variable's dtype unconditionally, so a value that overflowed it - signed, negative or unsigned - raised OverflowError out of the whole load instead of warning and degrading, same as any other undecodable sentinel. Also adds the end-to-end test for the rule the module exists for: a version 3 array of zeros with no CF attributes comes back through decode_parameters and mask_and_scale with its zeros intact, not just with an empty sentinels tuple. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
No proxy: a zarr.Array is picklable and a store is a namespace, so the array goes to as_lazy_data directly and CF decoding is a visible map_blocks layer. Chunking is rounded up to whole multiples of the store's own, never below, because reading any part of a Zarr chunk fetches the whole object. The Dask cache key carries the array's metadata document rather than its address, which alone goes stale when a store is rewritten in place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
The cache test compared two Dask names, which Dask makes equal by deterministic tokenisation whether or not a cache key is passed, so it passed with the key removed from read_data entirely. It now replaces the process-wide cache and asserts the first read fills it and the second reuses it. Chunk alignment warned whenever rounding up overshot the array extent and the extent did not tile the store grid. Capping at the extent leaves one block spanning the dimension, which reads each store chunk exactly once, so nothing was subdivided and the warning was false - on an ordinary daily (365, 500, 1000) store, on every load. The two conditions that do mean something, an oversize store chunk and a subdivided fixed dimension, are kept and now have tests saying so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
Both Zarr versions answer the same questions the netCDF implementation does, so a divergence between the two fails here rather than somewhere in the loader. Adds the check that no module in the cf package names an implementation class. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
Zarr places no restriction on reusing a dimension name across arrays, so a name alone can link two variables that mean different things by it. An unnamed dimension is anonymous and matches nothing, including another anonymous dimension. netCDF names are unique within a group, so this is a widening that no netCDF file can observe. Two pre-existing unit-test doubles modelled `dimensions` but not `shape`, which the old bare-name `spans()` never read and the new pair-based one does: `_NetCDFVarWithDimensions` (shared by the CFBoundaryVariable, CFClimatologyVariable and CFLabelVariable span tests) gains a `shape` property derived from a deterministic per-name size map, and the two `test_CFVariable.py` cases that compared against a bare `MagicMock` now compare against the same concrete stub on both sides - a MagicMock collaborator was auto-answering the new `_dimension_identities()` call with something that iterates as empty, silently flipping an assertion instead of failing loudly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
A loader that has opened a store hands it straight over, so the choice of backend stays with the loader rather than becoming a branch inside the reader. A borrowed dataset is not closed by the reader that borrowed it. Also wires the canonical Zarr test store's data variable to its grid mapping variable by name, which CF classification needs to recognise the grid mapping variable as one at all - it was present but unreferenced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
Open the store, hand it to CFReader, and run the same cube-building loop the netCDF loader runs. One group per call: a store can hold unrelated datasets, so a group with no data variables names the groups that have some rather than returning nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
Locally by the marker file a store puts at its root, remotely by the URI scheme and the .zarr suffix. Not by the suffix alone: that would claim any directory someone named .zarr, and joined with the remote rule it would leave the scheme test unreachable. An NCZarr URL is still NCZarr's, which needs saying in code because the store it names is a real one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
test_the_agent_still_picks_netcdf_for_a_netcdf_file opened a real file with no skip_data guard, so missing test data raised FileNotFoundError instead of skipping - the opposite of lib/iris/tests/AGENTS.md's "missing data skips affected tests by design, silently, and without failing". Follows the same @_shared_utils.skip_data idiom already used in test_load_meshes.py. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4
Contributor
⏱️ Performance Benchmark Report: 8421ac9Performance shiftsFull benchmark resultsGenerated by GHA run |
Contributor
⏱️ Performance Benchmark Report: 8421ac9Performance shiftsFull benchmark resultsGenerated by GHA run |
Contributor
⏱️ Performance Benchmark Report: 8421ac9Performance shiftsFull benchmark resultsGenerated by GHA run |
Contributor
⏱️ Performance Benchmark Report: 8421ac9Performance shiftsFull benchmark resultsGenerated by GHA run |
Contributor
⏱️ Performance Benchmark Report: 8421ac9Performance shiftsFull benchmark resultsGenerated by GHA run |
pp-mo
requested changes
Oct 19, 2026
pp-mo
left a comment
pp-mo
left a comment
Member
There was a problem hiding this comment.
As discussed, a preliminary set of change requests (mostly minor) and queries.
I think some of my comments on the docs could be superceded by a general attempt to improve the style and user-focus of the documentation -- e.g. explain that docs are for users not developers, should only cover practical matters, and should avoid describing minor details and edge-cases if they are complex in content and confuse the general discussion.
Meanwhile I am still working on more of the main code, and haven't really started with the testing.
Review on SciTools#7336 asked for the user manual to carry practical matters and leave the rationale out. Cut the sections that only explained why, kept the ones a reader hits, and corrected three statements that were wrong. Dimension names: `_ARRAY_DIMENSIONS` is read, so version 2 dimensions are named. The old "does not pass it on to the cube" meant only that the attribute is withheld from `cube.attributes`, and read as though the dimensions came out anonymous. Dimension names: a name used at two lengths does not reach the user as the warning the text promised. `ZarrDataset.dimensions` is a lazy property and the CF layer reads per-variable `dimensions` instead, so nothing computes it during a load. What a user does see is the orphaned coordinate in LOAD_PROBLEMS, so say that. Attributes: keep the netCDF save failure, including the error text verbatim, because the netCDF library names the attribute and not the cube and a user has nothing else to search for. Trimmed to the symptom and the fix, and pointed at it from Saving. Dropped the `_NCProperties` section, the version 3 justification for ignoring the storage fill value, and the 9.96921e+36 example. All of that is in the module docstrings of `_decode.py` and `_dataset.py`, where a developer looks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: a1b138dd-45fa-4794-b53d-6b56018d4128
A dimension name used at two different lengths warned from `ZarrDataset.dimensions`, a lazy property. A load never read it: the CF layer works from each variable's own `dimensions`, so nothing assembled the mapping and the warning reached the unit tests and no user. Spying on `_assemble_dimensions` through `iris.load` of a conflicting store counted zero calls. Assembling in `__init__` costs no extra reads, because `variables` is materialised by everything that opens a store for loading, and it removes the failure mode rather than papering over it: a warning raised from a lazy property nobody triggers is unreachable by construction. Without the mapping the clash still reaches the user, but only as the orphaned coordinate in LOAD_PROBLEMS, which names the coordinate and not the cause. The warning is the only thing that says why. The unit test passed either way, because it read the property itself. Added an integration test that loads a conflicting store through `iris.load`, which is the path that was broken. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: a1b138dd-45fa-4794-b53d-6b56018d4128
`_DimensionIdentity` replaces the pair-or-sentinel that `_dimension_identities` returned, so the function has one documented element type instead of `tuple[str, int] | _AnonymousDimension`. Equality and hashing are now written out: a named dimension compares on `(name, size)`, an anonymous one on identity. `set.issubset`, `in` and `tuple.index` all reach them, so they are pinned by their own tests. `_store_chunk_exceeds_dask_target` moves above `_align_chunks`, its only caller. `ugrid_load` had two branches with identical bodies, one listing http schemes and one `REMOTE_STORE_SCHEMES`. They are one branch over the union now. The matching pair in `iris.loading` cannot merge the same way: those two call different `iris.io` entry points, one of which takes no constraints. `ZarrDataset.attributes` asserts the group is open before building `_ZarrAttributes`, matching `encoding` next to it, and `_ZarrAttributes` takes a typed node and says in its docstring that writes reach the store. `_dimension_names` keeps two branches rather than gaining a third for an unsupported version. zarr-python parses `zarr_format` as a `Literal` and raises before Iris sees the node - `expected Literal[3], got 4` for an array in a version 3 group - so a third branch would be unreachable. Said so at the `else`, and pinned the guarantee with a test, which is what fails if zarr-python ever widens it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: a1b138dd-45fa-4794-b53d-6b56018d4128
`iris.loading._generate_cubes` and `ugrid_load.load_meshes` each defined an identical `_categorise` closure, so "what counts as an NCZarr target" was written twice in two modules. It is `iris.io._categorise_uri` now, with its own tests. `decode_uri` keeps its signature. It is public, documented with doctests, and a native Zarr store cannot be categorised from the URI anyway: remotely it is the scheme, locally it is a directory that only a marker file tells from any other, and reading that marker inside a string function would mean a stat per decode. `REMOTE_STORE_SCHEMES` becomes `_REMOTE_STORE_SCHEMES`. Neither the design document nor the plan argues for it being public; it was written without an underscore and nothing outside Iris reads it. `_dimension_names` now names both versions it reads and raises on anything else. zarr-python rejects such an array first, so the branch is reached only through a stand-in in the tests, but naming 2 and 3 in the code is what a reader new to Zarr needs - the previous `else` asked them to already know there was no third option. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: a1b138dd-45fa-4794-b53d-6b56018d4128
trexfeathers
commented
Oct 19, 2026
Iris sized Dask chunks on the Zarr path and then rounded them back up to whole multiples of the store's chunking. That is gone. Chunking is now `_optimum_chunksize` and nothing further, the same as netCDF. The measurements supported the alignment, so this is a proportionality call rather than a correction. Against a 33.6 MB store chunk divided into quarters, aligning held lower peak RSS at every worker count above one (305 MB against 446 MB at four workers), ran in half the time, and fetched each store chunk 1.0 times against 3.2. What decided it is that alignment only ever changed the answer in one case. Wherever `_optimum_chunksize` expands it already lands on a whole multiple, so alignment is a no-op; the only case it altered was a stored chunk larger than Dask's target. netCDF passes the file's own chunking into that same function and has always accepted the division, so correcting it for Zarr alone made two formats behave differently over physics they share, and made CHUNK_CONTROL mean different things depending on what was loaded. The warning alignment carried is kept, because it is what makes leaving the choice to the user a fair one: it says the situation exists and names the tool. It lives in `iris.fileformats.cf.loader` now, on the path both backends take, and stays quiet when the caller has pinned a chunking -- they have already made this decision. New tests run it over netCDF and Zarr so the two cannot drift. `test__align_chunks.py` goes. The read_data test that pinned rounding-up is inverted to pin the division, with the reasoning in its docstring. Recorded in spec section 4.4 and the decision log, which reverses the 2026-09-22 entry that added alignment, and in the plan's record of what changed during implementation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: a1b138dd-45fa-4794-b53d-6b56018d4128
pp-mo
requested changes
Oct 20, 2026
PR 5 (SciTools#7338) merged first, so this takes the resolution its plan laid out for whichever of the two siblings landed second. The three predicted collisions: - `CFDataset.format` and `NetCDFDataset.format` arrived on both branches. One copy kept, PR 5's wording, with PR 4's `encoding` beside it. The variable half of `format` and `encoding` is PR 4's alone and is untouched. - `ZarrDataset.supports_dtype` is implemented here, as PR 5's plan said PR 4 would have to: `True` for every dtype, because Zarr's data types cover the whole fixed-width numeric range in both versions. Spec section 4.2. - The shared `CFDataset` contract merged cleanly into a duplicate `test_format_is_a_non_empty_string`; one copy kept, and PR 5's `test_supports_the_contract_dtype` moved to sit beside it in the read half PR 4 split out. Both halves now run against Zarr as well as netCDF. Two more conflicts outside that list. The `cf/_reader.py` module docstring: brownfield reworded the paragraph saying a Zarr store could be read "once a CFDataset implementation exists for it", which this branch supersedes by being that implementation. And the spec, where both sides had edited the status table, the pull request table and the change log; those are merged to what is now true, with SciTools#7336 recorded as in review and SciTools#7338 as merged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7310b86f-6389-4b3a-941a-c5983b1e287e
`encoding` on the `CFDataset` interface becomes `storage_settings`, on both the dataset and the variable, and `create_variable`'s `**encoding` with them. Iris already uses "encoding" for character encoding -- `EncodedVariable`, `VariableEncoder`, the `_Encoding` attribute, and a test fixture of that name -- so the member was the odd one out. Neither class is in `iris.fileformats.cf.__all__`, so nothing public moves. `_ZarrAttributes` no longer documents which attributes are withheld. That is the caller's decision, made where the mapping is built, and both constants already carry the reason; the class docstring was a second copy that could go stale on its own. `_CUBELIST_SAVER_MODULES` becomes `_savers.cubelist_savers`, holding the saver functions rather than module-name strings. Identity is what the check actually wants: `save` is also reached with a saver that was never looked up by extension -- one the caller passed, and the one chosen directly for an NCZarr target -- and `netcdf.save` has no `append` parameter for those to fall back to. `_check_init_savers()` is called before the check so the registry is populated on those paths too. The test that pinned the module set moves to `test_save.py`, which is where `iris.io.save` is tested. Zarr tests use `mocker` rather than `monkeypatch`, which is what the developer guide asks for and what 151 of the 168 test files that mock already do. Every use here was a plain attribute swap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: a1b138dd-45fa-4794-b53d-6b56018d4128
Contributor
⏱️ Performance Benchmark Report: 219ec96Performance shiftsFull benchmark resultsGenerated by GHA run |
Contributor
⏱️ Performance Benchmark Report: 4395390Performance shiftsFull benchmark resultsGenerated by GHA run |
pp-mo
requested changes
Oct 21, 2026
Member
There was a problem hiding this comment.
Various odd issues raised.
Also some general points:
- no support for mesh data -- you can't load UGRID style data from Zarr : is this intentional ??
- quite a few test modules contain functions which target several different methods/functions, but don't arrange them into a class-per-tested-thing as I would like/expect.
including at least (but there may be more?) ...- tests/unit/fileformats/cf/test__DimensionIdentity.py
- tests/unit/fileformats/zarr/dataset/test_ZarrDataset.py
- tests/unit/fileformats/zarr/dataset/test_read_data.py
- tests/unit/fileformats/zarr/test__decode.py
- tests/unit/fileformats/zarr/test_dependency_errors.py
- lib/iris/tests/unit/io/test_load_stores.py
iris.load on a UGRID Zarr store returned a cube that had silently dropped its mesh coordinates: CFReader classifies them as UGRID coords, so the ordinary aux-coord path skipped them and nothing built the mesh. mesh and location were left on the cube as stray attributes. No warning, nothing in LOAD_PROBLEMS. iris.mesh.load_meshes reached a store and then rejected it, because the handler test named only nc_loader.load_cubes. The design document (section 4.7) already called for load_meshes to accept Zarr stores, so this is an implementation gap rather than a decision. The capability was already present: _meshes_from_cf builds a correct MeshXY from a ZarrDataset unmodified. The inline mesh block in netcdf/loader.py becomes _prepare_mesh_coords and _attach_mesh_coords in ugrid_load.py, called by both loaders. Two functions rather than one because the MeshCoords must be built before _load_cube and attached after it - built after, _add_unused_attributes has already taken the variable's mesh and location attributes. The "mesh could not be found" message keeps being raised on the iris.fileformats.netcdf logger, which ugrid_load's own logger would drop at INFO. Also from the review: - read_data restructured to a single exit, and the duplicated as_lazy_data call hoisted out of both branches. - _decode's surface narrowed to decode_parameters, decoding_required and mask_and_scale; every other name underscored, and the rule written into the module docstring. - The CFDataset contract asserts the chunk shape under a per-backend chunking_key, rather than searching storage_settings for it by value. netCDF4 spells the key chunksizes, Zarr spells it chunks. - zarr/stores.py imports the contract's constants instead of restating them. - _dimension_names tested through ZarrDatasetVariable.__init__. - test_one_cube_per_data_variable given three real data variables; the canonical store's second array is a grid mapping. - A unit test for the constraint var_callback. - Six test modules regrouped into one class per tested thing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: bd070a33-3348-4ab3-bd65-d7a1f9b60ae8
Contributor
Author
|
(posted by Claude Opus) Thanks @pp-mo - all of this is actioned in 5688b24, with the detail in each thread. The two general points from your review body: Mesh data. Not intentional, and worse than "no support" - Class per tested thing. Done for all six modules you listed:
To answer the question inside that one: no, it is not documented. Full suite after all of it: 12533 passed, 66 skipped, nothing failing. The skip count is unchanged from before the branch. |
load_meshes carries its own copy of the format dispatch (design document section 2.3), so every format the main load chain learns has to be taught to this copy too - and the copy lives under iris/fileformats/netcdf/. The mesh code that ugrid_load shares with the Zarr loader names no format at all; the dispatch is the whole reason Zarr appears here. Add a TODO naming the event that removes it, and record in the design document that unifying the two cube-building loops takes this duplication with it. Also fold two comments that were both saying "do not import zarr eagerly" into the one that needed to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: bd070a33-3348-4ab3-bd65-d7a1f9b60ae8
Contributor
⏱️ Performance Benchmark Report: db6f148Performance shiftsFull benchmark resultsGenerated by GHA run |
Contributor
⏱️ Performance Benchmark Report: c10b5d9Performance shiftsFull benchmark resultsGenerated by GHA run |
trexfeathers
added a commit
to trexfeathers/iris
that referenced
this pull request
Oct 23, 2026
The rebase onto brownfield brought three changes made while this branch was being built, each of which this work has to meet. The generic saver is `_CFSaver`, renamed and made private at review of SciTools#7338. Every reference here follows it, and the private name is written as a literal rather than a Sphinx role, since there is no page to link to. `iris.io` no longer keys the cubelist savers by module name: it holds the saver functions themselves, in `_savers.cubelist_savers`. `iris.io._zarr_save` is registered there beside the netCDF saver, and `iris.fileformats.zarr.save` registers itself at the foot of its own module - the only place it can be named by identity without `iris.io` importing zarr to do it. Both routes a caller can take to a Zarr save are covered by tests. UGRID meshes load from Zarr as of SciTools#7336, so the mesh round trip passes and its strict `xfail` is gone, with the documentation and the design record that described the gap. `ZarrDataset.storage_settings` is the member the interface settled on, so the write tests ask for it by that name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 96841741-ed31-4388-9014-e3f94eb82651
trexfeathers
added a commit
to trexfeathers/iris
that referenced
this pull request
Oct 23, 2026
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
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 fourth of eight pull requests delivering native Zarr support for Iris, and the subject of #6979:
iris.load("store.zarr")andiris.load("s3://bucket/store.zarr")return cubes, for Zarr version 2 and version 3 stores, local or remote, root group or a named subgroup viagroup=. Part of #6979.Behaviour changes. Four, all previously errors. A local
.zarrdirectory used to raiseIsADirectoryError; it now loads. A remote URL with schemes3,gs,azorabfsused to raiseValueError("Iris cannot handle the URI scheme: ..."); it now loads too, passed through to zarr-python without Iris adding a dependency or handling credentials.CFVariable.spansand its three overrides now match dimensions on(name, size)pairs rather than on the bare name, because a Zarr dimension name carries none of netCDF's uniqueness guarantee; no netCDF file can observe the difference, and the test suite asserts both backends give identical answers. A glob matching a plain directory - one with no Zarr marker file - used to raiseIsADirectoryError; it now raisesValueError: No format specification could be found...instead, because the load chain checks for a Zarr store before it checks for a directory it cannot otherwise handle. Nothing that loaded before this pull request fails after it; only that one exception's class changed.Public API added.
iris.fileformats.zarr(load_cubes),iris.io.load_stores,CFDataset.format,CFDataset.encodingandCFDatasetVariable.encoding.Spec amendments. This pull request also updates
docs/superpowers/specs/2026-09-21-zarr-io-design.md, the design document the wider programme works from, in a few places a reviewer who already knows that document should not be surprised by:CFDataset.formatlands here rather than with the saver, becauseZarrDatasetcannot tellZARR_V2fromZARR_V3without it and this pull request merges before the saver's.CFDatasetcontract splits into a read half and a write half, because this pull request implements the Zarr read path only, and a shared test body with half of it skipped would hide an incomplete implementation behind a skip.iris.io.load_filesbecomes a public wrapper over a private_load_files, so the load chain can expand a glob once, before partitioning files from stores, without narrowingload_files's own documented contract for other callers.ZarrDataset.attributeswithholds netCDF-c's_NCPropertiesstamp alongside the existing_ARRAY_DIMENSIONSconvention, so a store's provenance string from one writer does not turn into a stray cube attribute that a different writer's output of the same data would not carry.UGRID meshes load too. Nothing in
iris.fileformats.netcdf.ugrid_loadis netCDF-specific - it reads aCFReader, and aCFReaderover aZarrDatasetanswers the same questions - so the Zarr loader calls into it rather than growing a second copy. This closes a gap rather than adding scope: the design document already saidiris.mesh.load_mesheswould accept Zarr stores, and before this it reached one and then rejected it, because the handler test named onlynetcdf.loader.load_cubes. Worse,iris.loadon a UGRID store returned a cube that had silently dropped its mesh coordinates -CFReaderclassifies them as UGRID coords, so the ordinary aux-coord path skipped them and nothing built the mesh - while leavingmeshandlocationon the cube as stray attributes. No warning, nothing inLOAD_PROBLEMS. The mesh block that was inline innetcdf/loader.pyis now_prepare_mesh_coordsand_attach_mesh_coords, shared by both loaders; two functions because the MeshCoords have to be built before_load_cubeand attached after it, or_add_unused_attributeshas already claimed those two attributes. One netCDF detail survives the move: the "mesh could not be found" message keeps being raised on theiris.fileformats.netcdflogger rather thanugrid_load's own, which sits atINFOand would drop a debug message at source.What this does not do. No saving:
ZarrDataset's write surface exists because the interface declares it and raisesNotImplementedError; that is PR 6. No real-world fixtures or benchmarks: every store here is synthetic, built intmp_pathor in memory; the cut-down NOAA GFS and ESA EOPF samples are PR 7. No xarray as a test dependency: that lands in PR 6, with the saver it validates.Changelog. No fragment in this pull request, deliberately: fragments are numbered for the pull request that lands them on
main, and this one lands onbrownfield. The merge-back pull request owns the fragments for the whole programme.Lock files.
requirements/py3{12,13,14}.ymlcarryzarr >=3.0.8in their optional-dependency section, and all three lock files are regenerated to match, pulling inzarr3.4.1 withnumcodecsanddonfig. This matters more than a lock-file refresh usually does: CI builds its test environment fromrequirements/locks/, so without the regeneration every Zarr test in this pull request would have skipped rather than run, and the whole feature would have been invisible to CI at exactly the point it needed to be visible.How to review it. The new code builds a Zarr store that answers as a
CFDataset, then teaches the CF layer to hold one, then teaches Iris to find one; reading it in that order tracks how it was written. A netCDF-only reviewer who wants to check only what touches existing behaviour should look at three things: thespanschange to(name, size)matching, the same identity rule now carrying through coordinate attachment in_nc_load_rules/helpers.py, and the load-chain changes that let a target be a directory or a URL rather than only a file. None of the three changes what any netCDF file loads as — dimension names are unique within a netCDF group, so matching on(name, size)and matching onnameare the same key there, and the test suite asserts both backends give identical answers.Base. This branch was developed on top of
zarr-io-pr-3, because it calls four names PR 3 introduced —CFDatasetVariable.read_data,iris.fileformats.cf.loader._get_actual_dtype,_LAZYVAR_MIN_BYTES, and thecache_key=keyword onas_lazy_data. Now that #7316 has merged, it is rebased ontobrownfieldand carries only its own commits. The full test suite was re-run on the rebased result: 12328 passed, 66 skipped, nothing failing. An earlier preview of this work ran as trexfeathers#99, based onzarr-io-pr-3, so that its diff showed this pull request's changes and nothing else; the review comments there are answered below.Review findings already actioned. An automated review of the preview found four defects, each reproduced and fixed here, and each recorded in the design document. Two of them are worth a reviewer's attention in their own right.
spansturned out to be only half of the dimension question: once a coordinate is known to span a data variable, the loading rules still have to say which of its dimensions the coordinate covers, and they did that by name alone — so a data variable with dimensions('d', 'd')at sizes(2, 3)sent a three-point coordinate ondtoLOAD_PROBLEMS, despite the right axis being uniquely identifiable. The identity rule is now a single function used by bothspansand a sharedhelpers._common_data_dims, rather than two rules that could drift; netCDF cannot observe the difference, its dimension names being unique within a group. Separately, theZarrStorefile element now answersNonewhen it is handed a file handle:requires_fh = Falsemeans "consult me without a handle", not "consult me with one", and because the caller that opens a file passes a basename rather than a path, a NIMROD file namedweatherwas classified as Zarr whenever the working directory happened to contain aweather/zarr.json. The other two are a vectormissing_value, which CF permits and netCDF4 masks on but this code read as a scalar, and chunk sizing, which used the stored dtype where it should use the decoded one — a packedint8variable that unpacks tofloat64is eight times its stored size by the time a task holds it.Review by @pp-mo, actioned. Beyond the mesh gap above:
ZarrDatasetVariable.read_datarestructured to a single exit, which also hoisted theas_lazy_datacall that was identical on both branches of the chunking test;_decode's surface narrowed todecode_parameters,decoding_requiredandmask_and_scale, with every other name underscored and the rule written into the module docstring;_dimension_namesnow tested throughZarrDatasetVariable.__init__rather than directly;zarr/stores.pyimports the shared contract's constants instead of restating them;test_one_cube_per_data_variablegiven three real data variables, the canonical store's second array being a grid mapping; a unit test added for the constraintvar_callback; and six test modules regrouped into one class per tested thing.One suggestion was not taken as written. The shared contract's chunking assertion cannot read
storage_settings["chunking"], becausestorage_settingskeys are each backend's own spelling: netCDF4 sayschunksizes, Zarr sayschunks, which is why the original assertion searched by value. Each contract subclass now declares achunking_keyand the assertion isvariable.chunking == storage_settings[self.chunking_key]- which does pin the specific key, as asked.Unifying the two cube-building loops is deferred rather than refused. Now that the Zarr loader has the same mesh handling, the two bodies differ only in how the dataset is opened, so the refactor got easier here, not harder; it is internal, with neither
load_cubessignature moving. It is recorded in the design document as an issue to raise at merge-back, so that nobody starts on it while this programme's pull requests are still interdependent and in flight.