Repository navigation
[Bugfix][Bench] Save per-request stage metrics in detailed results - #6464
EchoHayate wants to merge 4 commits into
Conversation
Signed-off-by: Allen Wu <allenwu2795@gmail.com> Co-authored-by: TRAE CLI <traecli@bytedance.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR appears to belong to: docs/design/module/benchmarking.md. Module owners: @alex-jw-brooks @Bounty-hunter @EchoHayate, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
|
Author self-review:
Validation boundary: this fixes artifact serialization only. It does not establish a performance improvement or claim that existing stage metrics fully separate producer, connector, admission, and consumer waiting. @alex-jw-brooks @Bounty-hunter, could you please review when convenient? |
hsliuustc0106
added
the
benchmark/profiler/metrics/logger
…d-stage-metrics Co-authored-by: TRAE CLI <traecli@bytedance.com> # Conflicts: # tests/benchmarks/patch/test_patch.py
|
Current-head update after refreshing onto upstream
Evidence boundary is unchanged: this preserves already-produced per-request stage snapshots in benchmark artifacts; it does not claim a runtime performance improvement or fuller stage attribution. |
Signed-off-by: Allen Wu <allenwu2795@gmail.com> Co-authored-by: TRAE CLI <traecli@bytedance.com>
Author self-review / current-main refresh
Fresh current-head verification:
Evidence boundary remains benchmark artifact correctness; this PR does not claim a runtime performance improvement or complete producer/connector/admission/consumer waiting attribution. Current head reviewed: @alex-jw-brooks @Bounty-hunter, this is conflict-free again and ready for review once the refreshed CI completes. |
Omni ReviewBot: no human activity for 15 days@EchoHayate this pull request has had no human commit, comment or review since 2026-09-08. Please consider marking this PR as draft until work can resume. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
September 30 lifecycle follow-upI rechecked this against upstream Main now writes compact, request-aligned The proposed next step is to refresh only the explicit @alex-jw-brooks @Bounty-hunter, is a separately gated full-snapshot field still No new tests or GPU runs were performed for this audit; the September 8 |
Omni ReviewBot routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
vllm-omni-review-bot
left a comment
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
3 actionable finding(s).
CI at
1ed39c8b9d93(2026-10-10T02:31:04.509969+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Full review analysis
Scan:
| Category | Result |
|---|---|
| Tests / verification | 2 finding(s) below |
| Security | 1 finding(s) below |
| Docs / comments | no finding reported |
| Behavior / compatibility | no finding reported |
| Correctness | no finding reported |
Validated:
- [resolved] set_save_detailed mirrors set_print_stage because patched benchmark() has no save_detailed arg (patch.py:2611). Residual: serve.py is the only production arming site.
- [resolved] None→{} index alignment is implemented and tested at test_patch.py:142,150-161; residual after rebase is main's compact None sentinel / vllm_itls_ms strip (kept at patch.py:2972).
- [claim-verified] scope is the three benchmark files; no scheduler/runtime/telemetry generation changes.
- [claim-verified] unsupported backend openai stays False: test_save_detailed_does_not_request_stage_metrics_for_unsupported_backend.
- [claim-verified] scheduler/orchestrator unchanged; serving_chat._filter_stage_metrics_detail (serving_chat.py:470-472) will now emit already-built snapshots on the wire when the client asks.
- [claim-verified] non-detailed unchanged: _SAVE_DETAILED False leaves key absent (test_detailed_stage_metrics_are_opt_in).
3 actionable finding(s).
Verdict: REQUEST CHANGES
See inline comments below.
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
| os.environ["DAILY_OMNI_SAVE_EVAL_ITEMS"] = "1" | ||
| _use_endpoint_backend_when_implicit(args) | ||
| set_print_stage(getattr(args, "print_stage", False)) | ||
| set_save_detailed(getattr(args, "save_detailed", False)) |
There was a problem hiding this comment.
[P0] PR 6464 is mergeable=false / mergeable_state=dirty at head 1ed39c8
Evidence and suggested fix
PR 6464 is mergeable=false / mergeable_state=dirty at head 1ed39c8. This head’s serve.py:68 adds set_save_detailed(...) beside set_print_stage and has no Video-MME/timeout calls, while current main’s serve.main independently runs VIDEOMME_SAVE_EVAL_ITEMS and set_request_timeout_s(omni_request_timeout_s) at the same setup site. Rebase onto main and keep both wirings; resolving the serve.py conflict by taking this head would drop --omni-request-timeout-s.
Evidence: GitHub pulls/6464 (live, not in the diff): mergeable=false, mergeable_state=dirty, head=1ed39c8b9d935e003e5d30101b82db45790f8c64. PR-time vllm_omni/benchmarks/serve.py:23 set_save_detailed, and serve.py:68 set_save_detailed(getattr(args, "save_detailed", False)) — serve.py:59-68 has seed/daily-omni env flags then set_print_stage/set_save_detailed only, no videomme or timeout. Current main vllm_omni/benchmarks/serve.py (not in this PR diff; live https://raw.githubusercontent.com/vllm-project/vllm-omni/main/vllm_omni/benchmarks/serve.py): imports set_request_timeout_s (not set_save_detailed) and in main() runs if getattr(args, "videomme_save_eval_items", False): / os.environ["VIDEOMME_SAVE_EVAL_ITEMS"] = "1" then request_timeout_s = getattr(args, "omni_request_timeout_s", None) / set_request_timeout_s(request_timeout_s) before set_print_stage. Trigger: merge/rebase of this dirty PR. Adverse effect: taking this head’s serve.py as conflict resolution drops --omni-request-timeout-s and Video-MME eval-item wiring from main.
There was a problem hiding this comment.
Addressed in 0adccecd272b442d9552c14dbe226169727e4270 by merging the inspected upstream and preserving set_request_timeout_s, Video-MME evaluation setup, endpoint initialization and detailed-metrics wiring together. The tested commit merges cleanly with the inspected origin/main@131a7731a. This is local conflict verification, not a claim that GitHub CI or the merge gate has passed.
AI assistance: TraeCode assisted with this verification and reply.
| "input_lens": [output.prompt_len for output in outputs], | ||
| "errors": [output.error for output in outputs], | ||
| } | ||
| _add_detailed_stage_metrics(result, outputs) |
There was a problem hiding this comment.
[P1] This diff inserts _add_detailed_stage_metrics(result, outputs) immediately af…
Evidence and suggested fix
This diff inserts _add_detailed_stage_metrics(result, outputs) immediately after the result dict is built, writing a full per-request result["stage_metrics"] list (missing snapshots as {}) when _SAVE_DETAILED is set. Current main already serializes compact, request-aligned result["request_stage_metrics"] at that same builder via _compact_request_stage_metrics (token count, finish_reason, audio frames/duration; missing snapshots as None; vllm_itls_ms stripped), GitHub reports mergeable=false / dirty, and test_benchmark_preserves_stage_metrics_request_order_and_missing_snapshots asserts that compact contract. Rebase: keep the compact request_stage_metrics block unconditionally and attach the full snapshots only under --save-detailed as a second key — do not replace request_stage_metrics with stage_metrics, and do not land two arrays with different missing sentinels without an explicit name/schema split.
Evidence: PR head patch.py:730 result["stage_metrics"] = _request_stage_metrics(outputs) (missing → {}). Live main patch.py:3602-3606 request_stage_metrics = [_compact_request_stage_metrics(...) for output in outputs] then result["request_stage_metrics"] = request_stage_metrics. Live main tests/benchmarks/patch/test_patch.py:2101 assert result["request_stage_metrics"] == expected with expected None gaps and no itls.
Suggestion: request_stage_metrics = [
_compact_request_stage_metrics(getattr(output, "stage_metrics", None)) for output in outputs
]
if any(request_stage_metrics):
result["request_stage_metrics"] = request_stage_metrics
_add_detailed_stage_metrics(result, outputs)
There was a problem hiding this comment.
Addressed in 0adccecd272b442d9552c14dbe226169727e4270: compact request_stage_metrics remains independent of detailed mode and the full map is an additional opt-in stage_metrics key. The documentation explicitly separates null and {} missing values. Tests cover both schemas through real benchmark construction and CLI-to-saved-JSON execution, including out-of-order completion, failed requests and warmups.
AI assistance: TraeCode assisted with this verification and reply.
| return True | ||
|
|
||
| backend = getattr(args, "backend", None) | ||
| if getattr(args, "save_detailed", False) and backend in _STAGE_METRICS_BACKENDS: |
There was a problem hiding this comment.
[P2] This hunk makes --save-detailed also set `extra_body.return_stage_metrics=tru…
Evidence and suggested fix
This hunk makes --save-detailed also set extra_body.return_stage_metrics=true for daily-omni, openai-chat-omni, /v1/videos, and the image backends (patch.py:138-139 via serve.py:68-71). That is a request-protocol change, not JSON-only: unchanged serving_chat._filter_stage_metrics_detail (serving_chat.py:470-471, applied at serving_chat.py:2096) then sends full snapshots on every chat/daily-omni SSE chunk (daily-omni reuses chat completions at patch.py:2577), and unchanged _apply_stage0_token_timings (patch.py:2181-2184) can fill Stage-0 ITL when client intervals are empty. /v1/videos is not SSE but still gates response metrics on the same flag (serving_video.py:447, unchanged). Image backends were already always-on (patch.py:140-141); openai-chat-omni already requested when tpot/itl percentiles are selected (patch.py:147). docs/cli/bench/serve.md:68-70 still describes --save-detailed as saving per-request ttfs/errors only (argparse help is not defined in this repo). Update that --save-detailed docs to say those Omni backends also ask the server for stage snapshots.
Evidence: Trigger: vllm-omni bench serve --save-detailed with backend daily-omni, openai-chat-omni (no tpot/itl percentiles), or /v1/videos. Adverse effect / unmet requirement: the flag now injects extra_body.return_stage_metrics, so the server request/response (and Stage-0 ITL fill) is no longer the compact workload, while docs still describe JSON-only per-request save. patch.py:138-139 if getattr(args, "save_detailed", False) and backend in _STAGE_METRICS_BACKENDS: / return True. patch.py:128 body.setdefault(RETURN_STAGE_METRICS_FIELD, True). serve.py:68-71 set_save_detailed(getattr(args, "save_detailed", False)) then args.extra_body = maybe_enable_stage_metrics(..., enabled=should_request_stage_metrics(args)). Unchanged by this diff, present in the PR-time tree: serving_chat.py:470-471 if cls._truthy_extra_body_flag(request, "return_stage_metrics"): / return metrics; serving_chat.py:2096 metrics=self._filter_stage_metrics_detail(omni_res.metrics, request),; patch.py:2181-2184 if exact_itls: / if sum(measured_itls) > 0: / output.itl = measured_itls. docs/cli/bench/serve.md:68-70 --save-detailed / "When saving the results, whether to include per request " / "information such as response, error, ttfs, tpots, etc.". Image backends already always-on unchanged: patch.py:140-141 if backend in _IMAGE_STAGE_METRICS_BACKENDS: / return True. openai-chat-omni tpot/itl already requested unchanged: patch.py:147 if backend == "openai-chat-omni" and selected_metrics.intersection({"tpot", "itl"}):.
Suggestion: if getattr(args, "save_detailed", False) and backend in _STAGE_METRICS_BACKENDS:
return True
There was a problem hiding this comment.
Addressed in 0adccecd272b442d9552c14dbe226169727e4270: the CLI documentation now identifies the request-protocol change, affected backends, possible response/SSE overhead and Stage-0 timing fallback. Explicit return_stage_metrics=false is preserved and tested. The claim is artifact correctness, not unchanged measurement overhead.
AI assistance: TraeCode assisted with this verification and reply.
Omni ReviewBot: finding feedback[p1] This diff inserts See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
…napshots Signed-off-by: Allen Wu <allenwu2795@gmail.com> Co-authored-by: TRAE CLI <traecli@bytedance.com>
EchoHayate
changed the title
Author self-review — October 10 refreshCurrent head reviewed:
Reproduction in a compatible vLLM-Omni test environment: python -m pytest -q \
tests/benchmarks/patch/test_patch.py \
tests/benchmarks/test_serve_cli.py \
tests/benchmarks/metrics/test_metrics.py \
tests/benchmarks/patch/test_benchmark_result.py \
tests/benchmarks/test_diffusion_backends_metrics.py \
tests/benchmarks/test_diffusion_benchmark_warmups.py \
tests/benchmarks/test_videomme_local_source.pyLocal environment: macOS arm64, CPython 3.12.13, vLLM 0.31 source at AI assistance: TraeCode assisted with the code/test refresh, local checks and this review summary. |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 600s (try 3 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; falling back to direct/cursor/auto). |
3 participants
Current refresh — October 10, 2026
Current head:
0adccecd272b442d9552c14dbe226169727e4270.The current-head author self-review and reproduction commands are in the October 10 comment below. Prior validation numbers and GPU results in this description remain historical evidence at their stated revisions, not fresh-head qualification. Official CI and formal review remain separate gates.
AI assistance: TraeCode assisted with the code/test refresh, local checks and English review replies.
The refreshed result preserves compact
request_stage_metrics(missing:null) and adds opt-in fullstage_metrics(missing:{}). The CLI documentation covers protocol opt-in, explicit opt-out and response/SSE overhead.Summary
Preserve the per-request stage snapshots that the canonical benchmark client
already receives when
--save-detailedis enabled.Today,
--print-stagecan aggregateMixRequestFuncOutput.stage_metrics, butthe benchmark result builder never copies those snapshots into the detailed
JSON result. This makes aggregate stage output visible while dropping the raw
request-aligned evidence needed for attribution.
Changes
return_stage_metrics=truefor supported Omni backends duringdetailed runs;
stage_metricsmapping per request, aligned withinput_lens,ttfts,itls,generated_texts, anderrors;{}for requests without a snapshot instead of shifting indexes;Scope
This adds canonical benchmark artifact serialization and requests server snapshots for supported detailed runs. It does not add
runtime events, change scheduler/orchestrator behavior, expose
stage_durations, or introduce model-specific telemetry.Validation
and detailed-only result gate;
tests/benchmarks/patch/test_patch.py:23 passed, 15 warnings;git diff --check: passed;Related context: #6453.