Fix review findings for chat-template repair (#5049)#5056
Conversation
1. Sandbox fallback Jinja env in _VariantTokenizerProxy.apply_chat_template
(use SandboxedEnvironment, matching _derive_assistant_prefix_by_render)
2. Unwrap benign outer-If guards in _template_ends_with_toplevel_for so
templates like {% if messages %}{% for ... %}{% endfor %}{% endif %}
are still repairable (preserves Qwen3-Guard rejection via else-branch
and add_generation_prompt-name checks)
3. Preserve raw name_or_path in _VariantTokenizerProxy._source_path so
local-path detection works for dict/list variant tokenizers
4. Context-aware strict-mode messages: omit "will still load" and
"Set UNSLOTH_STRICT_CHAT_TEMPLATE=1" when already raising
for more information, see https://pre-commit.ci
![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: d736a12bc4
ℹ️ 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".
| return None | ||
|
|
||
| node = _last_structural(ast.body) | ||
| while isinstance(node, jinja2.nodes.If) and not node.else_: |
There was a problem hiding this comment.
elif wrappers as structural in top-level loop gate
_template_ends_with_toplevel_for now unwraps outer If nodes whenever else_ is empty, but it does not check elif_. In Jinja, an if/elif/.../endif can have no else_ while still having alternate branches, so templates like {% if cond %}{% for ... %}{% endfor %}{% elif other %}...{% endif %} are incorrectly treated as “benign” and considered repairable. That lets _fix_chat_template inject an add_generation_prompt block after the outer endif, changing behavior for non-primary branches and potentially producing invalid prompts for those models.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the chat template repair logic in unsloth/tokenizer_utils.py. Key changes include refactoring _template_ends_with_toplevel_for to correctly identify repairable templates wrapped in simple if guards and updating _format_chat_template_message to provide more detailed feedback when templates are missing or have ineffective add_generation_prompt blocks. Additionally, the code now better tracks the source path of tokenizers for more accurate error reporting. I have no feedback to provide as there were no review comments to evaluate.
1 participant
Summary
Follow-up fixes for #5049 (merged as c5be8b1), addressing 4 issues found during code review:
Sandbox fallback in
_VariantTokenizerProxy: The read-only tokenizer fallback path inapply_chat_templateused a plainjinja2.Environment. Changed toSandboxedEnvironmentto match the security posture of_derive_assistant_prefix_by_render, which already uses sandboxing for the same model-load-time rendering context.Benign outer-If unwrapping in
_template_ends_with_toplevel_for: Templates wrapped in harmless guards like{% if messages %}{% for ... %}{% endfor %}{% endif %}were rejected by the top-level-For check, preventing repair. Now unwraps outer-If nodes that have noelsebranch and don't testadd_generation_prompt, while still rejecting structural wrappers like Qwen3-Guard.Local-path diagnostics for dict/list variants:
_VariantTokenizerProxyappended a variant label toname_or_path, breakingos.path.isdir()in_name_is_local_path. Added_source_pathto preserve the raw path for local-path detection, passed vialocal_path_sourceto_format_chat_template_message.Strict-mode message consistency: When
UNSLOTH_STRICT_CHAT_TEMPLATE=1is set and aRuntimeErroris raised, the message no longer says "The model will still load" or "Set UNSLOTH_STRICT_CHAT_TEMPLATE=1 to raise instead of warn."Test plan