Skip to content

[Perf][Model] Enable CoVo audio encoder CUDA graphs - #8572

Open
jeffaa729 wants to merge 11 commits into
vllm-project:mainfrom
jeffaa729:perf/covo-audio-encoder-cudagraph
Open

jeffaa729 wants to merge 11 commits into
vllm-project:mainfrom
jeffaa729:perf/covo-audio-encoder-cudagraph

Conversation

@jeffaa729

@jeffaa729 jeffaa729 commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

Part of #7452: add upstream encoder CUDA graph support for the CoVo-Audio Stage 0 audio encoder.

Changes

  • Implement SupportsEncoderCudaGraph on the CoVo Stage 0 model, including capture, replay, eager fallback, audio-item selection, and trimming embeddings to each audio’s valid token count.
  • Preserve encoder batch grouping: with the default two-item capture limit, a group larger than two audio items runs eagerly in one call, without splitting.
  • Add regression tests for eager output lengths, disabled/non-CUDA behavior, graph-versus-eager output, the 188- and 376-token capture budgets, and whole-group eager fallback for a three-item input. The CUDA test carries the L4 hardware marks and exercises GPUModelRunner._execute_mm_encoder.

To enable the feature in Stage 0:

enforce_eager: false
compilation_config:
  cudagraph_mm_encoder: true
  encoder_cudagraph_token_budgets: [188, 376]
  encoder_cudagraph_max_vision_items_per_batch: 2

Benchmark

Measured on one A100 80 GB with tencent/Covo-Audio-Chat, vLLM 0.31.0, and vLLM-Omni commit 1a1772ee2. The on/off serving configurations differed only in cudagraph_mm_encoder.

Encoder input Eager median (range), ms Graph median (range), ms Change
One audio, 35 valid tokens 23.91 (23.78–25.62) 22.89 (22.88–22.90) −4.3%
Two audios, 35 + 161 valid tokens 40.01 (39.98–40.06) 39.10 (39.09–39.11) −2.3%

The manager captured two graphs. The serving log recorded 100 graph hits and zero misses; the graph-off log showed no manager initialization.

Client concurrency Pipeline latency off → on, median Throughput off → on, median
1 4.192 → 4.170 s 0.2358 → 0.2367 req/s
8 24.769 → 24.520 s 0.1991 → 0.2006 req/s
32 85.618 → 86.304 s 0.1963 → 0.1953 req/s

Memory

Measured on one NVIDIA A100-SXM4 80 GB.

In an isolated encoder measurement, capturing the two encoder graphs took 0.419 seconds:

Metric Before encoder capture After encoder capture Increase
PyTorch allocated GPU memory 1,346.4 MB 1,369.8 MB 23.4 MB
PyTorch reserved GPU memory 1,476.4 MB 1,671.4 MB 195.0 MB
Host RSS — — 410.6 MB

Updated full-serving Stage 0 validation on A100-SXM4 80 GB with vLLM 0.31.0 recorded:

Metric Encoder graphs off Encoder graphs on
Available KV cache memory 22.65 GiB 22.49 GiB
KV cache capacity 424,016 tokens 421,088 tokens

Test plan

pytest -s -v tests/model_executor/models/covo_audio/test_encoder_cudagraph.py

pre-commit run --files \
  vllm_omni/model_executor/models/covo_audio/covo_audio.py \
  vllm_omni/model_executor/models/covo_audio/covo_audio_llm.py \
  tests/model_executor/models/covo_audio/test_encoder_cudagraph.py

The focused regression suite passed on A100-SXM4 80 GB with vLLM 0.31.0:

 4 passed, 15 warnings in 0.57s

Ruff, formatting, and git diff --check passed for the pending changes. The full pre-commit command has not yet been verified after the final cleanup.

AI assistance

I used Codex to inspect the pinned vLLM interface, develop and review the integration and regression test, analyze benchmark logs, and draft this description. I reviewed the changes and benchmark results.

