Render chat templates via hf-chat-template for transformers parity#452
Open
GregoryBolshakov wants to merge 1 commit into
Open
Render chat templates via hf-chat-template for transformers parity#452GregoryBolshakov wants to merge 1 commit into
GregoryBolshakov wants to merge 1 commit into
Conversation
kalosm-llama's minijinja renderer diverged from
transformers.apply_chat_template on ordinary conversations. System
messages were emitted with role "developer" (the MessageType serde
rename flows through context!{role}), so templates that branch on
role == 'system' substituted a default system prompt, dropped the
message, or raised "roles must alternate". trim_blocks/lstrip_blocks
were also unset, and tojson did not match Python's json.dumps.
Delegate rendering to hf-chat-template, verified byte-identical to
Python transformers across a golden corpus of real model templates.
The renderer's public shape (create/format) and the minijinja::Error
type carried by this crate's error enums are unchanged; bos/eos are
passed per call via the render input.
Drop the direct minijinja Environment and minijinja-contrib usage
(minijinja is retained only for its Error type); move chrono to
dev-dependencies since only a test uses it now.
|
Preview available at https://floneum.github.io/kalosm/pr-preview/pr-452/ |
Contributor
Author
|
Heads up on CI: the only failing tests are the live-API ones in |
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.
Fixes #451.
The chat template renderer drifts from what
transformers.apply_chat_templateproduces, mostly around the system role.MessageType::SystemPromptserializes to"developer", and that value flows throughcontext!{ role }, so any template that checksrole == 'system'never matches the system message. Depending on the template that means a default system prompt gets substituted, the message gets dropped, or the render raises "roles must alternate". On top of that,trim_blocks/lstrip_blocksaren't set andtojsondoesn't match Python'sjson.dumps.I checked this against 20 real model templates from the Hub (68 cases), comparing byte-for-byte against output generated by Python transformers. Of the 48 cases the current renderer can express, 31 matched and 17 didn't, and 3 of those raise an exception on a plain system/user/assistant chat (Falcon 7B, Gemma 3, Mistral 7B). After this change all 48 match.
This delegates rendering to hf-chat-template, a small crate that does one thing: render HF chat templates byte-identically to transformers (Python string methods,
strftime_now,trim_blocks/lstrip_blocks, a transformers-compatibletojson, standalone template files). It brings in minijinja, which is already in the tree, plus serde_json.The renderer's public shape (
create/format) doesn't change, and theminijinja::ErrorthatLlamaModelErrorandLlamaSourceErrorcarry stays the same, so there's no API break. minijinja is kept just for that error type. The five existing chat_template tests are unchanged. Two smaller knock-on changes: serde_json was optional behindhf-config-jsonand the renderer needs it now, so it's a normal dependency; and chrono moves to dev-dependencies since only a test still uses it.If you'd rather not take the dependency, the alternative is reimplementing pycompat, the block trimming, a Python-compatible
tojson, andstrftime_nowinline, which is what the crate already does. Happy to go either way.One note: I couldn't run the full test suite on my machine (the dev-dependencies pull in openssl-sys and I don't have system OpenSSL here), but
cargo check,fmt, andclippyare clean on kalosm-llama, and I checked the rendering against the published crate separately.