diff --git a/CLAUDE.md b/CLAUDE.md index 76c4703..2ce838d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -62,13 +62,26 @@ repeats the playbook, you are re-litigating — record the delta in the playbook instead. When a playbook claim turns out wrong, don't just fix it: add a dated row to its superseded-claims table so old docs stop misleading people. -## Training throughput +## Training runs -Same contract as quantization, different subject: **`docs/pfi/training-throughput-playbook.md` -is the durable home** for why a training run is slow — the 10-minute scaling -triage that names the regime before you profile, the padding/masking landmines, -the profiler traps, and its own superseded-claims table. Read it before -hypothesising about kernels. +Same contract as quantization, different subject: +**`docs/pfi/training-throughput-playbook.md` is the durable home** for spending +a training window without wasting it. Two halves, and you want different ones at +different moments: + +- **§1–§3, why a run is SLOW** — the 10-minute scaling triage that names the + regime before you profile, the padding/masking landmines, the profiler traps, + the serving-path and base-viability pre-flights. Read before hypothesising + about kernels. +- **§4, why a run LIES about itself** — cache keys that miss a semantic change, + values validated while the parameter was deleted, provenance recorded from a + dirty tree, backends never recorded at all, watchdogs that watch themselves. + **Read §4 before you launch**, and run its §4.8 checklist. Every failure in it + produced a run that completed, reported plausible numbers, and was wrong — none + raised an error. + +(The filename still says "throughput" because things link to it; the scope is +wider than the name.) The instruments are committed at [`scripts/training-probes/`](scripts/training-probes/) with raw output kept alongside, so the claims can be re-derived rather than @@ -77,7 +90,10 @@ taken on faith. ⚠ **Measure before you argue.** The playbook exists because a four-model frontier panel produced four self-retractions in ninety minutes on this question, and every one of them was a derivation while every survivor was a -measurement. +measurement. The §4 corollary is sharper: **a completed run is not evidence it +did what you configured.** Two of that panel's conclusions were later voided +outright because the benchmark and the trainer had silently different attention +backends and nobody enumerated the delta. ## Purpose diff --git a/docs/pfi/gemma4-erp-tune-sizing.md b/docs/pfi/gemma4-erp-tune-sizing.md index a75d6d6..1b03e3f 100644 --- a/docs/pfi/gemma4-erp-tune-sizing.md +++ b/docs/pfi/gemma4-erp-tune-sizing.md @@ -364,6 +364,38 @@ Model-agnostic lessons from this investigation are in probes are at [`scripts/training-probes/`](../../scripts/training-probes/). What follows is Gemma-4-specific. +> ## ⚠⚠ CORRECTION 2026-08-26 — MUCH OF THIS SECTION MEASURES THE WRONG PROCESS +> +> **The benchmarks below were run against the SERVING seat with +> `attn_implementation="sdpa"` set explicitly. Training was running +> `flex_attention` the whole time.** `ATTN_IMPLEMENTATION = "flex_attention"` +> was a module constant passed unconditionally into `from_pretrained`, and +> run 1's step-time distribution (n=1,445; min 11.84 / p50 19.75 / p99 30.52 / +> max 45.79 s/it, the max being step 1's compile) confirms it stayed compiled — +> a dynamo fallback sits in the hundreds of seconds per step. +> +> **FALLS** — describes sdpa, not the training run: +> the three-point scaling fit and its 68% quadratic share; the kernel table +> (`fmha_cutlassF/B` sm80, `EFFICIENT_ATTENTION`, attention 65.2%); the **8.6% +> MFU** figure quoted above and throughout; the projection that elementwise +> becomes the largest line item post-fix; and "adopt `flex_attention`" as the +> round-two headline lever — **which round one already had.** +> +> **SURVIVES** — measured on the live training run: +> the padding/bucketing win (44.3 → 20.1 s/it); the zero-pad fast-path +> second-order effect; the eval-battery noise-floor work. +> +> ⚠ **Do not assume the direction of the correction.** Training's real MFU is +> *unmeasured*, not obviously better. Flex with a BlockMask ought to beat +> dense-masked sdpa, but that is a prediction and this investigation has been +> unkind to those. +> +> The root cause was procedural, not technical, and it is written up as +> playbook **§4.3**: run 1 recorded no attention backend in its provenance, so +> the benchmark/trainer delta was invisible and nobody enumerated it. Run 2 +> onward records `attn_implementation_requested` **and** `_resolved`, plus the +> torch/transformers versions and dynamo's compile counters. + ### 6.1 Where the step time goes Real checkpoint, GPU0, `attn_implementation="sdpa"`, PEFT + gradient diff --git a/docs/pfi/training-throughput-playbook.md b/docs/pfi/training-throughput-playbook.md index 4407ce2..9ed0bfa 100644 --- a/docs/pfi/training-throughput-playbook.md +++ b/docs/pfi/training-throughput-playbook.md @@ -1,22 +1,33 @@ -# Training throughput playbook — how to find where the step time went +# Training playbook — spending a training window without wasting it _Sibling to [`model-quantization-playbook.md`](model-quantization-playbook.md). -That one is for making a model small; this one is for making a training run -fast. Same contract: **model-agnostic lessons live here, model-specific ones -stay in the per-model artifact and link up.**_ +That one is for making a model small; this one is for spending a training +window well. Same contract: **model-agnostic lessons live here, model-specific +ones stay in the per-model artifact and link up.**_ -First written 2026-08-24 out of the Gemma-4 26B-A4B ERP/RP tune, which ran at -8.6% MFU and cost a four-model frontier panel and most of a night to explain. -The worked example in §7 is that run. The lessons above it are not about +First written 2026-08-24 as a throughput playbook, out of the Gemma-4 26B-A4B +ERP/RP tune that ran at 8.6% MFU and cost a four-model frontier panel and most +of a night to explain. **§1–§3 are still that**: how to find where the step +time went. **§4 is the other half**, added 2026-08-26 — how to keep a run's +artifacts from lying about what they are. The filename still says +`training-throughput-playbook.md` because things link to it; the scope is +wider than the name. + +The worked example in §8 is that first run. The lessons above it are not about Gemma-4. -> **Read this before hypothesising about kernels.** The single most expensive +> **Read §1–§3 before hypothesising about kernels.** The single most expensive > failure in that investigation was not a wrong hypothesis. It was *four > people, including four frontier models, reasoning confidently from > arithmetic instead of spending ten minutes on a measurement that settled > it.* Two of the panel's conclusions were retracted by their own authors > within the hour. Every retraction was a derivation; every survivor was a > measurement. +> +> **Read §4 before you launch.** Every failure in it produced a run that +> completed, reported plausible numbers, and was wrong about itself. None +> raised an error. Two of them cost a panel and a night *after* the fact, +> chasing a configuration the run was already in. --- @@ -450,7 +461,346 @@ the free discriminator: **if both arms return zero deflections the model is binary; if only one does, the regex is fine and the difference is real.** An instrument artifact does not care which arm it runs against. -## 4. Panel / consult discipline for perf work +## 4. When the artifact lies about itself + +§1–§3 are about a run that is *slow*. This section is about a run that +**completes, reports plausible numbers, and is wrong** — and about the derived +artifacts that go on repeating the wrong thing afterwards. + +Every failure below was found on the Gemma-4 ERP/RP tune between 2026-08-24 and +2026-08-26. **Not one of them raised an error.** They are ordered by how much +they cost. + +> **The shape they share**, stated by brokkr-smithy-dev on 2026-08-26 after the +> third instance in a day: *when you change what an artifact means, every +> derived artifact keyed on the old meaning is now a liar.* Caches, logs, +> comments, manifests, benchmark write-ups. The fix is always the same — put the +> meaning in the key — and the reason it keeps happening is that the old +> artifact still loads, still parses, and still looks right. + +### ⚠ 4.1 ⭐⭐ A cache key must cover the MEANING of the cached thing, not just its inputs + +The encode cache for the ERP corpus was keyed on corpus identity, `max_seq_len`, +base-model path and chat-template sha. Run 2 then added an **impersonation +loss-mask** — 813 turns whose labels change from trained to ignored. + +The mask was not in the key. Run 2 would have hit run 1's 609 MB cache, reused +its **unmasked** encodings, trained the impersonation straight back in, and +written `impersonation_mask_sha256` into its own provenance manifest while doing +it. + +Nothing downstream could have caught it: + +| signal | what it would have shown | +|---|---| +| error / exception | none — a cache hit is the happy path | +| sample count | unchanged — 0 samples were fully masked | +| record count | unchanged — 20,982 either way | +| loss curve | normal | +| provenance manifest | asserts the mask was applied | + +A seven-hour run whose artifact claims a property it does not have, invisible +from every number anyone would think to check. + +**The rule.** A cache key covers every input that can change the *semantics* of +the output, not just the ones that change its *shape*. Concretely: + +- Hash the **content** of every auxiliary file (mask, filter list, label map), + not its path. A path is stable across an edit; that is the whole problem. +- Carry an explicit **`ENCODE_VERSION`** integer and bump it whenever the + encoder's output can change for identical inputs. Cheap, blunt, and it + catches the cases you did not think to hash. +- **Extract the key computation into a named, tested function.** Ours became + `core.encode_cache_key` specifically so a test could assert that a masked run + and an unmasked run get different keys — and that a legitimate resume still + gets the same one. An inline dict comprehension inside the loader cannot be + tested and will not be. + +```python +def test_encode_cache_key_separates_a_masked_run_from_an_unmasked_one(): + unmasked = encode_cache_key(ordered, **common) + masked = encode_cache_key(ordered, **common, impersonation_mask_sha256="d"*64) + assert unmasked != masked + # and the cache must still HIT on a legitimate resume + assert masked == encode_cache_key(ordered, **common, impersonation_mask_sha256="d"*64) +``` + +**Generalises past caches.** Any memoised, derived, or checkpointed artifact has +this problem: encode caches, tokenised datasets, precomputed embeddings, +distillation logits, eval-result files. If it was derived under one meaning and +is reused under another, it is now a liar. + +### ⚠ 4.2 ⭐⭐ Validating a VALUE is not validating the PARAMETER + +Run 2 died after the full encode and after all 1,013 weight shards had loaded: + + TypeError: TrainingArguments.__init__() got an unexpected keyword + argument 'warmup_ratio' + +`warmup_ratio` exists in transformers 4. It is **gone in 5.15.1**, which keeps +only `warmup_steps`. The harness had careful config-level validation — it +checked `0.0 <= warmup_ratio < 1.0` and rejected an unknown scheduler name — and +none of it could have caught this. **The value was in range. The parameter had +been deleted.** + +The check that catches it has to run against the **installed library**, not +against your own schema: + +```python +def assert_training_arguments_accepts(kwargs: dict, cls) -> None: + accepted = set(inspect.signature(cls.__init__).parameters) + unknown = sorted(set(kwargs) - accepted) + if unknown: + raise TypeError(f"{cls.__name__} does not accept {unknown}. " + f"It accepts: {', '.join(sorted(accepted))}") +``` + +Which requires building the kwargs as **data** first — *you cannot check the +argument list of a call you have already made.* That single structural change is +the lesson; the signature diff is trivial once the kwargs are a dict. + +Three riders: + +- **Keep the portable unit in config, convert at the call site.** We kept + `warmup_ratio` in the config file and convert to `warmup_steps` on the wire. + A ratio survives a change in corpus size; a step count silently becomes a + different fraction of the run. +- **Mirror the framework's own arithmetic exactly.** HF ceilings *twice* — + records into micro-batches, then micro-batches into optimizer steps. One + combined division gave 1,311 steps for our corpus where the trainer reports + 1,312, which would have put warmup a step short. Pin it with a test against a + step count a real run actually printed. +- **The failure mode is the expensive one: late.** Anything checkable from the + config, the library signature, or a file on disk belongs *before* the + tokenizer, the encode, and the model load. Ours now dies in under a second. + +### ⚠ 4.3 ⭐⭐ Record what the run RESOLVED to, never what it requested + +Run 1's provenance recorded no attention backend at all. Six weeks of +conclusions rested on the answer. + +An MFU investigation had profiled the **serving seat** with +`attn_implementation="sdpa"` explicitly set, produced a kernel table +(`fmha_cutlass*_sm80`, `EFFICIENT_ATTENTION`, attention 65.2% of step), an 8.6% +MFU figure, and a headline recommendation: **adopt `flex_attention` for round +two.** + +Training had been running `flex_attention` the entire time. + + ATTN_IMPLEMENTATION = "flex_attention" # module constant + ...from_pretrained(..., attn_implementation=ATTN_IMPLEMENTATION) + +A panel, a kernel profile and three rounds of arithmetic went toward a +configuration the run was already in. The single biggest round-two optimisation +identified was a thing training already had. + +**The correction has to be stated as a split, because some of the work +survives.** After the flex finding, on this investigation: + + FALLS — describes the sdpa seat, not the training run + the three-point scaling fit and its 68% quadratic share + the kernel table + the 8.6% MFU number + "adopt flex_attention" as the round-two headline lever + + SURVIVES — measured on the live training run + the padding/bucketing win, 44.3 -> 20.1 s/it + the zero-pad fast-path second-order effect + the eval-battery noise-floor work (a different instrument entirely) + +⚠ **Do not assume the direction of the correction.** Training's real MFU is now +*unmeasured*, not obviously better. Flex with a BlockMask ought to beat +dense-masked sdpa — but that is a prediction, and predictions have done badly +here. + +**What to record, and why two fields:** + +```python +"attn_implementation_requested": ATTN_IMPLEMENTATION, # a constant +"attn_implementation_resolved": model.config._attn_implementation, # what happened +"torch_version": torch.__version__, +"transformers_version": transformers.__version__, +"dynamo_counters": _dynamo_counters(), # best-effort, nullable, never raises +``` + +Requested and resolved are **different claims**. The requested value is a line +in your source; the resolved value is what the library settled on after checking +availability, and only that one describes the run. A framework that silently +downgrades an unavailable backend will make them differ, and that difference is +exactly what you want on the record. + +The dynamo counters matter for the same reason: dynamo's fallback to +**uncompiled** flex is silent, roughly 20× slower, and documented not to work +correctly through the backward pass. The counters are the only in-band evidence +it did not happen. + +**When the run is already going and the field is missing** — as ours was — you +can often still answer it, but only forensically. For us: + +1. **Source**, for what was requested (unconditional constant, no fallback flag). +2. **A config-only side probe** on the same library versions, for what it + resolves to: `AutoConfig.from_pretrained(path, attn_implementation=...)` then + read `_attn_implementation`. No weights, no GPU, seconds. +3. **The step-time distribution**, for whether it stayed compiled. Run 1's + 1,445 logged steps ran min 11.84 / p50 19.75 / p99 30.52 / max 45.79 s/it, + the maximum being step 1's compile. A dynamo fallback would sit in the + hundreds of seconds per step. Nothing in the trace approaches it. + +That is three sources of evidence to replace one recorded field, and it only +worked because the source tree happened to still be on disk. Record the field. + +### ⚠ 4.4 ⭐⭐ Never train from a dirty tree — the provenance commit will be a lie + +Run 1's manifest recorded `harness_commit 35a4e8e`. The working tree carried +**224 uncommitted lines** across two modules for the entire run, so the recorded +commit predates the code that trained the adapter. The artifact is **not +reproducible from the commit it names**, and nothing says so. + +`_git_commit()` calling `git rev-parse HEAD` is not wrong; it is *incomplete*. +It answers "what is HEAD" when the question is "what code ran." + +**Add a cleanliness assertion to the pre-flight**, beside the corpus and holdout +checks: + +```bash +git diff --quiet && git diff --cached --quiet || { + echo "REFUSING: working tree is dirty; harness_commit would not describe this run" + exit 1 +} +``` + +Record `git describe --always --dirty` rather than a bare SHA if you want a +softer version, but an outright refusal is better: a run long enough to be worth +provenance is long enough to be worth one commit first. + +**Correcting it after the fact — annotate, never edit.** We left +`provenance.json` untouched and wrote a `PROVENANCE-NOTE.md` beside it. Editing +a shipped artifact so it says something it did not say is the worse failure. + +⚠ **State what is NOT wrong.** A bare correction note casts doubt over every +field it does not mention, and the next reader has no way to tell which. Ours +ends by listing what remains accurate — recipe sha, root shas, base model path +and revision, template sha, the override triple, the dropped ids. + +### ⚠ 4.5 ⭐ A watchdog whose pattern matches its own argv can only ever return "alive" + +The training monitor polled liveness with: + +```bash +while pgrep -f "erp_sft_harness --config" >/dev/null; do sleep 60; done +echo "PROCESS EXITED" +``` + +`pgrep -f` matches full command lines — **including the monitor's own**, because +the pattern is right there in its argv. The loop matched itself, so the exit +branch was unreachable **for every possible input**. The run crashed and the +watchdog reported nothing, because it was watching itself. + +It also blocked the recovery: the launcher's already-running guard used the same +pattern, matched the monitor, and refused to start the replacement run. + +**This is the inert-gate shape in a liveness check** — a test that cannot return +the failing verdict for any input. It is the same defect as an assertion +comparing a value to itself, and it hides better, because a watchdog that never +fires looks exactly like a system that never breaks. + +Fixes, in order of preference: + +```bash +RUNPID=$(pgrep -f "" | head -1) # resolve ONCE, from a clean shell +while kill -0 "$RUNPID" 2>/dev/null; do sleep 60; done +``` + +- **Poll a captured PID, not a pattern.** `kill -0` cannot self-match. +- If you must pattern-match, **break the literal** so it is absent from your own + argv (`"erp_sft_harn""ess --config"`), and put the guard in a **file** rather + than an inline `ssh host '...'` — the invoking command line is argv too. +- Never `pkill -f` a shared pattern: it is handle-blind and kills every other + agent's monitor on the box along with yours. + +**Test the negative.** Whatever the gate is, construct the input that should +make it fire and confirm that it does. Every gate in this project that has ever +caught anything was one somebody deliberately broke first. + +### ⚠ 4.6 ⭐ An instrument nobody runs is not an instrument + +The harness test suite was **10 passed / 4 failed**, and there was **no pytest +installed in the training venv at all** — so nothing had ever run it. A run had +already shipped an adapter through it. + +The good version of that news: all four failures were *stale tests*, not broken +code. Each asserted a behaviour that had been deliberately changed — +`chat_template_path` became required, the provenance schema grew three keys, +persona trimming gave way to the unfittable path. The bad version: nobody knew +that, because the instrument was dark. + +- **Installing the test runner is part of standing up the training venv**, not a + later nicety. It is three pure-Python packages and touches nothing in the + torch/transformers stack. +- **Repair stale tests to the current contract; do not delete them.** Each + rewrite is an opportunity to write down *why* the behaviour changed — ours now + carry the corpus measurement showing the removed persona-trimming costs zero + samples on this corpus. +- **Mutation-check any test guarding an invisible property.** A test for a + silent failure is itself silent when it is vacuous. Break the code + deliberately and confirm the test fails: + +| deliberate break | test that must fail | +|---|---| +| OR-merge → last-wins | whole-run masking | +| drop the sha assertion | corpus/mask disagreement | +| resolve indices after the merge | source-index resolution | +| re-add the removed kwarg | installed-signature check | + +If breaking it changes nothing, you have documentation, not a test. + +### ⚠ 4.7 ⭐ Fix a stale measurement at the SOURCE, or the copy carries it forward + +A launcher script carried the comment *"the 609 MB encode cache (2.5 min to +reuse, ~4.3 HOURS to rebuild)."* That figure predated the encoder's own +parallelisation. Measured on run 2: **145.5 seconds** on 32 workers. Off by a +factor of 106. + +It was believed twice — once to project an 11.7-hour window for a 7.6-hour job, +and once when it was copied verbatim into a **new** launcher written by the same +person who had just measured the real number. **The stale figure propagated in +the same motion that was supposed to retire it.** + +- When a measurement in a comment is superseded, `grep` the repo for the number + before you fix the one in front of you. +- Copying a header wholesale copies its claims wholesale. Re-read boilerplate + you paste for facts that have expired. +- Prefer a **dated** measurement in prose — "145.5 s on 32 workers, measured + 2026-08-26" — over a bare figure. A dated claim invites a re-check; a bare one + reads as timeless. + +### 4.8 The pre-launch honesty checklist + +Ten minutes, before the window opens. Every item is something that produced a +completed, plausible, wrong run above. + +``` +[ ] working tree committed git diff --quiet && git diff --cached --quiet +[ ] test suite green and the runner is actually installed +[ ] cache key covers the change bump the version integer; hash aux file CONTENT +[ ] kwargs checked by NAME against the installed library signature +[ ] config validated before tokenizer, encode and model load +[ ] provenance records RESOLVED backend, library versions, aux-file shas +[ ] present-and-null, not absent a run that claims nothing must say so explicitly +[ ] watchdog tested negative kill something and confirm it fires +[ ] stale numbers grepped for the figure, repo-wide, not just in view +``` + +⚠ **The present-and-null line is load-bearing and the least obvious.** Emit +every provenance key always, `null` when unused. A manifest that *omits* a key +when there was nothing to report is indistinguishable from one written by a +harness too old to know the key exists — so an adapter trained without your +safeguard becomes byte-indistinguishable from one trained with it. Present-and- +null is a statement; absent is an accident. + +--- + +## 5. Panel / consult discipline for perf work Perf investigations are unusually good at generating confident wrong answers, because the arithmetic is easy and the ground truth is expensive. Specific @@ -470,7 +820,7 @@ guards, learned the hard way: --- -## 5. Superseded claims — do not follow these +## 6. Superseded claims — do not follow these | claim | status | replaced by | |---|---|---| @@ -480,8 +830,12 @@ guards, learned the hard way: | The chunked CE is a 2–5× under-estimated cost centre | **WRONG** (2026-08-24) | Measured 1.1% of step forward, ≲3% with recompute | | `attn_implementation="flash_attention_2"` is the per-layer lever | **NOT A FLAG** (2026-08-24) | All-or-nothing at `from_pretrained`; per-layer needs a custom fn on `ALL_ATTENTION_FUNCTIONS`. FA2 also caps head_dim at 256. | | Bucket size ~256 is needed to preserve source diversity | **UNNECESSARY** (2026-08-24) | Diversity is flat in bucket size; the global micro-batch shuffle does that work — see §3.2 | +| The 2026-08-24 kernel table / 68% quadratic share / 8.6% MFU describe the TRAINING run | **WRONG** (2026-08-26) | They describe the **serving seat**, benchmarked with `attn_implementation="sdpa"` set explicitly. Training ran `flex_attention` throughout. Training's real MFU is now *unmeasured* — see §4.3 for the full falls/survives split, and do not assume the correction's direction. | +| "Adopt `flex_attention`" is the round-two headline lever | **ALREADY BANKED** (2026-08-26) | It was live in round one. A panel, a kernel profile and three rounds of arithmetic went toward a configuration the run was already in — see §4.3 | +| The encode cache takes ~4.3 hours to rebuild | **WRONG** (2026-08-26) | **145.5 s** on 32 workers, measured on run 2. The stale figure predated the encoder's own parallelisation, was used to project an 11.7 h window for a 7.6 h job, and was then copied verbatim into a new launcher — see §4.7 | +| `warmup_ratio` is a valid `TrainingArguments` kwarg | **REMOVED IN transformers 5** (2026-08-26) | 5.15.1 keeps only `warmup_steps`. Keep the ratio in config, convert at the call site, and diff kwarg NAMES against the installed signature — see §4.2 | -## 6. Measured negatives — don't re-chase +## 7. Measured negatives — don't re-chase - **Fused MoE kernel (`grouped_mm`) as the throughput fix.** Measured 0.9% *slower* than the Python loop and bit-identical. Independently, dense GEMM is @@ -498,7 +852,7 @@ guards, learned the hard way: --- -## 7. Worked example — Gemma-4 26B-A4B ERP/RP tune, 2026-08-24 +## 8. Worked example — Gemma-4 26B-A4B ERP/RP tune, 2026-08-24 Model-specific detail lives in [`gemma4-erp-tune-sizing.md`](gemma4-erp-tune-sizing.md) §6. The short version,