Signed-off-by: jeffaa729 <hoiwanglo@gmail.com>
Signed-off-by: jeffaa729 <hoiwanglo@gmail.com>
@jeffaa729
jeffaa729 marked this pull request as ready for review October 7, 2026 16:29
@vllm-omni-review-bot

Copy link
Copy Markdown

This PR appears to belong to: docs/design/module/model_integration.md, docs/design/module/ar_runtime.md.

Module owners: @tzhouam @gcanlin @fake0fan

Routing: @tzhouam via module of the changed files, semantic router, CODEOWNERS; @gcanlin via module of the changed files, semantic router; @fake0fan via module of the changed files

@jeffaa729, 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.

@vllm-omni-review-bot

Copy link
Copy Markdown
Omni ReviewBot routing record

Assigned Strict on cursor (cursor-grok-4.6-high) under experiment fleet-strict-cursor-grok46-zcode-glm53flash-5050-c5-z10-20261002.

@vllm-omni-review-bot

vllm-omni-review-bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Omni ReviewBot triage note

Automated triage of commit c7c08a7484d9 produced:

  • Priority: high. Prompt maintainer attention is suggested.

These are automated triage suggestions only — the final decision belongs to the maintainers.

@jeffaa729
jeffaa729 marked this pull request as draft October 7, 2026 18:41
@jeffaa729

Copy link
Copy Markdown
Contributor Author

Author self-review:

I reviewed the CoVo Stage 0 encoder CUDA Graph integration, including protocol wiring, capture and replay inputs, eager fallback, per-item selection, valid-token trimming, and the 188/376-token budget behavior. I also checked the disabled/non-CUDA path and the three-item split across the two-item capture limit.

The graph-versus-eager embedding comparison, greedy-token comparison, graph-hit evidence, and A/B benchmark results are documented in the PR. The focused regression suite, applicable pre-commit hooks, and git diff --check pass at this head.

@jeffaa729
jeffaa729 marked this pull request as ready for review October 7, 2026 23:59
@hsliuustc0106 hsliuustc0106 added the enhancement New feature or request label Oct 8, 2026
@vllm-omni-review-bot

Copy link
Copy Markdown
Omni ReviewBot attempt record

Review attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)).

@vllm-omni-review-bot

Copy link
Copy Markdown
Omni ReviewBot attempt record

Review attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 600s (try 3 of 3)).

@vllm-omni-review-bot

Copy link
Copy Markdown
Omni ReviewBot attempt record

Review attempt ended as failed (failed; falling back to direct/cursor/auto).

EncoderItemSpec,
)

_MAX_AUDIO_TOKENS = 188

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

covo_audio.py already has MAX_AUDIO_TOKENS = 188. Can we import that one?

feats = self.encoder(inputs["audio_features"]).last_hidden_state
return self.audio_adapter(feats)

def encoder_eager_forward(self, mm_kwargs: dict[str, Any], path: str = "default") -> torch.Tensor:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

embed_multimodal does the same dtype cast + encoder + adapter steps. Could it call encoder_eager_forward? Then the eager and graph paths can't go out of sync.

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.

@LinzeShi Hi , thanks for the review ! I have pushed the updated version which reuse the constant and the encoder_eager_forward to avoid duplicated codes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: jeffaa729 <hoiwanglo@gmail.com>

@MrlixiangWE MrlixiangWE left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Checked 8ac6b09 merged into main (fd6c69d) on A800 80GB (vLLM 0.31.0) with tencent/Covo-Audio-Chat: stage 0 with enforce_eager: false, cudagraph_mm_encoder on and off, greedy, seven distinct audio inputs over four requests, each arm run twice.

Request Audio embeddings vs eager Greedy output vs eager
1 audio (3.5 s, then 25.7 s) bit-identical identical tokens and audio
2 audios bit-identical identical tokens and audio
3 audios (graph path splits 2 + 1) relative L2 2.2–4.4% same text; audio codes differ from token 7 of 186

Eager with the same three audios sent as 2 + 1 matches the graph output bit for bit, so the three-audio difference comes from the split. With the graphs on, stage-0 KV is 19.61 GiB instead of 19.77 GiB, and stage-0 RSS is 7.2 GiB in both arms.

assert model.embed_multimodal(**_inputs([35], "cpu", seed=0))[0].shape == (35, 6)


@pytest.mark.cuda

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On current main this mark fails the check-mark hook (#8708 rejects a bare pytest.mark.cuda), and the CUDA lanes now select core_model and cuda and L4 or and H100 (#8888), so neither collects this test: the L4 lane on main + this PR gives 1515 passed, the same as main. @hardware_test(res={"cuda": "L4"}) from tests/helpers/mark.py would put it in the L4 lane.

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.

Updated the CUDA test to use @hardware_test(res={"cuda": "L4"}) from tests/helpers/mark.py, replacing the bare @pytest.mark.cuda.

for lengths, seed in [([35], 0), ([35, 161], 1), ([35, 161, 50], 2)]:
inputs = _inputs(lengths, "cuda", seed)
expected = model.embed_multimodal(**inputs)
actual = manager.execute(inputs)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This calls the manager directly and skips the runner's supports_modality gate: with modalities=["audios"] in get_encoder_cudagraph_config this test still passes, while _execute_mm_encoder would run every audio batch eagerly. Driving a batch through GPUModelRunner._execute_mm_encoder and checking graph_hits, as the Qwen3-Omni test does (test_vision_encoder_cudagraph.py:355), would catch it. test_stage0_stays_eager_without_cuda_or_opt_in also passes with covo_audio/ reverted to main.

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.

Updated the regression to follow pr #8215 Qwen3-Omni’s _run_encoder_step() pattern and enter GPUModelRunner._execute_mm_encoder(). It now exercises the supports_modality gate and checks graph hits, output parity and a single three-item eager fallback.

I also temporarily injected modalities=["audios"] through a test-only plugin. The test failed as expected: it observed eager batches [1, 2, 3] instead of [3], confirming that it catches the routing mistake.

The CPU eager/off test remains a fallback check; the enabled runner test now provides the CoVo-specific integration coverage.

Validation on the A100:

pytest -s -v tests/model_executor/models/covo_audio/test_encoder_cudagraph.py

All tests passed:

4 passed, 15 warnings in 0.57s

@jeffaa729

Copy link
Copy Markdown
Contributor Author

Checked 8ac6b09 merged into main (fd6c69d) on A800 80GB (vLLM 0.31.0) with tencent/Covo-Audio-Chat: stage 0 with enforce_eager: false, cudagraph_mm_encoder on and off, greedy, seven distinct audio inputs over four requests, each arm run twice.

Request Audio embeddings vs eager Greedy output vs eager
1 audio (3.5 s, then 25.7 s) bit-identical identical tokens and audio
2 audios bit-identical identical tokens and audio
3 audios (graph path splits 2 + 1) relative L2 2.2–4.4% same text; audio codes differ from token 7 of 186
Eager with the same three audios sent as 2 + 1 matches the graph output bit for bit, so the three-audio difference comes from the split. With the graphs on, stage-0 KV is 19.61 GiB instead of 19.77 GiB, and stage-0 RSS is 7.2 GiB in both arms.

Thanks for the review. CoVo now use SingleReplayEncoderCudaGraphManager as #8215 using encoder_cudagraph_single_replay = True. With the current default batches of 2 capture limit, larger groups will execute together in one eager forward.

I repeated the comparison on an A100-SXM4 80 GB with vLLM 0.31.0. Both arms used enforce_eager: false.

Seven distinct audio inputs were sent over four requests, each repeated twice per arm:

Request Enabled path Embeddings vs eager Tokens, audio codes and waveform samples vs eager
One audio: 3.5 s / 25.65 s Graph Bit-identical Bit-identical
Two audios Graph Bit-identical Bit-identical
Three audios Whole-group eager Bit-identical Bit-identical

The updated path recorded 8 item hits and 6 item misses, representing the two three-audio eager groups.

The encoder timing remained slightly faster:

  • 23.719 → 22.863 ms for one audio (−3.61%)
  • 40.243 → 39.375 ms for two (−2.16%)

@MrlixiangWE MrlixiangWE left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rechecked at 3352cae. The CUDA test now carries the L4 marks, so the L4 ready lane collects it, and it goes through GPUModelRunner._execute_mm_encoder. A CPU copy of that test with the graph capture stubbed passes on this head and on main + this PR with vLLM 0.32.0.

The description still describes the earlier head: the Changes list says the tests cover splitting a three-item input, the memory table shows 21.78 GiB of KV cache with the graphs both on and off, which predates #7632 (19.61 GiB with the graphs on vs 19.77 GiB off in the run from my earlier review), and the Test Plan says the new regression test has not been run yet. Please update these, and mention that a group larger than the two-item capture limit now runs eagerly in one call.

@jeffaa729

Copy link
Copy Markdown
Contributor Author

Rechecked at 3352cae. The CUDA test now carries the L4 marks, so the L4 ready lane collects it, and it goes through GPUModelRunner._execute_mm_encoder. A CPU copy of that test with the graph capture stubbed passes on this head and on main + this PR with vLLM 0.32.0.

The description still describes the earlier head: the Changes list says the tests cover splitting a three-item input, the memory table shows 21.78 GiB of KV cache with the graphs both on and off, which predates #7632 (19.61 GiB with the graphs on vs 19.77 GiB off in the run from my earlier review), and the Test Plan says the new regression test has not been run yet. Please update these, and mention that a group larger than the two-item capture limit now runs eagerly in one call.

Updated the followings in the description :

  1. in the Changes section : replaced the three-item splitting claim with whole-group eager fallback above the configured two-item limit
  2. refreshed the KV-cache figures from our A100 run (22.65 GiB off / 22.49 GiB on)
  3. recorded the completed regression run: 4 passed, 15 warnings in 0.57s.

@vllm-omni-review-bot

Copy link
Copy Markdown

Omni ReviewBot: no human activity for 7 days

@jeffaa729 this pull request has had no human commit, comment or review since 2026-10-21. Please confirm the current plan and next step. 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.

@amy-why-3459 amy-why-3459 left a comment

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.

Read through the CoVo Stage 0 encoder CUDA graph changes at 3352cae. The wiring looks consistent with the upstream manager: batch_mm_kwargs passed to postprocess_encoder_output is the already-selected sub-batch, so indexing audio_num_tokens by batch_idx is correct, and the float32 → encoder-dtype conversion on replay is handled by the manager's copy_ into the captured buffer. The single-replay fallback test for the 3-item group is a nice touch.

A few small code-level notes inline (none blocking):

  • the capture shape hard-codes (128, 3000) separately from the processor's feature_size=128 / 30 s window;
  • select_encoder_cudagraph_items indexes every value in mm_kwargs, which assumes all of them are batch-major tensors;
  • postprocess_encoder_output silently clamps a length above MAX_AUDIO_TOKENS.

One observation on the benchmark: the end-to-end numbers are within noise (and slightly worse at concurrency 32) while reserved memory grows ~195 MB, so keeping cudagraph_mm_encoder opt-in as it is now seems right.

raise ValueError("CoVo encoder CUDA graph budget must fit at least one audio item")
encoder_dtype = next(self.encoder.parameters()).dtype
return EncoderCudaGraphCaptureInputs(
{"audio_features": torch.zeros((batch_size, 128, 3000), device=device, dtype=encoder_dtype)}

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.

nit: 128 and 3000 duplicate the processor's feature_size=128 and the 30 s Whisper window in covo_audio.py. If those ever change (or a checkpoint uses a different mel count), capture would record a graph whose input buffer no longer matches the replay tensor and _copy_padded_buffer would fail at runtime. Could these come from shared constants (or from the encoder config's num_mel_bins) so the two can't drift?

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.

Updated the processor and graph capture to use shared audio constants. The capture frame count is derived from the window duration, sample rate, and hop length, removing the duplicated (128, 3000) dimensions.

]

