diff --git a/.github/workflows/benchmarks_run.yml b/.github/workflows/benchmarks_run.yml index bada87bd85..6cf29848d5 100644 --- a/.github/workflows/benchmarks_run.yml +++ b/.github/workflows/benchmarks_run.yml @@ -73,7 +73,7 @@ jobs: env: IRIS_TEST_DATA_LOC_PATH: benchmarks IRIS_TEST_DATA_PATH: benchmarks/iris-test-data - IRIS_TEST_DATA_VERSION: "2.30" + IRIS_TEST_DATA_VERSION: "2.32" # Lets us manually bump the cache to rebuild ENV_CACHE_BUILD: "1" TEST_DATA_CACHE_BUILD: "2" diff --git a/.github/workflows/ci-tests.yml b/.github/workflows/ci-tests.yml index 45ec8cb151..2e915912f9 100644 --- a/.github/workflows/ci-tests.yml +++ b/.github/workflows/ci-tests.yml @@ -54,7 +54,7 @@ jobs: env: # NOTE: IRIS_TEST_DATA_VERSION is also set in benchmarks_run.yml - IRIS_TEST_DATA_VERSION: "2.30" + IRIS_TEST_DATA_VERSION: "2.32" ENV_NAME: "ci-tests" steps: diff --git a/benchmarks/benchmarks/generate_data/um_files.py b/benchmarks/benchmarks/generate_data/um_files.py index 40bf83e79c..4801956d98 100644 --- a/benchmarks/benchmarks/generate_data/um_files.py +++ b/benchmarks/benchmarks/generate_data/um_files.py @@ -2,7 +2,7 @@ # # This file is part of Iris and is released under the BSD license. # See LICENSE in the root of the repository for full licensing details. -"""Generate FF, PP and NetCDF files based on a minimal synthetic FF file. +"""Generate FF, PP, NetCDF and Zarr files based on a minimal synthetic FF file. NOTE: uses the Mule package, so depends on an environment with Mule installed. """ @@ -122,12 +122,13 @@ def add_field(level_: int, time_step_: int) -> None: ff_path = save_paths.get("FF", None) pp_path = save_paths.get("PP", None) nc_path = save_paths.get("NetCDF", None) + zarr_path = save_paths.get("Zarr", None) if ff_path: new_ff.to_file(ff_path) if pp_path: fields_to_pp_file(str(pp_path), new_ff.fields) - if nc_path: + if nc_path or zarr_path: temp_ff_path = None # Need an Iris Cube from the FF content. if ff_path: @@ -139,12 +140,18 @@ def add_field(level_: int, time_step_: int) -> None: new_ff.to_file(temp_ff_path.name) ff_cube = load_cube(temp_ff_path.name) - save_cube(ff_cube, nc_path, zlib=compress) + if nc_path: + save_cube(ff_cube, nc_path, zlib=compress) + if zarr_path: + # Zarr's own spelling of compression. The Zarr saver rejects + # netCDF keywords such as zlib= rather than translating them. + compressors = "auto" if compress else None + save_cube(ff_cube, zarr_path, mode="w", compressors=compressors) if temp_ff_path: temp_ff_path.close() -FILE_EXTENSIONS = {"FF": "", "PP": ".pp", "NetCDF": ".nc"} +FILE_EXTENSIONS = {"FF": "", "PP": ".pp", "NetCDF": ".nc", "Zarr": ".zarr"} def create_um_files( @@ -177,7 +184,9 @@ def create_um_files( for file_type in file_types: file_ext = FILE_EXTENSIONS[file_type] save_path = (save_dir / f"{compress}").with_suffix(file_ext) - files_exist = files_exist and save_path.is_file() + # exists() not is_file(): a Zarr store is a directory, so is_file() + # is always False for one and REUSE_DATA would never hold. + files_exist = files_exist and save_path.exists() save_paths[file_type] = str(save_path) if not REUSE_DATA or not files_exist: diff --git a/benchmarks/benchmarks/load/__init__.py b/benchmarks/benchmarks/load/__init__.py index 5c5a62a515..abba7ea577 100644 --- a/benchmarks/benchmarks/load/__init__.py +++ b/benchmarks/benchmarks/load/__init__.py @@ -18,7 +18,7 @@ class LoadAndRealise: params = ( [(50, 50, 2), (1280, 960, 5), (2, 2, 1000)], [False, True], - ["FF", "PP", "NetCDF"], + ["FF", "PP", "NetCDF", "Zarr"], ) param_names = ["xyz", "compressed", "file_format"] @@ -78,7 +78,7 @@ def time_stash_constraint(self, _, __, ___) -> None: class TimeConstraint: - params = ([3, 20], ["FF", "PP", "NetCDF"]) + params = ([3, 20], ["FF", "PP", "NetCDF", "Zarr"]) param_names = ["time_dim_len", "file_format"] def setup_cache(self) -> dict: diff --git a/benchmarks/benchmarks/save.py b/benchmarks/benchmarks/save.py index 4bac1b1450..efdc5d7940 100644 --- a/benchmarks/benchmarks/save.py +++ b/benchmarks/benchmarks/save.py @@ -9,35 +9,48 @@ from .generate_data.ugrid import make_cube_like_2d_cubesphere +#: Target name and save keywords per format. Zarr needs mode="w" because its +#: saver defaults to mode="w-" and raises FileExistsError on a target that +#: exists - and ASV calls a timing function many times between setup() calls. +SAVE_TARGETS = { + "NetCDF": ("tmp.nc", {}), + "Zarr": ("tmp.zarr", {"mode": "w"}), +} -class NetcdfSave: - params = [[50, 600], [False, True]] - param_names = ["cubesphere-N", "is_unstructured"] - def setup(self, n_cubesphere, is_unstructured): +class Save: + params = [[50, 600], [False, True], list(SAVE_TARGETS)] + param_names = ["cubesphere-N", "is_unstructured", "file_format"] + + def setup(self, n_cubesphere, is_unstructured, file_format): self.cube = make_cube_like_2d_cubesphere( n_cube=n_cubesphere, with_mesh=is_unstructured ) + self.save_target, self.save_kwargs = SAVE_TARGETS[file_format] def _save_data(self, cube, do_copy=True): if do_copy: # Copy the cube, to avoid distorting the results by changing it # Because we known that older Iris code realises lazy coords cube = cube.copy() - save(cube, "tmp.nc") + save(cube, self.save_target, **self.save_kwargs) def _save_mesh(self, cube): # In this case, we are happy that the mesh is *not* modified save_mesh(cube.mesh, "mesh.nc") - def time_netcdf_save_cube(self, n_cubesphere, is_unstructured): + def time_save_cube(self, n_cubesphere, is_unstructured, file_format): self._save_data(self.cube) - def time_netcdf_save_mesh(self, n_cubesphere, is_unstructured): - if is_unstructured: + def time_save_mesh(self, n_cubesphere, is_unstructured, file_format): + # iris.mesh.save_mesh is defined in iris.fileformats.netcdf.saver and + # writes netCDF only, so there is nothing to time for Zarr. A cube + # that *has* a mesh still saves it through time_save_cube, with + # is_unstructured=True. + if is_unstructured and file_format == "NetCDF": self._save_mesh(self.cube) - def tracemalloc_netcdf_save(self, n_cubesphere, is_unstructured): + def tracemalloc_save(self, n_cubesphere, is_unstructured, file_format): # Don't need to copy the cube here since track_ benchmarks don't # do repeats between self.setup() calls. self._save_data(self.cube, do_copy=False) diff --git a/docs/superpowers/plans/2026-10-26-zarr-io-pr7.md b/docs/superpowers/plans/2026-10-26-zarr-io-pr7.md new file mode 100644 index 0000000000..18fa721678 --- /dev/null +++ b/docs/superpowers/plans/2026-10-26-zarr-io-pr7.md @@ -0,0 +1,1019 @@ +# Zarr I/O PR 7 — Real-world test data and benchmarks: Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +| | | +|---|---| +| **Spec** | [`docs/superpowers/specs/2026-09-21-zarr-io-design.md`](../specs/2026-09-21-zarr-io-design.md) §5 (PR 7), §6, §7, §12.1, §12.5 | +| **Implements** | Pull request 7 of 9. `Part of #6961` — never a closing keyword. | +| **Branch** | `zarr-io-pr-7`, branched from `zarr-io-pr-6`. Opened as a preview pull request in `trexfeathers/iris` targeting `zarr-io-pr-6`; retargeted at `SciTools/iris:brownfield` once [#7346](https://github.com/SciTools/iris/pull/7346) merges | +| **Baseline** | `f594416da` on `zarr-io-pr-6`; Iris `3.17.0.dev16`; zarr `3.4.1`; xarray `2026.9.0`; numpy `2.5.2`; asv `0.6.x`; Python 3.12 | +| **Delivers** | The published NOAA GFS version 3 store in `SciTools/iris-test-data`, integration tests that load and save it, and ASV coverage of Zarr saving and loading | +| **Behaviour change** | None. Every library file this plan touches is a test module, a benchmark, or a design document. The one exception is `benchmarks/benchmarks/generate_data/um_files.py`, which is benchmark support code and reaches no shipped module. | +| **Public API change** | None. | + +**Goal:** Prove the native Zarr reader and writer against a store NOAA actually publishes, and give ASV a Zarr save and a Zarr load to watch for regressions. + +**Architecture:** One published store — the NOAA GFS forecast archive, version 3 and sharded — is contributed to `SciTools/iris-test-data` from the subset already cut. A new module under `lib/iris/tests/integration/zarr/` loads and saves it and asserts what it shows that no synthetic fixture does. A second, small module fills the one read-side gap that dropping the ESA EOPF fixture leaves. The benchmarks follow the shapes already in the repository: `ZarrSave` mirrors `NetcdfSave` over the same cubesphere cubes, and `LoadAndRealise` grows a `"Zarr"` file format beside its `"NetCDF"` one. + +**Tech Stack:** Python 3.12–3.14, zarr-python ≥3.0.8, xarray (test-only), pytest, ASV, pre-commit (ruff, numpydoc, and the repository's local hooks). The companion repository is `SciTools/iris-test-data`, via the fork `trexfeathers/iris-test-data`. + +--- + +## Global Constraints + +Every task's requirements implicitly include this section. + +| Constraint | Value, verbatim from its source | +|---|---| +| Target branch | `brownfield`, not `main` (spec §5). Until #7346 merges, the preview pull request targets `zarr-io-pr-6` | +| PR labels | `Agentic` and `Type: Feature Branch` (spec §5) | +| Issue reference | `Part of #6961`. **Never** a closing keyword — the issues close at merge-back (spec §5) | +| Changelog | No `changelog/` fragment in this pull request. Fragments are written once at merge-back; the pull request body must say so (spec §5) | +| GitHub posts | Every pull request description, comment and review reply opens with `(posted by Claude Opus)`, before anything else. This **replaces** any trailing harness attribution (root `AGENTS.md`) | +| Commit messages | Out of scope for the above; they keep `Co-Authored-By:` trailers | +| No network in tests | "**Remote stores** are covered by an in-memory store and a local directory store only. No test touches the network" (spec §6). Every new test reads a local path under `TEST_DATA_DIR`, or builds a store in `tmp_path` | +| Test skipping | A test needing `iris-test-data` carries `@_shared_utils.skip_data`. `lib/iris/tests/integration/zarr/__init__.py` already carries `pytest.importorskip("zarr", minversion="3.0.8")` and covers everything under it (spec §6) | +| Test style | pytest style, plain assertions, no unittest classes in new files (`lib/iris/tests/AGENTS.md`) | +| Test prose | A test docstring is notes, under about six lines. Never name a task, plan or review finding in a test name, docstring or comment; cite an issue, a pull request or a design-document section (`lib/iris/tests/AGENTS.md`) | +| Benchmark data generation | "Be sure not to use the benchmarking environment to generate any test objects/files" — anything a benchmark needs built is built through `benchmarks/benchmarks/generate_data` and `run_function_elsewhere` (`benchmarks/README.md`) | +| Benchmark re-runs | ASV calls a timing function many times between `setup()` calls (`benchmarks/README.md`). A benchmark that writes must survive being called again (F7) | +| Third-party data | Data contributed to `iris-test-data` from an outside publisher keeps its upstream attribution and names its upstream licence. The repository's own blanket licence statement is not this pull request's to change (F16) | +| Copyright header | Every new Python file opens with the four-line BSD header (root `AGENTS.md`) | +| Line length | 88 characters (Ruff default) | +| Published prose | `.github/WRITING.md` governs every line written for a person to read, including this plan, the two pull request bodies and the test-data README | +| Before finishing | Run the repository's pre-commit hooks and fix everything they report (root `AGENTS.md`) | + +### Running anything + +The conda environment is `iris-dev`. Either activate it, or prefix commands with +`conda run -n iris-dev`. It now holds `asv`, `nox` and an editable install of +Iris, added for this work with the user's agreement; `zarr 3.4.1` and +`xarray 2026.9.0` were already there. Do not install or remove anything else +without asking. + +Set `NPY_DISABLE_CPU_FEATURES="AVX512F,AVX512CD,AVX512_SKX"` before running the +suite, per the root `AGENTS.md`. + +`iris.config.TEST_DATA_DIR` resolves to +`/data/home/martin.yeo/repos/iris-test-data/test_data`, a checkout of +`trexfeathers/iris-test-data`. Task 1 puts the new store there, which is what +lets Task 3 run green before the companion pull request merges. + +### Running the benchmarks + +Do not use `nox`, and do not let ASV build an environment. `conda.anaconda.org` +answers 403 on this network, the `ssstack` lockfile support lives on the +`bm-agents` branch and not on this one, and `ssstack` is not on `PATH`. Use the +route that bypasses environment building altogether (F11): + +```bash +export OVERRIDE_TEST_DATA_REPOSITORY=/data/home/martin.yeo/repos/iris-test-data/test_data +python benchmarks/bm_runner.py trialrun "" "$(which python)" +``` + +Anything built by `create_um_files` needs Mule in that interpreter, which +`bm_runner` installs only when it builds the environment itself. Task 6's first +step installs it. + +## Review Focus + +Five things that no test in the suite exercises and that a person loading +published Zarr would meet in ordinary work. Each is pinned by a test in the +task named. All five were run against the GFS subset or against a store built +in `tmp_path` on 2026-10-26; none is speculative. + +1. **A CRS variable a store references through `coordinates` rather than + `grid_mapping`.** The NOAA GFS archive does this. Iris builds no coordinate + system from it, so `cube.coord("latitude").coord_system` is `None` and + `spatial_ref` arrives as a scalar auxiliary coordinate carrying `crs_wkt`. + A user who sees a `crs_wkt` in the file and no coordinate system on the cube + will read it as an Iris bug. → **Task 3, Step 1**: + `test_the_crs_variable_arrives_as_a_coordinate_not_a_coordinate_system`, + and **Task 7** raises the issue. +2. **An attribute whose name collides with an array property.** The published + store gives `lead_time`, `expected_forecast_length` and + `ingested_forecast_length` an attribute literally called `dtype`, holding + `"timedelta64[us]"`. That is one of the six names the merge-back changelog + records as having been silently shadowed on load, and the programme's fix + for it has never been run against a store that actually contains one. → + **Task 3, Step 1**: `test_an_attribute_named_dtype_does_not_shadow_the_dtype`. +3. **Replacing a store that already exists.** `test_save.py` pins that the + default `mode="w-"` refuses an existing target, and that `mode="a"` adds to + one. Nothing pins that `mode="w"` — the keyword §4.5 offers as the way back + to replacement — actually replaces, so a script rerun over the same path, + or an ASV timing function called a second time, rests on an untested + promise. → **Task 3, Step 1**: + `test_an_existing_store_is_replaced_under_mode_w`, and **Task 5, Step 1**, + where `ZarrSave` depends on it. +4. **Reading a fixed-length unicode variable.** ` List[Path]`. + +- [ ] **Step 1: Show the defect** + +```bash +cd /data/home/martin.yeo/repos/iris-test-data/indexing +conda run -n iris-dev python -c " +import generate_index as g +from pathlib import Path +print(len(g.getDataFilepaths(Path('../test_data/Zarr')))) +" +``` + +Expected: a count in the tens — one path per `zarr.json` and per chunk object +(F13). + +- [ ] **Step 2: Make a store one path** + +Change `getDataFilepaths` so that a directory whose name ends in `.zarr` is +yielded as a single path, and nothing beneath it is walked. Leave +`IGNORE_SUFFIXES` and every other file alone. + +- [ ] **Step 3: Re-run the count** + +Run Step 1's command again. Expected: `1`. + +- [ ] **Step 4: Check the rest of the pipeline takes a directory** + +```bash +conda run -n iris-dev python -c " +import generate_index as g +from pathlib import Path +infos = g.getFileInfos(g.getDataFilepaths(Path('../test_data/Zarr'))) +for key, items in infos.items(): + for info in items: + print(info.path, '|', len(info.items or []), 'items |', info.exceptions) +" +``` + +Expected: one entry, with a cube summary and no exception. `Image.open` on a +directory raises `IsADirectoryError`, which is an `OSError` and so is caught by +the existing `except IOError`. + +- [ ] **Step 5: Commit** + +```bash +git add indexing/generate_index.py +git commit -m "Index a Zarr store as one item, not as its chunks" +``` + +- [ ] **Step 6: Push and open the companion pull request** + +```bash +git push -u origin zarr-samples +gh pr create --repo SciTools/iris-test-data --base master \ + --head trexfeathers:zarr-samples --title "A NOAA GFS Zarr sample store" +``` + +The body opens with `(posted by Claude Opus)`, says what the store is and where +it came from, says that the indexing change is what keeps a store from becoming +thirty-five INDEX entries, and names the licence. It is not hard wrapped. + +--- + +## Task 3: The NOAA GFS sample, loaded and saved + +Characterisation, not development: the behaviour already exists, and these +tests say what it is. A test that fails here is a defect to record and raise in +Task 7, not to fix in this pull request. + +**Files:** +- Create: `lib/iris/tests/integration/zarr/test_gfs_sample.py` +- Modify: `lib/iris/tests/integration/zarr/fixtures.py` + +**Interfaces:** +- Consumes: `iris.tests._shared_utils.skip_data` and `get_data_path`, and + Task 1's `test_data/Zarr/gfs_forecast_sample.zarr`. +- Produces: nothing other code calls. + +- [ ] **Step 1: Write the module** + +Every test carries `@_shared_utils.skip_data`. A fixture returns +`_shared_utils.get_data_path(("Zarr", "gfs_forecast_sample.zarr"))`. A second +fixture, modelled on `_assert_clean_load` in `test_load.py`, resets +`LOAD_PROBLEMS`, escalates `IrisLoadWarning` to an error for the duration, and +asserts `LOAD_PROBLEMS.problems == []` afterwards — the sample loads silently +(F1), so the strict fixture is correct here. + +```python +def test_the_sample_yields_four_cubes(sample): + cubes = iris.load(sample) + assert sorted(cube.name() for cube in cubes) == [ + "air_temperature", + "eastward_wind", + "northward_wind", + "precipitation_flux", + ] + + +def test_the_sharded_variable_loads_lazily(sample): + cube = iris.load_cube(sample, "air_temperature") + assert cube.has_lazy_data() + assert cube.shape == (2, 8, 120, 120) + + +def test_the_two_dimensional_time_coordinate_spans_both_its_dimensions(sample): + cube = iris.load_cube(sample, "air_temperature") + assert cube.coord_dims(cube.coord(var_name="valid_time")) == (0, 1) + + +def test_the_base64_fill_value_is_read_and_masks_nothing(sample): + cube = iris.load_cube(sample, "air_temperature") + assert not np.ma.is_masked(cube.data) + + +def test_the_crs_variable_arrives_as_a_coordinate_not_a_coordinate_system(sample): + cube = iris.load_cube(sample, "air_temperature") + assert cube.coord("latitude").coord_system is None + assert "crs_wkt" in cube.coord("spatial_ref").attributes + + +def test_an_attribute_named_dtype_does_not_shadow_the_dtype(sample): + cube = iris.load_cube(sample, "air_temperature") + coord = cube.coord("forecast_period") + assert coord.attributes["dtype"] == "timedelta64[us]" + assert coord.dtype == np.dtype("f8") + + +def test_a_structured_attribute_survives(sample): + cube = iris.load_cube(sample, "air_temperature") + assert cube.coord(var_name="valid_time").attributes["statistics_approximate"] + + +def test_the_sample_round_trips(tmp_path, sample): + cube = iris.load_cube(sample, "air_temperature") + with iris.FUTURE.context(save_split_attrs=True): + iris.save(cube, tmp_path / "rt.zarr") + back = iris.load_cube(str(tmp_path / "rt.zarr")) + del back.attributes.globals["Conventions"] + assert back == cube + + +def test_an_existing_store_is_replaced_under_mode_w(tmp_path, sample): + ... + + +def test_the_round_trip_does_not_keep_the_source_chunk_layout(tmp_path, sample): + ... + + +def test_xarray_opens_the_round_tripped_store(tmp_path, sample): + ... +``` + +`test_the_crs_variable_arrives_as_a_coordinate_not_a_coordinate_system` carries +the Review Focus 1 docstring: the sample's data variables name `spatial_ref` in +`coordinates` and carry no `grid_mapping` attribute, so there is nothing for +Iris to build a coordinate system from, and the issue Task 7 raises is cited by +number. + +`test_an_attribute_named_dtype_does_not_shadow_the_dtype` carries the Review +Focus 2 docstring: the publisher records the source NumPy dtype in an attribute +of that name, and `dtype` is one of the names that used to be shadowed on load. + +`test_an_existing_store_is_replaced_under_mode_w` saves one cube to a path, +then saves a differently named cube to the same path with `mode="w"`, and +asserts the reloaded store holds only the second. `test_save.py` pins what +`mode="w-"` refuses and what `mode="a"` adds; `mode="w"` is the third of +§4.5's three modes. + +`test_the_round_trip_does_not_keep_the_source_chunk_layout` opens both stores +with `zarr.open_group` and asserts the source array is sharded — `shards == +(1, 4, 120, 120)` — while the saved one is not. Its docstring says Iris sizes +chunks itself and neither §4.4 nor §4.5 promises to preserve a store's layout +(F2). + +`test_xarray_opens_the_round_tripped_store` saves all four cubes, opens the +result with `xarray.open_zarr`, and asserts the four data variable names. The +assertion is on the open, because the failure mode this guards is a `TypeError` +during open (spec §6). + +- [ ] **Step 2: Run the module** + +```bash +conda run -n iris-dev pytest lib/iris/tests/integration/zarr/test_gfs_sample.py -v +``` + +Expected: every test PASSES, and none is skipped. A skip means +`TEST_DATA_DIR` is not pointing at Task 1's output — fix that rather than +accepting it, per `lib/iris/tests/AGENTS.md`. + +- [ ] **Step 3: Correct the synthetic fixture's docstring** + +`fixtures.py` says "A cut-down copy of the real store replaces this." It does +not: the synthetic store keeps `test_load.py` running where `iris-test-data` is +absent, and it is the more CF-conforming of the two, because it gives its data +variable a `grid_mapping` attribute that the published store does not have. +Replace that sentence with what is true, and say where the real store's tests +live. + +- [ ] **Step 4: Run the whole Zarr integration package** + +```bash +conda run -n iris-dev pytest lib/iris/tests/integration/zarr/ -v +``` + +Expected: everything that passed before still passes, and the new module's +tests pass. + +- [ ] **Step 5: Commit** + +```bash +git add lib/iris/tests/integration/zarr/test_gfs_sample.py \ + lib/iris/tests/integration/zarr/fixtures.py +git commit -m "Load and save the NOAA GFS sample store" +``` + +--- + +## Task 4: A fixed-length unicode variable loads + +The one thing the EOPF fixture was going to cover that nothing else does (F6). +It needs no external data, which is the whole reason it survives the cut. + +**Files:** +- Modify: `lib/iris/tests/unit/fileformats/zarr/loader/test_load_cubes.py` + +**Interfaces:** +- Consumes: `iris.fileformats.zarr.load_cubes`, already imported in that module. +- Produces: nothing other code calls. + +- [ ] **Step 1: Write the test** + +Add one parametrised test beside the existing synthetic load tests. + +```python +@pytest.mark.parametrize("zarr_format", [2, 3]) +def test_a_fixed_length_unicode_variable_loads(tmp_path, zarr_format): + ... + assert coord.points.tolist() == ["b02", "b03", "b04"] +``` + +It builds a store in `tmp_path` holding a `