-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(deepseek): update reasoner model for V3.2 tool call support #3815
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
- Remove 'tools' from reasoner unsupported params since DeepSeek V3.2 reasoner now supports tool calls with thinking mode - Fix bug where copy.deepcopy overwrote the filtered config for reasoner models, making param filtering ineffective - Add reasoning_content injection/extraction for tool call continuations (required by DeepSeek API for multi-turn tool calls in thinking mode) - Fix typo in constant name (REASONSER -> REASONER) - Update documentation URL to point to current thinking_mode guide Closes camel-ai#3811
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
bytecii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, left some comments
| # Store the last reasoning_content from model response. | ||
| # For DeepSeek reasoner models with tool calls, the | ||
| # reasoning_content must be passed back in subsequent requests. | ||
| self._last_reasoning_content: Optional[str] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also add a TODO to improve the reasoning content storage
also nit:
| self._last_reasoning_content: Optional[str] = None | |
| self._last_reasoning_content: str | None = None |
|
|
||
| def _inject_reasoning_content( | ||
| self, | ||
| messages: List[OpenAIMessage], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| messages: List[OpenAIMessage], | |
| messages: list[OpenAIMessage], |
| in subsequent requests for proper context management. | ||
|
|
||
| Args: | ||
| messages: The original messages list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| messages: The original messages list. | |
| messages (OpenAIMessage): The original messages list. |
| if ( | ||
| not reasoning_injected | ||
| and isinstance(msg, dict) | ||
| and msg.get("role") == "assistant" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any enum for the assistant we can use here?
| reasoning_injected = False | ||
|
|
||
| for msg in reversed(messages): | ||
| if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can separate these checkings to make it clearer?
is_reasoning = (not reasoning_injected) and ("reasoning_content" not in msg)
is_tool_call = msg.get("tool_calls") is not None
is_assistant = msg.get("role") == "assistant"
if is_reasoning and is_tool_call and is_assistant:
...
| and "reasoning_content" not in msg | ||
| ): | ||
| new_msg = dict(msg) | ||
| new_msg["reasoning_content"] = self._last_reasoning_content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be not None?
| processed.append(msg) | ||
|
|
||
| if reasoning_injected: | ||
| self._last_reasoning_content = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move it below the reasoning_injected = True?
| r"""Extract reasoning_content from the model response. | ||
|
|
||
| Args: | ||
| response: The model response. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| response: The model response. | |
| response (ChatCompletion): The model response. |
|
|
||
| def _extract_reasoning_content( | ||
| self, response: ChatCompletion | ||
| ) -> Optional[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ) -> Optional[str]: | |
| ) -> str | None: |
| tools: Optional[List[Dict[str, Any]]] = None, | ||
| ) -> Dict[str, Any]: | ||
| request_config = self.model_config_dict.copy() | ||
| import copy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to the top
Summary
Update the DeepSeek model implementation to support the latest DeepSeek V3.2 reasoner features, including tool call support in thinking mode.
Changes
Bug Fix
copy.deepcopy(self.model_config_dict)was overwriting the filtered config for reasoner models, making the parameter filtering completely ineffective.Feature Updates
'tools'fromREASONER_UNSUPPORTED_PARAMSsince DeepSeek V3.2 reasoner now supports tool calls with thinking mode (docs).reasoning_contentinjection/extraction: For multi-turn tool calls in thinking mode, the DeepSeek API requiresreasoning_contentfrom the model response to be passed back in subsequent requests. This follows the same pattern already used byVolcanoModel.Minor
REASONSER→REASONER)Tests
Added tests for:
reasoning_contentinjection into assistant messages with tool callsreasoning_contentis storedCloses #3811