Repository navigation
Repo - PERF - Shorten test wall time and close periodic kinetic samples - #593
Conversation
matt-pharr
changed the title
github-actions
Bot
added
perf
logan-nc
left a comment
logan-nc
left a comment
There was a problem hiding this comment.
Read in full (test files, workflows, the one src/ hunk). Nothing pushed. Findings not already on the record:
- The 1.11 groups are red because the precompile job's depot save silently failed. Its log reads
Failed to save: Unable to reserve cache with key julia-depot-v3-…-1.11-1.11.9-…, another job may be creating this cache,actions/cache/savetreats that as a warning, so the job stayed green andneeds: precompilelet the three groups run intofail-on-cache-miss: true(julia-test-version.yaml:218,226). The restore key is identical to the save key, so this is not a fingerprint bug; it is the reservation race (the cancelled run on b1d1062 is the likely holder). Either fail the precompile job when its save fails, or let a group rebuild on a miss. The same setting will also redden every fork PR, whose read-only token cannot save a cache, unless develop already holds the key. - The registry check will stop the suite after the next develop merge. develop now has
test/runtests_localstability.jlandtest/runtests_analysis_plots.jl(#568, #573); neither is intest/suites.jl, andruntests.jlraiseserror("Update test/suites.jl …")before any test runs. - Stamp. The PR's own
src/patch differs between ea32a0c and the head 685c52c, so the stamp check is red; theTorque.jlendpoint closure needs a restamp. - What the suite no longer pins, for the maintainers to accept explicitly:
et_ric ≈ 0.800637and the three Δ′ diagonals (q = 2, 3, 4) from the DIII-D large-N Riccati testsets are removed with nothing in their place. The comment points atdiiid_n1_riccati, which does track the Δ′ diagonal, but no workflow runs the harness, so CI keeps no production-size numerical check after this. Also quieter than the comments admit: Monte-Carlo sample counts cut ~10× with the moment tolerances unchanged (flakiness risk),isfiledropped for most ErrorFields plots, and the sensitivity comparisons sliced tosets[1:1]so the second hoop is untested. - The Torque.jl hunk is not test-only. It overwrites all five sampled arrays at θ = 1 with the θ = 0 samples before the periodic spline, which changes the spline input at rounding level; the 53-row harness result covers it at 1e-8, fine, but it is a numerical change and the note should say so rather than "test feedback".
- The body says keep this a draft with no reviewer; it is marked ready and requests me. One of the two is stale.
|
@logan-nc sorry for the premature review request. I have a few comments.
This PR is now ready for your eyes, it should be quick. I would like to get this in ASAP as it will streamline the merging of the rest of our 2.0.0 items. Any further commits you see after this point will just be addressing merge conflicts with develop as they arise throughout the day. |
matt-pharr
changed the title
logan-nc
left a comment
logan-nc
left a comment
There was a problem hiding this comment.
Second pass at 4ed1f1fd2, this time on the big picture as Matt asked. Last night's items (cache-reservation race, missing registry entries, stale stamp, draft wording) are all addressed; I am not repeating them. Nothing below blocks approval; the two harness cases at the end are the only pre-merge ask.
The design is right: precompile once per Julia version, hand the compiled environment to parallel groups through a required artifact with caches as optional acceleration, aggregate behind one stable Tests status, and prepare merge_group so the queue replaces the update-and-rerun cycle. What follows is where the time goes after this PR, because that is what decides the next structural step.
Where the wall time goes now
Measured on the latest run (40500657019, a develop-merge commit) and the three most recent Documentation runs on this branch:
| Component | Measured | On the critical path? |
|---|---|---|
GPEC image rebuild, any src/ change |
331 s (1.x) / 337 s (1.11), serial, per version | yes, gates every group |
| Documentation job | 425, 448, 554 s on every PR push | yes, required check, untouched by this PR |
| Longest group (core, 1.11) | 305 s job; 242 s in test files, of which 96 s is compile | yes |
| Other groups | api 141 s / errorfields 119 s of test-file time (1.11) | no |
| Environment artifact | 759 MB / 943 MB, 6 to 10 s to upload | no |
| Warm test-only push | about 7 min end to end |
Two consequences. First, Documentation is now the long pole on a warm push and ties the cold path: it recompiles GPEC in a separate, unpinned docs environment on every push. Second, the "image rebuild" cost is almost entirely the PrecompileTools workload, not the package itself. I measured that on feynman with a fresh stacked depot at develop db90e824d:
| GPEC image build | |
|---|---|
default (src/Precompile.jl runs two Solovev cases) |
178 s (187 s wall) |
precompile_workload = false |
22 s (29 s wall) |
So roughly nine tenths of the 5.5-minute precompile step is the workload executing physics at build time, once per Julia version, on every PR that touches src/.
Structural suggestions (post-2.0 direction, with what I would do now)
-
Turn the workload off for CI test and docs environments, now. One
LocalPreferences.tomlline written by the workflow (precompile_workload = false, the switch already documented indocs/src/set_up.md) should take the precompile job from about 5.5 min to under 1 min per version on asrc/change. The groups then pay more first-use compilation, but they pay it in parallel, and they already spend 74 to 126 s compiling with the workload present. This needs one measurement, not an argument: run it once and compare the critical path. Keep the workload on for the images users build. The same switch in the docs environment should cut the Documentation job by a similar amount, since Documenter never runs physics. -
Documentation should stop being a per-push gate. Two options. (a) Trigger it on
merge_groupplus PRs that touchdocs/, with a skip-aware status job exactly likeTests, so every PR still passes it once in the queue; this loses per-push previews. (b) Keep it per push but have the docs environment consume the pinned CI manifest and the same depot and GPEC-image cache keys asjulia-test-version.yaml, so it stops compiling its own copy of everything. I would do (a) now because it is twenty lines and removes the long pole, and keep (b) for when the queue is running. Either way, the sentence in the rollout notes about keepingDocumentationrequired stays true. -
Name the floor. With the workload off, the floor on a
src/-touching PR is the 22 to 40 s image build plus first-use JIT in the groups. Below that, only one thing helps: Julia rebuilds the whole package image for any change undersrc/, so a Tearing edit recompiles Vacuum and Equilibrium. The post-2.0 answer is subpackages with path dependencies inside this repo (Utilities, Equilibrium, Vacuum, ... as their ownProject.toml), which also shortens every developer's edit-test loop without Revise. Not for this PR; worth an issue with the numbers above as its motivation. -
Make the two-tier contract explicit in the test files, not only in the PR description. Matt's framing is right: the harness tracks numbers, the suite exercises paths. The weak point is that the shrunken fixtures and sample counts replace the production ones, so the full-size checks are gone rather than moved. A one-line convention fixes that: each test file reads one tier flag (
GPEC_TEST_TIER=fullor similar) and picksnsample, grid sizes and fixture decks from it, with the fast values as the default.suites.jlalready carries one attribute per file; a second one (tier) is natural. A scheduled workflow then runs the full tier plus the harness ondevelopnightly. When Matt's Slurm fanout harness lands, that is the job it slots into, and it closes the gap this PR opens by removing pins (see below). -
Pin the Julia matrix to explicit minors. The floating
1.xis how the 1.13 periodic-spline break arrived mid-weekend, and each new Julia minor now needs a 3500-line manifest commit before CI is reproducible again.['1.11', '1.13']bumped deliberately is calmer. If early warning on the newest Julia is wanted, run it as an advisory job that does not feed the requiredTestsstatus, so a Julia release never blocks merges for everyone at once. -
Merge-queue rollout notes. Branch protection on
developcurrently hasDocumentationandTestsrequired withstrict(require up to date) on. The GitHub docs say the queue "provides the same benefits as the Require branches to be up to date before merging branch protection" without requiring the author to update first, sostrictbecomes redundant once the queue is on and is worth switching off in the same change. Both required checks already handlemerge_group, andPR Conventionsis not required, so the queue cannot stall on a check that never reports. One addition for the notes: set the queue's status-check timeout explicitly, because a required check that never reports is the only queue failure mode the docs describe. -
Self-hosted snapshot path.
ci/local-julia-cache.shand theGPEC_LOCAL_CACHE_DIRbranches are fine to ship, but nothing in CI exercises them. One sentence inci/manifests/README.mdsaying they are manually tested on pharr-desktop keeps the next person from assuming CI covers them.
Speed nits (each with what it buys)
- Rebalance the groups from the measured file times. core carries 242 s of test-file time against errorfields 119 s and api 141 s. Moving
runtests_layer_overlap_fitzpatrick.jl(40 s) andruntests_innerlayer.jl(18 s) intoerrorfields, where their SLAYER siblings already live, andruntests_grid_refinement.jl(30 s) intoapigives roughly 154 / 171 / 177 s, about 90 s off the longest group. JULIA_NUM_THREADS: 4on the 4-vCPU hosted runners instead of 2;OPENBLAS_NUM_THREADS: 1already prevents oversubscription. The threaded stages (ODE chunks, kinetic surfaces) are what the slow files run.JULIA_CPU_TARGET.genericis baseline x86-64 with no FMA or AVX in Julia-compiled kernels, which is both slower and numerically further from a workstation run. The multi-target string Julia's own binaries use (generic;sandybridge,-xsaveopt,clone_all;haswell,-rdrnd,base(1)) keeps images portable across hosted runners. Flagged as an experiment, becauseclone_alllengthens the image build; measure it after item 1 above, when the build is short.timeout-minutes: 90on the precompile job no longer matches reality; 30 catches a hung cache upload instead of letting it burn an hour of runner time.
Are the tests now insufficient anywhere?
Choices I accept as-is: kinetic smoke precision in the error-fields deck, coil replay on one set with the nominal sweep still covering all sets, four rendered PNGs with object checks on the rest, dropping the duplicate Solovev full run.
Three places I would change:
-
The removed pins are tracked only in a harness case this PR did not run. The DIII-D Riccati energy pin and the Δ′ diagonal pins now live in
diiid_n1_riccati(delta prime (BVP diagonal),total energy Re(et[1])), and the kinetic endpoint change inTorque.jlis only exercised by thesolovev_kinetic_*cases. The report in the body randiiid_n1alone. I havediiid_n1_riccati,solovev_kinetic_ntvqueued on feynman against currentorigin/developand will post the table here when it finishes; if it is clean, that plus the existing report is the evidence the removals need. -
Restore the sample counts in
runtests_monte_carlo.jl. That file now runs in 2.6 to 3.5 s including compile, so the 10× cut saved at most tens of seconds. What it cost: with 512 or 2000 samples andrtol=1e-2on a sampled mean, the tests pass because of the particular seed, not because of statistical margin, so a change in Julia's RNG stream between minors turns them red with no code change. Keep the cuts inruntests_error_fields.jl, where Monte Carlo is only exercised on the DIII-D run; put the synthetic distribution tests back to their original N (or a tier switch, item 4). -
One loose golden value per module on the fixtures that already run. The gap a paths-only contract leaves is a wrong number that still runs: a factor of two in a torque, a sign flip in Δ′, a dropped Jacobian. The harness catches those, but it is manual, author-run, and not on the queue. A single
isapproxwithrtol=1e-3per physics module on the small fixtures each file already builds (Solovev et[1], one NTV torque on the Solovev kinetic deck, one Δ′ diagonal on the TJ analytic deck, one|∇ψ|average) costs nothing at run time, is cheap to maintain because the tolerance is loose, and catches gross breakage in seconds. Tight pins stay in the harness; loose pins belong here.
Third-party human approval is still required before this merges.
|
Follow-up to my review: the two harness cases that track the removed pins and the kinetic endpoint change, run on feynman against this PR's actual merge base Note for whoever merges: |
|
Thanks for the detailed follow-up, Nik, and for running the two additional harness cases. The clean Riccati and Solovev kinetic comparisons close the remaining regression check. We tested disabling the precompile workload on GitHub. GPEC itself compiled in about 17 seconds, but first-use compilation pushed the warm test workflow to roughly 10 minutes. Restoring the workload brought it down to 6m02s, so we’ve kept it enabled for tests and disabled it for docs. Documentation passed in 3m55s and 5m40s across the two runs. We also applied your group rebalance and reduced the precompile timeout to 30 minutes. On Monte Carlo, I don’t think restoring all sample counts is warranted: the primary distribution comparisons still use 20,000/30,000 samples, while the 512-sample coherent-ring checks exercise fixed resultant magnitudes or cancellation. We’ve retained those counts. I’d keep the broader tiering, golden-value, matrix-policy, and architectural suggestions outside this PR. The intended split remains small fixtures with inexpensive correctness checks in CI, and production numerical tracking in the harness. The body now records the measurements, your harness results, and the disposition of the suggestions. All tests and docs pass. This seems like a reasonable stopping point for the overhaul; could you give it a final look and approve if you’re satisfied? |
This PR shortens the unit-test feedback loop, which had reached roughly 50 minutes on GitHub. The goal is to exercise code paths and interfaces with small fixtures, retain inexpensive correctness checks, and leave production-size numerical tracking to the regression harness. The target is an entire warm-cache test workflow under five minutes, including setup.
What changes
core,api, anderrorfieldsgroups in parallel for Julia 1.11 and the latest stable Julia. Each version precompiles once before releasing its test groups.ubuntu-slimfor conventions, change detection, and final status checks. Julia tests and documentation builds remain onubuntu-latest.TestsandDocumentationhandlemerge_group, queue candidates always run the full battery, and queue documentation builds skip deployment. PR conventions remain PR-specific.Current status
Merged develop
057c4af0dinto9fd6366without conflicts. The 54-file registry, Julia parsing, and diff hygiene checks pass locally; CI for this integration revision is pending. The PR-owned physics patch remains unchanged (stable patch ID39c86967d7210c14302178bd781a42063dfa2162). The harness stamp is refreshed under the requested report-reuse policy; actual run refs are preserved below, and the latest upstream physics changes have not been rerun through the harness by this session.Latest measured result at
8f572769a: all checks pass. The workload-enabled warm test workflow completed in 6m02s (13:07:41–13:13:43 UTC). Precompile jobs took 1m10s / 53s; the longest groups were Julia 1.11 ErrorFields at 4m26s and API at 4m24s, with core at 3m04s. Warm comparison. This improves on the earlier 7m05s warm measurement, but remains above the five-minute target; runner variability and upstream changes prevent treating these runs as a controlled benchmark.The workload-disabled experiment passed but took approximately 10 minutes warm: first-use compilation lengthened the groups. The solver workload has therefore been restored for tests. It remains disabled for documentation, whose two measured builds passed in 3m55s and 5m40s. Group balancing and registration of all 54 test files are retained. No physics implementation was changed by these optimization commits.
Earlier workload-enabled baseline and regression provenance follow.
Ready for human review. All CI checks passed at
49e67d41f, including the full 52-file battery on Julia 1.11.9 and 1.13.3, both production precompile jobs, documentation, and conventions. The develop merge's source-image rebuild made that run take 13m49s: precompile jobs took 8m17s and 5m55s. Logs confirm only GPEC was recompiled (423s / 304s); 488 / 489 dependency images were already compiled. Completed run, attempt 1.The unchanged-commit warm rerun passed in 7m05s (10:16:14–10:23:19 UTC). Precompile took 1m12s / 1m03s, confirming reuse of both dependency and GPEC images. The longest group was Julia 1.11 core at 5m18s; its test files consumed 4m16s, led by vacuum (76.5s), layer overlap (40.7s), equilibrium (33.9s), and grid refinement (30.9s). Warm run, attempt 2.
The previous warm-cache test workflow at
c63aafbaatook 5m02s, from 09:23:19 to 09:28:21 UTC on 2026-10-27, including setup, precompile, cache handoff, all test groups, and final status. Precompile jobs took 59s and 1m01s; the longest group took 3m45s. Sampled logs confirm compatible dependency-cache and exact GPEC-image hits, with the full environment artifact skipped. Measured run.The five-minute target is not consistently met: the earlier warm run was 5m02s, while the latest is 7m05s. Remaining work is primarily test execution/first-use compilation and group balance. Cold compilation, source changes, cache eviction, and runner queueing can still take longer; this is not a guarantee that every run finishes within five minutes.
Nik's cache-handoff, missing-registry-entry, stale-stamp, and stale draft-description findings have been addressed. The coverage choices below are explicit review items. Third-party human approval is required before merging; Nik's current review is a comment, not an approval.
Disposition of Nik’s second review
Nik explicitly identified the two harness cases as his only pre-merge ask; the remaining suggestions are recorded here as implemented, measured-and-rejected, or deferred. His GitHub review remains COMMENTED, so a human approval is still required.
Merge-queue rollout
This PR prepares the workflows; it does not enable the repository queue setting. After review and merge, enable the queue for
develop, retain the requiredTestsandDocumentationchecks and at least one human approval, use merge commits, and start with build concurrency 1. Keep "Only merge non-failing pull requests" enabled.Local checks verified that a queue event runs the complete test battery without PR metadata and cannot call
deploydocs; PR/push deployment behavior is preserved. The first real merge-group run must be verified when the queue is enabled. Setup guidance is indocs/development/git-workflow.md.Coverage choices for reviewers
Release note
diiid_n1comparison reports 53 unchanged quantities. (harness @ cb5c8fb)Unit tests use smaller fixtures and shared compilation to shorten feedback while retaining interface and execution-path checks. Production-size numerical regression is reported separately.
Regression report
Additional reviewer-run comparisons: Nik’s full reports show 17 Riccati quantities and 6 Solovev kinetic quantities unchanged, all reported differences zero, comparing
6d655c74cwith4ed1f1fd2on feynman, Julia 1.11.7, pinned manifest, 4 Julia / 4 BLAS threads. These are reviewer-provided results, not a new run by this session. The stamp is now refreshed to9fd6366under the requested report-reuse policy: the PR-owned source patch is identical to the reviewer-tested4ed1f1fd2patch (stable patch ID39c86967d7210c14302178bd781a42063dfa2162). The newly merged upstream changes have not been independently rerun through the harness at this integration revision.53 unchanged, comparing develop
3456e6365withfa1b61ae2through the SDCC fanout runner onsirius. Both refs used Julia 1.12.6, the same pinned manifest, two Julia threads and one BLAS thread; prep, both comparison tasks, and report exited successfully.The actual comparison ran at
fa1b61ae2against3456e6365. The original report stamp was refreshed to49e67d41funder the requested report-reuse policy: the PR's ownsrc/patch relative to its merged develop parent is unchanged (verified stable patch ID15b55b40f2307b5adfe516006c49b20dc6fdc676). Develop adds transport-model changes in the new merge, so the full source trees are not identical. This latest integration revision has not been rerun through the harness; the report below retains its original run refs.SDCC report:
/home/ITER/pharrm/Projects/GPEC_dev/regress-fanout/runs/pr593-fa1b61ae2-20261027/report.txt.Full SDCC regression report
Notes for reviewers
Test orchestration uses optimization level 0, test processes use level 1, and production package images remain at level 2. Bounds checks stay enabled in CI. The registry rejects omitted or duplicated test files. The periodic-endpoint closure is the only physics-source change relative to the merged develop parent.