-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix: use chat_template_kwargs for DeepSeek V3.1 Terminus reasoning control #9081
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: main
Are you sure you want to change the base?
Conversation
…ntrol - Add chat_template_kwargs support to OpenRouterChatCompletionParams type - Convert reasoning configuration to chat_template_kwargs for DeepSeek V3.1 Terminus models - Set thinking parameter based on reasoning enabled state (not excluded) - Add comprehensive tests for the new behavior - Ensures reasoning can be properly disabled (default OFF) for DeepSeek V3.1 Terminus Fixes #8270
Review complete - no new issues found. The merge commit (4eb98a7) integrates main branch changes without introducing any DeepSeek V3.1 Terminus-specific regressions. Previous feedback has been properly addressed. Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
src/api/providers/openrouter.ts
Outdated
|
|
||
| const transforms = (this.options.openRouterUseMiddleOutTransform ?? true) ? ["middle-out"] : undefined | ||
|
|
||
| // Special handling for DeepSeek V3.1 Terminus models |
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.
Consider extracting the DeepSeek-specific logic (converting reasoning to chat_template_kwargs) into a helper function to reduce duplication (appears in both createMessage and completePrompt).
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.
@roomote sould we really do that ?
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.
Fixaroo
See task on Roo Cloud
Extracted the duplicated DeepSeek V3.1 Terminus logic into a helper method. All tests and checks passed.
Extract duplicated DeepSeek V3.1 Terminus specific logic from createMessage and completePrompt methods into a reusable handleDeepSeekV31Terminus helper method. This improves code maintainability by following the DRY principle.
bozoweed
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.
@roomote you have to investigate if needed and apply or not as you want
src/api/providers/openrouter.ts
Outdated
| chatTemplateKwargs: { thinking?: boolean } | undefined | ||
| finalReasoning: OpenRouterReasoningParams | undefined | ||
| } { | ||
| if (!modelId.startsWith("deepseek/deepseek-v3.1-terminus")) { |
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.
modelId.startsWith("deepseek/deepseek-v3.1-terminus") can be made more efficience by using modelId.include("deepseek-v3.1-terminus") because of distilled version or something like that
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.
@roomote can you look at my rievew and validate or invalidate it ?
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.
Fixaroo
See task on Roo Cloud
Changed startsWith() to includes() to support DeepSeek V3.1 Terminus model variants (including :exacto). All tests pass.
|
@roomote resolve conflicts of that pr appeared from latest change in target branch |
Merge conflicts resolved successfully. All local tests and checks passed. |
Resolved merge conflict by combining: - DeepSeek V3.1 Terminus chat_template_kwargs support from PR - Tool metadata support from main branch
This PR addresses Issue #8270 based on @bozoweed's suggestion.
Problem
When using DeepSeek V3.1 Terminus via OpenRouter, reasoning/thinking appears to be enabled by default with no way to turn it off. This leads to worse answers for some tasks and increases token usage.
Solution
Following @bozoweed's insight, this PR implements the
chat_template_kwargsapproach with thethinkingparameter for DeepSeek V3.1 Terminus models:chat_template_kwargsin the OpenRouter handlerchat_template_kwargs: { thinking: boolean }reasoningparameter for these models (replaced bychat_template_kwargs)thinking: falsewhen no reasoning is configuredChanges
Testing
thinking: truewhen reasoning is enabledthinking: falsewhen reasoning is disabled (default)chat_template_kwargsfor non-Terminus modelsFixes #8270
Thanks to @bozoweed for the solution!
Important
This PR implements
chat_template_kwargsfor DeepSeek V3.1 Terminus models to control reasoning, with tests added to verify the new behavior.chat_template_kwargsfor DeepSeek V3.1 Terminus models inopenrouter.ts.chat_template_kwargs: { thinking: boolean }.thinking: falsewhen no reasoning is configured.openrouter.spec.tsto verifythinking: truewhen reasoning is enabled andthinking: falsewhen disabled.chat_template_kwargsfor non-Terminus models.README.mdfiles.This description was created by
for 4eb98a7. You can customize this summary. It will automatically update as commits are pushed.