Repository navigation
[Perf] Allow disabling per-request stage metrics - #8554
Conversation
- Add collect_stage_metrics, defaulting to True, across the CLI, Python API, engine, and orchestrator configuration path. - Add --collect-stage-metrics / --no-collect-stage-metrics CLI options while preserving existing default behavior. - Reject configurations combining log_stats=True with collect_stage_metrics=False. - Propagate the setting from the orchestrator to each stage pool. - When disabled, skip stage output timestamp tracking and avoid building or routing per-request stage metrics entirely. Signed-off-by: Alessio Netti <26897207+AlessioNetti@users.noreply.github.com>
- Verify disabling stage metrics prevents metric construction and routing. - Verify stage pools skip output-timestamp tracking when collection is disabled. - Verify the orchestrator rejects log_stats=True with stage metrics disabled. - Verify the CLI rejects --log-stats combined with --no-collect-stage-metrics. Signed-off-by: Alessio Netti <26897207+AlessioNetti@users.noreply.github.com>
Signed-off-by: Alessio Netti <26897207+AlessioNetti@users.noreply.github.com>
- Using pre-existing OmniRequestOutput mock instead of SimpleNamespace Signed-off-by: Alessio Netti <26897207+AlessioNetti@users.noreply.github.com>
- Now accounting for headless configs - ValueError message updated to better reflect defaults - Fix handling of log_stats/disable_log_stats when creating AsyncOmni/DuplexOmni instance Signed-off-by: Alessio Netti <26897207+AlessioNetti@users.noreply.github.com>
Signed-off-by: Alessio Netti <26897207+AlessioNetti@users.noreply.github.com>
|
This PR appears to belong to: docs/design/module/observability.md, docs/design/module/profiling.md. Module owners: @gcanlin @lishunyang12 @vraiti Routing: @gcanlin via module named in the PR description; @lishunyang12 via module of the changed files, semantic router; @vraiti via module of the changed files, semantic router @AlessioNetti, 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. |
Omni ReviewBot routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
linyueqian
left a comment
There was a problem hiding this comment.
The metrics opt-out makes sense for per-frame workloads, but the skipped path also handles request cleanup and speech completion metadata. Both of these still need to function correctly when metric collection is disabled.
I reviewed d3168cb statically without executing fork code or tests, and I have not verified the performance claim.
| stage_metrics = None | ||
| segment_finished = req_state.streaming.enabled and req_state.streaming.segment(stage_id).finished | ||
| if output.finished or segment_finished: | ||
| if self.collect_stage_metrics and (output.finished or segment_finished): |
There was a problem hiding this comment.
[important] When collect_stage_metrics=False, completed AR requests leak entries in MultimodalOutputProcessor._native_text_metrics_by_request. Since _update_stats_from_output stores num_generation_tokens even when log_stats=False, and the normal completion path only drains this cache via build_stage_metrics, skipping that call permanently leaks one record per completed request because release_binding does not clear it. Please decouple cache cleanup from metrics construction or avoid creating these records entirely when collection is disabled. A regression test should complete several requests with both flags disabled and verify that the cache remains empty.
| segment_finished = req_state.streaming.enabled and req_state.streaming.segment(stage_id).finished | ||
| if output.finished or segment_finished: | ||
| if self.collect_stage_metrics and (output.finished or segment_finished): | ||
| stage_metrics = pool.build_stage_metrics( |
There was a problem hiding this comment.
[important] This change also drops the stage 0 finish_reason and token count from the speech API. Because SpeechOutputTokenCounter reads these exclusively from stage_metrics, running with --no-collect-stage-metrics and --disable-log-stats leaves stage0_finish_reason=None and output_tokens=0. This causes Qwen3TTSAdapter.validate_generation to skip its length-limit check, meaning a Qwen3-TTS Base request that exhausts its codec budget will return incomplete audio as a successful response rather than raising Qwen3TTSCodecLimitError. Please ensure completion and usage fields are preserved independently of optional timing metrics, and cover the flag-off path with the existing Base truncation, error, and usage expectations.
There was a problem hiding this comment.
Thanks for flagging this - I addressed the issue with commit 54710a3.
Now StagePool.build_stage_metrics() always builds and returns a minimal StageRequestMetrics object with all its non-optional fields (including in/out token counts plus finish reason) populated. OrchestratorAggregator._merge_stage_metric_event() then processes the minimal metrics set, and skip any expensive book-keeping and merging work.
I also added regression tests to ensure that the minimal set of StageRequestMetrics fields is always preserved, as well as new Qwen3 TTS tests that run with stage metrics enabled and disabled:
test_qwen3_tts_base_nonstream_completion_and_usage: a "length" finish reason must raiseQwen3TTSCodecLimitErrorwithout publishing usage; a normal "stop" must return audio with correct token usage.test_qwen3_tts_base_sse_completion_usage: checks that successful streaming emits audio and a completion event with correct usage, without an error event.
…t_stage_metrics=False - Updated StagePool.build_stage_metrics() to always build a minimal StageRequestMetrics object containing in/out token counts, finish reason, batch ID, and all non-optional fields, even when collect_stage_metrics=False; - Similarly, OrchestratorAggregator._merge_stage_metric_event() now always processes this minimal metrics set, and skips the expensive telemetry merging when collect_stage_metrics=False; - This fixes broken termination logic for Qwen3 TTS, which relied on the returned token count and finish reason; - Tests have been updated to ensure that the right fields are returned depending on collect_stage_metrics. Signed-off-by: Alessio Netti <26897207+AlessioNetti@users.noreply.github.com>
…_metrics=False Signed-off-by: Alessio Netti <26897207+AlessioNetti@users.noreply.github.com>
Omni ReviewBot: supersededThe CI failure noted on |
Signed-off-by: Alessio Netti <26897207+AlessioNetti@users.noreply.github.com>
linyueqian
left a comment
There was a problem hiding this comment.
I can confirm the earlier issues are fixed at f95db5ff, as completed requests and segment boundaries now properly drain the native metrics cache, and disabling diagnostics preserves the token counts and finish reasons used by speech validation and usage reporting. The new regression tests cover the cache cleanup as well as the Qwen3-TTS success and truncation paths with diagnostics both on and off.
Could you also refresh the PR description and validation SHAs? The lightweight stage metrics are now built and routed, and the listed 01445d98 results predate these recent fixes. This is just a non-blocking description update. As a note, my review was strictly static across the code and tests, and I haven't run the fork code or remeasured the performance claim.
|
Thank you for the review @linyueqian - I just updated the PR description like you suggested. |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
1 similar comment
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). |
Signed-off-by: Alessio Netti <26897207+AlessioNetti@users.noreply.github.com>
Purpose
We use a custom vllm-omni implementation of the Moshi/Hibiki model architecture; with version
0.28we noticed a significant performance degradation, approximately halving the maximum number of concurrent real-time streams on one instance. For our testing, we used a host equipped with a single H100 GPU and with an AMD 7313P CPU.After collecting profiling data, we discovered that ~40% of the samples collected for the vllm-omni orchestrator process are in
OrchestratorAggregator._merge_stage_metric_event(). In turn, this turned out to be related to book-keeping of per-request stage metrics, which is performed by default even whenlog_stats=False.This overhead is particularly pronounced in the case of Moshi/Hibiki, as it produces output audio frames at a ~12.5Hz cadence. MiniCPM, on the other hand, operates at a ~1Hz interaction cadence, resulting in much less book-keeping work required to merge stage metrics and thus less overhead.
This PR addresses this performance issue by introducing a new
collect_stage_metricsorchestrator flag (as well as--collect-stage-metrics/--no-collect-stage-metricsCLI flags) that allows disabling output timestamp tracking and avoid building most per-request stage metrics.Note that a small subset of stage metrics (in/out tokens, finish reason, stage gen time) is still collected with
collect_stage_metrics=False, as these are needed for correct functioning of some TTS models (e.g., Qwen3). However, the expensive book-keeping and merging work is nonetheless avoided, eliminating most metrics overhead and restoring performance to expected levels.The option requires setting
log_stats=False, and conflicting configurations are rejected.log_statsdefaults toFalsein Python andTruein API serving, whilecollect_stage_metrics=Trueby default, maintaining legacy behavior.Test Plan
vLLM Version: 0.31.0
vLLM-Omni Commit:5f95115e
Tested branch commit: f95db5f
For the general orchestrator and serving tests:
For the Qwen3 TTS-specific tests:
Test Result
collect_stage_metrics=Falsestill carries required request information (in/out token counts, finish reason).AI assistance: Codex (GPT-6.1 Sol) was used to draft the changes needed to selectively disable per-request telemetry.
BEFORE SUBMITTING: read CONTRIBUTING.md and run the precheck-pr skill with the code agent for a self-check against project conventions.