Skip to content

REGRESSION - IMPROVEMENT - Pin the package set and fingerprint run environments - #361

Open
d-burg wants to merge 2 commits into
developfrom
feature/harness-env-integrity
Open

REGRESSION - IMPROVEMENT - Pin the package set and fingerprint run environments#361
d-burg wants to merge 2 commits into
developfrom
feature/harness-env-integrity

Conversation

@d-burg

@d-burg d-burg commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

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), while Manifest.toml is untracked and each worktree ran Pkg.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_ref takes the local branch pointer with no upstream check, so a months-old local develop silently becomes the baseline.

What this changes

The package set is pinned across refs. 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 — source code becomes the only variable by construction. --no-pin-manifest opts out, loudly. If a pinned set turns out to be 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.

This deliberately does not track Manifest.toml in 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:

Ref 1: develop  @ 5b6ba34a (2026-08-12)
       env: julia 1.11.6, arm64-apple-darwin24.0.0, manifest 7e5c34ad (pinned), 1 thread/8 BLAS

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-change additionally 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 that DIIID-like_SLAYER_example sets parallel_threads = 1 because threaded BVP Δ′ was not trusted to be reproducible, while the DIII-D ideal deck — the one whose Δ′ the harness pins — runs parallel_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 develop vs local on macOS arm64, Julia 1.11.6.

solovev_n1 — full gpec_run path (worktree + example deck + instantiate):

Ref 1: develop  @ a0cad260   env: julia 1.11.6, arm64-apple-darwin24.0.0, manifest 7e5c34ad (pinned), 1 thread/8 BLAS
Ref 2: local    @ local      env: julia 1.11.6, arm64-apple-darwin24.0.0, manifest 7e5c34ad (pinned), 1 thread/8 BLAS
total energy Re(et[1])        5.748525e-01     5.748525e-01     0.0e+00    OK
total energy Im(et[1])       -1.054421e-03    -1.054421e-03     0.0e+00    OK
plasma energy Re(ep[1])      -9.821799e+00    -9.821799e+00     0.0e+00    OK
vacuum energy Re(ev[1])       1.039665e+01     1.039665e+01     0.0e+00    OK
vacuum matrix min eigenvalue  2.171612e+00     2.171612e+00     0.0e+00    OK
ODE steps (saved/total)       394 / 626        394 / 626        0.0e+00    OK
q0 / q95                      1.900006 / 3.147422                0.0e+00   OK
beta_t / beta_n               4.620965e-02 / 3.215511            0.0e+00   OK
# singular surfaces           2                2                0.0e+00    OK
q, pressure profiles          22fbe91bcd6e…    22fbe91bcd6e…    identical  OK
Summary: 21 unchanged

ggj_reference and ggj_ray_q500i (computed cases): 4 unchanged each, 0.0e+00 on every quantity.

Also exercised directly:

  • Cache hit under a matching environment — a repeat run reported Cached: ggj_reference @ a0cad260 rather than re-running.
  • Pre-migration rows (NULL environment columns, from before this change) read cleanly, and is_cached returns false for them against a fresh key, so they are re-run rather than trusted.
  • Stale-baseline detection fired for real: develop moved 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/develop unprompted.
  • upstream_lag on a branch with no upstream, and on HEAD, 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_q500i computed 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 missing for NULL while something only skips nothing, so every pre-migration row would have thrown on read; and HEAD matched the always-present symbolic ref origin/HEAD, which would have reported every feature branch as a stale copy of develop.

JuliaFormatter and pre-commit are 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

…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>
Copilot AI lite review requested due to automatic review settings August 12, 2026 20:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-change for 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.

Comment thread docs/development/regression-harness.md Outdated
Comment on lines +117 to +120
**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:
Comment on lines +11 to +12

"""
Comment thread docs/src/developer_notes.md Outdated
…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>
@d-burg

d-burg commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

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. docs/src/developer_notes.md linked ../../development/regression-harness.md, which resolves outside the build directory, and Documenter terminates on :cross_references. Now an absolute GitHub URL, matching how that file already links DOC_STANDARD.md.

Cache wording: also correct. The schema keeps UNIQUE(commit_hash, case_name), so a re-run replaces the stored result — there is no per-environment cache. Reworded to say what actually happens: the fingerprint provides invalidation (a cached result from a different environment is re-run rather than reused), and --allow-env-mismatch reuses whatever is cached regardless of provenance.

SHA not imported in env.jl — this one is a false positive, and I've left the code as is. regression-harness/regress.jl:3 does using Dates, HDF5, JSON, Printf, SHA, SQLite, Tables, TOML, and every src/*.jl is included into that scope; none of the sibling files carry their own imports either (database.jl uses SQLite/DBInterface, runner.jl uses TOML/Dates, same way). The using SHA visible at runner.jl:12 is inside the run-info epilogue string literal — subprocess code, not a module-level import.

Empirically: the harness ran end-to-end three times while validating this branch and reported manifest hashes (manifest 7e5c34ad (pinned)), which is SHA.sha256 executing. An UndefVarError would have surfaced on the first call.

Happy to add an explicit using SHA to env.jl if reviewers prefer defensive imports, but it would be the only file in the harness doing so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants