-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: enable reasoning display for DeepSeek V3 models with reasoning effort #7371
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -380,6 +380,55 @@ describe("OpenAiHandler", () => { | |
| const callArgs = mockCreate.mock.calls[0][0] | ||
| expect(callArgs.temperature).toBe(0.6) | ||
| }) | ||
|
|
||
| it("should detect DeepSeek V3 models with reasoning effort as reasoning models", async () => { | ||
| const deepseekV3Options: ApiHandlerOptions = { | ||
| ...mockOptions, | ||
| openAiModelId: "deepseek-v3", | ||
| openAiCustomModelInfo: { | ||
| ...openAiModelInfoSaneDefaults, | ||
| supportsReasoningEffort: true, | ||
| }, | ||
| reasoningEffort: "medium", | ||
| } | ||
| const deepseekHandler = new OpenAiHandler(deepseekV3Options) | ||
| const stream = deepseekHandler.createMessage(systemPrompt, messages) | ||
| for await (const _chunk of stream) { | ||
| // consume stream | ||
| } | ||
| // Assert the mockCreate was called with R1 format messages | ||
| expect(mockCreate).toHaveBeenCalled() | ||
| const callArgs = mockCreate.mock.calls[0][0] | ||
| // When DeepSeek is detected as a reasoning model, it uses R1 format | ||
| // which combines system and user messages | ||
| expect(callArgs.messages[0].role).toBe("user") | ||
| expect(callArgs.messages[0].content).toContain("You are a helpful assistant.") | ||
| expect(callArgs.reasoning_effort).toBe("medium") | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we also verify that there's no separate system message in the converted messages? This would make the R1 format usage more explicit: |
||
| }) | ||
|
|
||
| it("should detect DeepSeek-chat models with reasoning effort as reasoning models", async () => { | ||
| const deepseekChatOptions: ApiHandlerOptions = { | ||
| ...mockOptions, | ||
| openAiModelId: "deepseek-chat", | ||
| openAiCustomModelInfo: { | ||
| ...openAiModelInfoSaneDefaults, | ||
| supportsReasoningEffort: true, | ||
| }, | ||
| reasoningEffort: "high", | ||
| } | ||
| const deepseekHandler = new OpenAiHandler(deepseekChatOptions) | ||
| const stream = deepseekHandler.createMessage(systemPrompt, messages) | ||
| for await (const _chunk of stream) { | ||
| // consume stream | ||
| } | ||
| // Assert the mockCreate was called with R1 format messages | ||
| expect(mockCreate).toHaveBeenCalled() | ||
| const callArgs = mockCreate.mock.calls[0][0] | ||
| // When DeepSeek is detected as a reasoning model, it uses R1 format | ||
| expect(callArgs.messages[0].role).toBe("user") | ||
| expect(callArgs.messages[0].content).toContain("You are a helpful assistant.") | ||
| expect(callArgs.reasoning_effort).toBe("high") | ||
| }) | ||
| }) | ||
|
|
||
| describe("error handling", () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,7 +89,12 @@ export class OpenAiHandler extends BaseProvider implements SingleCompletionHandl | |
| const enabledR1Format = this.options.openAiR1FormatEnabled ?? false | ||
| const enabledLegacyFormat = this.options.openAiLegacyFormat ?? false | ||
| const isAzureAiInference = this._isAzureAiInference(modelUrl) | ||
| const deepseekReasoner = modelId.includes("deepseek-reasoner") || enabledR1Format | ||
| // Check if this is a DeepSeek model with reasoning enabled | ||
| const isDeepSeekWithReasoning = | ||
| (modelId.toLowerCase().includes("deepseek") && reasoning) || | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this intentional to keep the detection logic inline? Consider extracting
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue comment mentions GLM-4.5 has the same problem. Should we consider adding similar detection for GLM models in this PR, or would that be better as a separate fix to keep this PR focused? |
||
| modelId.includes("deepseek-reasoner") || | ||
| enabledR1Format | ||
| const deepseekReasoner = isDeepSeekWithReasoning | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The variable naming here could be clearer. We have |
||
| const ark = modelUrl.includes(".volces.com") | ||
|
|
||
| if (modelId.includes("o1") || modelId.includes("o3") || modelId.includes("o4")) { | ||
|
|
||
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.
These tests look good, but what about edge cases like "DeepSeek-V3" (uppercase) or "deepseek-v3.1"? Should we add tests to ensure the case-insensitive matching works correctly for all variations?