Fix DecoderState input_ids check regression introduced in #2103 - #2148
Conversation
PR #2103 (Gemma4 multimodal support) added an optional decoder_input_ids_ for models requiring input_ids alongside inputs_embeds (e.g. Gemma4). However, the check used the combined model_.session_info_ which aggregates inputs from ALL sessions (decoder + embedding + vision + speech). Because the embedding session always has input_ids as its primary input, HasInput('input_ids') returns true for every VLM with an embedding model — including mistral3, whose decoder ONNX only accepts inputs_embeds. This caused input_ids to be injected into the decoder's ORT feed at runtime, producing: RuntimeError: Invalid input name: input_ids Fix: create a decoder-only SessionInfo for this check so it only fires when the decoder ONNX itself actually declares input_ids as an input. Gemma4 behaviour is preserved since its decoder ONNX has input_ids. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Akshay Sonawane (@apsonawane) Can you check if this makes sense? |
There was a problem hiding this comment.
Pull request overview
Fixes a regression in the multimodal decoding pipeline where DecoderState could incorrectly assume the decoder ONNX model accepts input_ids, due to using an aggregated SessionInfo that also includes embedding-session inputs.
Changes:
- Narrow the
input_ids-presence check inDecoderStateto a decoder-onlySessionInfobuilt fromdecoder_session_. - Prevent passing an invalid
input_idsinput to decoders that only acceptinputs_embeds(e.g., mistral3 / Pixtral-family decoders).
|
Thanks- also a good point raised by kunal-vaishnavi where decoder potentially should not take input ids. Akshay Sonawane (@apsonawane) do you have a suggestion on what the proper contract for the components should be? Is the current contract with input ids in the decoder ok? |
|
Justin Chu (@justinchuby) is input_ids in gemma4 used for any other reason other than embedding lookup? If not, we should remove it from decoder and keep input_embeds as input. Embedding model already resolves tokens to embeddings so ideally decoder would not need input_ids. Looks like mistral3/pixtral, phi3-vision, qwen3.5 follows the same contract of keeping input_ids out of decoder. |
|
Ti-Tai Wang (@titaiwangms) Thank you for this fix. Shouldn't this issue be caught by mistral tests you added? |
|
The key is its per layer embedding architecture. If we are not careful there would be multiple slicing and duplication. Or a more complicated decoder signature. I can share more details here in a bit |
|
Justin Chu (@justinchuby) Akshay Sonawane (@apsonawane) The fix is correct for both models, and the design debate is a separate concern from this bug fix. By narrowing the check to decoder-only
The bug in PR #2103 was that the combined Whether Gemma4's decoder should eventually be refactored to accept only |
|
Akshay Sonawane (@apsonawane) Good catch on the test gap. The existing tests likely cover each session type in isolation and don't exercise I'll add a unit test that:
This would have caught the PR #2103 regression at the time it was introduced. |
|
I created onnxruntime/mobius#296 and a discussion on #2149. Please take a look. |
) The bug: DecoderState constructor used combined session_info_ (decoder + vision + embedding sessions) to check HasInput('input_ids'). The embedding session always declares input_ids, so the check incorrectly injected input_ids into the decoder feed for models like Mistral3 whose decoder has no input_ids input — causing an ORT 'Invalid Feed Input Name' error. The fix (src/models/multi_modal.cpp, commit 4271782): use a decoder-only SessionInfo for the HasInput('input_ids') check. Two test model variants are added to test/test_models/: - multimodal-decoder-no-input-ids/ (Mistral3-like: embedding has input_ids, decoder does NOT — the case that was broken) - multimodal-decoder-with-input-ids/ (Gemma4-like: both embedding and decoder declare input_ids — should succeed with or without fix) test/python/test_decoder_state_input_ids.py exercises both: - Mistral3-like: generation must succeed; with the pre-fix code it would fail because input_ids would be incorrectly fed to a decoder session that never declared it. - Gemma4-like: generation must also succeed; the fix correctly identifies that the decoder declares input_ids and feeds it. test/test_models/create_decoder_input_ids_test_models.py is the script used to regenerate the dummy ONNX model files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
If we can add a negative test that would be good |
Problem
PR #2103 (Add Gemma4 multimodal support) introduced an optional
decoder_input_ids_inDecoderStatefor models like Gemma4 that requireinput_idsalongsideinputs_embeds. The guard condition was:if (model_.session_info_.HasInput(model_.config_->model.decoder.inputs.input_ids)) {However,
model_.session_info_is a combinedSessionInfoaggregating inputs from ALL sessions — decoder, embedding, vision, and speech. Since the embedding session always takesinput_idsas its primary input,HasInput("input_ids")returnstruefor every VLM that has an embedding model, even when the decoder ONNX itself has noinput_idsinput.Impact
Any VLM using the 3-model pipeline (embedding + decoder) where the decoder does not have
input_idswill fail at inference with:Confirmed broken: mistral3 (Ministral-3B / Pixtral family), whose decoder only accepts
inputs_embeds.Fix
Scope the check to a decoder-only
SessionInfoso it only fires when the decoder ONNX itself declaresinput_idsas an input:Gemma4 is unaffected — its decoder ONNX genuinely has
input_idsas an input, so the narrowed check still fires correctly for that model.Verification
Tested with the exported Ministral-3B-Instruct-2512 (mistral3 type, 3-model VLM):
generator.set_inputs(inputs)no longer throwsRegressed by: #2103