Fix two audio prompt-placeholder invariant violations (multi-modal #51, #52) - #114
Merged
Merged
Conversation
Both are merge blockers found reviewing the ASR audio cascade. Each made well-formed audio requests fail with the same opaque vLLM error, from different causes: RuntimeError: Expected there to be 1 audio prompt placeholders corresponding to 1 audio items, but instead found 0 prompt placeholders! 1. _hf_processor_applies_updates was left at the base implementation, which returns True for raw (non-embedding) items. That tells vLLM the processor already expanded <|audio|> itself, so vLLM skips applying our PromptReplacement and merely searches the prompt for the transcript ids - which are not there, because _call_hf_processor deliberately leaves the marker in place and returns the transcript out of band. Override it to False. Only the uncached path consults the hook (the cached path hardcodes False), so this was silently contingent on mm_processor_cache_gb, on the entry point, and on the vLLM version. Configuration-dependent correctness is worse than a hard failure. The default path is untouched. 2. A clip with no recognizable speech transcribes to "", which produced a zero-length replacement. vLLM discards zero-length placeholder content and then reports the item as missing, so silence, music, or a 200 ms clip returned an opaque 500. It also left engine startup dependent on what the ASR model does with the silent profiling clip. Substitute a single space when the transcript is empty or whitespace-only, so every audio item occupies at least one prompt position and the model sees an audio turn that said nothing. Done in _transcribe rather than the replacement callback so audio_num_tokens stays consistent with the flat token buffer. Raise a clear ValueError if even the fallback tokenizes to nothing, rather than letting vLLM's opaque error resurface. This also makes startup deterministic, so the silent profiling clip is deliberately kept - it is now the better input, since noise or a tone would invite a variable-length hallucinated transcript. Tests drive vLLM's real _apply_hf_processor_text_mm and _maybe_apply_prompt_updates with real MultiModalDataItems rather than mocking the machinery under test, and need no GPU or ASR model. Reverting either fix makes the corresponding tests fail with the exact production error. Four pre-existing tests asserted the old zero-length behavior and were updated, since they encoded the bug. docs/AUDIO.md documents the empty-transcript policy, as it is observable. Signed-off-by: aviv ron <rona@il.ibm.com>
aviv1ron1
requested review from
antonpibm,
freunda and
yairallouche
as code owners
August 3, 2026 15:46
The `audio` extra (soundfile, librosa) was declared but referenced by nothing: not `dev`, not `dev-vllm20`, not `test`, not the `tutorials` extra, and not either CI workflow. The only mention of `uv sync --extra audio` anywhere was inside an ImportError string in asr.py. So no documented install path produced a working audio checkpoint. A synced pod running the integration tests failed with: ModuleNotFoundError: No module named 'soundfile' in test_audio_serving_smoke.py and test_answerability_over_audio.py. This affected `dev` (vLLM 0.19) exactly as much as `dev-vllm20` — neither carried the extra. Changes: - `dev` and `dev-vllm20` now request granite-switch[hf,compose,audio]. `test` picks it up transitively via include-group. - gpu-tests.yaml states `--extra audio` explicitly. Redundant with the dev group today, but it documents the dependency where it is used so a later group refactor cannot silently drop it again. - docs/AUDIO.md gains an Installing section. `audio` is deliberately not folded into the `vllm` extra, since text-only vLLM users should not carry libsndfile and numba; that makes `--extra vllm --extra audio` the supported serving command, which needs saying out loud. ci.yaml is left alone on purpose: the CPU tier installs no vLLM, so the audio tests skip there, and tests/unit/test_asr.py deliberately exercises the librosa-absent path. Verified per group with `uv export`: dev, dev-vllm20 and test all now resolve librosa==0.11.0 and soundfile==0.14.0, and both documented serving combos (--extra vllm/vllm20 --extra audio) resolve without tripping the declared conflicts. The uv.lock delta is only the three requires-dev entries plus an audioread marker that simplifies as a consequence. Signed-off-by: aviv ron <rona@il.ibm.com>
The audio tests are spread across five directories (unit, vllm, composer, integration), and two of the files mix audio tests in with unrelated ones, so selecting the audio tier meant listing paths plus a `-k` filter. Register an `audio` marker and apply it so `pytest -m audio` selects all 131 of them: - module-level `pytestmark` on the four dedicated files (test_asr, test_chunking, test_audio_processor, and the three integration files) - class-level `@pytest.mark.audio` on TestAudioConfig in test_config.py and on the two audio classes in test_chat_template.py, so the non-audio tests in those files stay unselected test_audio_processor's existing single `pytestmark` skipif becomes a list to hold both marks. Documented in docs/AUDIO.md: `pytest -m audio` for everything, or `pytest -m "audio and not gpu"` for the 118-test CPU tier that runs in seconds. No test bodies changed; the full suite still collects 1490 tests. Signed-off-by: aviv ron <rona@il.ibm.com>
Composing with --enable-audio produced a checkpoint whose switch lookup table was one row shorter than the config.json shipped beside it. Loading it through the HF backend failed with an out-of-bounds embedding gather, surfacing as the CUDA device-side assert `indexSelectSmallIndex: srcIndex < srcSelectDimSize`. Root cause: SingleSwitch sizes the table max(config.vocab_size, max_ctrl_id + 1) at construction. Compose builds the model from a config whose vocab_size is copied verbatim from the base checkpoint, then grows the vocabulary in step 3 via resize_token_embeddings, which updates config.vocab_size and both embedding matrices but leaves the buffer untouched. Without audio the construction-time and load-time computations agree by coincidence, because compose ends with vocab_size == max_ctrl_id + 1. The <|audio|> marker is the first token to make vocab_size larger than that and break the tie. The resulting mismatch is not recoverable at load time. from_pretrained discards the stored tensor and there is no _init_weights rule for the buffer, so it is left as uninitialised memory. Every id then reads as a control id and the token-exchange rewrite sends out-of-range ids into the embedding gather. Three changes: - Rebuild the table at the end of compose step 3, after the resize, so the saved buffer and the shipped config agree. The sizing rule is factored out into build_control_to_substitute_lut() so __init__ and the new SingleSwitch.rebuild_control_to_substitute_lut() cannot diverge. - Assert the invariant in validator.validate_control_lut(), called after the rebuild. An internally inconsistent checkpoint now fails during compose instead of faulting on a GPU much later, and the error names both lengths. - Stop add_audio_token evicting the adapter control tokens. add_special_tokens replaces additional_special_tokens rather than extending it, and transformers exposes no accessor for the current list, so the marker call now re-passes the control tokens explicitly via keep_special_tokens. Previously, enabling audio silently dropped all of them from all_special_tokens and from the saved tokenizer_config.json. Token ids are unchanged: the marker still takes the next free id. MockTokenizer in the composer tests had append semantics for add_special_tokens and so could never have caught that eviction; it now mirrors the replace behaviour of the real tokenizer. The vLLM switch keeps its own copy of the sizing expression and is deliberately untouched: it never resizes embeddings and never writes checkpoints, and importing from the HF backend would couple two independently installable extras. Only newly composed checkpoints are fixed. Existing audio checkpoints still ship a stale table and need a re-compose; they now fail clearly at from_pretrained rather than faulting on the GPU. Tests: 158 unit, 565 hf, 154 composer. The save/load round-trip test was verified to fail without the rebuild, reproducing the original mismatch report. Signed-off-by: aviv ron <rona@il.ibm.com>
…t was failing and was a problematic test. we should not test inference as it is unstable and not deterministic Signed-off-by: aviv ron <rona@il.ibm.com>
Signed-off-by: aviv ron <rona@il.ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes three defects in the ASR audio cascade, plus the packaging gap that blocked testing them.
1.
_hf_processor_applies_updatescontract violation (#51)Left at its base value, the hook returns
Truefor raw items — telling vLLM the HF processor already expanded<|audio|>. vLLM then skips ourPromptReplacementand searches for transcript ids that aren't there, since_call_hf_processordeliberately leaves the marker in place and returns the transcript out of band. Result:RuntimeError: Expected there to be 1 audio prompt placeholders ... but instead found 0.Fix: override to return
False— we never expand the marker ourselves.Only the uncached path consults the hook (the cached path hardcodes
False), so with the default cache on the bug is invisible. It needs cache-off and a string prompt, which startup profiling passes — so the sharpest symptom is the engine failing to boot at--mm-processor-cache-gb 0. The default path is byte-identical after this change.2. Empty/silent transcript breaks the placeholder invariant (#52)
Silence, music, or a 200 ms clip transcribes to
""→ a zero-length replacement, which vLLM discards before reporting the item missing, so a well-formed request became an opaque 500. Fires on both paths at any cache setting.PlaceholderRange(offset, length)cannot expresslength=0, and the item cannot be dropped either — its count comes from the parsed request.Fix:
_EMPTY_TRANSCRIPT_TEXT = " ", applied in_transcriberather thanreplacement()so it also reachesaudio_num_tokens; raises clearly if even the fallback tokenizes to nothing. Only a cascade hits this — a real encoder's placeholder length tracks duration and is never zero.3.
control_to_substitute_lutlength driftFound triaging a CUDA
indexSelectSmallIndex: srcIndex < srcSelectDimSizeassert.SingleSwitchsizes the table fromconfig.vocab_size, but compose builds the model from the base config's value and grows the vocabulary only afterwards — so--enable-audioshipped a checkpoint whose table was one row short of its ownconfig.json. Unrecoverable at load:from_pretraineddiscards the mismatched tensor and leaves uninitialised memory, so every id reads as a control id and the rewrite gathers out of bounds. Latent before this branch, where compose ends withvocab_size == max_ctrl_id + 1and the two computations agree by coincidence.Fix:
build_control_to_substitute_lut()so__init__and the rebuild cannot diverge.validator.validate_control_lut()assertslen(lut) == config.vocab_sizeat compose time.add_audio_tokenno longer evicts the control tokens:add_special_tokensreplacesadditional_special_tokensinstead of extending it, so enabling audio silently dropped them from the savedtokenizer_config.json. Token ids unchanged.4. Missing
audioextraaudio(soundfile,librosa) was declared and referenced by nothing — notdev,dev-vllm20,test, or either workflow; integration tests died onModuleNotFoundError: No module named 'soundfile'. Now requested bydev/dev-vllm20(testinherits) and stated explicitly ingpu-tests.yaml;docs/AUDIO.mdgains an Installing section.ci.yamluntouched — the CPU tier installs no vLLM.Tests
tests/vllm/test_audio_processor.py— 38 (was 33), driving vLLM's real_apply_hf_processor_text_mm/_maybe_apply_prompt_updates; CPU-only.test_token_exchange.py,test_validator.py,test_tokenizer_setup.py— LUT sizing, rebuild-after-resize, strict save/load round trip, compose-time assertion, special-token preservation.tests/integration/test_audio_uncached_processor.py(new) — the only test exercising the uncached path §1 fixes. Boots with the cache disabled (version-detectingmm_processor_cache_gbvs the olderdisable_mm_preprocessor_cache) and asserts the marker is gone fromprompt_token_ids. The cache check gates the fixture, so the module skips rather than silently passing on the cached path.audiomarker added:pytest -m audioselects 132 tests. Removedtest_answerability_over_audio.py— it asserted a non-deterministic model verdict.§1, §2 and the §3 rebuild were each confirmed as genuine guards by reverting them in turn.
Verification
CPU: 158 unit, 565 hf, 154 composer, all pre-commit hooks, DCO.
GPU,
pytest -m audioon both vLLM 0.19 and 0.20 — 132 passed. This includes the new uncached-path module, so §1 is verified end-to-end on both versions: the engine boots with the processor cache disabled and the marker is replaced.GPU, full suite on
1362bac:1490 passed, 1 failed, 0 errors— was1440 / 1 / 31. ZeroMISMATCHor device-side asserts, and the CUDA-assert test now passes. The one failure was the answerability test since removed.Not verified: audio under non-eager (every audio test sets
enforce_eager=True) and under TP>1.