fix(chat_templates): check find() return value before slicing on placeholders by Ricardo-M-L · Pull Request #5763 · unslothai/unsloth

Ricardo-M-L · 2026-05-25T09:44:34Z

Summary

Two places in construct_chat_template() use str.find() for sentinel placeholders ({INPUT} / {OUTPUT}) without checking the -1 return. Same shape as the try_fix_tokenizer fix that landed in #4923.

Bug 1 — except-block fallback (around line 2464)

except:
    ending = chat_template[chat_template.find("{OUTPUT}") + len("{OUTPUT}"):]

If chat_template has no {OUTPUT} marker, find() returns -1, the slice starts at offset 7 (-1 + len("{OUTPUT}")), and ending becomes garbage from the start of the template. That garbage is then re.escape-d and fed back into the template-recovery regex on the next line, which finds nothing — so the user sees a confusing IndexError on response_part = response_part[0] instead of the actual problem (missing placeholder).

Bug 2 — final trim before return (around line 2610-2611)

input_part  = input_part [:input_part .find("{INPUT}")]
output_part = output_part[:output_part.find("{OUTPUT}")]
return modelfile, jinja_template, input_part, output_part

If either placeholder is missing, find() returns -1, and s[:-1] silently drops the last character. The function returns a corrupted template prefix to the caller, who then trains on subtly wrong tokens.

Fix

Split each find() from its arithmetic / slice, then raise a clear RuntimeError naming the missing placeholder when the result is -1. Mirrors the guard pattern from #4923.

Test plan

python3 -c "import ast; ast.parse(open('unsloth/chat_templates.py').read())" — parses cleanly
Existing test_construct_chat_template() should still pass (it uses a well-formed template with both placeholders)
Manually: a template missing {OUTPUT} now raises RuntimeError("...must contain '{OUTPUT}' placeholder.") instead of IndexError: list index out of range

🤖 Generated with Claude Code

…eholders 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 unslothai#4923 (`try_fix_tokenizer`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

gemini-code-assist

Code Review

This pull request introduces validation for required placeholders in chat templates to prevent runtime errors during string processing. Specifically, it adds checks for the '{OUTPUT}' placeholder in construct_chat_template and both '{INPUT}' and '{OUTPUT}' in the process function, raising a RuntimeError if they are missing. The reviewer suggests expanding the check in construct_chat_template to include the '{INPUT}' placeholder to prevent a potential IndexError later in the logic. Additionally, the feedback recommends improving the error messages by including the actual template content and using user-friendly descriptions instead of internal variable names.

gemini-code-assist · 2026-05-25T09:50:40Z

+        output_pos = chat_template.find("{OUTPUT}")
+        if output_pos == -1:
+            raise RuntimeError(
+                "Unsloth: chat_template must contain a '{OUTPUT}' placeholder."
+            )
+        ending = chat_template[output_pos + len("{OUTPUT}"):]


While checking for {OUTPUT} prevents garbage slicing, the subsequent regex logic at line 2472 still relies on the presence of the {INPUT} placeholder. If {INPUT} is missing, re.findall will return an empty list, leading to an IndexError at line 2474. It is better to check for both placeholders here to provide a clear error message and avoid the confusing IndexError mentioned in the PR description. The error message has been updated to include the template value as per repository rules for dynamic user-facing messages.

Suggested change

output_pos = chat_template.find("{OUTPUT}")

if output_pos == -1:

raise RuntimeError(

"Unsloth: chat_template must contain a '{OUTPUT}' placeholder."

)

ending = chat_template[output_pos + len("{OUTPUT}"):]

output_idx = chat_template.find("{OUTPUT}")

input_idx = chat_template.find("{INPUT}")

if output_idx == -1 or input_idx == -1:

raise RuntimeError(

f"Unsloth: chat_template must contain both '{{INPUT}}' and '{{OUTPUT}}' placeholders. "

f"Template: {chat_template}"

)

ending = chat_template[output_idx + len("{OUTPUT}"):]

References

User-facing warning messages should be dynamically generated to include the specific configuration values they refer to, rather than using hardcoded examples, to ensure accuracy and avoid confusion.

gemini-code-assist · 2026-05-25T09:50:40Z

+    if input_idx == -1:
+        raise RuntimeError(
+            "Unsloth: input_part must contain '{INPUT}' placeholder."
+        )
+    if output_idx == -1:
+        raise RuntimeError(
+            "Unsloth: output_part must contain '{OUTPUT}' placeholder."
+        )


The error messages here use internal variable names (input_part, output_part) which might be confusing for users. Additionally, they are slightly inconsistent with the error message format used earlier in the function. Consider using more descriptive language that refers to the template sections and include the actual content of the sections to help with debugging, as per repository guidelines for dynamic user-facing messages.

Suggested change

if input_idx == -1:

raise RuntimeError(

"Unsloth: input_part must contain '{INPUT}' placeholder."

)

if output_idx == -1:

raise RuntimeError(

"Unsloth: output_part must contain '{OUTPUT}' placeholder."

)

if input_idx == -1:

raise RuntimeError(

f"Unsloth: The instruction section of the template must contain the '{{INPUT}}' placeholder. "

f"Section: {input_part}"

)

if output_idx == -1:

raise RuntimeError(

f"Unsloth: The response section of the template must contain the '{{OUTPUT}}' placeholder. "

f"Section: {output_part}"

)

References

User-facing warning messages should be dynamically generated to include the specific configuration values they refer to, rather than using hardcoded examples, to ensure accuracy and avoid confusion.

