From 00352e298597151254d282d84619df36302b2ac2 Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Thu, 3 Sep 2026 09:29:26 -0700 Subject: [PATCH 1/4] Make the seed settable, and pre-register what its variance will be read to mean (#51, #135) Both issues are blocked on the same missing number. #51's matched-operating-point read put the RampNet-vs-YOLO residual at 0.039 F1, and #51's own rule is that differences under ~0.02 should not be read -- but every arm in that comparison is seed 0, the ultralytics default, in all eight args.yaml. #135's power analysis reached the same wall from the other side and said so outright: the binding limit is unmeasured seed variance, n=1. Nothing downstream can be called real without it, RampNet 2.0 included. Neither trainer could vary the seed at all. train.py hardcoded manual_seed(42) with no flag; run_yolo_train_tillicum.slurm never passed one to ultralytics. THE HALF THAT WAS EASY TO GET WRONG. Stage 2 has TWO sources of run-to-run randomness, and only one was ever set. DistributedSampler carries its own seed (default 0) and derives each epoch's permutation from seed + epoch in set_epoch(), independent of torch.manual_seed. A sweep that moved only the torch seed would reuse one data order across every replicate, understating the spread -- and would do it silently, since no log line distinguishes the two cases. So both seeds move together, with one exact exception: at the historical seed 42 the sampler must stay at its historical 0, or the DEFAULT stops reproducing published runs. That pairing is rampnet/seeding.py::sampler_seed_for, tested rather than commented. Also load-bearing, and the reason the klone launcher is a new file rather than a flag on run_train.slurm: train.py writes best_model.pth and latest_checkpoint.pth to the CURRENT DIRECTORY, not to --checkpoint-dir. Three replicates launched from one directory would overwrite each other's best model and, worse, each other's resume state -- a resume file from another seed loads silently as if it were this run's own, converging the arms onto one lineage with nothing in the log to say so. run_train_seed.slurm gives each replicate its own RUNDIR and cd's into it. PRE-REGISTERED, before any replicate finished: docs/seed_variance_51_135.md fixes the decision rule. SD <= 0.010 and the architecture advantage is real at ~4 sigma; SD >= 0.020 and #51 closes with "the supervised baseline is statistically indistinguishable from RampNet at matched operating points"; between is ambiguous and needs both campaigns. The second branch makes our own headline smaller and is accepted in advance. One deliberate deviation from the #51 protocol: save_period=1. The arms ran -1, which is exactly why the epoch-curve follow-up had to be retracted -- no per-epoch weights exist anywhere and cannot be recovered. ~20 GB against 1 TB buys back that analysis. tests/test_seeding.py (21) checks the plumbing at the source level, because every failure here is silent: the default still mapping to sampler seed 0, all three RNGs following the flag, the launcher refusing to default SEED, per-seed RUNDIR isolation, and the Tillicum heredoc's positional list still lining up with its unpack -- an off-by-one there shifts imgsz into epochs and trains a wrong model that finishes green. Full suite: 1,343 passed, 1 skipped. Co-Authored-By: Claude Opus 5 --- docs/seed_variance_51_135.md | 137 ++++++++++++++ rampnet/seeding.py | 43 +++++ .../run_yolo_train_tillicum.slurm | 23 ++- stage_two/run_train_seed.slurm | 95 ++++++++++ stage_two/train.py | 38 +++- tests/test_seeding.py | 167 ++++++++++++++++++ 6 files changed, 495 insertions(+), 8 deletions(-) create mode 100644 docs/seed_variance_51_135.md create mode 100644 rampnet/seeding.py create mode 100644 stage_two/run_train_seed.slurm create mode 100644 tests/test_seeding.py diff --git a/docs/seed_variance_51_135.md b/docs/seed_variance_51_135.md new file mode 100644 index 00000000..6ee16b10 --- /dev/null +++ b/docs/seed_variance_51_135.md @@ -0,0 +1,137 @@ +# 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.** + +## 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. Replicates run the full `epochs=60` schedule 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. + +**Decision rule for #51**, on the sample SD `s` of the three Campaign A replicates: + +| `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. + +## 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 a cheap extension + (~$119 each) and are the pre-registered response if the result lands in the ambiguous + band. They are not being run up front. +- **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. +- **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. +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). +SEED=1 sbatch stage_two/run_train_seed.slurm +``` + +The seed plumbing itself is unit-tested in `tests/test_seeding.py` — including that the +default reproduces published runs exactly (`sampler_seed_for(42) == 0`) and that the +Tillicum launcher's positional argument list still lines up with its unpack, an +off-by-one that would silently train at the wrong seed. diff --git a/rampnet/seeding.py b/rampnet/seeding.py new file mode 100644 index 00000000..8ff8cbfa --- /dev/null +++ b/rampnet/seeding.py @@ -0,0 +1,43 @@ +"""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. + """ + 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..a55a2dde 100644 --- a/scripts/model_comparison/run_yolo_train_tillicum.slurm +++ b/scripts/model_comparison/run_yolo_train_tillicum.slurm @@ -86,6 +86,20 @@ 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) 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 +158,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 +187,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 +221,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..bc2eeee5 --- /dev/null +++ b/stage_two/run_train_seed.slurm @@ -0,0 +1,95 @@ +#!/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 +# 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 +# +# 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. +SEED="${SEED:?set SEED to the replicate's seed, 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" + +echo "Loading Conda environment..." +source activate sidewalkcv2 + +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 "-------------------------------------------" + +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 a1a48a63..4fcc00e8 100644 --- a/stage_two/train.py +++ b/stage_two/train.py @@ -21,6 +21,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 @@ -45,6 +46,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") args = parser.parse_args() if args.preset == 'finetune' and args.init_weights is None: parser.error("--preset finetune requires --init-weights") @@ -71,9 +77,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 @@ -255,7 +270,17 @@ def __getitem__(self, idx): if len(val_dataset) == 0 and rank == 0: print("Warning: Validation dataset is empty.") -train_sampler = DistributedSampler(train_dataset, num_replicas=world_size, rank=rank, shuffle=True, drop_last=True) +# 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 +# reproduces those runs byte for byte, while any other --seed moves the data order too. +# +# 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 = 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) @@ -274,6 +299,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}") else: writer = None diff --git a/tests/test_seeding.py b/tests/test_seeding.py new file mode 100644 index 00000000..d0e212bf --- /dev/null +++ b/tests/test_seeding.py @@ -0,0 +1,167 @@ +"""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 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" + + +# -------------------------------------------------------------------------------- +# 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_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.""" + 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 call in src, f"{call} missing" + assert "torch.manual_seed(42)" not in src + assert "np.random.seed(42)" not in src + + +def test_train_sampler_seed_is_derived_not_hardcoded(): + """The regression this guards: dropping the sampler seed (back to a constant 0) and + leaving a sweep that varies initialization only.""" + src = TRAIN_PY.read_text(encoding="utf-8") + sampler_line = next(l for l in src.splitlines() + if "DistributedSampler(train_dataset" in l or + (l.strip().startswith("seed=sampler_seed_for"))) + assert "sampler_seed_for(args.seed)" in src + assert "shuffle=True" in src + # and the val sampler must NOT be shuffled, so it needs no seed + val_line = next(l for l in src.splitlines() if "val_sampler = DistributedSampler" in l) + assert "shuffle=False" in val_line + + +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] + n_passed = len(re.findall(r'"\$[A-Z_]+"', 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 From 2b7471c8f90debc62a312bd888a71ea6cfeff1f9 Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Fri, 4 Sep 2026 12:58:53 -0700 Subject: [PATCH 2/4] An apostrophe killed all three klone seed replicates; parse every launcher in CI (#51, #135) Jobs 39515025, 39515026 and 39515027 were submitted on 2026-09-03 at 15:01 and were all dead one second later, FAILED with exit 2. The stdout files are zero bytes; the whole record of the failure is 99 bytes of stderr: slurm_script: line 46: unexpected EOF while looking for matching `}' Line 46 was the required-SEED guard, whose message read "the replicate's seed". Inside ${VAR:?...} bash parses the word for quoting even within double quotes, so the lone apostrophe opened a quote and the closing brace was never found. Removing it is the entire fix; the guard still aborts on an unset SEED, which is tested rather than assumed. WHAT IT COST. Nothing in dollars -- klone ckpt is free -- and everything in calendar. The free half of the campaign produced no GPU-seconds for 27 hours while the paid Tillicum half ran normally beside it, so the asymmetry pointed the wrong way: the arm that measures RampNet's own seed spread, which #135 named as the binding limit, is the one that had not started. /gscratch/scrubbed/jfroehli/seedvar/ did not exist. WHY NOTHING CAUGHT IT. Nothing in the suite reads these files as shell. test_seeding.py asserts on their content with regexes, which a syntactically broken script passes happily, and a launcher is the one artifact whose failure is invisible locally -- you learn about it on the cluster, hours later, from an empty log. tests/test_slurm_scripts.py now runs bash -n over every tracked .slurm (23 of them, ~1.5 s, no cluster and no network) and fails on exactly this. Verified by reintroducing the apostrophe: two tests fail, and they name the file. The glob prunes dot-directories deliberately -- .claude/worktrees/ holds whole nested checkouts, and collecting their launchers would scale this test with however many branches happen to be on disk. Co-Authored-By: Claude Opus 5 (1M context) --- stage_two/run_train_seed.slurm | 7 ++- tests/test_slurm_scripts.py | 81 ++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 tests/test_slurm_scripts.py diff --git a/stage_two/run_train_seed.slurm b/stage_two/run_train_seed.slurm index bc2eeee5..1d5aa104 100644 --- a/stage_two/run_train_seed.slurm +++ b/stage_two/run_train_seed.slurm @@ -43,7 +43,12 @@ 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. -SEED="${SEED:?set SEED to the replicate's seed, e.g. SEED=1}" +# 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}" 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 From 455e1c4d68af95d0af4a3713c63c2b2476c90f6f Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Fri, 4 Sep 2026 13:52:18 -0700 Subject: [PATCH 3/4] Two of the seed sweep's own guards were vacuous; fix them and the reading rule (#51, #135) Review of PR #155. Three findings verified by mutation, all silent. The test that guards the campaign's central invariant did not guard it. test_train_sampler_seed_is_derived_not_hardcoded asserted "sampler_seed_for(args.seed)" in src against the whole file, and train.py already contains that string in a log line. Deleting seed= from the DistributedSampler call -- which is exactly the regression rampnet/seeding.py exists to prevent, and which would make all three replicates share one data order -- left the suite green. It is now asserted on the parsed statement. Two smaller versions of the same mistake: "random.seed(args.seed)" in src is satisfied by np.random.seed(args.seed), so the stdlib call that drives the horizontal-flip augmentation could be deleted unnoticed; and the Tillicum positional-argument count matched only "$VAR", so inserting "${VAR}" mid-list shifted every later field by one and passed. Both now fail as intended. The pre-registered decision rule divided 0.039 by Campaign A's SD alone. That gap is a difference between two single-seed runs, so its standard error is sqrt(s_A^2 + s_B^2). At s_A = 0.010 the table read 4 sigma; with s_B = 0.010 it is 2.8, and with s_B = 0.020 it is inside the band the same table calls indistinguishable -- the conclusion could invert with no change to any input the rule looked at. Campaign B was measuring s_B concurrently and had no reading of its own at all, leaving half the campaign free to be interpreted afterwards. Amendment 1 corrects the sigma, makes the bands disjoint at their endpoints, gives Campaign B a rule against #135's measured 0.0063 paired MDE, and says what happens if B does not finish in time. The original table is kept verbatim. No result from either campaign had been scored when this was written. Also: the doc's headline number, its source document and the script that computes its statistic all live on PR #154, not on main, which is now stated rather than left as a dead link; neither campaign had a written path from its checkpoints to the statistic, and both gaps are named; the Campaign A reproduce command omitted the PYTHON= that Tillicum needs; and Campaign B writes its only artifact to /gscratch/scrubbed, which purges on a ~21-day idle window while the same document says the campaign's calendar is unbounded. Launcher changes are additive only, since both campaigns are in flight: an optional RAMPNET_ENV escape hatch (default path unchanged), a world-size warning, and header notes. The RUNDIR default is deliberately NOT moved off scrubbed -- that would put a resubmission somewhere different from its queued siblings, so the required copy-out is documented instead and the move left as a decision. Co-Authored-By: Claude Opus 5 --- docs/seed_variance_51_135.md | 169 ++++++++++++++++-- rampnet/seeding.py | 8 + .../run_yolo_train_tillicum.slurm | 5 +- stage_two/run_train_seed.slurm | 49 ++++- stage_two/train.py | 5 +- tests/test_seeding.py | 98 ++++++++-- 6 files changed, 301 insertions(+), 33 deletions(-) diff --git a/docs/seed_variance_51_135.md b/docs/seed_variance_51_135.md index 6ee16b10..29b1c745 100644 --- a/docs/seed_variance_51_135.md +++ b/docs/seed_variance_51_135.md @@ -5,11 +5,27 @@ below were written first, so the interpretation cannot be chosen after seeing th 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 +**#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 @@ -80,11 +96,23 @@ 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. Replicates run the full `epochs=60` schedule 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. +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: +**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 | |---|---| @@ -95,12 +123,103 @@ schedule instead of the run would have confounded the comparison. **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) + +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 a cheap extension - (~$119 each) and are the pre-registered response if the result lands in the ambiguous - band. They are not being run up front. + 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 @@ -111,6 +230,15 @@ The ≥0.020 branch is a live possibility, not a formality. (#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 @@ -122,16 +250,33 @@ Both launchers take the seed as an environment variable and record it in the job ```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). +# 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 reproduces published runs exactly (`sampler_seed_for(42) == 0`) and that the -Tillicum launcher's positional argument list still lines up with its unpack, an -off-by-one that would silently train at the wrong seed. +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 index 8ff8cbfa..913c9e38 100644 --- a/rampnet/seeding.py +++ b/rampnet/seeding.py @@ -37,6 +37,14 @@ def sampler_seed_for(seed: int) -> int: 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 diff --git a/scripts/model_comparison/run_yolo_train_tillicum.slurm b/scripts/model_comparison/run_yolo_train_tillicum.slurm index a55a2dde..349fc9f2 100644 --- a/scripts/model_comparison/run_yolo_train_tillicum.slurm +++ b/scripts/model_comparison/run_yolo_train_tillicum.slurm @@ -89,8 +89,9 @@ 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) it is the binding question, and #51's own rule is -# that differences under ~0.02 should not be read. +# (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 diff --git a/stage_two/run_train_seed.slurm b/stage_two/run_train_seed.slurm index 1d5aa104..afaf2892 100644 --- a/stage_two/run_train_seed.slurm +++ b/stage_two/run_train_seed.slurm @@ -17,10 +17,23 @@ # 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 +# 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 @@ -59,8 +72,28 @@ EPOCHS="${EPOCHS:-1}" # the published recipe is 1 epoch / 9,378 steps (#8 mkdir -p "$RUNDIR/checkpoints" "$REPO/logs" -echo "Loading Conda environment..." -source activate sidewalkcv2 +# 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 @@ -83,10 +116,18 @@ 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 \ + "$TORCHRUN" --nnodes $SLURM_NNODES \ --nproc_per_node $NPROC_PER_NODE \ --rdzv_id $SLURM_JOB_ID \ --rdzv_backend c10d \ diff --git a/stage_two/train.py b/stage_two/train.py index 4fcc00e8..a568cbef 100644 --- a/stage_two/train.py +++ b/stage_two/train.py @@ -274,7 +274,10 @@ def __getitem__(self, idx): # 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 -# reproduces those runs byte for byte, while any other --seed moves the data order too. +# 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, diff --git a/tests/test_seeding.py b/tests/test_seeding.py index d0e212bf..89afdada 100644 --- a/tests/test_seeding.py +++ b/tests/test_seeding.py @@ -7,6 +7,7 @@ number in it. """ +import ast import re from pathlib import Path @@ -24,6 +25,37 @@ 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 # -------------------------------------------------------------------------------- @@ -42,6 +74,19 @@ def test_every_other_seed_maps_to_itself(seed): 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.""" @@ -64,28 +109,50 @@ def test_train_py_declares_seed_defaulting_to_the_historical_value(): 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.""" + 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 call in src, f"{call} missing" + 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 test_train_sampler_seed_is_derived_not_hardcoded(): - """The regression this guards: dropping the sampler seed (back to a constant 0) and - leaving a sweep that varies initialization only.""" - src = TRAIN_PY.read_text(encoding="utf-8") - sampler_line = next(l for l in src.splitlines() - if "DistributedSampler(train_dataset" in l or - (l.strip().startswith("seed=sampler_seed_for"))) - assert "sampler_seed_for(args.seed)" in src - assert "shuffle=True" in src - # and the val sampler must NOT be shuffled, so it needs no seed - val_line = next(l for l in src.splitlines() if "val_sampler = DistributedSampler" in l) - assert "shuffle=False" in val_line + """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 = _assigned_call("train_sampler") + assert isinstance(call.func, ast.Name) and call.func.id == "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(): @@ -140,7 +207,10 @@ def test_tillicum_launcher_positional_args_line_up(): src = TILLICUM_SLURM.read_text(encoding="utf-8") call = src.split("run_train() {", 1)[1].split("<<'PY'", 1)[0] - n_passed = len(re.findall(r'"\$[A-Z_]+"', call)) + # 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" From 24535344326abcbf9b6785fc5bce097b1d4be2ce Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Fri, 4 Sep 2026 14:37:11 -0700 Subject: [PATCH 4/4] Record that Amendment 1 was ratified, and when (#51, #135) An amendment to a pre-registration is only worth the audit trail attached to it. This records who ratified it and the one fact that decides whether it is legitimate: it was ratified on 2026-09-04, before any replicate from either campaign had been scored. Nothing about the rule changes. The cut points remain 0.010 and 0.020 as originally written; the amendment applies them to sqrt(s_A^2 + s_B^2) rather than s_A alone. Co-Authored-By: Claude Opus 5 (1M context) --- docs/seed_variance_51_135.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/seed_variance_51_135.md b/docs/seed_variance_51_135.md index 29b1c745..e67d6c60 100644 --- a/docs/seed_variance_51_135.md +++ b/docs/seed_variance_51_135.md @@ -125,6 +125,8 @@ 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