[Bugfix][Core] Fix multimodal prompt building for rows without media - #404
[Bugfix][Core] Fix multimodal prompt building for rows without media#404natedemoss wants to merge 4 commits into
Conversation
`_build_messages` substitutes `<image>`/`<video>`/`<audio>` placeholders by
splitting the prompt on a regex alternation built from the placeholders a row
actually carries. Three problems with how that set is built:
- A row whose media column is absent (`None`) contributes nothing, so for a
text-only row of a mixed dataset the alternation collapses to `"()"`. That
pattern matches the empty string at every position, and the prompt comes back
as one `{"type": "text"}` dict per character. `filter_long_prompt` already
treats text-only rows as a supported case, so such rows do reach here.
- A media type name that `MultimodalTypes.get` does not know was skipped
silently. A plausible typo (`{"images": "images"}` instead of
`{"image": "images"}`) therefore dropped every image and trained text-only
against a VLM dataset, with no warning. Reject it instead, and name the
supported types in the `--multimodal-keys` help.
- The leftover-media check ran even when no message had string content. A row
already in list-of-dicts form is deliberately passed through untouched
("no processing will be done"), so its media is never spent on a placeholder
and the check aborted the whole dataset load with a count-mismatch assert.
Only run it when placeholder expansion actually happened.
Also add the missing space in that assert's message.
Tests: tests/test_build_messages.py covers all three, plus the substitution
paths that must not change. Three of the nine fail on main.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: natedemoss <ndemoss28@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive unit tests for _build_messages and refactors the function to handle edge cases, such as raising a ValueError for unknown multimodal types, avoiding empty placeholder splitting, and ensuring leftover media validation only runs when substitution occurs. The review feedback suggests robustly handling single string or dictionary media entries by wrapping them in a list, adding a test for this behavior, and replacing the runtime assert statement with an explicit AssertionError to prevent it from being bypassed under Python optimization flags.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: natedemoss <ndemoss28@gmail.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: natedemoss <ndemoss28@gmail.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: natedemoss <ndemoss28@gmail.com>
|
I found one remaining mixed-message case at from vime.utils.data import _build_messages
row = {
"text": [
{"role": "system", "content": "You are helpful."},
{"role": "user", "content": [
{"type": "image", "image": "a.png"},
{"type": "text", "text": "Describe it."},
]},
],
"images": ["a.png"],
}
_build_messages(row, "text", True, {"image": "images"})This raises
Could the regression coverage include these mixed conversations and define how inline media relates to the row media column? Simply counting only matched placeholders would also need to preserve the existing all-string missing-placeholder validation. Verified through the real |
I was poking at
--multimodal-keyswith a dataset that mixes image rows and plaintext rows, and ended up finding three separate problems in
_build_messages.The prompt gets shattered when a row has no media
The placeholder split builds a regex alternation out of whatever media the row
actually carries. When the media column is
Nonenothing goes into that set, thepattern collapses to
"()", and that matches the empty string at every position:One content dict per character. These rows definitely reach here —
filter_long_promptalready has an explicit text-only branch for exactly this shape of dataset.
I want to be upfront that this is the least severe of the three. I checked
Qwen2-VL, Qwen3-VL and GLM-4.1V, and all three render the shattered content to
byte-identical text, so it's wasted work and a fragile representation rather than
corrupted training data. Still worth not doing.
A typo in
--multimodal-keyssilently drops every imageMultimodalTypes.get()returnsNonefor a name it doesn't know, and the old codejust skipped those. So
'{"images": "images"}'— plural, which is an easy thing totype given the column is usually called
images— quietly attaches no media at alland you train text-only against a VLM dataset with nothing in the logs to tell you.
This one I'd call the real bug. I made it raise, and listed the supported types in the
--multimodal-keyshelp text since they weren't written down anywhere.This is the only behaviour change in the PR that could break a currently-working run,
so shout if you'd rather it stayed a warning.
Rows already in list-of-dicts form abort the dataset load
If a message's content is already a list, the code deliberately passes it through
("no processing will be done"). But the leftover-media check afterwards doesn't know
that happened, so the row's media has no placeholder to be spent on and the whole
Datasetconstruction dies:Now it only runs the check when placeholder expansion actually happened. (Also put the
missing space back in that message.)
Tests
tests/test_build_messages.py, wired into the synchronized upstream CPU tests jobnext to
test_filter_long_prompt.py. Nine cases: three fail onmain, the other sixpin the substitution behaviour that should not move.
One asymmetry I left alone and pinned instead:
images: Noneleaves the content as aplain string,
images: []gives a single text dict. Collapsing[]into the no-mediapath would be tidier but you'd lose the
Not enough image dataassert when a prompthas
<image>and an empty column, which seemed like the more useful behaviour to keep.Verification
CPU jobs all pass locally (~740 tests across plugin contracts, upstream sync CPU,
utils and agent adapter).
pre-commitclean.Two things I couldn't run on my machine, neither touched by this change: the GPU
suites, and
test_glm5_indexer_q_norm/test_rollout_metrics/test_rollout_routing_replay_validation, which need megatron and vllm from thevllm/vime:latestimage.test_cispo_lossandtest_policy_lossfail for me on cleanmaintoo — Windows boxwith no C++ toolchain, so
torch.compilecan't get throughpick_vec_isa. Unrelated.AI-assisted, per CONTRIBUTING: the diff and tests were written by Claude Opus 5
(Anthropic, via Claude Code) working from my direction, and it's attributed in the
commit trailer. I've reviewed every changed line and run the tests above.