-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(core): mirror Qwen3 reasoning on outbound history #4294
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 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,38 @@ import { | |||||||||||
| hasExplicitOutputLimit, | ||||||||||||
| } from '../../tokenLimits.js'; | ||||||||||||
|
|
||||||||||||
| type AssistantMessageWithReasoningFields = | ||||||||||||
|
Collaborator
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. [Suggestion]
Suggested change
This requires importing the existing type: import type { ExtendedChatCompletionAssistantMessageParam } from '../converter.js';— qwen-latest-series-invite-beta-v28 via Qwen Code /review |
||||||||||||
| OpenAI.Chat.ChatCompletionAssistantMessageParam & { | ||||||||||||
| reasoning_content?: string | null; | ||||||||||||
| reasoning?: string | null; | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
|
Collaborator
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. [Suggestion] The functions here describe what they do but not why. Compare with Consider adding a brief comment explaining that newer vLLM builds consume — qwen-latest-series-invite-beta-v28 via Qwen Code /review |
||||||||||||
| function shouldMirrorReasoningContentForQwen3(model: string): boolean { | ||||||||||||
| return model.toLowerCase().includes('qwen3'); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| function mirrorReasoningContentToReasoning( | ||||||||||||
| message: OpenAI.Chat.ChatCompletionMessageParam, | ||||||||||||
| ): OpenAI.Chat.ChatCompletionMessageParam { | ||||||||||||
| if (message.role !== 'assistant') { | ||||||||||||
| return message; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const assistant = message as AssistantMessageWithReasoningFields; | ||||||||||||
| if ( | ||||||||||||
| typeof assistant.reasoning_content !== 'string' || | ||||||||||||
| assistant.reasoning_content.length === 0 || | ||||||||||||
| typeof assistant.reasoning === 'string' | ||||||||||||
| ) { | ||||||||||||
| return message; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| return { | ||||||||||||
| ...assistant, | ||||||||||||
| reasoning: assistant.reasoning_content, | ||||||||||||
| } as OpenAI.Chat.ChatCompletionMessageParam; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Default provider for standard OpenAI-compatible APIs | ||||||||||||
| */ | ||||||||||||
|
|
@@ -75,9 +107,13 @@ export class DefaultOpenAICompatibleProvider | |||||||||||
| // Apply output token limits to ensure max_tokens is set appropriately | ||||||||||||
| // This prevents occupying too much context window with output reservation | ||||||||||||
| const requestWithTokenLimits = this.applyOutputTokenLimit(request); | ||||||||||||
| const messages = shouldMirrorReasoningContentForQwen3(request.model) | ||||||||||||
| ? requestWithTokenLimits.messages.map(mirrorReasoningContentToReasoning) | ||||||||||||
| : requestWithTokenLimits.messages; | ||||||||||||
|
|
||||||||||||
| return { | ||||||||||||
| ...requestWithTokenLimits, | ||||||||||||
| messages, | ||||||||||||
| ...(extraBody ? extraBody : {}), | ||||||||||||
| }; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
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.
[Suggestion] All 21 existing tests in this file use the
'should ...'convention (e.g.,'should merge extra_body into the request'). The 3 new tests break this pattern with present tense ('mirrors ...','does not overwrite ...','does not mirror ...'). Consider prefixing with'should'for consistency:— qwen-latest-series-invite-beta-v28 via Qwen Code /review