-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: handle Mistral thinking type in streaming responses #6843
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 | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -57,10 +57,17 @@ export class MistralHandler extends BaseProvider implements SingleCompletionHand | |||||||
| if (typeof delta.content === "string") { | ||||||||
| content = delta.content | ||||||||
| } else if (Array.isArray(delta.content)) { | ||||||||
| content = delta.content.map((c) => (c.type === "text" ? c.text : "")).join("") | ||||||||
| // Handle array content, filtering out "thinking" type and other non-text types | ||||||||
| content = delta.content | ||||||||
| .filter((c: any) => c.type === "text") | ||||||||
|
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 use of
Suggested change
This would improve type safety and make the code more maintainable. |
||||||||
| .map((c: any) => c.text || "") | ||||||||
| .join("") | ||||||||
| } | ||||||||
|
|
||||||||
| yield { type: "text", text: content } | ||||||||
| // Only yield if we have actual content to send | ||||||||
| if (content) { | ||||||||
| yield { type: "text", text: content } | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| if (chunk.data.usage) { | ||||||||
|
|
@@ -97,7 +104,11 @@ export class MistralHandler extends BaseProvider implements SingleCompletionHand | |||||||
| const content = response.choices?.[0]?.message.content | ||||||||
|
|
||||||||
| if (Array.isArray(content)) { | ||||||||
| return content.map((c) => (c.type === "text" ? c.text : "")).join("") | ||||||||
| // Handle array content, filtering out "thinking" type and other non-text types | ||||||||
| return content | ||||||||
| .filter((c: any) => c.type === "text") | ||||||||
|
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. Same type safety consideration here - could we use a proper type instead of |
||||||||
| .map((c: any) => c.text || "") | ||||||||
| .join("") | ||||||||
| } | ||||||||
|
|
||||||||
| return 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.
Great test coverage for the streaming case! However, I notice we're missing tests for the
completePromptmethod which also received the same fix. Would it make sense to add a test case to ensurecompletePrompthandles the "thinking" type correctly as well?