def select_encoder_cudagraph_items(self, mm_kwargs: dict[str, Any], indices: list[int]) -> dict[str, Any]:
return {key: value[indices] for key, value in mm_kwargs.items()}

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 assumes every value in mm_kwargs is a tensor indexed by item on dim 0. Today the CoVo fields are audio_features and audio_num_tokens, so it works, but if a non-tensor or a non-batched field is ever added (or audio_num_tokens arrives as a Python list), value[indices] will raise. Consider selecting only the known keys or guarding with isinstance(value, torch.Tensor).

audio_num_tokens = batch_mm_kwargs.get("audio_num_tokens")
features = outputs["default"]
for batch_idx, original_idx in enumerate(indices):
n = int(audio_num_tokens[batch_idx]) if audio_num_tokens is not None else MAX_AUDIO_TOKENS

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.

If audio_num_tokens ever exceeds MAX_AUDIO_TOKENS, the slice below silently returns 188 rows while the placeholder count expects more, which would surface later as a hard-to-trace embedding/placeholder mismatch. A cheap assert n <= MAX_AUDIO_TOKENS (or min, matching whatever the eager embed_multimodal path does) would make that failure explicit.

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.

Added an assertion that the requested length is at most MAX_AUDIO_TOKENS before slicing, with the invalid count included in the error message.

