feat(keep): --max-latency-ms as a constraint on every promotion

[rpoornac]

Why

The optimizer maximized output_throughput and nothing else. Latency was
measured, reported and fed to the prompts, but no latency number could block a
KEEP.

That is survivable for a lever that raises throughput without touching
per-request latency. It is unsafe for any lever that raises throughput by
making each stream slower: against a throughput-only gate such a lever does not
merely tolerate a latency regression, it selects for the largest one on offer,
because that is where the throughput is.

Measured on one MI355X running a 1.26B-parameter vision model at six views per
forward pass: against the best unsplit configuration, splitting the card into
eight partitions with two streams each raised aggregate throughput about 20%
while mean end-to-end latency went from 183 ms to 1211 ms, a 6.6x
regression. Under a throughput-only gate that candidate is promoted,
current_best becomes the 1211 ms configuration, and the session signs off with
a +20% win that nobody serving an interactive workload can deploy.

What

--max-latency-ms names a ceiling on mean end-to-end latency. It is a
constraint, not a target: it does not say when the run stops, only which
winners are admissible, so it sits outside the --target-* mutually-exclusive
group and composes with whichever objective is in use. It is absolute rather
than relative, because an SLA is a fixed number the workload owner already
knows and a percentage cap would ratchet upward as the baseline improved. It is
off by default, leaving KEEP behaviour exactly as it was when unset.

Shape

This is a redraw. An earlier revision of this PR landed the constraint as a
parallel latency subsystem — its own module, its own resolver chain, its own
gate beside the existing ones. Review was that it should ride the mechanisms
that already own the concern, and it does now: 200 lines of business code,
down from 633, with the 266-line _latency_budget.py deleted outright.

One veto channel. The SLA is applied where the gain gates already rule.
graded_comparison marks an over-budget candidate REVERT and names it in a
new GradedComparison.veto_reason, so every consumer of the verdict honours it
without a check of its own: the promotion choke point, explore's round ladder,
the kernel stack, the integrate lane. Explore therefore refuses in the round
that measured the variant, which keeps an over-budget config from being folded
onto the stack and becoming the anchor the rest of the batch is graded against —
and its ledger row says latency_budget_exceeded rather than blaming a gain the
variant comfortably made.

One field. Every dict reaching the gate carries e2el_mean_ms, copied from
VariantResult by the lane that built it. There is no alias table and no nested
descent: a lane that does not carry the field is that lane's bug, not a spelling
the lookup should learn.

One copy of the budget. SharedState.latency_budget_ms, written at launch
and archived with the session, so a resume restores it with no second source to
reconcile. No environment variable (the gate runs in the Coordinator process, so
nothing else read it) and no task-params tier (nothing wrote it). Parse failures
happen only at the CLI, which exits 2 — the switch on a fail-closed gate must
not itself fail open.

Fail-closed

When set, the gate refuses a candidate that reported no end-to-end latency,
because an unmeasured constraint is not a satisfied one. That makes latency part
of the promotion contract rather than a field lanes may or may not fill, which
is why each lane copies it: the rule must refuse the untimed, not the
unplumbed.

It fails closed at the boundary too. If the baseline itself exceeds the ceiling,
the run stops with baseline_over_latency_budget instead of warning and then
refusing every candidate for the rest of --max-hours to discover something
knowable at launch. That also means no current_best can be over budget, so no
Objective.reached() needs to learn about the constraint to avoid signing off
on an inadmissible champion.

Refusals are recorded to SharedState.latency_refusals and render into the
orchestration prompt as === Latency budget (constraint) ===. Without it a
session that ends near baseline is indistinguishable from one that exhausted its
headroom, and the router cannot tell a binding SLA from an exhausted search
space — the two call for opposite responses. The block is rendered unwrapped:
swallowing it would have the model route as if the session were unconstrained,
which is a silent failure of the constraint itself.

Tests

44 in test_latency_budget.py: the predicate, the lift veto and its recorded
refusal, the lanes' real return dicts, CLI validation, resume from archived
state, and the rendered prompt block. Explore's REVERT is an end-to-end test in
test_explore_executor.py that drives the real ladder through the executor
harness and asserts the reverted row's reason and gate.

The lane tests are built from each executor's own return dict rather than from a
fixture already shaped the way the gate wants — a fixture that supplies the
field cannot catch a lane that forgets it, and one test strips the field back
off to show the gate then refuses.

Docs

SKILL.md's operator→CLI flag table (the source of truth for forwarding a
stated value, so an SLA given in prompt form is not dropped), the
orchestration.md routing rule naming the prompt block, and
docs/conceptual/optimization-loop.md, where KEEP semantics are defined.

Review notes

Split out of #1288, where the flag existed but only ExploreExecutor honoured
it while the help text claimed otherwise. Orthogonal to the partition work: the
constraint applies to any throughput-for-latency trade, so it is reviewable and
useful on its own.

[github-actions]

CI E2E report — ❌ Failed

item	value
result	❌ Failed
model	Qwen/Qwen3-0.6B (dense)
resources	1× GPU, TP=1
PR branch	feat/max-latency-ms
commit	93dc8b96de19ecfdf136f283b3207948a227b084
session_id	f599af78-9b38-43f2-b577-21bc12df4bbd
queue → dispatch	23m 31s
run time	144m 2s
total	167m 33s
reason	platform reported Failed: container hyperloom exited with code 1 (Error): pruned_families : []
detail	`platform reported Failed: container hyperloom exited with code 1 (Error): pruned_families : []

details

[ZhengGong-amd]

1. fail-closed silently kills promotion for integrate_patch / framework_agent / specialist / GEAK lanes. Their result dicts never populate e2el_mean_ms (verified: 0 occurrences of e2el in integrate_patch.py, framework_agent.py, specialists/rebench.py). Once any --max-latency-ms is set, latency_from_result returns None for these lanes and every KEEP is refused via latency_unmeasured_under_budget — regardless of actual latency. This only surfaces as a log.warning, so operators see "nothing KEEPs anymore" with no diagnosis. Fix: populate e2el_mean_ms from VariantResult.e2el_mean_ms in the three bench dicts (and fix the already-broken ttft_ms/itl_ms → ttft_mean_ms/tpot_mean_ms while there), or reuse the existing disk-fallback reconstruction in sessions.py. Please also add a test that exercises _lift_to_current_best with each executor's real result shape — the current test hardcodes e2el_mean_ms on every lane and would not have caught this.

2. The report claims "within budget" unconditionally. report.py's best-config line appends ", within budget" without ever comparing kept_e2el to the budget. Since baseline is allowed to exceed budget (warn-only) and still gets written to current_best, the report can state "1211.0 ms, within budget" against a 200 ms budget. Fix: compare against the budget and label accordingly.

3. orchestration.md tells the model to read latency_refusals, but nothing renders it into any prompt section (render.py / conversation.py have zero references). Fix: add a Latency budget (constraint) prompt section when a budget is set, or drop the pointer to an invisible field.

4. Resume path clears the env-layer budget before restoring it. _export_latency_budget(args.max_latency_ms) runs unconditionally before the if args.resume_from: branch, so on resume without the flag it pops HYPERLOOM_MAX_LATENCY_MS before _restore_latency_budget_from_state ever reads it — the documented "CLI > env > archived state" priority is dead code for the env layer. Fix: guard the :1772 export the same way _export_workload_envs_for_optimize already is (only when not args.resume_from).

5. SKILL.md's operator→CLI flag table doesn't list --max-latency-ms. Since this table is documented as the source of truth for forwarding user-stated flags, a user's latency SLA stated in prompt form will be silently dropped. Please add it.

[rpoornac]

All five addressed in 4b765ed06. The branch is also rebased onto current main, which matters for two of these because upstream deleted machinery this PR was built on — details at the end.

1. Fail-closed killed promotion for the bench lanes. Confirmed, and the shape is slightly different on current main than when you filed it. main has since fixed the attribute reads (rb.ttft_mean_ms / rb.tpot_mean_ms) but deliberately keeps emitting them as ttft_ms / itl_ms "for the collectors" — and end-to-end latency was still absent entirely. So the gate, looking only for the canonical name on the top level, saw an untimed candidate every time. Fixed in two halves:

• integrate_patch and specialists/rebench now carry end-to-end latency, emitted as e2el_ms to match the siblings beside it so the collector contract is untouched.
• latency_fields_from_result normalizes what promotion reads: the spellings in flight and the one layer of nesting integrate_patch uses when it returns the measurement under bench_result. The four lift dicts go through it, so current_best carries the latency it was graded on. Before: result.get("e2el_mean_ms") → None → refused. After: {'ttft_mean_ms': 40.0, 'e2el_mean_ms': 150.0, 'tpot_mean_ms': 12.0} → passes a 200 ms budget.

On the test: agreed the old ones couldn't have caught this, since every one hardcoded e2el_mean_ms on the lift dict. TestLaneResultShapes now drives the lanes' real payload shapes through _lift_to_current_best, and pins the canonical names against a VariantResult rename so reading a field the object doesn't have can't silently return None again.

2. Report claimed "within budget" unconditionally. Fixed — it compares now. Against a 200 ms budget with 1211 ms kept it reads `1211.0` ms, **over budget by 1011.0 ms**, followed by a line saying no candidate has yet come in under the SLA and that the budget does not refuse the baseline. An untimed current_best says latency not measured rather than claiming compliance.

3. orchestration.md pointed at an invisible field. Added the section rather than dropping the pointer. SharedState.to_latency_budget_summary() renders === Latency budget (constraint) === when a budget is set, and the guidance now names that block:

budget    : 200 ms mean end-to-end, enforced on every KEEP
unmeasured: refused (a constraint that was not measured is not satisfied)
refused   : 2 winner(s) so far
  - cpx-2-streams (explore): 1211 ms
  - qpx-4-streams (framework): not measured
A list that keeps growing means the SLA is the binding limit, not an exhausted search space

4. Resume cleared the env tier before restoring it. Guarded exactly as you suggested. Worth flagging: that fix exposed a latent crash one line further on — with the env no longer popped, an unparseable HYPERLOOM_MAX_LATENCY_MS reached float() and would have ended the resume with a traceback. _restore_latency_budget_from_state now falls through to the archived budget on any unusable env value, matching how resolve_latency_budget_ms treats a zero tier.

5. SKILL.md flag table. Row added, noting it is a constraint that combines with --target-* and that omitting it does not lose a preference but removes the SLA from the search.

What the rebase changed. main retired the post-KEEP confirmation round (_stack_rebench.py deleted) and the framework_agent action (framework_agent.py and _promote_framework_agent deleted). Two pieces of this PR went with them: the StackRebenchResult.e2el_mean_ms plumbing, and the framework lane's lift. Framework candidates now land through integrate_patch, which carries the fix, so the coverage you asked about is intact — but the commit message, CHANGELOG and explore comments claiming the gate grades a rebench's latency were describing code that no longer exists, and are corrected.

One adjacent bug, already dead — no action. While confirming finding 1 I found the same class of bug in the retired lane's result_dir: getattr(r, "result_dir", "") on a VariantResult with no such field, always "", fed straight to parse_eval_results, so framework_agent's accuracy gate could never parse a result and silently stayed None. It was wrong twice over — even populated, it pointed at the benchmark_* workspace, while lm-eval writes to $EVAL_RESULT_DIR one level up under the grid slot. I checked whether it survived into the integrate_patch path: it did not. Every remaining result_dir read in the tree is params.get("result_dir") behind sanitize_result_dir, and integrate_patch grades from override_result_dir or Path(bench["workspace"]).parent. Flagging only in case past FRAMEWORK accuracy verdicts look suspiciously absent.

Verification. test_latency_budget.py 30 → 57 tests, all passing. Full inference_optimizer suite: 11441 passed, 20 pre-existing failures that reproduce identically on pristine 120cb3262 (API-key and cluster-dependent suites). ruff check and ruff format --check clean.

[rpoornac]

Adding the concrete case that motivated the flag, since the "why" is easier to
judge against numbers than against the general argument.

Measured on one MI355X running a 1.26B-parameter vision model at six views per
forward pass. Against the best unsplit configuration, splitting the card into
eight partitions with two streams on each raised aggregate throughput about 20%
while per-request mean end-to-end latency went from 183 ms to 1211 ms, a 6.6x
regression. The optimizer maximizes output_throughput and nothing else —
latency is measured, reported and fed to the prompts, but no latency number could
block a KEEP — so that candidate is promoted, current_best becomes the 1211 ms
configuration, and the session signs off with a +20% win that nobody serving an
interactive workload can deploy.

The sharp edge is that this is not mere tolerance of a regression. Facing a lever
that buys throughput by making each stream slower, a throughput-only gate
selects for the largest regression on offer, because that is where the throughput
is. For most levers in the search space the omission is survivable — a
serving-config flag that raises throughput rarely destroys per-request latency at
the same time — but this class of lever is exactly where it isn't.

With --max-latency-ms 250, the same KEEP is refused with
latency_budget_exceeded: 1211 ms exceeds the 250 ms budget (4.84x),
current_best stays at the 183 ms configuration, and the refusal is recorded in
latency_refusals so the report states what was rejected instead of silently not
mentioning it.

Three design points that follow from this, and are the reason the PR is shaped
the way it is:

• It is a constraint, not a target, so it sits outside the --target-*
  mutually-exclusive group and composes with whichever objective is already in
  use. An SLA does not replace the thing you are optimizing for.
• It is absolute rather than relative, because an SLA is a fixed number the
  workload owner already knows, and a percentage cap against the baseline would
  silently ratchet upward as the baseline improved.
• It is off by default, so KEEP behaviour is unchanged for every existing
  user, and it fails closed when set — a constraint nobody measured is not one
  anybody satisfied. That is what made the plumbing fixes in the second commit
  necessary rather than optional: under a budget, a lane that never hands the gate
  a latency number refuses every KEEP it produces, whatever it measured. The two
  reason strings stay distinct on purpose, since "too slow" needs a different
  candidate and "never timed" needs the benchmark to report latency at all.

Note on the force-push since my reply above: the branch is now rebased onto the
live main tip, and one stray ## [v1.0.0] heading that my earlier rebase had
left in the middle of the CHANGELOG's [Unreleased] section is removed — it was
refiling unrelated unreleased entries under a shipped release. No code change;
the diff is otherwise identical at 19 files. CI is green on the new head with
e2e still running.

[ZhengGong-amd]

The constraint should land — a throughput-only gate doesn't tolerate a
latency-for-throughput trade, it selects for the worst one. But this lands it as
a parallel latency subsystem next to three mechanisms that already own the
concern. Please redraw rather than adding guards to the current diff; the
business code should end up roughly an order of magnitude smaller.

1. One field. Every bv reaching _lift_to_current_best carries
e2el_mean_ms (or an explicit None), copied from VariantResult. Drop the
alias table and the nested descent. As it stands the gate reads via
latency_from_result while current_best still reads bv.get("e2el_mean_ms")
verbatim (writeback.py:3024-3026), so the mean_e2el_ms case your own test
promotes archives None and the report prints "latency not measured". A lane
that doesn't copy the field is that lane's bug — fix the return dict, don't
teach the lookup another spelling. Test the lanes' real return dicts;
**latency_fields_from_result(result) as a fixture is the decoder validating
itself, which is why the mismatch above got through.

2. One veto channel. GradedComparison.vetoed already means "throughput
wins but we still refuse" (today: intvty). Put the SLA there — add
veto_reason if you need to tell the two apart — instead of a second
if budget_ms > 0 block a few lines below if graded.vetoed. Same in explore:
the latency check belongs in the existing intvty / gain / accuracy ladder, not
beside it. Keeping the earlier round is right; it just isn't a new mechanism.

3. One copy of the budget. state.latency_budget_ms only. Drop
HYPERLOOM_MAX_LATENCY_MS: the gate runs in the Coordinator process, so unlike
the partition shape no worker subprocess reads it, and resume already has the
value in state.json. Also drop the params tier — nothing in the repo writes
params["latency_budget_ms"]. That collapses four parsers of one float into
one. Parse failures may only happen at the CLI (exit 2); today a bad value at
runtime falls through to 0.0, which silently turns the SLA off while the
operator believes it's on — a fail-closed gate whose own switch fails open.

4. Fail closed at the boundary. If the baseline is already over budget, don't
open the loop — fail with a clear stop_reason. Warning and then refusing every
candidate spends the whole --max-hours discovering something knowable at
launch. That removes the exemption and the "over-budget champion" prose in the
report. Related: with the exemption in place, a session can end under "target
reached" holding an over-budget current_best, since no Objective.reached()
knows about the constraint.

5. Prompt: one line of contract, no defensive try/except. Keep the SKILL.md
row and the orchestration.md rule. The except Exception around
to_latency_budget_summary() follows the local pattern, but the neighbours wrap
advisory blocks — swallowing this one makes the model route as if the session
were unconstrained, which is a silent failure of the constraint itself. It's
pure string assembly with no I/O; let it raise. Also sync
docs/conceptual/optimization-loop.md, which is where KEEP semantics are
defined.

6. Tests follow the contract. Predicate, lift veto, explore REVERT, real lane
fields, CLI validation, resume-from-state. Drop the source-string assertion on
conversation.py, the self-referential payload, the permutations of the four
resolvers, and the retired framework task kind (_lift_to_current_best is
near-agnostic to task_kind, so the seven-way parametrization runs one path
seven times).

The constraint is worth merging. What merges should be one more veto on KEEP,
not a latency subsystem alongside it.

[rpoornac]

Redrawn rather than guarded, in fdf7825ab. The branch is rebuilt from the current main tip, so it is no longer conflicting. Business code is 633 → 200 lines, and _latency_budget.py — the 266-line module that held the alias table, the nested descent and the four-tier resolver — is gone entirely.

Two of your points turned out to be live bugs rather than only shape objections, and I want to name both because the second one is worse than filed.

1. One field. Confirmed, including the divergence you pointed at. On the old head the gate read through latency_from_result while current_best archived bv.get("e2el_mean_ms") verbatim, so a mean_e2el_ms candidate passed a 200 ms budget on 150 ms and then archived None — the report said "latency not measured" about a KEEP it had just graded. And you were right about why it survived: the lift dicts in those tests were built by splatting latency_fields_from_result(result), so the fixture arrived already canonical and the archive site was never exercised. The decoder was validating itself.

Every dict reaching the gate now carries e2el_mean_ms, copied from VariantResult by the lane that built it — integrate_patch, the specialist rebench, the kernel promotion, the warm-replay lift and the integrate replay path. The alias table and the nested descent are deleted; a lane that does not carry the field is that lane's bug. Worth noting one thing the redraw made visible: the nesting case never needed a decoder at all, because _record_integrate_keep already unwraps bench_result before building the dict. The lane tests now drive each executor's real return dict — run_specialist_rebench is called with the benchmark stubbed and its own dict asserted on, and there is a test that strips the field back off to show the gate then refuses, so the plumbing stays part of the contract.

2. One veto channel. Done, and it collapsed further than the review asked. main has since replaced vetoed with GradedComparison.verdict, so the SLA now rides that: graded_comparison marks an over-budget candidate REVERT and names it in a new veto_reason. The separate if budget_ms > 0 block is gone, and so is the separate explore check — explore already reads graded.verdict, so the constraint reaches its ladder for free and only needed the REVERT branch to name the reason. That matters for what the ledger says: the reverted row used to read gain_below_threshold on a variant that gained 100x, which would send the search looking for throughput it already had. Keeping the earlier round costs nothing now; it is not a mechanism, it is the same verdict read one round sooner.

3. One copy of the budget. state.latency_budget_ms only. HYPERLOOM_MAX_LATENCY_MS is gone — you were right that nothing outside the Coordinator process read it — as is the params tier, which nothing wrote. Four parsers of one float collapse to zero: argparse validates it, so --max-latency-ms 200ms now exits 2 instead of resolving to 0.0. That last one was the sharper half of the point. I checked it before fixing it: an unparseable value turned the gate off while the operator believed an SLA was enforced, and a 5000 ms candidate passed clean. A fail-closed gate whose switch fails open is worse than no gate, because it is silent.

4. Fail closed at the boundary. Agreed, and this is the one I find hardest to defend as it stood. The old code's own comment said an over-budget baseline meant "every candidate that follows will be refused, and the session would otherwise spend its whole run discovering that one KEEP at a time" — and then warned and continued anyway. It described the failure and chose the weaker response. It now stops with baseline_over_latency_budget, which is registered in the closed vocab and carries a report explanation saying what the operator should do next. That removes the exemption, the over-budget-champion prose, and with it your related point: no Objective.reached() needs to learn about the constraint, because no current_best can be over budget once the baseline path fails closed too.

5. Prompt. try/except dropped — your reading of the local pattern is right, the neighbours wrap advisory blocks and this one is the constraint itself, so swallowing it would have the model route as if the session were unconstrained. It is pure string assembly; it raises now. SKILL.md row and orchestration.md rule kept, and docs/conceptual/optimization-loop.md gains a "Latency budget (constraint on KEEP)" section, since you are right that that is where KEEP semantics are defined.

6. Tests. 57 → 44 in test_latency_budget.py, and the file is 664 → ~300 lines. Dropped: the source-string assertion on conversation.py, the self-referential payloads, the four-resolver permutations (three of the four resolvers no longer exist), and the seven-way task_kind parametrization — you were right that _lift_to_current_best is near-agnostic to it, so it ran one path seven times. What is there now is the contract: the predicate, the lift veto and its recorded refusal, the lanes' real return dicts, CLI validation, resume from archived state, and the rendered prompt block. The explore REVERT is a real end-to-end test in test_explore_executor.py rather than a unit fixture — it drives the actual ladder through the harness and asserts the reverted row's reason and the latency_budget gate row. I first wrote it as a unit test in the latency file and deleted it: it was re-implementing the ladder in the assertion, which is the same self-referential problem in a new place.

Verification. Full tree: 16323 passed, 8 failed — 7 reproduce identically on pristine main (test_external_multi_node, test_agentx_repair, test_profile_and_kernel_handlers), and the 8th is a root-owned .env in my checkout that a clean worktree does not have. ruff check and ruff format --check clean. CI is green on the head: 25 success, 2 skipped.

One thing worth flagging that only the redraw caught, since it is a repo rule rather than a review point: latency_budget_ms or 0.0 tripped the no-falsy-budget rehearsal check. Harmless here only because zero happens to mean "off" — which is exactly the coincidence that rule refuses to rely on. Fixed at every read.