Serialization improvements - #1409
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds provenance capture and drift-detection tooling around serialized ICON test data, and updates the serialization driver + documentation so regenerations produce attributable metadata and human-reviewable diffs instead of “silent” data changes.
Changes:
- Add
scripts/python/serdata.pyCLI/library to parse ICON version banners, fingerprint Serialbox digests, diff archives/namelists, and backfill metadata for old archives. - Extend
scripts/python/run_serialization.pywith experiment/comm-size selectors, per-task result reporting (run_summary.json), archive metadata writing, and per-archive comparison report generation. - Make experiment archive versions explicit per experiment (no shared default) and document the end-to-end regeneration/publishing workflow.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/python/serdata.py | New provenance + fingerprint/diff/backfill utilities and serdata CLI commands. |
| scripts/python/run_serialization.py | Adds selectors, task result collection, archive metadata writing, and diff report generation. |
| model/testing/src/icon4py/model/testing/definitions.py | Removes shared default for ExperimentDescription.version; pins explicit versions per experiment. |
| scripts/tests/python/test_serdata.py | New tests for banner parsing, fingerprinting, diffing, report rendering, and backfill. |
| scripts/tests/python/test_run_serialization.py | New tests for selector behavior, explicit versioning, JSON serialization, and report naming/printing helpers. |
| docs/testdata_generation.md | New runbook describing generation, review, publishing, and archive metadata format. |
| scripts/tests/python/data/** | Adds/updates fixture archives/logs/namelist dumps used by new tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jcanton
left a comment
jcanton
left a comment
There was a problem hiding this comment.
is the tests/data necessary or once we backfill we can run tests against data on the server and pick one experiment and test current version against previous version (the server always has the two most recent or more)
A regeneration silently absorbed an unrelated upstream ICON commit and surfaced days later as three failing datatests, with nothing recording which Fortran build the reference values came from. ICON already prints its git describe at startup and the generation script already copies that log into every archive, so the fact was on disk all along and nobody read it. Datatest sessions now print it for every archive they use. A describe string is a valid git rev, so explaining a changed reference value is a git log between the two. Also: per-experiment versions, so bumping one no longer invalidates the other five; a preflight that shows what a campaign will be built from before it costs hours; --dry-run; per-task failure isolation; and the datatests run against the new data before it is published. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
66f64d2 to
5d72daf
Compare
jcanton
changed the title
Auto-discovers the savepoints of a serialized archive and summarizes the fields they hold, so reference data can be checked for carrying usable values before a datatest relies on it: an all-zero tracer validates nothing, and neither does one that is bit-identical across the process that is supposed to change it. Four sub-commands: 'savepoints' lists what an archive contains, 'stats' reports min/max/mean/nonzero-fraction per field, 'diff' does the same for the difference between two timestamps or between two savepoints at the same date, and 'shell' opens an interpreter on the archive. Fields whose trailing axis indexes the tracer list are split per species and labelled from the icon4py tracer indices. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- pytest_testnodedown needs optionalhook: the plugin is loaded for every session in the repo, and without xdist installed pytest aborted before running anything - the unit tests wrote into the module-level record, so a plain run printed provenance for archives it had never read - test_run_serialization.py imported icon4py, which the scripts-only test environment does not have, so ./scripts/test python collected nothing - --allow-dirty skipped the whole preflight, including the revision print it exists for Drops the datatest wrapper (the runbook prints the command), TaskResult, the seen()/merge() indirection, and most of the tests; halves the runbook. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- take the tracer order from the model instead of recomputing it - SavepointRef kept a whole metainfo dict to read one key from it - drop the tests that guarded one-line properties, and the ones that never ran in the scripts-only test environment format_table stays: rich.table truncates to the terminal width, and savepoint and field names are the point of that output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_summarize_components branched twice over the same call; the module docstring restated --help. The diff command had no tests, which is how the SavepointRef change broke it: the new ones fail on that exact bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
run_command captured stdout and stderr and then let CalledProcessError report only the exit status, so an sbatch rejection arrived as "returned non-zero exit status 1" with the reason discarded. Also makes the icon4py import in copy_ser_data lazy: it was the only one at module scope, and scripts/run imports every module to assemble the CLI, so it was paid by every command. That also lets the driver's tests run in the scripts-only test environment again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jcanton
changed the title
…ments * origin/main: Bump ghex to 0.9.0 (#1421) Combine compute_perturbed_quantities_and_interpolation further (#896) Only test python 3.12 in github workflows (#1417) Hash CI_JOB_NAME for gt4py cache directory (#1415) Bump GT4Py to v1.2.0 (#1414) Add a few more factory attrs to MPI tests (#1406) Fix boundary-row computation of pos_on_tplane_e and primal/dual_normal_cell (#1410) Minimal config system (#1391) Physics driver with Muphys (#1301) fix flaky test_cross_product (#1408) # Conflicts: # model/testing/src/icon4py/model/testing/definitions.py # uv.lock
Cut earlier as a duplicate of the runbook. It is, deliberately: a campaign ends in a terminal on the cluster, which is not where the runbook is. Points at the local copy so it cannot disagree with the code that just ran. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review comments from @nfarabullini. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
msimberg
left a comment
msimberg
left a comment
There was a problem hiding this comment.
No important comments. Question on the provenance tracking.
| @@ -0,0 +1,45 @@ | |||
| # ICON4Py - ICON inspired code in Python and GT4Py | |||
There was a problem hiding this comment.
I'm really not sure what this file is here for. Can you explain? I only see read_icon_revision used in one test.
If it really is needed, wouldn't it be easier to cache read_icon_revision instead of the manual cache-like thing with record?
There was a problem hiding this comment.
I also have the same question. I am curious about what is read_icon_revision used for?
There was a problem hiding this comment.
Leftovers. read_icon_revision did have a second caller — record() right below it, which datatest_utils.download_experiment called once per datatest. record kept a session-wide {archive: revision} dict that pytest_terminal_summary printed as a "Serialized test data" block, to inform where the used data is coming from.
That was part of a larger idea: having the test session itself track and report which ICON build produced the data it was validating against. We removed that and just record the provenance in the archive — the slurm log ICON writes carries its own git describe, the data version is pinned, and CI never runs against data nobody has looked at. When new data does turn up and a test starts disagreeing, the revision is recorded in the archive and both repos can be bisected by hand, which is what docs/testdata_generation.md describes. These two functions were simply left behind.
🤖 Written by an agent on behalf of @jcanton
🧑 and edited by @jcanton himself
OngChia
left a comment
OngChia
left a comment
There was a problem hiding this comment.
Looks good. I think you have used the script and know if this works well. I just left two-related questions because I didn't have enough understanding of how it works.
| @@ -0,0 +1,45 @@ | |||
| # ICON4Py - ICON inspired code in Python and GT4Py | |||
There was a problem hiding this comment.
I also have the same question. I am curious about what is read_icon_revision used for?
| dry_run: Annotated[ | ||
| bool, typer.Option("--dry-run", help="List the archives that would be written, then stop.") | ||
| ] = False, | ||
| allow_dirty: Annotated[ |
There was a problem hiding this comment.
Maybe you have told me before. And sorry if this is a stupid question, as I have not generated serialized data for a long time. May I know the reason for raising an error when a developer wants to generate the serialized data with modified icon4py checkout? Is it because you will record the icon version in icon4py using read_icon_revision function and compare it with the icon build that generates the serialized data? Therefore, it is important to check that you are did not modify icon4py?
There was a problem hiding this comment.
exactly: if you generate data without having committed your changes it will record the commit that does not match the code (both for icon and icon4py)
Leftover from the session-level provenance idea that was reverted in favour of recording provenance in the archive: the terminal summary section, the xdist workeroutput plumbing that fed it, and the module behind both. 🤖 Written by an agent on behalf of @jcanton
|
When developing, you can test your changes on CSCS CI before merge with the You can pass options to override pipeline variables, for example:
Available options are:
For each option, See The Merging Once your PR is approved and ready for merging, add it to the merge queue. The Optional Tests To run benchmarks you can use:
For more detailed information please look at CI in the EXCLAIM universe. |
from a friend:
Tooling around the serialized test data.
git describe, sogit log OLD..NEW -- <path>explains a changed reference value directly.ExperimentDescription.versionloses its shared default: experiments are versioned and regenerated individually, so bumping one no longer invalidates the other five.run-serializationgains--dry-run, a preflight that prints the ICON source tree revision and refuses a modified icon4py checkout, and per-task failure isolation so one bad task no longer aborts an 18-task campaign.scripts/python/inspect_savepoints.py: inspect the savepoints and fields of an archive.docs/testdata_generation.mdreplaces the HackMD page.