Signed-off-by: jeffaa729 <hoiwanglo@gmail.com>

@amy-why-3459 amy-why-3459 left a comment

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.

Thanks for the follow-up. I re-checked only the new commit 88d2ac00.

The shared constants look good: hop_length=160 / chunk_length=30 match WhisperFeatureExtractor's defaults, so processor output is unchanged and the capture shape still resolves to (B, 128, 3000). Restricting select_encoder_cudagraph_items to audio_features / audio_num_tokens is safe too, since those are the only keys the replay-buffer, eager-forward and postprocess paths read.

One small follow-up on the new length check is inline.

n = int(audio_num_tokens[batch_idx]) if audio_num_tokens is not None else MAX_AUDIO_TOKENS
assert n <= MAX_AUDIO_TOKENS, f"CoVo audio token count {n} exceeds {MAX_AUDIO_TOKENS}"
item = features[batch_idx, :n, :]
dest[original_idx] = item.clone() if clone else item

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.

Two small points on this check:

  1. assert is stripped under python -O, so in an optimized deployment this goes back to silent truncation. Raising a ValueError would keep the guard in all modes.
  2. The eager path in embed_multimodal (features[i, :n, :] around L139-140) still truncates silently when n > MAX_AUDIO_TOKENS. Applying the same check there would keep the graph and eager paths failing the same way instead of only the graph path raising.

@jeffaa729 jeffaa729 Oct 30, 2026 •

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.

Updated both paths to raise ValueError when n > MAX_AUDIO_TOKENS.

Signed-off-by: jeffaa729 <hoiwanglo@gmail.com>

@amy-why-3459 amy-why-3459 left a comment

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.

Incremental review of b8ca825. Replacing the assert with a ValueError and checking the eager path too is an improvement (asserts disappear under -O). Two small follow-ups below.

if audio_num_tokens is not None:
n = int(audio_num_tokens[i])
if n > MAX_AUDIO_TOKENS:
raise ValueError(f"CoVo audio token count {n} exceeds {MAX_AUDIO_TOKENS}")

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.

Only the upper bound is checked. A negative audio_num_tokens value would make features[i, :n, :] silently drop tokens from the end instead of failing. Consider if not 0 <= n <= MAX_AUDIO_TOKENS: here and in postprocess_encoder_output. Also, raising inside embed_multimodal fails the whole encoder batch in the model runner rather than just the offending request; if the processor can compute the token count, validating there would reject a bad request earlier. Could you add a small test for the over-limit (and negative) case on both paths?

