Skip to content

[Bugfix][Doc] fix Prometheus per-stage audio metrics and drop RTF - #4798

Closed
ZacheryAU wants to merge 6 commits into
vllm-project:mainfrom
ZacheryAU:prometheus_rtf
Closed

ZacheryAU wants to merge 6 commits into
vllm-project:mainfrom
ZacheryAU:prometheus_rtf

Conversation

@ZacheryAU

@ZacheryAU ZacheryAU commented Jun 30, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

Solve #4681

Summary

Test Plan

vLLM Version:
0.26.0
vLLM-Omni Commit:
900a7f0

  • Collect and compare results of /metrics | grep audio_ of an audio-output model to see if
    • RTFs are removed
    • other audio metrics can be shown normally
  • Run python -m pytest tests/metrics/test_modality.py to see if any failure

Test Result

/metrics before

metrics_before.txt

/metrics after

metrics_after.txt

result of pytest

Show more
============================================================= 26 passed, 17 warnings in 0.19s ==============================================================

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@Gaohan123 Gaohan123 linked an issue Jul 2, 2026 that may be closed by this pull request
1 task done
@Gaohan123 Gaohan123 added this to the v0.24.0 milestone Jul 2, 2026
Comment thread docs/design/metrics.md Outdated
- Expose pipeline-level request and latency metrics that span the full multi-stage execution (orchestrator scope).
- Preserve all upstream vLLM per-engine metrics (`vllm:*`) for stages backed by an AR LLM engine, and reshape their `engine` label into `stage` + `replica` so multi-replica deployments gain per-replica visibility automatically.
- Expose per-modality SLO metrics that the upstream `vllm:*` families do not capture — audio TTFP / RTF / duration / frames / streaming continuity / silent-loss.
- Expose per-modality SLO metrics that the upstream `vllm:*` families do not capture — audio TTFP / duration / frames / streaming continuity / silent-loss.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here it is the general metric rather than stage-level. So we should keep it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted

Comment thread docs/usage/metrics.md
|--------|------|-------------|-------------|
| `vllm:omni_audio_ttfp_s` | Histogram | — | Time from request arrival to first audio packet/frame |
| `vllm:omni_audio_duration_s` | Histogram | — | Audio content duration (`audio_frames / sample_rate`) |
| `vllm:omni_audio_rtf` | Histogram | — | Real-time factor (`stage_gen_time_s / audio_duration_s`); streaming TTS SLO red line `< 1`; uses `RTF_BUCKETS` |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same

