feat(core): audio file/URL constructors and representation-aware capability guards - #1601
feat(core): audio file/URL constructors and representation-aware capability guards#1601jakelorocco wants to merge 2 commits into
Conversation
Assisted-by: Claude Code Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
Signed-off-by: jakelorocco <59755218+jakelorocco@users.noreply.github.com>
|
@markstur, could you please take a look at this PR since it touches / adds some audio feature enhancements in areas you'd previously worked on? |
There was a problem hiding this comment.
This is something that Claude picked up, though it seems fairly minor as I don't think we would hit this very often. Mentioning it as better safe than sorry, but could also see ignoring this:
_generate_from_intrinsic serializes context messages at openai.py:760 without a prefetch, so an AudioUrlBlock there resolves via the blocking resolve_base64() on the event loop.
The cache added here (here being L1013) makes this a hit in almost every real ordering, since any prior generation on the standard path warms it. The one case it misses is an intrinsic called on a context no generation has touched, which is the documented pattern for the intrinsic helpers (check_certainty(context, backend) over a hand-built ChatContext). Worst case is one bounded 30 s download, not wrong output.
Suggest adding this after line 748 for symmetry with the standard path:
messages: list[Message] = self.formatter.to_chat_messages(linearized_context)
await prefetch_audio_urls(messages)
| else: | ||
| return None | ||
| else: | ||
| if not hasattr(c, "audio"): |
There was a problem hiding this comment.
Again, another minor thing that Claude found while reviewing. I think it probably makes sense to just be aware of this but not to fix as that would result in a lot of extra calls.
The fallback fires only when the attribute is absent, but message_from_template_representation (chat.py:354-364) always reads tr.audio / tr.images. So a component exposing audio = None while declaring audio=[...] on its representation passes this guard, then has the clip put on the Message by the formatter and silently dropped by HF's apply_chat_template. Same silent drop this fallback exists to prevent, different shape. test_representation_fallback_not_consulted_when_attribute_present currently pins that as intended.
No built-in can hit it: Instruction.audio and its format_for_llm both read self._audio (instruction.py:204, 229), same for Message. So this is about the extension surface, not a live bug.
Falling back whenever the attribute yields nothing, rather than only when it's missing, would make the guard agree with the payload path. That does cost the optimization the docstring calls out, since every attachment-free Instruction/Message would then pay two extra format_for_llm() calls per generation on the HF path. Fine to keep as-is and just document that a component's attribute must agree with its representation, but worth being a deliberate choice rather than an accident.
markstur
left a comment
markstur
left a comment
There was a problem hiding this comment.
Good feature. Nice to fix the dead end AudioUrlBlock. See inline comment, however, about how vLLM can support remote URLs. Probably just FYI and no change.
I think Paul's comment should be addressed with an await prefetch.
Otherwise the doc additions, in particular, are a great addition. I had to check to see if I forgot to commit something like that (I didn't. I think I was too focused on m serve examples).
Also some little doc/comment nits inline.
| f"Could not read audio file {os.fspath(path)[:120]!r}: expected a path " | ||
| f"to an audio file on disk. ({type(e).__name__}: {e}) " | ||
| "To load remote audio, fetch the bytes and use AudioBlock.from_bytes(); " | ||
| "for base64 data use AudioBlock(value, format=...)." |
There was a problem hiding this comment.
Instead of fetch, this guidance can now use from_url() e.g.:
To load a remote URL use AudioBlock.from_url() or AudioUrlBlock; for raw bytes use AudioBlock.from_bytes()
| self.format = format | ||
|
|
||
| def resolve_base64(self) -> str: | ||
| """Return the audio as raw base64, downloading it once per URL. |
There was a problem hiding this comment.
there is a race condition where the same URL can be downloaded multiple times in parallel and the last one just wins. Not a big concern. I think (if not fixed) a comment somewhere would be good. Maybe here?
| carries a `"reasoning_content"` field. | ||
|
|
||
| Raises: | ||
| ValueError: If the message contains an `AudioUrlBlock`. The OpenAI Chat |
There was a problem hiding this comment.
outdates docstring. The AudioUrlBlock ValueError was removed
|
|
||
| ### Remote audio | ||
|
|
||
| No provider accepts audio by URL — OpenAI Chat Completions has no audio-by-URL content |
There was a problem hiding this comment.
This statement is fine for Mellea/OpenAI, but...
https://docs.vllm.ai/en/v0.6.2/getting_started/examples/openai_audio_api_client.html
So the current AudioUrlBlock was partly just nice symmetry with ImageUrlBlock even though it was a dead end, but was also a potential extension to support vLLM's non-comforming extension of OpenAI API (or maybe anticipating an OpenAI API future change?) which can do the URL download.
I'm OK with ignoring the vLLM extension, keeping this comment as-is and just saying we'll deal with that in the future (or maybe never). Probably not a priority and no need to speculate. Essentially that's why there was no issue to implement the vLLM specific audio_url.
I just want to make sure we agree that we can kick this down the road and if future us wants to add remote audio-by-URL support, we'd probably just add a check similar to is_vllm_server_with_...() to make the prefetch/cache skippable? This and that could co-exist.
3 participants
Pull Request
Issue
Fixes # N/A
Description
Adds some functionality to make it easier to work with audio and adds a few guards as well. Matches functionality with the images feature.
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.