Regression baseline machinery: portable native baselines + reconstruction-input capture#761
Merged
Conversation
…pture Make committed regression baselines reproducible from their stored native blobs on any machine and provider, so `ref test-cases replay` and the coupling gate stop reporting spurious drift. Two layers: 1. Port the PlaceholderMap path machinery (from #759/#756): a single bidirectional map (`<OUTPUT_DIR>` / `<TEST_DATA_DIR>` / `<SOFTWARE_ROOT_DIR>`) drives capture sanitise, replay hydrate and the comparator, sourcing the software root from `config.paths.software`; plus `<USER>`/`<DATE>` provenance redaction in the committed bundle. 2. Capture reconstruction inputs and store native portably: - `copy_execution_outputs(..., extra_globs=)` persists extra declared globs beyond the bundle-referenced files (deduped, directories skipped). - new `Diagnostic.reconstruction_inputs` declares the raw artefacts a provider's `build_execution_result` re-scans (provenance YAML, driver `*_cmec.json`) that the curated set would otherwise exclude. - `snapshot_native` sanitises the slot to placeholders before digesting, so native digests are machine-independent (re-mint uploads nothing) and replay can hydrate the blobs anywhere -- load-bearing now that the captured provenance/cmec files are path-bearing. Rebuild stays on `build_execution_result`, so a replay re-derives the bundle from the captured inputs (option 2) rather than from an incomplete output dir. Tests: extra_globs behaviour, snapshot portability incl. `<SOFTWARE_ROOT_DIR>` and machine-independent digests, and a reconstruction-input round-trip proving replay rebuilds from a captured provenance file (positive) and fails without it (negative). Constraint: native blobs must stay machine-independent or re-mints churn the store Constraint: capture_execution writes the production results dir, so native is only sanitised in the throwaway output slot, never in place there Rejected: option 1 (rebuild from committed output.json) | neuters replay's ability to catch build_execution_result regressions Directive: a provider declaring reconstruction_inputs must also ensure build_execution_result is deterministic under replay (stable session dir; idempotent in-place mutations like pmp clean_up_json) Confidence: high Scope-risk: moderate Not-tested: real esmvaltool/pmp providers (no reconstruction_inputs declared yet; needs conda + baseline data) Claude-Session: https://claude.ai/code/session_01XCLPbjwskcRnJJ3JoQ2Tng
Wire up the providers to the reconstruction-input capture so their committed baselines can be rebuilt from the persisted native set on replay. - esmvaltool: persist `executions/recipe/run/*/*/diagnostic_provenance.yml`, the raw provenance `build_execution_result` globs to discover the run's outputs. The existing `prepare_regression_output` already stabilises the timestamped session dir to `recipe` and every path the provenance holds is under the execution output dir, so native sanitisation makes it portable. - pmp: persist the raw driver `*_cmec.json` that `process_json_result` consumes, declared once as `PMP_RECONSTRUCTION_INPUTS` in `pmp_driver` and shared by the three diagnostics (variability-modes, annual-cycle, enso). Tests: an esmvaltool capture -> sanitise -> hydrate -> rebuild round-trip proving the provenance is captured and the bundle rebuilds from it, and a pmp test pinning the declaration and that the glob matches the driver's CMEC JSON naming (mip-scoped and plain) but not the curated bundle. Constraint: pmp clean_up_json mutates the raw cmec.json in place during build; it runs before capture and is byte-idempotent on an already-cleaned file, so the capture->replay round-trip is stable Confidence: high Scope-risk: narrow Not-tested: full esmvaltool/pmp mint+replay end-to-end (needs conda + baseline data; validated via unit round-trips and the generic reconstruction-input test instead) Claude-Session: https://claude.ai/code/session_01XCLPbjwskcRnJJ3JoQ2Tng
…eview Two quality fixes from the /simplify pass (no behaviour change): - `snapshot_native` now takes `source` + an unbound `PlaceholderMap` and binds `with_output(source.bundle_output_dir)` internally, matching `stage_build`'s signature. Callers no longer pre-bind the map, so the two stages can't drift on which output directory they sanitise. - `_copy_extra_globs` takes an `exclude` set and owns all dedup, replacing the split copy/append-with-membership-check in `copy_execution_outputs`. Confidence: high Scope-risk: narrow Claude-Session: https://claude.ai/code/session_01XCLPbjwskcRnJJ3JoQ2Tng
Correctness: - Re-dump output.json provenance redaction with ensure_ascii=False so it matches production's pydantic model_dump_json; non-ASCII provenance no longer rewrites the whole file, preserving the "only redacted fields differ" committed-digest invariant. - Sort the PMP plot/data globs (annual_cycle, enso, variability_modes) so the output bundle's keys -- and the committed output.json bytes/digest -- are filesystem-order independent and reproducible across hosts. Cleanup / altitude: - Extract _rewrite_committed_json so float-rounding and provenance-redaction share one load/transform/conditional-redump path instead of two clones. - Single-source the ESMValTool provenance glob (_PROVENANCE_GLOB) so the reconstruction_inputs declaration and build_execution_result scan cannot drift. - Add baseline_placeholders() factory used by run/mint/replay/build, and a _baseline_placeholders property on RegressionValidator, so the placeholder token set is declared once per context. - Coerce CMEC metadata values to strings inside ILAMB's format_cmec_output_bundle rather than patching units at the call site. Tests: - Explicitly @pytest.mark.skip the offline test_validate_test_case_regression for all providers. Native baselines now live in the object store (Framework B), so RegressionValidator cannot rebuild from the committed bundle alone; regression coverage is provided by `ref test-cases replay`. The test body is retained for local step-through debugging. Constraint: Native baselines are object-store-only, so the committed-bundle replay path cannot run offline without sharing stage_materialise into the test. Rejected: Delete RegressionValidator | kept for local step-through debugging per maintainer request; migration intent not yet reconciled. Rejected: Wire the native store into RegressionValidator | larger architectural change, parked. Confidence: high Scope-risk: moderate Not-tested: PMP/ESMValTool committed-bundle rebuild (no committed catalog; skipped)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds ChangesPortable regression baseline replay
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/climate-ref/tests/integration/test_native_roundtrip.py (1)
303-309: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
capture_execution()still looks host-specific for native digests.This helper now passes a bound
PlaceholderMap, but the shared implementation inpackages/climate-ref-core/src/climate_ref_core/regression/capture.py:328-388still snapshotsbase_dirstraight aftercopy_execution_outputs(...)and never sanitises the copied tree first. That leaves native blobs authored via this path sensitive to absolute host paths, so these round-trip tests can miss the digest-churn bug this PR is meant to prevent.Possible fix in
packages/climate-ref-core/src/climate_ref_core/regression/capture.pyrelpaths = copy_execution_outputs( scratch_directory, results_directory, fragment, result, include_log=include_log, extra_globs=extra_globs, ) base_dir = results_directory / fragment committed = write_committed_bundle(base_dir, regression_dir, placeholders=placeholders) + placeholders.sanitise(base_dir) native = build_native_snapshot(base_dir, relpaths) return committed, native
🧹 Nitpick comments (2)
packages/climate-ref/tests/integration/test_native_roundtrip.py (1)
394-395: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the same
PlaceholderMapfor replay comparisons.These tests now hydrate with
PlaceholderMap, but the laterreplacementsmapping is still hand-rolled. If<SOFTWARE_ROOT_DIR>ever appears in the rebuilt bundle, the comparison path drifts from capture again. Please derive the replacements from the same map via.with_output(replay_dir).as_replacements().Also applies to: 594-595
packages/climate-ref-esmvaltool/tests/integration/test_diagnostics.py (1)
26-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the parked offline regression path opt-in, not permanently skipped.
An unconditional
@pytest.mark.skipmeans this body is not actually available for local step-through debugging unless someone edits the test file. Askipifbehind an env var or pytest option would keep CI parked while preserving the debug path you describe here. The same applies to the matching skip added in the other provider integration suites.Example pattern
+import os + -@pytest.mark.skip( +@pytest.mark.skipif( + not os.getenv("REF_ENABLE_OFFLINE_REGRESSION_DEBUG"), reason="Parked: RegressionValidator replays the committed bundle offline, but native baselines " "now live in the object store (Framework B), so build_execution_result cannot rebuild from the " "repo alone. `ref test-cases replay` provides regression coverage via the store; this offline " "path is retained (body intact) for local step-through debugging." )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fcd1905-89f5-4ee8-bf2e-5aaefd3a3bc8
📒 Files selected for processing (24)
changelog/761.fix.mdpackages/climate-ref-core/src/climate_ref_core/diagnostics.pypackages/climate-ref-core/src/climate_ref_core/output_files.pypackages/climate-ref-core/src/climate_ref_core/regression/capture.pypackages/climate-ref-core/src/climate_ref_core/testing.pypackages/climate-ref-core/tests/unit/regression/test_capture.pypackages/climate-ref-core/tests/unit/test_output_files.pypackages/climate-ref-esmvaltool/src/climate_ref_esmvaltool/diagnostics/base.pypackages/climate-ref-esmvaltool/tests/integration/test_diagnostics.pypackages/climate-ref-esmvaltool/tests/unit/diagnostics/test_base.pypackages/climate-ref-example/tests/integration/test_diagnostics.pypackages/climate-ref-ilamb/src/climate_ref_ilamb/standard.pypackages/climate-ref-ilamb/tests/integration/test_diagnostics.pypackages/climate-ref-pmp/src/climate_ref_pmp/diagnostics/annual_cycle.pypackages/climate-ref-pmp/src/climate_ref_pmp/diagnostics/enso.pypackages/climate-ref-pmp/src/climate_ref_pmp/diagnostics/variability_modes.pypackages/climate-ref-pmp/src/climate_ref_pmp/pmp_driver.pypackages/climate-ref-pmp/tests/integration/test_diagnostics.pypackages/climate-ref-pmp/tests/unit/test_pmp_driver.pypackages/climate-ref/src/climate_ref/cli/test_cases/_stages.pypackages/climate-ref/src/climate_ref/cli/test_cases/baselines.pypackages/climate-ref/src/climate_ref/cli/test_cases/run.pypackages/climate-ref/tests/integration/test_native_roundtrip.pypackages/climate-ref/tests/unit/cli/test_test_cases.py
Per code review: provenance redaction scrubbed userId/date but left the nested `platform` block intact. Real PMP provenance records `platform.Name` (the minting hostname, e.g. "gus") and `platform.Version` (the kernel version) — both leak host identity into the git-tracked bundle and churn its committed digest across machines, defeating the portability goal. Redact them to <HOSTNAME>/<HOST_VERSION>; keep the coarse `OS` value as portable context. Extracts a `_redact_fields` helper so the provenance and platform sub-blocks share one set-if-present-and-changed loop, and strengthens test_write_committed_bundle_redacts_and_placeholders_provenance to assert the host fields no longer survive. No committed baseline leaks today (PMP, the only provenance-bearing provider, has no committed regression bundles yet), so this hardens the redaction before those land. Constraint: redaction must change only the targeted fields to keep the committed digest reproducible at mint and replay. Confidence: high Scope-risk: narrow
Drop comments that restated the code (e.g. "Find the other outputs") and tighten the rest to the part that isn't obvious: why the PMP globs are sorted (deterministic committed output.json digest), why output.json re-dumps with ensure_ascii=False, why platform host fields are redacted while OS is kept, and why ILAMB coerces metadata to str. Comment-only; no behaviour change.
lewisjared
force-pushed
the
feat/regression-machinery
branch
from
6c8148f to
013fd18
Compare
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Replace the per-file maps (_COMMITTED_FLOAT_JSON_KWARGS / _UNROUNDED_COMMITTED_FILES / _PROVENANCE_BEARING_FILES / _COMMITTED_DUMP_KWARGS) and their round/redact/rewrite helpers with a single pass: every committed JSON file is float-rounded, provenance- redacted, and re-dumped with one canonical format (sort_keys, indent, allow_nan=False, ensure_ascii=False). A file with no floats or no provenance just passes those steps through, so output.json no longer needs a special "unrounded, preserve native order" case. The bundle comparator is structural (compares dict key sets, not bytes), so sorting output.json keys is safe and makes the committed bytes fully deterministic. Net ~130 fewer lines; the serialisation rule now lives in one constant instead of four filename-keyed maps. Note: output.json is now sorted/canonical, so existing committed baselines should be re-minted to match; the tolerant replay comparison reconciles them until then. Constraint: committed bytes must be identical at mint and replay across machines. Confidence: high Scope-risk: moderate
This was referenced Jul 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch builds out the portable regression-baseline machinery (native baselines in an object store, reconstruction-input capture, the
ref test-cases run/mint/replay/buildlifecycle) and includes a round of code-review fixes on top.The most recent commit (
fix(regression): address code-review findings in baseline machinery) addresses findings from a high-effort review of the diff:Correctness
output.jsonredaction byte-stability — the provenance-redaction re-dump now usesensure_ascii=False, matching production's pydanticmodel_dump_json. Previously, any non-ASCII provenance value (e.g. an accenteduserId) caused the whole file to be re-escaped, breaking the "only redacted fields differ" committed-digest invariant. Verified with a non-ASCII round-trip (only the redacted fields change; no\uescapes).annual_cycle,enso, andvariability_modesare now sorted, so the output bundle's keys (and the committedoutput.jsonbytes/digest) are filesystem-order independent across hosts.Cleanup / altitude
_rewrite_committed_jsonso float-rounding and provenance-redaction share one load/transform/conditional-redump path._PROVENANCE_GLOB) so thereconstruction_inputsdeclaration and thebuild_execution_resultscan can't drift apart.baseline_placeholders(paths, config)factory used by all fourref test-casesverbs, plus a_baseline_placeholdersproperty onRegressionValidator, so the placeholder token set is declared once per context.format_cmec_output_bundlerather than patchingunitsat the call site.Tests
@pytest.mark.skipthe offlinetest_validate_test_case_regressionfor all providers. Native baselines now live in the object store, soRegressionValidatorcannot rebuild from the committed bundle alone; regression coverage is provided byref test-cases replay. The test body is retained intact for local step-through debugging.Parked (not addressed here)
RegressionValidatorshould be deleted or wired to the native store (stage_materialise) so the offline committed-bundle check runs in CI again. The offline tests are skipped for now; the class and its unit tests are retained.handle_execution_result) intentionally does not persistreconstruction_inputs— documented as designed.Verification
uv run pre-commit run(ruff check + format, mypy) — all pass.test_get_data_pathfailures unrelated to this branch).test_validate_test_case_regression: 49 explicitly skipped.https://claude.ai/code/session_01XCLPbjwskcRnJJ3JoQ2Tng
Summary by CodeRabbit