From 5656e29975964ec0c63c455b0fac3354e3cef8f4 Mon Sep 17 00:00:00 2001 From: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> Date: Mon, 21 Sep 2026 09:55:39 +0100 Subject: [PATCH 01/19] Convert `test_coding_standards` to pre-commit hooks (#7285) * Experimental pre-commit hook. * netcdf4_imports fail faster. * Introduce .hooks directory. * Improved structure. * Fully replace test_coding_standards. * Add AGENTS.md note. * Fix MyPy. --- .hooks/README.md | 5 + .hooks/check_categorised_warnings.py | 110 ++++++++ .hooks/check_license_headers.py | 184 ++++++++++++++ .hooks/check_netcdf4_imports.py | 106 ++++++++ .hooks/check_python_versions.py | 198 +++++++++++++++ .pre-commit-config.yaml | 32 +++ AGENTS.md | 1 + MANIFEST.in | 1 + lib/iris/tests/test_coding_standards.py | 320 ++++++------------------ 9 files changed, 716 insertions(+), 241 deletions(-) create mode 100644 .hooks/README.md create mode 100644 .hooks/check_categorised_warnings.py create mode 100644 .hooks/check_license_headers.py create mode 100644 .hooks/check_netcdf4_imports.py create mode 100644 .hooks/check_python_versions.py diff --git a/.hooks/README.md b/.hooks/README.md new file mode 100644 index 0000000000..35143ce7de --- /dev/null +++ b/.hooks/README.md @@ -0,0 +1,5 @@ +# Local pre-commit hooks + +Each module in this directory is a local pre-commit hook for Iris, written in the format expected by the pre-commit framework. See [`.pre-commit-config.yaml`](../.pre-commit-config.yaml). + +**TODO: review this new framework fully.** diff --git a/.hooks/check_categorised_warnings.py b/.hooks/check_categorised_warnings.py new file mode 100644 index 0000000000..a3d90122a3 --- /dev/null +++ b/.hooks/check_categorised_warnings.py @@ -0,0 +1,110 @@ +# Copyright Iris contributors +# +# 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. + +"""Pre-commit hook: ensure all warnings are categorised. + +All UserWarnings raised by Iris must be categorised (not use the base +UserWarning class), to allow users to filter them. + +Usage (pre-commit passes staged file paths as arguments):: + + python .hooks/check_categorised_warnings.py [file ...] + +Exit codes: + 0 – all clear + 1 – one or more files have categorisation issues +""" + +import ast +from pathlib import Path +import sys + + +def _get_warnings_issues(path: Path) -> list[str]: + """Return a list of warning categorisation issues in *path*.""" + try: + file_text = path.read_text() + except (OSError, UnicodeDecodeError): + # Let other tools handle file read errors + return [] + + try: + tree = ast.parse(source=file_text, filename=str(path)) + except SyntaxError: + # Let other hooks (check-ast) handle syntax errors + return [] + + violations = [] + + # Find all .warn() calls + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + + # Check if this is a .warn(...) call + if not (isinstance(node.func, ast.Attribute) and node.func.attr == "warn"): + continue + + warn_ref = f"{path}:{node.lineno}" + + # Check for 'category' keyword argument + category_kwargs = [k for k in node.keywords if k.arg == "category"] + + if not category_kwargs: + violations.append( + f"{warn_ref}: warning raised without category= kwarg — " + "use a specific IrisWarning subclass" + ) + else: + category_kwarg = category_kwargs[0] + # Check if it's using the base UserWarning + category_name = None + if isinstance(category_kwarg.value, ast.Name): + category_name = category_kwarg.value.id + elif isinstance(category_kwarg.value, ast.Attribute): + category_name = category_kwarg.value.attr + + if category_name == "UserWarning": + violations.append( + f"{warn_ref}: warning uses base UserWarning — " + "use a specific IrisWarning subclass" + ) + + return violations + + +def check_file(path: Path) -> list[str]: + """Return a list of violation strings for *path*, empty if clean.""" + if not path.suffix == ".py": + return [] + + return _get_warnings_issues(path) + + +def main(argv: list[str] | None = None) -> int: + if argv is None: + argv = sys.argv[1:] + + paths = [Path(p) for p in argv] + all_violations: list[str] = [] + + for path in paths: + all_violations.extend(check_file(path)) + + if all_violations: + print( + "Warning categorisation check failed.\n" + "All warnings raised by Iris must be categorised with a specific " + "IrisWarning subclass.\n" + ) + for v in all_violations: + print(v) + return 1 + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.hooks/check_license_headers.py b/.hooks/check_license_headers.py new file mode 100644 index 0000000000..0b47271dd3 --- /dev/null +++ b/.hooks/check_license_headers.py @@ -0,0 +1,184 @@ +# Copyright Iris contributors +# +# 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. + +"""Pre-commit hook: ensure Python files have proper license headers. + +All Python files (with some exceptions) must start with the Iris license +header. + +Usage (pre-commit passes staged file paths as arguments):: + + python .hooks/check_license_headers.py [file ...] + +Exit codes: + 0 – all clear + 1 – one or more files lack the required license header +""" + +from fnmatch import fnmatch +from pathlib import Path +import subprocess +import sys + +LICENSE_TEMPLATE = """# Copyright Iris contributors +# +# 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.""" + +# Patterns of files to exclude from license header check +_EXCLUDE_PATTERNS = ( + "setup.py", + "noxfile.py", + "build/*", + "dist/*", + "docs/gallery_code/*/*.py", + "docs/src/developers_guide/documenting/*.py", + "docs/src/user_manual/tutorial/plotting_examples/*.py", + "docs/src/user_manual/tutorial/regridding_plots/*.py", + "docs/src/_build/*", + "lib/iris/analysis/_scipy_interpolate.py", +) + + +def _find_repo_root(start_path: Path) -> Path | None: + """Find the repository root by looking for .git.""" + current = start_path.resolve() + while current != current.parent: + if (current / ".git").exists(): + return current + current = current.parent + return None + + +def _should_check(rel_path: str) -> bool: + """Return True if relative path should be checked for a license header.""" + if not rel_path.endswith(".py"): + return False + + return not any(fnmatch(rel_path, pat) for pat in _EXCLUDE_PATTERNS) + + +def _get_license_header_content(path: Path) -> str: + """Get the content to check for license header, accounting for shebang.""" + content = path.read_text() + if content.startswith("#!"): + # Strip shebang line + content = "\n".join(content.split("\n")[1:]) + return content + + +def check_file(path: Path, repo_root: Path | None = None) -> list[str]: + """Return a list of violation strings for *path*, empty if clean. + + Args: + path: Path to check (can be relative or absolute). + repo_root: Repository root. If None, will be inferred. + + Returns + ------- + List of violation strings. + """ + # Ensure path is absolute for consistent comparison + path = path.resolve() + + if repo_root is None: + repo_root = _find_repo_root(path) + if not repo_root: + repo_root = path.parents[2] if len(path.parents) > 2 else path.parent + + repo_root = repo_root.resolve() + + try: + rel_path = path.relative_to(repo_root).as_posix() + except ValueError: + # Path is not under repo root + return [] + + if not _should_check(rel_path): + return [] + + try: + content = _get_license_header_content(path) + except (OSError, UnicodeDecodeError): + # Let other tools handle file read errors + return [] + + if not content.startswith(LICENSE_TEMPLATE): + return [f"{path}: missing or incorrect license header"] + + return [] + + +def _get_all_tracked_files(repo_root: Path) -> list[Path]: + """Get all Python files tracked by git using git ls-files. + + This is much faster than recursive glob for checking entire repo. + """ + try: + output = subprocess.check_output( + ["git", "ls-files", "*.py"], + cwd=repo_root, + text=True, + ) + return [ + repo_root / line.strip() for line in output.splitlines() if line.strip() + ] + except (subprocess.CalledProcessError, FileNotFoundError): + # git not available or not a git repo; fall back to glob + return list(repo_root.rglob("*.py")) + + +def main(argv: list[str] | None = None) -> int: + if argv is None: + argv = sys.argv[1:] + + all_violations: list[str] = [] + + # If files provided, check only those files (pre-commit mode) + if argv: + paths = [Path(p) for p in argv] + repo_root = _find_repo_root(paths[0]) + if not repo_root: + repo_root = ( + paths[0].parents[2] if len(paths[0].parents) > 2 else paths[0].parent + ) + + for path in paths: + all_violations.extend(check_file(path, repo_root)) + else: + # No files provided; check entire repo (test mode) + # This requires finding the repo root first + cwd = Path.cwd() + repo_root = _find_repo_root(cwd) + if not repo_root: + # Try to find .git by going up from cwd + current = cwd.resolve() + while current != current.parent: + if (current / ".git").exists(): + repo_root = current + break + current = current.parent + + if not repo_root: + print("Error: Could not find git repository root") + return 1 + + for path in _get_all_tracked_files(repo_root): + all_violations.extend(check_file(path, repo_root)) + + if all_violations: + print( + "License header check failed.\n" + "All Python files must start with the Iris license header.\n" + ) + for v in all_violations: + print(v) + return 1 + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.hooks/check_netcdf4_imports.py b/.hooks/check_netcdf4_imports.py new file mode 100644 index 0000000000..00cb1ef7dd --- /dev/null +++ b/.hooks/check_netcdf4_imports.py @@ -0,0 +1,106 @@ +# Copyright Iris contributors +# +# 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. + +"""Pre-commit hook: ensure netCDF4 is only imported via _thread_safe_nc. + +Direct imports of the netCDF4 library are forbidden throughout Iris (except in +the permitted files listed below). All access must go via the thread-safe +wrapper at ``iris.fileformats.netcdf._thread_safe_nc``. + +Usage (pre-commit passes staged file paths as arguments):: + + python .hooks/check_netcdf4_imports.py [file ...] + +Exit codes: + 0 – all clear + 1 – one or more files contain a forbidden netCDF4 import +""" + +import ast +from pathlib import Path +import sys + +# Files that are allowed to import netCDF4 directly. +_PERMITTED_SUFFIXES = ( + # The thread-safe wrapper itself. + "iris/fileformats/netcdf/_thread_safe_nc.py", + # The test for the wrapper. + "iris/tests/unit/fileformats/netcdf/_thread_safe_nc/test_NetCDFWriteProxy.py", + # The tests for the bytecoding dataset wrapper. + "iris/tests/unit/fileformats/netcdf/test_bytecoding_datasets.py", + # The system test that checks netCDF4 is importable. + "iris/tests/system_test.py", +) + + +def _is_permitted(path: Path) -> bool: + """Return True if *path* is one of the permitted files.""" + as_posix = path.as_posix() + return any(as_posix.endswith(suffix) for suffix in _PERMITTED_SUFFIXES) + + +def _has_netcdf4_import(node: ast.AST) -> bool: + """Return True if *node* is a direct netCDF4 import statement.""" + if isinstance(node, ast.Import): + return any(alias.name == "netCDF4" for alias in node.names) + if isinstance(node, ast.ImportFrom): + return node.module is not None and node.module.startswith("netCDF4") + return False + + +def check_file(path: Path) -> list[str]: + """Return a list of violation strings for *path*, empty if clean.""" + if _is_permitted(path): + return [] + + file_text = path.read_text() + + # Fast pre-filter: skip parsing if "netCDF4" doesn't appear at all. + if "netCDF4" not in file_text: + return [] + + try: + tree = ast.parse(source=file_text, filename=str(path)) + except SyntaxError: + # Let other hooks (check-ast) handle syntax errors. + return [] + + violations = [] + for imp in ast.walk(tree): + if not isinstance(imp, (ast.Import, ast.ImportFrom)): + continue + if _has_netcdf4_import(imp): + violations.append( + f"{path}:{getattr(imp, 'lineno', 0)}: direct netCDF4 import — " + "use iris.fileformats.netcdf._thread_safe_nc instead." + ) + + return violations + + +def main(argv: list[str] | None = None) -> int: + if argv is None: + argv = sys.argv[1:] + + paths = [Path(p) for p in argv] + all_violations: list[str] = [] + + for path in paths: + all_violations.extend(check_file(path)) + + if all_violations: + print( + "netCDF4 import check failed.\n" + "All netCDF4 imports must be via iris.fileformats.netcdf._thread_safe_nc.\n" + ) + for v in all_violations: + print(v) + return 1 + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.hooks/check_python_versions.py b/.hooks/check_python_versions.py new file mode 100644 index 0000000000..bd138c0169 --- /dev/null +++ b/.hooks/check_python_versions.py @@ -0,0 +1,198 @@ +# Copyright Iris contributors +# +# 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. + +"""Pre-commit hook: ensure Python versions are consistent across config files. + +Python versions must be updated consistently everywhere they appear in the +repository (pyproject.toml, noxfile.py, CI workflows, requirements files, etc.). + +This hook runs when any version-related file is modified and validates that +all versions are in sync across the entire repository. + +Usage (pre-commit passes staged file paths as arguments):: + + python .hooks/check_python_versions.py [file ...] + +Exit codes: + 0 – all versions are consistent + 1 – version inconsistencies detected +""" + +from pathlib import Path +import sys + +from packaging.version import Version + +# Files that contain Python version information +_VERSION_CONFIG_FILES = { + "pyproject.toml", + "noxfile.py", + ".github/workflows/ci-wheels.yml", + ".github/workflows/ci-tests.yml", + "benchmarks/bm_runner.py", +} + + +def _find_repo_root(start_path: Path) -> Path: + """Find the repository root by looking for .git.""" + current = start_path.resolve() + while current != current.parent: + if (current / ".git").exists(): + return current + current = current.parent + return start_path + + +def _extract_supported_versions(root_dir: Path) -> list[str] | None: + """Extract supported Python versions from pyproject.toml. + + Returns None if versions cannot be determined. + """ + pyproject = root_dir / "pyproject.toml" + if not pyproject.exists(): + return None + + try: + content = pyproject.read_text() + # Look for "Programming Language :: Python :: 3.X" entries + import re + + matches = re.findall(r'"Programming Language :: Python :: (3\.\d+)"', content) + if matches: + return sorted(set(matches)) + except (OSError, UnicodeDecodeError): + pass + + return None + + +def check_consistency(root_dir: Path) -> list[str]: + """Check that Python versions are consistent across all config files. + + Returns a list of violation strings, empty if all consistent. + """ + # Get the authoritative version list from pyproject.toml + all_supported = _extract_supported_versions(root_dir) + if not all_supported: + # Can't validate without knowing what versions should be supported + return [] + + _parsed = [Version(v) for v in all_supported] + latest_supported = str(max(_parsed)) + + violations = [] + checks = [] + + # pyproject.toml - version classifiers + pyproject_toml_file = root_dir / "pyproject.toml" + if pyproject_toml_file.exists(): + expected = "\n ".join( + [f'"Programming Language :: Python :: {ver}",' for ver in all_supported] + ) + checks.append( + (pyproject_toml_file, expected, "pyproject.toml version classifiers") + ) + + # noxfile.py - _PY_VERSIONS_ALL + nox_file = root_dir / "noxfile.py" + if nox_file.exists(): + expected = "_PY_VERSIONS_ALL = [" + ", ".join( + [f'"{ver}"' for ver in all_supported] + ) + checks.append((nox_file, expected, "noxfile.py _PY_VERSIONS_ALL")) + + # CI workflows + ci_wheels_file = root_dir / ".github" / "workflows" / "ci-wheels.yml" + if ci_wheels_file.exists(): + expected = "python-version: [" + ", ".join( + [f'"{ver}"' for ver in all_supported] + ) + checks.append((ci_wheels_file, expected, "ci-wheels.yml python-version")) + + ci_tests_file = root_dir / ".github" / "workflows" / "ci-tests.yml" + if ci_tests_file.exists(): + expected = ( + f'python-version: ["{latest_supported}"]\n' + f'{" " * 8}session: ["doctest", "gallery"]' + ) + checks.append((ci_tests_file, expected, "ci-tests.yml doctest/gallery")) + + benchmarks_dir = root_dir / "benchmarks" + benchmark_runner_file = benchmarks_dir / "bm_runner.py" + if benchmark_runner_file.exists(): + expected = f'python_version = "{latest_supported}"' + checks.append((benchmark_runner_file, expected, "bm_runner.py python_version")) + + # Requirements files + requirements_dir = root_dir / "requirements" + if requirements_dir.exists(): + for ver in all_supported: + req_yaml = requirements_dir / f"py{ver.replace('.', '')}.yml" + if req_yaml.exists(): + expected = f"- python ={ver}" + checks.append( + (req_yaml, expected, f"requirements/py{ver.replace('.', '')}.yml") + ) + + # CI tests file version entries + if ci_tests_file.exists(): + for ver in all_supported: + expected = f'python-version: "{ver}"\n{" " * 12}session: "tests"' + checks.append((ci_tests_file, expected, f"ci-tests.yml py{ver} tests")) + + # Run all checks + for path, search, description in checks: + try: + content = path.read_text() + if search not in content: + violations.append( + f"{path}: missing expected version entry for {description}" + ) + except (OSError, UnicodeDecodeError): + pass + + return violations + + +def main(argv: list[str] | None = None) -> int: + if argv is None: + argv = sys.argv[1:] + + # Find repository root + if not argv: + # No files provided, can't determine repo root + # In this case, do nothing (let normal test handle it) + return 0 + + paths = [Path(p) for p in argv] + repo_root = _find_repo_root(paths[0]) + + # Check if any of the version-related files are in the staged files + is_version_file_staged = any( + str(p).endswith(tuple(f"/{name}" for name in _VERSION_CONFIG_FILES)) + or str(p).endswith(tuple(_VERSION_CONFIG_FILES)) + for p in paths + ) + + if not is_version_file_staged: + # Only run check if a version-related file is being modified + return 0 + + violations = check_consistency(repo_root) + + if violations: + print( + "Python version consistency check failed.\n" + "Python versions must be updated consistently across all config files.\n" + ) + for v in violations: + print(v) + return 1 + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index cb2c271459..c7c59bf582 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -14,6 +14,7 @@ ci: files: | (?x)( .github\/workflows\/.*| + .hooks\/.+\.py| benchmarks\/.+\.py| docs\/.+\.py| lib\/.+\.py| @@ -25,6 +26,37 @@ files: | minimum_pre_commit_version: 1.21.0 repos: +- repo: local + hooks: + - id: check-netcdf4-imports + name: Check netCDF4 imports are via _thread_safe_nc + language: python + entry: python .hooks/check_netcdf4_imports.py + types: [python] + - id: check-license-headers + name: Check license headers + language: python + entry: python .hooks/check_license_headers.py + types: [python] + - id: check-categorised-warnings + name: Check warnings are categorised + language: python + entry: python .hooks/check_categorised_warnings.py + types: [python] + - id: check-python-versions + name: Check Python version consistency + language: python + entry: python .hooks/check_python_versions.py + additional_dependencies: [packaging] + files: | + (?x)( + pyproject\.toml| + noxfile\.py| + \.github/workflows/(ci-tests|ci-wheels)\.yml| + benchmarks/bm_runner\.py| + requirements/py\d+\.yml + ) + # Hook for pre-commit's built-in checks - repo: https://github.com/pre-commit/pre-commit-hooks diff --git a/AGENTS.md b/AGENTS.md index 164eac11e7..ab5816a721 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -90,6 +90,7 @@ export CARTOPY_CACHE_DIR=~/.local/share/cartopy - [`lib/iris/tests/AGENTS.md`](lib/iris/tests/AGENTS.md) — test-specific +- Before finishing a change, run the repository's pre-commit hooks and fix any reported issues. This keeps deterministic checks in place for all agents and helps catch formatting, lint, and policy violations before review. ## Code Style diff --git a/MANIFEST.in b/MANIFEST.in index 7c3ddac806..c952daa2e0 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -73,6 +73,7 @@ exclude pixi.lock # SECTION: generic exclusions # (1) top-level directories to omit entirely prune .github +prune .hooks prune .nox prune .tox prune .coverage diff --git a/lib/iris/tests/test_coding_standards.py b/lib/iris/tests/test_coding_standards.py index dbfd726640..21b6527e4d 100644 --- a/lib/iris/tests/test_coding_standards.py +++ b/lib/iris/tests/test_coding_standards.py @@ -3,132 +3,86 @@ # 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. -import ast -from datetime import datetime -from fnmatch import fnmatch -from glob import glob +import importlib.util import os from pathlib import Path -import subprocess -from typing import Iterator, List, Tuple, cast -from packaging.version import Version import pytest import iris -from iris.tests import system_test -from iris.tests.unit.fileformats.netcdf import test_bytecoding_datasets - -LICENSE_TEMPLATE = """# Copyright Iris contributors -# -# 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.""" # Guess iris repo directory of Iris - realpath is used to mitigate against # Python finding the iris package via a symlink. -IRIS_DIR = Path(iris.__file__).parent.resolve() -IRIS_INSTALL_DIR = Path(IRIS_DIR).parent.parent -DOCS_DIR = Path(IRIS_INSTALL_DIR) / "docs" / "iris" -DOCS_DIR = iris.config.get_option("Resources", "doc_dir", default=str(DOCS_DIR)) -exclusion = ["Makefile", "build"] -DOCS_DIRS = glob(str(Path(DOCS_DIR) / "*")) -DOCS_DIRS = [DOC_DIR for DOC_DIR in DOCS_DIRS if Path(DOC_DIR).name not in exclusion] - +IRIS_DIR = os.path.realpath(os.path.dirname(iris.__file__)) +IRIS_INSTALL_DIR = os.path.dirname(os.path.dirname(IRIS_DIR)) # Get a dirpath to the git repository : allow setting with an environment # variable, so Travis can test for headers in the repo, not the installation. IRIS_REPO_DIRPATH = os.environ.get("IRIS_REPO_DIR", IRIS_INSTALL_DIR) +def _load_hook(hook_name: str): + """Load a hook module from .hooks/. + + Args: + hook_name: Name of the hook module (without .py extension). + + Returns + ------- + The loaded module. + + Raises + ------ + RuntimeError: If the hook module cannot be loaded. + """ + hook_path = Path(__file__).parents[3] / ".hooks" / f"{hook_name}.py" + spec = importlib.util.spec_from_file_location(hook_name, hook_path) + if spec is None or spec.loader is None: + raise RuntimeError(f"Failed to load hook: {hook_name}") + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + def test_netcdf4_import(): """Use of netCDF4 must be via iris.fileformats.netcdf._thread_safe_nc .""" - # Please avoid including these phrases in any comments/strings throughout - # Iris (e.g. use "from the netCDF4 library" instead) - this allows the - # below search to remain quick and simple. - from iris.fileformats.netcdf import _thread_safe_nc - from iris.tests.unit.fileformats.netcdf._thread_safe_nc import test_NetCDFWriteProxy + # Logic lives in .hooks/check_netcdf4_imports.py (also used as a pre-commit hook). + check_netcdf4_imports = _load_hook("check_netcdf4_imports") + check_file = check_netcdf4_imports.check_file - import_strings = ("import netCDF4", "from netCDF4") - - files_including_import = [] + all_violations = [] for file_path in Path(IRIS_DIR).rglob("*.py"): - file_text = file_path.read_text() - - if any([i in file_text for i in import_strings]): - files_including_import.append(file_path) + all_violations.extend(check_file(file_path)) - expected = [ - Path(_thread_safe_nc.__file__), - Path(test_NetCDFWriteProxy.__file__), - Path(system_test.__file__), - Path(__file__), - Path(test_bytecoding_datasets.__file__), - ] - assert set(files_including_import) == set(expected) + message = ( + "The following files import netCDF4 directly, which is not allowed:\n" + + "\n".join(all_violations) + + "\nAll netCDF4 imports must be via iris.fileformats.netcdf._thread_safe_nc." + ) + assert not all_violations, message -def test_python_versions() -> None: +def test_python_versions(): """Test Python Versions. Test is designed to fail whenever Iris' supported Python versions are updated, insisting that versions are updated EVERYWHERE in-sync. + + Logic lives in .hooks/check_python_versions.py (also used as a pre-commit hook). """ - all_supported = ["3.12", "3.13", "3.14"] - _parsed = [Version(v) for v in all_supported] - latest_supported = str(max(_parsed)) - - root_dir = Path(__file__).parents[3] - workflows_dir = root_dir / ".github" / "workflows" - benchmarks_dir = root_dir / "benchmarks" - - # Places that are checked: - pyproject_toml_file = root_dir / "pyproject.toml" - requirements_dir = root_dir / "requirements" - nox_file = root_dir / "noxfile.py" - ci_wheels_file = workflows_dir / "ci-wheels.yml" - ci_tests_file = workflows_dir / "ci-tests.yml" - benchmark_runner_file = benchmarks_dir / "bm_runner.py" - - text_searches: List[Tuple[Path, str]] = [ - ( - pyproject_toml_file, - "\n ".join( - [f'"Programming Language :: Python :: {ver}",' for ver in all_supported] - ), - ), - ( - nox_file, - "_PY_VERSIONS_ALL = [" + ", ".join([f'"{ver}"' for ver in all_supported]), - ), - ( - ci_wheels_file, - "python-version: [" + ", ".join([f'"{ver}"' for ver in all_supported]), - ), - ( - ci_tests_file, - ( - f'python-version: ["{latest_supported}"]\n' - f'{" " * 8}session: ["doctest", "gallery"]' - ), - ), - (benchmark_runner_file, f'python_version = "{latest_supported}"'), - ] - - for ver in all_supported: - req_yaml = requirements_dir / f"py{ver.replace('.', '')}.yml" - text_searches.append((req_yaml, f"- python ={ver}")) - - text_searches.append( - ( - ci_tests_file, - f'python-version: "{ver}"\n{" " * 12}session: "tests"', - ) - ) - - for path, search in text_searches: - assert search in path.read_text() - - -def test_categorised_warnings() -> None: + check_python_versions = _load_hook("check_python_versions") + repo_root = Path(IRIS_REPO_DIRPATH) + + violations = check_python_versions.check_consistency(repo_root) + + message = ( + "Python version consistency check failed.\n" + "Python versions must be updated consistently across all config files.\n" + + "\n".join(violations) + ) + assert not violations, message + + +def test_categorised_warnings(): r"""To ensure that all UserWarnings raised by Iris are categorised, for ease of use. No obvious category? Use the parent: @@ -145,151 +99,35 @@ class _WarnComboCfDefaulting(IrisCfWarning, IrisDefaultingWarning): \"\"\" pass + Logic lives in .hooks/check_categorised_warnings.py (also used as a pre-commit hook). """ - warns_without_category = [] - warns_with_user_warning = [] - tmp_list = [] + check_categorised_warnings = _load_hook("check_categorised_warnings") + check_file = check_categorised_warnings.check_file + all_violations = [] for file_path in Path(IRIS_DIR).rglob("*.py"): - file_text = file_path.read_text() - parsed = ast.parse(source=file_text) - calls: Iterator[ast.Call] = cast( - "Iterator[ast.Call]", - filter(lambda node: hasattr(node, "func"), ast.walk(parsed)), - ) - warn_calls: Iterator[ast.Call] = filter( - lambda c: getattr(c.func, "attr", None) == "warn", calls - ) - - warn_call: ast.Call - for warn_call in warn_calls: - warn_ref = f"{file_path}:{warn_call.lineno}" - tmp_list.append(warn_ref) - - category_kwargs = filter(lambda k: k.arg == "category", warn_call.keywords) - category_kwarg: ast.keyword | None = next(category_kwargs, None) - - if category_kwarg is None: - warns_without_category.append(warn_ref) - # Work with Attribute or Name instances. - elif ( - getattr(category_kwarg.value, "attr", None) - or getattr(category_kwarg.value, "id", None) - ) == "UserWarning": - warns_with_user_warning.append(warn_ref) - - # This avoids UserWarnings being raised by unwritten default behaviour. - assert warns_without_category == [], ( - "All warnings raised by Iris must be raised with the category kwarg." - ) + all_violations.extend(check_file(file_path)) - assert warns_with_user_warning == [], ( - "No warnings raised by Iris can be the base UserWarning class." + message = "The following files have warning categorisation issues:\n" + "\n".join( + all_violations ) + assert not all_violations, message + + +def test_license_headers(): + """Check that all Python files have the required license header. + Logic lives in .hooks/check_license_headers.py (also used as a pre-commit hook). + """ + check_license_headers = _load_hook("check_license_headers") + repo_root = Path(IRIS_REPO_DIRPATH) + + all_violations: list[str] = [] + for file_path in check_license_headers._get_all_tracked_files(repo_root): + all_violations.extend(check_license_headers.check_file(file_path, repo_root)) -class TestLicenseHeaders: - @staticmethod - def whatchanged_parse(whatchanged_output): - r"""Returns a generator of tuples of data parsed from - "git whatchanged --pretty='TIME:%at". The tuples are of the form - ``(filename, last_commit_datetime)``. - - Sample input:: - - ['TIME:1366884020', '', - ':000000 100644 0000000... 5862ced... A\tlib/iris/cube.py'] - - """ - dt = None - for line in whatchanged_output: - if not line.strip(): - continue - elif line.startswith("TIME:"): - dt = datetime.fromtimestamp(int(line[5:])) - else: - # Non blank, non date, line -> must be the lines - # containing the file info. - fname = " ".join(line.split("\t")[1:]) - yield fname, dt - - @staticmethod - def last_change_by_fname(): - """Return a dictionary of all the files under git which maps to - the datetime of their last modification in the git history. - - .. note:: - - This function raises a ValueError if the repo root does - not have a ".git" folder. If git is not installed on the system, - or cannot be found by subprocess, an IOError may also be raised. - - """ - # Check the ".git" folder exists at the repo dir. - if not (Path(IRIS_REPO_DIRPATH) / ".git").is_dir(): - msg = "{} is not a git repository." - raise ValueError(msg.format(IRIS_REPO_DIRPATH)) - - # Call "git log" to get the details of all the files and when - # they were last changed. - output = subprocess.check_output( - ["git", "log", "--name-status", "--pretty=TIME:%ct"], - cwd=IRIS_REPO_DIRPATH, - ) - - output = output.decode().split("\n") - res = {} - for fname, dt in TestLicenseHeaders.whatchanged_parse(output): - if fname not in res or dt > res[fname]: - res[fname] = dt - - return res - - def test_license_headers(self): - exclude_patterns = ( - "setup.py", - "noxfile.py", - "build/*", - "dist/*", - "docs/gallery_code/*/*.py", - "docs/src/developers_guide/documenting/*.py", - "docs/src/user_manual/tutorial/plotting_examples/*.py", - "docs/src/user_manual/tutorial/regridding_plots/*.py", - "docs/src/_build/*", - "lib/iris/analysis/_scipy_interpolate.py", - ) - - try: - last_change_by_fname = self.last_change_by_fname() - except ValueError as err: - # Caught the case where this is not a git repo. - msg = "Iris installation did not look like a git repo?\nERR = {}\n\n" - return pytest.skip(msg.format(str(err))) - - failed = False - for fname, last_change in sorted(last_change_by_fname.items()): - full_fname = Path(IRIS_REPO_DIRPATH) / fname - is_file = full_fname.is_file() - full_fname = str(full_fname) - - if ( - full_fname.endswith(".py") - and is_file - and not any(fnmatch(fname, pat) for pat in exclude_patterns) - ): - with open(full_fname) as fh: - content = fh.read() - if content.startswith("#!"): - # account for files with leading shebang directives - # i.e., first strip out the shebang line before - # then performing license header compliance checking - content = "\n".join(content.split("\n")[1:]) - if not content.startswith(LICENSE_TEMPLATE): - print( - "The file {} does not start with the required " - "license header.".format(fname) - ) - failed = True - - if failed: - raise ValueError("There were license header failures. See stdout.") + message = ( + "The following files are missing or have incorrect license headers:\n" + + "\n".join(all_violations) + ) + assert not all_violations, message From b0ab90b98ca91352566643d9434a59d25026a7d1 Mon Sep 17 00:00:00 2001 From: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> Date: Mon, 21 Sep 2026 11:53:08 +0100 Subject: [PATCH 02/19] More agent guidance. (#7287) --- AGENTS.md | 37 +++++--- lib/iris/AGENTS.md | 200 +++++++++++++++++++++++++++++++++++++++ lib/iris/ARCHITECTURE.md | 176 ++++++++++++++++++++++++++++++++++ 3 files changed, 401 insertions(+), 12 deletions(-) create mode 100644 lib/iris/AGENTS.md create mode 100644 lib/iris/ARCHITECTURE.md diff --git a/AGENTS.md b/AGENTS.md index ab5816a721..f41aa689f8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,8 +9,12 @@ built around CF-compliant multi-dimensional arrays ("Cubes"). Subdirectory AGENTS.md files take precedence for their subtrees: - [`changelog/AGENTS.md`](changelog/AGENTS.md) — documentation on changelog - [`docs/AGENTS.md`](docs/AGENTS.md) — documentation-specific rules +- [`lib/iris/AGENTS.md`](lib/iris/AGENTS.md) — how to write library source code - [`lib/iris/tests/AGENTS.md`](lib/iris/tests/AGENTS.md) — test-specific rules +For a map of where code lives and how data flows, see +[`lib/iris/ARCHITECTURE.md`](lib/iris/ARCHITECTURE.md). + ## Project Overview @@ -90,24 +94,29 @@ export CARTOPY_CACHE_DIR=~/.local/share/cartopy - [`lib/iris/tests/AGENTS.md`](lib/iris/tests/AGENTS.md) — test-specific -- Before finishing a change, run the repository's pre-commit hooks and fix any reported issues. This keeps deterministic checks in place for all agents and helps catch formatting, lint, and policy violations before review. +- Before finishing a change, run the repository's pre-commit hooks and fix any reported issues. (This is expected to encode a growing number of preferred coding practices, to enable productive collaboration with non-deterministic LLM agents). ## Code Style -```bash -# Lint -ruff check lib/iris +Linting and formatting are driven by pre-commit, not by invoking tools +directly. Run pre-commit from a conda environment named `pre-commit`. -# Auto-fix safe lint issues -ruff check --fix lib/iris +```bash +# Check and auto-fix the files you changed (preferred while iterating) +pre-commit run --files -# Format -ruff format lib/iris +# Check and auto-fix everything +pre-commit run --all-files -# Check formatting without writing -ruff format --check lib/iris +# Run a single hook, e.g. after a formatting-only change +pre-commit run ruff-format --all-files ``` +Pre-commit hooks rewrite files in place, so re-stage and re-run until it +passes cleanly. Only fall back to `ruff check` / `ruff format` directly when +pre-commit is unavailable — the hook config is the source of truth for +versions and arguments. + - **Line length**: 88 characters (Ruff default). - **Docstrings**: NumPy style; strictly validated. - **Copyright header**: Every new Python file must start with: @@ -119,8 +128,12 @@ ruff format --check lib/iris # See LICENSE in the root of the repository for full licensing details. ``` -- **Imports**: Ruff-managed ordering. No direct `import netCDF4` — always use - `iris.fileformats.netcdf._thread_safe_nc` for thread safety. +- **Imports**: Ruff-managed ordering, applied by pre-commit. No direct + `import netCDF4` — always use `iris.fileformats.netcdf._thread_safe_nc` for + thread safety. +- **Readability for humans and agents**: see + [`lib/iris/AGENTS.md`](lib/iris/AGENTS.md) for the house style on locality, + greppability, explicitness and comment hygiene. ## Development Conventions diff --git a/lib/iris/AGENTS.md b/lib/iris/AGENTS.md new file mode 100644 index 0000000000..ad1cd73e11 --- /dev/null +++ b/lib/iris/AGENTS.md @@ -0,0 +1,200 @@ +# AGENTS.md + +Agent instructions for `lib/iris/` — the Iris library source. + +These rules govern **how code is written** under this tree. Tests have their +own rules in [`tests/AGENTS.md`](tests/AGENTS.md). + +For **where code lives and how data flows**, read +[`ARCHITECTURE.md`](ARCHITECTURE.md) before exploring unfamiliar subsystems. +It is an orientation map only; algorithm detail belongs in module docstrings. + + +## Why This File Exists + +Iris will be maintained by humans and agents together for the foreseeable +future. Agents read code through a narrow context window using text search, +and cannot run the code to find out what it does. Humans read with intuition +and experience, but limited patience. + +These two readers want mostly the same things: explicit names, local +reasoning, small units. This file records that shared style, plus the few +places where the two genuinely diverge and which way to resolve them. + +Note that models are trained to imitate human code and to please human diff +reviewers — not to produce code that is cheap to navigate later. Default +output therefore drifts towards long functions, redundant comments and +defensive wrapping. Correct for that deliberately. + + +## Fast Rules + +1. Write code that can be understood from the file in front of you. +2. Name things so they can be found by exact text search. +3. Prefer explicit and slightly verbose over implicit and clever. +4. Prefer duplication over an abstraction you are not confident in. +5. Match surrounding Iris idioms even where you would choose differently. +6. Every comment and docstring must still be true after your edit. +7. Explain subsystems in module docstrings, not inline or in side files. + + +## Locality — Readable In One Place + +- Keep behaviour derivable from the function's own body, signature, type + hints and docstring. Avoid designs that require opening three other files. +- Prefer composition and explicit delegation over deep inheritance and mixin + stacks. Resolving an MRO across modules costs an agent many tool calls and + is opaque to a newcomer. Where Iris already uses mixins (e.g. + `iris.common.mixin`, metadata managers), follow the existing pattern rather + than inventing a parallel one. +- Do not add indirection (registry, dispatch dict, plugin hook, base class) + for a single caller. +- Pass dependencies in as arguments rather than reaching for module-level + mutable state. + + +## Greppability + +Exact text search is the primary retrieval tool. Code that cannot be found +does not exist. + +- Never construct identifiers at runtime: no `setattr(obj, f"{name}_bounds", + ...)`, no `getattr(module, method_name)` dispatch where a dict or `if/elif` + would do. A constructed name is unfindable for every reader. +- Give public names enough distinctiveness to search for. Local `data`, + `cube`, `result` are fine; a public helper called `_process` is not. +- Declare `__all__` in modules with a public surface. No `import *`. +- Make error and warning messages distinctive and mostly static, so a + traceback maps to exactly one line. Put interpolated values at the end: + `f"Cannot collapse a coordinate with bounds: {coord.name()}"`. +- Do not reuse one helper name across modules for differing behaviour. + + +## Explicitness + +- Add type hints to new or modified public functions. Agents cannot execute + code to discover types, so hints are the cheapest reliable signal. Do not + retrofit hints to code you are not otherwise changing. +- Avoid `**kwargs` passthrough on public API — spell out the parameters. If + passthrough is unavoidable, document the accepted keys. +- Use keyword arguments at call sites for anything not obviously positional. + Booleans are always keyword. +- Replace repeated magic strings with module-level constants. +- Return one documented type. Do not write functions whose return type + depends on an argument's value. Iris has some legacy examples; add no more. +- State laziness in the docstring: whether the result is lazy, and whether + the call realises data. + + +## Comments and Docstrings + +A stale comment is worse than no comment — an agent treats it as evidence and +a human treats it as documentation. + +- Comments explain **why**: the CF rule, the constraint, the bug worked + around. Never restate what the code plainly does. +- Fix or delete any comment your change invalidates, even if you did not + write it. +- Do not add docstrings or comments to code you did not otherwise change — + but see the module docstring exemption below. +- NumPy-style docstrings are mandatory and validated. State the contract: + units, shapes, laziness, mutation, exceptions raised. +- Link non-obvious workarounds to their source: + `# See https://github.com/SciTools/iris/issues/1234`. + + +## Module Docstrings Carry the Explanation + +Prose explaining a subsystem has to live somewhere, and the module docstring +is the best available home. Inline comments are the wrong place: they +interrupt the code for every reader and scatter a single argument across +dozens of sites. A companion Markdown file is also wrong: nothing validates +it, nothing forces anyone to open it, and it rots unseen. The module +docstring is the only location that is read automatically whenever the file +is opened, published by Sphinx, checked by numpydoc, and reviewed in the same +diff as the code it describes. + +Be generous with it. Sixty lines of orientation above a two-thousand-line +module is cheap, because it is read once per visit rather than once per call +site. Stop short of a tutorial. + +Cover whichever apply: + +- what the module is for, and what it deliberately does not do; +- the vocabulary — name the concepts the code assumes you already know; +- invariants the code depends on but cannot assert; +- why the design is as it is, including approaches tried and rejected; +- known traps, and the modules this one is coupled to. + +Leave out anything a reader or Sphinx can already derive: API listings, +signatures, call sequences, per-release history. Explanation scoped to a +single class belongs in that class's docstring instead. + +**Exemption to "do not document code you did not change":** if you had to +work out how a subsystem behaves in order to edit it, writing that +understanding into the module docstring is in scope. The comprehension was +expensive and is otherwise discarded when your context ends; the next reader, +human or agent, should not have to repeat it. Several of the most commonly +misread modules — `_concatenate.py`, `_merge.py`, `common/resolve.py` — +currently carry only a line or two of module docstring apiece. + + +## Size and Shape + +Agents pay for every line they read; humans lose the thread. Existing giants +such as `cube.py` (~5600 lines) are legacy — do not grow them without cause, +but do not opportunistically split them either. + +- New modules: aim under ~1000 lines, one coherent concept each. +- Functions: aim to fit one screen (~50 lines). +- Use guard clauses and early returns instead of nested conditionals. +- Nesting beyond three levels is a signal to extract a named helper. +- When adding genuinely separable code to an oversized module, put it in a + new private sibling module rather than growing the giant. + + +## Where Human and Agent Preferences Diverge + +| Tension | Resolution | +|---|---| +| Verbosity vs concision | Favour explicit. But never narrate line by line. | +| Abstraction vs duplication | Duplicate up to ~3 occurrences; abstract only once the shape is proven. | +| Clever idiom vs plain code | Plain. No nested comprehensions beyond two levels, no walrus inside complex expressions, no metaclass tricks. | +| Small files vs cohesion | Cohesion wins. Do not fragment into micro-modules purely to shrink files. | +| Type hints vs noise | Hint new and changed public code only. | +| Consistency vs local improvement | Consistency wins. A uniform mediocre idiom beats a mix of good ones. | + + +## Anti-Patterns — Do Not Introduce + +- Dynamically generated attributes, methods or module members. +- `eval`, `exec`, or `getattr` string dispatch in library code. +- Metaclasses or `__getattr__`, except in existing deprecation shims. +- Silent `except Exception: pass`. +- Boolean flags that switch a function between two unrelated behaviours — + write two functions. +- In-place mutation of input cubes or coordinates; return new objects. +- Refactors, renames or "improvements" outside the scope of the change. + + +## Pre-Finish Checklist + +- `pre-commit run --files ` passes, with any auto-fixes + re-staged and the run repeated until clean. +- New and changed public functions have type hints and NumPy docstrings. +- No comment or docstring was left stale by the change. +- Laziness behaviour is preserved and documented. +- Tests updated per [`tests/AGENTS.md`](tests/AGENTS.md); changelog fragment + added per [`../../changelog/AGENTS.md`](../../changelog/AGENTS.md). + + +## ⚠️ Meta-Instruction: Auto-Update Rule +- **Trigger**: You MUST automatically update this `AGENTS.md` file if a new + structural pattern, critical bug fix, or persistent repository rule is + established during this session. +- **Constraint 1**: Keep the file under 200 lines. Delete obsolete rules to + make room for new ones. +- **Constraint 2**: Do not ask for user permission before updating. Perform the + edit silently as your final step. +- **Constraint 3**: Only log global, reusable lessons. Do not log temporary or + component-specific fixes. diff --git a/lib/iris/ARCHITECTURE.md b/lib/iris/ARCHITECTURE.md new file mode 100644 index 0000000000..ef87c5529b --- /dev/null +++ b/lib/iris/ARCHITECTURE.md @@ -0,0 +1,176 @@ +# ARCHITECTURE.md + +An orientation map for `lib/iris`: **where things live and how data flows**. + +## Scope of This Document + +This file answers "where do I look for X?" and "what talks to what?". + +It deliberately does **not** explain how any algorithm works. Subsystem +detail belongs in the module docstring of the code it describes, where it +is validated by numpydoc, rendered by Sphinx, and travels in the same diff +as the code. + +The test for anything added here: **would it still be true after a competent +refactor?** Module locations and data flow survive refactors. Call sequences +and class member lists do not — do not record them here. + + +## The One-Paragraph Version + +Iris turns data files into `Cube` objects and back again. Loading detects a +file's format, hands it to a format-specific handler that yields many small +"raw" cubes, then combines those into fewer, larger cubes. Everything in +between — analysis, arithmetic, regridding, plotting — operates on `Cube`. +Data inside a cube is normally **lazy** (a Dask array) and is only realised +when something asks for it. + + +## Layer Map + +| Layer | Location | Responsibility | +|---|---|---| +| Public API | `__init__.py`, `loading.py` | `load*`, `save`, `Constraint`, policy objects | +| I/O dispatch | `io/` | URI decoding, format detection, handler dispatch | +| Format handlers | `fileformats/` | Per-format read/write; CF interpretation | +| Combination | `_merge.py`, `_concatenate.py`, `_combine.py` | Raw cubes into fewer, larger cubes | +| Core data model | `cube.py`, `coords.py`, `aux_factory.py`, `mesh/` | `Cube`, coordinates, derived and mesh coords | +| Metadata | `common/` | Metadata classes, CF property mixin, operand resolution | +| Laziness | `_lazy_data.py`, `_data_manager.py` | Dask boundary; real-vs-lazy storage | +| Analysis | `analysis/` | Aggregation, regridding, interpolation, maths | +| Visualisation | `plot.py`, `quickplot.py`, `palette.py`, `symbols.py` | Matplotlib and Cartopy integration | + + +## The Load Pipeline + +``` +iris.load / load_cube / load_cubes / load_raw loading.py + -> _load_collection -> _generate_cubes loading.py + -> iris.io.decode_uri, expand_filespecs io/__init__.py + -> FORMAT_AGENT.get_spec io/format_picker.py + matches on magic number, leading line, + file extension or URI protocol + -> format handler yields raw cubes fileformats/.py + -> constraint filtering (_CubeFilterCollection) loading.py + -> combination _combine.py + -> CubeList.merge -> ProtoCube _merge.py + -> CubeList.concatenate _concatenate.py +``` + +Key points that are stable over time: + +- **`load_raw` skips the combination step.** That is its entire purpose, and + it is the fastest way to see what a format handler actually produced. +- **Format handlers are registered, not hard-coded.** `FORMAT_AGENT` in + `fileformats/__init__.py` holds every `FormatSpecification`. To find out + which formats are supported, read that registry — do not rely on any list, + including this one, staying current. +- **Handlers are generators** yielding one `Cube` per field or variable. + A single PP file commonly yields thousands. +- **Combination behaviour is policy-driven**, not fixed: `iris.LOAD_POLICY` / + `iris.COMBINE_POLICY` (`CombineOptions` in `_combine.py`) decide whether + merge, concatenate, both, or neither is applied, and in which order. +- **Failures are captured, not discarded.** `LOAD_PROBLEMS` in `loading.py` + collects objects that could not be loaded, for inspection after the fact. + +### Merge versus concatenate + +Both combine a `CubeList` into fewer cubes, and they are not interchangeable: + +- **merge** (`_merge.py`) builds a *new* dimension from cubes that differ only + by scalar coordinate values. +- **concatenate** (`_concatenate.py`) extends an *existing* dimension by + joining cubes along it. + +### NetCDF specifics + +`fileformats/netcdf/loader.py` drives CF interpretation through a rules engine +in `fileformats/_nc_load_rules/`, which translates CF metadata into Iris +coordinates. `fileformats/cf.py` provides the CF-level view of a file. +Never `import netCDF4` directly — use +`fileformats/netcdf/_thread_safe_nc.py`. + + +## The Save Pipeline + +`iris.save` in `io/__init__.py` dispatches on file extension to a +format-specific saver (`fileformats/netcdf/saver.py`, `pp_save_rules.py`, +and so on). Saving is *not* a registry lookup — it is extension-based, which +is a genuine asymmetry with loading and a common source of confusion. + + +## Core Data Model + +- **`Cube`** (`cube.py`) — an n-dimensional data array plus CF metadata. +- **`DimCoord`** — describes exactly one dimension; points must be monotonic. +- **`AuxCoord`** — may span any number of dimensions, including none. +- **`CellMeasure`**, **`AncillaryVariable`** — further CF payloads on a cube. +- **`AuxCoordFactory`** (`aux_factory.py`) — manufactures a coordinate on + demand from a formula over other coordinates, e.g. hybrid height. This is + why a cube can report a coordinate that is not stored anywhere. +- **`MeshXY`**, **`MeshCoord`** (`mesh/`) — unstructured UGRID support. + +Invariants that hold across the codebase: + +- Operations return **new** cubes; they do not mutate their inputs. +- Metadata is carried by namedtuple-like classes in `common/metadata.py`; + `CFVariableMixin` (`common/mixin.py`) gives every object the same CF + property interface (`standard_name`, `units`, `attributes`, `rename()`). +- Comparison can be **lenient or strict** (`common/lenient.py`). Lenient is + the default for most user-facing operations, which is why two objects may + compare equal despite differing metadata. +- `Resolve` (`common/resolve.py`) decides the metadata and dimension mapping + of the result when two cubes are combined; cube arithmetic goes through it. + + +## Laziness + +Laziness is a cross-cutting concern, not a feature of one module. + +- `DataManager` (`_data_manager.py`) holds *either* a real array or a lazy + one, never both, for cubes and coordinates alike. +- `_lazy_data.py` is the Dask boundary: `as_lazy_data`, `as_concrete_data`, + `co_realise_cubes`, and chunking policy. +- `cube.data` **realises** data. `cube.core_data()` and `cube.lazy_data()` do + not. Library code should avoid `.data` unless realisation is intended. + + +## Structural Oddities Worth Knowing + +These are real and will otherwise cost you a confused half-hour: + +- **Several modules are very large** — `common/resolve.py` (~2600 lines), + `fileformats/pp.py` (~2400), `cube.py` (~5600). They are legacy. Do not + grow them unnecessarily, but do not opportunistically split them either. +- **GRIB support is an external package** (`iris_grib`), loaded through a + registered handler; it is not vendored here. +- **`iris/etc/`** holds colour palettes and a config template — not the CF + reference tables, which live in the repository-root `etc/`. CF standard + names reach the code as the generated module `std_names.py`. +- **`experimental/`** is genuinely unstable API and exempt from the usual + deprecation guarantees. + + +## Where Do I Look For...? + +| Question | Start here | +|---|---| +| Why did my file not load? | `LOAD_PROBLEMS` in `loading.py` | +| Why was my format not recognised? | `FORMAT_AGENT` in `fileformats/__init__.py` | +| Why did my cubes not merge? | `_merge.py`; compare against `load_raw` output | +| Why is this cube's metadata wrong after arithmetic? | `common/resolve.py` | +| Why do these two objects compare equal? | `common/lenient.py` | +| Why did my data realise unexpectedly? | `_data_manager.py`, `_lazy_data.py` | +| Where does this coordinate come from? | `aux_factory.py` | +| How is a CF attribute interpreted? | `fileformats/_nc_load_rules/` | + + +## Maintaining This File + +- Keep it under ~200 lines. It is an index, not a manual. +- Record structure and data flow only. Push explanation down into module + docstrings. +- Do not enumerate things the code already enumerates (format lists, class + members, function signatures). Point at the authoritative location instead. +- If a change here would also need a change to a module docstring, change the + docstring and leave this file alone. From 40a988e0e1c7bb40863f671896de5da4ba4056c8 Mon Sep 17 00:00:00 2001 From: Bill Little Date: Wed, 23 Sep 2026 14:30:11 +0100 Subject: [PATCH 03/19] Design specification for native Zarr I/O (#7292) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 SciTools/iris#6977, #6979, #6980 when the programme completes. Co-Authored-By: Claude Opus 5 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 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 Session-Id: 82e42faf-fdb4-4d78-b163-d85ad5cf32c8 * Link the spec to the parked attribute-model issue SciTools/iris#7288 now holds the CF-attribute-model discussion that section 10 parks. Co-Authored-By: Claude Opus 5 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 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 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 * 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 * 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 * 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 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 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 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 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 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 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 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 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 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 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(" 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 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 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 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 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 Session-Id: 45914679-20ef-4bcf-b7fe-c48ab76a3cef --------- Co-authored-by: Claude Opus 5 Co-authored-by: Martin Yeo --- .../specs/2026-09-21-zarr-io-design.md | 1845 +++++++++++++++++ lib/iris/AGENTS.md | 93 +- 2 files changed, 1899 insertions(+), 39 deletions(-) create mode 100644 docs/superpowers/specs/2026-09-21-zarr-io-design.md diff --git a/docs/superpowers/specs/2026-09-21-zarr-io-design.md b/docs/superpowers/specs/2026-09-21-zarr-io-design.md new file mode 100644 index 0000000000..0470ff8fb5 --- /dev/null +++ b/docs/superpowers/specs/2026-09-21-zarr-io-design.md @@ -0,0 +1,1845 @@ +# Native Zarr I/O for Iris + +> **This is a living document.** §12 is the single progress record for the +> programme — pull request states, open questions and the decision log. Update +> §12 as work lands; do not track status anywhere else. + +| | | +|---|---| +| **Phase** | Design, awaiting approval | +| **Progress** | 0 of 7 pull requests raised; merge-back not started — see §12.1 | +| **Next action** | Spec approval, then the implementation plan | +| **Blocked on** | Nothing | +| **Branch** | `zarr-io-design` on `bjlittle/iris`, targeting `SciTools/iris:brownfield` | +| **Delivery** | Seven pull requests into `brownfield`, then one merge-back into `main` — §5 | +| **Resolves** | SciTools/iris#6977, #6979, #6980 — at merge-back, not before | + +**Created:** 2026-09-21 · **Last updated:** 2026-09-23 (see §12.6) +**Baseline:** `brownfield` at `b0ab90b98`, Iris `3.17.0.dev4` +**Verified against:** zarr-python 3.4.0, numcodecs 0.16.5, Python 3.13, +dask 2026.7.1, xarray 2026.7.0 + +Line references in this document are accurate as of the baseline commit. +Claims marked **[verified]** were checked by running code against the versions +above; everything else is read from source or specification. + +--- + +## 1. Summary + +Iris can already read and write Zarr, but only through the netCDF-c NCZarr +driver, which implements the **Zarr version 2** specification and writes scalar +variables as length-one arrays. This document specifies **native Zarr support** +built on zarr-python: reading Zarr version 2 and version 3 stores, and writing +version 3 stores. + +The work is structured as seven pull requests against the `brownfield` branch. +Four of them are behaviour-preserving refactors that promote +`iris.fileformats.cf` to a package and extract the format-agnostic CF machinery +out of `iris.fileformats.netcdf`; three add user-visible Zarr capability on top. +No pull request both moves code and changes behaviour. + +The central design move is a `CFDataset` abstraction: a narrow, explicit +interface describing what a CF-conforming array store must provide, with one +implementation per backend. The CF variable classes, the loader and the saver +are rewritten against that interface instead of against the netCDF4 Python API. + +Every rule in §4 is derived from the Zarr and CF specifications. Real published +stores are cited as evidence that a code path will be exercised, and where such +a store is non-conforming it is named as a malformation and handled with a +warning — never by bending the reader to fit one publisher's files. + +Where the two specifications are silent and an established *de-facto* +convention has filled the gap, that is a third case, distinct from both a +specification rule and a publisher's mistake — and misreading it as the latter +is its own failure mode (§4.4, `_FillValue`). Such a convention is adopted only +when it is named, its source cited, and the cost of ignoring it stated. The +test of one is whether an independent reader depends on it, not whether a file +happens to contain it. + +--- + +## 2. What exists today + +### 2.1 The CF layer reaches straight into netCDF4 + +`lib/iris/fileformats/cf.py` (1721 lines) classifies the variables in a file +into `CFDataVariable`, `CFCoordinateVariable`, `CFGridMappingVariable` and so +on. It is nominally format-agnostic, but in practice it calls netCDF4 directly: + +- `CFVariable.__init__` (cf.py:92-118) calls `data.ncattrs()` and + `data.group().filepath()`. +- `CFVariable.__getattr__` (cf.py:198-206) forwards every unknown attribute to + the underlying netCDF4 variable, conflating **properties** (`shape`, `dtype`, + `dimensions`) with **CF attributes** (`standard_name`, `units`). It caches the + result onto the instance, so the touch-tracking used by `cf_attrs_used()` + registers only the first access. +- `CFReader.__init__` (cf.py:1336) branches on `self._dataset.file_format`, and + (cf.py:1356) calls `set_auto_chartostring(False)`. + +Across `cf.py`, `_nc_load_rules/` and `netcdf/`, there are 86 `getattr(cf_*` +or `hasattr(cf_*` call sites and 57 direct uses of netCDF-only APIs +(`ncattrs`, `getncattr`, `setncattr`, `chunking()`, `file_format`). + +### 2.2 The loader and saver are generic code in a netCDF-shaped box + +`netcdf/loader.py` (979 lines) and `netcdf/saver.py` (3190 lines) are mostly CF +logic. The genuinely netCDF-specific parts are small and identifiable: + +| Module | netCDF-only content | +|---|---| +| `loader.py` | `_get_cf_var_data` chunking and proxy selection; VLEN handling; `_bytecoding_datasets` string machinery | +| `saver.py` | dataset open/close; `_setncattr`; `_create_cf_dimensions`; `_create_generic_cf_array_var`; write proxies; `_dask_locks` | + +Everything else — `CFNameCoordMap`, `_get_dim_names`, `_add_aux_coords`, +`_add_cell_measures`, `_add_aux_factories`, `_create_cf_grid_mapping`, the CF +constant tables, `ChunkControl` — is format-agnostic and currently +unreachable from any other format. + +### 2.3 The load and save chain assumes a single seekable file + +A Zarr store is a **directory**, or a URL. Both break the existing chain: + +- `iris.io.load_files` (io/__init__.py:241) does `open(fn, "rb")` on every path, + which raises `IsADirectoryError` for a store. +- `iris.io.expand_filespecs` (io/__init__.py:196-199) calls + `Path(fn.removeprefix("//")).expanduser().absolute()` on every spec, which + mangles `s3://bucket/data.zarr` into `//s3:/bucket/data.zarr`. +- `iris.loading._generate_cubes` (loading.py:110) raises + `ValueError("Iris cannot handle the URI scheme: %s")` for any scheme that is + not `file`, `http`, `https`, `nczarr` or `data`. +- `iris.io.save` (io/__init__.py:530) sends a `CubeList` to a per-cube append + loop unless `"iris.fileformats.netcdf" in saver.__module__`. +- `iris.mesh.load_meshes` carries a second copy of the same scheme dispatch in + `netcdf/ugrid_load.py:156-183`. + +### 2.4 Zarr facts that shape the design + +All **[verified]** against zarr-python 3.4.0: + +- Version 3 arrays carry `metadata.dimension_names` as a tuple. Version 2 + arrays have no such field; zarr-python raises `ValueError("Zarr format 2 + arrays do not support dimension names.")` if you try. Version 2 stores use + the xarray convention of an `_ARRAY_DIMENSIONS` attribute holding a list. +- Scalar arrays have `shape == ()` and `dimension_names is None`. +- Attributes are JSON. NumPy scalars, NumPy arrays, `bytes` and `numpy.int64` + all raise `TypeError: Object of type ... is not JSON serializable`. Python + floats, ints, strings, lists and `numpy.str_` are accepted. A failed + assignment leaves the in-memory attributes mapping dirty, so every later + write to that mapping fails with the **first** error, not its own. +- `float("nan")` is accepted and round-trips, written as the bare token `NaN`, + which is not valid strict JSON. +- Reads return a plain `numpy.ndarray`. Zarr applies **no** masking and **no** + `scale_factor`/`add_offset` handling; netCDF4 does both automatically. +- `zarr.Array` is picklable, and `dask.array.from_zarr` produces an unmasked + `numpy.ndarray` dask meta. +- `zarr.consolidate_metadata` warns that consolidated metadata is not part of + the version 3 specification. + +Two further facts come from the Zarr *specifications* rather than from +zarr-python, and they set the boundary between what Iris must support and what +Iris must merely defend against: + +- An attribute value is **an arbitrary JSON literal**. Nested objects and + arrays are legal Zarr. CF's attribute model is flat scalars and strings, so + any conforming Zarr store may carry attributes that CF cannot express. +- An array's `fill_value` is a **required** version 3 metadata field. For a + float the permitted encodings are a JSON number, `"NaN"`, `"Infinity"`, + `"-Infinity"`, or the hex string form `"0xYYYYYYYY"`; base64 is not among + them. CF's `_FillValue` is an *optional attribute*. These are two different + things, and the design reconciles them rather than conflating them. +- **`fill_value` says what unwritten storage reads as. It does not say the + value is missing.** The two are easy to conflate and the conflation is + destructive: zarr-python defaults `fill_value` to **zero** — `np.int16(0)` + for `int16`, `np.float32(0.0)` for `float32`, on both version 2 and version + 3 **[verified]**. Treating it as a CF missing-value sentinel masks every + valid zero in an array that simply never specified one. And because the + field is *required* on version 3, there is no "array without a usable + `fill_value`" case for a CF attribute to fall back into. §4.4 sets the + precedence accordingly. + +--- + +## 3. Constraints and decisions taken + +These were agreed before design and are not revisited here. + +| Decision | Choice | +|---|---| +| Dependency | `zarr` is **optional** and lazily imported. Absent, Zarr tests skip and Zarr use raises a clear `ImportError`. | +| Module layout | Extract the format-agnostic CF machinery out of `fileformats/netcdf/` into generic siblings. | +| Write support | **Version 3 only**, but the saver must be shaped so version 2 write is a later, small addition. | +| Read support | **Version 2 and version 3.** | +| Deprecation | Relocated public names keep working and warn on use. | +| Remote stores | URLs are passed through to zarr-python. Iris adds no dependency and handles no credentials. | +| Groups | Root group by default; a `group=` keyword selects another, on both load and save. | +| Real-world data | NOAA GFS from dynamical.org, and ESA EOPF Sentinel Zarr samples. | + +Two further constraints come from the repository's own agent rules +(`lib/iris/AGENTS.md`): no dynamically generated attributes, no `getattr` +string dispatch, and `__getattr__` only in existing deprecation shims. The +design honours these; where it cannot, it says so. + +--- + +## 4. Design + +### 4.1 Module layout + +`iris.fileformats.cf` becomes a **package**. It is where all format-agnostic +CF machinery ends up, and a flat module cannot hold a dataset abstraction, a +loader and a saver alongside 1721 lines of variable classification. + +``` +lib/iris/fileformats/ + cf/ + __init__.py # public surface: re-exports + __all__, nothing else + dataset.py # NEW CFDataset / CFDatasetVariable + loader.py # NEW generic CF -> Cube (from netcdf/loader.py) + saver.py # NEW generic Cube -> CF (from netcdf/saver.py) + _variables.py # every CFVariable subclass, classic and UGRID (was cf.py) + _group.py # CFGroup (was cf.py) + _reader.py # CFReader (was cf.py) + netcdf/ + _dataset.py # NEW NetCDFDataset, NetCDFDatasetVariable + loader.py # netCDF load_cubes + deprecating aliases + saver.py # netCDF Saver, save, save_mesh + ... # _thread_safe_nc, _bytecoding_datasets, _dask_locks unchanged + zarr/ + __init__.py # NEW public: load_cubes, save, Saver + _dataset.py # NEW ZarrDataset, ZarrDatasetVariable + _decode.py # NEW CF masking and unpacking that netCDF4 does for free + loader.py # NEW + saver.py # NEW +``` + +Every public name that `iris.fileformats.cf` exports today is re-exported from +`__init__.py`, so `from iris.fileformats.cf import CFReader` is unaffected and +no deprecation is needed for the move itself. This is the shape `iris.mesh` +already has: a 36-line `__init__.py` over `components.py` and `utils.py`. + +The private split cuts at the three seams already in `cf.py`, each of which +is a different kind of thing: the variable classifiers (cf.py:78-1116), the +`CFGroup` mapping that collects them (cf.py:1117-1280) and the `CFReader` +that drives the whole thing (cf.py:1281-1721). That gives `_variables.py` at +roughly 1120 lines, `_group.py` at 180 and `_reader.py` at 460. + +`_variables.py` sits just over the ~1000-line aim in `lib/iris/AGENTS.md`, +and stays there deliberately. Do not split the three `CFUGrid*` classes into a +fourth module to get under the number; that is a line count in search of a +rationale. The UGRID classes are not a separate +concern from the classic ones — they are `CFVariable` subclasses with the +same shape (`cf_identity`, `cf_identities`, a classmethod `identify`), doing +the same job of classifying a CF-netCDF variable by its attributes. +`CFReader._variable_types` lists all ten in one tuple (cf.py:1291-1302); +`CFGroup.non_data_variable_names` enumerates `connectivities`, `ugrid_coords` +and `meshes` alongside `bounds`, `labels` and `cell_measures`; and the +module-private `_is_str_dtype` (cf.py:78) is called from both sets, so the +split would export a private helper across a module boundary to buy nothing. +The `iris.mesh` analogy does not hold either: that package holds cube-level +data-model objects, whereas these are variable classifiers doing the work of +`CFAuxiliaryCoordinateVariable`. `lib/iris/AGENTS.md` settles it — *"Cohesion +wins. Do not fragment into micro-modules purely to shrink files."* + +The re-export layer in `__init__.py` is the ordinary Iris pattern, not a +concession. `iris.mesh` re-exports from four modules, two of them in a +different subtree entirely (`iris.fileformats.netcdf.saver`, +`iris.fileformats.netcdf.ugrid_load`), so that users import `MeshXY` and +`save_mesh` from one place. Surfacing objects where users expect them is a +genuine convenience, and it is what leaves the private file layout free to +change. `lib/iris/AGENTS.md` says so explicitly; its "do not add indirection" +rule governs implementation layers, not API surface. + +The re-exports are verbatim and static — `from ._reader import CFReader`, name +for name, gathered into `__all__` — so `class CFReader` still finds the +definition in one grep. No renaming, no conditional imports, no logic in +`__init__.py`. + +`_variables.py`, `_group.py` and `_reader.py` are private because +they are a file layout, not an API. Third-party code that reaches past +`iris.fileformats.cf` into a submodule is reaching into the split itself, which +is exactly the thing that should stay free to move. + +`iris.fileformats.zarr` shadows the third-party `zarr` distribution by name. +Absolute imports mean `import zarr` inside that package still resolves to the +third-party one; xarray has the same arrangement. The package docstring says so. + +### 4.2 The `CFDataset` interface + +`cf/dataset.py` defines two abstract classes. They are deliberately small: +only what the CF variable classes, `cf/loader.py` and `cf/saver.py` actually +need. + +```python +class CFDatasetVariable(ABC): + """One named array in a CF-conforming dataset.""" + name: str + dimensions: tuple[str, ...] + shape: tuple[int, ...] + dtype: np.dtype + size: int + fill_value: Any | None + chunking: tuple[int, ...] | None # None when the store is unchunked + attributes: MutableMapping[str, Any] # materialised once; tracks reads + + def __getitem__(self, keys) -> np.ndarray: ... + def __setitem__(self, keys, values) -> None: ... + + def write_handle(self) -> Any: ... # picklable __setitem__ target; see 4.5 + + +class CFDataset(ABC): + """A CF-conforming array store, open for reading or writing.""" + location: str # path or URL, for messages and proxies + mode: str # "r" | "r+" | "a" | "w" | "w-"; 4.5 + variables: Mapping[str, CFDatasetVariable] + dimensions: Mapping[str, int] + attributes: MutableMapping[str, Any] + + def create_dimension(self, name: str, size: int) -> None: ... + def create_variable(self, name, dtype, dimensions, *, + fill_value=None, **encoding) -> CFDatasetVariable: ... + def sync(self) -> None: ... + def finalise(self) -> None: ... # one-shot; NOT part of close() + def close(self) -> None: ... +``` + +Three members exist only to keep multi-process writing reachable later, and +are explained in §4.5: `write_handle`, `mode` and `finalise`. They cost +almost nothing now — `mode` is a string the implementations already track +internally, `write_handle` returns `self` for Zarr, and `finalise` is a no-op +for netCDF — and their absence is what would force a breaking interface +change later. + +The split that matters is `attributes` versus everything else. Today +`cf_var.units` might be a CF attribute or a netCDF property and the caller +cannot tell. After this change, CF attributes are only ever reached through +`.attributes`, and properties are named fields. + +`create_variable` takes `**encoding` against the general rule about keyword +passthrough, because the accepted keys are backend-specific by nature +(`zlib`/`complevel`/`chunksizes` for netCDF, `compressors`/`chunks`/`shards` +for Zarr). Each implementation documents its own accepted keys, and +`cf/saver.py` never constructs them. + +### 4.3 `CFVariable.__getattr__` and backwards compatibility + +`CFVariable.__getattr__` (cf.py:198) does two unrelated jobs today, and the +refactor separates them. Counting what reaches it from inside Iris: + +| Reached via `__getattr__` | Count | Job | +|---|---|---| +| `.dimensions` | 28 | proxy the backing netCDF4 variable | +| `.shape` | 11 | proxy | +| `.dtype` | 11 | proxy | +| `.getncattr` | 6 | proxy | +| `.size`, `.group`, `.chunking` | 7 | proxy | +| CF attribute names (`units`, `bounds`, `coordinates`, ...) | the rest | open-world data | + +`self.getncattr(attr)` at cf.py:223 is the clearest case: `CFVariable` does not +define `getncattr`, so the call resolves through `__getattr__` onto the netCDF4 +object. Everything in the "proxy" rows is exactly what `CFDatasetVariable` +(§4.2) declares explicitly and typed, so the refactor does not have to defend +`__getattr__` - it shrinks it to the one job that justifies it. + +**The surviving job is legitimate.** CF attributes are an open-ended set of +data keys read from a file; there is no schema to enumerate, so this is the +narrow exception `lib/iris/AGENTS.md` now carves out. The condition attached to +that exception is that `.attributes` is the path library code takes, and +`__getattr__` is only a convenience skin over it. + +So, on `CFVariable`: + +- `__getattr__` resolves against `self.attributes` - one implementation on the + base class, **identical for netCDF and Zarr**. It must not raise `TypeError` + for Zarr-backed variables: that would put a hole in the most-used public + accessor of the very abstraction §4.2 exists to provide. A missing key + raises plain `AttributeError`, which is correct: + `getattr(nc_var, "bounds", None)` is a deliberate idiom throughout the loader + and depends on `hasattr` semantics working. +- **No `setattr(self, name, value)` caching.** Today's version caches by + mutating `self.__dict__`, so an instance's shape depends on its access + history - that genuinely is the "dynamically generated attributes" + anti-pattern. A read-through to a mapping materialised once at construction + is faster than today's *first* access and stays inspectable. +- On a miss, a netCDF-backed variable falls back to the backing netCDF4 object + and issues a deprecation warning naming the `CFDatasetVariable` member that + replaces it, so `cf_var.getncattr("units")` and `cf_var.ndim` keep working + for one cycle. A Zarr-backed variable has no such object, so the fallback + simply does not apply - consistent, not a special case. + +**Known limitation, documented rather than fixed.** `__getattr__` fires only +when normal lookup fails, so a file carrying an attribute named `filename`, +`cf_name` or `spans` is silently shadowed by the class member of that name. CF +reserves no namespace, so this cannot be ruled out. `.attributes` has no such +ambiguity, which is the practical argument for making the mapping the library +path. + +**Consequence for attribute tracking, the sharpest edge in PR 2.** The +`_cf_attrs` "which attributes were read" set is currently a side effect of +`__getattr__`, and `cf_attrs_unused()` has exactly one consumer: +`netcdf/loader.py:190`, which decides which attributes survive onto the cube. +Move the reads to `.attributes` and the tracking must move with them, or every +CF-reserved attribute silently leaks onto loaded cubes. `.attributes` is +therefore a tracking mapping, not a plain `dict`, and PR 1's tests must pin +`cf_attrs_unused()` before PR 2 touches it. + +### 4.4 Reading + +#### Dimension names + +`ZarrDatasetVariable.dimensions` reads `metadata.dimension_names` on version 3 +and the `_ARRAY_DIMENSIONS` attribute on version 2. A scalar array yields `()`. +An array with neither, and rank greater than zero, raises `ValueError` naming +the array: synthesising names would silently produce wrong cubes. +`ZarrDataset.dimensions` is then the union of the per-array names with their +sizes, and a name used at two different sizes is an error. + +NCZarr-written stores use a `_scalar_` pseudo-dimension for scalar variables. +`cf.py` already handles this in `CFVariable.spans` (cf.py:179-184) via +`_NCZARR_SCALAR_DIMENSION`. The three subclasses that override `spans` +(`CFBoundaryVariable` cf.py:415, `CFClimatologyVariable` cf.py:491, +`CFLabelVariable` cf.py:814) do **not**, which is a latent bug on the existing +NCZarr path. It is fixed in PR 2, and is the only `bugfix` fragment the +merge-back owes: everything else in the programme is new capability, a +relocation or a deprecation. + +#### Fill values: storage fact versus masking instruction + +Zarr's array-level `fill_value` and CF's `_FillValue` attribute are different +things — see §2.4 — and the difference runs the opposite way to the intuitive +one. `fill_value` is a *storage* fact: it is what a chunk that was never +written reads back as. `_FillValue` is a *semantic* one: it declares a value to +mean "missing". Only the second is a masking instruction, so **the CF attribute +governs masking and the storage field does not.** + +The precedence is therefore version-aware, matching what released xarray does +(`use_zarr_fill_value_as_mask`, `xarray/backends/zarr.py:1989-1995`): + +| | masking source | +|---|---| +| **Version 3** | the CF `_FillValue`/`missing_value` attributes only. `Array.fill_value` is *not* a mask source | +| **Version 2** | `Array.fill_value` is used as the mask source when no CF attribute is present, because version 2 stores predate the convention and this is what their writers meant | + +Reading `Array.fill_value` as a sentinel on version 3 would mask valid data in +the ordinary case, not the exotic one: zarr-python defaults the field to zero, +so an `int16` array holding `[0, 1, 2]` with no CF attributes at all has its +valid zero masked **[verified]**. And since the field is *required* on version +3, an "only when the array carries no usable one" fallback would never fire, so +a genuine CF sentinel that differs from the storage fill — `_FillValue = -999` +over a default `fill_value = 0` — could never win. Both cases were reproduced +against a store written by xarray **[verified]**. + +`Array.fill_value` is still read, and still used: it is what an unwritten chunk +yields, it round-trips through `create_variable` on save (§4.5), and on version +2 it is the mask source above. It is simply not a CF missing-value declaration. + +#### A floating `_FillValue` is base64 on version 3 + +That is a convention, not a malformation. Released xarray encodes a +floating-point `_FillValue` attribute as base64 of a little-endian `float64` — +`struct.pack("