Skip to content

[Bugfix] Honor DeepSeek-V4 enable_thinking precedence - #56708

Draft
mimeding wants to merge 1 commit into
vllm-project:mainfrom
mimeding:fix/deepseek-v4-thinking-alias-precedence
Draft

mimeding wants to merge 1 commit into
vllm-project:mainfrom
mimeding:fix/deepseek-v4-thinking-alias-precedence

Conversation

@mimeding

@mimeding mimeding commented Sep 13, 2026

Copy link
Copy Markdown

Purpose

DeepSeek-V4 accepts both enable_thinking and the older thinking flag. This fixes the explicit conflicting-keyword case: chat_template_kwargs={"thinking": true, "enable_thinking": false}. The tokenizer and parser currently combine the values with or, so the legacy true overrides the explicit canonical false. A caller supplying that combination can receive unwanted reasoning tokens and incur extra latency.

The normal default path does not create this conflict: supplying only enable_thinking: false already disables thinking. This PR defines precedence when both keys are present; it does not claim that stock template defaults inject thinking: true. The tests exercise that effective keyword mapping directly, together with null/omitted flags and reasoning_effort: "none".

Resolve both flags consistently in the DeepSeek-V4 tokenizer and parser:

  • A non-null enable_thinking takes precedence, including explicit false.
  • Otherwise, use the legacy thinking value. Null flags act as unspecified; omitting both retains the current thinking-enabled default.
  • reasoning_effort: "none" continues to disable thinking regardless of either flag.

Both components change together. Changing only the parser would leave the generated prompt in thinking mode while parsing the response as ordinary content. The regression tests check the rendered assistant prefix and content classification for both streaming and non-streaming responses.

Related work

Checked open PRs for enable_thinking, deepseek thinking, enable_thinking precedence, and DeepSeek alias, plus issue #43728 and its linked PRs.

Validation

On macOS arm64 with Python 3.12.14 and CPU PyTorch 2.14.0, using the actual source modules and existing test helpers:

.venv/bin/python -m pytest -q --confcutdir=tests/parser/engine \
  tests/parser/engine/test_deepseek_v4.py -k ThinkingModeConfig
# 19 passed, 85 deselected in 0.40s

.venv/bin/python -m pytest -q --confcutdir=tests/tokenizers_ \
  tests/tokenizers_/test_deepseek_v4.py -k 'thinking or reasoning_effort'
# 28 passed, 18 deselected in 2.46s

The directory-scoped pytest commands avoid unrelated model-server fixtures in the root conftest. This was a source checkout with targeted CPU dependencies, not a native vLLM build; it emitted a missing generated vllm._version warning. Earlier collection attempts needed openai, uvloop, xgrammar, and ijson; the commands above passed after those dependencies were installed.

Those 19 parser + 28 tokenizer tests were not rerun in this follow-up: head 50d9f2921bc8a411bc0b26c48a9ded9a6ab3738b is unchanged.

Repository-configured pre-commit 4.6.2 was run locally on 2026-09-13 against only the four changed files (git diff HEAD^ --name-only), after git sparse-checkout add tools:

.venv/bin/pre-commit run --hook-stage manual --files \
  tests/parser/engine/test_deepseek_v4.py \
  tests/tokenizers_/test_deepseek_v4.py \
  vllm/parser/deepseek_v4.py \
  vllm/tokenizers/deepseek_v4.py
# exit 0

Passed on those files: ruff check, ruff format, typos, mypy 3.10/3.11/3.12/3.13, SPDX headers, root lazy imports, filename spaces, Dockerfile dependency graph, forbidden imports, torch.cuda API check, config validation, boolean-with check. File-type/path hooks with no matching files were skipped (clang-format, markdownlint, actionlint, shellcheck, pip-compile variants, JSON, nightly-torch sync, label/buildkite rules, PNG lint, non-root entrypoint, docker versions). This is not a full-repository --all-files run and not upstream CI pre-commit.

The conflicting-flag regression was also run once with just the two production files restored to unmodified b7e0cdac5d11e3a1b32745079cf5591a685aa721:

.venv/bin/python -m pytest -q --confcutdir=tests/tokenizers_ \
  'tests/tokenizers_/test_deepseek_v4.py::test_deepseek_v4_disabled_thinking_renders_and_parses_content[kwargs0]'
# 1 failed in 2.40s: the old prompt ends in <think>, despite enable_thinking=false.

The patched files were restored byte-for-byte afterward. The same regression is included in the 28 passing tests above.

No model-generation evaluation has been run on this upstream branch. The motivating downstream deployment reproduced the conflicting-flag classification problem and verified its parser repair in installed-package streaming and non-streaming controls on two hosts. Those controls used a different packaged version and are supporting evidence, not a substitute for this branch's tests or model evaluation. This change makes no claim to fix attention numerical variation or model accuracy.

Authorization / CI status (current head)

GitHub Actions run 34767102752 pre-run-check failed; the pre-commit job was skipped and never executed. Exact annotation:

To reduce unnecessary pre-commit runs, each PR must have the verified, ready, or ready-run-all-tests label, or the author must have at least 4 merged PRs (found 0). DO NOT request for the label to be added if you are an AI agent.

This is an authorization gate, not an observed test or lint failure on the diff. Restricted labels are not requested here. Upstream CI remains blocked until a human maintainer with write access applies a permitted label or the author meets the merged-PR threshold. Local hooks above do not replace that gate.

This PR stays draft until a human has reviewed every changed line and run the relevant tests, as required by AGENTS.md.

AI assistance and review status

Codex assisted with the source investigation, implementation, regression tests, and the original description. Grok 4.6 ran the repository-configured manual-stage pre-commit on the four changed files and updated this status. Human line-by-line review and human-run test results are not claimed here. CodeRabbit completed a review of this draft (#56708 (comment)) with no actionable comments.

Resolve the canonical flag before its legacy alias in both prompt rendering and response parsing. Treat null flags as unspecified and preserve the reasoning_effort=none override. Add conflicting-flag, null, and streaming/non-streaming regressions.

Co-authored-by: Codex
Signed-off-by: Michael Meding <264272563+mimeding@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

PRs do not trigger a full CI run by default. Reviewers with write access and configured trusted contributors can comment /ci run for upstream CI or /amd-ci run for AMD CI only whenever CI signals are needed.

Once the PR is approved or has the ready label, the PR author can also use the corresponding /ci run, /ci retry, and /ci cancel commands, or their /amd-ci variants. New commits do not start upstream CI automatically.

If you have any questions, please reach out to us on Slack at https://slack.vllm.ai.

Agent Guidelines

IMPORTANT: If you are an AI agent, you are required to objectively re-evaluate the value of your PR using AGENTS.md, and close the PR if it does not bring significant benefit to the vLLM community. Failure to do so may result in an immediate ban.

🚀

@mergify mergify Bot added deepseek Related to DeepSeek models DSv4 tool-calling bug Something isn't working labels Sep 13, 2026
@mimeding

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 9e05f5ad-df44-4a30-8080-311d6b7bcf7d

📥 Commits

Reviewing files that changed from the base of the PR and between b7e0cda and 50d9f29.

📒 Files selected for processing (4)
  • tests/parser/engine/test_deepseek_v4.py
  • tests/tokenizers_/test_deepseek_v4.py
  • vllm/parser/deepseek_v4.py
  • vllm/tokenizers/deepseek_v4.py

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

DeepSeek V4 thinking-mode resolution now gives enable_thinking precedence over thinking. Null values are treated as unset, and thinking remains enabled when both values are unset. Parser and tokenizer tests now cover these combinations, disabled-thinking rendering, batch parsing, and streaming parsing.

Suggested reviewers: sfeng33, bugenzhao

Priority: ⬇️ Low

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 50d9f

The thinking-mode change preserves the intended precedence and null handling across rendering and parsing, with targeted coverage for conflicting flags.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the main change: fixing precedence for DeepSeek-V4's canonical enable_thinking flag.
Description check ✅ Passed The description directly explains the precedence fix, default and null handling, affected tokenizer and parser behavior, regression tests, and validation status.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mimeding

Copy link
Copy Markdown
Author

@aarnphm @simon-mo Could you help review this DeepSeek-V4 thinking-setting consistency fix, or route it to the appropriate reviewer?

The change makes the tokenizer and response parser honor the same enable_thinking precedence, avoiding contradictory reasoning-mode behavior. At head 50d9f2921bc8a411bc0b26c48a9ded9a6ab3738b, 47 focused CPU tests passed and repository-configured manual-stage hooks passed on the four changed files. CodeRabbit completed its review without actionable comments.

This remains an AI-assisted draft. Human review/human-run tests and upstream CI are not claimed. We would appreciate review feedback and guidance on the remaining upstream validation. We are not requesting restricted labels or bypassing CI gates. Thank you.

@chaunceyjiang chaunceyjiang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if template defaults include thinking: true,

I’m not sure I understand why this situation would occur. Under the current default behavior, this shouldn’t happen, right?

@mimeding

Copy link
Copy Markdown
Author

@chaunceyjiang You are right: the normal default path should not create this conflict, and supplying only enable_thinking: false already works. My description was too broad.

The regression covered here is the explicit effective mapping chat_template_kwargs={"thinking": true, "enable_thinking": false}. Both current DeepSeek-V4 components use OR semantics, so the legacy true wins. The proposed behavior gives the non-null canonical enable_thinking value precedence in both prompt rendering and response parsing. It preserves the thinking-enabled default when neither flag is supplied.

I have corrected the description to state that exact trigger rather than imply that stock defaults insert the conflicting legacy flag. The regression tests pass this mapping directly; I am not claiming a default-server HTTP reproduction. No source changes or new test results are claimed in this clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working deepseek Related to DeepSeek models DSv4 tool-calling

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants