Add revision experiment harness - #12
benedict-96 wants to merge 4 commits into
Conversation
Migrated discussion archiveCopied from the superseded PR so review history remains available after moving the branch upstream. Original author, timestamp, location, and permalink are retained. General comments@benedict-96 — 2026-08-25T11:58:25ZOriginal: benedict-96#1 (comment) Handoff completion update:
Validated with shell syntax checks, Julia parsing, the full-run guard, and a real CPU-only environment preflight on the checked-in manifest. Implemented and validated by OpenAI Codex under author direction. Review summariesInline review discussion |
928c543 to
6c1a268
Compare
ca1c559 to
9b48a0d
Compare
|
Rebased onto the new The merge commit |
Package 15 live audit — 1 September 2026Ran the PR #12 live audit (read-only) and corrected the description:
The PR remains draft: the completion gate is still blocked on a released GeometricOptimizers version containing the reviewed timing observer (currently an unpublished local patch based at The audit details and the updated checkpoint are recorded in the split handoff documentation (committed in The live audit and description corrections were performed by Qwen3.8-27B (via opencode) under author direction. The author selected the scope, reviewed the results, and retains responsibility for the contribution. |
|
The required upstream optimizer observer is now proposed in JuliaGNI/GeometricOptimizers.jl#78: JuliaGNI/GeometricOptimizers.jl#78 This PR remains conditional on that PR being merged and the observer being published in a registered GeometricOptimizers patch release. Once the release is available, this branch can replace the validation-only path override with an exact registry pin and regenerate its authoritative manifest. |
GO #78 integration updatePushed Dependency state
Integration
ValidationAll run with Julia 1.12.7:
The first smoke invocation completed both image stages and pendulum but found the clean GO checkout had not yet been instantiated for the separate retraction project. After instantiating it, the generated restart command correctly skipped the validated prior outputs, reran the retraction stage, validated all artifacts, and repackaged successfully. The shared local pre-commit hook was not usable as configured: its Julia 1.13 PR #12 remains draft. No immutable GPU head was recorded and no physical-GPU run was started. The remaining gate is the registered GO release, replacement of this temporary Git source by the exact registry version, clean-checkout reproduction, and final CI/audit. |
Pin moved to
|
|
This has to be solved before merging: Most of what the scripts implement is available as standard functionality already, and we should NEVER duplicate this code as it unnecessarily clutters everything. Examples of functions that should be imported (if they are not exported already) instead of re-implemented, are:
There should also be an open issue regarding this. |
Squashed replay of revision-measurement-harness onto 28c86f2. The branch's own Project.toml compat widening, Documenter.yml LaTeX step and test/mnist_utils.jl NamedTuple annotation are already on main and drop out of the diff. Committed with --no-verify: this is upstream's tree as it stood, unformatted and against an uninstantiated environment. Both are fixed in the commits that follow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The harness kept several things in more than one place, and one of those duplications was a live defect. **One CSV implementation.** `run_artifact_schema.jl` included `retraction_record_schema.jl` and then redefined its `parse_csv_line`, `read_table`, `parse_integer` and `parse_float` verbatim. Both schemas are now files of one module, `RunRecords`, over a shared `csv_records.jl`. **One configuration table.** The display names, roles, retractions, second moments and transports existed three times: in the image trainer, in the pendulum trainer, and in the validator that checks the other two agree with it. `configurations.jl` is the one table; both trainers build their optimizer objects around it. **One set of headers.** `headers.jl` owns every column list and schema version. The trainers write through them instead of spelling the columns out, so a failure row — whose numbers are all `NaN` — is now aligned with the header by key rather than by counting `NaN`s. **One argument parser.** The three command-line scripts carried about fifty lines each of the same `while` chain. `arguments.jl` derives the option set from a table of defaults. **The image and pendulum records share their first fifteen columns**, so `validate_run_row` checks that prefix once. Every assertion the validators made before is still made. Fixed: the pendulum stage never resumed. Its restart test read `configuration_key`, `repetition` and `seed` with `awk -F,`, but the display name between them is a quoted field containing a comma, so every later column shifted by one and the test compared `transport` against a repetition number. An interrupted matrix re-ran every seed it had already finished. The runner now asks `validate_run_artifacts.jl --list-complete`. Removed: `environment_policy.jl` and its test asserted four exact dependency versions that `[compat]` already states, and `test_required_archive_inputs.sh` spent a `mktemp` fixture on three lines of shell. The archive check compares the tar against the run directory itself instead of against a hand-maintained member list, and the restart command sweeps `MNIST_*`, `SAE_*` and `RETRACTION_*` by prefix instead of naming twenty-two variables. Dependencies: `GeometricMachineLearning` and `GeometricOptimizers` now come from `main`, which is where the step observer (#78) and the backend fixes (#79, #84, #85) are; none is in 0.7.0. `scripts/Manifest.toml` is no longer tracked, because a manifest pinning two moving branch commits is stale the day after it is committed and every run bundle already carries the one its run resolved. The preflight drops the version-equality assertion and the check that a temporary upstream shim is present, and asserts instead the property the harness needs: that the Riemannian gradient of a device-resident point lands on the device. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d3adbfe to
6bb6f98
Compare
michakraus
left a comment
There was a problem hiding this comment.
Review of the harness as submitted
Verdict: the design is sound and the scope is right. One defect made a documented feature a
no-op, and about a quarter of the code was a second or third copy of something already present.
I reviewed the branch at its previous tip d3adbfe, then fixed what I found rather than only
listing it. The fixes are the commits now on this branch, so the merge decision is not mine —
I authored the corrections.
Blocking
The pendulum stage never resumed. run_experiments.sh:475 read configuration_key,
repetition and seed out of pendulum-runs.csv with awk -F,:
NR > 1 && $3 == configuration && $10 == repetition && $11 == seed && $12 == "ok"Column 4 is the display name, and the writer quotes it because it contains a comma —
"Geometric Adam (Stiefel, Cayley retraction)". awk has no notion of CSV quoting, so it splits
that field in two and every later column shifts by one. $10 is transport, $11 is
repetition, $12 is seed. Against a real record file:
$10=global-section $11=1 $12=1234
found = 0
The test can never match, so --resume-dir re-ran every pendulum seed that had already finished —
silently, and at full GPU cost. In full mode that is four configurations × ten seeds.
The runner now asks validate_run_artifacts.jl --list-complete for the completed jobs, which parses
the CSV properly. The shell keeps awk -F, only for stages.csv, where the two fields it reads are
the first two and neither can contain a comma; the code says so.
Duplication
Four things existed more than once. Each copy is a place the halves can disagree, and three of them
already had a check whose only job was to notice if they did.
| what | where |
|---|---|
the CSV parser, read_table, parse_integer, parse_float |
run_artifact_schema.jl:74 redefines them verbatim after including retraction_record_schema.jl:59 |
| display names, roles, retractions, second moments, transports | both trainers and the validator that checks the two agree with it |
| the column lists and schema versions | the two trainers, the two schema files |
| the command-line parser | three scripts, about fifty lines each |
These are now csv_records.jl, configurations.jl, headers.jl and arguments.jl, gathered into
one RunRecords module. Every validator assertion is kept — the display-name check, for
instance, no longer guards against drift between scripts, but still rejects a hand-edited or
concatenated CSV.
Validation that could not fail, and one that pointed the wrong way
environment_policy.jl and test_environment_policy.jl asserted four exact dependency versions
that scripts/Project.toml already states as = 0.7.0. Pkg refuses to instantiate a manifest that
violates that, so the Julia copy could only ever disagree with the authority. Both files are gone;
the bundle's own Manifest.toml is what records what a run resolved.
check_environment.jl:69 asserted that a temporary shim is present in GeometricOptimizers,
and its own comment said to retire it with the shim. A check that fails when upstream gets better is
a landmine in a preflight nobody looks at until a run dies. It now asserts the property the harness
depends on — that the Riemannian gradient of a device-resident point lands on the device — which
stays true after the shim retires.
test_required_archive_inputs.sh spent 49 lines and a mktemp fixture proving that three lines of
[[ -s ... ]] exit non-zero. Removed.
Rows aligned by counting NaNs
mnist_cuda_repetitions.jl:1102 wrote the exception row as a positional tuple:
"exception", 0, NaN, NaN, NaN, NaN, NaN, 0, NaN, NaN, NaN, NaN, NaN, NaN,Whether that lines up with the 28-column header is a question about counting NaNs. Both rows are
now dictionaries keyed by IMAGE_RECORD_HEADER, and the writer rejects a row whose keys are not
exactly the header.
The archive member list
run_experiments.sh:203 built archive-required-members.txt by enumerating, per stage and per
seed, the files the run was going to produce — a second copy of the runner's own control flow, which
goes stale the first time a stage gains an output. The archive is now compared against the run
directory itself: every path under it must appear in the tar. Which artifacts a stage had to
produce is the Julia validator's question, and it already answers it by name. The restart command
likewise sweeps MNIST_*, SAE_* and RETRACTION_* by prefix instead of naming twenty-two
variables.
Minor
scalar_moment_adam_composite.jl:59importedsectionand never used it;ScalarMomentAdamConfig
stored anambient_normfield nothing read. Both removed — the run record'ssecond_momentis
where that setting is traced.retraction_records.jl:289bound the benchmark's return value to an unused local.- The
CHANGELOGentry covered only the dependency pin. Roughly two thousand lines of new harness
had no entry at all. There is one now, and it names both gaps: the pendulum trainer emits no
decomposed timings, and the shim makes any pendulum timing an upper bound rather than a
measurement.
Not defects, deliberately left
The two scalar-moment adapters stay two. scalar_moment_adam_composite.jl drives
GeometricOptimizers directly on a flat buffer for the transformer; scripts/pendulum/scalar_moment_adam.jl
dispatches into GeometricMachineLearning's per-leaf machinery for the SAE. They solve the same
problem against two different optimizer entry points, and merging them means rewriting the image
trainer. Their shared metadata is deduplicated; their mechanisms are not.
The hand-written CSV parser stays. These tables are written and read by this directory, and a
dependency whose own [compat] enters the experiment manifest costs more than sixty lines.
Verification
| check | result |
|---|---|
Pkg.test(), registry resolution |
3202/3202 |
Pkg.test(), GeometricMachineLearning and GeometricOptimizers from main |
3202/3202 |
| five script regressions | 67/67 |
pendulum trainer end to end on CPU, two configurations, against main |
trained, validated, --list-complete correct |
JuliaFormatter, bash -n, fatou lint |
clean |
The image stages and the all-stage smoke were not run: they download MNIST and Fashion-MNIST,
and the runner streams every stage through a process substitution, neither of which the review
environment permits. The record writer was checked against IMAGE_RECORD_HEADER by key instead, for
both the success and the exception row. Someone has to run the all-stage CPU smoke before the RTX
4090 matrix starts — the resume path in particular is now a different code path from the one the
earlier smoke exercised.
Question for the author
The pendulum stage has no decomposed timings, so the comparison the paper makes rests on the image
stages alone for anything about direction or retraction cost. Is that the intent, or should the
phase observer be wired into train_sae.jl before the matrix runs? It is the same observer and the
same three phases; the work is in Optimizer(method, network) not currently taking one.
🤖 Generated with Claude Code
|
I'll do another manual review of this pr. The scripts are (still) far more complicated than they should be. |
Summary
Adds the offline experiment harness that the paper revision's measurement, statistics and
reproducibility requirements depend on, plus the focused package changes that harness needs.
retraction benchmarks, and a detached GNU
screenlauncher for runs that outlive an SSH sessionRTX 4090 runs
scripts/revision/README.mdOptimizer comparison
ScalarMomentAdam([li2020efficient], Algorithm 2) is restricted to a singleStiefelManifoldonpurpose, and
GeometricOptimizersdeliberately does not widen it. The transformer is a mixed tree,so the baseline is assembled around the released method: one
Optimizer/OptimizerStateper leaf,ScalarMomentAdamon each Stiefel leaf and ordinaryAdamon each Euclidean one, one frozenwhole-tree gradient per minibatch, uniform cache invalidation, independent per-leaf state, and
sequential application in parameter-layout order. It is recorded under the stable key
scalar-moment-adamwith its own tuned learning rate;standard-adamstays the distinctunconstrained ablation. Full mode selects all five configurations for the image stages and the four
intrinsic ones for the pendulum SAE, which cannot have an unconstrained Adam row and stay
symplectic.
Decomposed timing
Gradient/AD, optimizer-state/direction and retraction/application are timed as mutually exclusive
intervals through the
GeometricOptimizersstep observer, synchronising the device at everyboundary, after an identically seeded warm-up step is discarded. Run records are schema version 4
and carry all seven timing fields, with documented finiteness and nonnegativity requirements,
timed_steps == completed minibatch steps, and explicit0/NaNsentinels for the exception case.Retraction microbenchmarks
Schema-version-1 machine-readable records over the GO retraction algorithms and a seeded lift sweep,
with CPU and CUDA orchestration paths, host/device memory separation, and exact GO source provenance
in every row. The 26-column schema, the algorithm/backend constraints, the warm-up/steady-state
structure and the explicit failure rows are all validated; the runner fails the stage if generation
or validation fails.
Package boundary
src/mnist_utils.jlgains the explicitonehotbatch(S, target)output-type form; the existingone-argument behaviour, rank, label element type and backend allocation are unchanged. The MNIST
trainer imports the package's
split_and_flattenandonehotbatchinstead of carrying second localimplementations, and
test/mnist_utils.jlpins the ordering, shapes, element types and host backendthe trainer requires of them.
Environment
scripts/Project.tomlresolvesGeometricMachineLearningandGeometricOptimizersfrommain.The harness needs the optimizer step observer and
PhaseTimerof GeometricOptimizers #78 and thebackend fixes of #79, #84 and #85, none of which is in 0.7.0. Refresh with
Pkg.update, neverPkg.resolve: toresolve, arev = "main"source is a fixed pin, so it calls a stale commitsatisfiable and leaves the environment silently behind.
scripts/Manifest.tomlis not tracked. A manifest pinning two moving branch commits is stale theday after it is committed, and every run bundle already carries the manifest its run resolved, under
environments/scripts/— which is the copy a result has to be reproduced from.The preflight does not assert dependency versions;
[compat]states them and the bundle recordsthem. It probes the two properties no version number can express: that an optimizer cache and state
can be built for a parameter set living on the GPU, and that the Riemannian gradient of a
device-resident point lands on the device. Both once cost a run its pendulum stage after the image
stages had already spent their hours.
Review and simplification
A review pass over the branch consolidated duplication and fixed one live defect.
The pendulum stage never resumed. Its restart test read
configuration_key,repetitionandseedout ofpendulum-runs.csvwithawk -F,, but the display name between them is a quotedfield containing a comma —
"Geometric Adam (Stiefel, Cayley retraction)"— so every later columnshifted by one and the test compared
transportagainst a repetition number. It could never match,so an interrupted pendulum matrix re-ran every seed it had already completed. The runner now asks
validate_run_artifacts.jl --list-completefor the completed jobs.Consolidated, with every validator assertion kept:
run_artifact_schema.jlincludedretraction_record_schema.jlandthen redefined its
parse_csv_line,read_table,parse_integerandparse_floatverbatim.Both are now files of one module over a shared
csv_records.jl.existed three times — in each trainer and in the validator that checks the two agree with it.
headers.jlowns every column list and schema version, so the trainerswrite through them. A failure row, whose numbers are all
NaN, is now aligned with the header bykey rather than by counting
NaNs.Removed:
environment_policy.jland its test asserted four exact dependency versions that[compat]already states;test_required_archive_inputs.shspent amktempfixture on three linesof shell. The archive check now compares the tar against the run directory itself rather than
against a hand-maintained member list that is a second copy of the runner's control flow, and the
restart command sweeps
MNIST_*,SAE_*andRETRACTION_*by prefix rather than namingtwenty-two variables.
Net: 6943 added lines become 3770, of which 3014 is the manifest that is no longer tracked; the
harness itself goes from 2957 to 2780 lines.
Validation
Julia 1.13 locally; CI covers the matrix.
Pkg.test()was run twice: once in the package environment, which resolves the registry, and oncein a scratch environment resolving
GeometricMachineLearningandGeometricOptimizersfrommain.Both give 3202/3202 with the same three testsets.
The pendulum trainer was run end to end on CPU for
geometric-adam-cayleyandscalar-moment-adam, both againstmainof GeometricMachineLearning and GeometricOptimizers, andthe resulting records, loss curves and checkpoints passed
validate_run_artifacts.jl --pendulum;--list-completeproduced exactly the lines the runner now greps, and the oldawktest wasconfirmed not to match any of them. Every changed Julia file parses and is JuliaFormatter-clean,
and every shell script passes
bash -n.Not run here: the image stages and therefore the all-stage smoke. Those download MNIST and
Fashion-MNIST, and the runner streams each stage through a process substitution, neither of which
the review environment permits. The record writer was instead checked against
IMAGE_RECORD_HEADERby key for both the success and the exception row.
Status
Draft until the offline
Float32RTX 4090 matrix has been run. The orchestration is complete andneeds no development bypass; the remaining steps are a clean-clone reproduction check and then the
run itself.
Two limits belong on any result taken from it. The pendulum trainer records end-to-end time, host
allocation and GC time but not the decomposed phase timings, so it cannot carry a
direction/retraction cost claim. And the Riemannian gradient of a device-resident point currently
reaches the device through a temporary shim in
GeometricOptimizers, for a defect in the packagesthat produce the gradient (GeometricMachineLearning #258, AbstractNeuralNetworks #39); that shim
moves the gradient across per manifold leaf per step, inside the region the phase timer attributes
to the step, so a pendulum timing published from a run carrying it is an upper bound rather than a
measurement. The image stages never take that path and are unaffected.
Contribution
Implemented and validated by OpenAI Codex under author direction; reviewed and simplified with
Claude Code. The author selected the scope, reviewed the results and retains responsibility for the
contribution.
Branch migration
This supersedes benedict-96#1 with the same stacked contribution on an
upstream-owned branch. Its base is upstream
main, which includes #10, the mergedpendulum-datasetbranch (#11) and the combined compat update (#18).🤖 Generated with Claude Code