policy: rank levers from what previous runs measured - #120
Open
Gaurang-2005 wants to merge 5 commits into
Open
Gaurang-2005 wants to merge 5 commits into
Gaurang-2005 wants to merge 5 commits into
Conversation
The loop has been able to read its own past since GitM-Labs#118 and nothing consulted it. `select_interventions` scored every candidate as `coverage x expected_delta_mean` — a hand-authored constant, identical on every run no matter what the last one measured — so a lever that lost 9% last night ranked exactly where it did before anyone ran it. Substitution, not a blend. `expected_delta_mean` is an estimate of the lever's effect; a delta the lever actually recorded on this GPU is a better estimate of that same quantity, so it replaces it and coverage — a property of this trace — is untouched. There is no weighting constant between prior and measurement, because a number like `K = 2` reads as calibrated and is not; `playbook/match.py` refuses to pick such a threshold and says so, and this follows it. Ranking stays a precedence tuple for the reason that module gives: terms answering different questions should not collapse into a scalar where one can quietly outvote another. Four terms now — rejected -> not worth a run -> demoted -> magnitude, then name "Not worth a run" sits above the demotion, and finding that out is what the end-to-end check was for. With the demotion higher, a lever measured at -9% in every run outranked a conflicted lever estimated at +1.4%, because the loser's record did not disagree with itself. That spends the run on a result already in hand. Estimate sign first, then consistency among the levers that might actually help. A conflicted record demotes but never removes. Won twice and lost twice is not a measured neutral: it behaved differently under conditions the record does not capture, which makes it the weaker bet while that holds. The demotion lifts by itself once the record stops disagreeing, because it describes the evidence rather than the lever. No SKU means no substitution. A result measured on an H100 says nothing about an MI355X, and scoring one from the other is the single mistake the record's GPU key exists to prevent — so an unnamed box keeps the prior rather than guessing which record applies. Off by default, on `Policy.use_history` and `LoopConfig.use_history`, matching the two flags already on Policy. Merging changes no ranking; turning it on is its own decision, and the loop writes `history_read.json` beside the other run artifacts so what the ranking saw is on the record. History is passed in rather than read from disk inside the ranker, which keeps selection a pure function of its arguments. Validation: 1266 passed, 1 skipped, exit 0; Ruff clean. Nine new tests. Also driven end to end against a real runs/ directory from generate_demo_runs.py: with the flag off all three levers score an identical +0.0167 from the constant; with it on they separate to +0.1550 measured, +0.0144 measured and demoted, and -0.0300 measured. No run against a live GPU. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`use_history` was a flag someone had to know existed, defaulting to off, which meant the record stayed unread unless a caller went looking for it. The run now asks: where previous runs left measured results and there is a terminal to ask at, it says how many and offers to rank from them. Three notes on what the question is, because the obvious reading of it does not match how runs are stored. Nothing can be overwritten. Each run writes into `runs/<uuid4>/`, so a new run never lands on an existing `verification.json` and there is no collision to resolve. The choice is only whether this run *reads* the earlier exports. Declining skips them for this run and leaves every one of them on disk — measurements that cost GPU time are not discarded by answering a prompt. No answer means yes. An unattended run — a 24h budget started over ssh, a cron job — must not sit on a prompt forever, and of the two answers using the record is the one that throws nothing away. Without a terminal there is nobody to ask, so it takes the same default immediately rather than waiting out sixty seconds against a pipe that will never reply. The question is put before the capture, not after it, so nobody is answering a prompt that arrived an hour into their run. `LoopConfig.use_history` becomes tri-state: None asks, True and False decide outright and are never second-guessed, which is what keeps scripted and scheduled runs deterministic. Validation: 1275 passed, 1 skipped, exit 0; Ruff clean. Nine new tests driving a real pipe rather than a StringIO, since the prompt waits on select and a stream that cannot be selected would test something the run never does. One of them pins that declining deletes nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The prompt went into `run_loop`, which put it on the wrong side of the library boundary. `gitm.optimize` is the embedded entry point — a vLLM process calls it in-line — and a function that can block on stdin is one such a caller cannot use. Nothing would have appeared on a terminal nobody was watching; the run would simply have stopped for sixty seconds in the middle of someone else's process, once per run, for a question they never asked to be asked. A prompt is a property of being run by a person at a terminal, so it now lives with the other things that are: `gitm run` asks before it calls optimize, and passes the answer down as an argument. The three layers each keep one job. `runs_with_results` in `optimizer/history` counts run folders that left an export, which is cheap enough to call before deciding whether there is anything worth asking about — it stats directories rather than parsing them. `gitm/cli` owns the question and the sixty-second timeout. `run_loop` takes a bool and never asks; unset now reads as off rather than as "go and find out". `--use-history` and `--no-history` decide it without a prompt, for scripted and scheduled runs, and an explicit flag is never re-litigated. Behaviour at the terminal is unchanged from the previous commit: the question is still put before any capture, silence still means use them, no tty still means use them, and declining still deletes nothing. Two of the new tests are structural rather than behavioural. One reads the scheduler's source and fails if `select.select` or a prompt helper ever drifts back into it; the other pins that `optimize` takes the answer as a parameter and contains no `input(`. Both guard the boundary rather than today's arrangement of it. Validation: 1276 passed, 1 skipped, exit 0; Ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| # Read once per run, filtered to this box. A lever measured on another GPU is | ||
| # not evidence about this one, and load_history counts what it filtered out | ||
| # rather than letting a thin record look like a weak lever. | ||
| prior_runs = load_history(runs_dir(cfg.scratch), gpu_sku=pctx.sku) if use_history else None |
There was a problem hiding this comment.
Malformed history can abort runs
When history is enabled, this makes every prior export part of the run-critical Phase 3 path, but the reader only verifies that each result is a dictionary. A malformed but parseable export can raise while computing speedup - 1.0, or pass a non-finite delta into ranking and JSON output. Because this happens after capture, one damaged old export can abort or corrupt an otherwise expensive run; validate and skip malformed result fields or isolate failures per export.
This comment has been minimized.
This comment has been minimized.
Two review catches, both live once the record started deciding which experiments run. Records keyed on (lever, gpu_sku) merged results from different models measured on the same box, which a shared scratch directory guarantees: runs/ is per-machine, not per-checkpoint. A +40% on kimi-k2.5 and a -20% on glm-5.2 came back as one record reading a confident +10%, describing neither — and not flagged conflicted either, because both were kept and significant, so both counted as wins. Nothing in the record said it was answering about a model nobody asked about. `fingerprint` is the third key. It already rides in every export's provenance and is exactly the right grain: qualification derives it from kernel mix and shapes, so the same model and config reproduce it and a different checkpoint does not. Asking with the wrong one now returns None, which falls back to the catalog estimate — the same shape as never having measured the lever at all, which is the truth in that case. `load_history` filters on it beside gpu_sku and counts the exclusions as filtered rather than skipped, and the rendered table gains a workload column, without which two rows differing only by model look like one row recorded twice. The reader also trusted its own numbers too far, and this now runs in Phase 3 — after the capture has been paid for. A string `speedup` raised TypeError out of the entire read, taking every sound run with it and aborting the run that had just spent its budget on a trace. A NaN delta passed the isinstance check, survived the mean, and reached json.dumps, which writes the literal NaN into `history_read.json` — not valid JSON, in an artifact whose only purpose is being read by something else. True passed it too, bool being an int subclass, and read as a measured +100%. A value that is not a real finite number is now absent rather than coerced, and the attempt still counts as won or lost: what is missing is the magnitude, not the result. That is the distinction this module already draws everywhere else. Validation: 1283 passed, 1 skipped, exit 0; Ruff clean. Seven new tests, including the two-model average, the string speedup that aborted the read, the NaN that produced invalid JSON, and True reading as +100%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI pins ruff 0.12.11 and lints the whole tree with `ruff check .`; my local run was 0.16.8 over `gitm/ tests/`. UP038 wants `int | float` rather than `(int, float)` in isinstance, and the rule was removed in 0.16, so the newer version had nothing to say about it. The code this replaced used the `|` form already — the tuple was mine. Behaviour is identical; only the spelling changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this closes
#118 gave the loop the ability to read its own past. Nothing called it.
select_interventionsstill scored every candidate ascoverage × expected_delta_mean— a hand-authored constant, identical on every run no matter what the last one measured — so a lever that lost 9% last night ranked exactly where it did before anyone had ever run it.This wires the record into the ranking, and asks once whether to use it. It is the return edge that makes the thing a loop rather than a pipeline:
Substitution, not a blend
expected_delta_meanis an estimate of a lever's effect. A delta that lever actually recorded on this GPU is a better estimate of the same quantity, so it replaces it. Coverage — a property of the current trace — is untouched.There is deliberately no weighting constant between prior and measurement. A number like
K = 2reads as calibrated and is not;playbook/match.pyrefuses to pick such a threshold and says so, and this follows it. For the same reason ranking stays a precedence tuple rather than one blended score — terms answering different questions should not collapse into a scalar where one can quietly outvote another:"Not worth a run" sits above the demotion, and finding that out is what the end-to-end check was for. With the demotion higher, a lever measured at −9% in every run outranked a conflicted lever estimated at +1.4%, because the loser's record did not disagree with itself. That spends the run re-confirming a result already in hand. Estimate sign first, then consistency among the levers that might actually help. Unit tests alone did not show this; driving it against a real
runs/directory did.What it looks like
Three levers with identical coverage, against a real history:
--no-history--use-historyenable_expert_parallelkv_cache_dtype_fp8enforce_eagerOff, it is a three-way tie broken alphabetically — which is the problem. On, they separate by evidence.
Four rules
The question, and where it lives
gitm runasks once, before any capture, when previous runs left results and there is a terminal to ask at:Nothing can be overwritten. Each run writes into its own
runs/<uuid4>/, so a new run never lands on an existingverification.json. The choice is only whether this run reads the earlier exports; declining leaves every one of them on disk. Measurements that cost GPU time are not discarded by a keystroke.No answer means yes. An unattended run must not sit on a prompt, and of the two answers using the record is the one that throws nothing away. Without a terminal there is nobody to ask, so it takes the same default at once rather than waiting out the timeout against a pipe that will never reply.
The prompt is in the CLI, not the loop —
17c619cmoves it there.gitm.optimizeis the embedded entry point a vLLM process calls in-line, and a function that can block on stdin is one such a caller cannot use. Nothing would have appeared on a terminal anyone was watching; the run would simply have stopped for sixty seconds inside someone else's process.--use-history/--no-historydecide it without a prompt for scripted runs, and an explicit flag is never re-litigated.Behaviour change on merge, stated plainly
For
gitm run, the effective default becomes on, since silence means yes. Foroptimize()/run_loop(), it stays off unless explicitly passed — an embedded caller's ranking does not change because of an upgrade.Not in this PR
deviation_table.pystill has no consumer. This PR shares no file with it, imports nothing from it, and branches from the same commit — the two are independent and can merge in either order.largest_residualis untouched; autoresearch still ranks by mean fractional overshoot.Validation
ruffclean acrossgitm/andtests/.select.selector a prompt helper ever drifts back into it; the other pins thatoptimizetakes the answer as a parameter and contains noinput(. Both guard the boundary rather than today's arrangement of it.runs/directory fromgenerate_demo_runs.py(merged in history: read previous runs' measured results back #118) — that is where the precedence bug above surfaced.os.pipe(), not aStringIO: the prompt waits onselect, and a stream that cannot be selected would be testing something the run never does.🤖 Generated with Claude Code
The PR is not yet safe to merge because malformed historical metadata can still abort a run when history is enabled.
Findings
Summary
This PR makes historical optimization results available to intervention ranking while preserving current-trace coverage and isolating measurements by GPU and workload fingerprint.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Previous verification exports] --> B[Load history] B --> C{GPU SKU and fingerprint match?} C -- No --> D[Ignore record] C -- Yes --> E[Validate finite delta] E --> F[Rank interventions] G[Current trace coverage] --> F H[Catalog estimate] --> F F --> I[Selected experiments] I --> J[New verification export] J --> AReviews (3) · Last reviewed commit: "history: the isinstance form ruff 0.12 a..."