7 commits

Author	SHA1	Message	Date
Vineeth Sai Varikuntla	3177523091	Render a static prefix when default_system_message is None (#8117) ... * Render a static prefix when default_system_message is None A chat template can start with a preamble that has no {SYSTEM} placeholder. construct_chat_template emitted it only from the {% if messages[0]['role'] == 'system' %} arm, and with no {SYSTEM} slot that arm is unreachable: a caller system message hits the loop's raise_exception instead. So the preamble rendered in no successful conversation at all. The Ollama modelfile returned by the same call still contains it, so training text built from the Jinja template lacked the prefix the served model was later prompted with: trained on : '### User: Hi\n### Assistant: Yo</s>' served : 'Below are some instructions that describe some tasks.\n\n...' Give that branch the same {% else %} arm the default_system_message path gets. Both arms are then identical literals, so the existing 'system part is the same' regex folds them into one unconditional emit, and a caller-supplied system message still reaches raise_exception, which is what #7199 intended. Only templates with a static prefix and default_system_message=None change: across 21 combinations of system part and default message, the other 18 produce byte-identical Jinja and every modelfile is unchanged. * Shorten static preamble comments * Keep BOS-only prefixes role-strict --------- Co-authored-by: Wasim Yousef Said <wasimysdev@gmail.com>	2026-08-08 14:57:17 +02:00
Vineeth Sai Varikuntla	90f5170d0a	Escape caller template text spliced into Jinja string literals (#7731) ... * Escape caller template text spliced into Jinja string literals 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. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Escape \r and strip the BOS before escaping in construct_chat_template 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. * Tighten the comments around the Jinja literal escaping * Trim the escaping comments further --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <danielhanchen@gmail.com>	2026-08-02 05:32:25 -07:00
Vineeth Sai Varikuntla	2eae08cc28	Make auto-appended EOS deterministic in chat templates (#7702) ... --------- Co-authored-by: oobabooga <112222186+oobabooga@users.noreply.github.com>	2026-08-01 00:53:07 -03:00
Andrew Chen	b307823b1d	fix(chat_templates): bind loop_messages when default_system_message is None (#7199) ... * fix(chat_templates): bind loop_messages when default_system_message is None construct_chat_template(default_system_message=None) built a system part that binds loop_messages only inside the `{% if messages[0]['role'] == 'system' %}` arm. The `Fix missing loop_messages` step right below then found no unconditional `{% set loop_messages = messages %}`, concluded loop_messages was missing, and rewrote `{% for message in loop_messages %}` back to `{% for message in messages %}` -- undoing the `messages[1:]` skip. A caller-supplied system message therefore reached the loop and tripped raise_exception: Only user and assistant roles are supported! Add the `{% else %}` arm so loop_messages is always bound, mirroring the default_system_message is not None branch minus the default text. That also stops the rewrite from firing, since the unconditional binding is now present. Renders before / after, same template, same inputs: default_system_message input before after None system msg raise_exception 'Be terse.\n### User: Hi\n' None no system '### User: Hi\n' unchanged 'You are helpful.' system msg 'Be terse.\n### User: Hi\n' unchanged 'You are helpful.' no system 'You are helpful.\n...' unchanged The rewrite still fires for templates with no {SYSTEM} part, which is what it was there for -- verified unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Scope loop_messages binding to {SYSTEM} templates for PR #7199 The None branch now only adds the else arm when system_part contains {SYSTEM}, so a static prefix with no {SYSTEM} placeholder keeps raising on a caller system message instead of silently dropping it. Strengthen the tests: assert the default does not leak when a caller system message is present, and add a regression test for the static prefix case. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: danielhanchen <danielhanchen@gmail.com>	2026-07-19 06:33:48 -07:00
Vineeth Sai	d42256a5c5	Fix construct_chat_template leaking {INPUT}/{OUTPUT} sentinel into the chat template (#6531) ... * Fix construct_chat_template leaking {INPUT}/{OUTPUT} sentinel into the template In construct_chat_template's inner process() helper, the branch handling a section that starts with the {INPUT}/{OUTPUT} sentinel sliced the part from part.find(which) (which is 0 in that branch), so the literal sentinel was re-included in the generated Jinja chat template. The endswith branch already slices correctly with part[:part.find(which)]; this slices past the sentinel with part[len(which):], so a template whose input or output section begins with the sentinel (for example a user turn that starts with {INPUT}) renders correctly instead of emitting a literal {INPUT}/{OUTPUT}. Added a regression test covering {INPUT}-leading and {OUTPUT}-leading sections. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-06-24 00:40:53 -07:00
Daniel Han	a6dc10dad2	Reduce and tighten comments and docstrings across the test suite (#6429) ... * Reduce and tighten comments and docstrings in tests Shorten verbose comments and docstrings across the test suite without changing any test logic. Remove narration that restates the next line, collapse long module and test docstrings to a single line, and drop banner separators. Keep regression context (issue and PR references, run ids), skip reasons, mocking and timing rationale, license headers, lint and type directives, and commented-out code. Comments and docstrings only: an AST signature check confirms no code, assertions, or string literals changed, and the suite byte-compiles cleanly. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-06-18 01:07:09 -07:00
Ricardo-M-L	af6504f900	fix(chat_templates): check find() return value before slicing on placeholders (#5763) ... * fix(chat_templates): check find() return value before slicing on placeholders Two places in `construct_chat_template()` use `str.find()` for sentinel placeholders (`{INPUT}` / `{OUTPUT}`) without checking the -1 return: 1. The `except:` fallback (around line 2464) computes `chat_template[chat_template.find("{OUTPUT}") + len("{OUTPUT}"):]`. If the template has no `{OUTPUT}` marker, `find()` returns -1 and the slice starts at offset 7 (`-1 + len("{OUTPUT}")`), producing garbage that's then `re.escape`-d and fed back into the template-recovery regex. The user sees a confusing `IndexError` on `response_part = response_part[0]` instead of the real problem. 2. The final trim before returning (`input_part[:input_part.find("{INPUT}")]` and the matching `{OUTPUT}` line) silently drops the last character when the placeholder is missing — `find()` returns -1, and `[:-1]` slices everything except the last character, returning a corrupted template prefix to the caller. Replace both with an explicit `-1` check that raises a clear `RuntimeError` naming the missing placeholder, matching the existing guard pattern from #4923 (`try_fix_tokenizer`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(chat_templates): also guard {INPUT} and fallback regex/separator paths Builds on the {OUTPUT} / final-trim guards in this branch by closing the three remaining ways the except-block fallback in construct_chat_template() can still raise a confusing IndexError or AttributeError on malformed templates: 1. Validate both {INPUT} and {OUTPUT} before deriving `ending`. The regex two lines later (`{INPUT} + ending + ...`) still produced an empty list and crashed on `response_part[0]` if {INPUT} was missing. 2. Guard the regex no-match case. Some templates contain both placeholders but not in a recoverable two-example shape, in which case `re.findall` returns an empty list and `[0]` raises. 3. Initialize `found = None` before the separator-search loop and raise if the loop never sets it. Previously, if the first iteration's `re.finditer` was empty the loop broke without binding `found`, and `found.group(1)` raised AttributeError on the stale int left over from the outer rfind loop. Rephrase the final-trim error messages from internal variable names ("input_part") to user-facing wording ("instruction section") and include a bounded (200-char) excerpt of the offending content so the error is debuggable without being unbounded. Add tests/python/test_construct_chat_template_validation.py covering each failure mode with a fake tokenizer (no HF_TOKEN, no model download, CPU-only). * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-05-25 06:19:01 -07:00