[fix] Revert "fixing mm placeholder replacement issue with gemma3" #28285
Conversation
Signed-off-by: Kevin H. Luu <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly reverts a previous change that introduced a regression in the multi-modal test suite for Gemma3 models. The revert ensures the proper ordering of newline tokens during placeholder replacement, which is critical for the correct functioning of multi-modal processing.
| def get_repl_toks(tok: int) -> list[int]: | ||
| if tok == newline_3: | ||
| return [newline_2, newline_1] | ||
| return [newline_1, newline_2] |
There was a problem hiding this comment.
The change from [newline_2, newline_1] to [newline_1, newline_2] is crucial for correctly decomposing the newline_3 token (\n\n\n). While _apply_token_matches (lines 370-380) handles both permutations as equivalent for combining into newline_3, the _find_mm_placeholders function, which uses this decomposition, relies on a specific order for accurate matching of multi-modal placeholders. The previous incorrect order led to a breaking test, highlighting the sensitivity of the system to this sequence. It would be beneficial to add a comment explaining why this specific order is required for decomposition, given that combination is order-agnostic, to prevent future regressions.
| return [newline_1, newline_2] | |
| return [newline_1, newline_2] # Decompose as '\n' then '\n\n' for consistent placeholder matching |
…llm-project#28285) Signed-off-by: Kevin H. Luu <[email protected]>
…llm-project#28285) Signed-off-by: Kevin H. Luu <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
…llm-project#28285) Signed-off-by: Kevin H. Luu <[email protected]>
Revert #27538 since it's breaking Multi-Modal Test Extended 3