Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Code Review
This pull request implements support for DeepSeek's thinking blocks within the Anthropic transformer, ensuring assistant messages include a thinking block when thinking is enabled. It also updates the platform configuration to support output_config for DeepSeek and adds comprehensive unit tests. Feedback suggests optimizing the ensureAssistantThinkingBlocks function by moving variable definitions outside the loop and using more efficient slice manipulation techniques.
| func ensureAssistantThinkingBlocks(messages []MessageParam) { | ||
| for i, msg := range messages { | ||
| if msg.Role != "assistant" { | ||
| continue | ||
| } | ||
| if hasThinkingBlock(msg) { | ||
| continue | ||
| } | ||
|
|
||
| emptyThinking := "" | ||
| thinkingBlock := MessageContentBlock{ | ||
| Type: "thinking", | ||
| Thinking: &emptyThinking, | ||
| } | ||
|
|
||
| if msg.Content.Content != nil { | ||
| // Convert simple string content to multiple content with thinking + text | ||
| textBlock := MessageContentBlock{ | ||
| Type: "text", | ||
| Text: msg.Content.Content, | ||
| } | ||
| messages[i].Content = MessageContent{ | ||
| MultipleContent: []MessageContentBlock{thinkingBlock, textBlock}, | ||
| } | ||
| } else { | ||
| // Prepend thinking block to existing multiple content | ||
| messages[i].Content.MultipleContent = append( | ||
| []MessageContentBlock{thinkingBlock}, | ||
| messages[i].Content.MultipleContent..., | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The ensureAssistantThinkingBlocks function can be optimized by moving the emptyThinking variable definition outside the loop. Additionally, for prepending the thinking block to MultipleContent, using a pre-allocated slice or slices.Insert (if using Go 1.21+) would be more efficient than the current append pattern which creates multiple intermediate slices.
func ensureAssistantThinkingBlocks(messages []MessageParam) {
emptyThinking := ""
for i, msg := range messages {
if msg.Role != "assistant" {
continue
}
if hasThinkingBlock(msg) {
continue
}
thinkingBlock := MessageContentBlock{
Type: "thinking",
Thinking: &emptyThinking,
}
if msg.Content.Content != nil {
// Convert simple string content to multiple content with thinking + text
textBlock := MessageContentBlock{
Type: "text",
Text: msg.Content.Content,
}
messages[i].Content = MessageContent{
MultipleContent: []MessageContentBlock{thinkingBlock, textBlock},
}
} else {
// Prepend thinking block to existing multiple content
newContent := make([]MessageContentBlock, 0, len(messages[i].Content.MultipleContent)+1)
newContent = append(newContent, thinkingBlock)
newContent = append(newContent, messages[i].Content.MultipleContent...)
messages[i].Content.MultipleContent = newContent
}
}
}
Uh oh!
There was an error while loading. Please reload this page.