…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).

danielhanchen · 2026-05-25T11:56:58Z

Reviewed and pushed a follow-up commit (93bc533) extending the guards.

What it adds

In the except-block fallback: also validate {INPUT} (not just {OUTPUT}). Without this the very next line's regex "{INPUT}" + ending + "(.+?{OUTPUT}" + ending + ")" still returns an empty list when {INPUT} is missing, and response_part[0] still IndexErrors. (Acks gemini-code-assist's first suggestion, with a per-placeholder "missing" list so the message names exactly which one is absent.)
Same branch: guard the re.findall no-match case explicitly. Some templates contain both placeholders but not in a recoverable two-example shape, in which case the existing code still hits response_part[0].
Same branch: 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 before binding found, and found.group(1) raised AttributeError on the stale int from the outer rfind loop.
For the final-trim guards: rephrase the messages from internal names (input_part / output_part) to "instruction section" / "response section" and include a bounded 200-char repr excerpt of the offending content. (Acks gemini-code-assist's second suggestion, with the excerpt capped to keep tracebacks readable on large templates.)
New test file tests/python/test_construct_chat_template_validation.py covering each failure mode with a tiny fake tokenizer (no HF_TOKEN, no model download, CPU-only). Runs in the existing consolidated-tests-ci.yml Core matrix. 6/6 pass locally; the existing tests/test_gemma4_chat_template.py still passes; the inline test_construct_chat_template() happy path against meta-llama/Meta-Llama-3-8B-Instruct still produces an identical Jinja template.

Out of scope, deliberately not in this PR

Narrowing the bare except: around the primary parser. It catches everything including deliberate validation errors from the primary path, which routes legitimately-bad templates through fallback recovery they shouldn't get. Real concern, but the surface area is large and risks regressing templates the fallback currently rescues. Better as a separate change.
Moving placeholder validation earlier so the modelfile and Jinja are never built from a malformed input_part / output_part. The final-trim guards make this defensive rather than load-bearing, but reordering ~150 lines of the function is more invasive than the scope here.

for more information, see https://pre-commit.ci

danielhanchen · 2026-05-25T13:18:46Z

Thanks for the PR!

Five actionable findings from round 29 reviewer aggregate, plus an origin/main merge that absorbs the chat_templates.py fix landed in PR #5763. Skipped #4 / #5 (studio.txt + constraints.txt hub bump) because CI evidence from round 26 contradicts that suggestion; the real broken combo only happens via the --no-deps no-torch path which is already bumped in no-torch-runtime.txt + pyproject.toml. 1. core/inference/diffusion.py: round 28 reordered _release_chat_backend_for_diffusion BEFORE _release_other_gpu_owners_for_diffusion to surface the helper / advisor busy check early, but that meant the chat unload inside _release_chat_backend_for_diffusion now fired before the training / export conflict check in the second helper. A direct backend caller (tests, scripts) or a route-precheck race with a newly-started training run would then unload the user's chat and then 409 with nothing loaded. Split the helper busy check into _raise_if_helper_advisor_busy_for_diffusion (cheap, no side effects), keep _release_chat_backend_for_diffusion as the actual chat unload with an opt-out flag, and reorder load_model to: (a) helper check, (b) training / export check + idle export shutdown, (c) chat unload. All raises now fire BEFORE any destructive unload. 2. Merge origin/main: absorbs af6504f (PR #5763 chat_templates.py find() guards + the new tests/python/test_construct_chat_template_validation.py regression test). Removes the 101-line stale-rebase silent revert that round 29 reviewer 5 and 8 flagged. 3. frontend/src/features/images/images-page.tsx: supportsNegativePrompt now also honours customFamily when no model is loaded yet, so a Custom HF repo with family flux.2 / flux.2-klein correctly hides the negative prompt field instead of silently sending it. 4. routes/inference.py /images/generate: report the ACTUAL PNG width / height from PIL Image.size instead of echoing back the requested payload values. FLUX-family pipelines round to vae_scale_factor * 2, so a request for 520x520 lands as 512x512 internally; metadata now matches the bytes on the wire. Tests: 98 targeted (diffusion + cached_gguf + inference_validation) and frontend npm run typecheck pass locally.

…eholders (unslothai#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 unslothai#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>

Ricardo-M-L requested review from danielhanchen and rolandtannous as code owners May 25, 2026 09:44

gemini-code-assist Bot reviewed May 25, 2026

View reviewed changes

[pre-commit.ci] auto fixes from pre-commit.com hooks

24f1d4b

for more information, see https://pre-commit.ci

danielhanchen merged commit af6504f into unslothai:main May 25, 2026
39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(chat_templates): check find() return value before slicing on placeholders#5763

fix(chat_templates): check find() return value before slicing on placeholders#5763
danielhanchen merged 3 commits into
unslothai:mainfrom
Ricardo-M-L:contrib/fix-find-sentinel-checks

Ricardo-M-L commented May 25, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot May 25, 2026

Uh oh!

gemini-code-assist Bot May 25, 2026

Uh oh!

danielhanchen commented May 25, 2026

Uh oh!

danielhanchen commented May 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Ricardo-M-L commented May 25, 2026

Summary

Bug 1 — except-block fallback (around line 2464)

Bug 2 — final trim before return (around line 2610-2611)

Fix

Test plan

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 25, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 25, 2026

Choose a reason for hiding this comment

Uh oh!

danielhanchen commented May 25, 2026

Uh oh!

danielhanchen commented May 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants