feat(metrics): opt-in MLPerf early-stopping percentile estimates (LoadGen parity) by nvzhihanj · Pull Request #417 · mlcommons/endpoints

nvzhihanj · 2026-07-16T18:04:29Z

What

Adds an on-by-default MLPerf-LoadGen-style early-stopping percentile estimate to the tail-latency metrics (ttft / tpot / latency) for scenarios that measure percentile latencies (Server/Poisson, fixed concurrency) — closing a parity gap with LoadGen (loadgen/early_stopping.cc; mlcommons/inference#2188, mlcommons/inference#1095; MLPerf Inference paper arXiv:1911.02549). LoadGen's fixed-sample requirement (~270k queries for Server p99) is impractical for slow modern workloads (LLM, and especially T2V at minutes per query); early stopping replaces it with a data-dependent binomial-confidence test so short runs surface an honest, conservative tail estimate instead of a silently under-reported empirical percentile.

metrics/early_stopping.py — pure port of the LoadGen binomial-confidence math (continued-fraction regularized incomplete beta, no scipy), validated against LoadGen reference values (e.g. h_min(t=0, p99) = 459; SingleStream-style floors: p90 = 64, p99 = 662).
On by default (⚠️ review question): the feature is non-invasive — computed once at run COMPLETE from data the aggregator already keeps (the exact path sorts that array once, in place, and shares it with the percentile-grid computation), hot path untouched, and the output field is purely additive — so every run gets loadgen-parity tail certification without knowing the flag exists. settings.early_stopping.enabled: false / --no-early-stopping is the single opt-out (for consumers that strictly validate the summary schema). CONFIDENCE = 0.99 and TOLERANCE = 0.0 are LoadGen-parity algorithm constants (results.cc:157-158), and the percentile targets derive from each series' own report percentile grid (filtered to ≥ p50 — an ES estimate is a tail certification), so the scenario gate percentile is always covered — nothing to tune per config, no way to accidentally weaken the statistical claim.
The metrics aggregator computes the estimates once at run COMPLETE from the exact sorted series (cold path; hot path untouched). result_summary.json gains a compact early_stopping_percentile map per metric — keys mirror the percentiles grid (≥ p50), values are the conservative estimate or null when the run is below that percentile's floor. The rich detail (empirical/n/min_queries/discarded) is INFO-logged by the aggregator and reproducible offline.
docs/early_stopping.md — design, layering, and the multi-percentile marginal-confidence caveat.
scripts/early_stopping_estimate_from_events.py — post-hoc tool that recomputes ES for any historical run from its events.jsonl. It decodes lines with the product's typed EventRecord (the JSONL writer's mirror decoder), so event names, payload shapes, TPOT chunk semantics, tokenizer counting, and the ES math all come from product modules.

Estimate-only by design: no target-latency pass/fail gate and no dynamic mid-run halt (scope in the doc).

Validation

Unit (suite: 1351 passed): math vs LoadGen reference values, conservative/below-floor behavior, registry → snapshot → report integration, post-hoc script gating/parsing.
No-GPU smoke (echo server, n = 300): the early_stopping_percentile map reports estimates for p50–p97 (floors 11–130) and null for p99/p99.9 (floors 662/6636) — the short-run behavior the feature exists for.
End-to-end on a DS-R1 B300×8 Server (Poisson) 40k-query run: the in-band blocks exactly match an independent post-hoc recomputation from the run's event log (estimate/empirical/n/discarded, all three metrics — including TPOT's tokenization path). At n = 40k the p99 estimate is the expected conservative rank shift (400th → 353rd-highest sample); the ES-vs-empirical delta is small on a well-provisioned run and grows on short/subsampled runs exactly as the binomial math predicts. (Absolute perf figures from the internal run are intentionally omitted.)

Notes for review

Draft for early feedback: naming/placement (settings.early_stopping vs nesting under a metrics section), the report block shape, and whether scripts/es_from_events.py should graduate to a CLI subcommand.
Planned follow-up: T2V/VLM validation at p90 with fixed concurrency = 1.

🤖 Generated with Claude Code

github-actions · 2026-07-16T18:04:40Z

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

gemini-code-assist

Code Review

This pull request introduces an optional early-stopping percentile estimate feature for tail-latency metrics (TTFT, TPOT, latency), mirroring the MLPerf LoadGen binomial-confidence-backed approach. The implementation includes updates to the metrics aggregator, a new configuration schema, and a post-hoc analysis script for historical event logs. I have reviewed the changes and suggest addressing a potential resource leak in the analysis script by using a 'with' statement for file handling and improving maintainability in the '_betacf' function by replacing magic numbers with named constants.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

nvzhihanj

Review Council — Multi-AI Code Review

Reviewed by: Codex (gpt-5.5, xhigh) + Claude | Depth: thorough

Posting 14 inline findings (1 critical, 5 medium, 8 low). Verified against HEAD before posting; summary comment follows.

nvzhihanj · 2026-07-16T21:15:40Z

Review Council — Multi-AI Code Review

Reviewed by: Codex (gpt-5.5, xhigh) + Claude | Depth: thorough (diff ~1.3k lines)

Found 15 issues across 10 files. All line numbers and claims verified against HEAD (c720c8c) before posting; two findings were independently flagged by both reviewers.

🔴 Must Fix (critical/high)

#	File	Line	Category	Reviewer(s)	Summary
1	`config/schema.py`	927	api-contract	Both	`EarlyStoppingConfig` stole `Settings`' `@cyclopts.Parameter(name="*")` decorator → every dotted CLI flag broken (`--client.warmup-connections` fails; integration test fails at HEAD) + ambiguous `--settings.enabled` bound to both warmup and ES

🟡 Should Fix (medium)

#	File	Line	Category	Reviewer(s)	Summary
2	`metrics/early_stopping.py`	122	bug	Claude	`find_min_passing` hangs for `p>=1.0`, garbage for `c==1.0` — reachable via the post-hoc script's unvalidated `--percentiles`/`--confidence`
3	`metrics/early_stopping.py`	205	data-integrity	Claude	block `empirical` (`ceil(p·n)−1`) diverges from the percentile grid (`floor(p·(n−1))`, `method="lower"`) in the same JSON
4	`metrics/report.py`	109	bug	Both	ES blocks never rendered in report.txt/console; doc claims they are
5	`scripts/es_from_events.py`	107	error-handling	Claude	silent malformed-line swallowing; non-dict JSON / missing keys crash with raw tracebacks
6	`tests/.../test_early_stopping_integration.py`	89	testing	Claude	INTERRUPTED/empty-series/tpot+latency/empirical-vs-grid branches untested

🔵 Consider (low)

#	File	Line	Category	Summary
7	`docs/early_stopping.md`	92	documentation	"ES is a no-op for offline" is factually wrong — nothing gates by scenario
8	`docs/early_stopping.md`	9	documentation	dangling reference to a non-public companion repo (AGENTS.md rule)
9	`scripts/es_from_events.py`	116	bug	duplicate-ISSUED semantics diverge from aggregator → spurious `--compare` mismatches on retry runs
10	`scripts/es_from_events.py`	158	design	lazy import (AGENTS.md), unclosed `--compare` handle, double sort per series
11	`scripts/es_from_events.py`	184	design	dead dict-shape compat branch for a format that never shipped
12	`.../registry.py`	331	bug	empty target series omits the ES block entirely (looks like feature-off)
13	`.../snapshot.py`	254	design	ES block bypasses `_scrub_nonfinite` → future NaN would break final-snapshot write
14	`templates/*_full.yaml`	92	documentation	regeneration lost the warmup comment (generator comment-key collision)
15	`tests/unit/scripts/test_es_from_events.py`	113	testing	`main()`/`_cross_check`/corrupt-input branches unexercised

Fixes for all findings are being applied to this branch iteratively.

🤖 Generated with Claude Code

- critical: restore @cyclopts.Parameter(name="*") on Settings — inserting EarlyStoppingConfig had stolen it, renaming every dotted CLI flag to --settings.* and double-binding --settings.enabled. New CLI-surface tests pin the flag namespace (tests/unit/commands/test_cli_flag_surface.py). - validate 0<p<1 and 0<c<1 in the ES math (p>=1 hung the doubling search; c>=1 returned garbage) + the post-hoc script rejects bad --percentiles/ --confidence up front. - block empirical now uses the report grid convention (floor(p*(n-1)), np.percentile method=lower) so it can never disagree with the grid. - report.txt/console render the ES blocks per metric (doc claimed it). - empty target series with ES on emit n=0/sufficient:false blocks instead of silently omitting; INTERRUPTED snapshots pinned ES-free by test. - ES estimate/empirical pass through _scrub_nonfinite like every other field. - es_from_events.py: count+warn malformed lines, tolerate non-dict/missing-key events, aggregator-parity duplicate-ISSUED handling, top-level imports, context-managed --compare read, single sort, dead dict-shape branch removed. - regenerate_templates.py: comment keys are dotted paths — warmup/early_stopping enabled comments coexist; previously collided comments (name, samples, eval_method, drain) reappear in templates. - docs: removed non-public companion-repo reference, corrected the offline no-op claim, documented the empirical convention and empty-series behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

nvzhihanj · 2026-07-16T21:41:47Z

Review Council — fixes applied (`166f39a`)

