fix: raise ValueError for image/audio inputs in LocalHFBackend#1410
fix: raise ValueError for image/audio inputs in LocalHFBackend#1410markstur wants to merge 3 commits into
Conversation
LocalHFBackend silently dropped image and audio content on all three generation paths (_generate_from_context_standard, _generate_from_context_with_kv_cache, _generate_from_intrinsic). This encouraged hallucination when attempting image-text-to-text or audio-text-to-text with a query and an image/audio block that is dropped. - Add _check_no_multimodal_blocks() helper using get_images_from_component and get_audio_from_component so Instruction images/audio are caught as well as Message images/audio and future components - Call the check before to_chat() on all three paths so no formatting work is done on the error path - Add public .images and .audio properties to Instruction (Message already had them; required for get_*_from_component to work) - Export get_images_from_component from mellea.core (get_audio already was exported) Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com> Assisted-by: IBM Bob
This touches a component only to make Instruction and Message more consistent in Component PR ChecklistUse this checklist when adding or modifying components in Protocol Compliance
Content Blocks
Integration
|
psschwei
left a comment
psschwei
left a comment
There was a problem hiding this comment.
LGTM
One question (maybe for @jakelorocco): do we anticipate adding multimodal support for the HF backend in the future?
FYI -- For audio, it might be a significant enough to need a new backend. I didn't look into it enough to be sure, but the model might need to be loaded by AutoModelForSpeechSeq2Seq (I do have an AI-generated prototype of that suggested solution). Personally, I think that ideally one backend would just figure out how to load different models differently with Auto... or a pipeline name param or something, but I'm aware that there are probably reasons to have more backends that are simpler (easy) vs try to make one backend do everything for HF (which can get ugly). Something to investigate in future. In the meantime, our audio-text-to-text support in backends is limited (reflecting the current state of models/servers), but I think it useful that we get the API plumbing right and catch non-supported cleanly like this instead of silently dropping. |
ajbozarth
left a comment
ajbozarth
left a comment
There was a problem hiding this comment.
Claude review below, I'll leave final approval to others as I dont have the bandwidth to do a manual review.
Correct root-cause fix — using get_*_from_component catches Instruction/Message/future components, and placing the check before to_chat() avoids wasted work. Two non-blocking nits below.
| _GENERATE_KWARGS_ALLOWLIST: frozenset[str] = _compute_generate_kwargs_allowlist() | ||
|
|
||
|
|
||
| def _check_no_multimodal_blocks( |
There was a problem hiding this comment.
This helper isn't called from _generate_from_raw, which formats actions directly and would still silently drop multimodal content — so the "silently dropped" gap is only partially closed. Either guard that path too or note the remaining gap in the PR description / a follow-up issue.
|
|
||
| Raises: | ||
| ValueError: If no adapter is registered for the requested intrinsic. | ||
| ValueError: If the context contains images or audio; `LocalHFBackend` |
There was a problem hiding this comment.
Two Raises: ValueError entries — merge into one ValueError: with a combined description to match Google-style convention.
_generate_from_raw was silently dropping image/audio content in actions by passing them to formatter.print() without any guard. Add a per-action call to _check_no_multimodal_blocks (ctx=None, since ctx is never rendered on the raw path). Also make ctx optional in the helper. Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com> Assisted-by: IBM Bob
Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com>
planetf1
left a comment
planetf1
left a comment
There was a problem hiding this comment.
Nice fix — a hard error is much better than silently dropping images/audio and letting the model hallucinate around them. Reviewed this thoroughly, no blockers. Two test gaps worth closing before merge, noted below. Everything else checked out fine.
| def audio(self) -> list[AudioBlock | AudioUrlBlock] | None: | ||
| """Returns the audio associated with this instruction.""" | ||
| return self._audio | ||
|
|
There was a problem hiding this comment.
These properties are what let the guard detect multimodal content on an Instruction (it relies on hasattr(c, "images")). All the new tests use Message, though — could you add a test that goes through Instruction instead, so this path is actually covered?
| if not ctx.is_chat_context: | ||
| raise Exception("Does not yet support non-chat contexts.") | ||
|
|
||
| _check_no_multimodal_blocks(None, ctx) |
There was a problem hiding this comment.
_generate_from_intrinsic also got the guard, but nothing tests it. Could you add one for this path too?
4 participants
Pull Request
Issue
Fixes #1405
Description
LocalHFBackend silently dropped image and audio content on all three generation paths (_generate_from_context_standard, _generate_from_context_with_kv_cache, _generate_from_intrinsic). This encouraged hallucination when attempting image-text-to-text or audio-text-to-text with a query and an image/audio block that is dropped.
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.