Conversation
…s present resolve_thinking_mode() forced mode='thinking' when tools were present, ignoring an explicit enable_thinking=False from the caller. This wasted generation budget on reasoning tokens for clients that explicitly requested no thinking while using tool calling. An explicit disable now takes precedence over the tools-presumed-thinking heuristic. Closes FlashML-org#474
There was a problem hiding this comment.
🟢 Approval recommended
The remaining findings are minor style nits and do not block approval.
Pull request overview
Merges the upstream fix so explicit thinking disable flags override tool-triggered thinking mode.
Changes:
- Prioritizes
enable_thinking=Falseandthinking=False. - Adds regression tests for conflicting flags and tool presence.
File summaries
| File | Description | Findings |
|---|---|---|
tests/tokenizer/test_resolve_thinking_mode.py |
Adds resolver regression tests. | Nit (2 votes): replace the Unicode em dash with an ASCII hyphen. |
python/freetoken/tokenizer/tokenize.py |
Applies explicit disable precedence. | Nit (2 votes): replace the Unicode em dash with an ASCII hyphen. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| when tools are offered (dsv4 only emits well-formed tool calls in thinking | ||
| mode) or when the caller requests it via ``chat_template_kwargs``. | ||
| An explicit ``enable_thinking=False`` (or ``thinking=False``) from the | ||
| caller always wins, even when tools are present — the caller knows their |
|
|
||
| def test_explicit_disable_overrides_tools(): | ||
| """An explicit enable_thinking=False (or thinking=False) must win even | ||
| when tools are present — the caller knows their intent better than the |
gdevenyi
force-pushed
the
5080/03-developer-role-391
branch
from
September 18, 2026 03:06
f4b6ebe to
925a217
Compare
gdevenyi
force-pushed
the
5080/04-thinking-tools-475
branch
from
September 18, 2026 03:06
805e347 to
fb4f839
Compare
gdevenyi
force-pushed
the
5080/03-developer-role-391
branch
from
September 19, 2026 21:31
925a217 to
d4a2701
Compare
gdevenyi
force-pushed
the
5080/04-thinking-tools-475
branch
from
September 19, 2026 21:31
fb4f839 to
4eaec8c
Compare
gdevenyi
force-pushed
the
5080/03-developer-role-391
branch
from
September 23, 2026 23:35
d4a2701 to
8ff853c
Compare
gdevenyi
force-pushed
the
5080/04-thinking-tools-475
branch
from
September 23, 2026 23:36
4eaec8c to
9591b11
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Merges upstream FlashML-org#475 (
fix(tokenizer): respect explicit enable_thinking=false even when tools present, head5a212bea, for FlashML-org#474). Stacked on #11.Changes
enable_thinking=False(orthinking=False) now resolves to chat mode before the "tools present -> thinking" rule inresolve_thinking_moderuns.Testing
Tested together with the previous PR (FlashML-org#391) at 805e347.
Unit tests:
pytest tests/tokenizer tests/server -m "not slow": 852 passed, 4 skipped.Live (one
get_weathertool, "What is the weather in Paris? Use the tool."):chat_template_kwargs: {enable_thinking: false}:reasoning_contentalready empty, tool call{"city": "Paris"}. The bug fix(tokenizer): respect explicit enable_thinking=false even when tools present FlashML-org/FreeToken#475 fixes does not show on this model's chat template, so this merge can't be confirmed as a fix here.enable_thinking: false:reasoning_contentempty, content empty, tool call{"city": "Paris"}.chat_template_kwargs:reasoning_content151 characters, tool call{"city": "Paris"}, so default thinking still works.No regression; the fix itself is unverified on this model.
Environment:
nvidia/Qwen3.8-Flash-Next-NVFP4Stack: the
5080deploybranch ismaine0886cc plus upstream PRs merged one at a time for a single-user, full-context (262,144-token) deployment on this card. Each PR in the stack is based on the previous one, so its diff is exactly one merge step. Merge them in order, or merge the top of the stack alone.How tests were run: with the server stopped, since a running server holds almost all VRAM and GPU tests fail spuriously. Benchmarks are streamed chat completions with 256 tokens generated with
ignore_eos, one request at a time. Prompts are salted so the radix cache cannot hit.Rebased 2026-09-17 onto upstream main
cac247a(v0.1.3). The whole stack was rebuilt merge by merge on the new base (previous basee0886cc); the merge resolutions were replayed unchanged viagit rerere, and the rebuilt tip differs from the old one by exactly thee0886cc..cac247afile set. This PR's head is nowfb4f839.Rebased 2026-09-19 onto upstream main
cc1f5c2(4 commits pastcac247a: FlashML-org#471 greedy sampling in mixed batches, FlashML-org#518 WeightLoadError, FlashML-org#521 tvm-ffi jit arch, FlashML-org#524 install index). Same replay as before viagit rerere; the replayed stack differs from the previous tip8adde91by exactly thecac247a..cc1f5c2file set. This PR's head is now4eaec8c.🤖 Generated with Claude Code
https://claude.ai/code/session_01Bu6LgoxLR4wETqb7RPR2vt