Skip to content

fix(chat_templates): bind loop_messages when default_system_message is None - #7199

Merged
danielhanchen merged 2 commits into
unslothai:mainfrom
chuenchen309:fix/construct-chat-template-none-system
Jul 19, 2026
Merged

danielhanchen merged 2 commits into
unslothai:mainfrom
chuenchen309:fix/construct-chat-template-none-system

Conversation

@chuenchen309

@chuenchen309 chuenchen309 commented Jul 17, 2026 •

Copy link
Copy Markdown
Contributor

Problem

construct_chat_template(default_system_message = None) produces a template that raises as soon as the caller passes a system message:

Only user and assistant roles are supported!

The same template built with a default_system_message handles that input fine, so the failure depends on a parameter that is only meant to supply a fallback.

Cause

The system part binds loop_messages only inside the {% if %} arm (chat_templates.py:2640):

partial_system = \
    "{% if messages[0]['role'] == 'system' %}"\
        "{{ " + partial_system + " }}"\
        "{% set loop_messages = messages[1:] %}"
if default_system_message is not None:
    ...
    partial_system += "{% else %}"\
        "{{ '" + full_system + "' }}"\
        "{% set loop_messages = messages %}"\
    "{% endif %}"
else:
    partial_system += "{% endif %}"          # no else arm

Then, 8 lines below:

# Fix missing loop_messages
if "{% set loop_messages = messages %}" not in jinja_template:
    jinja_template = jinja_template.replace(
        "{% for message in loop_messages %}",
        "{% for message in messages %}",
        1,
    )

That step exists for templates with no {SYSTEM} part, which never bind loop_messages at all. But it tests for the unconditional binding. In the None path loop_messages is bound — at :2640, to messages[1:] — just not unconditionally. So the test passes, the loop is rewritten to messages, and the messages[1:] skip is undone. The system message reaches the loop and hits raise_exception.

loop_messages isn't missing here; only the {% else %} arm is.

Fix

Add the {% else %} arm, mirroring the is not None branch minus the default text. loop_messages is then always bound, and the rewrite no longer fires because the unconditional binding is present.

How I tested this

Rendered the generated template for all four combinations, same chat_template, same inputs:

default_system_message input before after
None system message raise_exception 'Be terse.\n### User: Hi\n'
None no system message '### User: Hi\n' unchanged
'You are helpful.' system message 'Be terse.\n### User: Hi\n' unchanged
'You are helpful.' no system message 'You are helpful.\n### User: Hi\n' unchanged

Only the broken row changes, and it lands on the same output the is not None row already produced — which is right, since a caller-supplied system message makes the default irrelevant.

Checked separately that the Fix missing loop_messages step still fires for a template with no {SYSTEM} part, i.e. its actual purpose: identical render before and after.

Tests added to tests/python/test_construct_chat_template_validation.py, which already had the CPU-only fake tokenizer. Red before the fix (only the [None] parametrization; the ['You are helpful.'] one passes either way), green after. tests/python/: 305 → 308 passed, identical single pre-existing failure (test_negative_control_no_tokenizers), --ignore on test_unsloth_run_tool_policy_resolver.py which needs pydantic (not installed here, unrelated to this change). ruff check clean; scripts/run_ruff_format.py applied to the test file — chat_templates.py is in the hook's exclude list, so it is left formatted as-is.

Notes

I found this by rendering the produced Jinja rather than reading the builder — my first two readings of the mechanism were both wrong, and only the actual template output showed that the loop had been rewritten.


Disclosure: this contribution is fully AI-authored and autonomous (Claude Code, acting on this account). An AI found the bug, ran the repro, wrote the tests, and wrote this description; the human account holder reviews every change and is accountable for it. The verification above is real and re-runnable from the diff — it was just done by an AI, not a person. If unsupervised AI contributions aren't what you want here, say the word and I'll close this.

…s 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>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request fixes an issue in the chat template generation where loop_messages was not being bound when default_system_message was set to None. It updates the Jinja template generation logic in unsloth/chat_templates.py to include an {% else %} block that binds loop_messages = messages, and adds corresponding unit tests to validate this behavior. No review comments were provided, so there is no feedback to address.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

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.
@danielhanchen

Copy link
Copy Markdown
Member

Thanks for the fix. I reproduced the bug and confirmed the fix through the real apply_chat_template path: on main a {SYSTEM} template with default_system_message=None and a caller system message raises TemplateError: Only user and assistant roles are supported! (checked with Qwen2.5-0.5B and TinyLlama), and with your change it renders correctly with a single BOS. The Ollama modelfile is byte identical, and a late or second system message still correctly raises.

I pushed one adjustment on top (11c7af3):

  1. Scoped the new else arm to templates that actually contain {SYSTEM}. The unconditional binding was also firing for a template with a static prefix but no {SYSTEM} placeholder, where system_part is still non-empty. In that case a caller system message was silently dropped instead of raising, which diverged from the default_system_message set path (which still raises). Guarding on "{SYSTEM}" in system_part keeps your fix for the intended case and preserves the raise for prefixes that cannot represent a system message.

  2. Strengthened the tests: assert the default does not leak when a caller system message is present, and added a regression test for the static prefix case.

After this, the only behavior difference versus main is the intended {SYSTEM} plus None fix. tests/python/test_construct_chat_template_validation.py passes (12 tests) and ruff is clean.

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 11c7af355c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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
danielhanchen merged commit b307823 into unslothai:main Jul 19, 2026
43 of 47 checks passed
@danielhanchen

Copy link
Copy Markdown
Member

Thanks!

VectorCipher pushed a commit to VectorCipher/unsloth that referenced this pull request Jul 20, 2026
…s None (unslothai#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 unslothai#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>
wasimysaid added a commit that referenced this pull request Aug 8, 2026
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants