Repository navigation
Brownfield - #7309
Draft
trexfeathers wants to merge 20 commits into
Draft
Brownfield#7309trexfeathers wants to merge 20 commits into
trexfeathers wants to merge 20 commits into
Conversation
* Experimental pre-commit hook. * netcdf4_imports fail faster. * Introduce .hooks directory. * Improved structure. * Fully replace test_coding_standards. * Add AGENTS.md note. * Fix MyPy.
* Add design spec for native Zarr I/O Specifies a six-PR programme adding native Zarr support to Iris: reading Zarr v2 and v3 stores, and writing v3. Alternates behaviour-preserving refactors with user-visible features so that no feature PR carries a large diff and every relocation is independently verifiable. The central move is a CFDataset abstraction separating CF attributes from storage properties, with one implementation per backend. cf.py, the loader and the saver are rewritten against that interface rather than against the netCDF4 Python API. Zarr behaviours that shape the design were verified against zarr-python 3.4.0 rather than read from documentation: v2 cannot carry dimension names, JSON attribute serialisation rejects all NumPy types and poisons the attribute mapping on failure, reads apply no masking or unpacking, and shard shape is exposed separately from chunk shape. Closes #6977, #6979, #6980 when the programme completes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Ground the Zarr design in the real NOAA GFS store Cut the GFS forecast subset ahead of the dynamical.org endpoint closing on 30 September 2026, and fold what the real data revealed back into the spec. Two findings change the read design. The _FillValue attribute on every float32 data variable is the base64 string 'AAAAAAAA+H8=', a float64 NaN that matches neither JSON number syntax nor the array dtype, while the array's own fill_value property is correct; the array property is therefore authoritative and a non-numeric attribute warns rather than raises. Real CF attributes are also not always scalars: valid_time carries a nested JSON object, which netCDF cannot express and Iris has no handling for. Also corrects a misreading of test_nczarr.py. Its "nczarr"/"xarray" parametrisation names two netCDF-c URL modes, not the xarray package, which is not an Iris dependency. That makes the NCZarr xarray mode a v2 fixture generator for free, and makes cross-reader testing a dependency decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Separate Zarr spec facts from publisher malformations Reworks the design spec after review. `iris.fileformats.cf` becomes a package rather than three flat sibling modules, split along the four kinds of thing already in `cf.py`. The programme grows to seven pull requests so the move lands on its own, carrying #7259's tests ahead of the rewrite they protect. Re-frames the two findings that had been credited to real data. The base64 `_FillValue` in the NOAA GFS store is legal Zarr but violates CF 2.5.1, so it is a malformation to warn about, not a design learning; what survives is that Zarr's required array `fill_value` and CF's optional `_FillValue` attribute are distinct concepts. Nested JSON attributes are genuine — the v3 spec permits an arbitrary JSON literal, CF has no equivalent, and two independent producers use them. Confirms ESA EOPF is Zarr v2 by reading the store: `zarr.json` 404s, `.zgroup` holds `{"zarr_format": 2}`. Its root group holds no arrays at all, which makes the empty-root-group design a necessity. Its bands carry correct flat CF attributes alongside a redundant `_eopf_attrs` copy. Settles the module layout, cross-reader testing (xarray costs exactly one conda package) and EOPF's role. Parks the CF-attribute-model question for its own discussion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Link the spec to the parked attribute-model issue #7288 now holds the CF-attribute-model discussion that section 10 parks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Record the specification-over-sample-file rule Iris implements published conventions, and the temptation when adding a new format backend is to design against whatever file is to hand. Two findings in the Zarr work were initially credited to "real data" when one was a publisher's CF violation and the other was already written down in the Zarr v3 specification. Adds a short section saying so, and tightens two existing paragraphs to partly pay for the lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Give the Zarr spec a single progress record The document is meant to be living, but its state was scattered: the programme in section 5, settled decisions split between sections 3 and 10, the parked question in prose, the cut-down GFS fixture's location buried in section 7, and pull request and issue states nowhere at all. Adds section 12 as the one place state lives — pull requests, issues, open questions with their provisional answers, an append-only decision log, artefacts and document history — and a status block at the top that points to it. Section 10 keeps only the substantive parked question; its settled list moves to the decision log. Follows the convention the merge/concatenate spec set with its section 6: update the progress record as work lands, and track status nowhere else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Frame the Zarr programme as a proof of concept The seven pull requests explore whether the design survives contact with the code, on a feature branch, at speed. Official pull requests follow later, so the sub-issues stay open and nothing here uses a GitHub closing keyword. Drops the `Closes` column from the progress table and replaces the issue-state table with one that says what each issue is *for* in this programme. Issue state lives on GitHub. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Confirm the proof-of-concept working rules Seven separate pull requests, so each step keeps its own CI signal and can be verified alone. Tests on every production change, because they are the proof in "proof of concept". Changelog fragments deferred to the official pull requests, stated in each pull request body so the omission does not read as an oversight. The one exception noted: the `_NCZARR_SCALAR_DIMENSION` gap in the three overriding `spans` methods is a user-visible bugfix and will need its own fragment when the official work is written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Retract the proof-of-concept framing; specify the merge-back This is the delivery plan, so the spec has to say how the work reaches users, not just how it reaches the feature branch. Adds the merge-back of `brownfield` into `main` as the terminal step, and gives it the three obligations the feature-branch pull requests defer: the closing keywords for the three sub-issues, five changelog fragments across `feature`, `dependency`, `deprecation`, `bugfix` and `internal`, and a documentation build against `main`. Both deferrals now have structural reasons rather than the retracted proof-of-concept one. A closing keyword only fires on merge into the default branch, so `Closes` on a `brownfield` pull request would link an issue and never close it. A changelog fragment is named for its pull request number, so one numbered for a feature-branch pull request would advertise a change `main` has not received — which is what the two pull requests already merged into `brownfield` do, carrying no fragment while #7267 into `main` does. Documentation moves into PRs 4 and 6, alongside the code it describes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Keep the CFUGrid variable classes with their peers The proposed cf package split the CFVariable subclasses across _variables.py and _ugrid.py. Nothing justified that seam beyond getting _variables.py under the ~1000-line aim in lib/iris/AGENTS.md, and the same file says cohesion wins over file size. The UGRID classes are peers of the classic ones: same shape, same job, listed in the same tuple in CFReader._variable_types, exposed as peer getters on CFGroup, and sharing the module-private _is_str_dtype helper that the split would have had to export across a module boundary. The cf package now splits three ways, not four, and _variables.py is allowed over the line aim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Narrow the __getattr__ ban; make CFVariable backend-agnostic lib/iris/AGENTS.md banned __getattr__ outright except in deprecation shims. That overreaches: CF attributes are an open-ended set of data keys read from a file, with no schema to enumerate, which is a different thing from __getattr__ used to invent behaviour. The ban is replaced by a checkable exception - forward to a declared Mapping, and make that Mapping the path library code takes. The rules on computed names and getattr string dispatch are unchanged in substance; iris.fileformats has 42 constant-name lookups and zero dispatch sites, so they were never in tension. A clarifying clause says so, since the text was not landing that way. The spec was worse than the rule. Section 4.3 had CFVariable.__getattr__ raise TypeError for Zarr-backed variables, putting a hole in the most-used public accessor of the abstraction section 4.2 exists to provide. It now reads through .attributes on the base class for both backends, drops the setattr instance caching, and keeps a one-cycle deprecating fallback to the netCDF4 object for the proxy uses (.dimensions, .shape, .dtype, .getncattr) that CFDatasetVariable replaces. Also records the consequence PR 2 has to handle: cf_attrs_unused() feeds netcdf/loader.py:190 and is currently a side effect of __getattr__, so the read tracking has to move to .attributes with the reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Endorse package re-exports instead of merely tolerating them lib/iris/AGENTS.md never said whether a package __init__.py may surface objects defined in private submodules. The omission read as disapproval - the nearest rule, "do not add indirection ... for a single caller", is about implementation layers, not API surface - and the Zarr spec duly wrote an apologetic paragraph explaining why cf/__init__.py had to break a rule it was not breaking. Surfacing low-level objects at the level users import from is a genuine convenience, and it is what leaves a private file layout free to change. iris.mesh already does it across four modules and two subtrees. The rule is now stated positively, with the conditions that keep it greppable: verbatim and static, gathered into __all__, no renaming on the way out, no conditional imports, no logic in __init__.py. iris.common's "import *" is marked as legacy rather than precedent. The indirection rule gains a clause saying what it does not cover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Keep multi-process Zarr writing reachable Coordinated writing to one target from many processes is an Iris differentiator for netCDF and is wanted for Zarr by name. It stays out of scope for size, but the spec previously dismissed it in a single out-of-scope bullet deferring to "what zarr-python guarantees" - which is nothing. zarr 3.4.0 removed the v2 synchroniser machinery: create_array has no synchronizer parameter, open_group warns "synchronizer is not yet implemented", and ProcessSynchronizer and zarr.sync are gone. Any coordination will be Iris's own. Four provisions, none of which builds the feature: - _dask_locks.py moves to cf/ in PR 5. It imports only threading and four dask modules, no netCDF at all, so it is generic already and only its docstring says otherwise. - CFDatasetVariable.write_handle() becomes the coordination seam. netCDF returns today's lock-carrying NetCDFWriteProxy; Zarr returns the zarr.Array, which is picklable and round-trips shape and chunks. A future lock-carrying Zarr proxy needs no interface change. - The lock stays inside the dataset implementation; cf/saver.py holds no lock and knows no scheduler. - CFDataset carries mode from the start, because the Zarr shape of this feature is a region write into an existing store, which an interface whose only verbs are create_dimension and create_variable cannot express. Also fixes a live correctness gap the question exposed. da.store(..., lock=False) is only safe where the lazy source tiles the Zarr chunk grid exactly; two tasks sharing a chunk read-modify-write and silently lose an update. The chunk-alignment invariant is now written down, with create_variable deriving chunks from the source and the saver rechunking when a caller forces a conflicting chunksizes. Separates finalise() from close() so consolidating metadata is one-shot and owned by the returned Delayed, rather than raced by N workers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Cover Zarr store durability and metadata/data desynchronisation The spec said nothing about what an interrupted save leaves behind. For a fresh store the hazard is absent by construction - metadata is written once at array creation and never edited - but that was accidental rather than stated, and save(..., group=...) writes into a store that may already exist, which is in current scope. Two format hazards confirmed against zarr 3.4.0. Removing one chunk object from a complete array reads back as fill_value with no warning, so an interrupted save leaves a store that opens cleanly and silently under-reports data. Adding an array to a consolidated store without re-consolidating makes it invisible to a default reader, which prefers consolidated metadata when present, while an unconsolidated reader of the same store sees it. Neither is fixable from inside Iris, so the design avoids creating the conditions instead: - The create-once invariant is now written down as a constraint on future work: never resize, rechunk, or change dtype, fill_value, codecs or dimension names of an existing array. - save takes mode, defaulting to "w-" - raise if the target exists. This differs from the netCDF saver's clobber deliberately, because a Zarr clobber is many non-atomic deletes. - Adding to a store that carries consolidated metadata must re-consolidate as its final act, or raise. - finalise() is always the last write. Write-to-temp-then-rename is rejected and the reasoning recorded: it is atomic on POSIX but object stores have no equivalent, so the guarantee would evaporate where Zarr is most used. Icechunk is named as the ecosystem's answer in both the spec and the planned documentation. The create-once invariant also carries most of the safety argument for the future parallel-write work, since workers that only fill pre-created chunks never touch metadata. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Drop ZarrDataProxy; decode in the graph, align chunks on read @bjlittle asked whether the netCDF proxy classes are actually necessary for Zarr, given that chunks are inherent to the store structure. They are not, and the draft was wrong to mirror them. Every reason NetCDFDataProxy exists is a netCDF4/HDF5 reason: an unpicklable Variable, a thread-unsafe library forcing a global lock, and an open file handle that must not span a lazy graph. None holds for Zarr, where zarr.Array is picklable and a chunk is an independent object in a key-value store. Two of the draft's three justifications for a proxy were false on inspection; the third, the dask cache key, wants a hashable identity rather than a class, so as_lazy_data gains cache_key= and _lazy_data stops importing from fileformats.netcdf. CF decoding becomes an explicit dask graph layer rather than hidden behaviour inside __getitem__, which also makes each rule testable on a plain ndarray with no store. Adds a read-side chunk-alignment guard mirroring the write-side invariant. A Zarr chunk is the atomic unit of storage, and _optimum_chunksize shrinks below it when the store chunk already exceeds the 128 MiB dask target, so four tasks each fetch and decompress the same 488 MiB object. The same trap exists on the netCDF path, since NetCDFDataProxy reopens the Dataset per __getitem__ and discards HDF5's chunk cache; recorded as an open question rather than changed here. Also corrects PR 2's description, which still promised the TypeError seam that section 4.3 already rejected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Cover the write proxy: no Zarr equivalent, but write_handle stays @bjlittle asked whether NetCDFWriteProxy exists purely to support multi-process writing, and so belongs entirely to future scope. It does not. Carrying the file lock is its second job; its first is to be a __setitem__ target that outlives the closed Dataset, which a deferred save needs even single-threaded, because a netCDF4.Variable is neither picklable nor able to outlive its Dataset. Neither job exists for Zarr. Verified end to end: a zarr.Array keeps writing after a pickle round trip, a deferred da.store lands after every in-process reference to the group is dropped, and the whole delayed graph survives pickle before computing, which is what a distributed scheduler requires. So write_handle() is justified by present netCDF need rather than by future-proofing, and the Zarr side returns the array itself. Records that native Zarr restores deferred saving, which the NCZarr path had to give up: Saver.__exit__ computes NCZarr writes eagerly because netCDF-c cannot reopen a Zarr store for deferred writes. PR 6 gains a test for this. Corrects the claim that library code instantiates NetCDFWriteProxy; it builds the EncodedNetCDFWriteProxy subclass. Also records a defect found while checking this, and deliberately not fixed here: da.store now returns a tuple rather than a Delayed for a sequence of sources, so iris.save(compute=False).compute() raises AttributeError against the documented contract. Every internal caller uses dask.compute, so no test covers it. Q5 in section 12.3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Link the deferred-save return-type defect to #7291 The cause is now known: #6451 adapted Iris to dask/dask#11844 for dask 2025.4, switching every internal caller to dask.compute() and relaxing the test assertion to accept either shape. The code change was right; the docstrings and the user manual were left promising a Delayed completed by result.compute(). Raised as #7291. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Correct the collaborator attribution to @trexfeathers Every decision in the spec's log was agreed with GitHub user trexfeathers, not bjlittle. The two pair on this work, and the bjlittle account owns the fork the branch is pushed to, which is why the mistake was invisible. Only the @ mentions change. The `bjlittle/iris` and `bjlittle/iris-test-data` fork paths stay as they are: those are the git remotes in use and remain correct. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Align concurrent Zarr writes to the shard, not the chunk The invariant licensing da.store(..., lock=False) was stated over the chunk grid. That is insufficient when sharding is enabled: several inner chunks occupy one shard, and a shard is a single stored object, so tasks writing distinct chunks still read-modify-write the same object. Verified on zarr 3.4.0 / dask 2026.7.1 -- da.arange(128, chunks=8) into chunks=(8,), shards=(64,) tiles the chunk grid exactly and still lost 72-96 of 128 values in 12 of 12 trials. Aligning the source to the shard, or to a whole multiple of it, gives 0 of 12. Restated as the write-alignment invariant, over Array.shards when the array is sharded and Array.chunks otherwise. xarray guards the same unit via safe_chunks (pydata/xarray#10831); the earlier draft cited that precedent and then under-specified it. Raised by the OpenAI Codex review of #7292. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 45914679-20ef-4bcf-b7fe-c48ab76a3cef * Correct the Zarr fill-value rules: masking and encoding Two errors in one section, both of which would have produced wrong data. Masking took its sentinel from Array.fill_value in preference to the CF _FillValue attribute. But fill_value is a storage fact -- what an unwritten chunk reads back as -- not a declaration that a value is missing. zarr-python defaults it to zero, so an int16 array holding [0, 1, 2] with no CF attributes had its valid zero masked. And because the field is required on version 3, the "fall back to _FillValue only when the array carries no usable one" branch could never fire, so a genuine CF sentinel could never win. Both reproduced against a store written by xarray. The precedence is now version-aware, matching xarray's use_zarr_fill_value_as_mask. The base64 _FillValue was called a publisher's CF violation and a "file malformation to defend against". That was wrong. base64(struct.pack("<d", nan)) is exactly 'AAAAAAAA+H8=', and writing a float32 NaN with xarray produces that literal string. It is xarray's version 3 encoding, which most version 3 data in the wild will carry. Worse, writing the numeric form this spec specified makes the store unreadable by default xarray, which raises TypeError and fails the whole dataset open. Iris now reads both forms and writes base64 for floats: a deliberate, documented deviation from CF 2.5.1 for one attribute whose dtype is load-bearing, taken because interoperability is the reason for native Zarr. It does not reopen the base64-envelope rejection for general attributes. This was the "Conform to the Specification, Not to the Sample File" rule applied backwards -- crediting a convention to a publisher's slip. Sections 1 and 8 now name that inverse failure mode. Raised by the OpenAI Codex review of #7292. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 45914679-20ef-4bcf-b7fe-c48ab76a3cef * Write _FillValue when saving masked data, not just fill_value Taking CF masking off the storage fill value has a write-side consequence the saver did not meet. Masked arrays were stored as data.filled(fill_value) with the array's fill_value set to match, and nothing else. On version 3 the CF attribute is now the masking declaration and the storage field is not, so such a store reads back with every masked point silently unmasked -- a round trip that loses the mask. The saver must also write a _FillValue attribute in this case, even when the source cube carries none of its own. Setting fill_value alone is the write-side form of the same conflation the read-side precedence made. Found while applying the #7292 review, not raised by it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 45914679-20ef-4bcf-b7fe-c48ab76a3cef * Separate the Zarr read unit from the write unit chunking() returned Array.shards when present, on the reasoning that "the shard is the unit a reader actually fetches". That is not how sharding works. Zarr's sharding codec is indexed: a reader fetches the shard index, then byte ranges for only the inner chunks it needs. Requiring shard-sized dask blocks destroys that, and does so because of the explicit decode layers this design introduced. Bare dask slicing can push a slice down into the array's own __getitem__, but a map_blocks layer in between cannot. On a shape=(1024,), chunks=(8,), shards=(1024,) int32 array, reading the first eight values took 2,084 bytes with inner-chunk blocks and 6,148 -- the whole shard -- with shard-sized blocks plus a decode layer. Without the decode layer both were 2,084, which is why the layer is the deciding factor. chunking() therefore returns Array.chunks, and read alignment aggregates upward into whole multiples of it, never down and never to a shard boundary. Shard-sized blocks remain a write requirement only. The NOAA GFS fixture is sharded, so this is exercised rather than hypothetical. Raised by the OpenAI Codex review of #7292. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 45914679-20ef-4bcf-b7fe-c48ab76a3cef * Key the Zarr lazy-data cache on array metadata, not its address The proposed cache_key of (store location, array path, zarr format) is an address, and iris._lazy_data.CACHE is a process-wide LRUCache(100), so the address goes stale the moment a store is rewritten in place -- which mode="w" supports and a notebook or long-running service does routinely. Reproduced against the real key shape: an array reopened after its shape changed from (4,) to (6,) returned the cached four values, silently truncating; and an array whose fill_value changed from -1 to -2, with chunks left unwritten, returned the old fill. Under the new masking rules that second case feeds the version 2 mask, so stale metadata becomes wrongly masked data. The address-only key would also have been weaker than the netCDF key it replaces -- repr(NetCDFDataProxy) already carries shape and dtype -- so the relocation would not have been behaviour-preserving, which was its premise. Now keyed on json.dumps(Array.metadata.to_dict(), sort_keys=True), which covers shape, chunks, shards, dtype, fill_value, codecs, dimension_names and attributes in one value. This narrows the window rather than closing it: metadata identity still cannot detect changed chunk contents under identical metadata, and neither can the netCDF key. Q6 records session scoping as the durable fix. Raised by the OpenAI Codex review of #7292. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 45914679-20ef-4bcf-b7fe-c48ab76a3cef * Record the #7292 review outcome: tests, questions and decisions Names the test for each accepted finding in section 6, and assigns them to the pull requests that must carry them (PR 4 for the read side, PR 6 for the write side). Every one of the five findings now has a test that fails without its fix. The cross-reader test that catches the _FillValue encoding error was already promised in section 6 -- "if xarray cannot read Iris output, the decision was wrong" -- and simply had not been written. It now has a specific case. Also adds Q6 (cache lifetime) and Q7 (taking the base64 deviation upstream to zarr-conventions/CF), the xarray backend and pydata/xarray#10831 references, the decision-log entry and the document-history row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 45914679-20ef-4bcf-b7fe-c48ab76a3cef * Restructure the Zarr spec for readability §4.4 and §4.5 held 617 of the document's 1815 lines between them with three subheadings, all three in §4.5. §4.4 had none: ten topic-level run-in bold lead-ins and nothing else, so an outline of the document stopped exactly where the implementable detail began. Both sections now carry `####` headings per topic. This also fixes a mis-nesting: `Encoding` was sitting under "Multi-process writes", which it has nothing to do with. Recast the design history from "an earlier draft said X, that was wrong" into the rule it implies: "do not do X, because Y". The content earns its place — CF §2.5.1 leads a fresh reader straight back to the numeric `_FillValue`, and mirroring NetCDFDataProxy is the obvious move for a Zarr reader — so it stays next to the rule it guards rather than moving to the decision log. Only the voice changes, from a fact about this document's history to a fact about the code. Refresh the header block: the last-updated date, and the dask and xarray versions that the [verified] claims added yesterday depend on. No normative content changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 45914679-20ef-4bcf-b7fe-c48ab76a3cef --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Martin Yeo <martin.yeo@metoffice.gov.uk>
) * pre-commit does not need to be in a dedicated environment. Session-Id: 16168c01-abcd-41e8-a19e-87e22391c55b * Implementation plan for the cf package split The first of the seven pull requests in the native Zarr I/O design spec (docs/superpowers/specs/2026-09-21-zarr-io-design.md, section 5): land the unit test coverage from #7259 against today's cf.py, then split cf.py into the iris.fileformats.cf package, behind an __init__.py that re-exports every public name. Committed first so that the rest of the pull request has something to be read against. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 16168c01-abcd-41e8-a19e-87e22391c55b * Unit test coverage for iris.fileformats.cf Squashed from #7259, branch cf_reader_zarr, applied to today's cf.py unchanged. Eighteen files under lib/iris/tests, +2354/-643, twelve of them new: - identify_mixins.py, holding the shared SpansMixin, IdentifyByAttributeMixin and IdentifyByAttributeListMixin bodies that every CFVariable subclass is then run against; - one test module per CF variable class, including the private _CFFormulaTermsVariable; - test_CFVariable.py for the base class, pinning the attribute-access and cf_attrs_* behaviour; - the three existing test_CFUGrid* modules rewritten onto the shared mixins, which is most of the deletions; - test_CFGroup.py and test_CFReader.py substantially extended; - tests/test_cf.py moved to tests/integration/, where a test that drives real files end to end belongs. Landing this before the package split is the point: the tests were written against today's behaviour by people who were not doing the rewrite, so the net that catches a regression in later pull requests did not move with the thing it is testing. The one non-test line rewrites an incoming TODO comment to cite the issue it turned into, #7296 - the guard it marks is dead code and is deliberately not fixed here. Part of #6977 Co-authored-by: Martin Yeo <martin.yeo@metoffice.gov.uk> Co-authored-by: Patrick Peglar <patrick.peglar@metoffice.gov.uk> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 16168c01-abcd-41e8-a19e-87e22391c55b * Move cf.py into a package, unchanged git mv lib/iris/fileformats/cf.py lib/iris/fileformats/cf/_variables.py, plus a new __init__.py that re-exports all sixteen public names verbatim into __all__. No code moved between modules yet: _variables.py still holds all three seams, and the cut into _group.py and _reader.py is the next commit. git records the move as R099 rather than R100 because the module docstring moved up to __init__.py, where the .. z_reference:: directive belongs now that the package is what users import. _variables.py takes a one-line placeholder, rewritten properly with the cut. Apart from that docstring, nothing in the 1721 lines changed - which is the point of keeping this commit separate. Six references in the incoming test suite named the flat module and had to be retargeted at iris.fileformats.cf._variables, because each resolves a private name or a module global that the re-export layer deliberately does not carry: - _NCZARR_SCALAR_DIMENSION and _CFFormulaTermsVariable, private constants and classes that are not public API; - the _thread_safe_nc and hh patch targets, which must name the module whose globals CFReader reads; - the CFBoundaryVariable patch in test_derived_bounds_boundary_guard_continue_branch. That one is the sharp case: patching the re-export instead would leave CFReader looking at the real class, so the test would pass without exercising the branch it exists for. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 16168c01-abcd-41e8-a19e-87e22391c55b * Split the cf package into _variables, _group and _reader Cut the two lower seams out of _variables.py into modules of their own, leaving a one-way dependency chain _variables <- _group <- _reader, and redistribute the imports so each module takes only what it uses. The effect worth having is that iris.fileformats.netcdf is now imported by _reader.py alone. Classification and grouping no longer reference netCDF at all; they work against anything presenting netCDF-like variables, dimensions and attributes. That is the coupling the native Zarr work has to remove, and this puts all of it behind one import block in one file. No behaviour change and no API change. Every line of moved code is byte-identical to its former self - reconstructing _variables.py from the three files reproduces the original exactly - and the sixteen public names are re-exported from iris.fileformats.cf unchanged. The only new prose is the four module docstrings. Three mocker.patch targets in test_CFReader.py follow CFReader into _reader: a patch rebinds a module global, so it must name the module whose global the code under test actually reads. Adds test___init__.py, covering the package surface itself, which the inherited suite does not touch: that every public name a private module defines is re-exported, that every __all__ entry resolves, that only _reader is coupled to netcdf, that the package imports in a fresh interpreter from either side of the cf/netcdf cycle, and that private names stay private. Folds four corrections into the plan, each marked in place: reference_terms is patched with patch.dict rather than patch and so needed no retarget at all; the broken-target list was missing two entries and carrying a spurious one; the rename in the previous commit is R099 not R100; and Connectivity is not used by _group.py, that was a substring match on CFUGridConnectivityVariable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 16168c01-abcd-41e8-a19e-87e22391c55b * Keep the split invisible to Sphinx and to repr() The docs build is the one check that cannot be delegated to a hook, and it found two things everything else missed. The full suite, every pre-commit hook, the ruff --select F gate and all seven package-surface tests were green at the point these were still broken. Read the Docs sets fail_on_warning, and the split produced one: CFReader carries a "CFGroup = CFGroup" class attribute, so autodoc documents CFGroup both at module level and nested under CFReader. That was always true. What the split added is that CFGroup.__module__ stopped naming the module being documented, so Sphinx emitted a canonical registration for each and the two collided. A broken pull request, not a cosmetic complaint. Worse, and silent: reference_terms disappeared from the API page altogether. Autodoc documents module-level data only where the module's own source assigns it. An imported name produces no entry, no warning and no failure. __all__ does not help - that rescues classes, via a __module__ check a dict does not have. Both fixes are in __init__.py. Re-pointing __module__ at the package is not a Sphinx workaround: __module__ is what repr() prints and what pickle records, and before the split all sixteen names said iris.fileformats.cf. Leaving it pointing into the private layout was itself the behaviour change. Restoring it fixes the duplicate as a side effect and costs no documentation - the nested CFReader.CFGroup entry still renders. reference_terms is re-stated with its doc comment, which is the only fix short of turning on imported-members for every module in Iris. Two tests hold them, and _defined_public_names now reads the module source rather than vars(), so it sees data members too - the kind of name that had already slipped through once. Docs build now reports "build succeeded." with no warning count, and all sixteen public names render. Full suite unchanged against the baseline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 39f0a4a4-3696-4644-a61d-d88088b71c86 * Act on the independent review A clean-context adversarial review of the preceding four commits confirmed the motion is byte-faithful and the netCDF seam one-way, and found four things worth fixing. The __module__ rewrite is withdrawn. It was introduced to stop Sphinx describing CFGroup twice, and it did - but inspect resolves a class's source through __module__, so pointing it at the package sent inspect looking for each class body in __init__.py, where it is not. That broke getsource and silently removed the [source] link from all fifteen classes on the API page, with no warning in a -W build to say so. One silent docs defect had been traded for another. The duplicate is now suppressed where it arises, with :meta private: on the CFReader.CFGroup alias, and the classes report their defining module exactly as iris.mesh's re-exports already do. Two module docstrings said things the code does not do. _variables claimed nothing in it reads array data, but CFCoordinateVariable.identify fetches values when called with monotonic=True; the docstring now names that path and where it is reached from. _group described promoted as recording variables referenced by nothing, which matches neither promotion path; it now describes both. test_derived_bounds_boundary_guard_continue_branch asserted something true whether or not its branch fired, so it could not detect the patch-target drift it exists to guard against. It is now parametrised over both cases and asserts on promotion, the branch's only observable - verified by re-pointing the target at the re-export, which now fails the test where before it passed. Also: ARCHITECTURE.md no longer points at the deleted cf.py, and the fresh-interpreter import test grows a timeout, since an import deadlock is what it looks for and would otherwise hang the suite rather than fail it. identify_mixins.py is deliberately untouched. It is byte-identical to #7259's, and that is the property which makes the test-carrying commit reviewable; its dropped assertion belongs as a comment on that pull request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 16168c01-abcd-41e8-a19e-87e22391c55b * Record the re-export docs pitfalls in docs/AGENTS.md Three ways a re-export package loses documentation, two of them silently and none of them visible to the test suite. Found the hard way while splitting iris.fileformats.cf; PRs 2 through 7 of the Zarr I/O programme will each create another such package. Filed under docs/ rather than the root AGENTS.md, which already exceeds its own 200-line budget and delegates documentation rules here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 16168c01-abcd-41e8-a19e-87e22391c55b * Record PR 1 in the spec Now that #7298 exists, section 12.1 and the header Progress line can name it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 16168c01-abcd-41e8-a19e-87e22391c55b --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Patrick Peglar <patrick.peglar@metoffice.gov.uk>
Three lessons from the cf package split (#7298), where the full suite was run about six times when seconds-long targeted runs would have done, and where eighteen CF integration tests were silently skipped throughout for want of iris-test-data. Correct the test runner row. pytest-xdist is installed but is not in addopts, so a bare pytest runs serial - roughly half speed on the full suite. The table said "-n auto" as though it were configured, which is where the belief that it is automatic comes from. Replace the flat "run targeted, then run everything" advice with tiers, and give indicative timings so the cost of reaching for the full suite is visible. Add the two counterintuitive findings, both measured: -n auto makes sub-ten-second selections slower rather than faster, and -n above the core count buys a few percent of wall time for a large increase in CPU. Note that -p no:cacheprovider, the obvious way to stop concurrent runs colliding, also disables --lf and --ff; -o cache_dir gives isolation and keeps them. Add the verification lesson worth the most: comparing failures before and after a change - the standard evidence that a refactor broke nothing - cannot see a test that never ran, because a skip is neither a pass nor a failure and a FAILED|ERROR diff filters it out. The comparison comes back clean whether coverage is intact or absent, so skips in the area under change have to be read directly. Session-Id: 16168c01-abcd-41e8-a19e-87e22391c55b Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* Agent guidance: don't let agents rewrite their own rules Four AGENTS.md files carried a "Meta-Instruction: Auto-Update Rule" directing an agent to update the file itself, without asking permission, silently, as its final step - and to delete existing rules to stay under a 200-line cap. Every part of that is a problem. Rules that govern how agents work should change the way anything else does: in a diff, with a reason, reviewed by someone. A silent edit is none of those. "Delete obsolete rules to make room" invites an agent to remove guidance it finds inconvenient, with "obsolete" as its own call. And the cap was already unmet by two of the four files carrying it - the root file at 259 lines and lib/iris at 215 - so an agent obeying the instruction would have started deleting at once. Replace it with the part worth keeping: if you learn a durable, reusable rule, propose it. The replacement is three lines shorter than what it removes, in each of the four files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Agent guidance: what a before/after test comparison cannot prove This file already warns that a failure diff cannot see a test that stopped running. Two more instruments in the same family read as evidence and are not. Warning totals are not reproducible under -n auto. Import-time module deprecations are counted once per worker that imports the module, so the total tracks how the tests were distributed rather than anything about the code. Five runs of one unchanging tree gave 6483, 6485, 6484, 6483, 6483. A recent pull request body quoted a one-warning increase and explained its cause; there was no cause, and the explanation had to be withdrawn. Error counts can depend on collection order. The ten errors in integration/netcdf/test_coord_systems.py appear in a tier run and not in the full suite, from module import order alone, so a number taken from one selection says nothing about another. Also: "do not weaken existing checks" is right but incomplete, because a test can pin a bug. One recently asserted that a write handle skipped string encoding - the behaviour that silently corrupted data. Inverting it strengthened the suite. Say so, and set the bar: prove the expectation was wrong before treating a red test that way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Agent guidance: an unreachability comment is a claim, not an aside This file already says a stale comment is worse than none, because an agent reads it as evidence. The most expensive shape of that is a comment asserting some branch cannot be taken: it is load-bearing, it is checked by nothing, and when the condition behind it changes there is no failure to notice. A real one read "only reachable for a dataset opened without string encoding; the saver always encodes". It had stopped being true, the branch it excused was reached from the saver, and lazy string data went to the file corrupted. Nothing in the suite could have caught it, because the claim lived only in prose. So: pin it with a test or an assert, or restructure until it is true by construction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Agent guidance: doctests are not in the pytest suite Nothing under lib/iris/tests runs the >>> examples - there is no --doctest-modules in addopts - so an agent that edits a docstring example and runs the tests gets a green suite and no signal at all. The examples are real: 29 library modules and 30 user guide files carry them. Only Sphinx runs them, and it is not cheap: CI's `nox -s doctest` does a clean html build, gallery included, before `make doctest`. Say so, so the cost is budgeted once before finishing rather than discovered in CI, and give the cheaper local form for iterating. Also warn off the obvious shortcut. `pytest --doctest-modules` and `python -m doctest` look like a fast substitute, but 150 testsetup and testcode directives supply context that only Sphinx applies, so they report failures that are not real - the worst outcome, since it teaches an agent to distrust the check. docs/AGENTS.md pointed at `make doctest` alone, without the build prerequisite or the CI session. Point it here instead of restating it, so the two cannot drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Fix typos and a truncated line in the AGENTS.md files Six corrections, no changes of meaning: - root: "chngelog/" -> "changelog/", and "fragments lives ... is built" -> "fragments live ... are built". - root: "pre-commit tht will ensure" -> "pre-commit, which will ensure". - root: the Testing section's first bullet stopped mid-sentence at "test-specific ". The identical bullet earlier in the same file ends "test-specific rules", so the intended wording was not a guess. - lib/iris/tests: "regressionc overage" -> "regression coverage", a transposition across a line break. - changelog/README.rst, which changelog/AGENTS.md is a symlink to: "changlog fragment file" -> "changelog". These survived because pre-commit never sees them. The global `files:` allow-list at the top of .pre-commit-config.yaml admits only workflows, Python under a few trees, and pyproject.toml - no markdown or rst at all - so codespell has never run on any of these files. Left alone here: widening that allow-list would pull every other document in the repository into scope at once, which is its own change with its own fallout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Let pre-commit see the AGENTS.md files The global files: allow-list admitted only workflows, Python under a few trees, and pyproject.toml. Every AGENTS.md fell outside it, so none of the hooks had ever run on one - which is how "chngelog", "tht", "regressionc overage" and a line that stopped mid-sentence survived in files whose whole purpose is to be read carefully. Add one alternation, matching AGENTS.md at any depth so files added later are covered without further edits. The allow-list only ever admits files, so nothing else changes treatment. Fallout, measured by running the hooks over all five: one trailing space in docs/AGENTS.md and two trailing blank lines in lib/iris/tests, both auto-fixed here. codespell passes. blacken-docs is types: [file, rst] and so does not touch markdown. Deliberately narrow: a pattern for markdown generally would pull every document in the repository into scope at once, which is a bigger change than this one and wants its own review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* Share the docs page metadata rules with pre-commit A page missing its `readingtime` directive or its sphinx-needs item could only be discovered by building the documentation, which takes minutes. That is a slow way to learn about a one-line omission, and an especially poor one for an agent, which will often skip the build entirely and let CI find it. Every rule involved is answerable from the page's own source text, so move them into `.hooks/check_docs_page_metadata.py` and run them from two places: a new pre-commit hook, and the documentation build. This repository already uses that arrangement for `check_netcdf4_imports.py`, which is both a hook and the body of a test in `test_coding_standards.py`. Sharing one implementation, rather than writing a fast approximation beside the real check, is what makes the hook trustworthy: the two cannot disagree about whether a page is valid, so a clean hook run means the build will not fail on metadata. The build keeps the rules it alone can apply -- the pages below `generated/` are written by autosummary and sphinx-gallery while the build runs, so they have no source for a hook to read. `readingtime_validator` is therefore redundant and removed: its rule is one of the shared ones, and leaving it in place would report each fault twice. Its replacement, `page_metadata_validator`, validates every page by whichever means can see it. `user_manual_directives` loses `validate_items` and goes back to being about the directive its name describes. Both are covered by tests carrying each way a page's metadata can be broken, so the checker itself cannot rot unnoticed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Check generated pages from their sources, not from the build The previous commit left the pages below `generated/` to the build, on the grounds that they have no source file to read. That was true of their reStructuredText and false of their metadata, which is authored in files that are under version control the whole time: - a gallery page's item lives in the module docstring of its script in `docs/gallery_code`; - an API page's item lives in the module's own docstring in `lib`, and reaches the page through `automodule`, which is why it is absent from the apidoc stub and present in the built needs view. So read the docstrings. `check_item` is the existing item check with the readingtime rule lifted out of it, applied now to whatever is rendered at the top of a page -- a page's own text, or the docstring a page is generated from. `library_modules` reproduces what apidoc is configured to build pages from; it selects exactly the 63 modules that have a page today, and additionally `iris.experimental.raster`, which is excluded from the build to avoid a gdal conflict but carries its item regardless. This is a smaller design than the one it replaces, and covers more. The validator no longer reads `get_needs_view`, so it no longer has to run late in the build: it reports at `builder-inited`, seconds in rather than minutes. 89 files that only CI would have checked are now checked before a commit -- including the case that motivated this work, a new library module whose docstring states no Diataxis type. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 09902815-5f62-42a6-b0ef-4c4903ad4a3b * Drop the changelog fragment while this branch is in flux The PR number it is named for was a guess, and the humans will decide what this work contributes once the branch settles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 09902815-5f62-42a6-b0ef-4c4903ad4a3b * Document the page metadata rules for agents An agent that adds a documentation page, a gallery script or a library module now has one place to read what metadata the page must carry, and is pointed at the hook rather than at a multi-minute build to check it. `docs/AGENTS.md` gains a Page Metadata section covering all three; a new `lib/iris/AGENTS.md` paragraph sits with the module docstring rules, because that is where a library module's item is actually written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 09902815-5f62-42a6-b0ef-4c4903ad4a3b --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
![scitools-ci[bot]](https://avatars.githubusercontent.com/in/212324?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Templating
This PR includes changes that may be worth sharing via templating. For each file listed below, please either:
- Action the suggestion via a pull request editing/adding the relevant file in the SciTools/.github
templates/directory. 1 - Raise an issue against the SciTools/.github repo for the above action if you really don't have 10mins spare right now. Include an assignee, to avoid it being forgotten.
- Dismiss the suggestion if the changes are not suitable for templating.
You will need to dismiss this review before this PR can be merged. Recommend the reviewer does this as their final action before merging, as this text will continually update as commits come in.
Templated files
The following changed files are templated:
-
.pre-commit-config.yaml, templated bySciTools/.github/templates/.pre-commit-config.yaml -
MANIFEST.in, templated bySciTools/.github/templates/MANIFEST.in -
benchmarks/benchmarks/generate_data/__init__.py, templated bySciTools/.github/templates/benchmarks/benchmarks/generate_data/__init__.py
Template candidates
The following changed files are not currently templated, but their parent directories suggest they may be good candidates for a new template to be created:
Footnotes
-
Include this text in the PR body to avoid any notifications about applying the template changes back to the source repo!
@scitools-templating: please no update notification on: iris↩
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7309 +/- ##
==========================================
+ Coverage 90.36% 90.88% +0.51%
==========================================
Files 93 100 +7
Lines 25847 26310 +463
Branches 4802 4827 +25
==========================================
+ Hits 23356 23911 +555
+ Misses 1710 1646 -64
+ Partials 781 753 -28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
* Give AGENTS.md tangible actions on comment and docstring brevity lib/iris/AGENTS.md states the intent — humans and agents both read this code, and models drift towards redundant comments — but turns it into no action a writer can check themselves against. "Be generous" was the only length guidance anywhere, written for module docstrings and easily read as licence for all prose. Add a Brevity subsection under Comments and Docstrings. Docstring length is left to judgement deliberately: every hard limit had a counterexample in Iris itself, and a rule that can be argued away teaches that this file's rules are negotiable. What replaces it is decidable at the moment of writing — delete each sentence and see what breaks, then place what survives by audience: user, maintainer, or reviewer. Comments do get a budget, since nothing about them is that context-sensitive. Add a section on the case that actually produces the bloat: facts found while debugging, which get written down at the line that taught them rather than in a home that fails loudly when they stop being true. Route those to a named test, with tests/AGENTS.md granting the terse register that keeps the test docstring from becoming the new dumping ground. Also ban citing artefacts of how the work was done — review findings, task or plan numbers, phases — in source, tests and published prose. They resolve to nothing for a later reader and nothing checks them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: a90b0788-0eb0-4728-b157-f150b44924fa * Credit the model at the start of anything posted to GitHub Iris has many authors using many models, each harness with its own attribution habits or none. A short uniform marker at the top of the text says who is speaking before the reader starts reading, which a footer cannot, and one form for every model beats a different footer for each. Stated as replacing any trailing attribution the harness asks for, since that is the only way it stays uniform. Commit messages are excluded: they are not posts, and their trailers are the one form of attribution that is machine-readable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: a90b0788-0eb0-4728-b157-f150b44924fa --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* Name the reply-shaped comment in lib/iris/AGENTS.md #7310 aimed the debugging rules at verbosity. Review of #7303 found a second failure mode that is independent of length: prose that reads as one side of an argument the reader never heard. Every example was short enough to pass the existing rules. The provenance is the same - these are written while investigating a test failure - so both edits land on rules that already exist. - "Facts Learned by Debugging" gains three mechanical tells, usable by a reviewer and not requiring the author to simulate their own ignorance. - The pre-finish stranger-read now names reply-shape alongside verbosity; pointed only at length, it passes a one-line fragment unchanged. 282 lines, under the 300 cap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Ban citations that decay: line numbers and counts Six comments on the Zarr I/O branch cited line numbers in other files. Every one was already wrong by the end of that branch: prose edits shifted the lines beneath them, silently retargeting each citation. Two ended up pointing at blank lines, one at deleted code. Review objected independently to numbering a set of assignments, on the same grounds - the count goes out of date and nothing notices. So the rule covers both, and both have the same fix: name the thing. Also adds a changelog note to check a name's provenance before recording its removal, so deleting something that predates the branch gets the fragment it needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Regroup prose rules; drop one war story Three fixes for accumulation damage, no rules changed: - "Be generous" was qualified fifty lines before it was stated. The qualifier now lives on the sentence it limits. - The two citation bans sat under "Facts Learned by Debugging", which is about where a discovered fact should live. They apply to every comment, so they move next to "link workarounds to their source" - the same subject stated positively. - The unreachable-branch rule carried two lines of incident history. Rules here should stand on their mechanism, so illustrations of a pattern stay and justifications for a rule go. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Give laziness its own section Laziness had three scattered one-liners - one in Explicitness, one inside the docstring-contract bullet, one in the checklist - against a hundred lines on how to write comments. It is the harder thing to get right and the more expensive to get wrong, and none of the three named an API. Names the accessors an agent should reach for, and the ones it should not. The checklist line stays; the Explicitness line moves here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Bulletise the specification section; tighten the __getattr__ ban Two scannability fixes, no rules changed. The specification section was the only unbroken ten-line paragraph in an otherwise bulleted file, and was doing three jobs in one block. The __getattr__ anti-pattern ran seven lines among one-liners; same content, six. Net zero lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Say what to do when a review contradicts a written rule A reviewer cannot be expected to hold every AGENTS.md in mind; an agent has just loaded all of them. That asymmetry has no rule attached to it, so the default is silent compliance, which converts one reviewer's preference into a change against a written convention with nothing downstream to catch it. Grounded in #7303, where five references to a design document tracked in the branch were removed on request and replied to with "Removed.", against a rule in lib/iris/AGENTS.md naming a design document as a sanctioned citation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Add changelog fragment Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* Ban citations that are relative to when they were written "This PR", "currently", "the new behaviour": each reads as fixed and is not. A reader a year later has a different "now", and nothing in the build checks that the sentence still holds - the same failure as a line number, which this bullet already covered. Found by review on #7303, which flagged two and a sweep of the same file turned up three more. Folded into the existing decay bullet rather than given its own, and the per-case mechanism for line numbers and call-site counts dropped to pay for it: both are plain from naming the thing. lib/iris stays at 299 lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Add changelog fragment Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Remove changelog entry. Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…st it (#7303) * Plan: PR 2 — CFDataset and the CF variable classes rewritten against it Thirteen tasks taking the narrow CFDataset/CFDatasetVariable ABC pair from spec §4.2 and routing every CF attribute and netCDF-API access in cf/, _nc_load_rules/ and netcdf/ through it. Two behaviour changes, both about attribute names that collide with a Python member: such an attribute now reaches the cube on load and the file on save. Not the `spans` fix spec §5 anticipated -- that gap turned out to be unreachable, so Task 7 characterises current behaviour instead. Fifteen findings from reading and running the code record where the implementation had to settle something §4.2 left open, and Task 13 writes those back into the spec so PRs 3-6 are built against the real interface. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 25b4f2ac-c5e0-47dc-9296-aef279fc9573 * Add the CFDataset interface and its tracking attributes mapping cf/dataset.py declares what the CF layer needs from a storage format: typed properties for the storage, and one open-world mapping for the CF attributes. TrackedAttributes records which attributes were looked up, which is how Iris decides what survives onto a loaded cube. Nothing is wired to it yet. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 56828920-920e-46d8-9a40-9016df2fc836 * Add NetCDFDatasetVariable, the read surface Implements the CFDatasetVariable interface over a thread-safe netCDF variable wrapper, plus the netCDF-only members - the VLEN check, the Xarray-bridge data array, and the backing wrapper itself - that the CF layer used to reach for through getattr. Not wired to anything yet. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 56828920-920e-46d8-9a40-9016df2fc836 * Add NetCDFDataset, the read surface Opens a netCDF file, or borrows an already-open one, and presents its variables, dimensions and global attributes through the CF interface. The NetCDF3 "use nccopy" warning moves here from CFReader, behind an explicit warn_legacy_format flag: the file format is netCDF vocabulary, and CFReader is no longer allowed to know it. Dimension lengths are read via DimensionWrapper.size rather than len(dimension): the wrapper is a composition object whose __getattr__ forwards ordinary attribute lookups but is never consulted for implicit dunder-protocol calls, so len() raises TypeError where .size reaches the same value. Allowlists the new test's direct netCDF4 import in check_netcdf4_imports.py, alongside the existing thread-safe-wrapper and bytecoding-dataset test exceptions: the test deliberately exercises NetCDFDataset.from_existing() against a bare netCDF4.Dataset, as the Xarray bridge hands to iris.save / CFReader. Not wired to anything yet. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 56828920-920e-46d8-9a40-9016df2fc836 * Explain and pin from_existing's stipulated "r+" mode Review flagged the hardcoded mode as looking arbitrary. It is deliberate: netCDF4 exposes no public attribute recording a dataset's open mode, so a borrowed dataset's true mode is unobservable. "r+" is correct for this PR's only caller (Saver, a write path) and consistent with the EncodedDataset wrapping applied just below, which __init__ also applies to every mode but plain "r". Adds a comment recording that rationale and a test pinning the stipulated value. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Add the netCDF CFDataset write surface create_dimension, create_variable and write_handle(). write_handle is the seam that lets a Dask worker write into a variable after the saver has closed the file, and is where the Zarr implementation will return a zarr.Array instead of a reopen-on-write proxy. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Write netCDF attributes through the CFDataset interface _NetCDFAttributes.__setitem__ applies the ASCII coercion that saver.py's _setncattr applied, so the coercion stays on the netCDF path - as the spec requires - instead of following CF attributes into generic code. _bytes_if_ascii moves from saver.py to _dataset.py; saver.py imports it back until its own _setncattr goes. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Pin create_variable's cache-update and wrap a long test line Add a test that materialises .variables before calling create_variable, so the "if self._variables is not None" branch in create_variable is covered by something other than a happy-path property rebuild. Verified by temporarily removing that branch: the new test fails, the rest of the suite is unaffected. Also wrap the institution-name line in test_global_non_ascii_round_trips under 88 characters, without changing the string's non-ASCII content. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Add the shared CFDataset contract test One test body, subclassed per implementation, as the spec's testing section requires. PR 4 supplies three fixtures and inherits the lot. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Fix round 1: document picklability scope, widen chunking check - Record why the shared contract deliberately does not round-trip write_handle() through pickle: the handle carries a backend-injected lock whose type depends on the active Dask scheduler, and it is unpicklable precisely where nothing ever pickles it (the default threaded scheduler), so a naive round-trip would wrongly fail a correct backend. - Accept numpy integers alongside plain int in the chunking assertion, since a Zarr chunk shape is plausibly numpy.int64. - Note that deprecated_netcdf_member is deliberately excluded from the shared contract, being an explicitly netCDF4-specific escape hatch. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Resolve CFVariable attributes against a declared mapping CFVariable.__getattr__ now reads self.attributes - a TrackedAttributes - instead of reaching for arbitrary members of the netCDF4 variable, and no longer caches what it finds onto the instance. dimensions, shape, ndim, dtype and size become declared properties, so structure and file data are no longer reached for through the same syntax. The instance cache had to go: it made a re-read after cf_attrs_reset() invisible to attribute tracking, and tracking is what decides which file attributes survive onto a loaded cube. cf_data is still a netCDF4 variable; that swap is later in this PR. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Pin the sharp edges of the CFVariable attribute rewiring Three classes of input the rewiring could plausibly break: a file attribute whose name collides with a CFVariable member; getattr with a default and hasattr, which the loading rules use 86 times between them and which depend on AttributeError and nothing else; and an attribute read again after cf_attrs_reset(), which is what removing the instance cache actually changes. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Pin what the typed properties do to attribute tracking A file attribute named after one of the five typed properties no longer vanishes from the loaded cube. The old __getattr__ recorded such a name as read before handing off to netCDF4, so _add_unused_attributes dropped it even though the value returned was never the file's; a declared property short-circuits __getattr__ and records nothing, so the attribute survives. That is a deliberate difference, and it deserves a test that says so. Parametrise the collision test over all five names and assert the tracking outcome through cf_attrs_unused(), not through attributes[name] - which would itself record the read the test exists to detect. Also assert each shadowed member's own value rather than merely that it is not the file's. The weaker form passed against a member returning None, and was trivially true of attributes and cf_data, which cannot equal a string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 99010c7d-5098-4102-9977-77317175fd9b * Pin how spans() treats the NCZarr _scalar_ dimension Committed before the code changes, so the record shows these pass against unmodified spans() methods. The design spec calls the missing _NCZARR_SCALAR_DIMENSION check in the three overriding spans() methods a latent bug. It is latent in the strong sense: unreachable. Each override opens with "if self.dimensions" and then tests set(source[:-1]), which for the one-element tuple NCZarr produces is the empty set - a subset of every target. The overrides already answer True. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 99010c7d-5098-4102-9977-77317175fd9b * Ask _is_scalar() rather than restate the rule four times CFVariable.spans and its three overrides each decided for themselves what a scalar variable is, and only one of the four knew about NCZarr's _scalar_ pseudo-dimension. The behaviour is unchanged - see the tests committed just before this - but the question is now asked in one place. That matters from PR 4: ZarrDataset derives dimension names from the store rather than from netCDF's rules, and will ask the same question of data that has never been through them. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 99010c7d-5098-4102-9977-77317175fd9b * Fix round 1: drop a non-discriminating spans test, add direct _is_scalar coverage Review found two gaps in the Task 7 tests, neither in spans() itself: - test_nczarr_scalar_target_is_spanned_by_a_scalar asserted nothing its name didn't already promise elsewhere. Once the source is scalar, spans() never inspects the target, so this passed for any target and was redundant with test_nczarr_scalar_dimension_spans. Removed. - _is_scalar() is this task's produced interface - the definition PR 4's Zarr backend will read directly - but was only exercised indirectly through spans(). Added TestIsScalar with direct cases: no dimensions, the NCZarr scalar dimension alone, that dimension alongside another, and ordinary dimensions. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 99010c7d-5098-4102-9977-77317175fd9b * Read CF attributes from cf_var.attributes in the loading rules 94 sites in helpers.py and 3 in actions.py: getattr(cf_var, CF_ATTR_X, None) becomes cf_var.attributes.get(CF_ATTR_X). The brief's line-based inventory (66 sites) undercounts because several `getattr(...)` calls wrap across 2-3 source lines and never appear as `getattr(cf_` on a single line; completeness here is proved by `grep -n 'getattr(\|hasattr('` against the *unmodified* file, not by the wrapped brief list. Four sites are not the plain rewrite: - helpers.py's `_add_or_capture` and actions.py:560 look up a name held in a variable. getattr string dispatch is banned by lib/iris/AGENTS.md; a mapping lookup is the sanctioned form, and reads as one. - helpers.py's `get_attr_units` asks whether a flag attribute exists without marking it used, so that it still reaches the cube. It said so by reaching past the CFVariable to the netCDF4 object; it now says so with .untracked. - helpers.py's `get_cf_bounds_var` (CF_ATTR_BOUNDS/CF_ATTR_CLIMATOLOGY) is deliberately NOT converted and stays as getattr(). iris.fileformats.cf. _reader synthesises a `.bounds` link for "newstyle" derived bounds (FUTURE.derived_bounds) by assigning `cf_var.bounds = ...` directly onto the CFVariable instance, bypassing `.attributes` entirely. Plain attribute lookup finds that value before `__getattr__` (and so `.attributes`) is ever consulted; `.attributes.get(...)` cannot see it and silently drops the derived-bounds link. Converting this site broke test_bounds_files.py; reverting just these two lines fixed it. This is a genuine gap in the pre-verified "31 CF_ATTR_* constants, zero netCDF4 API-surface collisions" check: the hazard here is a *different* kind of collision (a CFVariable-instance attribute set elsewhere in the codebase), not a netCDF4 Variable API member. No behaviour change for real files: the densest attribute check (test_netcdf__loadsaveattrs.py, 2059 tests) and the derived-bounds suite are both fully green, and the wider unit+integration selection reproduces the baseline exactly (4748 passed, 26 skipped, 10 known F14 errors, 0 failed). The regression net itself needed repair to prove this. ~20 fake `cf_var` construction sites across 24 test files modelled the *pre-Task-6* CFVariable: flat Mock kwargs (`Mock(standard_name=X, ...)`) that plain `getattr` read directly, never touching `.attributes`. Once the loading rules read `.attributes`, those doubles could not answer - not a false failure, a correctly-exposed broken double: it modelled an interface CFVariable no longer has. Added `CFVariableDouble` (lib/iris/tests/unit/fileformats/ nc_load_rules/helpers/__init__.py), a double with a real `TrackedAttributes` at `.attributes` and `__getattr__` routing to it, exactly like production CFVariable - so a test can read `double.name` to build its expected value and the production `double.attributes.get("name")` path resolves the same value from the same mapping. Rewired every affected construction site (and lib/iris/tests/test_netcdf.py::TestNetCDFCRS's hand-rolled `Var` double) onto it; no test assertion changed. nc_load_rules suite: 332 passed, 0 failed, matching the pre-Task-8 baseline exactly. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Fix round 2: pin the untracked flag probe, guard the double's recursion, restore types Four items from review: - test_get_attr_units.py: add test_flag_values_untracked, the one assertion that pins `name in cf_var.attributes.untracked` (not `.attributes`) in get_attr_units's flag probe. Confirmed it catches the regression: reverting the probe to a tracked read fails this new test. - helpers.py: comment at the `attr_key="cf_name"` site in build_and_add_names explaining why the capture branch there is unreachable today (cf_name is a structural member, not a file attribute) and what would happen if _build_name_var ever grew logic that could fail. - CFVariableDouble.__getattr__: add the same recursion guard production CFVariable carries (_variables.py:86,275) - dunder names and "attributes" itself must never resolve through self.attributes, or a blank instance (copy/deepcopy/unpickling via cls.__new__) recurses infinitely instead of raising AttributeError. - test__normalise_bounds_units.py: restore the type annotations removed in the previous round (`units: str | None = None, unitless: bool = False) -> CFVariableDouble`, `attrs: dict[str, Any]`), and mark the five call sites and two structural-attribute assignments with per-line `# type: ignore` and a comment, rather than hiding the CFVariableDouble/CFBoundaryVariable mismatch by leaving the helper untyped. No production behaviour changed; nc_load_rules suite is 333 passed (the existing 332 plus the new guard test), matching baseline plus one. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Read CF attributes from cf_var.attributes in the netCDF loader Seven sites. The constraint fast path also loses a computed attribute name and a comment about a CFVariable property cache that no longer exists. Four test files build CFVariable/CFDataVariable doubles that no longer match the interface: test__translate_constraints_to_var_callback.py's MagicMocks carried standard_name as a Python attribute, which is not where CFVariable looks any more; test__get_cf_var_data.py and test__load_cube.py spec a bare MagicMock(spec=CFVariable), which only allows names CFVariable declares at class level; `.attributes` is assigned in __init__, not declared, so the spec rejected it until added explicitly. `.dimensions` had to be set too, for a different reason: it IS a declared property, so the spec allows it, but the auto-created child mock iterates empty and test_cf_data_chunk_control's per-dimension branch never ran; test_build_and_add_auxiliary_coordinate.py::TestDtype set scale_factor and add_offset as plain Python attributes on a CFVariableDouble, which CFVariableDouble's own docstring reserves for structural members, not CF attributes - the sanctioned route is the attributes mapping, which _get_actual_dtype now reads. Fixing the constraint-callback doubles to drive ncattrs/getncattr also exposed that the "attribute not present -> not a mismatch" branch had never actually been exercised: the old MagicMock doubles auto-vivify any attribute access, so `hasattr` was always True and the miss path was dead code under test. With attributes genuinely absent, that branch now answers True more often, matching the code's stated intent ("the cube may still acquire the name later in the load") rather than the previous coincidental mismatch. Several expected result lists changed to match; this is a fix to what the doubles were pinning, not a change in production semantics for real netCDF4-backed variables, where hasattr and "in cf_var.attributes" already agreed on the miss path before this commit. The netCDF storage reaches in _get_cf_var_data - _data_array, datatype, chunking(), the proxy class - are deliberately untouched: they need cf_data to be a CFDatasetVariable, which is a later commit in this PR. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Read CF attributes from cf_var.attributes in the UGRID loader Fourteen sites, including the third and last computed attribute name in the loading path: getattr(mesh_var, connectivity.cf_role). Five hasattr probes - cf_role, topology_dimension and the three *_node_connectivity names - become "in mesh_var.attributes", which still records the attribute as read - otherwise face_node_connectivity and its neighbours would start appearing as stray attributes on every mesh cube. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Finish the UGRID read conversion and pin what the doubles model node_coordinates was the one CF read left as direct attribute access in an if/elif chain whose other two branches already go through .attributes. Mandatory on a mesh topology variable, so it subscripts; a miss becomes KeyError, as it already does for grid_mapping_name. CFVariableDouble now seeds ignored=_CF_ATTRS_IGNORE, as production CFVariable does, so scale_factor and friends start already-read in a double as they do in a real load. NameConstraint(long_name=...) was pinned by nothing: every shared test variable either matches long_name or omits it, and omission is permissive. One contradicting variable restores the discrimination. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Read and write CF attributes through cf_var.attributes in CFReader CFReader is the one module that writes CF attributes during a load: the formula-terms code synthesises a bounds link onto a variable that did not carry one. Those writes land in the mapping now, alongside the reads that look for them - moving one without the other would write a link nothing could see. Two sites needed a None guard rather than a rewrite. cf_root_coord and root_bounds_var can each be None, and getattr(None, "bounds", None) answers None where None.attributes raises. The first already had a comment saying so. The module docstring now names getncattr, which it always required: _attributes_source calls it for every name ncattrs() returns, and the docstring listing "variables", "dimensions" and "ncattrs" without it predates this PR. test_CFReader.py's netcdf_variable() mock built CFVariable.attributes from a hardcoded empty ncattrs(), while CF attribute kwargs (bounds, coordinates, standard_name, ...) were set as plain Mock attributes - harmless while _reader.py only read them by direct attribute access, which falls through to the raw mock on a miss. Routing those reads through .attributes exposed the gap: it stayed empty regardless of the kwargs given. ncattrs()/getncattr() are now wired to reflect the same attribute names, so a mutation made before the CFVariable is built - which is what every mutating test in this file does, on the line above its CFReader(...) - is seen by both paths. CFVariable.__init__ snapshots the attributes, so a mutation made after it is seen by neither. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Make the CFReader test double represent CF attributes by presence netcdf_variable() derived its ncattrs()/getncattr() from a curated name list filtered on truthiness. The list omitted long_name and carried climatology, and truthiness meant a legitimately falsy attribute could not be represented at all: standard_name="" produced an empty ncattrs() where a real file lists the name and returns "". Both surfaces now come from one presence-keyed dict of the CF kwargs the helper was given, and long_name is one of them. That makes the standard_name-or-long_name fallback in the formula-terms promotion reachable in a unit test for the first time; two tests now pin it, including the empty-standard_name-and-no-long_name case that raises. This also moves the freeze point. ncattrs()/getncattr() used to resolve through getattr(ncvar, ...) on every call; they now read cf_attributes, a dict fixed the moment netcdf_variable() returns. So no post-call mutation reaches .attributes any more, whatever its timing relative to CFReader(...) - narrowing what 8bfbb25a8's last paragraph described. Absence is now expressed by not passing the kwarg at all, which is why test_derived_bounds_promotes_reference_terms's "absent standard_name" case had to stop deleting the attribute after construction and build the variable without standard_name from the start. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Build a NetCDFDataset in CFReader, so cf_data is a CFDatasetVariable The swap this PR was for. CFReader opened a netCDF dataset and handed its variables straight out; every CFVariable therefore wrapped a netCDF4.Variable, and so did every consumer downstream. It now opens a NetCDFDataset, and cf_data is a CFDatasetVariable. Nothing outside iris.fileformats.netcdf calls netCDF4 API any more. The twelve identify() reads, CFVariable.__init__, CFReader's mesh probe, grid-mapping parse and global attributes, and the loader's five storage questions all go through the interface instead. _getncattr is gone: _NetCDFAttributes absorbs the malformed-file case it existed for. CFVariable copies the attribute mapping rather than wrapping it. The storage object's mapping writes through to the file, and CFReader synthesises a bounds link during load, on a file opened read-only. ugrid_load.py's mesh-location read no longer goes via cf_var.location, which now collides with CFDatasetVariable.location (the dataset's own path) rather than the UGRID node/edge/face location: it reads the already-validated local instead. The test doubles that stood in for a netCDF4 variable stand in for a NetCDFDatasetVariable now. Doubles that hand cf_data a plain array gain a small RealArrayCfData wrapper for the is_emulated/ is_variable_length gates _get_cf_var_data now checks unconditionally, and the ones in test__get_cf_var_data.py gain a spec= so that an invented member is an error rather than a mock. Assertions follow the type change throughout: test_CFReader.py's cf_data identity checks become cf_data.variable checks, and test_CFVariable.py's attribute-materialisation test is replaced by a snapshot test of the same construction-time contract. Part of #6977 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Deprecate reaching netCDF4 API through a CFVariable CFVariable.__getattr__ falls back to the backing variable for any name the CF attributes do not supply. That was how the class worked, and third-party code will have relied on it, so it stays for one release cycle - but it now says so. The warning waits until here because it could not have been switched on earlier: consumers came off the fallback one task at a time, and a warning during that would have fired once per attribute per variable per load. ugrid_load.py's own error messages were the one surviving reach-through - cf_var.name rather than cf_var.cf_name - found by running the netCDF, CF and integration suites with the warning enabled; fixed here alongside it, since that is what proves the route unused. It warns only on success. hasattr() probes reach this method, and a probe that comes back False has not used anything. Part of #6977 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Fix round 1: pin the borrow-decodes behaviour change, restore three coverage gaps A: NetCDFDataset.from_existing wraps anything lacking THREAD_SAFE_FLAG in an EncodedDataset, unconditionally. Before this branch, CFReader stored a borrowed dataset untouched, so a bare netCDF4.Dataset - what the Xarray bridge hands to iris.load - kept its character data as raw bytes; it is now decoded. This is a genuine, previously-undocumented behaviour change on a public entry point, not a defect: a char auxiliary coordinate that used to be silently dropped now loads. Pinned with TestABorrowedBareDataset in test_CFReader__dataset.py, which needs a function-local `import netCDF4` to build a genuinely bare dataset - added to check_netcdf4_imports.py's allow-list, following the pattern already used for test_NetCDFDataset.py. Documented the wrap's unconditional nature (it does not consult DECODE_TO_STRINGS_ON_READ) at the wrap itself. C: the "r+" stipulation's comment in NetCDFDataset.from_existing claimed one caller, a write path - false since 936ef88e7 added CFReader's borrowed-read branch as a second, read-only caller. Rewritten to name both callers and note nothing reads NetCDFDataset.mode but __repr__. D, E, G: three coverage gaps the review found by mutation, each restored with a test I applied the mutation to and watched fail before reverting from a /tmp copy: - _NetCDFAttributes's "read once, at construction" contract had no test since 936ef88e7 replaced the call-count test with a snapshot test instead of adding this one alongside it. - CFReader's borrowed-path wiring of its own warn= into NetCDFDataset.from_existing(warn_legacy_format=...) was untested; the owned-path equivalent already was. - test_getattr_of_a_non_attribute_reaches_the_variable lost its __getattr__-does-not-cache guard when it gained an assert_called_once_with; restored alongside it. F: renamed test_only_unread_attributes_reach_the_cube to test_an_unread_attribute_reaches_the_cube and recorded, in a comment, that the fixture cannot discriminate the complementary "only" direction - every attribute it reads is filtered by saver.py's _CF_ATTRS before tracking is consulted - rather than leave the name claiming more than the test pins. Part of #6977 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Characterise saving into a caller-supplied dataset iris.fileformats.netcdf.save() accepts an open dataset in place of a path, and ncdata uses that to translate between Iris and Xarray by passing an object that only emulates one. Nothing in the suite tested it, and the next commit rewrites every line it runs through. Written against unmodified code, so it is green on arrival. That is what makes it a guard rather than a specification. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Save through CFDataset rather than through netCDF4 Saver now owns a NetCDFDataset. Dimensions, variables, attributes and data writes all go through the interface, so the saver no longer knows what kind of file it is writing. Three netCDF-only reads remain, all through the explicit 'self._dataset.dataset' escape hatch: the file-format check that decides whether int64 is available, and the third-party cf_patch hook, which is documented as receiving a netCDF4 dataset. Grep for it to find them. The grid-mapping variable keeps a netCDF4 handle too. Its sixty-three parameter assignments bypass the ASCII-to-bytes coercion every other attribute goes through, so converting them would change the file - a separate change, recorded as finding F8. One deliberate behaviour change: a coordinate attribute whose name collides with a netCDF4 Python member - 'shape', 'size', 'dtype' and the rest - used to be dropped silently, because the "don't clobber" check asked hasattr(). It asks the attribute mapping now, and the attribute reaches the file. NetCDFDataset needed two changes to carry the saver. 'closed' now asks the file as well as the flag, because a borrowed dataset is closed by whoever opened it and the flag is never set; complete() has to refuse on the file's answer, not on its own. And create_variable() now registers in the variables mapping unconditionally, materialising it first if need be. Keeping an already-built mapping in step was not enough: each NetCDFDatasetVariable caches its own attribute values, so a second wrapper for one variable loses attributes written through the first - and the saver creates a variable, then looks it up by name to write more. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Give cf_patch back its netCDF4 variable, and pin what was unpinned cf_patch is a reserved iris.site_configuration hook, documented since Iris 1.3 as receiving netCDF4 objects. Saving through a CFDataset unwrapped the dataset it is handed but not the variable, so a third-party hook began receiving a NetCDFDatasetVariable: setncattr() raised, and a plain 'variable.name = value' went nowhere at all, a CF variable having no __setattr__. Both arguments are unwrapped now, and a test installs a real hook and reads the file back afterwards. The existing test installed a Mock and never looked at what it was passed, so it could not have caught either failure. Three things this branch changed or rewrote had no test that would notice them being undone: - An attribute named after a netCDF4 Python member - 'shape', 'size' and the rest - used to be dropped in silence and now reaches the file. Reverting that check to the old hasattr() left the whole suite green. - Eight of the sixty-three grid-mapping parameter writes were read back by nothing. They are correct, and byte-identical to the pre-branch tree, but this change rewrote all sixty-three of them by hand, and a mistake in one is silent: they are assigned straight onto the netCDF4 variable rather than through the attribute mapping. - The grid-mapping unit test said 'assert a, b', which is an assertion with a message rather than an equality. It is an equality now. Also corrected: a comment saying sixty-one assignments where there are sixty-three, and five docstrings still describing cf_var_cube as a netCDF4 variable, where internally it is now a NetCDFDatasetVariable. The cf_patch unwrap does not apply to those - it is at the call site. Saving through a CFDataset asks an object that only emulates netCDF4 - ncdata, the Xarray bridge - for three members the saver never used to reach for: Dimension.size, Dataset.ncattrs() and Variable.ncattrs(). All three are public netCDF4 API. They are recorded where the test emulators define them, deliberately with no fallback for an emulator that lacks them. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Reconcile the Zarr I/O spec with the interface PR 2 built Six things the spec's §4.2 sketch did not settle, which the implementation had to. None is a correction: §4.2 called its own listing "deliberately small: only what the CF variable classes actually need", and this is what they turned out to need. PRs 3 to 6 are written against §4.2, so it has to be the real interface. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Correct the PR 2 plan where the implementation proved it wrong The plan's last Definition-of-Done bullet asks for exactly this: "any step the implementation proved wrong corrected in place". Nine corrections, each marked in the text so a reader can see what moved and why. The behaviour-change count is the important one. The Definition of Done said two; the decision ledger records eight, and two of those eight are only partly pinned by tests. A pull request whose central claim is "nothing else changed" is only as good as its list of exceptions, so the list is now counted from the rulings rather than from this document, which had been wrong about the number twice. The rest: cf/dataset.py has never been free of the *word* netCDF and never could be, since spec 4.2 mandates deprecated_netcdf_member - so the boundary bullet and Step 7's grep now check imports and executable tokens instead of prose. Step 7's other greps become allow-lists, its count goes 66 -> 68 on the corrected grid-mapping total, and its -W error::IrisDeprecation check is replaced: that form exits 4 during conftest import on an unrelated, pre-existing deprecation, so it never ran. Step 6's diff regains the normalisation 6.3 prescribes. The grid-mapping count goes 61 -> 63 in all nine places. Task 12's summary said two self._dataset.dataset escape hatches where its own step text says three. Steps 3 and 4's spec entries go from five decisions to thirteen. And Step 8's body is replaced with the one that was actually written. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Correct the PR 2 body, plan and spec where review found them wrong Review of Task 13 raised five Important and four Minor findings; these are the accepted ones. Documentation only - no library behaviour changed. The important one is behaviour change 5. The body said the AttributeError -> KeyError conversion happened at three call sites; there are six, two of them on the *save* path in saver.py, where a reader of a list headed "behaviour changes" would not think to look. The three new ones were measured rather than extrapolated from the other three: softening each subscript to .get() and running unit/fileformats/ and integration/ leaves the suite identical to an unmutated control, so five of the six have no test that fails if reverted. The gaps ship disclosed rather than closed, and the body states that as a decision instead of leaving it open as "Reviewer's call" - closing them would mean five new tests for malformed-input paths, none reachable from a well-formed file, in the final commit of a branch whose whole claim is that it changed nothing. A ninth behaviour change joins the list: reaching a netCDF4 member through a CFVariable now emits an IrisDeprecation where it used to be silent, and that reaches callers outside Iris because CFVariable is exported. It appeared only under "Not here", so the defect was placement rather than concealment. Its pin was verified by reversion like every other item's - deleting the warn_deprecated call fails two of the three TestDeprecatedNetcdfMember tests and leaves the third passing. Every count word in the body was then re-derived by command rather than adjusted by eye, after this branch shipped a wrong number twice; that turned up "seven PolarStereographic attributes", which are seven assignments over six attribute names. The rest: the comment at helpers.py:1278 described a `cf_var.bounds = ...` assignment that does not exist on this branch and an attributes mapping that cannot see it, when in fact the link is stored in that mapping like any other attribute - and a stored None there means "invalidated", which this call site cannot tell from its own default. The load-half paragraph cited ugrid_load.py's `cf_var.name` reads, which this branch moved to `cf_name`; the true statement is stronger, and is what the reach-through-as-error run measures at zero. The plan described F14 twice as xdist scheduling, where it is deterministic on module import order. Task 11 Step 16 kept the `-W error::IrisDeprecation` command that exits 4 at collection and so never ran; Step 7's copy was corrected and this one was missed. And spec 4.2 gains __enter__ / __exit__, which the implementation has and the Definition of Done requires that block to match. pr-body.md and the plan's embedded copy of it are byte-identical, verified by extracting the block from the plan and diffing it. Note also that 11ba437a8's message says "Nine corrections" where there were ten - left alone, because history is not rewritten for a message count. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Fix the regression and the gaps the final review found Part A, the one that matters: saving a factory with an absent dependency raised AttributeError on this branch where the merge base succeeded. The rewrite of boundsterm_varname replaced getattr(termvar, "bounds", None), which tolerated termvar being None, with termvar.attributes.get("bounds"), which does not - and termvar is None whenever _name_coord_map has no name for the coordinate, as it has none for an absent orography. Restores the tolerance and pins it with a save/read-back test asserting both formula_terms strings, since a fix that silently dropped the orog term would also have stopped raising. The rest are gaps rather than faults: - The unlimited-dimension contract member is now exercised. It accepts either sanctioned answer, create or NotImplementedError, and rejects the third: quietly creating a fixed-length dimension instead. - _GETATTR_RECURSION_GUARD's comment justified its narrowness by a probe that no longer reaches __getattr__. Replaced with the reason that holds: a broader exclusion would hide real CF attributes, _Encoding and _FillValue among them. - CFVariable.filename's docstring sat before the try block, attached to nothing. - emulated_data_array's setter refused nothing, while its getter refused. Unguarded it would have written a file attribute named _data_array, because VariableWrapper.__setattr__ forwards every set to the contained object. - NetCDFDataset.closed's fallback for a borrowed emulator with no isopen() had no test; ncdata's emulator has one, so nothing else reached it. - Four added lines exceeded the 88-character limit. The pull request body gains a tenth behaviour change, not a tenth change: from_existing turns auto-chartostring off on a dataset the caller still owns, which the merge base's save path never did. The load path already did exactly this, and the call is inert for anything from_existing has to wrap, but a permanent change to a caller's object belongs in the list. The suite figures are re-measured to match: 11989 passed, +265. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Record PR 2 in the Zarr I/O progress table Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Fix two undocumented regressions the PR review found Both are behaviour changes this branch introduced without documenting them, which is exactly what the pull request's central negative claim forbids. Neither adds an item to the list of ten: they are regressions fixed before merge, not behaviour the branch means to introduce. A write handle now always encodes. NetCDFDatasetVariable.write_handle chose its proxy class from the variable's wrapper type, on the stated premise that an unencoded variable is unreachable from the saver. It is not: NetCDFDataset.from_existing only wraps a dataset that lacks THREAD_SAFE_FLAG, so a borrowed _thread_safe_nc.DatasetWrapper keeps plain variables, and a deferred save into one took the unencoded branch. Lazy U3 data saved that way reached the file corrupted - ['abc', 'def'] read back as ['aaa', 'ddd']. Restore the merge base's rule that writes are always encoded, since write-side string encoding is not selectable. A read no longer makes a write lock. NetCDFDataset handed every variable the dataset's lock as it built them, and that lock comes from _dask_locks.get_worker_lock, which refuses the process scheduler because the *saver* cannot use it. Merely listing variables is a read, so CFReader._has_meshes made iris.load() fail under dask.config.set(scheduler="processes"), reporting a scheduler "not supported by the Iris netcdf saver" in the middle of a load. Variables now take a write_lock_factory and call it inside write_handle(), so the lock is still one per dataset - which is the point of the dataset owning it - but is made only when a write actually needs it. The unit test that pinned the unencoded-proxy expectation is inverted rather than deleted, and its sibling renamed so the pair reads as the two halves of one rule. The two tests that observed the lock at construction time now observe it at write time, and additionally pin that no lock exists before a handle is asked for. Two regression tests are added: a borrowed-dataset string round trip asserting on the file's contents, and a load under the real process scheduler. Re-derived the suite numbers in the PR body by collection, per module: 11989 -> 11992 passed, and +265 -> +268 with 198 -> 200 tests in new modules and 66 -> 67 in modified ones. Skips and warnings unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Withdraw an invented cause for a noisy warning count The pull request body explained the 6482 -> 6483 step in the full suite's warning total as the legacy attribute-handling deprecation raised once more by an added save. Measurement does not support that. Five runs of one unchanging tree give 6483, 6485, 6484, 6483, 6483: the summary names the same 13 warning sites every time, and the total moves only because import-time module deprecations are counted once per -n auto worker that imports the module, so it tracks how the tests were distributed. The body's whole argument is that its numbers were measured rather than narrated, so an explanation fitted to noise is the one kind of error it cannot afford. Replace it with the measurement, say plainly that the warning totals carry no weight, and name the pass and skip counts - which are stable across those runs - as the ones the claim rests on. Prose only. No suite number changes, and the list of documented behaviour changes stays at ten. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4 * Give iris.fileformats.cf.dataset its API page metadata Every module apidoc turns into an API page must carry exactly one sphinx-needs item, written in its module docstring. This module was added without one, so the page generated from it had none and the documentation build reported an error. Takes the form used by its sibling, the `iris.fileformats.cf` package. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Trim comments and docstrings in netcdf/_dataset.py Actions Stephen Worsley's review of #7303, plus the brevity and ephemeral-reference rules added to lib/iris/AGENTS.md in #7310. - Module docstring gains a read/write orientation paragraph, answering the review's repeated "I'm not yet clear on the big picture", and loses its citation of a working spec path. - Rationale that defended the design, narrated the investigation or restated the signature is removed; contracts are kept. - Reasoning repeated at seven sites (the assert on _dataset) is dropped; the asserts remain. - "netCDF-only members below." now reads identically at both sites, as a label rather than an explanation. - ncdata is named, rather than alluded to as "the Xarray bridge". No production code changes; 620 lines to 526. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Action the second and third reviews of #7303 Stephen Worsley's reviews of 2026-09-29, 13:50 and 16:16, across the CF layer. Same themes as the first: prose that answers a question the reader never asked, and references to artefacts of how the work was done. lib/iris/fileformats/cf/dataset.py - Module docstring: states plainly that these classes are stricter than a netCDF variable about what attribute syntax may reach, rather than announcing "the split that matters"; names ``attributes`` as a property; drops the spec-path citation and the "netCDF today, Zarr next" aside. - TrackedAttributes, location, dtype, create_variable, __len__ and deprecated_netcdf_member: rationale cut, contracts kept. deprecated_netcdf_member now names the exception it raises. lib/iris/fileformats/cf/_variables.py - The __getattr__ recursion guard no longer defends itself against a broader exclusion nobody proposed. - __getattr__ likewise; "it" replaced by its referent. lib/iris/fileformats/cf/_reader.py - Spec-path citation removed. The comment justifying a line that this PR deleted goes too, including the part that predates the PR. lib/iris/fileformats/_nc_load_rules/helpers.py - Both comments added by this PR are removed, restoring the prior state. One asserted an unreachable branch in prose, which lib/iris/AGENTS.md requires be pinned by a test or an assert instead. No production code changes. 2054 passed, 23 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Action the fourth review of #7303 Stephen Worsley's review of 2026-09-30, on the saver and loader. lib/iris/fileformats/netcdf/saver.py - VariableEmulator removed. This PR left it referenced by nothing but the comment describing it; CFVariable now carries that role, marked with a TODO for the Union it becomes once Zarr has a CFDataset implementation. - The grid-mapping comment no longer counts the assignments it describes, or cites a review finding. - Thread-safety, file-lock and factory-bounds comments cut to their first claim; the ones that only restated the code are gone. - The write_handle comment is kept, per review, with a TODO to recheck it once Zarr behaviour is settled. lib/iris/fileformats/netcdf/loader.py - Dropped a comment that restated the line below it. changelog/7303.internal.rst - Added: this PR had no fragment, and it now removes a public name. 2054 passed, 23 skipped - unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Modify changelog entry. Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Action the fifth review of #7303, and sweep the rest Stephen Worsley's review of 2026-09-30, on the tests. The seven comments, and then every other instance of the same thing: grepping the PR's 62 changed files for finding, task, step, phase, PR and spec-section references turned up eleven more that the review had not yet reached, all in test docstrings and comments. Test names and docstrings now say what they pin, not which piece of process produced them. - test_bounds_files.py: "pre-existing behaviour" could be read as either "before this PR" or "outside the FUTURE context"; the sentence is cut to the half that carries the reason for asserting on the file. - contract.py: the write_handle pickling note goes from sixteen lines to five, keeping why a round-trip assertion would wrongly fail. - TestShadowedAttributeNames and TestReadAfterReset are named for their subject; the latter now points at the test that covers the same ground on a real file, rather than at a plan step. 2054 passed, 23 skipped - unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Restore spec reference. Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Replace line-number citations with the names they point at Six comments cited line numbers in other files. Every one of them was already wrong: the prose-trimming commits earlier in this branch shifted the lines beneath them, silently retargeting each citation at unrelated code. Two pointed at blank lines, one at code this branch deleted. Name the function, class or constant instead. Those are greppable, and they survive an edit above them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Restore the spec references removed during review A design document is a durable, greppable citation, and the spec file is tracked in this branch, so each reference resolves for anyone reading the code. Removing them during review lost that; this puts them back verbatim. The one exception is TestShadowedAttributeNames, whose docstring opened "Review Focus 1. Spec section 4.3's known limitation, pinned." Only the spec half returns: a review-focus number names an artefact of how the work was done and means nothing once the review is closed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Manual fixes to Sphinx references. * Quoting for grid_variable. * Rephrase fix to change. * Specifically reference design spec. * Fix trailing whitespace. * Action the sixth review of #7303 Four comments, all on test prose. The ":504" citation had already drifted: that line now holds a section comment, not the "continue" it named. Most of that comment answered an objection nobody raised - what the derived_bounds branch does - so only the fact the test turns on survives. "The whole point of the PR" and "pre-PR behaviour" say nothing to a reader after the merge. Swept the file for the same shape and found two more, in TestABorrowedBareDataset and TestAttributeTrackingAcrossReset; all now name #7303, which stays true and stays findable. The "known gap" comment, which two reviewers read and neither could follow, keeps its first claim - this test does not pin that *only* unread attributes reach the cube - and loses the mutation-testing evidence and the sketch of what pinning it would take. Twelve lines to three. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Say ncdata, not "Xarray bridge" #4994 "Xarray bridge" is the goal, and it is closed. ncdata is the package that delivers it, and ncdata is what actually hands Iris an object with the netCDF4 API, so that is what the comments should name. Seven sites, all added by this branch. Left alone: loader.py and saver.py, where the same phrase has been since #5212 in 2023, and docs/src/whatsnew/3.7.rst, which is a published release note. Also in this review: "naming the saver" now quotes the message it meant ("not supported by the Iris netcdf saver"), and two comments recording what a test used to be called or used to assert are cut back to what the test pins. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Update lib/iris/tests/unit/fileformats/netcdf/saver/test_Saver__user_dataset.py Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> * Build a real CFDataVariable instead of registering the double `CFDataVariable.register(CFVariableDouble)` bought exactly one thing: the `assert isinstance(cf_var, cf.CFDataVariable)` guarding the `capture_invalid=True` branch of `_nc_load_rules.helpers.get_attr_units`. Two tests in this package reach that branch; the other 331 never touch it. The price was out of proportion. `register()` writes into the production class's own ABC registry, process-wide, as a side effect of importing a test helper module. It leaks upward, so `isinstance(double, CFVariable)` became true as well - which the comment did not say. There is no `unregister`. Give those tests a genuine `CFDataVariable` instead, built by `real_cf_data_variable()` over storage carrying just the `attributes`, `location` and `dtype` that `CFVariable` reads. The assert is now exercised against the type it actually asserts, rather than against a class talked into claiming it, and no test mutates a production class. Subclassing `CFDataVariable` for the double itself was tried and rejected: `CFVariable` exposes `dtype`, `shape`, `ndim` and `size` as read-only properties delegating to storage, and nine files in this package assign them directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Rename CFVariableDouble to CFVariableStandIn The class docstring already opened "A minimal stand-in for CFVariable", and two other test modules in this branch use "stand-in" in the same sense. The name now agrees with them. "Double" also reads poorly in a CF codebase, where the word is a datatype first - the CDL fixtures a few directories over are full of `double phenom(x)`. Local variables and prose renamed to match; the CDL keyword is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1646c3f4-845e-4b3a-963f-84bc54921793 * Better phrasing for test_materialising_variables_makes_no_write-lock. Session-Id: cbe088b2-7705-4889-81aa-2512af9023b0 * Remove docstring for test_materialising_variables_makes_no_write-lock. Session-Id: cbe088b2-7705-4889-81aa-2512af9023b0 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: stephenworsley <49274989+stephenworsley@users.noreply.github.com>
* Add WRITING.md, a prose guide for comments, commits and GitHub posts Collected the sentence rules that make prose readable at a skim: put the conclusion in the first clause, one fact per sentence, name what you did instead of hedging, and no rhetorical contrast unless the reader would assume the rejected side. Added formatting rules that apply to GitHub posts alone: no hard wrapping, collapsed sections for evidence and never for a claim, two alerts at most, permalinks instead of line numbers. Moved three rules out of lib/iris/AGENTS.md instead of copying them, so each keeps one home: what decays silently, which artefacts of the work must never be cited, and the three tells that a comment is a reply to a reviewer. All three govern commit messages and pull request bodies as much as they govern code, and two copies would have drifted. Both AGENTS.md files stay under their 300-line cap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: cbe088b2-7705-4889-81aa-2512af9023b0 * Widen the WRITING.md reference to all agent-human communication Moved the pointer from "Posting to GitHub" into the orientation block at the top of the file. It was scoped to posts, which read as though chat replies, specs and plans were exempt. They are not: the sentence rules govern every piece of prose a person reads, and only the formatting rules are specific to GitHub. Added chat replies to WRITING.md's own scope list for the same reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: cbe088b2-7705-4889-81aa-2512af9023b0 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* Action the #7292 spec review by @pp-mo Fourteen of eighteen comments changed the design or the document; two were already covered, one declined, one a correction to the review itself. The substantive ones share a theme: the design had taken netCDF's guarantees as if they were properties of CF, and Zarr provides weaker ones. - Dimension names are optional in Zarr v3, entries may be null, and a name guarantees nothing across arrays. A store with no dimension information now loads with anonymous dimensions instead of raising, and the matching identity is the (name, size) pair. - CFDataset gains format, encoding and supports_dtype, with the rule that nothing above the interface tests the implementation type. The one site that would have broken it is the netCDF format-string branch in the saver, which PR 5 relocates into generic code. - CFVariable.__getattr__ is deprecated for removal a cycle later, adding PR 2.5. - The Zarr saver takes Zarr-native encoding keywords rather than translating netCDF ones onto codecs. - mode="a" is separated from append, and appending is declared out of scope. - Format detection splits local from remote; load_files keeps its invariant and a new load_stores takes directory and remote targets. - The dask cache key stays Array.metadata.to_dict(), with its assumption turned into a sensitivity test. Q3 closed, Q9 opened, #7328 and #7329 raised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 4c2b54ac-e589-4c9a-9174-5aac6cd9c711 * Replace jargon in the __getattr__ passages "carve-out" and "unearned" appear nowhere in Iris, and "buys nothing" is a metaphor rather than a statement. .github/WRITING.md bans all three: do not introduce a term of art that appears once in the repository, and no novel metaphors. The sentence also stacked them into a double negative, so a reader who decoded every word still had to work out that it agreed with the reviewer's conclusion while disagreeing with the reasoning. Split into two sentences that each say one thing. No change of meaning in either section. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: e903df09-9092-4917-864f-8ea03848eda8 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* Remove the getattr anti-patterns from lib/iris/AGENTS.md The rule was meant to protect greppability, and twice now it has cost more than it earned: first on #7303, where it read as banning the one `__getattr__` that CF attributes genuinely need, and again on #7316, where it read as banning the module-level `__getattr__` that PEP 562 added for deprecating a relocated name. Decision by @trexfeathers on #7316. All three bullets go: dynamically generated members, `eval`/`exec`/`getattr` string dispatch, and the `__getattr__`/metaclass ban with its narrow exception. The Greppability section's ban on constructing identifiers at runtime is untouched, and carries the reasoning the anti-patterns were standing in for. The Zarr spec and `cf/dataset.py` cited the exception; both now argue their case without it. The spec's dated decision-log entries are left as the record of what was decided when, with a new entry for this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 2798af64-37a9-4709-9e88-73c698f9d6b4 * Correct the record: the rule never caught anything The decision-log entry said the rule had "caused confusion twice", which overstates it. We never fell foul of it. Twice it was argued against and twice the code was kept unchanged - `CFVariable.__getattr__` on #7303, the module-level deprecation shim on #7316 - so its whole effect was to ask for the argument. That is the cost, and it is a different and smaller claim than the one the entry made. Correction from @trexfeathers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 2798af64-37a9-4709-9e88-73c698f9d6b4 * Keep eval and exec banned They shared a sentence with `getattr` string dispatch and went out with it, but they carry none of its ambiguity: no pull request in this programme has ever had to argue whether something counted as `eval`. Back as their own bullet, free of `getattr`. Metaclasses stay out. Say so if they should follow. Requested by @trexfeathers on #7335. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 2798af64-37a9-4709-9e88-73c698f9d6b4 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* Plan: PR 3 — the CF loader relocated, behind one storage seam Six tasks splitting netcdf/loader.py along the seam spec §4.1 implies: the CF rules engine and the dask chunking policy are not netCDF's work and move to a new iris.fileformats.cf.loader with git mv, while load_cubes, NetCDFDataProxy and DEBUG stay where they are because §4.8 names them as netCDF API that does not move. Reading a variable's array is the one place the format shows through, so it becomes a single abstract method -- CFDatasetVariable.read_data(policy) -- which PR 4's Zarr variable implements and the shared contract tests already cover. The policy is a zero-argument callable, deliberately: invoking it only once the result is known to be lazy keeps CHUNK_CONTROL.from_file()'s KeyError behind the small-enough-to-read-whole shortcut, where hoisting it would make small unchunked cubes start raising. No behaviour change intended. Fifteen findings record where reading the code settled something the spec left open; the largest reversed this plan's own first draft, which had DEBUG moving behind a deprecation forward. PEP 562 gives modules __getattr__ and no __setattr__, so a forwarded name can be read but never assigned -- and DEBUG is a flag users assign to. Every `netcdf.DEBUG = True` in the wild would have silently stopped working. Opens against brownfield while #7303 is still in review, so until that squash-merges GitHub will show its commits inside this PR. §2 records 26c4a73 as the fence and the rebase --onto that drops them afterwards. Part of #6977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 52cea575-d7cf-4b04-8636-9c92f4e138ae * Let as_lazy_data take its cache key from the caller Replaces the isinstance(data, NetCDFDataProxy) special case with a cache_key= keyword. The netCDF loader passes repr(proxy), so the keys are byte-for-byte what they were, and iris._lazy_data no longer imports anything from iris.fileformats. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6d3f88ec-4810-4835-9173-12b5725a0e1d * Give CFDatasetVariable a read_data hook The loader asked a netCDF variable for its data by building a netCDF proxy itself. It now asks the storage object, through one new abstract method, and supplies only the part that is CF's business: what chunking a lazy result should have. The netCDF body moves verbatim, branch order included: the KeyError for an unchunked variable under from_file() still sits behind the "small enough to read whole" shortcut, so a small contiguous cube still loads. Also repairs five test helpers outside the brief's file list (nc_load_rules/helpers, RealArrayCfData and two cf_data mocks) that constructed cf_data without a read_data method and so broke when _get_cf_var_data stopped branching on is_emulated/is_variable_length itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6d3f88ec-4810-4835-9173-12b5725a0e1d * Move the CF loader to iris.fileformats.cf.loader Interpreting CF metadata and running the loading rules is not netCDF's job; it only lived in iris/fileformats/netcdf/loader.py because netCDF was the only store that did it. The file moves with git mv so the history follows, and what stays behind at the old path is what is genuinely netCDF's: load_cubes, NetCDFDataProxy and DEBUG. CHUNK_CONTROL and ChunkControl move with the loader and keep warn-on-use aliases at their old names, per the spec's deprecation table. DEBUG does not move: it is a flag users assign to, and a module __getattr__ cannot intercept assignment, so forwarding it would have made setting it silently stop working. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6d3f88ec-4810-4835-9173-12b5725a0e1d * Fix review round 1: stacklevel, docstrings, explicit imports Four Minor findings from review of the CF-loader relocation: - The module __getattr__ forward called warn_deprecated with its default stacklevel=2, which names the forward's own call site inside netcdf/loader.py rather than the user's code - and because the "once per location" warning filter keys on that frame, it also meant one warning covered the whole process instead of one per call site. Use stacklevel=3, pinned by a test asserting the recorded warning's filename is the caller's, not loader.py's. - test__relocated_names.py's import-silence test reloaded the whole iris.fileformats.netcdf package, which never actually re-executes netcdf/loader.py's body (the package only fetches already-bound names from it) - so switched to reloading the submodule directly, which is what the test claims to pin. That reload rebuilds load_cubes as a new function object, though, breaking the `==` identity check ugrid_load.load_meshes relies on to recognise netCDF sources against the handler iris.fileformats.FORMAT_AGENT captured at startup - reproduced directly before fixing it by restoring load_cubes after the reload. - Two docstrings overstated: netcdf/loader.py's comment on the logger import line didn't mention it also pulls in _bytecoding_datasets, and cf/loader.py's "nothing here imports netCDF4" sat uncommented against three deferred reach-backs into iris.fileformats.netcdf for DEBUG and the saver. Both now name what they border. - Two fixtures (test_stringdata.py, test_delayed_save.py) assigned to iris.fileformats.cf.loader._LAZYVAR_MIN_BYTES without importing that submodule themselves, relying on another module having already done so - the same silent-failure shape test__relocated_names.py pins elsewhere. Added the explicit import, matching test_NetCDFDatasetVariable.py's existing pattern. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6d3f88ec-4810-4835-9173-12b5725a0e1d * Fix review round 2: comment the third deferred netCDF reach-back cf/loader.py's docstring claimed every deferred import back into iris.fileformats.netcdf is commented at its call site with why and when it goes, but the Saver import in _load_cube_inner had no such comment - only saver._CF_ATTRS and loader.DEBUG did. Added the matching comment so the claim is true of all three. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6d3f88ec-4810-4835-9173-12b5725a0e1d * Move the CF loader's tests to match The test tree mirrors the source tree, so the five modules that test functions now in iris.fileformats.cf.loader move with them. test_load_cubes and test__relocated_names stay: they test what stayed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6d3f88ec-4810-4835-9173-12b5725a0e1d * Point the chunking documentation at its new home The user manual told people to import CHUNK_CONTROL from the netCDF loader, which now warns. Two of the seven references are executed, so this is a doctest change as well as a prose one. The 3.8 and 3.9 whatsnew pages keep the old name: they record what shipped in those releases, where it was correct. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 6d3f88ec-4810-4835-9173-12b5725a0e1d * Fix final review round: six minor fixes for the CF loader move - Add a module __dir__ to lib/iris/fileformats/netcdf/loader.py so the relocated CHUNK_CONTROL/ChunkControl names are discoverable via dir(), inspect.getmembers() and tab-completion (PEP 562 __getattr__ only covers reads). - Document that mock.patch on the old netcdf.loader path is a silent no-op, alongside the existing note about assignment not being interceptable. - Add tests pinning both behaviours in test__relocated_names.py. - State, in CFDatasetVariable.read_data's docstring, that _LAZYVAR_MIN_BYTES must be read through the module object rather than imported by name, so tests can patch the cutoff - a constraint the ABC alone would not communicate to a future storage implementer. - Fix a wrong PR reference in test__get_cf_var_data's module docstring (the Zarr case is PR 4, loading, not PR 6, saving) and cite the design spec section instead of a programme-internal PR number. - Drop a review-finding ID and a "now" from a stale comment in test_build_and_add_auxiliary_coordinate.py, and replace three programme PR-number references in cf/loader.py with durable spec citations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: cbe088b2-7705-4889-81aa-2512af9023b0 * Record PR 3 in the spec's progress table The pull request is #7316. Also corrects #7298 from "In review" to "Merged" - the spec's header says §12 is where programme status lives, so a merged pull request still marked in review defeats the one place that is meant to be true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: cbe088b2-7705-4889-81aa-2512af9023b0 * Record #7303 as merged in the spec's progress table PR 2 landed today, which is what made the rebase onto brownfield possible. §12 is where this programme tracks state, so it says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: cbe088b2-7705-4889-81aa-2512af9023b0 * Action @stephenworsley's review of the CF loader relocation Eight comments on prose and comment hygiene, none changing behaviour. Name the deferred chunking callable `chunking_policy` at the call site, so the reader sees what kind of object `read_data` is handed, and say why it is deferred. Align the two `read_data` docstrings on the reviewer's wording for when the policy may be called. Drop the three ellipsis-continuation comment markers and the proxy-class rationale from the netCDF `read_data` body, and fix "Workaround for this it return". All four are text that moved verbatim from `netcdf/loader.py`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 2798af64-37a9-4709-9e88-73c698f9d6b4 * Action @stephenworsley's review of the PR 3 tests Name the contract's read_data test for the case it actually covers, a small variable, and say in it that the lazy path is a per-format test rather than a contract one. The contract is not exhaustive here and nothing said so. Pin _LAZYVAR_MIN_BYTES to zero in the two emulation tests. Both assert the chunking policy is never consulted, and both arrays were small enough that the read-whole branch would have been reason enough. With the cutoff at zero only the emulation branch can explain it. The relocation note in the user manual says "raises a deprecation warning" rather than "warns". Also, per the thread on the deferred imports: the two that go when the saver relocates now lead with a greppable `TODO:`. The third, for DEBUG, is permanent and keeps its plain comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 2798af64-37a9-4709-9e88-73c698f9d6b4 * More nuanced comment about partial _chunks_from_chunk_control. Session-Id: 2798af64-37a9-4709-9e88-73c698f9d6b4 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* Deprecate reading CF attributes as CFVariable attributes cf_var.units and cf_var.attributes["units"] are the same read, with the same effect on which attributes reach the cube. __getattr__ now warns on the spelling that cannot be type-checked, and that any class member of the same name silently shadows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 342f7a60-cd73-4ed9-a082-66946311e117 * Read the last four CF attributes through CFVariable.attributes get_cf_bounds_var and _build_connectivity were the only code in Iris still resolving a CF attribute through CFVariable.__getattr__, so a normal load emitted the deprecation added for callers outside Iris. Attribute tracking is unchanged: TrackedAttributes records a read from get() and from __getitem__ exactly as it did from __getattr__. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 342f7a60-cd73-4ed9-a082-66946311e117 * Add the implementation plan for Zarr I/O PR 2.5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 342f7a60-cd73-4ed9-a082-66946311e117 * Correct two comments the review showed to be false _build_connectivity claimed cf_role is always present because it is how a connectivity variable is identified. It is not: identify() finds the variable through the mesh variable's role attribute and never reads the connectivity variable's own cf_role, so both reads can raise. The coordinate test claimed attribute tracking is what keeps "bounds" and "climatology" off the loaded coordinate. _add_unused_attributes filters on netcdf.saver._CF_ATTRS, which holds both names, so that test passes with tracking broken. The connectivity test is the one that pins tracking. Widens the deferred issue to cover cf_role as well as start_index. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 342f7a60-cd73-4ed9-a082-66946311e117 * Correct the spec's account of PR 2.5 Four reads inside Iris still reached CFVariable.__getattr__ after PR 2, so section 5's claim that none did was wrong; they are now named, as is the measurement that found them. The silent-load test cannot promote the warning to an error. iris.load captures an exception raised while building a cube into LOAD_PROBLEMS, so an "error" filter drops the coordinate and the load returns silent and degraded - a test written that way passes against an implementation that still warns. IrisDeprecation lives in iris._deprecation, not iris.exceptions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 342f7a60-cd73-4ed9-a082-66946311e117 * Record PR 2.5 and the UGRID issue it turned up Cites #7332 from the code and the test that pin the current behaviour, instead of pointing at "the pull request this came from". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 342f7a60-cd73-4ed9-a082-66946311e117 * Give CFVariable.__getattr__ a single exit Reading a CF attribute warned on one exit of __getattr__ and delegated the warning on the other. A third exit added later would skip the obligation, and nothing would fail. A membership test on self.attributes replaces the try/except, which also keeps a KeyError off the traceback of a mistyped attribute: the fallback no longer resolves inside an except block. TrackedAttributes.__contains__ records a hit as read, so the usage record is unchanged. Recorded the general rule in lib/iris/AGENTS.md, beside the early-return rule that did not bound it. Both shapes stay legal; the new line says which to reach for when the function still owes work on the way out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: ff0b0a0c-e0cd-48fc-9385-68d00fdaa6f4 * Fix merge mistake. Session-Id: ff0b0a0c-e0cd-48fc-9385-68d00fdaa6f4 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Two instructions for future agents, neither affecting behaviour. lib/iris/AGENTS.md gains a TODO Comments subsection. Agents avoid the marker because the wider community discourages it, but Iris uses `# TODO: ` throughout and puts Ruff's TD and FIX rule families in the ignore list, so nothing here flags one. The rule says to use it, to leave it in the file rather than only in the pull request body, and to name the event that removes it. The Zarr spec gains PR 8, a terms-of-art sweep over everything the branch adds. The spec and the plans were written before the rule in .github/WRITING.md, so they may carry coinages it would now reject. A later agent decides whether a term is safe by checking whether the repository already uses it, so a coinage that reaches main cannot be told apart from an established one. The programme count goes from eight to nine, and the header's stale Last updated is corrected. Session-Id: 208cb6e9-cc1d-426c-9a1e-2dceff1d432b Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
) * Start the PR 5 plan: constraints, scope, findings, file structure The research half of the plan for relocating the CF saver. Records the five places the saver still names netCDF, the sibling relationship with PR 4, and the interface the tasks will leave behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 64bc7b40-f56b-470a-9d2a-ee65e6554380 * Add PR 5 plan task 1: the dataset answers for its dtypes Also records the NETCDF3_64BIT finding: netCDF4 reports that format as NETCDF3_64BIT_OFFSET, so the saver's narrowing never fires for it and an int64 save fails in the C library. Verified against the environment. The plan preserves it and raises an issue rather than fixing it here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 64bc7b40-f56b-470a-9d2a-ee65e6554380 * Add PR 5 plan task 2: relocate the dask lock toolkit Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 64bc7b40-f56b-470a-9d2a-ee65e6554380 * Add PR 5 plan task 3: name the saver's four netCDF hooks Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 64bc7b40-f56b-470a-9d2a-ee65e6554380 * Add PR 5 plan task 4: the move itself Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 64bc7b40-f56b-470a-9d2a-ee65e6554380 * Finish the PR 5 plan: repointing, the spec record, the pull request Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 64bc7b40-f56b-470a-9d2a-ee65e6554380 * Self-review the PR 5 plan: fix the signatures and the pointers The interface sketch disagreed with the tasks on three hook signatures, and four Review Focus entries named tests and steps that the tasks did not have. Also records which files look as though they need changing and do not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 64bc7b40-f56b-470a-9d2a-ee65e6554380 * Correct the PR 5 plan against PR 2.5, now merged IrisDeprecation lives in iris._deprecation and is re-exported from neither iris.exceptions nor iris.warnings, so the test file's import was wrong and a -W filter naming either would have failed open. The no-deprecation load test promoted the warning to an error, which #7333 showed passes against an implementation that still warns: iris.load captures the exception into LOAD_PROBLEMS and returns a degraded cube. It now records warnings and asserts the file contributed no load problems. Also picks up the new one-exit rule in lib/iris/AGENTS.md, and moves the baseline to the tip of brownfield. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 64bc7b40-f56b-470a-9d2a-ee65e6554380 * Stop the no-deprecation tests tripping the save_split_attrs warning A plain iris.save emits an IrisDeprecation of its own: the legacy attribute mode deprecated in Iris 3.8. Verified by recording warnings around a save on the current brownfield. Both tests now set the FUTURE flag, so what they assert is what they mean to assert. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 64bc7b40-f56b-470a-9d2a-ee65e6554380 * Ask the dataset whether it holds a dtype The saver read the open file's netCDF format string to decide whether to narrow an integer to int32. That read would become generic code naming netCDF formats once the CF saver moves, so the question now goes to the dataset as a capability, and the format survives only in the message the user reads. A file created as NETCDF3_64BIT reports itself as NETCDF3_64BIT_OFFSET and so escapes the narrowing, which fails the save. That is today's behaviour and is kept, with a test saying so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1aa0b859-b88c-47c7-b496-f2f17c513935 * Move the dask lock toolkit to iris.fileformats.cf The module coordinates dask workers writing to one target, and does it by asking dask which scheduler is in play. It imports nothing from Iris and nothing from netCDF4, so it was only ever netCDF's by where it sat. A second saver needing worker coordination should find it rather than grow its own. The code is unchanged. The docstring no longer describes the netCDF saver as the only caller. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1aa0b859-b88c-47c7-b496-f2f17c513935 * Gather the saver's netCDF-specific code into four methods The class mixes the CF data model with netCDF4's own vocabulary. Four blocks are the whole of the second part: creating a char variable over a string dimension, writing to an emulating variable or an NCZarr target, flushing those writes before the file closes, and handing the site-configured patch hook its unwrapped netCDF4 objects. Each is now a method with a name, called from the generic code that used to contain it. Nothing moves modules and nothing changes behaviour. _backend_write still types its cf_var parameter as NetCDFDatasetVariable, since its body reaches for netCDF-only members (is_emulated, unencoded_variable, emulated_data_array); _lazy_stream_data's own parameter widens to CFDatasetVariable ahead of the move, so the one call between them needs a type: ignore[arg-type]. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Session-Id: 1aa0b859-b88c-47c7-b496-f2f17c513935 * Relocate the CF saver to iris.fileformats.cf.saver Everything about writing a cube that does not depend on what it is written into now lives in iris.fileformats.cf.saver.CFSaver: the CF data model, and storage calls that go through CFDataset. iris.fileformats.netcdf.saver keeps what is netCDF's. The file to write into, the keywords netCDF4 accepts, and the four things netCDF does differently from a generic store. Saver, save and save_mesh stay exactly where they are, and Saver.write keeps its signature. Four names that are not netCDF's - CFNameCoordMap, CF_CONVENTIONS_VERSION, MESH_ELEMENTS and SPATIO_TEMPORAL_AXES - are reachable from both old homes, warning on use. The private constants moved too, and are imported back into iris.fileformats.netcdf.saver rather than forwarded. Code elsewhere in Iris still reads them from there, on the load path, where a deprecation per cube would be intolerable - and they are private, so owed no deprecation cycle. The next commit repoints those readers and the import goes with them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1aa0b859-b88c-47c7-b496-f2f17c513935 * Keep file_write_lock on the Saver API page Sphinx attaches a "#:" comment to an assignment only when the comment block runs straight into it. A plain "#" line sat between the two, so Saver.file_write_lock - a public attribute of a public class - dropped off the API page in the relocation. Reorder the three lines. sphinx.pycode.ModuleAnalyzer confirms it: before, ('Saver', 'file_write_lock') was absent from attr_docs; after, it is back with the text it had before the move. Also say in TestNothingInsideIrisUsesTheOldNames what the class really covers. A save and a load are quiet, which is not the same as nothing in the repository using an old name - two test modules still do, and the next commit repoints them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1aa0b859-b88c-47c7-b496-f2f17c513935 * Point every reader at the CF saver's new home The CF loader's two reaches into the netCDF saver were deferred with a comment saying they would become plain imports once the saver moved. It has, so they have. Four test modules read a relocated name through the netCDF package, which now warns. They read it from iris.fileformats.cf.saver instead, so that a full test run raises no deprecation of its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1aa0b859-b88c-47c7-b496-f2f17c513935 * Record the CF saver relocation in the design document Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1aa0b859-b88c-47c7-b496-f2f17c513935 * Record the pull request number in the design document Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1aa0b859-b88c-47c7-b496-f2f17c513935 * Correct the prose the relocation left untrue Four statements no longer held. A test docstring named two modules as still importing CF_CONVENTIONS_VERSION from iris.fileformats.netcdf, which 63accb7 repointed. Two comments in integration/test_cf.py still placed _CF_ATTRS in netcdf.saver. test__relocated_names called its list the methods the netCDF Saver answered to before, which counts the four hooks this branch invented. cf/_reader.py called itself the one module in iris.fileformats.cf coupled to the netCDF package, which cf/saver.py importing ATTRIBUTE_HANDLERS ended. The cf/saver.py module docstring promised that every storage call goes through CFDataset. _add_grid_mapping_to_dataset does not, so the docstring names the exception now, and the call site names what it costs a non-netCDF store: a bare AttributeError, because the line does not use deprecated_netcdf_member(). The import-cycle explanation was wrong in all three places it appeared. Tracing the module bodies from five entry points shows cf.saver running while the netCDF package is itself part-way through importing, not after it is fully loaded. Patching the eager netcdf import out of lib/iris/fileformats/__init__.py in memory, with its module-level load_cubes references made lazy, leaves all nine modules importing first cleanly, so that import is not what makes the cycle work. What does is netcdf/_attribute_handlers.py importing nothing from the CF package. The module docstring, the test docstring and the design document say that now, as does the pull request body. No code changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1aa0b859-b88c-47c7-b496-f2f17c513935 * Make the generic saver private The class a format subclasses is _CFSaver, not CFSaver. CFDataset is public because every member of it is something a format implements, and because a user can reach one of its variables through CFVariable.cf_data. The saver is the other shape: what a format implements is four private hooks, one of which the Zarr saver will supply and three of which it will inherit, while everything public on the class is the caller's API that each format's own public Saver inherits unchanged. Naming it publicly would promise the shape of thirty-six methods on the evidence of a single subclass, and only a deprecation cycle could take that back. The module stays public: it holds the four relocated names, and its docstring is where a format author is told what to subclass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 9b0539e4-6e83-4516-ae5b-c0800f4d83d7 * Fix the int64 narrowing on 64-bit-offset netCDF files netCDF4 reports a file created as "NETCDF3_64BIT" under the name "NETCDF3_64BIT_OFFSET", which was missing from the list of formats that cannot hold every dtype. Saving int64 data to that format therefore skipped the narrowing to int32 and failed inside the netCDF C library. The relocated saver is untouched: it asks supports_dtype, and the list it answers from is netCDF's own. Two tests existed only to pin the failure and are gone; the too-large-value test now runs over all three restricted formats, which pins that the message names the format of the file that was opened rather than the keyword the caller passed. Fixes #7337, at @stephenworsley's suggestion in review of #7338. The closing keyword goes on the merge-back, where the programme keeps them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 594a4b04-068d-461d-b814-86a0d6fea022 * Cut the import test to the one reachable order The test ran nine subprocesses, one per module in the CF and netCDF packages, to pin that each survives being the module a process asks for first. Python runs iris.fileformats.__init__ before any submodule body, and that imports the netCDF package, which draws in the whole cascade. Every one of the nine subprocesses therefore did identical work: the module it named was already in sys.modules by the time the interpreter reached it, so the final step was a dictionary lookup. One import of iris.fileformats covers every module in both packages. What the subprocess still buys is the cold start, with nothing pre-loaded, which an import inside the test process cannot give. Measured at 1.4s per case, so this returns about 11s of CPU to the suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 362bdfd8-761e-4373-80a0-f16d413f133e * Add missing pass. Session-Id: 362bdfd8-761e-4373-80a0-f16d413f133e * Mark the netCDF import as going, and say when The ATTRIBUTE_HANDLERS import leaves when the handlers themselves relocate into the CF package, which happens with Zarr saving. The docstring explained why the import is here but not that it is temporary, and nothing at the import site was greppable as a TODO. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 3ba642d6-761d-45d6-b741-6f172366fd86 * Don't attempt to exhaustively list possible format values. Session-Id: 87566283-5999-4e68-b805-e9275930f3e4 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* Start the PR 4 plan: header, constraints, scope and findings 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 * Add the PR 4 plan's file structure and interface design 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 * Add PR 4 plan tasks 1-3: the dependency, the interface, the variable 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 * Add PR 4 plan tasks 4-5: the Zarr group, and the shared contract 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 * Add PR 4 plan tasks 6-7: CF decoding, and the lazy read 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 * Add PR 4 plan tasks 8-10: spans, CFReader, and the Zarr loader 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 * Add PR 4 plan tasks 11-12: format detection and the load chain 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 * Finish the PR 4 plan: integration, docs, the pull request, verification 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 * Self-review the PR 4 plan: fix the pointers, close the gaps 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 * Correct the PR 4 plan against the files that are actually there 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 * Point the PR 4 plan's baseline at the merged spec The spec review merged as #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 * Base the PR 4 plan on zarr-io-pr-3, with a fork preview first 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 #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 * Add zarr as an optional dependency, and the package that uses it 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 * Split the Zarr store-builder tests into their own module 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 * Add format and encoding to the CF dataset interface 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 * Keep close and context-manager coverage in the read half of the contract 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 * Reflow CFDataset.format's docstring under the 88-character line limit 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 * Present a Zarr array through the CF dataset interface 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 * Present a Zarr group through the CF dataset interface 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 * Assert variables keep an omitted dimension name in their own dimensions 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 * Implement the CF decoding Zarr does not do for itself 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 * Stop an out-of-range missing-value attribute crashing the load 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 * Read Zarr data lazily, aligned to the store's own chunks 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 * Prove the Zarr read uses the cache, and stop a warning that cried wolf 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 * Ask the shared CFDataset read contract of the Zarr store 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 * Match CF dimensions on (name, size), not on the name alone 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 * Let CFReader read any already-open CF dataset 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 * Build cubes from a Zarr store 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 * Teach the format agent to recognise a Zarr store 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 * Skip the NetCDF-detection test when iris-test-data is absent 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 * Let the load chain reach a target that is not a file A new iris.io.load_stores answers 'this target is not a file', once, for a local directory and a remote URL alike, rather than teaching load_files about directories in one place and the scheme dispatch in another. The partition happens after glob expansion, so iris.load('data/*') still accepts a mix of netCDF files and Zarr stores. load_files no longer re-expands its input: _generate_cubes already runs expand_filespecs before partitioning files from stores, and expanding an already-expanded list silently collapses two identical paths produced by two different glob patterns into one, which broke the existing duplicate- match loading behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4 * Update the known-formats snapshot for the registered Zarr spec test_known_formats compares iris.fileformats.FORMAT_AGENT's registered specs against this snapshot. Registering the "Zarr" FormatSpecification added an entry the snapshot never gained, because the sweep that landed it covered lib/iris/tests/unit/io/ and lib/iris/tests/unit/fileformats/, and this file lives outside both. One line, in the alphabetical position FormatSpecification._sort_key already puts it: after the other priority-5 entries and before the first priority-4 one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4 * Restore load_files's public contract, and guard the dispatch split load_files is public API with no leading underscore and a documented promise that filenames may be globs; narrowing that silently to already-expanded paths broke the promise for any future caller. Its body moves to a private _load_files that expects pre-expanded paths - which is what the duplicate-collapsing fix actually needed - and public load_files goes back to expanding once, then delegating. _generate_cubes, the only caller, now calls _load_files directly with its own already-expanded, partitioned list, so the double-expansion bug stays fixed. Cross-reference comments at both independent file-or-store checks (loading.py's _generate_cubes and ugrid_load.py's load_meshes) name each other, so an edit to one is more likely to carry to the other - the duplication itself is intentional and out of scope here. The remote-URL dispatch test no longer calls iris.load against a real S3 URL: with s3fs installed (a common transitive Dask dependency) that would issue a genuine outbound request. It now mocks load_stores and asserts the URL reaches it unmangled, which is the actual claim under test, and was verified to fail under a reintroduced mangling bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4 * Load realistic Zarr stores end to end, both versions Version 2 comes from Iris's own NCZarr writer, so the native reader and netCDF-c are checked against each other. Version 3 comes from a fixture shaped like the NOAA GFS archive: sharded, packed, with a scalar grid mapping, a two-dimensional auxiliary coordinate and a base64 fill value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4 * Withhold netCDF-c's _NCProperties stamp, and add bounds to the Zarr fixture _NCProperties is netCDF-c's own bookkeeping attribute, stamped onto the root group of every store it writes; netCDF4-python hides it from ncattrs() by convention, so it has never reached a cube loaded through netCDF. Zarr has no equivalent convention, so the native reader surfaced it as a plain attribute, which would have put a stray provenance string on cube.attributes for every cube loaded from an NCZarr-written store. Withheld alongside _ARRAY_DIMENSIONS for the same reason, with its own unit test; the integration comparison no longer strips it by hand. Also adds bounds to the version 3 fixture's latitude coordinate: CFBoundaryVariable.spans is one of the overrides this pull request rewrote against the dimension contract, so a bounded coordinate exercises code this branch actually changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4 * Document Zarr loading The page a user comparing formats will look for, beside the netCDF one: what a store is, how a remote one is reached, what a Zarr dimension name does and does not guarantee, why the array fill value is not a missing-value flag, and why chunking rounds up. The S3 page no longer begins by saying this is impossible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4 * Fix review findings on the Zarr documentation page Remove branch-relative phrasing from the NCZarr section ("unchanged", "now uses", "has long been able to") and from the optional-dependency paragraph ("behaves exactly as before"): a reader with no memory of an Iris before native Zarr support has nothing for those words to contrast against. State both as plain facts about how Iris works instead. Pin the install example to the version floor Iris actually enforces, zarr >=3.0.8 per requirements/py3*.yml and the ImportError it raises, so following the page's own instruction cannot leave a reader on a version Iris still refuses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4 * Record PR 4 in the spec, and what implementation changed Closes open question Q2: fixed-length unicode arrays read as themselves on both Zarr versions, so #6961's note describes the specification rather than zarr-python. Logs the two decisions that move work between pull requests: format lands here rather than with the saver, and the shared dataset contract splits into read and write halves. Corrects two findings implementation disproved. zarr-python validates dimension_names against shape on read as well as on write, so a wrong-length version 3 store cannot be opened at all and the case is reachable only on version 2. The netCDF saver's refusal of a nested attribute names the attribute, though not the cube. Records two behaviours the design did not describe: _NCProperties is withheld from a Zarr dataset's attributes, matching what netCDF4 has always hidden on the netCDF path; and load_files keeps its glob-expanding contract as a wrapper over a private form, because the load chain expands once before partitioning files from stores. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4 * Give both Zarr load routes the same clear ImportError Calling iris.fileformats.zarr.load_cubes directly - the documented route for the group= keyword - raised a bare "No module named zarr" instead of the documented message, and nothing checked zarr's version, so a zarr 2.x install reached ZarrDataset and failed with an unrelated AttributeError instead of a clear error naming the version floor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4 * Fix the shared Zarr fixture's standing warning, and warn when a version 2 store's fill value will mask data stores.py gave its grid mapping no ellipsoid, so every test that loaded through it recorded a load problem and raised the warning that announces one, burying the one signal that would reveal a genuine Zarr load problem under a standing false positive; three independent reviews found this. The version 2 fill-value masking rule itself is unchanged - the design document's call to make, not this pass's - but nothing told a reader it had applied, so a store missing a real zero because of it looked the same as one that wasn't. Warn once per variable, from where the decode parameters are chosen rather than the per-chunk decode layer, naming the variable and the value. Also: corrects a design-document sentence that had the storage-attribute withholding backwards, records two deviations the implementation plan's own log had missed, updates a generic docstring left describing the bare-name matching this branch replaced, and corrects a comment that blamed a non-existent import cycle for a deferred import that no longer saves anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4 * Narrow the version 2 fill-value warning to a storage fill value of zero The masking rule is unchanged, but a realistic round trip through netCDF-c's NCZarr writer - a documented, supported path - raised the previous warning fourteen times across ten variables in one load, because NCZarr stamps its own pre-convention sentinel onto every variable regardless of CF attributes. That sentinel is a writer's deliberate choice, which is exactly the case design document section 4.4 keeps the rule for; zero is the case it calls out as the implausible one, since it is zarr-python's own default for a fill value nobody set. Warning only when the adopted sentinel is zero keeps the signal without training readers to ignore the whole class of warning. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 7a880931-8be4-4c5c-a2cd-afa8e486f9e4 * Updated lock files. Session-Id: 9ef4e3ac-2bfa-4661-98fa-c932129b9c36 * Decode every sentinel in a vector missing_value CF section 2.5.1 makes `missing_value` "a scalar or vector containing values indicating missing data", and netCDF4 masks on every element of one. The Zarr decoder passed the whole attribute to the scalar decoder, so a JSON array warned and was dropped - and on a version 2 store the variable then looked as though it declared no CF attribute at all, which handed the storage fill value the sentinel role and masked every valid zero. `_FillValue` is still read as a scalar, which is what the same section of the convention makes it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1847daff-208b-4cdf-9506-90fe16f8a998 * Size Zarr chunks on the decoded dtype, not the stored one A packed variable is unpacked block by block, on the way out of the graph, so what a task holds is the decoded block. Sized on the stored dtype, a (512, 512, 512) int8 array with float64 unpacking got one chunk spanning the array: 128 MiB stored, 1 GiB decoded, against Dask's 128 MiB target. The netCDF path reaches the same answer from the other end, its data proxy declaring the decoded dtype. For a variable with no packing the decoded dtype is the stored one, so nothing moves in the usual case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1847daff-208b-4cdf-9506-90fe16f8a998 * Carry the (name, size) rule through coordinate attachment `CFVariable.spans` matches a dimension on (name, size), but the loading rules then chose the cube dimension by name alone, with `cf_var.dimensions.index(name)`. Given a Zarr data variable whose dimensions are ('d', 'd') at sizes (2, 3) and a three-point coordinate on 'd', `spans` said yes and `.index('d')` answered 0, so the coordinate was rejected as the wrong length and recorded in LOAD_PROBLEMS - with the axis it belonged on uniquely identifiable. The identity rule becomes a function rather than a method, so that `spans` and the four `_add_*` routines decide by one rule rather than two that can drift apart. No netCDF file can observe the difference: a netCDF dimension name is unique within its group and carries one size. The mocked data variables in the load-rule tests gain the shape they would always have had. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1847daff-208b-4cdf-9506-90fe16f8a998 * Rule the Zarr store element out when a file handle is supplied `requires_fh = False` means "consult me without a handle", not "consult me with one". File dispatch passes `pathlib.Path(fn).name` - a basename, not a path - so the store test resolved it against the working directory: a NIMROD file named `weather` was classified as Zarr whenever the working directory happened to hold a `weather/zarr.json`, the priority-5 Zarr specification being consulted before priority-3 NIMROD. A store is a directory or a URL, so a target that opened as a single seekable file has settled the question already. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1847daff-208b-4cdf-9506-90fe16f8a998 * Record the review findings in the spec Four amendments, one per finding: the (name, size) rule carrying through coordinate attachment and the identity rule becoming a function (4.4); vector `missing_value` (4.4); chunks sized on the decoded dtype (4.4); the store element ruled out when a file handle is supplied (4.7). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1847daff-208b-4cdf-9506-90fe16f8a998 * Size as_dask() chunks on the decoded dtype too `CHUNK_CONTROL.as_dask()` hands sizing to Dask, and `da.from_array` sizes on the array it is given - the stored one - so the packed variable's 8x survived there after the default path was fixed. Dask's policy is now called directly, as `normalize_chunks("auto", shape, dtype=decoded_dtype)`, and the result passed on with every dimension fixed so that `as_lazy_data` cannot re-size it on the stored dtype. The algorithm and the configured target are still Dask's; only the dtype it budgets against is supplied rather than inferred. The (512, 512, 512) packed int8 probe now gives (255, 255, 255) under `as_dask()`: 126.5 MiB decoded, against the 128 MiB target. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1847daff-208b-4cdf-9506-90fe16f8a998 * Trim the Zarr docs to what a user acts on Review on #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 * Assemble the Zarr dimension mapping when the store is opened 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 * Action the local review points on the Zarr reader `_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 * Share the URI categoriser, and name the Zarr versions read `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 * Drop chunk alignment; warn for both backends instead 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 * Action four more review points `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 * Load UGRID meshes from Zarr, and action the rest of PR 4's review 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 * Say why a netCDF-named module names Zarr 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 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
the
benchmark_this
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
2 participants
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.
Description
WIP pull request tracking the difference between the
brownfieldfeature branch andmain.Checklist
Important
The Iris core developers are here to help! If anything below is unclear, just post a comment asking for help 😊
(further reading)
Tip
Things you can trigger on this PR:
9999for this PR's number - to re-trigger the CLA check:https://cla-assistant.io/check/SciTools/iris?pullRequest=9999