Repository navigation
[Bugfix] Fix audio.format parameter silently ignored in chat completions - #4718
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
…ons (vllm-project#4716) The audio.format field in chat completion requests was ignored — the server always returned WAV regardless of the requested format. Wire up the request's audio.format to the existing encoding infrastructure and add a fallback to WAV when the requested codec is unavailable. Closes vllm-project#4716 Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com>
86a1222 to
d46c46f
Compare
| if response_format == "wav": | ||
| raise | ||
| logger.warning( | ||
| "Failed to encode audio as '%s' (libsndfile may lack codec support), falling back to 'wav'.", |
There was a problem hiding this comment.
soundfile uses a bundled version of libsndfile, not the system one, so this should not happen.
There was a problem hiding this comment.
And aac is not supported by sndfile from pypi, so we shall not accept that as a valid format.
There was a problem hiding this comment.
Thanks Nick! I removed the try/except fallback entirely and also dropped AAC from the supported formats dict since the bundled libsndfile doesn't support it. Added input validation in serving_chat.py to reject invalid formats with a 400 error upfront.
| audio_format = audio_params.get("format", "wav") | ||
| else: | ||
| audio_format = "wav" | ||
| if audio_format == "pcm16": |
There was a problem hiding this comment.
Per https://developers.openai.com/api/reference/resources/audio/subresources/speech/methods/create, pcm16 is not a valid format name, we may want to validate user input against that list.
Edit: this is the chat completion endpoint, https://developers.openai.com/api/reference/resources/chat/subresources/completions/methods/create does use pcm16.
NickCao
left a comment
There was a problem hiding this comment.
And _create_diffusion_chat_completion has the same problem.
hsliuustc0106
left a comment
There was a problem hiding this comment.
Bugfix looks correct.
- Remove try/except fallback in audio_utils_mixin.py — soundfile bundles libsndfile so codec availability is deterministic. - Remove AAC from supported formats (not supported by bundled libsndfile). - Validate audio format against supported list before encoding, returning a 400 error for invalid formats. Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com>
alex-jw-brooks
left a comment
There was a problem hiding this comment.
Can you please add some tests to verify that the format conversion is correctly handled?
| sample_rate = int(sr_raw) | ||
|
|
||
| _valid_audio_formats = {"wav", "mp3", "flac", "opus", "pcm16", "pcm"} | ||
| audio_params = getattr(request, "audio", None) |
There was a problem hiding this comment.
Since the type of request is known here, can you avoid you use properties on the object instead of getattr?
There was a problem hiding this comment.
I've extracted the logic into a _resolve_audio_format() helper method that centralizes this, so the getattr is in one place only. Sounds good?
| else: | ||
| sample_rate = int(sr_raw) | ||
|
|
||
| _valid_audio_formats = {"wav", "mp3", "flac", "opus", "pcm16", "pcm"} |
There was a problem hiding this comment.
Can you centralize the supported formats to use a common def instead of inlining them here? They should also be in the protocol for audio, e.g., here, it would be better to use the same def so that we don't accidentally have this go out of sync with the protocol
There was a problem hiding this comment.
I've added SUPPORTED_AUDIO_FORMATS, SUPPORTED_CHAT_AUDIO_FORMATS, and DEFAULT_AUDIO_FORMAT constants in protocol/audio.py. Both serving_chat.py and audio_utils_mixin.py now import from there.
| if isinstance(audio_params, dict): | ||
| audio_format = audio_params.get("format", "wav") | ||
| else: | ||
| audio_format = "wav" |
There was a problem hiding this comment.
Similarly, I think that we should pull the default audio format from the protocol to use what is here
There was a problem hiding this comment.
deefault format now comes from DEFAULT_AUDIO_FORMAT imported from protocol/audio.py.
There was a problem hiding this comment.
Can you also make sure the docs are in sync? I.e., the examples all document aac as a format (e.g., here), so removing it as a supported format would make the behavior out of sync with OpenAI spec
…date docs - Add SUPPORTED_AUDIO_FORMATS, SUPPORTED_CHAT_AUDIO_FORMATS, and DEFAULT_AUDIO_FORMAT constants in protocol/audio.py. Remove aac from all Literal types and the supported_formats dict. - Extract _resolve_audio_format() helper in serving_chat.py to share format extraction and validation between _create_audio_choice and _create_diffusion_chat_completion (both had hardcoded "wav"). - Add unit tests for audio format conversion (magic bytes, format validation, pcm16 mapping, base64 encoding) and _resolve_audio_format. - Update docs to remove aac from supported output format lists. Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com>
|
Addressed all review feedback:
|
Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com>
…ate docs Sync with upstream PR vllm-project#4718 review feedback, adapted for nm-vllm-omni-ent: - Add SUPPORTED_AUDIO_FORMATS, SUPPORTED_CHAT_AUDIO_FORMATS, and DEFAULT_AUDIO_FORMAT constants in protocol/audio.py. Remove aac from all Literal types and supported_formats dict. - Extract _resolve_audio_format() helper in serving_chat.py shared between _create_audio_choice and _create_diffusion_chat_completion. - Add unit tests for audio format conversion and validation. - Update docs to remove aac from supported output format lists. Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com>
…turning empty choices Cherry-pick of upstream vllm-project/vllm-omni PRs vllm-project#4718 and vllm-project#4720, adapted for nm-vllm-omni-ent. - Wire up request audio.format to existing encoding infrastructure instead of hardcoding WAV. Validate format against supported set. - Remove AAC from supported formats (unsupported by bundled libsndfile). - Validate requested output modalities against engine capabilities upfront, returning 400 for unsupported modalities instead of silently returning empty choices with HTTP 200. - Validate that modalities is a list of strings. Closes vllm-project#4716 Closes vllm-project#4719 Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com>
…sts, update docs Sync with upstream PR vllm-project#4718 review feedback, adapted for nm-vllm-omni-ent: - Add SUPPORTED_AUDIO_FORMATS, SUPPORTED_CHAT_AUDIO_FORMATS, and DEFAULT_AUDIO_FORMAT constants in protocol/audio.py. Remove aac from all Literal types and supported_formats dict. - Extract _resolve_audio_format() helper in serving_chat.py shared between _create_audio_choice and _create_diffusion_chat_completion. - Add unit tests for audio format conversion and validation. - Update docs to remove aac from supported output format lists. Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com>
ChatCompletionRequest moved to vllm.entrypoints.openai.chat_completion.protocol ErrorResponse moved to vllm.entrypoints.openai.engine.protocol Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com>
…turning empty choices Cherry-pick of upstream vllm-project/vllm-omni PRs vllm-project#4718 and vllm-project#4720, adapted for nm-vllm-omni-ent. - Wire up request audio.format to existing encoding infrastructure instead of hardcoding WAV. Validate format against supported set. - Remove AAC from supported formats (unsupported by bundled libsndfile). - Validate requested output modalities against engine capabilities upfront, returning 400 for unsupported modalities instead of silently returning empty choices with HTTP 200. - Validate that modalities is a list of strings. Closes vllm-project#4716 Closes vllm-project#4719 Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com>
…sts, update docs Sync with upstream PR vllm-project#4718 review feedback, adapted for nm-vllm-omni-ent: - Add SUPPORTED_AUDIO_FORMATS, SUPPORTED_CHAT_AUDIO_FORMATS, and DEFAULT_AUDIO_FORMAT constants in protocol/audio.py. Remove aac from all Literal types and supported_formats dict. - Extract _resolve_audio_format() helper in serving_chat.py shared between _create_audio_choice and _create_diffusion_chat_completion. - Add unit tests for audio format conversion and validation. - Update docs to remove aac from supported output format lists. Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com>
When _resolve_audio_format returns an error (e.g. unsupported format like aac), _create_audio_choice returns an ErrorResponse instead of a list of choices. Without a guard, the callers crash: - Non-streaming: choices.extend() puts the ErrorResponse into the list, then audio_choices[0].message.audio raises AttributeError because ErrorResponse has no .message attribute. The client gets a 500 with "'tuple' object has no attribute 'message'" instead of a clean 400. - Streaming: the async generator iterates over the ErrorResponse, causing a similar crash. Add isinstance(choices_data, ErrorResponse) checks at both call sites. Non-streaming returns the ErrorResponse directly; streaming uses bare return to stop the generator (return-with-value is invalid in async generators). Verified on H100 with Qwen3-Omni: audio.format="aac" now returns a clean 400 with "Invalid audio format 'aac'" and the server stays alive. Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com>
| role = self.get_chat_request_role(request) | ||
| choices_data = self._create_audio_choice(omni_res, role, request, stream=True) | ||
| if isinstance(choices_data, ErrorResponse): | ||
| return |
There was a problem hiding this comment.
I think that this can result in bad behavior on chat completions, since audio format comes from the extra body in this case, so it doesn't have pydantic validation. E.g., if you make a stream request to Qwen3Omni for text+audio, it will yield the text part of the request without issues, then hit the error case, and silently shut the stream down instead of yielding the audio part. Some examples that may help:
# Assumes your server is running on port 8112, e.g., running with
# vllm serve Qwen/Qwen3-Omni-30B-A3B-Instruct --enforce-eager --omni --port 8112
from openai import OpenAI
BASE = "http://localhost:8112/v1"
MODEL = "Qwen/Qwen3-Omni-30B-A3B-Instruct"
client = OpenAI(base_url=BASE, api_key="dummy")
def test_streaming_invalid_format():
print("=== Streaming with audio.format='aac' ===")
try:
stream = client.chat.completions.create(
model=MODEL,
messages=[{"role": "user", "content": "Say hello in one sentence."}],
modalities=["text", "audio"],
audio={"voice": "alloy", "format": "aac"},
stream=True,
)
chunk_count = 0
has_audio = False
for chunk in stream:
chunk_count += 1
raw = chunk.model_dump(exclude_none=True, exclude_unset=True)
modality = raw.get("modality", "?")
choices = raw.get("choices", [])
finish = None
content_preview = ""
for c in choices:
delta = c.get("delta", {})
finish = c.get("finish_reason")
content = delta.get("content", "")
if modality == "audio":
has_audio = True
content_preview = f"[audio {len(content)}chars]"
elif content:
content_preview = repr(content[:60])
else:
content_preview = repr(delta)
print(f" chunk {chunk_count}: modality={modality} finish={finish} {content_preview}")
print(f" --- {chunk_count} chunks, has_audio={has_audio}")
if not has_audio:
print(" BUG: no audio chunks received despite requesting modalities=['audio']")
except Exception as e:
print(f" Error: {type(e).__name__}: {e}")
if __name__ == "__main__":
test_streaming_invalid_format()Running ^, you'll see this:
(vllm_omni) [alex-jw-brooks@h100 scratch]$ python streaming_audio_format_bug.py
=== Streaming with audio.format='aac' ===
chunk 1: modality=text finish=None {'content': '', 'role': 'assistant'}
chunk 2: modality=text finish=None 'Hello'
chunk 3: modality=text finish=None '!'
chunk 4: modality=text finish=None {'content': ''}
--- 4 chunks, has_audio=False
BUG: no audio chunks received despite requesting modalities=['audio']
Rather than handling it mid stream, I think we should probably validate this upfront, e.g., something like this:
async def _create_chat_completion(
self,
request: ChatCompletionRequest,
raw_request: Request | None = None,
) -> AsyncGenerator[str, None] | ChatCompletionResponse | ErrorResponse:
# Before starting the stream, check the audio format
audio_format_check = self._resolve_audio_format(request)
if isinstance(audio_format_check, ErrorResponse):
return audio_format_check
...
# Handle diffusion modewhich should give a 400 and better align with the non-stream case. I think we can also add a unit tests to the expansion tests for tests/e2e/online_serving/test_qwen3_omni_expansion.py to make the stream validates. e.g.,
from openai import BadRequestError
@hardware_test(res={"cuda": "H100", "rocm": "MI325"}, num_cards=2)
@pytest.mark.parametrize("omni_server", test_params[:1], indirect=True)
def test_invalid_audio_format_rejected(omni_server, openai_client) -> None:
"""Ensure we reject bad audio format prior to starting streaming."""
messages = dummy_messages_from_mix_data(
system_prompt=get_system_prompt(), content_text=get_prompt(),
)
with pytest.raises(BadRequestError, match="audio format"):
openai_client.client.chat.completions.create(
model=omni_server.model,
messages=messages,
modalities=["text", "audio"],
audio={"voice": "alloy", "format": "aac"},
stream=True,
)There was a problem hiding this comment.
thank you for the detailed explanation @alex-jw-brooks , very much appreciated!
There was a problem hiding this comment.
So I have moved the format validation upfront to _create_chat_completion (before the streaming/non-streaming branch), so an invalid format now returns a clean 400 before any chunks are sent. Removed the isinstance(choices_data, ErrorResponse) guards from the generators since they're no longer needed and added the E2E streaming test you suggested in test_qwen3_omni_expansion.py.
Move _resolve_audio_format() check to _create_chat_completion() so invalid formats (e.g. aac) return a 400 error immediately instead of silently dropping audio mid-stream. Add e2e test for the rejection. Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com>
…ons (vllm-project#4718) Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com> Signed-off-by: y_null <y_null@qq.com>
…ons (vllm-project#4718) Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com>
…ons (vllm-project#4718) Signed-off-by: Ricardo Noriega De Soto <rnoriega@redhat.com>
Purpose
Fix #4716 — The
audio.formatfield in chat completion requests was silently ignored. The server always returned WAV audio regardless of the requested format (mp3, flac, opus, etc.).Root cause:
_create_audio_choice()inserving_chat.pyhardcodedresponse_format="wav"when constructing theCreateAudioobject. The encoding infrastructure inaudio_utils_mixin.pyalready supports all formats — it was just never wired up.Changes:
serving_chat.py: Extractaudio.formatfrom the request and pass it toCreateAudioinstead of hardcoding"wav". Maps OpenAI's"pcm16"to soundfile's"pcm". Defaults to"wav"when not specified.audio_utils_mixin.py: Add fallback to WAV with a warning whensoundfile.writefails for the requested codec (e.g. MP3 on systems wherelibsndfilelacks LAME support).Test Plan
vLLM Version: latest
vLLM-Omni Commit: 2e6c83c
audio.formatset to"mp3","flac", and"wav"audio.formatstill return WAVTest Result
ruff checkandruff formatpass on both modified files