fix(ds4): correct reasoning effort prefix mapping (high unreachable, max rendered high) - #231
fix(ds4): correct reasoning effort prefix mapping (high unreachable, max rendered high)#231ktsaou wants to merge 2 commits into
Conversation
…max rendered high)
REASONING_EFFORT_MAX held the HIGH prefix text from the checkpoint's
reference encoder (encoding/encoding_dsv4.py) and a prefix was injected
only for effort == 'max'. As a result high rendered with no prefix at
all (identical to none/low) and max silently rendered the high prompt
("Absolute maximum with no shortcuts permitted...") instead of the
true max prompt ("Beyond maximum - exhaustive, relentless, and
uncompromising..."), so every request ran one effort level below the
requested one.
Rename the existing constant to REASONING_EFFORT_HIGH, add the true
REASONING_EFFORT_MAX text verbatim from encoding/encoding_dsv4.py in
deepseek-ai/DeepSeek-V4-Flash-0731, and inject for both high and max.
Verified live via /tokenize on a running server: high=84 tokens ending
"...no assumption is left unchecked", max=97 tokens ending "...no
error remains undiscovered". Behavior for none/chat and unset effort
is unchanged. The low -> high mapping in tokenizers/deepseek_v4.py is
intentional and untouched.
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in PRs do not trigger a full CI run by default. Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. Agent GuidelinesIMPORTANT: 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. 🚀 |
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe DeepSeek V4 tokenizer adds a high-effort reasoning prompt, revises the maximum-effort prompt, and selects the correct prompt for ChangesDeepSeek reasoning effort
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vllm/tokenizers/deepseek_v4_encoding.py (1)
274-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the effort-selection comment.
The branch now handles both
"high"and"max", but the preceding comment still mentions only max effort. Update it to describe both modes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/tokenizers/deepseek_v4_encoding.py` around lines 274 - 275, Update the comment immediately preceding the effort-selection branch to describe both "high" and "max" reasoning-effort modes, matching the conditions handled by the existing thinking_mode and reasoning_effort logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vllm/tokenizers/deepseek_v4_encoding.py`:
- Around line 70-81: Wrap the long string literals in REASONING_EFFORT_HIGH and
REASONING_EFFORT_MAX using adjacent string segments so every line stays within
88 characters. Preserve the rendered prompt text exactly, including spaces,
punctuation, newline escapes, and paragraph spacing.
---
Nitpick comments:
In `@vllm/tokenizers/deepseek_v4_encoding.py`:
- Around line 274-275: Update the comment immediately preceding the
effort-selection branch to describe both "high" and "max" reasoning-effort
modes, matching the conditions handled by the existing thinking_mode and
reasoning_effort logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 797b1517-eed6-4907-866f-a06754dc3c1c
📒 Files selected for processing (1)
vllm/tokenizers/deepseek_v4_encoding.py
| REASONING_EFFORT_HIGH = ( | ||
| "Reasoning Effort: Absolute maximum with no shortcuts permitted.\n" | ||
| "You MUST be very thorough in your thinking and comprehensively decompose the problem to resolve the root cause, rigorously stress-testing your logic against all potential paths, edge cases, and adversarial scenarios.\n" | ||
| "Explicitly write out your entire deliberation process, documenting every intermediate step, considered alternative, and rejected hypothesis to ensure absolutely no assumption is left unchecked.\n\n" | ||
| ) | ||
|
|
||
| REASONING_EFFORT_MAX = ( | ||
| "Reasoning Effort: Beyond maximum \u2014 exhaustive, relentless, and uncompromising.\n" | ||
| "You MUST reason with the utmost depth and rigor, leaving absolutely nothing to chance: exhaustively decompose the problem into its most fundamental components, trace every causal chain to its root, and resolve the underlying cause rather than any surface symptom.\n" | ||
| "Do not stop reasoning until you have independently verified the solution from multiple angles and are certain that no assumption remains unchecked and no error remains undiscovered.\n\n" | ||
| ) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap the long prompt literals.
Lines 72, 73, 78, and 79 exceed the 88-character limit. Split the literals with adjacent strings. Preserve all spaces and escape sequences so the rendered prompts do not change.
Proposed fix
- "You MUST be very thorough in your thinking and comprehensively decompose the problem to resolve the root cause, rigorously stress-testing your logic against all potential paths, edge cases, and adversarial scenarios.\n"
- "Explicitly write out your entire deliberation process, documenting every intermediate step, considered alternative, and rejected hypothesis to ensure absolutely no assumption is left unchecked.\n\n"
+ "You MUST be very thorough in your thinking and comprehensively decompose the "
+ "problem to resolve the root cause, rigorously stress-testing your logic "
+ "against all potential paths, edge cases, and adversarial scenarios.\n"
+ "Explicitly write out your entire deliberation process, documenting every "
+ "intermediate step, considered alternative, and rejected hypothesis to ensure "
+ "absolutely no assumption is left unchecked.\n\n"
...
- "You MUST reason with the utmost depth and rigor, leaving absolutely nothing to chance: exhaustively decompose the problem into its most fundamental components, trace every causal chain to its root, and resolve the underlying cause rather than any surface symptom.\n"
- "Do not stop reasoning until you have independently verified the solution from multiple angles and are certain that no assumption remains unchecked and no error remains undiscovered.\n\n"
+ "You MUST reason with the utmost depth and rigor, leaving absolutely nothing to "
+ "chance: exhaustively decompose the problem into its most fundamental components, "
+ "trace every causal chain to its root, and resolve the underlying cause rather "
+ "than any surface symptom.\n"
+ "Do not stop reasoning until you have independently verified the solution from "
+ "multiple angles and are certain that no assumption remains unchecked and no "
+ "error remains undiscovered.\n\n"As per coding guidelines, Python code must follow an 88-character line length limit.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| REASONING_EFFORT_HIGH = ( | |
| "Reasoning Effort: Absolute maximum with no shortcuts permitted.\n" | |
| "You MUST be very thorough in your thinking and comprehensively decompose the problem to resolve the root cause, rigorously stress-testing your logic against all potential paths, edge cases, and adversarial scenarios.\n" | |
| "Explicitly write out your entire deliberation process, documenting every intermediate step, considered alternative, and rejected hypothesis to ensure absolutely no assumption is left unchecked.\n\n" | |
| ) | |
| REASONING_EFFORT_MAX = ( | |
| "Reasoning Effort: Beyond maximum \u2014 exhaustive, relentless, and uncompromising.\n" | |
| "You MUST reason with the utmost depth and rigor, leaving absolutely nothing to chance: exhaustively decompose the problem into its most fundamental components, trace every causal chain to its root, and resolve the underlying cause rather than any surface symptom.\n" | |
| "Do not stop reasoning until you have independently verified the solution from multiple angles and are certain that no assumption remains unchecked and no error remains undiscovered.\n\n" | |
| ) | |
| REASONING_EFFORT_HIGH = ( | |
| "Reasoning Effort: Absolute maximum with no shortcuts permitted.\n" | |
| "You MUST be very thorough in your thinking and comprehensively decompose the " | |
| "problem to resolve the root cause, rigorously stress-testing your logic " | |
| "against all potential paths, edge cases, and adversarial scenarios.\n" | |
| "Explicitly write out your entire deliberation process, documenting every " | |
| "intermediate step, considered alternative, and rejected hypothesis to ensure " | |
| "absolutely no assumption is left unchecked.\n\n" | |
| ) | |
| REASONING_EFFORT_MAX = ( | |
| "Reasoning Effort: Beyond maximum \u2014 exhaustive, relentless, and uncompromising.\n" | |
| "You MUST reason with the utmost depth and rigor, leaving absolutely nothing to " | |
| "chance: exhaustively decompose the problem into its most fundamental components, " | |
| "trace every causal chain to its root, and resolve the underlying cause rather " | |
| "than any surface symptom.\n" | |
| "Do not stop reasoning until you have independently verified the solution from " | |
| "multiple angles and are certain that no assumption remains unchecked and no " | |
| "error remains undiscovered.\n\n" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vllm/tokenizers/deepseek_v4_encoding.py` around lines 70 - 81, Wrap the long
string literals in REASONING_EFFORT_HIGH and REASONING_EFFORT_MAX using adjacent
string segments so every line stays within 88 characters. Preserve the rendered
prompt text exactly, including spaces, punctuation, newline escapes, and
paragraph spacing.
Source: Coding guidelines
|
@coderabbitai addressed in aa2f68e — the comment now covers both high and max effort injection. |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
The reasoning-prefix bug reported here is valid. A GG-complete successor is now available in #235. #235 retains the corrected official high/max prompts and additionally covers the official low tier, Python/Rust renderer parity, message-level system tools, request-level system/tool ordering, and golden tests against the 0731 encoder. I am leaving this PR open for its author/maintainers rather than closing it automatically. |
|
Thanks for confirming and for the quick turnaround. Closing this in favor of #235, which covers everything here plus the low tier, Rust parity, ordering, and golden tests. For anyone landing here later: the same bug class exists in stock vLLM's |
…registry Port of the production CKV prefetch layer-cache poisoning fix (702d7fe in the overlay tree) onto this branch, adapted to this branch's formats. The registration site inside the CKV gather path registered ``kv_cache``, which for KVarN MLA prefill is the FP8 staging view returned by _stage_kvarn_mla_fp8_cache. That workspace is deliberately shared across ALL MLA layers (one dense arena keyed by device/pages/geometry), while the record it holds is materialized from the registering layer's own paged slice. Registering it therefore poisons every ``layer_caches`` entry: the next gather-eligible request's side-stream prefetches then gather all prefetched layers from that one tensor, feeding every prefetched layer a previous layer's KV (in production this was measured as an identical wrong byte checksum for every layer's chunk-0 gather from a single kv pointer, corrupting attention for every layer >= 1). The production fix registers the real per-layer paged cache; that tensor is directly gatherable there because production carries a native KVarN CKV gather over the packed paged record. This branch's _dcp_gather_ckv only accepts the 656-byte staged layout, so the faithful minimal fix is to keep KVarN MLA out of the registry entirely: prefetched layers get no registry entry, the target chain stops, and every gather stays on the synchronous per-layer path (the same path the first eligible request already takes). Non-KVarN formats still register their own per-layer paged view, which is unchanged and correct. The full registration fix (register the per-layer paged cache plus the native packed-record gather) lands with the b12x production backend (PR local-inference-lab#231). Signed-off-by: João Sequeira <email.sequeira@gmail.com>
…registry Port of the production CKV prefetch layer-cache poisoning fix (702d7fe in the overlay tree) onto this branch, adapted to this branch's formats. The registration site inside the CKV gather path registered ``kv_cache``, which for KVarN MLA prefill is the FP8 staging view returned by _stage_kvarn_mla_fp8_cache. That workspace is deliberately shared across ALL MLA layers (one dense arena keyed by device/pages/geometry), while the record it holds is materialized from the registering layer's own paged slice. Registering it therefore poisons every ``layer_caches`` entry: the next gather-eligible request's side-stream prefetches then gather all prefetched layers from that one tensor, feeding every prefetched layer a previous layer's KV (in production this was measured as an identical wrong byte checksum for every layer's chunk-0 gather from a single kv pointer, corrupting attention for every layer >= 1). The production fix registers the real per-layer paged cache; that tensor is directly gatherable there because production carries a native KVarN CKV gather over the packed paged record. This branch's _dcp_gather_ckv only accepts the 656-byte staged layout, so the faithful minimal fix is to keep KVarN MLA out of the registry entirely: prefetched layers get no registry entry, the target chain stops, and every gather stays on the synchronous per-layer path (the same path the first eligible request already takes). Non-KVarN formats still register their own per-layer paged view, which is unchanged and correct. The full registration fix (register the per-layer paged cache plus the native packed-record gather) lands with the b12x production backend (PR local-inference-lab#231). Signed-off-by: João Sequeira <email.sequeira@gmail.com>
2 participants
Problem
REASONING_EFFORT_MAXinvllm/tokenizers/deepseek_v4_encoding.pyholds the HIGH prefix text from the checkpoint's reference encoder (encoding/encoding_dsv4.py), and a prefix is injected only whenreasoning_effort == 'max'. Result on current main:reasoning_effort=high→ no prefix at all (renders identically to unset/low)reasoning_effort=max→ renders the HIGH prefix ("Absolute maximum with no shortcuts permitted…") instead of the true max prefix ("Beyond maximum — exhaustive, relentless, and uncompromising…")So every request runs one effort level below what was requested. Agentic workloads asking for
maxhave actually been gettinghighsince the 0731 checkpoint introduced the third effort level.Measured via
/tokenizeon a live server (r16 image line):low=5, high=5, max=84tokens. Expected per the reference encoder:low=empty,high=84,max≈97.Independently reported against stock vLLM: https://huggingface.co/deepseek-ai/DeepSeek-V4-Flash-0731/discussions/26 (byte-identical comparison against
encoding/encoding_dsv4.py; notes the same high/max finding in stocktokenizer_mode=deepseek_v4, so upstream vLLM likely wants the equivalent fix).Fix
REASONING_EFFORT_HIGH(its text is the reference encoder'shighprompt verbatim).REASONING_EFFORT_MAXwith the reference encoder's truemaxprompt, verbatim fromencoding/encoding_dsv4.pyindeepseek-ai/DeepSeek-V4-Flash-0731.highandmax.Validation
Patched file bind-mounted over a running production server:
reasoning_effort=high→ 84 tokens, ends "…no assumption is left unchecked." ✓reasoning_effort=max→ 97 tokens, ends "…no error remains undiscovered." ✓high) — unchanged ✓none→ chat mode — unchanged ✓Note:
tokenizers/deepseek_v4.pymapslow→highvia its else-branch. That mapping looks intentional (the fork exposes none/high/max) and is left untouched, but flagging it: the reference encoder has a distinctlowtier (thinking mode, empty prefix) that the fork currently doesn't expose.Summary by CodeRabbit
New Features
Bug Fixes