@Gaohan123 Gaohan123 modified the milestones: v0.24.0, v0.26.0 Jul 7, 2026
@hsliuustc0106 hsliuustc0106 added bug Something isn't working core related to core module: cache, scheduler, engine, worker, modelrunner documentation Improvements or additions to documentation labels Jul 17, 2026 — with ChatGPT Codex Connector
Comment thread vllm_omni/metrics/modality.py Outdated
"""
mm = getattr(engine_outputs, "multimodal_output", None)
if isinstance(mm, dict) and mm:
if isinstance(mm, Mapping) and mm:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bug fix is specifically about supporting non-dict mappings, but the relevant tests still pass plain dictionaries. Add a regression case with MultimodalPayload (or another non-dict Mapping) for both this top-level path and the nested path below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased and added MultimodalPayload inside test_modality.py

@hsliuustc0106 hsliuustc0106 added the ready label to trigger buildkite CI label Jul 23, 2026
@hsliuustc0106

Copy link
Copy Markdown
Collaborator

fix conflicts

Signed-off-by: ZacheryAU <zachery.au@gmail.com>
Signed-off-by: ZacheryAU <zachery.au@gmail.com>
Signed-off-by: ZacheryAU <zachery.au@gmail.com>
Signed-off-by: ZacheryAU <zachery.au@gmail.com>
@ZacheryAU

Copy link
Copy Markdown
Contributor Author

fix conflicts

rebased

Signed-off-by: ZacheryAU <zachery.au@gmail.com>
@Gaohan123 Gaohan123 modified the milestones: v0.26.0, v0.28.0 Aug 4, 2026
@hsliuustc0106
hsliuustc0106 requested a review from NickCao as a code owner August 12, 2026 04:44
@hsliuustc0106 hsliuustc0106 added ready label to trigger buildkite CI and removed ready label to trigger buildkite CI labels Aug 12, 2026
@Gaohan123 Gaohan123 modified the milestones: v0.28.0, v0.30.0 Sep 2, 2026
@vllm-omni-review-bot

Copy link
Copy Markdown

Omni ReviewBot: no human activity for 50 days

@ZacheryAU this pull request has had no human commit, comment or review since 2026-08-04. Please consider closing this PR if the work is no longer planned. 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.

@hsliuustc0106

Copy link
Copy Markdown
Collaborator

@ZacheryAU this PR is labeled ready + high priority, but CI is failing on the latest commit:

Could you please take a look and push an update to get CI green? Once the checks pass we can proceed with review/merge. Thanks!

@vllm-omni-review-bot

Copy link
Copy Markdown
Omni ReviewBot routing record

Assigned Strict on zcode (GLM-5.3-Flash) under experiment fleet-strict-cursor-grok46-zcode-glm53flash-5050-c5-z10-20261002.

@vllm-omni-review-bot vllm-omni-review-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omni ReviewBot review

8 actionable finding(s).

CI at 5c4972d0f266 (2026-10-09T23:17:52.057501+00:00): all required and operator-watched checks reported green. Observed Buildkite: buildkite/vllm-omni-amd-ci (failed), buildkite/vllm-omni-npu-ci (passed), buildkite/vllm-omni (passed), and 1 more.

Full review analysis

Scan:

Category Result
Tests / verification 6 finding(s) below
Security no finding reported
Docs / comments 2 finding(s) below
Behavior / compatibility no finding reported
Correctness no finding reported

Validated:

  • [claim-verified] PR 'Run pytest tests/metrics/test_modality.py → 26 passed': head file contains exactly 26 test functions (TestRegistration 1 + TestAudio 8 + TestObserveModalityAtFinalize 7 + TestObserveAudioFirstPacket 4 + TestObserveAudioStreamingFinalize 4 + TestBucketSelection 2)
  • [validated] vllm_omni/metrics/modality.py:155 hunk — gen_time_s removal is safe: stage_metrics is still consumed at modality.py:165 (n_frames = int(getattr(stage_metrics, "audio_generated_frames", 0) or 0)), the signature is unchanged, and the production caller vllm_omni/entrypoints/omni_base.py:608-615 still passes stage_metrics=_m
  • [validated] vllm_omni/metrics/definitions.py:31 AUDIO_RTF stem, :46-49 MEAN/MEDIAN/STD/PERCENTILES_AUDIO_RTF and :254 compute_audio_rtf are NOT dead code: live bench callers at vllm_omni/benchmarks/metrics/metrics.py:117-120,771,928-931, vllm_omni/benchmarks/patch/patch.py:968,1240, vllm_omni/experimental/fullduplex/client.py:354 — Prometheus-only removal matches the stated scope
  • [validated] MultimodalPayload implements Mapping with get() over tensors-then-metadata (vllm_omni/outputs/mm_outputs.py:110-114) and _METADATA_TENSOR_KEYS={'sr','sample_rate','audio_sample_rate'} (line 26), so defs.resolve_audio_sample_rate (definitions.py:305-340) still resolves rates from production payloads via the Mapping protocol
  • [validated] new regression tests drive the real helpers, not fixture-authored values: extract_mm_output (vllm_omni/metrics/utils.py:82-90, isinstance Mapping check), count_audio_frames (:163-171, mm_out.get('audio')), sample-rate fallback DEFAULT_AUDIO_SAMPLE_RATE=24000 (definitions.py:240); 24000-frame tensor / 24000 Hz → the asserted 1.0s is derived, and test_audio_path_nested_multimodal_payload (tests/metrics/test_modality.py:205) hits the outputs[0] branch at utils.py:85-89
  • [validated] gen_time_s removal leaves no orphan: stage_gen_time_ms is still consumed bench-side (STAGE_GEN_TIME_MS constants, definitions.py:87) and omni_base.py:608-615 passes the whole stage_metrics object unchanged

8 actionable finding(s).

Checked, no defect found:

  • vllm_omni/metrics/definitions.py:254 — Checked whether compute_audio_rtf is dead code after this PR removes its server-side caller — it is not: live callers remain in vllm_omni/benchmarks/patch/patch.py (:968, :1240) and vllm_omni/experimental/fullduplex/client.py (import :17, call :354), so removing the helper would break them; keeping it is correct. The stale 'shared by server-side observe' section header at definitions.py:251-253 is filed separately.

Verdict: REQUEST CHANGES

Findings

  • [P0] The merge gate reports this branch DIRTY / CONFLICTING ('the branch conflicts w… — tests/metrics/test_modality.py
Evidence for The merge gate reports this branch DIRTY / CONFLICTING ('the branch conflicts w…

The merge gate reports this branch DIRTY / CONFLICTING ('the branch conflicts with or trails the base') and both required checks are red (buildkite/vllm-omni-amd-ci build 10500, buildkite/vllm-omni-intel-ci build 6937). Head 5c4972d is itself a merge of main into prometheus_rtf, so the PR body's test evidence ('26 passed', produced at pre-merge commit 900a7f0) does not describe the current tree. Please re-sync onto current main and re-run python -m pytest tests/metrics/test_modality.py at the new head before re-requesting review. For reference, at head 5c4972d the resolution currently looks intact: zero remaining AUDIO_RTF_METRIC / RTF_BUCKETS / observe_audio_rtf references anywhere in the repo, and the new MultimodalPayload tests are present (tests/metrics/test_modality.py:205, whose from vllm_omni.engine.mm_outputs import MultimodalPayload resolves via the deprecation shim at vllm_omni/engine/mm_outputs.py:3) — preserve exactly that state in the re-resolution.

Evidence: Tree-level assertions verified at PR-time head 5c4972d: tests/metrics/test_modality.py:205 def test_audio_path_nested_multimodal_payload(self): — anchor exists as cited; tests/metrics/test_modality.py:7 from vllm_omni.engine.mm_outputs import MultimodalPayload resolves because vllm_omni/engine/mm_outputs.py:3 from vllm_omni.outputs.mm_outputs import * # noqa: F401,F403 re-exports the class defined at vllm_omni/outputs/mm_outputs.py:81 class MultimodalPayload(Mapping):, and pyproject.toml:293 filterwarnings = [ (lines 294-296 contain only two specific ignores, no 'error') does not escalate the shim's DeprecationWarning; repo-wide grep for AUDIO_RTF_METRIC|RTF_BUCKETS|observe_audio_rtf|omni_audio_rtf returns no matches (removals intact) and grep for ^<<<<<<<|^>>>>>>> returns no matches (no in-tree conflict markers). The decisive adverse state is OUTSIDE this diff and outside the tree — present only in the pipeline's gate/fetch step outputs and live CI, not re-checkable by file reads: gate_report 'MERGE STATE: DIRTY / CONFLICTING — the branch conflicts with or trails the base. Flag this as a blocking issue.' plus red required builds (vllm-omni-amd-ci 10500, vllm-omni-intel-ci 6937), and the '26 passed' test evidence attached to pre-merge commit 900a7f0 while the reviewed head is merge commit 5c4972d — unmet requirement: a mergeable branch with green required checks and test evidence describing the reviewed head; trigger: any merge attempt or review approval on the current head; adverse effect: the PR cannot merge cleanly and the merge resolution is untested by the cited green run.

- **[P2] This diff drops the audio_rtf family, but two in-code self-descriptions still s…** — `tests/metrics/test_modality.py`
Evidence for This diff drops the audio_rtf family, but two in-code self-descriptions still s…

This diff drops the audio_rtf family, but two in-code self-descriptions still say seven: tests/metrics/test_modality.py:35 '# Family registration — 7 audio families served from modality.py' sits directly above the 6-entry _EXPECTED_FAMILIES (lines 39-46), and vllm_omni/metrics/modality.py:3 says '7 audio business-semantic metric families.' while modality.py registers exactly six families (lines 38-72). Fix both counts to six to match the behavior this PR introduces; docs/design/metrics.md was already updated to 'six'.

Evidence: Trigger: PR removes the audio_rtf family (diff deletes _audio_rtf_family, observe_audio_rtf, AUDIO_RTF_METRIC, RTF_BUCKETS) and updates docs to 'six', but leaves stale counts in code. Decisive lines, unchanged by this diff, present in the PR-time tree: tests/metrics/test_modality.py:35 # Family registration — 7 audio families served from modality.py directly above the 6-entry list tests/metrics/test_modality.py:39-46 (_EXPECTED_FAMILIES = [ ... ending defs.AUDIO_SKIPPED_REQUESTS_METRIC, ]); vllm_omni/metrics/modality.py:3 7 audio business-semantic metric families. Text-path metrics (TTFT / ITL / while modality.py:38-72 registers exactly six families (_audio_ttfp_family, _audio_duration_family, _audio_frames_family, _audio_underrun_family, _audio_continuity_ok_family, _audio_skipped_family). Adverse effect: misleading self-documentation contradicting the metric contract this PR corrects.

Suggestion: # Family registration — 6 audio families served from modality.py

- **[P2] This diff migrates the finalize tests to MultimodalPayload (top-level at tests/…** — `tests/metrics/test_modality.py`
Evidence for This diff migrates the finalize tests to MultimodalPayload (top-level at tests/…

This diff migrates the finalize tests to MultimodalPayload (top-level at tests/metrics/test_modality.py:186, nested at :205, plus the empty-payload case) but leaves the one test that exists to check sample-rate resolution on the legacy dict shape production no longer emits: engine_outputs=_Bag(multimodal_output={"sample_rate": 16000}) at tests/metrics/test_modality.py:246. On the real type, rates ride MultimodalPayload.metadata — vllm_omni/outputs/mm_outputs.py:26 relocates sr/sample_rate/audio_sample_rate into .metadata and :110-114 get() reads tensors-then-metadata, which resolve_int_by_sequential_keys (vllm_omni/metrics/utils.py:57) reaches via Mapping.get. No test on this head covers that route (zero direct resolve_audio_sample_rate callers under tests/), and the migrated tests' 1.0s assertions currently pass via the DEFAULT_AUDIO_SAMPLE_RATE=24000 fallback (definitions.py:240) coinciding with the 24000-sample tensor. Extend the migration to this test:

        engine_outputs=_Bag(
            multimodal_output=MultimodalPayload(
                tensors={"audio": torch.zeros(16000)}, metadata={"sample_rate": 16000}
            )
        ),

Evidence: Current trigger: on this PR head, tests/metrics/test_modality.py:246 engine_outputs=_Bag(multimodal_output={"sample_rate": 16000}), — the only rate-resolution finalize test still on the legacy plain-dict shape — while the same diff migrates its siblings to the real payload type (:186-187 payload = MultimodalPayload(tensors={"audio": torch.zeros(24000)}), nested case at :205-211). Adverse effect: the production route for sample rates on the real type is untested. Unchanged by this diff, present in the PR-time tree: vllm_omni/outputs/mm_outputs.py:110-114 def get(self, key: str, default: Any = None) -> Any: / if key in self.tensors: / return self.metadata.get(key, default), fed by :26 _METADATA_TENSOR_KEYS: frozenset[str] = frozenset({"sr", "sample_rate", "audio_sample_rate"}) (relocated into .metadata in consolidate_tensors, :169-173), dispatched via vllm_omni/metrics/utils.py:57 raw = source.get(key) if isinstance(source, Mapping) else getattr(source, key, None) (MultimodalPayload subclasses Mapping). Grep over tests/ shows no direct resolve_audio_sample_rate callers and no finalize test supplying payload metadata; meanwhile the migrated tests' assert ("observe_audio_duration", "1", "0", 1.0) in stub.calls (test_modality.py:199, :222) passes only through the fallback vllm_omni/metrics/definitions.py:339-340 rate = resolve_audio_sample_rate_or_none(source) / return rate if rate is not None else default with :240 DEFAULT_AUDIO_SAMPLE_RATE = 24000, coinciding with the 24000-sample tensor — so a regression in payload metadata routing would not fail any test in this file.

Suggestion: engine_outputs=_Bag(
multimodal_output=MultimodalPayload(
tensors={"audio": torch.zeros(16000)}, metadata={"sample_rate": 16000}
)
),

- **[P2] This diff deletes the vllm:omni_audio_rtf row from the catalog in docs/design/m…** — `docs/design/metrics.md`
Evidence for This diff deletes the vllm:omni_audio_rtf row from the catalog in docs/design/m…

This diff deletes the vllm:omni_audio_rtf row from the catalog in docs/design/metrics.md and renames the section '### Audio (7)' to '### Audio (6)', but the Objectives bullet at line 9 of the same document still advertises 'audio TTFP / RTF / duration / frames / streaming continuity / silent-loss' as exposed per-modality SLO metrics — it now names a metric this PR removed. Drop 'RTF' from that bullet so the intro matches the catalog below it. (This is the only remaining stale RTF reference in either metrics doc.)

Evidence: docs/design/metrics.md:9 (unchanged by this diff, present at PR-time head): - Expose per-modality SLO metrics that the upstream \vllm:*` families do not capture — audio TTFP / RTF / duration / frames / streaming continuity / silent-loss.— still advertises RTF. The same diff removes the metric and its row: docs/design/metrics.md:170 at head reads### Audio (6)and the table (lines 176–181) lists only ttfp_s, duration_s, frames_total, underrun_s, continuity_ok_total, skipped_requests_total — no rtf row; the diff deletes| vllm:omni_audio_rtf | Histogram | — | Real-time factor ... |plusAUDIO_RTF_METRIC/RTF_BUCKETSin definitions.py andobserve_audio_rtf` in modality.py. Trigger: reader consults the Objectives intro of the merged doc; adverse effect: it points to vllm:omni_audio_rtf, which no longer exists, contradicting the catalog in the same file.

Suggestion: - Expose per-modality SLO metrics that the upstream vllm:* families do not capture — audio TTFP / duration / frames / streaming continuity / silent-loss.

- **[P2] This diff drops `vllm:omni_audio_rtf` (7 → 6 audio families), leaving three tot…** — `docs/design/metrics.md`
Evidence for This diff drops `vllm:omni_audio_rtf` (7 → 6 audio families), leaving three tot…

This diff drops vllm:omni_audio_rtf (7 → 6 audio families), leaving three total counts stale: docs/design/metrics.md:133 ('the 15 vllm:omni_* families'), docs/usage/metrics.md:10 ('the 15 vllm:omni_* families'), and the modality.py:3 docstring ('7 audio business-semantic metric families'). The PR already updated the per-section counts ('six audio families' at docs/design/metrics.md:73, '### Audio (6)' at :170), and the docs' own tables now sum to Pipeline (4) + Audio (6) + Transfer (4) = 14. Update both totals 15 → 14 and the docstring 7 → 6 in this PR — it is the change that made them wrong.

Evidence: Trigger: this diff deletes _audio_rtf_family/observe_audio_rtf (vllm_omni/metrics/modality.py) and AUDIO_RTF_METRIC/RTF_BUCKETS (vllm_omni/metrics/definitions.py), and updates docs/design/metrics.md:73 to 'OmniModalityMetrics registers six audio families' and :170 to '### Audio (6)'. Adverse effect: the totals contradict the PR-updated tables. Unchanged-by-this-diff lines present in the PR-time tree: docs/design/metrics.md:133 'All metrics — both the 15 vllm:omni_* families and the ~65 upstream vllm:* wrap families — are gated by the user's --log-stats CLI flag (default off).' and docs/usage/metrics.md:10 '...the 15 vllm:omni_* families are registered as placeholders with no data written to them' — while the same tree's tables enumerate Pipeline (4) (design:161-168) + Audio (6) (design:170-181, usage:44-49) + Transfer (4) (design:183-192, usage:59-62) = 14. Also in a diff-edited file: vllm_omni/metrics/modality.py:3 '7 audio business-semantic metric families.' versus exactly six family declarations at modality.py:38-72. Note the draft's original evidence is corrected: prometheus.py:30-39 registers defs.PROMPT_TOKENS and defs.GENERATION_TOKENS beyond the four cited constants, so 14 is derived from the docs' own tables, not from prometheus.py's imports.

Suggestion: All metrics — both the 14 vllm:omni_* families and the ~65 upstream vllm:* wrap families — are gated by the user's --log-stats CLI flag (default off).

- **[P3] This diff deletes the only server-side caller of compute_audio_rtf (the observe…** — `vllm_omni/metrics/definitions.py`
Evidence for This diff deletes the only server-side caller of compute_audio_rtf (the observe…

This diff deletes the only server-side caller of compute_audio_rtf (the observe_modality_at_finalize block calling defs.compute_audio_rtf(gen_time_s, duration_s) in modality.py), leaving the helper bench-side only — vllm_omni/benchmarks/patch/patch.py:968,1240 and vllm_omni/experimental/fullduplex/client.py:17,354 — while its sibling compute_audio_frames in the same section was already bench-only (patch.py:934,1231). The section header still claims 'Formula helpers (shared by server-side observe and bench-side calculation)', and the docstring's '(caller decides whether to observe)' no longer describes any server-side observer. Update the header so the file keeps telling the truth under the new behavior.

Evidence: In-diff removal of the sole server-side caller: vllm_omni/metrics/metrics diff deletes from observe_modality_at_finalize mod_metrics.observe_audio_rtf( stage_label, replica_label, defs.compute_audio_rtf(gen_time_s, duration_s), ). Stale header, unchanged by this diff, present in PR-time tree: vllm_omni/metrics/definitions.py:252 # Formula helpers (shared by server-side observe and bench-side calculation); docstring definitions.py:258-259 Returns 0.0 when audio_duration_s is non-positive (caller decides whether to observe; ...). Remaining callers, unchanged by this diff, present in PR-time tree: vllm_omni/benchmarks/patch/patch.py:968 output.audio_rtf = defs.compute_audio_rtf(audio_generate_time, audio_duration), patch.py:1240 output.audio_rtf = defs.compute_audio_rtf(output.latency, output.audio_duration), vllm_omni/experimental/fullduplex/client.py:17 from vllm_omni.metrics.definitions import compute_audio_rtf (call at :354). Repo-wide grep at head finds no other compute_audio_rtf references and zero matches for observe_audio_rtf / AUDIO_RTF_METRIC / RTF_BUCKETS — so no server-side observer remains, making the header false on this PR head.

Suggestion: # Formula helpers (bench-side calculation; the server no longer observes RTF)


🤖 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!

Comment thread docs/design/metrics.md
| `vllm:omni_e2e_request_latency_s` | Histogram | `model_name` | Pipeline-global end-to-end latency in seconds |

### Audio (7)
### Audio (6)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] This diff removes vllm:omni_audio_rtf entirely at head — vllm_omni/metrics/defi…

Evidence and suggested fix

This diff removes vllm:omni_audio_rtf entirely at head — vllm_omni/metrics/definitions.py:128-130 now lists only AUDIO_TTFP_S/AUDIO_DURATION_S/AUDIO_FRAMES_METRIC (no AUDIO_RTF_METRIC, RTF_BUCKETS, or observe_audio_rtf anywhere in vllm_omni/), and docs/design/metrics.md:170 reads '### Audio (6)' — yet the maintainer thread is not resolved: @Gaohan123 said 'I think here it is the general metric rather than stage-level. So we should keep it' and the inline reply 'reverted' does not match the head state, where the family is still fully gone. Observability consequence: after this change no /metrics family exposes audio-stage generation time — stage_gen_time_ms surfaces only in the stats logger (vllm_omni/metrics/stats.py:473), not in prometheus.py — so the streaming-TTS SLO (<1) is no longer computable server-side; RTF survives only bench-side via defs.AUDIO_RTF/defs.compute_audio_rtf (vllm_omni/benchmarks/metrics/metrics.py:117, vllm_omni/benchmarks/patch/patch.py:968; benchmarks/tts selects it as a percentile metric at bench_tts.py:129). Either restore RTF as a stage-global metric per the maintainer request, or record explicit maintainer sign-off on dropping the server-side family in the PR description.

Evidence: Anchor at PR-time head: docs/design/metrics.md:170 ### Audio (6); its table rows (176-181) list ttfp_s, duration_s, frames_total, underrun_s, continuity_ok_total, skipped_requests_total — no audio_rtf row. Removal complete: vllm_omni/metrics/definitions.py:128-130 AUDIO_TTFP_S = METRIC_PREFIX + AUDIO_TTFP + "_s" / AUDIO_DURATION_S = METRIC_PREFIX + AUDIO_DURATION + "_s" / AUDIO_FRAMES_METRIC = METRIC_PREFIX + AUDIO_FRAMES (AUDIO_RTF_METRIC absent); repo-wide grep finds zero hits for AUDIO_RTF_METRIC, RTF_BUCKETS, or observe_audio_rtf under vllm_omni/ at head. Observability half — unchanged by this diff, present at PR-time head: vllm_omni/metrics/stats.py:473 defs.STAGE_GEN_TIME_MS: float(evt.stage_gen_time_ms), (stats logger only), while vllm_omni/metrics/prometheus.py:7-39 registers only running/waiting/requests_success/e2e_latency/prompt+generation-token families, so no /metrics family carries audio-stage generation time and RTF<1 cannot be computed server-side. Bench-side RTF survives: vllm_omni/metrics/definitions.py:31 AUDIO_RTF = "audio_rtf", definitions.py:254 def compute_audio_rtf(audio_generation_latency_s: float, audio_duration_s: float) -> float:, consumed at vllm_omni/benchmarks/patch/patch.py:968 output.audio_rtf = defs.compute_audio_rtf(audio_generate_time, audio_duration). The quoted maintainer thread is external PR-review data from the fetch step (not in-tree); the head-state half of the claim was verified by reading the tree.

import torch
from prometheus_client import REGISTRY, generate_latest

from vllm_omni.engine.mm_outputs import MultimodalPayload

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] The new import added by this PR at tests/metrics/test_modality.py:7, `from vllm…

Evidence and suggested fix

The new import added by this PR at tests/metrics/test_modality.py:7, from vllm_omni.engine.mm_outputs import MultimodalPayload, resolves through the deprecated compat shim vllm_omni/engine/mm_outputs.py, which star-imports the canonical module and emits DeprecationWarning: Importing from 'vllm_omni.engine.mm_outputs' is deprecated. Use 'vllm_omni.outputs.mm_outputs' instead. on import (shim lines 3-9, unchanged by this diff). Repo-wide grep shows this new test import is the only remaining consumer of the old path, while the sibling test tests/engine/test_multimodal_accumulation.py:7 already imports from vllm_omni.outputs.mm_outputs import MultimodalPayload (canonical class MultimodalPayload(Mapping) at vllm_omni/outputs/mm_outputs.py:81). pyproject.toml's filterwarnings only suppresses two unrelated UserWarnings, so the warning surfaces in every collected run. Since this PR's purpose is migrating to the MultimodalPayload type, point the new import at the canonical module so the issue-2744 regression tests don't sit on the deprecated path.

Evidence: Trigger: pytest collecting the PR-added tests/metrics/test_modality.py executes line 7 from vllm_omni.engine.mm_outputs import MultimodalPayload (added by this diff), which imports the shim — unchanged by this diff, present in the PR-time tree — vllm_omni/engine/mm_outputs.py:3-9: from vllm_omni.outputs.mm_outputs import * # noqa: F401,F403 / warnings.warn("Importing from 'vllm_omni.engine.mm_outputs' is deprecated. Use 'vllm_omni.outputs.mm_outputs' instead.", DeprecationWarning, stacklevel=2). Adverse effect: a DeprecationWarning is emitted on every test run of the new file and the new code depends on a redirect-only shim; pyproject.toml:293-296 filterwarnings = ["ignore:.*does not have '__test__' attribute.*:UserWarning", "ignore:.*does not have '__bases__' attribute.*:UserWarning"] does not suppress it. Sole-consumer claim: repo-wide grep for engine.mm_outputs matches only tests/metrics/test_modality.py:7 and the warning string inside the shim itself. Canonical target exists: vllm_omni/outputs/mm_outputs.py:81 class MultimodalPayload(Mapping):; sibling precedent tests/engine/test_multimodal_accumulation.py:7 from vllm_omni.outputs.mm_outputs import MultimodalPayload.

Suggestion: from vllm_omni.outputs.mm_outputs import MultimodalPayload

@vllm-omni-review-bot

Copy link
Copy Markdown

Omni ReviewBot: finding feedback

[p1] This diff removes vllm:omni_audio_rtf entirely at head — vllm_omni/metrics/defi… — docs/design/metrics.md:170

See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working core related to core module: cache, scheduler, engine, worker, modelrunner documentation Improvements or additions to documentation ready label to trigger buildkite CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Remove per-stage Audio/Video RTF, keep only e2e RTF

4 participants