All 15 findings addressed:

#	Finding	Resolution
1	🔴 `Settings` lost `name="*"` decorator	Fixed — decorator restored on `Settings`, `EarlyStoppingConfig` un-flattened. New `tests/unit/commands/test_cli_flag_surface.py` pins the flag namespace (`--client.num-workers` top-level, no `--settings.*` leak, warmup keeps bare `--enabled`, ES stays namespaced). `tests/integration/commands/test_cli.py` passes again (5/5).
2	`find_min_passing` hangs for p≥1	Fixed — domain validation (`0<p<1`, `0<c<1`, `0≤d<p`) raises `ValueError`; the script also rejects bad `--percentiles`/`--confidence` up front. The hang was reproduced live by the new test before the fix.
3	`empirical` convention diverges from grid	Fixed — now `floor(p·(n−1))` (matches `np.percentile(..., method="lower")`); test pins block-empirical == grid value.
4	ES not rendered in text report	Fixed — `_display_metric` renders an "Early-stopping estimates" section (insufficient percentiles say so explicitly).
5	script silent/crashy on corrupt JSONL	Fixed — malformed lines counted + WARN'd; non-dict JSON and missing `sample_uuid`/`timestamp_ns` skip-and-count instead of crashing. One review sub-claim did not survive verification: `msgspec.DecodeError` is not a `ValueError` subclass — the original two-exception tuple was correct and is kept (with a comment); the new junk-line test caught this.
6	aggregator test gaps	Fixed — INTERRUPTED pinned ES-free; empty-series blocks; all three target series (incl. float `tpot_ns`) asserted; empirical-vs-grid pin.
7	doc: offline "no-op" claim	Fixed — reworded to match reality (computed, not gating).
8	doc: companion-repo dangling ref	Fixed — removed; conclusion inlined.
9	duplicate-ISSUED divergence	Fixed — aggregator parity (row kept, issue ts refreshed); test mirrors the retry sequence.
10	lazy import / unclosed handle / double sort	Fixed — top-level imports (`token_metrics` is stdlib-only at module level), context-managed `--compare` read, single sort shared by `es_blocks` + `_cross_check`.
11	dead dict-shape branch	Fixed — removed.
12	empty series omits ES block	Fixed — emits `n=0, sufficient:false` blocks on COMPLETE; documented.
13	ES block bypasses `_scrub_nonfinite`	Fixed — estimate/empirical scrubbed like every other numeric field.
14	templates lost warmup comment	Fixed at the root — `regenerate_templates.py` comment keys are now dotted paths (indentation-aware injector); warmup + ES comments coexist, and previously-collided comments (`name`, `samples`, `eval_method`, `drain`) reappear. `--check` mode passes.
15	script test gaps	Fixed — malformed-input, tool-call-warn, `_cross_check` match/mismatch, and `main()` (json output + arg validation) covered.

Gate: 1348 unit tests pass (+15 new), integration CLI tests pass, templates --check clean, ruff + ruff-format clean on all touched files.

🤖 Generated with Claude Code

nvzhihanj · 2026-07-16T22:43:47Z

Design follow-up (`706f7b5`)

Three refinements from continued review:

ES percentile targets are no longer a hardcoded set — they derive from each series' own report percentile grid, filtered to p ≥ ES_MIN_PERCENTILE (0.5, since an ES estimate is a tail certification and below-median grid entries aren't meaningful gates). One source of truth: whatever percentiles a series reports, ES covers — with the default grid that's p50/p75/p80/p90/p95/p97/p99/p99.9, and the block's empirical matches the grid value for every entry by construction. EarlyStoppingSpec(percentiles=None) means "derive"; explicit tuples remain for tests/offline analysis.
ES_TARGET_METRICS removed — a name-based frozenset in a distant module was fragile (a new latency metric would silently miss ES). Series now opt in with register_series(..., tail_latency=True) at the metric's definition site in the aggregator.
Math helpers fully documented — _betacf/_betai cite Numerical Recipes 3rd ed. §6.4 (modified Lentz continued fraction) with every magic number explained (eps=3e-16 ≈ 1.4× machine epsilon, fpmin=1e-300 Lentz near-zero guard, 400-iteration safety cap, the symmetry-split threshold); _odds/find_min_passing/_discard_count cite the exact LoadGen counterparts (early_stopping.cc:47-58, :62-114; results.cc:162-226).

Gate: 1348 unit tests pass; ruff/format clean on touched files.

🤖 Generated with Claude Code

nvzhihanj · 2026-07-16T23:02:50Z

Pre-commit gate is now fully green on the branch (b0315e1): all 13 hooks pass — mypy (src+tests, 287 files), ruff/ruff-format (pinned hook versions), prettier, license headers, regenerate-templates --check, uv lock check. Docs updated across the repo: --early-stopping in docs/CLI_QUICK_REFERENCE.md, README metrics feature row, and AGENTS.md (code tree + aggregator early-stopping bullet incl. the tail_latency registration flag and grid-derived percentiles).

🤖 Generated with Claude Code

nvzhihanj · 2026-07-17T17:05:51Z

Report/UX follow-up (e3e8d31): result_summary.json now carries a compact early_stopping_percentile map per metric — keys mirror the percentiles grid (≥ p50), values are the estimate or null when insufficient; the rich per-percentile detail (empirical/n/min_queries/discarded) moved to aggregator INFO logging and the post-hoc script. --no-early-stopping removed (negative=(); dead surface for a default-off feature, pinned by a CLI-surface test). The post-hoc script is renamed to scripts/early_stopping_estimate_from_events.py and now decodes lines with the product's typed EventRecord (JSONL-writer mirror decoder) — no hand-parsed wire format remains. Gate: 1351 unit tests, 13/13 pre-commit hooks.

🤖 Generated with Claude Code

nvzhihanj · 2026-07-17T17:57:07Z

Default flipped to ON (e4fbf3b) — flagged in the description as an explicit review question. Rationale: the feature is cold-path-only (computed once at COMPLETE from data the aggregator already keeps; the exact path now sorts that array once in place and shares it between the percentile grid, histogram, and ES estimates — so default-on adds no transient copy and roughly no finalize cost), the output field is additive, and the users who most need honest tail estimates are exactly those who wouldn't know to enable a flag. enabled: false / --no-early-stopping remains as the single opt-out for strict summary-schema consumers. A repeated-COMPLETE-snapshot test guards the in-place sort. Gate: 1352 unit tests, 13/13 pre-commit hooks.

🤖 Generated with Claude Code

gemini-code-assist · 2026-07-17T18:36:12Z

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

nvzhihanj · 2026-07-17T18:36:25Z

Review Council — round 2 (Codex + Claude, thorough)

11 findings (2 duplicates across reviewers), all verified against HEAD and fixed in a0ad253 + e5f6d48:

Severity	Finding	Fix
medium	grid entry `100.0` → ES target p1.0 → `ValueError` inside `publish_final` before its try/except — entire final snapshot lost (default-on!)	`es_targets_from_grid` excludes ≥100; crash-repro integration test
medium	empty enabled series: the all-null map was built in the snapshot then dropped by `_series_to_metric_dict` (`count==0 → {}`) — summary contradicted docs [both reviewers]	empty-rollup dict keeps the map; display hardened
medium	`execute.py` `--early-stopping` plumbing had zero test coverage — a refactor could silently disable the default-on feature in every real run	e2e assert: `early_stopping_percentiles` in a real run's `result_summary.json`
medium/P3	`discarded` reported the order-statistic offset `t`; the estimate IS the t-th highest, so only `t−1` are discarded (floor case: 0)	reports `t−1`; docstrings + anchors updated
low	int grids (`(99, 90, 50)`) produced ES keys (`"99.0"`) diverging from grid keys (`"99"`) — broke the 1:1 overlay contract	keys are `str()` of original grid values
low	`EventType.decode_hook` raises `NotImplementedError` for non-string event_type; msgspec doesn't wrap it → one line crashed the post-hoc tool	counted as malformed; fixture line added
low	duplicate `RECV_FIRST` (retry) diverged from the aggregator (first-only vs re-fire + window refresh)	aggregator parity; doubled `flush()` removed
low	`_betacf` 400-iteration cap comment was empirically false at n≈1e7 (cap IS reachable; measured ≤3e-8 error vs scipy)	comment corrected; plateaued-return documented as deliberate on the terminal path
low	docs drift: layering diagram `enabled=False`, stale script path in a comment, over-broad "present unless opted out"	all corrected

Gate: 1330 unit tests (lean consolidated suite), e2e + binding integration tests, 13/13 pre-commit hooks.

🤖 Generated with Claude Code

