refactor(regression): collapse path-placeholder clump into PlaceholderMap#759
refactor(regression): collapse path-placeholder clump into PlaceholderMap#759lewisjared wants to merge 4 commits into
Conversation
…rMap Introduce a frozen PlaceholderMap value object in climate_ref_core.output_files that owns the (token, directory) placeholder set. Config-stable tokens come from for_baseline(); the per-execution <OUTPUT_DIR> is late-bound via with_output(); sanitise()/hydrate()/as_replacements() replace the to_placeholders/from_placeholders/_placeholder_pairs free functions, which are removed. Every caller now passes a single placeholders argument instead of threading output_dir/test_data_dir/software_root_dir through ~8 signatures, and the verification side (RegressionValidator, conftest ExecutionRegression/DiagnosticValidator) derives its replacements from the same map so capture and verification can no longer declare different token sets and drift apart. Behaviour-preserving.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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-core/src/climate_ref_core/regression/capture.py (1)
201-243: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject unbound placeholder maps before writing the committed bundle.
write_committed_bundle()now trusts callers to have already called.with_output(...), but nothing enforces that invariant. PassingPlaceholderMap.for_baseline(...)here will silently leave execution-specific output paths in the committed bundle and digest those machine-specific bytes.Proposed fix
def write_committed_bundle( source_dir: Path, regression_dir: Path, *, placeholders: PlaceholderMap, ) -> dict[str, str]: @@ regression_dir.mkdir(parents=True, exist_ok=True) + + if not any(token == PLACEHOLDER_OUTPUT_DIR for token, _ in placeholders.pairs): + raise ValueError("placeholders must be bound with .with_output(...) before writing a committed bundle") for filename in COMMITTED_BUNDLE_FILES: source = source_dir / filename
🧹 Nitpick comments (1)
packages/climate-ref/tests/integration/test_native_roundtrip.py (1)
309-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
<SOFTWARE_ROOT_DIR>round-trips.This helper now hard-codes a baseline map without
software_root_dir, so both round-trip paths still exercise only<OUTPUT_DIR>and<TEST_DATA_DIR>. That leaves the new third token untested even though the CLI wiring now supplies it. Consider threading a fake software root through one capture/replay scenario and asserting the sanitise/hydrate round-trip end to end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e41d26ae-f335-4315-a47d-0a33527f677d
📒 Files selected for processing (11)
changelog/759.trivial.mdpackages/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/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/src/climate_ref/conftest_plugin.pypackages/climate-ref/tests/integration/test_native_roundtrip.py
…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 Climate-REF#759/Climate-REF#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
1 participant
Description
Behaviour-preserving refactor stacked on #756. It collapses the regression-test path-placeholder data clump into a single
PlaceholderMapvalue object.Previously the placeholder set (
<OUTPUT_DIR>/<TEST_DATA_DIR>/<SOFTWARE_ROOT_DIR>) was expressed three ways: looseoutput_dir/test_data_dir/software_root_dirparameters threaded through ~8 signatures, a private_placeholder_pairslist, and hand-built{real: token}dicts at the verification call sites. Adding the third placeholder (<SOFTWARE_ROOT_DIR>, in #756) had to be threaded through every layer, and the verification side had already drifted — it only knew two of the three tokens.PlaceholderMap(inclimate_ref_core.output_files) now owns the(token, directory)set. The configuration-stable tokens come fromPlaceholderMap.for_baseline(...); the per-execution<OUTPUT_DIR>is late-bound via.with_output(...); and.sanitise()/.hydrate()/.as_replacements()replace the old free functions. The three free functions (to_placeholders,from_placeholders,_placeholder_pairs) are deleted, and every caller now passes a singleplaceholdersargument. The verification side (RegressionValidator, plus the conftestExecutionRegression/DiagnosticValidator) derives its replacements from the same map, so capture and verification can no longer declare different token sets and drift apart.No behaviour change: the path rewriting, longest-match-first ordering, and float/provenance handling are identical. Verified with
ruff,ruff format,mypy, and the existing unit + integration suites (241 passed, 1 skipped), plus an independent review pass.Checklist
Please confirm that this pull request has done the following:
changelog/(759.trivial.md)Summary by CodeRabbit
New Features
Bug Fixes