Fix Qwen3Renderer stripping out last <think>, and Qwen3DisableThinkingRenderer not adding <think>\n\n</think> #178
+241
−4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… format The previous implementation only added `<think>\n` to assistant messages via the parent class, but the official Qwen3-8B tokenizer format requires the complete empty thinking block: `<think>\n\n</think>\n\n`. This commit fixes the issue by: 1. Overriding render_message() to prepend the complete empty thinking block to assistant messages that don't already have one 2. Delegating to the parent class for all rendering logic 3. Adding test cases to verify the fix matches official tokenizer behavior Fixes: thinking-machines-lab#176
thejaminator
commented
Dec 15, 2025
| self.strip_thinking_from_history | ||
| and message["role"] == "assistant" | ||
| and "</think>" in ac_content | ||
| and not is_last |
Author
There was a problem hiding this comment.
fix for Qwen3Renderer: Don't remove last think block content
issue:
2-TURN CONVERSATION - THINKING SHOULD BE PRESERVED:
================================================================================
TINKER:
<|im_start|>user
What is 2+2?<|im_end|>
<|im_start|>assistant
The answer is 4.<|im_end|>
HUGGINGFACE:
<|im_start|>user
What is 2+2?<|im_end|>
<|im_start|>assistant
<think> <---- Huggingface tokenizer template preserves <think>
Let me calculate this.
</think>
The answer is 4.<|im_end|```
thejaminator
commented
Dec 15, 2025
| if "<think>" not in content: | ||
| message = message.copy() | ||
| message["content"] = "<think>\n\n</think>\n\n" + content | ||
| return super().render_message(idx, message, is_last=is_last) |
Author
There was a problem hiding this comment.
for Qwen3DisableThinkingRenderer, it did not add <think>\n\n</think>\n\n during SFT.
================================================================================
BUG: Official tinker-cookbook Qwen3DisableThinkingRenderer
================================================================================
Actual output from renderer:
<|im_start|>user
What is 2+2?<|im_end|>
<|im_start|>assistant
<think> <---- Missing the \n\n</think> tokens
The answer is 4.<|im_end|>
================================================================================
Expected output from Qwen3-8B tokenizer:
================================================================================
<|im_start|>user
What is 2+2?<|im_end|>
<|im_start|>assistant
<think>
</think>
The answer is 4.<|im_end|>
Collaborator
|
Thanks for looking into this -- I agree that there's a bug. Could you check (and add a test) that |
thejaminator
commented
Dec 15, 2025
| elif message["role"] == "assistant" and "<think>" not in ac_content and is_last: | ||
| # Matching the paper, we force the assistant to start with <think>. Some SFT datasets include | ||
| # <think> in the assistant messages, we so don't need to re-add it in those cases. | ||
| ob_str += "<think>\n" |
Author
There was a problem hiding this comment.
the renderer test caught another bug:
in multiturn, <think> was added to the intermediate steps
Cookbook string: <|im_start|>user
Hello, how are you?<|im_end|>
<|im_start|>assistant
<think> <--- THIS SHOULDN'T GET ADDED
I'm fine, thank you!<|im_end|>
<|im_start|>user
What is the capital of France?<|im_end|>
<|im_start|>assistant
<think>
thejaminator
commented
Dec 15, 2025
| # XXX this causes inefficiency in RL, because the observations don't grow by appending to the end. | ||
| # Maybe we should just insert this empty thinking block in every message? | ||
| prefill = "<think>\n\n</think>\n\n" + (prefill or "") | ||
| return super().build_generation_prompt(messages, role, prefill) |
Author
There was a problem hiding this comment.
oops, the new generation test caught a bug. we still need to prefill like this
thejaminator
commented
Dec 15, 2025
| f"HF tokens: {hf_tokens}\n" | ||
| f"HF string: {tokenizer.decode(hf_tokens)}" | ||
| ) | ||
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Suggested fix for #177 and #176