…re + tests) Port loadgen early_stopping.cc/results.cc SingleStream estimate: es_percentile_estimate(sorted, p, c=0.99, d=0) returns a conservative confidence-backed percentile (sorted[n-t]) plus empirical and the min-queries floor. Fast Numerical-Recipes betai (no scipy). Validated vs loadgen values (h_min(t=0,p99)=459). Integration into build_stat/report/schema follows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…report/config Default-off YAML flag settings.early_stopping.{enabled,percentile,confidence,tolerance} -> aggregator subprocess CLI args -> MetricsRegistry -> SeriesSampler.build_stat (COMPLETE path) computes the estimate for TTFT/TPOT/latency only -> SeriesStat.early_stopping (optional trailing field) -> result_summary.json + report. No hot-path impact; LIVE snapshots unaffected. Adds the integration test, design doc (docs/early_stopping.md), and regenerated templates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…; tolerance is now an algorithm constant - settings.early_stopping.percentile (float) -> percentiles (list, default [0.5, 0.9, 0.95, 0.99]) so enabling ES is just enabled:true — the scenario gate percentile is always covered without per-yaml tuning. result_summary.json early_stopping becomes a list of self-describing per-percentile blocks. - tolerance removed from config/CLI/report: loadgen hardcodes d=0.0 (results.cc:158) and d>0 weakens the guarantee to percentile p-d, so it is TOLERANCE=0.0 in metrics/early_stopping.py, kept only as a defaulted arg for parity tests. - aggregator flag --es-percentile -> --es-percentiles (comma-separated); templates regenerated; docs/early_stopping.md updated (incl. marginal-confidence caveat). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… event log Rebuilds ttft/tpot/latency from events.jsonl with the aggregator rules (tracking-window gating; TPOT = (complete-recv_first)/tokens(text_after_first_chunk) via TextModelOutput + token_metrics.encode_lengths) and reuses metrics/early_stopping.py for the math — ES results for historical runs that never enabled the feature. --compare cross-checks against a run with in-band blocks (validated: exact match on estimate/empirical/n/discarded for all three metrics, including the TPOT tokenization). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…es + confidence become constants Review feedback: the unrolled percentiles list in the YAML and the confidence field added config surface for values users should not tune. LoadGen hardcodes confidence=0.99 and tolerance=0.0 (results.cc:157-158); the standard report set PERCENTILES=(0.5,0.9,0.95,0.99) always covers the scenario gate percentile. All three are now constants in metrics/early_stopping.py; the schema, launcher args, and aggregator CLI shrink to --early-stopping. Templates regenerated; docs updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

- critical: restore @cyclopts.Parameter(name="*") on Settings — inserting EarlyStoppingConfig had stolen it, renaming every dotted CLI flag to --settings.* and double-binding --settings.enabled. New CLI-surface tests pin the flag namespace (tests/unit/commands/test_cli_flag_surface.py). - validate 0<p<1 and 0<c<1 in the ES math (p>=1 hung the doubling search; c>=1 returned garbage) + the post-hoc script rejects bad --percentiles/ --confidence up front. - block empirical now uses the report grid convention (floor(p*(n-1)), np.percentile method=lower) so it can never disagree with the grid. - report.txt/console render the ES blocks per metric (doc claimed it). - empty target series with ES on emit n=0/sufficient:false blocks instead of silently omitting; INTERRUPTED snapshots pinned ES-free by test. - ES estimate/empirical pass through _scrub_nonfinite like every other field. - es_from_events.py: count+warn malformed lines, tolerate non-dict/missing-key events, aggregator-parity duplicate-ISSUED handling, top-level imports, context-managed --compare read, single sort, dead dict-shape branch removed. - regenerate_templates.py: comment keys are dotted paths — warmup/early_stopping enabled comments coexist; previously collided comments (name, samples, eval_method, drain) reappear in templates. - docs: removed non-public companion-repo reference, corrected the offline no-op claim, documented the empirical convention and empty-series behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…tion flag, documented math - ES percentile targets are no longer a separate hardcoded set: they derive from each series own report percentile grid, filtered to p >= ES_MIN_PERCENTILE (0.5) — an ES estimate is a tail certification, so below-median grid entries (p25/p10/p5/p1) are skipped. One source of truth: whatever percentiles a series reports, ES covers (default grid -> p50/p75/p80/p90/p95/p97/p99/p99.9). EarlyStoppingSpec.percentiles=None means derive; explicit tuples remain for tests/offline analysis. DEFAULT_PERCENTILES promoted to public for the post-hoc script default. - ES_TARGET_METRICS frozenset removed: series opt in with register_series(..., tail_latency=True) at the metric definition site, so a new latency metric declares its nature where it is registered instead of updating a distant name set. - metrics/early_stopping.py helpers documented with citations (Numerical Recipes 3rd ed. section 6.4 betacf/betai via modified Lentz; loadgen early_stopping.cc odds/MinPassingQueriesFinder, results.cc SingleStream estimate) and every magic number explained (eps=3e-16, fpmin=1e-300, the 400-iteration CF cap, the symmetry-split threshold). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…rence/AGENTS.md pre-commit run --all-files now passes all 13 hooks (incl. mypy on src+tests, prettier, license headers, regenerate-templates --check, uv lock check). Includes the pinned ruff-format hook normalization on touched files and prettier normalization of the markdown docs. Docs: --early-stopping added to docs/CLI_QUICK_REFERENCE.md common flags; README metrics feature row mentions the optional ES estimates; AGENTS.md gains metrics/early_stopping.py in the code tree and an aggregator bullet describing the tail_latency registration flag, grid-derived percentiles, COMPLETE-only computation, and the post-hoc script. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…ld mapping SERIES_TO_SUMMARY_FIELD promoted to a public constant in metrics/report.py (from_snapshot builds the summary fields from it) and imported by the script — the mapping can no longer drift. Script constants audited: _UNITS rekeyed by summary field and documented as script-local display formatting (latency in seconds for long-CoT runs), _TPOT_BATCH documented (bounds text buffering to ~64 MB while amortizing batch-encode overhead). _cross_check skips non-list early_stopping payloads (pre-release artifacts) instead of crashing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… dead negative flag; script renamed + typed decode Review feedback: - result_summary.json now carries a compact early_stopping_percentile map per metric — keys mirror the percentiles grid (>= p50), values are the estimate or null when insufficient. The rich detail (empirical/n/min_queries/ discarded/confidence) is INFO-logged by the aggregator and reproducible via the post-hoc script; the text report renders the map (N/A = insufficient). - --no-early-stopping removed (cyclopts Parameter negative=()): the feature is default-off, the negative was dead surface. CLI-surface test pins absence. - scripts/es_from_events.py -> scripts/early_stopping_estimate_from_events.py. - The script decodes lines with the product typed EventRecord (msgspec Decoder + EventType.decode_hook — the JSONL writer mirror), so event names, payload shapes, and TPOT chunk semantics all come from core/record.py and core/types.py instead of hand-parsed wire format; extract_tpot_text is now a two-line isinstance + text_after_first_chunk call. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…place The feature is non-invasive — computed once at run COMPLETE from data the aggregator already keeps, hot path untouched, output field additive — so it now defaults ON for every run; enabled: false / --no-early-stopping is the single opt-out (strict summary-schema consumers). The exact path sorts the raw array ONCE, in place (terminal path; arrival order is meaningless after COMPLETE), and shares it between the percentile grid, the histogram, and the ES estimates, so default-on adds no transient copy; a repeated-COMPLETE-snapshot test guards the in-place mutation. CLI-surface test flipped to pin the opt-out flag. Note: tests/integration/commands/test_benchmark_command.py template runs fail in this dev environment identically on the parent commit (toy char tokenizer vs chat-template warmup probe in the unpinned venv) — pre-existing, unrelated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…e the ES test suite - result_summary.json key early_stopping_percentile -> early_stopping_percentiles (plural, consistent with the sibling percentiles grid it overlays); renamed across snapshot/registry/report/script/docs. - Test consolidation: the ES tests had accreted one-per-review-round. Rewritten as a lean set where every test pins one named failure mode (loadgen reference anchors, grid/key overlay, sufficiency floor, empirical/grid convention, domain validation hang, wiring lifecycle, tail_latency gating incl. float dtype, in-place-sort identity, script gating/robustness/CLI contract, product-mapping drift, generated CLI surface). Net: ~25 redundant tests removed, chunk-semantics coverage MOVED to tests/unit/test_core_types.py where TextModelOutput.text_after_first_chunk lives (it had no direct product coverage), CLI-surface tests merged into one subprocess invocation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…true above-estimate count Review round 2 (Codex): - _series_to_metric_dict returned {} for count==0, silently dropping the all-null early_stopping_percentiles map the registry emits for enabled empty series — result_summary.json made enabled-but-insufficient indistinguishable from disabled. The empty-rollup dict now keeps the map, and _display_metric tolerates missing rollups (.get -> N/A) per its own partial-dict philosophy. - discarded reported the LoadGen order-statistic offset t, but the estimate IS the t-th highest sample, so only t-1 samples are discarded above it (at the floor, t==1, nothing is discarded). Now reports t-1; docstrings + reference anchors updated. Estimates themselves are unchanged. - test_cli_flag_surface.py removed as redundant with the integration CLI tests; its one non-redundant pin (silent --enabled double-bind between warmup and early_stopping) moved into tests/integration/commands/test_cli.py as an in-process parse assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…escape, recv_first parity - es_targets_from_grid replaces es_percentiles_from_grid: excludes grid entries >= 100 (p1.0 is outside the binomial domain and crashed build_snapshot INSIDE publish_final before its try/except — losing the whole final snapshot, with ES default-on) and keys the map by str() of the ORIGINAL grid values so int grids ((99, 90, 50)) key as they appear in percentiles — the 1:1 overlay contract holds for custom grids. Crash-repro integration test added. - script: EventType.decode_hook raises NotImplementedError for a non-string event_type and msgspec does not wrap it — one such line crashed the tool; now counted as malformed (fixture line added). - script: duplicate RECV_FIRST now mirrors the aggregator (re-fires ttft and refreshes the TPOT window start) instead of first-only; doubled flush() removed. - _betacf 400-cap comment corrected: the cap IS reachable at n~1e7 boundary searches (a~b~n/2); measured truncation error <= 3e-8 vs scipy — returning the plateaued value is deliberate on the terminal-snapshot path. - e2e guard: test_cli.py offline run asserts early_stopping_percentiles lands in result_summary.json (the execute.py arg plumbing had no end-to-end test). - docs: layering diagram said enabled=False; output wording scoped to COMPLETE snapshots and recorded series. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

