REGRESSION - IMPROVEMENT - Pin the package set and fingerprint run environments - #361
REGRESSION - IMPROVEMENT - Pin the package set and fingerprint run environments#361d-burg wants to merge 2 commits into
Conversation
…vironments The harness cached results keyed only on (commit, case), while Manifest.toml is untracked and every worktree resolved its own package set at run time. Two runs of the same source could therefore differ by a package set, and machine-epsilon differences in library math are amplified by the adaptive ODE controller and by ill-conditioned near-resonant diagnostics into double-digit-percent "regressions" that no source change caused. Resolves the confound reported in issue #338, plus the stale-baseline failure mode raised in its comments. Three mechanisms: - The working tree's resolved Manifest.toml is copied into every worktree before Pkg.instantiate(), so all refs in a comparison run against one package set. --no-pin-manifest opts out loudly. If a pinned set proves incompatible with an old commit's Project.toml, instantiate re-resolves it and the harness reports that the pin did not hold rather than reverting silently. - Every run records the environment that produced it (Julia version, host, resolved Manifest hash, Julia and BLAS thread counts), measured inside the subprocess after instantiate so it describes what ran rather than what was intended. Results cache under an environment key; a cached result from a different environment is re-run, and reports print each ref's environment and flag differences between them. Rows cached before fingerprinting carry no environment and are re-run once. - A ref that lags its remote tracking branch gets a stale-baseline banner with the commands to update it, so a months-old local develop can no longer be benchmarked against silently. Thread counts are recorded but deliberately not forced: forcing them would change how every run executes and would hide the parallel-vs-serial reproducibility question rather than exposing it. Exit status is now meaningful — non-zero when a run fails, and with --fail-on-change when any tracked quantity moved — so the harness can gate CI. Pass/fail against committed golden values is a separate, later change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the regression harness’ reproducibility and cache correctness by (1) pinning the package environment across compared refs (via copying the working tree Manifest.toml into each worktree) and (2) recording/fingerprinting the actual runtime environment for every run so mismatched cached results are not silently reused. It also adds stale-baseline detection for refs that lag their upstream and introduces an exit-status contract suitable for CI gating.
Changes:
- Pin the resolved package set across worktrees (opt-out via
--no-pin-manifest) and warn when the pin breaks due to re-resolution. - Record and report per-run environment metadata (Julia version, host, Manifest hash, Julia/BLAS threads) and use an environment key to decide cache reuse (opt-out via
--allow-env-mismatch). - Add stale-upstream banners and meaningful exit codes (
--fail-on-changefor CI).
IMPORTANT: NO PULL REQUESTS SHOULD EVER BE MERGED WITHOUT A THIRD-PARTY HUMAN REVIEWER'S APPROVAL.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| regression-harness/src/utils.jl | Adds upstream-lag detection and manifest pin-copying into worktrees. |
| regression-harness/src/types.jl | Extends CLI options to support manifest pinning, env mismatch policy, and fail-on-change. |
| regression-harness/src/runner.jl | Adds shared run-info epilogue, env-aware cache invalidation, and manifest pin plumbing through runners. |
| regression-harness/src/reporter.jl | Prints per-ref environment summaries and warns when compared runs differ by environment. |
| regression-harness/src/env.jl | Introduces environment fingerprint + env-key computation and run-info parsing helpers. |
| regression-harness/src/database.jl | Migrates schema to store environment columns and adds env-key-aware cache queries. |
| regression-harness/regress.jl | Wires new flags, stale-ref warnings, manifest pin default behavior, and CI-friendly exit status. |
| docs/src/developer_notes.md | Points readers to detailed regression harness flag/env behavior documentation. |
| docs/development/regression-harness.md | Documents new flags, pinning behavior, env fingerprinting, and exit status contract. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| **Every run records the environment that produced it** — Julia version, host, resolved Manifest | ||
| hash, Julia and BLAS thread counts — and results are cached under that environment. A cached | ||
| result from a different environment is re-run instead of silently reused, and every report prints | ||
| the environment of each ref: |
|
|
||
| """ |
…ecisely The cross-reference added to developer_notes.md pointed outside docs/src/, which Documenter rejects as an invalid local link, failing the docs build. Use the full GitHub URL, matching how the file already links to DOC_STANDARD.md. Also corrects the harness documentation: the schema keeps one row per (commit, case), so a re-run replaces the stored result rather than caching one result per environment. What the fingerprint adds is invalidation — a cached result from a different environment is re-run instead of reused — and --allow-env-mismatch reuses whatever is cached regardless of its provenance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — pushed 8d26ec9 addressing the review. Documentation build failure (the invalid local link): correct, and it's my error — CLAUDE.md warns about exactly this class. Cache wording: also correct. The schema keeps
Empirically: the harness ran end-to-end three times while validating this branch and reported manifest hashes ( Happy to add an explicit |
Addresses #338 — proposed fixes 1 and 2, plus the stale-baseline failure mode raised in its comments.
Problem
The harness cached results keyed only on
(commit_hash, case_name), whileManifest.tomlis untracked and each worktree ranPkg.instantiate()against a bare checkout. Two runs of the same source could therefore resolve different package sets, and machine-epsilon differences in library math are amplified by the adaptive ODE step controller and by ill-conditioned near-resonant diagnostics into double-digit-percent "regressions" that no source change caused. Separately,resolve_reftakes the local branch pointer with no upstream check, so a months-old localdevelopsilently becomes the baseline.What this changes
The package set is pinned across refs. The working tree's resolved
Manifest.tomlis copied into every worktree beforePkg.instantiate(), so all refs in a comparison run against one package set — source code becomes the only variable by construction.--no-pin-manifestopts out, loudly. If a pinned set turns out to be incompatible with an old commit'sProject.toml, instantiate re-resolves it and the harness reports that the pin did not hold rather than reverting silently.This deliberately does not track
Manifest.tomlin git, per the concern in the issue thread about pinning everyone to stale packages. Only the comparison is pinned, and only to whatever the person running it already has.Every run records the environment that produced it — Julia version, host, resolved Manifest hash, Julia and BLAS thread counts — measured inside the subprocess after instantiate, so it describes what actually ran. Results cache under an environment key, and a cached result from a different environment is re-run instead of reused. Reports print each ref's environment and print a banner when two compared runs did not share one:
Stale baselines are called out. A ref behind its remote tracking branch gets a banner naming the gap and the commands to close it.
Exit status is meaningful. Non-zero when a run fails;
--fail-on-changeadditionally exits non-zero when any tracked quantity moved, so the harness can gate CI. Pass/fail against committed golden values is a separate, later change.Deliberately not done
Thread counts are recorded, not forced. Pinning
BLAS.set_num_threads(1)would make runs more reproducible, but it silently changes how everyone's runs execute, invalidates historical comparisons, and would hide the parallel-vs-serial reproducibility question rather than expose it. Note thatDIIID-like_SLAYER_examplesetsparallel_threads = 1because threaded BVP Δ′ was not trusted to be reproducible, while the DIII-D ideal deck — the one whose Δ′ the harness pins — runsparallel_threads = 2. Recording thread counts is what makes that testable; a follow-up branch will test it directly.A tracked golden-generation environment (issue #338 proposed fix 3, and the Dependabot idea from the thread) is not here. It only makes sense once golden values exist, so it belongs with that work.
Validation
Each case run
developvslocalon macOS arm64, Julia 1.11.6.solovev_n1— fullgpec_runpath (worktree + example deck + instantiate):ggj_referenceandggj_ray_q500i(computed cases): 4 unchanged each,0.0e+00on every quantity.Also exercised directly:
Cached: ggj_reference @ a0cad260rather than re-running.is_cachedreturns false for them against a fresh key, so they are re-run rather than trusted.developmoved 46 commits mid-session (InnerLayer (GGJ) - NEW - rotated-ray collocation backend (:ray), robust to |Q| ~ 500 #323 landing), and the banner reported'develop' is 46 commit(s) behind origin/developunprompted.upstream_lagon a branch with no upstream, and onHEAD, correctly returns nothing.Notes for reviewers
Existing caches invalidate once. Rows cached before this change carry no environment fingerprint and never match a computed key, so the first run after merging re-runs them. That is the intent — those are precisely the entries whose provenance cannot be established.
One semantic merge worth a second opinion. #323 added the
ggj_ray_q500icomputed case whose script template ended with the old bare-runtime write; this branch replaces that mechanism with a shared run-info epilogue. The rebase merged both sides textually with no conflict, but the new case would then have written a file the parser reads as "no environment" — NaN runtime, unknown-environment key, never cacheable. The template is converted to%RUNINFO%here, and the case is validated above. @matt-pharr / whoever owns #323 may want to confirm.Two bugs caught by testing rather than reading, both now fixed: SQLite returns
missingfor NULL whilesomethingonly skipsnothing, so every pre-migration row would have thrown on read; andHEADmatched the always-present symbolic reforigin/HEAD, which would have reported every feature branch as a stale copy of develop.JuliaFormatterandpre-commitare not installed on the machine this was written on, so style was checked by hand against.JuliaFormatter.toml— margin, kwarg spacing,for … in, EOF newlines, trailing whitespace. The hooks have not run.🤖 Generated with Claude Code