[BugFix][Frontend] Align DeepSeek V4 frontend behavior - #14632
QwertyJack wants to merge 4 commits into
Conversation
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. Tip 💡 Consider Linking a Related Issue or RFCYour PR title contains the [BugFix] tag, indicating a bug fix or new feature. Linking a related issue or RFC in the PR description is strongly encouraged — it gives reviewers helpful context and speeds up the review. You can use any of these keywords:
🙏 Thanks for helping us keep the project well-organized! |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request ports essential DeepSeek V4 frontend compatibility patches from the v0.25 release branch to the main branch. These changes address discrepancies in tool placement, thinking mode defaults, and tool argument streaming efficiency. The patches are designed to be temporary and include conditional logic to automatically disable themselves once the corresponding upstream vLLM improvements are integrated. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Suggested PR Title:
[platform][Feature] Add patches for DeepSeek V4 frontend and tool streamingSuggested PR Summary:
### What this PR does / why we need it?
This PR introduces patches for DeepSeek V4 in `vllm_ascend` to align behavior with the DeepSeek V4 tokenizer default and checkpoint renderer, and to enable incremental streaming of long string parameters during tool calls. Specifically:
1. **Frontend Patch (`patch_deepseek_v4_frontend.py`)**: Corrects tool attachment behavior by attaching tools to a shallow copy of the first system message (or inserting one if absent) and defaults the parser to thinking mode when thinking controls are omitted.
2. **Tool Streaming Patch (`patch_deepseek_v4_tool_streaming.py`)**: Allows direct streaming of plain body deltas for string parameters instead of buffering until `</parameter>` arrives.
Feedback on the implementation:
- In `patch_deepseek_v4_frontend.py`, wrapping `apply_chat_template` on every call to `get_deepseek_v4_tokenizer` can cause redundant wrapping and potential recursion errors. It should be guarded to run only once.
- The patch for `DeepSeekV4Parser.__init__` assumes `chat_template_kwargs` is always passed as a keyword argument, which can raise a `TypeError` if passed positionally.
### Does this PR introduce _any_ user-facing change?
Yes, it changes how tools are attached to system messages and defaults the parser to thinking mode for DeepSeek V4. It also improves streaming responsiveness for tool call arguments.
### How was this patch tested?
New unit tests have been added in `tests/ut/patch/platform/test_deepseek_v4_thinking.py` and `tests/ut/patch/platform/test_deepseek_v4_tool_streaming.py` to verify the patched tokenizer, parser thinking mode, and tool streaming behavior.| def _patched_get_deepseek_v4_tokenizer(tokenizer: deepseek_v4.HfTokenizer): | ||
| dsv4_tokenizer = _original_get_deepseek_v4_tokenizer(tokenizer) | ||
| tokenizer_cls = type(dsv4_tokenizer) | ||
| original_apply_chat_template = tokenizer_cls.apply_chat_template | ||
|
|
||
| @wraps(original_apply_chat_template) | ||
| def apply_chat_template( | ||
| self, | ||
| messages, | ||
| tools=None, | ||
| **kwargs, | ||
| ): | ||
| if tools: | ||
| conversation = kwargs.get("conversation", messages).copy() | ||
| system_index = next( | ||
| (index for index, message in enumerate(conversation) if message.get("role") == "system"), | ||
| None, | ||
| ) | ||
| if system_index is None: | ||
| conversation.insert(0, {"role": "system", "tools": tools}) | ||
| else: | ||
| system_message = conversation[system_index].copy() | ||
| system_message["tools"] = tools | ||
| conversation[system_index] = system_message | ||
| kwargs["conversation"] = conversation | ||
| tools = None | ||
| return original_apply_chat_template(self, messages, tools=tools, **kwargs) | ||
|
|
||
| tokenizer_cls.apply_chat_template = apply_chat_template | ||
| return dsv4_tokenizer |
There was a problem hiding this comment.
The current implementation of _patched_get_deepseek_v4_tokenizer wraps tokenizer_cls.apply_chat_template every time get_deepseek_v4_tokenizer is called. This leads to redundant wrapping, performance degradation, and potential RecursionError over time. Additionally, if conversation is passed as a keyword argument or if the first argument of apply_chat_template is named conversation instead of messages, calling original_apply_chat_template with both messages positionally and conversation in kwargs will raise a TypeError due to multiple values for the same argument.
We should check if the class has already been patched and safely handle both messages and conversation argument bindings.
def _patched_get_deepseek_v4_tokenizer(tokenizer: deepseek_v4.HfTokenizer):
dsv4_tokenizer = _original_get_deepseek_v4_tokenizer(tokenizer)
tokenizer_cls = type(dsv4_tokenizer)
if not getattr(tokenizer_cls, "_patched_apply_chat_template", False):
original_apply_chat_template = tokenizer_cls.apply_chat_template
@wraps(original_apply_chat_template)
def apply_chat_template(
self,
messages,
tools=None,
**kwargs,
):
if tools:
conversation = kwargs.get("conversation", messages).copy()
system_index = next(
(index for index, message in enumerate(conversation) if message.get("role") == "system"),
None,
)
if system_index is None:
conversation.insert(0, {"role": "system", "tools": tools})
else:
system_message = conversation[system_index].copy()
system_message["tools"] = tools
conversation[system_index] = system_message
if "conversation" in kwargs:
kwargs["conversation"] = conversation
else:
messages = conversation
tools = None
return original_apply_chat_template(self, messages, tools=tools, **kwargs)
tokenizer_cls.apply_chat_template = apply_chat_template
setattr(tokenizer_cls, "_patched_apply_chat_template", True)
return dsv4_tokenizer| def _patched_deepseek_v4_parser_init( | ||
| self: DeepSeekV4Parser, | ||
| *args: Any, | ||
| **kwargs: Any, | ||
| ) -> None: | ||
| chat_kwargs = kwargs.get("chat_template_kwargs") or {} | ||
| if "thinking" not in chat_kwargs and "enable_thinking" not in chat_kwargs: | ||
| chat_kwargs = dict(chat_kwargs) | ||
| chat_kwargs["enable_thinking"] = True | ||
| kwargs["chat_template_kwargs"] = chat_kwargs | ||
|
|
||
| _original_deepseek_v4_parser_init(self, *args, **kwargs) |
There was a problem hiding this comment.
The _patched_deepseek_v4_parser_init function assumes chat_template_kwargs is always passed as a keyword argument. If it is passed positionally (e.g., DeepSeekV4Parser(tokenizer, chat_template_kwargs)), kwargs.get("chat_template_kwargs") will be None, and setting kwargs["chat_template_kwargs"] will result in a TypeError: __init__() got multiple values for keyword argument 'chat_template_kwargs' when calling the original __init__.
We should robustly handle both positional and keyword arguments for chat_template_kwargs.
def _patched_deepseek_v4_parser_init(
self: DeepSeekV4Parser,
*args: Any,
**kwargs: Any,
) -> None:
chat_kwargs = None
passed_positionally = False
if len(args) >= 2:
chat_kwargs = args[1]
passed_positionally = True
else:
chat_kwargs = kwargs.get("chat_template_kwargs")
if chat_kwargs is None:
chat_kwargs = {}
if "thinking" not in chat_kwargs and "enable_thinking" not in chat_kwargs:
chat_kwargs = dict(chat_kwargs)
chat_kwargs["enable_thinking"] = True
if passed_positionally:
args_list = list(args)
args_list[1] = chat_kwargs
args = tuple(args_list)
else:
kwargs["chat_template_kwargs"] = chat_kwargs
_original_deepseek_v4_parser_init(self, *args, **kwargs)
6b1774b to
f5daf03
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
f5daf03 to
0dce027
Compare
|
/rerun |
|
/rerun Rerun (failed jobs only):
|
0dce027 to
e633dfb
Compare
|
/rerun Rerun (failed jobs only):
|
1 similar comment
|
/rerun Rerun (failed jobs only):
|
3a1060f to
332db9f
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
1 similar comment
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
) ## What this PR does / why we need it? Backport PR #14632 to `releases/v0.27.1rc`. This aligns DeepSeek V4 frontend rendering, reasoning effort handling, compatibility behavior, incremental tool argument streaming, and related unit tests. ## Does this PR introduce any user-facing change? Yes. DeepSeek V4 chat template rendering and long tool argument streaming are corrected on the v0.27 release line. ## How was this patch tested? - Cherry-picked all four commits from PR #14632. - Added and updated DeepSeek V4 frontend unit tests. - Resolved release-branch patch registration conflicts. - vLLM main: vllm-project/vllm@ba07e4a --------- Signed-off-by: QwertyJack <7554089+QwertyJack@users.noreply.github.com> Co-authored-by: QwertyJack <7554089+QwertyJack@users.noreply.github.com>
Port the release parser default, system/tools rendering, and incremental long string argument streaming behavior to main. Signed-off-by: QwertyJack <7554089+QwertyJack@users.noreply.github.com>
Keep checkpoint-specific reasoning effort behavior and isolate tokenizer wrappers per instance while preserving legacy frontend compatibility. Signed-off-by: QwertyJack <7554089+QwertyJack@users.noreply.github.com>
Remove the pickle round-trip test rejected by the forbidden-import check and keep the streaming test tokenizer compatible with the current scanner API. Signed-off-by: QwertyJack <7554089+QwertyJack@users.noreply.github.com>
Signed-off-by: QwertyJack <7554089+QwertyJack@users.noreply.github.com>
332db9f to
6ed3917
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
2 participants
What this PR does / why we need it?
Ports the remaining DeepSeek V4 frontend behavior from the v0.25 release branch to
mainas temporary compatibility patches:The upstream status does not yet make these patches removable from the supported vLLM matrix:
58d3918e3predates the fix, so the compatibility wrapper remains necessary for the supported matrix and is behaviorally idempotent after the pin advances;The streaming fast path is installed only for the affected upstream contract,
arg_structural_chars=frozenset('>'). It automatically skips installation after upstream removes that filter and fails fast on an unknown non-null contract.Does this PR introduce any user-facing change?
Yes. DeepSeek V4 requests preserve their system prompt when tools are present, omitted thinking controls no longer leak reasoning/tool markup into content, and long string tool arguments stream incrementally rather than being buffered until parameter close.
How was this patch tested?
Tested against both vLLM sources supported by the current
mainbranch:The latest live vLLM
mainstill has the affected tools-placement and structural-filter contracts. Its full vLLM-Ascend pytest import is currently blocked earlier by the unrelated removal ofvllm.model_executor.layers.attention.pcp; the current pinned main and release-tag paths above are fully tested.