@jeffaa729 jeffaa729 Oct 30, 2026 •

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.

  1. Added the 0 <= n <= MAX_AUDIO_TOKENS check in the processor, before feature extraction and encoder batching.

  2. The new CPU test covers negative and over-limit counts. Each case explicitly asserts ValueError from both embed_multimodal and postprocess_encoder_output.

Signed-off-by: jeffaa729 <hoiwanglo@gmail.com>

@amy-why-3459 amy-why-3459 left a comment

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.

Reviewed the new commit 4b44b0d only. The 0 <= n <= MAX_AUDIO_TOKENS check is now applied consistently on the eager and graph paths, and the new CPU test covers both negative and over-limit counts there. One note on the processor-side check below.

if not 0 <= num_tokens <= MAX_AUDIO_TOKENS:
raise ValueError(f"CoVo audio token count {num_tokens} must be between 0 and {MAX_AUDIO_TOKENS}")
num_tokens_list.append(num_tokens)

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.

With this check, any input slightly longer than 30 s is now rejected at request time (e.g. 30.1 s -> 3010 cs -> 189 tokens > 188), while WhisperFeatureExtractor itself would just truncate the features to the 30 s window. Rejecting is probably safer than the previous placeholder/feature mismatch, but two small things:

  1. The error is phrased in tokens, which a client can't map back to their input. Including the duration and the limit (e.g. CoVo audio is {len(audio_array) / AUDIO_SAMPLE_RATE:.2f}s; at most {AUDIO_CHUNK_LENGTH}s is supported) would make the rejection actionable.
  2. The new test exercises embed_multimodal / postprocess_encoder_output but not this processor path. A small CPU test that feeds a just-over-30 s array through _call_hf_processor (and one at exactly 30 s, which should still give 188) would pin the boundary.

Also, since len(audio_array) can't be negative, the lower bound here is unreachable; only the upper bound matters in the processor.

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.

Updated the processor to check only the upper bound and report the input duration and supported 30-second window.

Added a CPU test through _call_hf_processor: exactly 30 seconds produces 188 tokens, while 30.1 seconds raises the duration-specific ValueError before feature extraction.

…r log

Signed-off-by: jeffaa729 <hoiwanglo@gmail.com>

@amy-why-3459 amy-why-3459 left a comment

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.

Incremental review of 44006b8 only.

The clearer duration-based error message and the boundary test (exactly 30s accepted with 188 tokens, 30.10s rejected before the feature extractor runs) look good. Dropping the 0 <= lower bound is fine since the token count cannot go negative.

Two small points:

  1. The token count is computed with a hard-coded 16000 (_calc_audio_num_tokens(len(audio_array), 16000)), while the new message divides by AUDIO_SAMPLE_RATE. They should be the same value today, but using AUDIO_SAMPLE_RATE in both places would keep the check and the message from drifting apart.
  2. The boundary test covers exactly 30.0s and +0.1s. Is there a window between 30.0s and the first length that pushes num_tokens past MAX_AUDIO_TOKENS (for example 30s plus a few samples) where the input is accepted but the Whisper-style feature extractor silently truncates the tail to 3000 frames? If so, a case like extra_samples=1 would document whether that is rejected or truncated, and the message "at most 30s is supported" would match the behavior exactly.

Signed-off-by: jeffaa729 <hoiwanglo@gmail.com>
@jeffaa729

Copy link
Copy Markdown
Contributor Author

Incremental review of 44006b8 only.

The clearer duration-based error message and the boundary test (exactly 30s accepted with 188 tokens, 30.10s rejected before the feature extractor runs) look good. Dropping the 0 <= lower bound is fine since the token count cannot go negative.

Two small points:

  1. The token count is computed with a hard-coded 16000 (_calc_audio_num_tokens(len(audio_array), 16000)), while the new message divides by AUDIO_SAMPLE_RATE. They should be the same value today, but using AUDIO_SAMPLE_RATE in both places would keep the check and the message from drifting apart.
  2. The boundary test covers exactly 30.0s and +0.1s. Is there a window between 30.0s and the first length that pushes num_tokens past MAX_AUDIO_TOKENS (for example 30s plus a few samples) where the input is accepted but the Whisper-style feature extractor silently truncates the tail to 3000 frames? If so, a case like extra_samples=1 would document whether that is rejected or truncated, and the message "at most 30s is supported" would match the behavior exactly.

