Repository navigation
[Bugfix][Bench] Save per-request stage metrics in detailed results #6464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
df21dcd
49b70d8
1ed39c8
0adccec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,7 +117,16 @@ | |
| "openai-image-edits-omni", | ||
| } | ||
| ) | ||
| _STAGE_METRICS_BACKENDS = frozenset( | ||
| { | ||
| "daily-omni", | ||
| "openai-chat-omni", | ||
| "/v1/videos", | ||
| *_IMAGE_STAGE_METRICS_BACKENDS, | ||
| } | ||
| ) | ||
| _PRINT_STAGE = False | ||
| _SAVE_DETAILED = False | ||
|
|
||
|
|
||
| def maybe_enable_stage_metrics(extra_body: dict[str, Any] | None, *, enabled: bool) -> dict[str, Any] | None: | ||
|
|
@@ -135,6 +144,8 @@ def should_request_stage_metrics(args: Any) -> bool: | |
| return True | ||
|
|
||
| backend = getattr(args, "backend", None) | ||
| if getattr(args, "save_detailed", False) and backend in _STAGE_METRICS_BACKENDS: | ||
| return True | ||
| if backend in _IMAGE_STAGE_METRICS_BACKENDS: | ||
| return True | ||
|
|
||
|
|
@@ -160,6 +171,12 @@ def set_print_stage(enabled: bool) -> None: | |
| _PRINT_STAGE = bool(enabled) | ||
|
|
||
|
|
||
| def set_save_detailed(enabled: bool) -> None: | ||
| """Set whether this benchmark run retains per-request detail.""" | ||
| global _SAVE_DETAILED | ||
| _SAVE_DETAILED = bool(enabled) | ||
|
|
||
|
|
||
| def _audio_continuity_threshold_s() -> float: | ||
| """Return the per-request underrun budget (s). | ||
|
|
||
|
|
@@ -969,6 +986,20 @@ class MixRequestFuncOutput(RequestFuncOutput): | |
| duplex_session_metrics: dict[str, object] | None = None | ||
|
|
||
|
|
||
| def _request_stage_metrics(outputs: Iterable[object]) -> list[dict[str, dict]]: | ||
| """Return one stage snapshot per request while preserving alignment.""" | ||
| return [dict(getattr(output, "stage_metrics", None) or {}) for output in outputs] | ||
|
|
||
|
|
||
| def _add_detailed_stage_metrics( | ||
| result: dict[str, Any], | ||
| outputs: Iterable[object], | ||
| ) -> None: | ||
| """Attach request-scoped stage snapshots only to detailed results.""" | ||
| if _SAVE_DETAILED: | ||
| result["stage_metrics"] = _request_stage_metrics(outputs) | ||
|
|
||
|
|
||
| _IMAGE_EDITS_EXTRA_BODY_FORM_FIELDS = ( | ||
| "negative_prompt", | ||
| "num_inference_steps", | ||
|
|
@@ -3692,6 +3723,7 @@ def measured_ttft(output: RequestFuncOutput) -> float | None: | |
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] This diff inserts Evidence and suggested fixThis diff inserts Evidence: PR head patch.py:730 Suggestion: request_stage_metrics = [
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in AI assistance: TraeCode assisted with this verification and reply. |
||
| # Plain-vLLM backends (e.g. the vLLM-text perf config) return upstream | ||
| # RequestFuncOutput objects without the Mix duplex fields; read them | ||
| # tolerantly or the whole benchmark result is discarded ("fallback to | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ | |
| maybe_enable_stage_metrics, | ||
| set_print_stage, | ||
| set_request_timeout_s, | ||
| set_save_detailed, | ||
| should_request_stage_metrics, | ||
| ) | ||
|
|
||
|
|
@@ -70,6 +71,7 @@ def main(args: argparse.Namespace) -> dict[str, Any]: | |
| set_request_timeout_s(request_timeout_s) | ||
| _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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P0] PR 6464 is mergeable=false / mergeable_state=dirty at head 1ed39c8 Evidence and suggested fixPR 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in AI assistance: TraeCode assisted with this verification and reply. |
||
| args.extra_body = maybe_enable_stage_metrics( | ||
| getattr(args, "extra_body", None), | ||
| enabled=should_request_stage_metrics(args), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] This hunk makes
--save-detailedalso set `extra_body.return_stage_metrics=tru…Evidence and suggested fix
This hunk makes
--save-detailedalso setextra_body.return_stage_metrics=truefordaily-omni,openai-chat-omni,/v1/videos, and the image backends (patch.py:138-139viaserve.py:68-71). That is a request-protocol change, not JSON-only: unchangedserving_chat._filter_stage_metrics_detail(serving_chat.py:470-471, applied atserving_chat.py:2096) then sends full snapshots on every chat/daily-omni SSE chunk (daily-omnireuses chat completions atpatch.py:2577), and unchanged_apply_stage0_token_timings(patch.py:2181-2184) can fill Stage-0 ITL when client intervals are empty./v1/videosis 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-omnialready requested when tpot/itl percentiles are selected (patch.py:147).docs/cli/bench/serve.md:68-70still describes--save-detailedas saving per-request ttfs/errors only (argparse help is not defined in this repo). Update that--save-detaileddocs to say those Omni backends also ask the server for stage snapshots.Evidence: Trigger:
vllm-omni bench serve --save-detailedwith backenddaily-omni,openai-chat-omni(no tpot/itl percentiles), or/v1/videos. Adverse effect / unmet requirement: the flag now injectsextra_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-139if getattr(args, "save_detailed", False) and backend in _STAGE_METRICS_BACKENDS:/return True. patch.py:128body.setdefault(RETURN_STAGE_METRICS_FIELD, True). serve.py:68-71set_save_detailed(getattr(args, "save_detailed", False))thenargs.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-471if cls._truthy_extra_body_flag(request, "return_stage_metrics"):/return metrics; serving_chat.py:2096metrics=self._filter_stage_metrics_detail(omni_res.metrics, request),; patch.py:2181-2184if 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-141if backend in _IMAGE_STAGE_METRICS_BACKENDS:/return True. openai-chat-omni tpot/itl already requested unchanged: patch.py:147if 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in
0adccecd272b442d9552c14dbe226169727e4270: the CLI documentation now identifies the request-protocol change, affected backends, possible response/SSE overhead and Stage-0 timing fallback. Explicitreturn_stage_metrics=falseis preserved and tested. The claim is artifact correctness, not unchanged measurement overhead.AI assistance: TraeCode assisted with this verification and reply.