Repository navigation
Normalize ChatML content once, instead of per render site - #159
Merged
ItzikVa merged 1 commit intoNov 2, 2026
Conversation
PR #150 correctly found why composing a 4.2 checkpoint corrupts prompts: the ALoRA Pass 1 scan iterates message.content, which is exactly the AST shape vLLM's content-format detector keys on, so composing flips it to 'openai' and every content arrives as a parts list the ChatML template cannot flatten. Its fix patched three render sites, and a fix built from a list of sites is only as complete as the list. Three sites were missed, all reproduced against the real ibm-granite/granite-4.2-3b template: - Assistant history rendered as [{'type': 'text', 'text': 'a1'}]. The PR scoped assistant turns out on the grounds that no client sends them as parts, but it is vLLM that produces the shape, by wrapping plain strings under 'openai' -- and it wraps the whole messages array, not just the roles we think of as incoming. - An assistant turn with tool_calls lost its text entirely. That branch gates on `content is string`, so a parts list fails the check and the text is dropped rather than repr'd. This one is the reason the suite could not see any of this: the break criterion was `"[{'type'" in rendered`, and an absence has no marker to grep for. The new test asserts presence instead. - The catch-all else branch raised TypeError on any other role. Base 4.2 escapes this only because vLLM detects 'string' for it, so it is a composed-only regression from removed protection, not a template difference. So normalize loop_messages once before the loop, which is what the 4.1 family already does natively and why 4.1 was never affected. Being role-agnostic it cannot be incomplete the same way, and a role added to a future base template is covered without touching this code. The per-message and tool-branch flattens are deleted as redundant -- two paths flattening the same data is how the earlier audio/text split happened. The preamble system-message flatten stays: it reads messages[0]["content"] before loop_messages exists. The plain-string path is asserted byte-identical to the base template, so the common case carries no regression risk; dict(_m, content=...) copies rather than mutates, which also keeps it legal under the immutable sandbox transformers renders with. Both silent injections now raise. A composer that no-ops a correctness fix writes and publishes the checkpoint before anything renders a prompt, and the inconsistency was itself a trap -- one of three sites failed loudly, two did not. Audio keeps one shared block, now upgraded in place. Behaviour change worth noting: the marker is emitted for audio parts in any role rather than user/system only. Widening is the safe direction, since a dropped audio part is what breaks vLLM's prompt replacement. All four new tests fail against the previous implementation and pass here, so they discriminate rather than merely pass. 735 CPU tests pass; the 2 vLLM skinning failures are pre-existing on macOS and reproduce with these changes stashed. Reported-by: Aviv Ron <aviv1ron1@users.noreply.github.com> Signed-off-by: itzikvaisman <yizhak.vaisman@ibm.com>
ItzikVa
marked this pull request as ready for review
November 2, 2026 08:17
ItzikVa
requested review from
antonpibm,
aviv1ron1,
freunda and
yairallouche
as code owners
November 2, 2026 08:17
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #150. The main diagnosis there is right and the fix direction is
right: a composed 4.2 ChatML checkpoint must render correctly with no launch
flags, because
--chat-template-content-format stringis per-server knowledgeand no flag helps the HF engine, which has no auto-detection at all.
This PR changes where the flatten happens. #150 patches three render sites
(the per-message user/system branch, the preamble system message, the tool
branch). The 4.2 template has seven content-reading sites, so three were
missed — which is the four issues Aviv found. This replaces the per-site
patches with one normalize pass before the message loop, mirroring what the
4.1 template already does natively.