Updated the processor to use AUDIO_SAMPLE_RATE consistently and check the exact sample limit before calculating tokens. And the error includes the sample count so the one-sample excess remains clear despite duration rounding.

@amy-why-3459 amy-why-3459 left a comment

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.

Incremental review of 5cdf6c9 (sample-exact duration check, AUDIO_SAMPLE_RATE).

Checking len(audio_array) > AUDIO_CHUNK_LENGTH * AUDIO_SAMPLE_RATE directly is clearer than going through the token count, and the 1-extra-sample boundary test is exactly the right edge. Including the sample count in the error message also resolves the confusing case where a 30s + 1 sample input would otherwise print as "30.00s; at most 30s".

Two small points:

  • MAX_AUDIO_TOKENS no longer appears in _call_hf_processor; if it is now unused in covo_audio.py, the import/definition can go so lint stays clean. If it is still used elsewhere (e.g. the encoder CUDA graph capture sizes), it would be worth an assertion or comment that _calc_audio_num_tokens(AUDIO_CHUNK_LENGTH * AUDIO_SAMPLE_RATE, AUDIO_SAMPLE_RATE) <= MAX_AUDIO_TOKENS, so the two limits cannot drift apart silently.
  • The check runs on the array after MultiModalDataParser resampling, as the comment says; for inputs that bypass the parser (e.g. a raw array passed with a different sample rate in the tuple), the length would be in the wrong units. If that path is possible, a short guard or comment would help.

No other concerns with this commit.

Signed-off-by: jeffaa729 <hoiwanglo@gmail.com>
@jeffaa729

Copy link
Copy Markdown
Contributor Author

Incremental review of 5cdf6c9 (sample-exact duration check, AUDIO_SAMPLE_RATE).

Checking len(audio_array) > AUDIO_CHUNK_LENGTH * AUDIO_SAMPLE_RATE directly is clearer than going through the token count, and the 1-extra-sample boundary test is exactly the right edge. Including the sample count in the error message also resolves the confusing case where a 30s + 1 sample input would otherwise print as "30.00s; at most 30s".

Two small points:

  • MAX_AUDIO_TOKENS no longer appears in _call_hf_processor; if it is now unused in covo_audio.py, the import/definition can go so lint stays clean. If it is still used elsewhere (e.g. the encoder CUDA graph capture sizes), it would be worth an assertion or comment that _calc_audio_num_tokens(AUDIO_CHUNK_LENGTH * AUDIO_SAMPLE_RATE, AUDIO_SAMPLE_RATE) <= MAX_AUDIO_TOKENS, so the two limits cannot drift apart silently.
  • The check runs on the array after MultiModalDataParser resampling, as the comment says; for inputs that bypass the parser (e.g. a raw array passed with a different sample rate in the tuple), the length would be in the wrong units. If that path is possible, a short guard or comment would help.

No other concerns with this commit.

Updated the existing 30-second processor test to assert that its calculated token count equals MAX_AUDIO_TOKENS, tying the two limits together. And added a guard rejecting direct tuple inputs whose sample rate differs from AUDIO_SAMPLE_RATE. Raw arrays without rate metadata are documented as already resampled to that rate.

@amy-why-3459 amy-why-3459 left a comment

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.

Incremental review of c7c08a7 only (sample-rate guard + MAX_AUDIO_TOKENS in the boundary test).

The _sr != AUDIO_SAMPLE_RATE check on the tuple path closes the bypass case from the previous note, and the comment on the raw-array branch makes the assumed units explicit. Switching the boundary assertion to MAX_AUDIO_TOKENS keeps the test tied to the shared constant rather than a hard-coded 188.

No further concerns with this commit.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants