feat(run): sampled runs that can never become the canonical run - #25
feat(run): sampled runs that can never become the canonical run#25jaakla wants to merge 1 commit into
Conversation
A wide-area analysis can run for hours before a late step fails. A sampled run executes the same pipeline over a deliberately smaller slice so that failure arrives in minutes. The risk this introduces is laundering: a fast pass over a clipped AOI presented as the analysis. Sampling clips or thins the inputs, so a sampled run proves the pipeline executes and nothing more -- clipping breaks every neighbourhood operation at the cut, row sampling destroys the spatial coherence a join depends on, and raster downsampling changes areas and slopes non-linearly. So non-promotion is enforced structurally, not by convention: - a sampled run record declares `mode: sampled` and must state what it *realized*, not only what was requested (TABLESAMPLE only approximates); - `runs.latest` may never reference one. It is what `verify`, the clean-rerun protocol, and every expectation attestation bind to; - `run --sample*` re-reads the manifest afterwards and fails if the pipeline promoted itself, and reports declared outputs it overwrote in place. Sampling reuses the existing `runtime.implementation.parameters` contract rather than adding a schema block: a parameter opts in with a `role` (sample_area | sample_rows | sample_fraction) and an optional `sample:` default. `canonical` must mean "no sampling", so the canonical run still passes nothing. A sampling parameter cannot pair step/field -- it selects input, not a processing threshold. `--dry-run` is deliberately untouched: it already means "print the command, execute nothing", and redefining a published flag would be worse than adding four honest ones. Reported as `runs.sample_isolation` by validate, and exposed to external harnesses as the additive `validation.sample_run_not_promoted` check. Evidence: 40 unit tests; eval 016-sampled-run (control) and 928-sampled-run-as-canonical (mutation, isolated). Full unit suite 452 passing; fixture evals 16/16 contract_ci, 26/26 mutation. Closes #20 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMMy6A73zi7yGjK3wHBhju
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29a68d4286
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| latest_after = get_in(after, "runs", "latest", "id") if after else None | ||
| if after and latest_after != latest_before: |
There was a problem hiding this comment.
Reject changes to the existing runs.latest record even when its ID remains unchanged. A pipeline can overwrite runs/<latest_before>.json with a valid mode: sampled record while leaving the manifest ID untouched; this comparison then passes, run_record_errors() finds no problem, and the sampled command returns success despite runs.latest now resolving to sampled evidence.
Useful? React with 👍 / 👎.
| # Any run record the pipeline just wrote must declare its realized sample; | ||
| # reuse the shipped check so the CLI and the check API cannot drift. | ||
| for record_path in sorted((root / "runs").glob("*.json")) if (root / "runs").is_dir() else []: |
There was a problem hiding this comment.
Require at least one newly created or modified sampled record for this invocation. If a pipeline ignores the sampling arguments and writes no record, this loop examines zero files (or only pre-existing valid files), leaves problems empty, and reports success without any mode: sampled marker or realized-sample measurement, so the CLI cannot substantiate that sampling occurred.
Useful? React with 👍 / 👎.
| overwritten = sorted( | ||
| path | ||
| for path, digest in outputs_after.items() | ||
| if path in outputs_before and outputs_before[path] != digest | ||
| ) |
There was a problem hiding this comment.
Treat outputs that existed before the sampled run but are absent afterward as overwritten. Because this comprehension iterates only outputs_after, a sampled pipeline that cleans its output directory and omits one declared artifact returns success with an empty warning for that file—even under --strict—although it has destroyed a canonical output and subsequent validation will fail.
Useful? React with 👍 / 👎.
| elif not value_has_type(value, parameter.type): | ||
| raise SamplingError(f"{_ROLE_FLAGS[role]} must be a {parameter.type} for parameter {parameter.id!r}") | ||
| extra_argv, extra_environment = parameter.bind(value) |
There was a problem hiding this comment.
Reject explicit values equal to the parameter's canonical no-sampling value. For example, an integer sampling parameter is required to use canonical 0, but --sample-rows 0 passes this type-only check and is labeled as a sampled run even though the declared semantics say that value disables sampling; the same bypass is possible with an empty string passed to a string role.
Useful? React with 👍 / 👎.
1 participant
Implements workstream (a) of #20: sampled runs, marked and structurally unable to become the canonical run. The other three workstreams were split out to #22 (skill guidance), #23 (cost estimation), #24 (federated execution).
The problem this creates, and how it is closed
A sampled run is a fast pass over a clipped AOI. Its numbers describe a slice, so the danger is not that sampling fails — it is that a sampled result gets presented as the analysis.
908-validation-launderingalready covers that failure class; this is a new route into it.Non-promotion is therefore enforced structurally rather than by convention:
inputs_hash(clipped inputs are different bytes), so it cannot share the canonical hash chain;mode: sampledand must state what it realized, not only what was requested —TABLESAMPLEand friends only approximate, so the realized rows/AOI/resolution are the measurement;runs.latestmay never reference a sampled record. That is whatverify, the clean-rerun protocol, and everyvalidation.expectationsattestation bind to;openmapstack run --sample*re-reads the manifest after the pipeline exits and fails if the pipeline promoted its own run — the guard does not rely on the pipeline behaving.Reported as
runs.sample_isolationbyvalidate, and exposed to external harnesses asvalidation.sample_run_not_promoted(additive, so no check-API major bump).CLI
--dry-runis deliberately untouched — it already means "print the command, execute nothing" (cli.py:49,README.md:173), and redefining a published flag would be worse than adding four honest ones. A single--sampling-size 1%was rejected for the same reason: percent of area, of rows, and of raster resolution are three different knobs with three different failure modes.Sampling a project that declares no matching role is refused with a message naming what the manifest must add, rather than silently running unsampled.
Where the parameter lives
Reuses the existing
runtime.implementation.parameterscontract instead of adding asampling:block. A parameter opts in with arole(sample_area|sample_rows|sample_fraction) and an optionalsample:default:canonicalmust be the role's no-sampling value, which is what keeps "the canonical run passes nothing" true. A sampling parameter cannot pairstep/field— it selects input, not a processing threshold, so there is no step value forparameters_match_stepsto agree with. This resolves the open question the issue raised aboutstep/fieldbinding.Outputs overwritten in place
If a sampled run rewrites the declared outputs, the existing
outputs_hashmachinery already refuses to call the project validated — the files no longer hash to what the canonical run recorded. The CLI reports the clobber explicitly (canonical_outputs_overwritten) so that later failure is not a surprise, and--strictturns it into a non-zero exit.Evidence
tests/test_sampling.py): the parameter contract, role resolution, run-record shape, the isolation invariant through bothvalidateand the check API, and the CLI end-to-end including a fixture pipeline that deliberately promotes itself and one that writes a record stating only its request.016-sampled-run— a sampled record sits beside the canonical one, marked, realized, notruns.latest, and nothing about its presence weakens the canonical run.928-sampled-run-as-canonical— the run of record is the sampled one. Every hash resolves and every declared check is present; it fails on the marking alone.Ran locally: full unit suite 452 passing; fixture evals 16/16 contract_ci, 26/26 mutation (100% detected, 26 isolated); worked-example
validate --preflightandverifyunchanged.Two notes on what I did not do:
coverage reportgate reads 68% locally, below its 70% floor. That is unchanged frommainin this environment (PyQGIS and Playwright absent, so thechecks/qgis.pyandchecks/visual.pybodies do not execute).checks/validation.pyitself went 79% → 80%.verifygoldens were hand-edited, not regenerated: this environment lacks DuckDB Spatial, so regenerating would have bakednot_testableinto lines CI produces aspassed. Only the new check line and the affected counts were changed.Choosing a representative sample is deliberately out of scope — it needs populated-place data the CLI does not have and has no deterministic oracle. That is #22.
🤖 Generated with Claude Code
https://claude.ai/code/session_01TMMy6A73zi7yGjK3wHBhju