Render a static prefix when default_system_message is None - #8117
wasimysaid merged 3 commits into
Conversation
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 unslothai#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.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c5b5ae480
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
2 participants
Summary
A chat template can begin with a static preamble that has no
{SYSTEM}placeholder, for example Unsloth's own Alpaca-style header. Whendefault_system_messageisNone, that preamble never appears in any rendered conversation, while the Ollama modelfile returned by the same call still contains it.Repro
The preamble has no
{SYSTEM}slot, so whether the caller passed a default system message should not decide whether it is part of the prompt. Today it does.Cause
The generated template puts the prefix inside the system arm only:
With no
{SYSTEM}slot that arm cannot be reached. A caller-supplied system message is not consumed by the system part, so it reaches the loop and hitsraise_exception('Only user and assistant roles are supported!'), whichtest_static_prefix_without_system_still_rejects_system_messagealready pins. So the only conversations that render successfully are the ones that take the{% endif %}path, and those drop the prefix.The effect is train/serve skew rather than an error:
apply_chat_templatebuilds training text from the Jinja template throughdataset.map, whilesave.py:create_ollama_modelfileserves the modelfile, so the model is finetuned without the text it is later prompted with. Nothing warns.Fix
Give that branch the same
{% else %}arm thedefault_system_message is not Nonepath already gets. With no{SYSTEM}in the system part the two arms are identical literals, so the existing "Check if system part is the same!" regex a few lines down folds them into a single unconditional emit:This deliberately keeps #7199's behaviour: a caller-supplied system message still reaches
raise_exception, because the collapsed template loops overmessagesand the template genuinely cannot render one. The change is only about what a plain user/assistant conversation renders.Blast radius
Generated
jinja_templateandmodelfilecompared before and after across 21 combinations (7 system-part shapes, including none,{SYSTEM}-bearing, apostrophe-bearing and BOS-prefixed, times 3default_system_messagevalues):default_system_message=None, which is the bugTest
test_static_prefix_without_system_renders_in_every_conversationintests/python/test_construct_chat_template_validation.py, parametrized overdefault_system_message, asserting the prefix is in the rendered conversation and in the modelfile from the same call. It uses the file's existing_NO_SYSTEM_CHAT_TEMPLATEand CPU-only_SuccessFakeTokenizer, so it needs no GPU or download.Fails before on the
Nonecase, passes after. The whole file goes 20 passed / 1 failed to 21 passed, andtests/python/test_get_chat_template_escaping.pyalongside it stays green (193 passed together).ruff checkclean on both changed files.