gemini-code-assist · 2026-07-17T19:58:19Z

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

…script emits run-shaped summaries - es_targets_from_grid preserves the grid own (descending) order and accepts summary-JSON key strings, so the map overlays percentiles in naming AND ordering; explicit overrides sort descending via grid_percentile_key. - early_stopping_percentiles is placed directly after percentiles (before the histogram) in both snapshot_to_dict and the report dict — single source of position: report.place_early_stopping_percentiles, shared with the script. - early_stopping_estimate_from_events.py: --compare replaced by --summary; the targets derive from the run own percentile grid (exact keys, exact order) and --json now writes the SUMMARY AUGMENTED with the maps — byte-identical shape to what an ES-enabled run would produce — instead of a bare block dump. es_blocks wrapper removed; ordering/augmentation reuse product helpers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

arekay-nv

Looks good - thanks for the quick turnaround!

…he series mapping lookup Plus clarifying comments where review questions showed the code did not explain itself: RECV_FIRST without a row is the expected untracked-sample path (not malformed input), and es_targets_from_grid deliberately keys by str() of the original grid entry rather than grid_percentile_key (float-style reconstruction would break int-grid key fidelity; the 6-fraction-decimal rounding equals the key helper 4 percent decimals). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

arekay-nv

Review Council — Multi-AI Code Review (round 3)

Reviewed by Codex (gpt-5.5) + Grok (cursor-grok-4.5-high) + Claude, depth thorough. All line numbers and claims verified against HEAD; findings already raised/fixed in prior rounds were dropped. Full tiered summary in the follow-up comment.

arekay-nv · 2026-07-17T22:34:34Z

Review Council — Multi-AI Code Review (round 3)

Reviewed by Codex (gpt-5.5, xhigh) + Grok (cursor-grok-4.5-high) + Claude · depth thorough

Raw: Grok 5, Codex 2, Claude 5. After verifying every line/claim against HEAD and dedup'ing against the 45 existing comments from prior rounds, 8 non-duplicative findings survived. The two most actionable are new and were not caught in earlier rounds.

🔴 Must Fix

#	File:Line	Category	Reviewers	Summary
1	`metrics/report.py:86`	data-integrity	Codex + Claude	The `166f39a` empty-series fix is dead code. `_series_dict:278` short-circuits `count==0`→`{}` before `_series_to_metric_dict`, so an enabled-but-empty tail series still emits `{}` in `result_summary.json` (looks feature-off). Producer emits the n=0 block; the report consumer drops it. Test asserts the helper, not `Report.from_snapshot`. Verified empirically.

🟡 Should Fix

#	File:Line	Category	Reviewers	Summary
2	`metrics/early_stopping.py:115`	documentation	Claude	`_betacf` docstring's "≤ 3e-8 rel error vs scipy at n=1e7" is false — actual 8.0e-5 (~2670× off), reproduced independently. Code is safe (boundary is odds≈0.01, not midpoint 0.5; refs match LoadGen), but the fabricated figure violates the "accurate comments" rule.
3	`metrics_aggregator/registry.py:375`	error-handling	Grok	ES runs unguarded on the finalize path: `publish_final` sets `_finalized=True` (publisher.py:198) then calls `build_snapshot` (211); the `try/except` at 219 only wraps the file-write. An ES exception (e.g. a ≥1.0 override, which now raises) loses the whole final report irrecoverably. Make ES best-effort. Defense-in-depth (not triggerable by default config).

🔵 Consider

#	File:Line	Category	Reviewers	Summary
4	`metrics/early_stopping.py:229`	documentation	Grok	`_discard_count` docstring says "(t+1)-th highest" but impl reports the t-th (`sorted[n-t]`, discarded=t−1). Same `t` used two ways — off-by-one trap.
5	`metrics/early_stopping.py:217`	bug (parity)	Claude	Binary search uses non-strict `<=`; LoadGen `early_stopping.cc:70` is strict `<`. Boundary `odds==1-c` returns h one smaller. Unreachable in IEEE-754 (refs match); parity note only.
6	`scripts/early_stopping_estimate_from_events.py:319`	bug	Grok	Gap line gated on truthy `r.empirical`, dropping a legitimate `0.0` (also line 344). Use `is not None`; guard the `%` separately.
7	`metrics/early_stopping.py:248`	design	Claude	`EarlyStoppingResult` hand-rolled while sibling `EarlyStoppingSpec` (line 83) is a `@dataclass(frozen=True, slots=True)` — repo convention. Cold-path nit.
8	`tests/unit/metrics/test_early_stopping.py:85`	testing	Grok + Claude	Untested port branches: `_betai` reflection (line 175), `_betacf` 400-iter cap (line 126), `t==1`/`discarded==0` boundary.

Dropped after verification

Codex [P2] scripts/Dockerfile.dev:50 UV_NO_SYNC — file is not in this PR's diff; Codex's worktree picked up unrelated history. Out of scope.
Override-percentiles ≥1.0 filter (registry.py:245) — the domain-validation topic was already raised (early_stopping.py:218) and fixed in 166f39a (_validate_domain); residual folded into finding [Roadmap] Inference Endpoints 2025Q4 GA Roadmap #3.

⚠️ Commit hygiene: 16 commits including 3 apparent fixups. Consider squashing before merge.

_{All findings posted as inline comments on their exact lines. Event: COMMENT (no approve/request-changes).}

github-actions Bot requested a review from arekay-nv July 16, 2026 18:04

github-code-quality Bot found potential problems Jul 16, 2026

View reviewed changes

Comment thread scripts/es_from_events.py Fixed

gemini-code-assist Bot reviewed Jul 16, 2026

View reviewed changes

Comment thread scripts/es_from_events.py Outdated

Comment thread src/inference_endpoint/metrics/early_stopping.py

nvzhihanj commented Jul 16, 2026

View reviewed changes

nvzhihanj mentioned this pull request Jul 17, 2026

MLPerf v6.1 LoadGen <> Endpoints++ submission checker issues mlcommons/inference#2628

Open

6 tasks

nvzhihanj marked this pull request as ready for review July 17, 2026 18:36

nvzhihanj requested a review from a team July 17, 2026 18:36

nvzhihanj marked this pull request as draft July 17, 2026 18:36

nvzhihanj and others added 12 commits July 17, 2026 11:36

nvzhihanj and others added 2 commits July 17, 2026 11:36

nvzhihanj force-pushed the feat/early-stopping-percentile-estimate branch from e5f6d48 to 021c223 Compare July 17, 2026 18:38

nvzhihanj marked this pull request as ready for review July 17, 2026 19:58

arekay-nv approved these changes Jul 17, 2026

View reviewed changes

nvzhihanj merged commit 598dbfd into main Jul 17, 2026
9 checks passed

arekay-nv reviewed Jul 17, 2026

View reviewed changes

nvzhihanj mentioned this pull request Jul 17, 2026

refactor(metrics): unify the percentile convention; address #417 round-3 review #423

Open

zihaok mentioned this pull request Jul 18, 2026

Finish submission checker log parsing in both endpoint logs and loadgen logs mlcommons/inference#2636

Open

Uh oh!

Conversation

nvzhihanj commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Validation

Notes for review

Uh oh!

github-actions Bot commented Jul 16, 2026

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

nvzhihanj left a comment

Choose a reason for hiding this comment

Review Council — Multi-AI Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nvzhihanj commented Jul 16, 2026

Review Council — Multi-AI Code Review

🔴 Must Fix (critical/high)

🟡 Should Fix (medium)

🔵 Consider (low)

Uh oh!

nvzhihanj commented Jul 16, 2026

Review Council — fixes applied (166f39a)

Uh oh!

nvzhihanj commented Jul 16, 2026

Design follow-up (706f7b5)

Uh oh!

nvzhihanj commented Jul 16, 2026

Uh oh!

nvzhihanj commented Jul 17, 2026

Uh oh!

nvzhihanj commented Jul 17, 2026

Uh oh!

gemini-code-assist Bot commented Jul 17, 2026

Uh oh!

nvzhihanj commented Jul 17, 2026

Review Council — round 2 (Codex + Claude, thorough)

Uh oh!

gemini-code-assist Bot commented Jul 17, 2026

Uh oh!

arekay-nv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arekay-nv left a comment

Choose a reason for hiding this comment

Review Council — Multi-AI Code Review (round 3)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arekay-nv commented Jul 17, 2026

Review Council — Multi-AI Code Review (round 3)

🔴 Must Fix

nvzhihanj commented Jul 16, 2026 •

edited

Loading

Review Council — fixes applied (`166f39a`)

Design follow-up (`706f7b5`)