Escape caller template text spliced into Jinja string literals - #7731
Merged
danielhanchen merged 5 commits intoAug 2, 2026
Merged
danielhanchen merged 5 commits into
danielhanchen merged 5 commits into
Conversation
construct_chat_template builds the HF Jinja template by concatenating the
caller's template text straight into '...' literals, in three places: the
process() helper, the add_generation_prompt literal, and full_system. None
of them escape, so Jinja reads the text as template source.
A single quote closes the literal early:
default_system_message = "Answer the user's question."
-> TemplateSyntaxError: expected token 'end of print statement', got 's'
and a backslash is decoded as a Jinja escape, which is silent:
default_system_message = r"Put the answer in \boxed{}."
-> 'Put the answer in \x08oxed{}.\n### User: Hi\n'
default_system_message = r"Files live in C:\Users\me"
-> TemplateSyntaxError: truncated \UXXXXXXXX escape
The backslash case is the worse one: no error, no warning, and every
formatted training sample is built with a backspace character where
\boxed was meant to be.
It is not limited to the system message. process() handles the
instruction and response sections too, so an apostrophe anywhere in the
template breaks it, for example "### User's turn: {INPUT}".
Escape backslashes then single quotes in each literal chunk. In process()
the text is split on the {INPUT}/{OUTPUT}/{SYSTEM} sentinel first, so the
' + message['content'] + ' concatenation markers it inserts are not
escaped along with it.
The Ollama modelfile splices default_system_message into a double-quoted
SYSTEM line with the same lack of escaping; that is a different format
with different rules and is left alone here.
vineethsaivs
force-pushed
the
fix/escape-jinja-literals-in-chat-template
branch
from
August 2, 2026 05:14
9a76e18 to
be993a8
Compare
Contributor
Author
|
Rebased onto current |
for more information, see https://pre-commit.ci
Two follow ups to the Jinja literal escaping in this PR.
Jinja rewrites a raw carriage return to \n inside a string literal before it
unescapes it, so a template authored on Windows loses its CRLF. Escaping \r
alongside \ and ' makes the round trip exact, and makes the generated template
render identically under jinja2, minja and llama.cpp's Jinja engine.
The BOS was stripped from the system section after process() had already
escaped it, so a bos_token holding a quote or a backslash no longer matched and
was left in the literal, then emitted a second time alongside {{ bos_token }}.
Strip it while the text is still raw.
Member
|
Thanks, this is a good catch and the analysis of the three splice sites is right. I went through it and pushed two follow ups directly to the branch (09daf70). 1.
|
| revision | all three engines agree | llama.cpp load errors |
|---|---|---|
main |
147/246 (60%) | 87 |
| this PR | 234/246 (95%) | 0 |
| with these two commits | 246/246 (100%) | 0 |
So the PR already fixes a real GGUF problem that was not in the description: on main, 87 of these templates fail to load in llama.cpp with unknown escape character \U (its lexer rejects unknown escapes) while minja silently swallows the backslash. The remaining 12 disagreements on your branch were all the \r family, which the escape closes.
Verification
- 250 case differential harness across base, this branch and the follow ups.
_ollama_modelfile,_unsloth_input_partand_unsloth_output_partare byte identical in every case, and the safe control templates (llama-3, alpaca, chatml, vicuna, zephyr, unsloth, and the four templates theLlama3_(8B)-Ollamanotebooks ship) are byte identical too. Exactly 20 of 250 cases move relative to your branch: the 18 CR ones and the 2 BOS ones, nothing else. - Mutation check: removing any one of the three
escape_jinja_literalcall sites makes the suite fail, so all three are load bearing. - 12 sandboxes of Python 3.10/3.12/3.13 by transformers 4.51.3/4.57.6/5.5.0/5.14.1 all produce byte identical output, and escaping is invariant under all 8 combinations of
trim_blocks,lstrip_blocksandkeep_trailing_newline. - Save and reload: stored value matches the in memory template for
save_jinja_filesboth ways, save to load to save is idempotent with no second escaping layer, and transformers 4.51.3 reads achat_template.jinjasidecar carrying the escaped template correctly. Loading never rewrites an artifact, so nothing already on disk changes on upgrade. train_on_responses_onlylabel mask is identical fresh (private markers present) versus after reload (markers derived by rendering probes), forforce_matchboth ways.- Repo tests (CPU): 3565 passed, 0 failed. Focused file goes 17 to 19 tests, and both new cases fail without the fix.
- Two step LoRA smoke on a template full of apostrophes and backslashes: finite losses, LoRA weights move, generation prompt keeps the raw text.
Two things I left alone
The Ollama modelfile is still unescaped, and it is unescaped in more places than you listed: the """ Go template block and the PARAMETER stop "..." line, not just SYSTEM "...". You were right to keep it out, and the Jinja helper must not be reused there since Ollama's parser does not do backslash decoding. Separate PR.
Same for _change_system_message and the mapping splice in get_chat_template, which have the identical hazard on the preset path. Your read on #4222 is correct, they are complementary. Worth noting for that one: it applies the message as a re.sub replacement, so \boxed becomes a backspace and C:\Users raises bad escape \U before Jinja ever sees it.
Also worth a release note: the contract is now raw text in, raw text out, so anyone who hand escaped user\'s to work around this will see a visible backslash. Nothing in tree does that on this path, the two pre-escaped DEFAULT_SYSTEM_MESSAGE vicuna constants go through _change_system_message instead.
Nice work, merging once CI is green.
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Member
|
@codex review |
1 similar comment
Member
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
danielhanchen
added a commit
that referenced
this pull request
Aug 2, 2026
* Escape the system message spliced into predefined chat templates
get_chat_template(..., system_message = ...) substitutes the message into a
{system_message} placeholder that sits inside a Jinja string literal in all 15
predefined templates that carry one, so a quote closes the literal and a
backslash is read as an escape:
vicuna "Answer the user's question." -> TemplateSyntaxError
vicuna r"Put it in \boxed{}." -> renders '\x08oxed{}'
vicuna r"C:\Users\me" -> TemplateSyntaxError
Reuse the escaper PR #7731 added for construct_chat_template, promoted to a
module-level _escape_jinja_literal and extended to escape double quotes so the
one helper covers llama-3.1's "..." literal as well as the '...' the rest use.
Apply it to the predefined branch of _change_system_message and to the ShareGPT
mapping values, and drop the hand-escaping from the two vicuna defaults, which
would otherwise be escaped twice.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Tighten the escaping comments
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
danielhanchen
added a commit
that referenced
this pull request
Aug 2, 2026
FastLlamaModel.from_pretrained took a `revision` argument and never read it, so the config, the weights and the tokenizer all came from the repo's default branch while the caller believed they had pinned a ref. Reported in #3544 by someone versioning their fine-tunes with branches, which makes it a silently wrong base checkpoint rather than an error. Forward it in llama.py (both AutoConfig loads, the three model loads, the tokenizer, the prefetch warm and the fp8 scale restore), plumb it through load_correct_tokenizer, and read it from kwargs in vision.py for the four AutoConfig, two processor and two tokenizer loads plus the VLM processor fallback. vision.py must not bind it as a named parameter: the weight load there forwards **kwargs, so binding it would drop it from that load. model_name is not always the repo the caller named. get_model_name can swap in a pre-quantized mirror, _offline_quantize_to_fp8 an fp8 temp dir, ModelScope a local snapshot, and fast_inference_setup a -bnb-4bit variant, and use_exact_model_name only gates the first of those. A ref from the original repo does not exist on the substitute, so _revision_for_resolved_repo drops it with a warning naming both repos when the resolution changed the name. The adapter load keeps the caller's revision, since that one really is for old_model_name. Supersedes the earlier attempt on this branch, whose chat-template hunk is handled by #7731 and #7746, whose vision.py signature change caused the drop described above, and whose load_vllm(revision = ...) raised TypeError because load_vllm has no such parameter. Fixes #3544
danielhanchen
added a commit
that referenced
this pull request
Aug 3, 2026
* fix: add revision parameter support and escape quotes in chat templates - Fix #3544: Add revision parameter to AutoConfig, AutoModelForCausalLM, AutoModelForSequenceClassification, and load_correct_tokenizer calls in FastLlamaModel.from_pretrained. This enables loading specific model revisions/branches from HuggingFace Hub. - Fix #3667: Escape single quotes in system messages before substituting into Jinja2 templates. This prevents TemplateSyntaxError when system messages contain apostrophes (e.g., "user's" in Vicuna templates). Signed-off-by: majiayu000 <1835304752@qq.com> (cherry picked from commit b0a6e41) * fix: propagate revision parameter to vLLM and PEFT loaders - Add revision to load_vllm_kwargs in llama.py to fix config/weights mismatch - Add revision to PEFT AutoConfig calls in loader.py (FastLanguageModel & FastModel) Addresses reviewer feedback from @chatgpt-codex-connector and @Datta0 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> (cherry picked from commit 14f89e4) * fix: add revision parameter to FastBaseModel in vision.py Propagate revision parameter to all from_pretrained calls in vision.py to ensure consistent version pinning for vision models. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> (cherry picked from commit c5aa4ec) * Forward revision to the config, weight and tokenizer loads FastLlamaModel.from_pretrained took a `revision` argument and never read it, so the config, the weights and the tokenizer all came from the repo's default branch while the caller believed they had pinned a ref. Reported in #3544 by someone versioning their fine-tunes with branches, which makes it a silently wrong base checkpoint rather than an error. Forward it in llama.py (both AutoConfig loads, the three model loads, the tokenizer, the prefetch warm and the fp8 scale restore), plumb it through load_correct_tokenizer, and read it from kwargs in vision.py for the four AutoConfig, two processor and two tokenizer loads plus the VLM processor fallback. vision.py must not bind it as a named parameter: the weight load there forwards **kwargs, so binding it would drop it from that load. model_name is not always the repo the caller named. get_model_name can swap in a pre-quantized mirror, _offline_quantize_to_fp8 an fp8 temp dir, ModelScope a local snapshot, and fast_inference_setup a -bnb-4bit variant, and use_exact_model_name only gates the first of those. A ref from the original repo does not exist on the substitute, so _revision_for_resolved_repo drops it with a warning naming both repos when the resolution changed the name. The adapter load keeps the caller's revision, since that one really is for old_model_name. Supersedes the earlier attempt on this branch, whose chat-template hunk is handled by #7731 and #7746, whose vision.py signature change caused the drop described above, and whose load_vllm(revision = ...) raised TypeError because load_vllm has no such parameter. Fixes #3544 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Tighten the revision comments * Gate the revision before the config probes, and never mix refs Four fixes from review: - The gate ran after the AutoConfig and PeftConfig probes, which already used the raw revision against the resolved name, so a pinned load_in_4bit load failed against the mirror instead of warning. Gate right after the resolution block and point both probes at the gated value, then re-gate before dispatch for the later fast_inference_setup remap. Feeding the second call the first result keeps the warning to one. - On a PEFT load model_name is necessarily the base model, so the late gate warned "Ignoring revision" for every versioned adapter and told the caller to pass use_exact_model_name, which cannot stop an adapter resolving its base. Skip the late gate for PEFT; PeftModel.from_pretrained already loads the adapter with the caller's revision. - load_vllm takes no revision, so vLLM fetches the default branch. Pinning only the config and the tokenizer put two refs in one model, which is worse than the old behaviour of ignoring the revision outright. Drop the pin with a warning before the config load whenever vLLM owns the weights. - _hub_repo_or_local_path resolved a cached snapshot without the revision, so an offline or local_files_only tokenizer load silently got the default ref: a revision handed to from_pretrained cannot re-point a local directory. Thread it into _resolve_hub_repo_local_dir and both call sites. Five new tests, one per fix, all failing before it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep the revision when vLLM was requested but is unavailable The vLLM guard sat at the end of the same block that turns fast_inference off when vLLM is missing or the GPU is older than sm70. In that case the load falls through in-process and can honour the revision, but the guard dropped it anyway. Re-check fast_inference in the condition. * Keep the pin where the load can honour it, and tailor the warning Three more from review: - A num_labels load goes through AutoModelForSequenceClassification in-process no matter what fast_inference says, so the vLLM guard was discarding a revision the load could have used. Condition it on the same `fast_inference and num_labels is None` predicate the prefetch warm already uses. - use_exact_model_name only gates the mapper substitution. The ModelScope download, the ALLOW_PREQUANTIZED_MODELS strip and fast_inference_setup ignore it, so the warning was sending callers round the same loop. Record whether the mapper is what moved the name and only offer the remedy then. - The tokenizer does not always come from the base model's repo. Loading a PEFT repo with an explicit tokenizer_name pointing at the adapter dropped the pin for the tokenizer while PeftModel loaded the adapter from the requested ref, mixing two refs. _revision_for_tokenizer_repo now resolves it where the repos are known and both dispatches carry it, replacing the tokenizer_name == model_name guess in llama.py and vision.py. vision.py pops it from kwargs, since the weight load forwards **kwargs and transformers has no such argument. Seven new tests, all failing before this. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep the adapter ref off the base tokenizer, and pin both or neither Three more from review, all fallout from splitting tokenizer_revision out: - Skipping the late gate for PEFT leaves base_revision naming the adapter, and a remote PEFT load without an explicit tokenizer_name reads its tokenizer from the base repo, so that ref was handed to the wrong repository. Both dispatches now derive one model_revision and pass it to the base load and to the tokenizer resolution alike, so the base tokenizer can only ever get the base model's ref. - FastLlamaModel is exported, and the architecture wrappers forward `revision` through **kwargs without the new internal tokenizer_revision, so a direct call pinned the config and weights while the tokenizer read the default branch. Fall back to `revision` when the tokenizer repo is the model repo, before the warm so it does not fetch the wrong ref either. - The vLLM guard cleared only the model pin, leaving vLLM on the default branch with the tokenizer still on the requested ref. Clear both, in llama.py and in the parallel FastBaseModel block. Seven new tests. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep one ref per repo on the fp8, vLLM config and tokenizer paths Four ways a pin could still land on the wrong ref: - A plain load that names its own repo as tokenizer_name kept the caller's revision even after a remap had already dropped it off the config and weights, so mirror weights paired with a pinned tokenizer. Only a PEFT adapter is a genuinely separate repo, so only it keeps that ref now. - FastModel probes the config before dispatching and FastBaseModel skips its own load while that config is set, so the vLLM path received a config read at the pinned ref alongside the default-branch weights vLLM fetches. The probed config is now withheld there; a caller's own config still goes down. - The get_auto_processor fallback under AutoProcessor ran unpinned. - _offline_quantize_to_fp8 read the default branch and cached under a name that ignored the revision, so load_in_fp8 with a revision quantized the wrong ref and could reuse another ref's artifact. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Drop the vLLM pin before the probe, and key fp8 artifacts on the raw ref FastModel withheld the probed config from FastBaseModel on the vLLM path, but model_types, auto_model and the text-only decision had already been derived from it, so default-branch weights could load with pinned-ref dispatch. The drop now happens before the probe instead, using the same predicate FastBaseModel does, which makes that guard a no-op on this path and lets the config go down untouched again. FastLanguageModel keeps its drop inside llama.py: that one also turns fast_inference off on pre-Volta GPUs and for a num_labels load, and the loader cannot see either without duplicating the device checks, so gating early there would discard a pin llama.py would have honoured. The fp8 cache name sanitized the ref by replacing every unsafe character with the same one, so release/v1 and release.v1 shared a directory and the second load reused the first ref's artifact. A digest of the raw ref now rides along with the readable form. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Gate the language probe on vLLM too, spare the adapter probe, stamp the saved ref FastLanguageModel still probed the config at the pinned ref while llama.py dropped that same ref for its vLLM load, so model_types could pick the architecture class off one ref and load weights from another. It now drops the pin before the probe like FastModel does, through _vllm_will_load_weights in llama.py, which llama.py itself now calls: the language path also falls back in-process on pre-Volta GPUs and for a num_labels load, so the predicate has to live where those checks are rather than be guessed at by the loader. That drop runs before is_peft is known, and it was zeroing the ref the PeftConfig probe reads. An adapter is loaded in-process by peft, so it keeps the ref: adapter_revision holds the value from before the vLLM drop. Pinning the tokenizer also desynced the save path, which restores tokenizer.model from tokenizer.name_or_path and so had no idea which branch to read. The loaded ref is now stamped on the tokenizer the way local_files_only and cache_dir already are, and the sentencepiece probe, its memo key and the restore all use it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stamp the loaded ref on the vision processor as well FastBaseModel builds its processor without going through load_correct_tokenizer, so the stamp save.py reads was only being applied on the text path and a pinned FastVisionModel load still restored tokenizer.model from the default branch. Stamped at the return rather than at each of the processor branches, so the AutoTokenizer fallback that runs when patch_tokenizer raises cannot lose it either. * Tighten the revision forwarding comments * Keep the note on why a PEFT load pins nothing --------- Signed-off-by: majiayu000 <1835304752@qq.com> Co-authored-by: majiayu000 <1835304752@qq.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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
2 participants
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.
The bug
construct_chat_templatebuilds the HF Jinja template by concatenating the caller's template text straight into Jinja'...'string literals. Three places do this and none of them escape:So whatever the caller passes is read as Jinja source. A single quote closes the literal early, and a backslash is decoded as a Jinja escape.
Running against
mainat3f2fc5af, with a passing control in the same run so the harness is not what is failing:Line 3 is the one that worries me most.
default_system_message = r"Put the answer in \boxed{}."is an ordinary math-SFT system prompt. There is no error and no warning; Jinja just decodes\bto0x08, and every sample built byformatting_prompts_funcoverdataset.mapcarries a backspace character where\boxedwas meant to be.It is not only the system message.
process()handles the instruction and response sections too, so an apostrophe anywhere in the template breaks it. The last line above is"### User's turn: {INPUT}\n### Assistant: {OUTPUT}</s>"with an untoucheddefault_system_message.This is reachable through the public API:
apply_chat_template(in__all__) callsconstruct_chat_template, assigns the result totokenizer.chat_template, andformatting_prompts_functhen callstokenizer.apply_chat_templateon every row. That is exactly the traceback in issue #3667.Prior art, and how this relates to what is already open
This class of bug has been hit before and patched at the symptom each time:
expected token 'end of print statement', got 's'forget_chat_template("vicuna"). Merged PR Fix/issue 3667 vicuna template #5357 fixed it by hand-escaping the constant, andDEFAULT_SYSTEM_MESSAGE["vicuna"]onmainstill carries the literaluser\\'s(line 221, and again at 251 forvicuna_old).chat_templates.pyhunk is entirely inside_change_system_message(line 1667), which is theget_chat_templatepreset path. It never reachesconstruct_chat_template(line 2374), which is the custom-template path this PR fixes. It also escapes only', not\, so the\boxedcorruption above survives it. The two are complementary; happy to rebase onto it or fold this in if you would rather land one change.No open or closed PR covers
construct_chat_templateescaping. The four PRs that have touched this function (#6531, #6008, #7199, #5763) are about placeholder leaks, trailing whitespace, andloop_messagesbinding.The fix
Escape backslashes first, then single quotes, in each literal chunk before it is concatenated.
In
process()the text is split on the{INPUT}/{OUTPUT}/{SYSTEM}sentinel before escaping, so the' + message['content'] + 'markers thatprocess()inserts on the next line keep their quotes. That ordering is the whole trick; escaping after the substitution would break the concatenation instead.The
add_generation_promptliteral needs its own call rather than riding onprocess(): it re-slicesoutput_partindependently, so fixing onlyprocess()still leaves the template unparseable.After:
Deliberately out of scope: the Ollama modelfile splices
default_system_messageinto a double-quotedSYSTEM "..."line with the same absence of escaping. That is a different format with different rules, so I left it rather than guess at modelfile quoting in a PR about Jinja. Say the word if you want it in the same change.One thing to double check on review: the "Check if system part is the same!"
re.subfurther down matches the system literal in both arms with a\1backreference. Both arms now carry the same escaped string, so the backreference still matches and the collapse still fires; the 14 existing tests in the file cover that path and pass unchanged.Tests
Added
test_quotes_and_backslashes_survive_into_the_jinja_templateto the existingtests/python/test_construct_chat_template_validation.py, parametrized over an apostrophe,\boxed{}and a Windows path. It uses a template that also puts an apostrophe in the instruction and response sections, so it covers all three splice sites, and it renders once withadd_generation_prompt = Trueto pin the third one specifically._rendergrew anadd_generation_promptkeyword defaulting toFalse, so no existing caller changes.Ran with pytest against the real
chat_templates.py(onlytransformers.utils.loggingstubbed,ollama_template_mappersloaded for real), three runs:The 3 failures before are the new parametrizations, and the 14 pre-existing tests are unaffected by the change.
This file is picked up by
Repo tests (CPU)instudio-backend-ci.yml, which auto-discoverstests/and does not ignoretests/python/, so the new test runs in CI without a workflow change.Lint:
chat_templates.pyis inextend-excludeinpyproject.toml, so ruff does not cover it;ruff 0.15.12(the pinned version) is clean on the test file.codespellreports the same three pre-existingdatashits on the unmodified file, so nothing new.