diff --git a/docs/seed_variance_51_135.md b/docs/seed_variance_51_135.md new file mode 100644 index 00000000..e67d6c60 --- /dev/null +++ b/docs/seed_variance_51_135.md @@ -0,0 +1,284 @@ +# Seed variance: the number both #51 and #135 are now blocked on + +**Status: PRE-REGISTERED 2026-09-03, before any replicate finished. The reading rules +below were written first, so the interpretation cannot be chosen after seeing the +numbers — the same discipline as the #71 checkpoint-selection protocol and Tillicum +measurement 3.** + +**Amended 2026-09-04 in code review, before any replicate was scored** — see +[Amendment 1](#amendment-1-2026-09-04). The original rule divided 0.039 by Campaign A's +SD alone, and gave Campaign B no reading at all. Both are corrected below; the original +text is kept verbatim so the amendment is auditable rather than a silent rewrite. + +## Where the inputs live + +**`docs/operating_point_parity_51.md` is not on `main` yet.** It, the script that +produces the pre-registered statistic (`scripts/analysis/operating_point_parity_51.py`) +and its artifact (`docs/data/operating_point_parity_51.json`) are all on branch +`fix/yolo-label-cache-rescue-51`, which is open as PR #154 against `main` and is **not** +a parent of this branch. So if this document merges first, `main` carries a +pre-registration whose headline 0.039 and whose scoring tool cannot be found from a +clean clone. Every link below marked † resolves only once #154 lands. Stated here rather +than left to be discovered. + +## Why this is the binding number + +Two independent lines of work arrived at the same wall from opposite sides. + +**#51.** [`operating_point_parity_51.md`](operating_point_parity_51.md)† found that the +published RampNet-vs-YOLO gap was mostly an operating-point artifact: at matched +operating points the residual is **0.039 F1**, not 0.252 or 0.160. Every arm in that +comparison is **one seed** — `seed: 0`, the Ultralytics default, in all eight +`args.yaml` files. #51's own rule is that differences under ~0.02 should not be read, +and 0.039 is close enough to that floor that the architecture claim cannot be +adjudicated at n=1. + +**#135.** The power analysis measured a paired MDE of 0.0063 on `manual_gold` and then +said plainly that the binding limit is **unmeasured seed variance, n=1** — Stage 2's +`train.py` hardcoded `torch.manual_seed(42)` with no flag. + +So the same missing measurement gates both. Everything downstream inherits it: with no +noise floor, no future RampNet 2.0 improvement can be called real either. + +## What is being run + +Two campaigns, launched 2026-09-03. + +### Campaign A — YOLO seed variance (#51), on Tillicum + +Three fresh replicates of **`y11x_tiles`**, the strongest YOLO leg and the one carrying +the 0.039. Config identical to the pre-registered #51 protocol and to the existing arm's +`args.yaml` in every respect except the seed: + +| | value | source | +|---|---|---| +| base | `yolo11x.pt` | `runs/y11x_tiles/args.yaml` | +| data | tiles, 557,413 train / 161,002 val | verified on Tillicum, exact match to the klone record | +| `imgsz` / `batch` | 1024 / 12 | as-run | +| `epochs` / `patience` | 60 / 20 | as-run — see "read at a matched epoch" below | +| `optimizer` | `auto` (resolves to `MuSGD`) | as-run | +| `seed` | **1, 2, 3** | the only variable | +| `save_period` | **1** | deviation, stated below | + +**Why Tillicum and not free klone.** The tiles arm is storage-bound on klone — it +consumes 8.5 MB/s against a filesystem measured at 8.3–11.8 MB/s, i.e. it sits *on* the +ceiling ([`tillicum.md`](tillicum.md)). Three concurrent replicates there would contend +for the wall itself and each would run slower than the 7.06–7.38 h/epoch a single arm +saw. On Tillicum the same arm uses 4% of available bandwidth and is genuinely GPU-bound +at 3.0 h/epoch, so three replicates run concurrently without interfering. + +**The one deliberate deviation: `save_period=1`.** The #51 arms ran `save_period: -1`, +which is exactly why the epoch-curve follow-up had to be retracted — no per-epoch +weights exist for any arm and they cannot be recovered. Keeping every epoch costs ~150 +MB × 44 × 3 ≈ 20 GB against a 1 TB allocation and buys back the budget analysis that is +currently foreclosed. It does not affect training. + +### Campaign B — RampNet seed variance (#135), on klone + +Three replicates of the committed Stage 2 recipe (1 epoch / 9,378 steps, constant lr +1e-5, global batch 16, selection on auto-label val loss) at seeds **1, 2, 3**, via +`stage_two/run_train_seed.slurm`. Free, preemptable, resumed by `--requeue` plus +`train.py`'s own `latest_checkpoint.pth`. + +**These replicates are not compared against the released checkpoint.** The released +model is a hand-copied epoch-1 checkpoint chosen by neither the paper's rule nor #84's +(see [`stage2_epoch_curve_84.md`](stage2_epoch_curve_84.md)), so its provenance differs +from a clean run of the committed recipe. The SD is computed over the three replicates +alone. + +## The reading, fixed in advance + +The statistic for both campaigns is the **macro-mean F1 over the seven pooled US +splits**, each replicate read at its own uniform threshold selected on the `sao_paulo` +dev split — i.e. exactly the parity protocol, applied per replicate. Selection never +touches a reported split. + +**Read at a matched epoch.** The existing `y11x_tiles` arm's `best.pt` is from ~ep44, +the point it reached before it ran out of free GPU. Each replicate is therefore read at +its **best-val epoch among epochs ≤ 44**, which is what "best.pt as-saved" meant for +that arm. "Best-val" is **`metrics/mAP50-95(B)` from the run's own `results.csv`** — this +Ultralytics build selects on mAP50-95 alone, not the 0.1/0.9 fitness blend, measured in +[`tillicum.md`](tillicum.md) (the retarget dry run reproduced the klone arm's ep21 +`best_fitness` to five decimals). Naming the column matters: the blend and mAP50-95 do +not always peak at the same epoch. + +Replicates are configured `epochs=60` so the LR curve is identical — a 44-epoch schedule +is *not* the first 44 epochs of a 60-epoch one, and truncating the schedule instead of +the run would have confounded the comparison. **They will not actually reach epoch 60**: +`CHAIN=5` buys six 24 h slices = 144 GPU-h, which at 3.0 h/epoch is about 48 epochs +before per-slice restart overhead. That is deliberate and costs nothing — the read is at +≤ 44 — but the runs stop around ep48, not ep60, and the LR curve they follow up to that +point is the 60-epoch one, which is the whole requirement. + +**Decision rule for #51**, on the sample SD `s` of the three Campaign A replicates. +*(Kept verbatim as the pre-registration of record. The σ it divides by is corrected, and +its bands made disjoint, in [Amendment 1](#amendment-1-2026-09-04) — apply that version.)* + +| `s` | reading | +|---|---| +| **≤ 0.010** | 0.039 is ≈4σ. The architecture advantage is small but real; #51 closes with the gap restated at 0.039 ± the measured spread. | +| **≥ 0.020** | 0.039 is inside 2σ. #51 closes with **"a supervised YOLO baseline is statistically indistinguishable from RampNet at matched operating points"** — and the `manual_gold` loss stands as the sharper statement. | +| **0.010–0.020** | Ambiguous. Report the interval, claim neither, and combine with Campaign B's SD for a two-sample test rather than asserting from A alone. | + +**This can make our own headline smaller, and that outcome is accepted in advance.** +The ≥0.020 branch is a live possibility, not a formality. + +## Amendment 1 (2026-09-04) + +**Ratified by Jon Froehlich, 2026-09-04, before any replicate was scored.** Raised in code review rather than by looking at a result: the correction below is arithmetic, the cut points 0.010 and 0.020 are unchanged from the original table, and Campaign B's rule was written with no number from either campaign in hand. + +Raised in code review of PR #155, **before any replicate had finished** — the three +klone replicates had not started (their launcher died at submit time on 2026-09-03) and +the Tillicum replicates were mid-schedule with no epoch scored. No number from either +campaign had been looked at when this was written. The table above is left in place +because it is the pre-registration of record; this section says how it is applied. + +### A1.1 The σ is the SD of the *gap*, not of Campaign A alone + +The table divides 0.039 by `s`, the SD over Campaign A's three replicates. But 0.039 is +a **difference between two single-seed runs** — RampNet 0.843 minus `y11x_tiles` 0.804 +(the parity table†). The standard error of a difference of two independent single draws +is + + s_gap = sqrt(s_A² + s_B²) + +not `s_A`. Using `s_A` alone assumes the RampNet side is noiseless at n=1, which is the +assumption #135 explicitly said was unjustified — and is the reason Campaign B is being +run at all. It is also anti-conservative in exactly the direction that flatters us: at +`s_A = 0.010` the table reads "≈4σ", but with `s_B = 0.010` the gap is 2.8σ, with +`s_B = 0.015` it is 2.2σ, and with `s_B = 0.020` it is 1.7σ — inside the band the table +itself calls indistinguishable. The published conclusion could invert with no change to +any input the original rule looked at. + +**So the bands are applied to `s_gap`, with the same cut points, and made disjoint:** + +| `s_gap` | reading | +|---|---| +| `s_gap < 0.010` | 0.039 is ≳4σ. The architecture advantage is small but real; #51 closes with the gap restated at 0.039 ± the measured spread. | +| `0.010 ≤ s_gap < 0.020` | Ambiguous. Report the interval and the two component SDs, and claim neither. A fourth and fifth Campaign A replicate are the pre-registered response. | +| `s_gap ≥ 0.020` | 0.039 is inside 2σ. #51 closes with **"a supervised YOLO baseline is statistically indistinguishable from RampNet at matched operating points"** — and the `manual_gold` loss stands as the sharper statement. | + +The endpoints now belong to exactly one row; in the original all three rows contained +0.010 and 0.020. + +**If Campaign B does not deliver `s_B` in time** — its calendar is unbounded, see the +limitations — the table is applied to `s_A` alone and the result is reported as an +**upper bound on significance / lower bound on σ**, in those words, never as the +finding. It is not permitted to quietly become the finding because B was slow. + +### A1.2 Campaign B's own reading + +The original document gave Campaign B no decision rule, which left half the campaign +free to be interpreted after the fact. Fixed here. + +Campaign B's statistic is `s_B`, the sample SD of the macro-mean US7 F1 over its three +replicates, read the same way as Campaign A's. It is read against **#135's measured +paired MDE of 0.0063** on `manual_gold`: + +| `s_B` | reading | +|---|---| +| `s_B < 0.0063` | Seed variance is smaller than the paired epoch-to-epoch MDE. #135's MDE stands as the binding limit on Run-A-style comparisons, and #135 closes on that. | +| `s_B ≥ 0.0063` | Seed variance dominates the paired MDE. **Every unpaired single-seed comparison in this repo, including #84's epoch curve read across runs, is limited by `s_B`, not by the MDE** — #135 closes with the noise floor restated at `s_B` and the MDE demoted to the paired case only. | + +Either way `s_B` feeds `s_gap` above, and either way it is recorded here with its three +per-replicate numbers, so a fourth replicate can be added to the same sample later. + +`s_B` is an **upper** bound on seed effect alone, for the reason already in the +limitations: klone `ckpt-all` preempts, and a requeued replicate resumes from +`latest_checkpoint.pth` without restoring the augmentation RNG stream, so requeue +boundaries contribute to the spread. If the campaign lands in a band whose edge is +within that uncertainty, say so rather than picking the side. + +### A1.3 How each campaign gets scored + +Neither campaign's path from artifact to statistic was written down. It is not a +one-liner and the gaps are real: + +**Campaign A.** For each replicate: pick the epoch checkpoint with the highest +`metrics/mAP50-95(B)` at epoch ≤ 44 from `runs//results.csv`, run the YOLO +detector over the eight splits (US7 + `sao_paulo`) at the 0.05 score floor, and write a +sweep file in the format of `docs/data/yolo_geometry_51/*.txt`. Then +`scripts/analysis/operating_point_parity_51.py`† must be pointed at the three new legs +— **its leg list is hardcoded**, so this needs a code change, not just new inputs. That +change is not in this PR. + +**Campaign B.** For each replicate's `best_model.pth`: regenerate `analysis_out/op_cache/` +over the same eight splits at the 0.05 floor (the committed op_cache is the *published* +checkpoint's and cannot be reused), then run the same parity script with the replicate +in place of RampNet. Also not in this PR. + +Until both exist as committed, argument-configured scripts, this campaign is not +replicable from a clean clone. That is a stated gap, not an oversight. + +## Stated limitations + +- **n=3 gives a wide interval on the SD itself** — with 2 degrees of freedom the 95% CI + on σ spans roughly 0.5σ̂ to 3.7σ̂. A fourth and fifth replicate are the pre-registered + response if the result lands in the ambiguous band. They are not being run up front. + Cost per extra replicate at $0.90/GPU-hour and 3.0 h/epoch: **~$130** as launched + (`CHAIN=5` caps the run at 144 GPU-h ≈ ep48), or ~$119 if stopped at ep44, the last + epoch the reading uses. An earlier draft quoted $119 while also saying the replicates + run all 60 epochs; those two are inconsistent — 60 epochs would be 180 GPU-h ≈ $162, + and the chain does not buy that much wall-clock. + Extend the campaign with seeds **4, 5, …, never 0**: `sampler_seed_for(0)` collides + with the published run's data order (`rampnet/seeding.py`). +- **Campaign A's replicates are Tillicum H200; the existing `seed: 0` arm is klone + L40S.** The SD is computed over the three same-hardware replicates only. The seed-0 + arm is a separate cross-hardware check, not a fourth sample — mixing them would + conflate seed with kernel and hardware differences. +- **Only `y11x_tiles`.** `y11l_*`, `y26_*` and the pano arms are not replicated; the SD + measured here is not automatically theirs. +- **Campaign B's calendar is unbounded.** `ckpt-all`'s duty cycle was 3.9% in 2026-08 + (#135), so a replicate can sit pending for days. It costs nothing, but it may not + finish alongside Campaign A. If it stalls, the fallback is Tillicum at ~$50/replicate; + that would be recorded here as a venue change, with the hardware caveat above. +- **Campaign B's artifact lands on a volume that purges, and the two facts interact.** + `train.py` writes `best_model.pth` to the working directory, so it lands in `RUNDIR`, + which defaults under `/gscratch/scrubbed` — purged on a ~21-day idle window + ([`stage2_epoch_curve_84.md`](stage2_epoch_curve_84.md); #84 put Run A's checkpoints + on the purchased `/gscratch/makelab` for exactly this reason). Combined with the + unbounded calendar above, a replicate can finish and then age out while its siblings + are still pending. **The copy-out in "Reproducing" is part of the protocol, not + housekeeping.** The default is left on scrubbed so it matches the replicates already + queued; moving it is a decision for a later campaign, not a mid-flight edit. +- **Seed variance is not the same as training-run variance.** Preemption, requeue + boundaries and non-deterministic kernels also move a result. Ultralytics sets + `deterministic=True` by default, which removes some of that for Campaign A; Campaign B + has no such guarantee and its spread is therefore an upper bound on seed effect alone. + +## Reproducing + +Both launchers take the seed as an environment variable and record it in the job log. + +```bash +# Campaign A, per replicate (Tillicum). CHAIN covers the 24 h normal-QoS ceiling. +# PYTHON is REQUIRED: the launcher defaults to `python`, which on Tillicum has no +# ultralytics -- our environment.yml pins CUDA 11.8 and does not transfer to Rocky 9 / +# H200. This is the interpreter docs/tillicum.md's as-run invocation uses. +cd ~/RampNet && mkdir -p logs +PYTHON=/gpfs/projects/makelab/$USER/envs/rampnet-yolo/bin/python \ +SEED=1 NAME=y11x_tiles_s1 YOLO_CKPT=yolo11x.pt \ + YOLO_DATA=/gpfs/scrubbed/$USER/yolo/tiles/data.yaml \ + YOLO_IMGSZ=1024 BATCH=12 EPOCHS=60 SAVE_PERIOD=1 CHAIN=5 \ + sbatch scripts/model_comparison/run_yolo_train_tillicum.slurm + +# Campaign B, per replicate (klone). logs/ must exist BEFORE submit -- Slurm opens +# --output against the submit directory, so a missing logs/ fails the job at start. +cd ~/RampNet && mkdir -p logs +SEED=1 sbatch stage_two/run_train_seed.slurm + +# ... and copy Campaign B's artifact off scrubbed as soon as a replicate completes. +# RUNDIR is /gscratch/scrubbed/$USER/seedvar/rampnet_s, which purges on a ~21-day +# idle window; /gscratch/makelab is purchased and never purged. +cp /gscratch/scrubbed/$USER/seedvar/rampnet_s1/best_model.pth \ + /gscratch/makelab/$USER/seedvar/rampnet_s1_best.pth +``` + +The seed plumbing itself is unit-tested in `tests/test_seeding.py` — including that the +default preserves the published pairing (`sampler_seed_for(42) == 0`; same seeds and +same data order, not bit-identical, since cuDNN autotuning and AMP are not seeded), that +the `DistributedSampler` actually receives that seed (asserted on the parsed statement, +because the same expression appears in a log line and a substring check passed after the +kwarg was deleted), and that the Tillicum launcher's positional argument list still lines +up with its unpack — an off-by-one that would silently shift `data` into `imgsz` and +train the whole arm at the wrong settings. diff --git a/rampnet/seeding.py b/rampnet/seeding.py new file mode 100644 index 00000000..913c9e38 --- /dev/null +++ b/rampnet/seeding.py @@ -0,0 +1,51 @@ +"""Seed bookkeeping for Stage 2 training. + +Stage 2 had two independent sources of run-to-run randomness and only one of them was +ever set: + +* ``torch`` / ``numpy`` / ``random`` were seeded to a hardcoded ``42`` -- that governs + weight initialization for the head, dropout, and the augmentation draws. +* ``DistributedSampler`` carries its **own** ``seed`` (default ``0``) and derives each + epoch's permutation from ``seed + epoch`` inside ``set_epoch()``. Nothing in + ``train.py`` touched it, so it stayed at ``0``. + +So every published run is the pair ``(42, 0)``, and the recipe's spread across seeds has +never been measured -- it is n=1. That was a footnote while the RampNet-vs-YOLO gap was +0.252 F1; at the matched-operating-point gap of 0.039 it is the binding number. See +``docs/seed_variance_51_135.md``. + +The trap this module exists to close: a sweep that varies only the torch seed reuses one +data order across every arm, which understates the true spread, and **does it silently** +-- no log line distinguishes the two. So the two seeds move together, with one exception +that has to be exact: at the historical torch seed the sampler must stay at its +historical ``0``, or the default stops reproducing the published runs. +""" + +HISTORICAL_SEED = 42 +"""The torch/numpy/random seed every published Stage 2 run used.""" + +HISTORICAL_SAMPLER_SEED = 0 +"""``DistributedSampler``'s default, which every published Stage 2 run inherited.""" + + +def sampler_seed_for(seed: int) -> int: + """Return the ``DistributedSampler`` seed that pairs with ``seed``. + + At :data:`HISTORICAL_SEED` this is :data:`HISTORICAL_SAMPLER_SEED`, so the default + reproduces published runs exactly. Every other seed maps to itself, so a sweep gets + a genuinely different data order as well as different initialization. + + The asymmetry is deliberate and is the whole point of the function: it is the only + way to add a seed flag without silently changing what the default does. + + .. warning:: + The mapping is therefore NOT injective: ``sampler_seed_for(0)`` and + ``sampler_seed_for(42)`` are both ``0``. A replicate at seed ``0`` gets a fresh + initialization but the **published run's data order**, which makes it less + independent of the published run than its seed column suggests. Seed 0 is the + natural first pick -- it is the ultralytics default every #51 YOLO arm used -- so + extend a Stage 2 sweep with 4, 5, ..., never with 0. + """ + if seed == HISTORICAL_SEED: + return HISTORICAL_SAMPLER_SEED + return seed diff --git a/scripts/model_comparison/run_yolo_train_tillicum.slurm b/scripts/model_comparison/run_yolo_train_tillicum.slurm index eefec30b..349fc9f2 100644 --- a/scripts/model_comparison/run_yolo_train_tillicum.slurm +++ b/scripts/model_comparison/run_yolo_train_tillicum.slurm @@ -86,6 +86,21 @@ EPOCHS="${EPOCHS:-60}" BATCH="${BATCH:--1}" # pin per config to match the klone runs PATIENCE="${PATIENCE:-20}" +# SEED. Every #51 arm ran seed=0 (the ultralytics default), so the whole baseline is +# n=1 and its run-to-run spread has never been measured. That was a footnote while the +# RampNet-vs-YOLO gap was 0.252 F1; at the matched-operating-point gap of 0.039 +# (docs/operating_point_parity_51.md -- on branch fix/yolo-label-cache-rescue-51 / PR +# #154, not on main yet) it is the binding question, and #51's own rule is that +# differences under ~0.02 should not be read. +# +# Ultralytics seeds torch/numpy/random from this AND sets deterministic=True by default, +# so it also governs augmentation and the initial head weights -- i.e. it is the whole +# run-to-run knob, not just the shuffle. +# +# Leave it 0 for anything meant to reproduce an existing arm. Set it for a seed sweep: +# SEED=1 NAME=y11x_tiles_s1 ... sbatch +SEED="${SEED:-0}" + # Keep a checkpoint every N epochs. Ultralytics defaults save_period=-1, keeping ONLY # last.pt and best.pt -- which forecloses, permanently and retroactively, any analysis # that needs a checkpoint from a specific epoch. @@ -144,7 +159,7 @@ APPTAINER_IMG="${APPTAINER_IMG:-}" echo "--- YOLO train on TILLICUM (issue #51 / #70) ---" echo "base: ${YOLO_CKPT}" echo "data: ${YOLO_DATA}" -echo "imgsz: ${YOLO_IMGSZ} epochs: ${EPOCHS} batch: ${BATCH} patience: ${PATIENCE}" +echo "imgsz: ${YOLO_IMGSZ} epochs: ${EPOCHS} batch: ${BATCH} patience: ${PATIENCE} seed: ${SEED}" echo "alloc: ${SLURM_GPUS_ON_NODE:-?} GPU(s), ${SLURM_CPUS_PER_TASK:-?} CPUs on ${SLURMD_NODENAME:-?}" echo "device: ${DEVICE} workers: ${WORKERS} (allocation and device differ on purpose -- see header)" echo "chain: ${CHAIN} follow-on job(s) queued after this one" @@ -173,11 +188,12 @@ fi run_train() { "$@" - "$YOLO_CKPT" "$YOLO_DATA" "$YOLO_IMGSZ" "$EPOCHS" "$BATCH" "$PATIENCE" \ - "$PROJECT" "$NAME" "$TRAIN_HOURS" "$DEVICE" "$SAVE_PERIOD" "$WORKERS" <<'PY' + "$PROJECT" "$NAME" "$TRAIN_HOURS" "$DEVICE" "$SAVE_PERIOD" "$WORKERS" \ + "$SEED" <<'PY' import os, sys from ultralytics import YOLO (ckpt, data, imgsz, epochs, batch, patience, project, name, hours, device, - save_period, workers) = sys.argv[1:13] + save_period, workers, seed) = sys.argv[1:14] last = os.path.join(project, name, "weights", "last.pt") # Still needed on Tillicum, but for a different reason than on klone: not preemption, # but the 24 h normal-QoS ceiling. A 60-epoch tiles schedule does not fit in one job, @@ -206,7 +222,7 @@ else: kw = dict( data=data, imgsz=int(imgsz), epochs=int(epochs), batch=batch_arg, patience=int(patience), project=project, name=name, device=dev, exist_ok=True, - save_period=int(save_period), workers=int(workers), + save_period=int(save_period), workers=int(workers), seed=int(seed), ) if float(hours) > 0: kw["time"] = float(hours) diff --git a/stage_two/run_train_seed.slurm b/stage_two/run_train_seed.slurm new file mode 100644 index 00000000..afaf2892 --- /dev/null +++ b/stage_two/run_train_seed.slurm @@ -0,0 +1,141 @@ +#!/bin/bash +# Train ONE Stage 2 seed replicate on klone, for the seed-variance campaign +# (docs/seed_variance_51_135.md; issues #51 and #135). +# +# WHY THIS IS A SEPARATE FILE AND NOT A FLAG ON run_train.slurm +# run_train.slurm is the preserved record of the published run and of #135's rungs. It +# should not churn, and more importantly a seed replicate needs two things that file +# deliberately does not do: a per-seed working directory, and a bounded epoch count. The +# same reasoning kept run_yolo_train_tillicum.slurm separate from its klone original. +# +# WHY A PER-SEED WORKING DIRECTORY -- THIS IS THE LOAD-BEARING PART +# train.py writes `best_model.pth` and `latest_checkpoint.pth` to the CURRENT DIRECTORY, +# not to --checkpoint-dir (see stage_two/train.py: torch.save(..., "best_model.pth")). +# Three seeds launched from one directory would therefore overwrite each other's best +# model AND each other's resume state, and the second failure is worse than the first: +# a resume file from another seed is silently loaded as if it were this run's own, so +# the arms converge on one lineage and nothing in the log says so. Each seed gets its +# own RUNDIR and cd's into it before torchrun. +# +# USAGE. Submit from the repo root; logs/ must ALREADY exist, because Slurm opens +# --output relative to the SUBMIT directory before this script runs (the mkdir below +# cannot help with that -- it is there for a RUNDIR that does not exist yet). +# mkdir -p logs +# SEED=1 sbatch stage_two/run_train_seed.slurm +# SEED=2 RUNDIR=/gscratch/scrubbed/$USER/seedvar/rampnet_s2 sbatch stage_two/run_train_seed.slurm +# +# RUNDIR IS ON A VOLUME THAT PURGES. best_model.pth -- the only artifact this campaign +# produces -- lands in RUNDIR, and /gscratch/scrubbed purges on a ~21-day idle window +# (docs/stage2_epoch_curve_84.md). Campaign B's calendar is unbounded (ckpt-all duty +# cycle 3.9%), so a replicate can finish and then sit unscored past that window while +# its siblings are still pending. COPY IT OUT as soon as the job completes: +# cp "$RUNDIR/best_model.pth" /gscratch/makelab/$USER/seedvar/rampnet_s${SEED}_best.pth +# /gscratch/makelab is purchased and never purged, which is where #84 put Run A's +# checkpoints for the same reason. The default is deliberately left on scrubbed so it +# matches the replicates already queued; changing it is a decision, not a cleanup. +# +# COST. klone ckpt is FREE and preemptable. One replicate is 1 epoch = ~3.5 h on 16 +# GPUs (~56 GPU-h, measured -- docs/stage2_training_cost.md). Preemption is handled by +# --requeue plus train.py's own latest_checkpoint.pth resume, so a requeued replicate +# continues rather than restarting. Calendar, not money, is the risk here: ckpt-all's +# duty cycle was 3.9% in 2026-08 (#135), so a replicate can sit pending for days. +#SBATCH -p ckpt-all +#SBATCH --job-name=rampnet_seedvar +#SBATCH --nodes=4 +#SBATCH --ntasks-per-node=1 +#SBATCH --gpus-per-node=4 +#SBATCH --cpus-per-task=12 +#SBATCH --mem=48G +#SBATCH --time=24:00:00 +#SBATCH --output=logs/seedvar_%j.out +#SBATCH --error=logs/seedvar_%j.err +#SBATCH --requeue +#SBATCH --constraint='l40s|l40|a40|a100' + +set -euo pipefail + +# The seed IS the experiment, so it is required rather than defaulted -- a replicate +# that silently ran at 42 would be a duplicate of the published run wearing a new name. +# NO APOSTROPHE in this message. Inside ${VAR:?...} bash parses the word for quoting +# even within double quotes, so a lone ' opens a quote and the closing } is never found: +# "unexpected EOF while looking for matching `}'". The script then dies at submit time +# with exit 2 in about one second, having printed nothing -- which is how jobs +# 39515025/26/27 failed on 2026-09-03 and then sat unnoticed for a day. +SEED="${SEED:?set SEED to the seed for this replicate, e.g. SEED=1}" + +REPO="${REPO:-$HOME/RampNet}" +DATA_ROOT="${DATA_ROOT:-/gscratch/scrubbed/$USER/rampnet_dataset}" +# NOT $HOME: klone home is a separate 10 GB quota that gscratch cleanup does not touch, +# and one replicate's per-epoch checkpoints alone would blow it. +RUNDIR="${RUNDIR:-/gscratch/scrubbed/$USER/seedvar/rampnet_s${SEED}}" +EPOCHS="${EPOCHS:-1}" # the published recipe is 1 epoch / 9,378 steps (#84) + +mkdir -p "$RUNDIR/checkpoints" "$REPO/logs" + +# Interpreter. Default behaviour is unchanged -- `source activate sidewalkcv2`, then +# torchrun off PATH -- but every other klone launcher here carries an escape hatch +# (PYTHON= in run_yolo_train.slurm and run_gold_bundle.slurm, RAMPNET_ENV in +# run_train_epoch_curve.slurm) and this one did not. It matters because #84's env was +# built at a PREFIX, /gscratch/scrubbed/$USER/envs/sidewalkcv2, which `source activate +# ` cannot resolve: under `set -e` the job would then die here having printed one +# line. Set RAMPNET_ENV to a conda prefix and the env's own torchrun is called by +# absolute path instead -- the pattern run_train_epoch_curve.slurm documents as the one +# proven on this cluster. +RAMPNET_ENV="${RAMPNET_ENV:-}" +if [ -n "$RAMPNET_ENV" ]; then + TORCHRUN="$RAMPNET_ENV/bin/torchrun" + if [ ! -x "$TORCHRUN" ]; then + echo "FATAL: RAMPNET_ENV=$RAMPNET_ENV has no executable bin/torchrun" >&2 + exit 1 + fi + echo "Using conda prefix ${RAMPNET_ENV} (no activation)" +else + echo "Loading Conda environment..." + source activate sidewalkcv2 + TORCHRUN=torchrun +fi + +export MASTER_ADDR=$(scontrol show hostnames $SLURM_JOB_NODELIST | head -n 1) +export OMP_NUM_THREADS=$SLURM_CPUS_PER_TASK +export MASTER_PORT=$(expr 10000 + $(echo -n $SLURM_JOBID | tail -c 4)) +# train.py lives in stage_two/ but imports the `rampnet` package from the repo root, and +# we run from RUNDIR, so neither is on sys.path by default. +export PYTHONPATH="$REPO:${PYTHONPATH:-}" + +NPROC_PER_NODE=${SLURM_GPUS_PER_NODE:-4} +WORLD_SIZE=$(($SLURM_NNODES * $NPROC_PER_NODE)) + +echo "--- Stage 2 seed replicate (#51 / #135) ---" +echo "Job ID: ${SLURM_JOBID}" +echo "Seed: ${SEED}" +echo "Run dir: ${RUNDIR} (cwd -- best_model.pth and latest_checkpoint.pth land here)" +echo "Data root: ${DATA_ROOT}" +echo "Epochs: ${EPOCHS}" +echo "Nodes: ${SLURM_NNODES} x ${NPROC_PER_NODE} GPU (world size ${WORLD_SIZE})" +echo "Node list: ${SLURM_NODELIST}" +echo "Restarts: ${SLURM_RESTART_COUNT:-0} (requeue resumes from latest_checkpoint.pth)" +echo "-------------------------------------------" + +# World size IS the global batch: train.py uses batch_size=1 per rank, so a replicate +# that lands on any other node/GPU count is a different optimisation regime, not a seed +# replicate of the published recipe. Same guard as run_train_epoch_curve.slurm. +if [ "${WORLD_SIZE}" -ne 16 ]; then + echo "WARNING: world size ${WORLD_SIZE} != 16. The published recipe's global batch" >&2 + echo " was 16; this is NOT a seed replicate of it at any other world size." >&2 +fi + +cd "$RUNDIR" + +srun --export=ALL \ + "$TORCHRUN" --nnodes $SLURM_NNODES \ + --nproc_per_node $NPROC_PER_NODE \ + --rdzv_id $SLURM_JOB_ID \ + --rdzv_backend c10d \ + --rdzv_endpoint $MASTER_ADDR:$MASTER_PORT \ + "$REPO/stage_two/train.py" \ + --seed "$SEED" \ + --epochs "$EPOCHS" \ + --data-root "$DATA_ROOT" \ + --checkpoint-dir "$RUNDIR/checkpoints" + +echo "--- Slurm job finished ---" diff --git a/stage_two/train.py b/stage_two/train.py index 7ad35e21..1d8e7d72 100644 --- a/stage_two/train.py +++ b/stage_two/train.py @@ -22,6 +22,7 @@ from rampnet.model import KeypointModel from rampnet.loading import load_checkpoint +from rampnet.seeding import HISTORICAL_SEED, sampler_seed_for # Learning-rate defaults per preset: training from ImageNet initialization # uses the paper's 1e-5; fine-tuning released/earlier RampNet weights wants a @@ -130,6 +131,11 @@ def parse_args(): "run always takes precedence, and warm-starting applies at step 0 only.") parser.add_argument('--checkpoint-dir', default='checkpoints', help="Directory for per-epoch checkpoints (default: checkpoints)") + parser.add_argument('--seed', type=int, default=HISTORICAL_SEED, + help=f"Seed for torch/numpy/random AND the DistributedSampler shuffle " + f"(default: {HISTORICAL_SEED}, the value every published run used). " + f"Change it only to measure run-to-run variance -- see " + f"docs/seed_variance_51_135.md") parser.add_argument('--lr-schedule', choices=LR_SCHEDULES, default='constant', help="'constant' is the paper recipe and the default -- every " "existing invocation is unaffected. 'cosine' decays --lr to " @@ -172,9 +178,18 @@ def cleanup_distributed(): rank, local_rank, world_size = setup_distributed() -torch.manual_seed(42) -random.seed(42) -np.random.seed(42) +# These were hardcoded to 42, so every published run shares one seed and the recipe's +# run-to-run spread is unmeasured (n=1). --seed defaults to 42, so nothing about an +# existing run changes; it exists so a seed sweep is possible at all. See +# docs/seed_variance_51_135.md for why that spread is now the binding number. +# +# The sampler seed has to move WITH this one. DistributedSampler takes its own `seed` +# (default 0) and derives the shuffle from seed + epoch via set_epoch(), so leaving it +# alone would give every "different" seed the identical data order -- a sweep that +# varies initialization only, silently understating the true spread. +torch.manual_seed(args.seed) +random.seed(args.seed) +np.random.seed(args.seed) new_root_dir = args.data_root @@ -356,8 +371,21 @@ def __getitem__(self, idx): if len(val_dataset) == 0 and rank == 0: print("Warning: Validation dataset is empty.") +# DistributedSampler has its OWN seed (default 0) and derives the shuffle from +# seed + epoch in set_epoch(), so it is independent of torch.manual_seed above. Every +# published run therefore paired manual_seed(42) with sampler seed 0, and +# sampler_seed_for() preserves that pairing exactly at the default: passing --seed 42 +# gives those runs' initialization AND their data order, while any other --seed moves +# the data order too. Not bit-identical, and no claim to be: cuDNN autotuning, AMP loss +# scaling and DDP allreduce ordering are not seeded and torch.use_deterministic_algorithms +# is not set. What is preserved is every source of randomness this script controls. +# +# Both halves have to move together. A sweep that varied initialization but reused one +# data order would understate the true run-to-run spread -- and would do it silently, +# since nothing in the logs distinguishes the two. train_sampler = ResumeSkipSampler( - DistributedSampler(train_dataset, num_replicas=world_size, rank=rank, shuffle=True, drop_last=True)) + DistributedSampler(train_dataset, num_replicas=world_size, rank=rank, shuffle=True, drop_last=True, + seed=sampler_seed_for(args.seed))) val_sampler = DistributedSampler(val_dataset, num_replicas=world_size, rank=rank, shuffle=False, drop_last=False) if len(val_dataset) > 0 else None train_loader = DataLoader(train_dataset, batch_size=1, sampler=train_sampler, num_workers=4, pin_memory=True) @@ -384,6 +412,11 @@ def __getitem__(self, idx): os.makedirs(args.checkpoint_dir, exist_ok=True) writer = SummaryWriter(log_dir='runs/experiment_1') print(f"Preset: {args.preset}, lr: {args.lr}, epochs: {args.epochs}, data root: {new_root_dir}") + # Both seeds in the log, always. A seed sweep whose arms cannot be told apart from + # their own logs is not reproducible, and the sampler half is the one that is easy + # to leave unset without noticing. + print(f"Seed: {args.seed} (sampler seed: {sampler_seed_for(args.seed)}), " + f"checkpoint dir: {args.checkpoint_dir}") print(f"LR schedule: {args.lr_schedule}" + (f" -> {args.lr * args.lr_final_frac:.3g} over {total_train_steps} steps" if args.lr_schedule != 'constant' else " (no decay, as in the paper)")) diff --git a/tests/test_resume_skip_sampler.py b/tests/test_resume_skip_sampler.py index ad0a5b43..977f1ea6 100644 --- a/tests/test_resume_skip_sampler.py +++ b/tests/test_resume_skip_sampler.py @@ -29,6 +29,8 @@ from check_lr_schedule_135 import load_from_train_py # noqa: E402 +from rampnet.seeding import HISTORICAL_SEED + import itertools # noqa: E402 LIFTED = load_from_train_py("ResumeSkipSampler", itertools=itertools, Sampler=Sampler) @@ -151,7 +153,8 @@ def test_checkpoint_interval_default_is_still_the_paper_recipe(): checkpointing granularity along with it. """ import argparse - mod = load_from_train_py("parse_args", "PRESET_LR", "LR_SCHEDULES", argparse=argparse) + mod = load_from_train_py("parse_args", "PRESET_LR", "LR_SCHEDULES", + argparse=argparse, HISTORICAL_SEED=HISTORICAL_SEED) argv = sys.argv try: sys.argv = ["train.py"] diff --git a/tests/test_seeding.py b/tests/test_seeding.py new file mode 100644 index 00000000..34b1d719 --- /dev/null +++ b/tests/test_seeding.py @@ -0,0 +1,258 @@ +"""Tests for the Stage 2 / YOLO seed plumbing (docs/seed_variance_51_135.md). + +These are cheap source-level assertions rather than training runs, and they exist +because every failure mode here is SILENT: a sweep whose arms share a data order, a +positional argument list that shifts by one, or a default that quietly stops +reproducing the published run all produce a job that finishes green with the wrong +number in it. +""" + +import ast +import re +from pathlib import Path + +import pytest + +from rampnet.seeding import ( + HISTORICAL_SAMPLER_SEED, + HISTORICAL_SEED, + sampler_seed_for, +) + +REPO = Path(__file__).resolve().parents[1] +TRAIN_PY = REPO / "stage_two" / "train.py" +SEED_SLURM = REPO / "stage_two" / "run_train_seed.slurm" +TILLICUM_SLURM = REPO / "scripts" / "model_comparison" / "run_yolo_train_tillicum.slurm" + + +def _assigned_call(target_name): + """Return the ``ast.Call`` assigned to ``target_name`` at module level in train.py. + + Substring assertions over the whole file are NOT good enough here. ``train.py`` also + logs ``sampler_seed_for(args.seed)`` in a print(), so ``"sampler_seed_for(args.seed)" + in src`` stays true even after the kwarg is deleted from the sampler itself -- which + is the one regression this file exists to catch. Parsing the actual statement is the + only assertion that distinguishes them. + """ + tree = ast.parse(TRAIN_PY.read_text(encoding="utf-8")) + for node in tree.body: + if not isinstance(node, ast.Assign): + continue + if not any(isinstance(t, ast.Name) and t.id == target_name for t in node.targets): + continue + value = node.value + # `x = Call(...) if cond else None` -- the val sampler's shape + if isinstance(value, ast.IfExp): + value = value.body + if isinstance(value, ast.Call): + return value + raise AssertionError(f"no module-level `{target_name} = (...)` in train.py") + + +def _kwarg(call, name): + for kw in call.keywords: + if kw.arg == name: + return kw.value + return None + + +# -------------------------------------------------------------------------------- +# sampler_seed_for +# -------------------------------------------------------------------------------- + +def test_historical_seed_maps_to_the_historical_sampler_seed(): + """The default must reproduce published runs, which paired manual_seed(42) with + DistributedSampler's own default of 0. If this ever returns 42, every reproduction + silently gets a different data order.""" + assert sampler_seed_for(HISTORICAL_SEED) == HISTORICAL_SAMPLER_SEED + assert HISTORICAL_SEED != HISTORICAL_SAMPLER_SEED # or the mapping is vacuous + + +@pytest.mark.parametrize("seed", [0, 1, 2, 3, 7, 41, 43, 1234]) +def test_every_other_seed_maps_to_itself(seed): + """A sweep has to move the data order too; identity is what guarantees that.""" + assert sampler_seed_for(seed) == seed + + +def test_seed_zero_shares_the_published_data_order(): + """The mapping is deliberately NOT injective, and seed 0 is where that bites. + + ``sampler_seed_for(0) == sampler_seed_for(42) == 0``, so a replicate at ``--seed 0`` + gets a different initialization from the published run but the SAME data order. That + is a legal thing to want; it is not a legal thing to do by accident, because it makes + two arms less independent than the seed column suggests. Campaigns A and B both use + 1/2/3, which is why this is documented rather than forbidden -- extend with 4, 5, ..., + never with 0. + """ + assert sampler_seed_for(0) == sampler_seed_for(HISTORICAL_SEED) + + +def test_distinct_seeds_give_distinct_sampler_seeds(): + """The campaign's three replicates must not collide with each other or with the + published run's data order.""" + campaign = [1, 2, 3] + sampler_seeds = [sampler_seed_for(s) for s in campaign] + assert len(set(sampler_seeds)) == len(campaign) + assert HISTORICAL_SAMPLER_SEED not in sampler_seeds + + +# -------------------------------------------------------------------------------- +# stage_two/train.py -- source assertions (importing it runs argparse and DDP setup) +# -------------------------------------------------------------------------------- + +def test_train_py_declares_seed_defaulting_to_the_historical_value(): + src = TRAIN_PY.read_text(encoding="utf-8") + assert "'--seed'" in src + assert re.search(r"add_argument\(\s*'--seed',\s*type=int,\s*default=HISTORICAL_SEED", + src), "the --seed default must be the named constant, not a literal" + + +def test_train_py_seeds_all_three_rngs_from_args(): + """torch, numpy and random were all hardcoded to 42; all three have to follow the + flag or the sweep varies only part of the state. + + Matched line-anchored, not as substrings: ``"random.seed(args.seed)" in src`` is + satisfied by ``np.random.seed(args.seed)`` alone, so the stdlib ``random`` call -- + the one that drives the horizontal-flip augmentation in EquiHeatmapDataset -- could + be deleted without failing anything. + """ + src = TRAIN_PY.read_text(encoding="utf-8") + for call in ("torch.manual_seed(args.seed)", + "random.seed(args.seed)", + "np.random.seed(args.seed)"): + assert re.search(rf"^{re.escape(call)}$", src, re.M), f"{call} missing" + assert "torch.manual_seed(42)" not in src + assert "np.random.seed(42)" not in src + + +def _unwrap_to(call, func_name): + """Descend through wrapper calls to the ``func_name`` call inside. + + ``train_sampler`` is ``ResumeSkipSampler(DistributedSampler(...))`` since #135's + resume-skip landed. The seed belongs on the INNER sampler -- putting it on the + wrapper would be inert -- so this walks in rather than asserting on the outer call, + and fails loudly if the target is not found at all. + """ + seen = [] + while isinstance(call, ast.Call): + name = call.func.id if isinstance(call.func, ast.Name) else None + seen.append(name) + if name == func_name: + return call + nested = [a for a in call.args if isinstance(a, ast.Call)] + assert len(nested) == 1, ( + f"cannot find {func_name}(...) in the assignment; walked {seen} and the last " + f"call has {len(nested)} nested calls, so which one to follow is ambiguous") + call = nested[0] + raise AssertionError(f"no {func_name}(...) call found; walked {seen}") + + +def test_train_sampler_seed_is_derived_not_hardcoded(): + """The regression this guards: dropping the sampler seed (back to its constant 0 + default) and leaving a sweep that varies initialization only. + + Asserted on the parsed ``train_sampler = DistributedSampler(...)`` statement, not on + the file text. Deleting the kwarg passes any whole-file substring check, because the + same expression appears in the seed log line. + """ + call = _unwrap_to(_assigned_call("train_sampler"), "DistributedSampler") + + shuffle = _kwarg(call, "shuffle") + assert isinstance(shuffle, ast.Constant) and shuffle.value is True, ( + "the train sampler must shuffle, or the seed is inert") + + seed = _kwarg(call, "seed") + assert seed is not None, ( + "train_sampler has no seed= -- every replicate would reuse DistributedSampler's " + "default of 0, i.e. one data order across the whole sweep, silently") + assert ast.unparse(seed) == "sampler_seed_for(args.seed)", ( + f"train_sampler seed= is {ast.unparse(seed)!r}; it must be derived from --seed " + f"via sampler_seed_for, not a literal") + + +def test_val_sampler_is_unshuffled_and_therefore_needs_no_seed(): + call = _assigned_call("val_sampler") + assert _kwarg(call, "shuffle").value is False + assert _kwarg(call, "seed") is None + + +def test_train_py_logs_both_seeds(): + """A replicate whose log cannot identify its own seed is not reproducible.""" + src = TRAIN_PY.read_text(encoding="utf-8") + assert 'f"Seed: {args.seed} (sampler seed: {sampler_seed_for(args.seed)})' in src + + +# -------------------------------------------------------------------------------- +# stage_two/run_train_seed.slurm +# -------------------------------------------------------------------------------- + +def test_seed_launcher_requires_a_seed(): + """Defaulting SEED would make an un-set replicate a silent duplicate of the + published run.""" + src = SEED_SLURM.read_text(encoding="utf-8") + assert 'SEED="${SEED:?' in src, "SEED must be required (:?), never defaulted" + + +def test_seed_launcher_isolates_each_replicate_in_its_own_directory(): + """train.py writes best_model.pth and latest_checkpoint.pth to the CWD, so shared + directories cross-contaminate resume state between replicates.""" + src = SEED_SLURM.read_text(encoding="utf-8") + assert 'RUNDIR="${RUNDIR:-' in src + assert "rampnet_s${SEED}" in src, "the default RUNDIR must be per-seed" + assert re.search(r'^cd "\$RUNDIR"$', src, re.M), "must cd into RUNDIR before torchrun" + assert "--seed \"$SEED\"" in src + + +def test_seed_launcher_does_not_write_checkpoints_to_home(): + """klone home is a separate 10 GB quota; per-epoch checkpoints blow it.""" + src = SEED_SLURM.read_text(encoding="utf-8") + rundir = next(l for l in src.splitlines() if l.startswith('RUNDIR=')) + assert "$HOME" not in rundir and "~/" not in rundir + + +# -------------------------------------------------------------------------------- +# scripts/model_comparison/run_yolo_train_tillicum.slurm +# -------------------------------------------------------------------------------- + +def test_tillicum_launcher_defaults_seed_to_the_published_value(): + """Every #51 arm ran seed=0, so the default has to stay 0 or reproducing an arm + silently trains a different model.""" + src = TILLICUM_SLURM.read_text(encoding="utf-8") + assert 'SEED="${SEED:-0}"' in src + + +def test_tillicum_launcher_positional_args_line_up(): + """The training call passes bare positionals into a heredoc, so an added argument + that is not also unpacked shifts every later field by one -- imgsz becoming epochs, + and so on. Nothing would raise; the run would just be wrong.""" + src = TILLICUM_SLURM.read_text(encoding="utf-8") + + call = src.split("run_train() {", 1)[1].split("<<'PY'", 1)[0] + # Both spellings, "$VAR" and "${VAR}". Counting only the bare form left the check + # blind to a braced positional: inserting "${LR0}" mid-list shifts data -> imgsz -> + # epochs by one and used to pass green. + n_passed = len(re.findall(r'"\$\{?[A-Za-z_][A-Za-z0-9_]*\}?"', call)) + + unpack = re.search(r"\)\s*=\s*sys\.argv\[1:(\d+)\]", src) + assert unpack, "could not find the sys.argv unpack" + n_unpacked = int(unpack.group(1)) - 1 + + assert n_passed == n_unpacked, ( + f"{n_passed} shell positionals but sys.argv[1:{n_unpacked + 1}] unpacks " + f"{n_unpacked}") + + names = re.search(r"\(ckpt, data, .*?\)\s*=\s*sys\.argv", src, re.S).group(0) + assert len([n for n in names.split("=")[0].strip("() ").split(",") if n.strip()]) == n_unpacked + + +def test_tillicum_launcher_forwards_seed_to_ultralytics(): + src = TILLICUM_SLURM.read_text(encoding="utf-8") + assert "seed=int(seed)" in src, "seed must reach YOLO.train(), not just the shell" + assert '"$SEED"' in src.split("run_train() {", 1)[1].split("<<'PY'", 1)[0] + + +def test_tillicum_launcher_echoes_the_seed(): + """The as-run seed has to be recoverable from the job log, not only from args.yaml + inside a checkpoint that may be purged.""" + src = TILLICUM_SLURM.read_text(encoding="utf-8") + assert "seed: ${SEED}" in src diff --git a/tests/test_slurm_scripts.py b/tests/test_slurm_scripts.py new file mode 100644 index 00000000..bcccf6dc --- /dev/null +++ b/tests/test_slurm_scripts.py @@ -0,0 +1,81 @@ +"""Every .slurm launcher must at least PARSE. + +This exists because of a real loss. `stage_two/run_train_seed.slurm` carried an +apostrophe inside a `${SEED:?...}` message; bash parses the word for quoting even +inside double quotes, so the closing brace was never found. The three klone Stage 2 +seed replicates (39515025/26/27, #51 / #135) therefore died at submit time with exit 2 +in about one second each, printed nothing to stdout, and sat unnoticed for a day while +the paid Tillicum half of the same campaign ran normally. + +Nothing else in the suite reads these files as shell. `test_seeding.py` asserts on +their CONTENT with regexes, which a syntactically broken script passes happily. A +launcher is the one artifact whose failure is invisible locally -- you find out on the +cluster, hours later, from an empty log -- so it is worth the two seconds `bash -n` +costs. CPU-only, no network, no cluster. +""" + +import shutil +import subprocess +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parents[1] + +# rglob, but never descend into dot-directories: `.claude/worktrees/` holds whole +# nested checkouts of this repo, and collecting their launchers would multiply this +# test by the number of branches anyone happens to have on disk. +SLURM_SCRIPTS = sorted( + p + for p in REPO.rglob("*.slurm") + if not any(part.startswith(".") for part in p.relative_to(REPO).parts) +) + +BASH = shutil.which("bash") + +requires_bash = pytest.mark.skipif( + BASH is None, + reason="bash not on PATH (CI runs ubuntu-latest, where it always is)", +) + + +def test_the_launchers_were_actually_found(): + """A glob that silently matches nothing would make every test below vacuous.""" + assert len(SLURM_SCRIPTS) >= 15, [str(p) for p in SLURM_SCRIPTS] + + +@requires_bash +@pytest.mark.parametrize( + "script", SLURM_SCRIPTS, ids=[str(p.relative_to(REPO)) for p in SLURM_SCRIPTS] +) +def test_slurm_script_parses(script): + """`bash -n` reads the script and checks syntax without running a line of it.""" + proc = subprocess.run( + [BASH, "-n", str(script)], + capture_output=True, + text=True, + ) + assert proc.returncode == 0, ( + f"{script.relative_to(REPO)} is not valid bash -- sbatch would accept it and " + f"the job would die in about a second with exit 2:\n{proc.stderr}" + ) + + +@requires_bash +def test_the_seed_guard_still_rejects_an_unset_seed(): + """The apostrophe fix must not have cost the guard its job. + + SEED is required rather than defaulted because a replicate that silently ran at 42 + would be a duplicate of the published run wearing a new name (#51 / #135). Sourcing + the whole launcher would try to reach Slurm, so this evaluates the one expansion. + """ + line = next( + ln + for ln in (REPO / "stage_two" / "run_train_seed.slurm").read_text().splitlines() + if ln.startswith("SEED=") + ) + proc = subprocess.run( + [BASH, "-c", line], capture_output=True, text=True, env={"PATH": "/usr/bin:/bin"} + ) + assert proc.returncode != 0, "an unset SEED must abort the job, not default" + assert "SEED" in proc.stderr