-
Notifications
You must be signed in to change notification settings - Fork 60
refactor: consolidated all provider request schemas #198
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
Summary by CodeRabbit
WalkthroughThis change centralizes all OpenAI and Anthropic API request/response types and model string parsing utilities into new, strongly typed Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPTransport
participant APIBuilder
participant Provider
participant ExternalAPI
Client->>HTTPTransport: Sends request (e.g., chat completion)
HTTPTransport->>APIBuilder: Convert to shared API struct
APIBuilder->>Provider: Passes typed API struct
Provider->>ExternalAPI: Marshals struct, sends HTTP request
ExternalAPI-->>Provider: Returns typed JSON response
Provider->>APIBuilder: Unmarshals to shared API struct
APIBuilder->>HTTPTransport: Converts to Bifrost response
HTTPTransport->>Client: Returns response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 7
Index *int `json:"index,omitempty"` | ||
ContentBlock *AnthropicContentBlock `json:"content_block,omitempty"` | ||
Delta *AnthropicDelta `json:"delta,omitempty"` | ||
Usage *schemas.LLMUsage `json:"usage,omitempty"` |
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.
🛠️ Refactor suggestion
Inconsistent usage type between streaming and non-streaming responses.
AnthropicStreamEvent
and AnthropicStreamMessage
use schemas.LLMUsage
for the usage field, while AnthropicTextResponse
and AnthropicChatResponse
use inline structs. This inconsistency could lead to confusion and mapping issues.
Consider using a consistent approach - either use schemas.LLLUsage
everywhere or define a separate AnthropicUsage
type and use it consistently across all response types.
Also applies to: 83-83
🤖 Prompt for AI Agents
In core/schemas/api/anthropic.go at lines 69 and 83, the usage field types are
inconsistent between streaming and non-streaming response structs, causing
potential confusion and mapping issues. To fix this, define a single consistent
usage type—either use schemas.LLMUsage everywhere or create a new AnthropicUsage
type—and update all response structs to use this unified type for their usage
fields.
type URLTypeInfo struct { | ||
Type ImageContentType | ||
MediaType *string | ||
DataURLWithoutPrefix *string // URL without the prefix (eg data:image/png;base64,iVBORw0KGgo...) | ||
} |
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.
🧹 Nitpick (assertive)
Potential redundancy in image handling types.
There are multiple types handling image content:
URLTypeInfo
(lines 144-148) - contains type, media type, and data URL infoAnthropicImageSource
(lines 99-104) - similar structure for Anthropic APIAnthropicImageContent
(lines 158-162) - another image type with URL and media type
This redundancy could lead to confusion. Consider consolidating these types or clearly documenting their distinct purposes.
Also applies to: 158-162
🤖 Prompt for AI Agents
In core/schemas/api/anthropic.go around lines 99-104, 144-148, and 158-162,
there are multiple similar types handling image content (URLTypeInfo,
AnthropicImageSource, AnthropicImageContent) which creates redundancy and
potential confusion. Review these types to identify overlapping fields and
purposes, then consolidate them into fewer, well-documented types that clearly
differentiate their roles or unify them if they serve the same function. Add
comments to clarify any distinctions if consolidation is not possible.
// BedrockChatRequest represents the unified request structure for Bedrock's chat completion API. | ||
// This typed struct optimizes JSON marshalling performance and supports various models. | ||
type BedrockChatRequest struct { | ||
Messages []BedrockMistralChatMessage `json:"messages"` // Formatted messages | ||
Tools []BedrockAnthropicToolCall `json:"tools,omitempty"` // Optional tool definitions | ||
ToolChoice *string `json:"tool_choice,omitempty"` // Optional tool choice ("auto", "any", "none") | ||
MaxTokens *int `json:"max_tokens,omitempty"` // Maximum tokens to generate | ||
Temperature *float64 `json:"temperature,omitempty"` // Sampling temperature | ||
TopP *float64 `json:"top_p,omitempty"` // Nucleus sampling | ||
ExtraParams map[string]interface{} `json:"-"` | ||
} | ||
|
||
func (r *BedrockChatRequest) MarshalJSON() ([]byte, error) { | ||
result := make(map[string]interface{}, 6+len(r.ExtraParams)) | ||
|
||
result["messages"] = r.Messages | ||
|
||
if r.MaxTokens != nil { | ||
result["max_tokens"] = *r.MaxTokens | ||
} | ||
if r.Temperature != nil { | ||
result["temperature"] = *r.Temperature | ||
} | ||
if r.TopP != nil { | ||
result["top_p"] = *r.TopP | ||
} | ||
if r.Tools != nil { | ||
result["tools"] = r.Tools | ||
} | ||
if r.ToolChoice != nil { | ||
result["tool_choice"] = *r.ToolChoice | ||
} | ||
|
||
maps.Copy(result, r.ExtraParams) | ||
|
||
return sonic.Marshal(result) | ||
} |
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.
🧹 Nitpick (assertive)
Consider clarifying the provider-specific field usage.
The BedrockChatRequest
struct mixes Mistral-specific message types with Anthropic-specific tool types. While this may work for Bedrock's unified API, it could be confusing for maintainers. Consider adding documentation to clarify which fields are used with which model families.
// BedrockChatRequest represents the unified request structure for Bedrock's chat completion API.
// This typed struct optimizes JSON marshalling performance and supports various models.
type BedrockChatRequest struct {
- Messages []BedrockMistralChatMessage `json:"messages"` // Formatted messages
- Tools []BedrockAnthropicToolCall `json:"tools,omitempty"` // Optional tool definitions
+ Messages []BedrockMistralChatMessage `json:"messages"` // Formatted messages (used by all models)
+ Tools []BedrockAnthropicToolCall `json:"tools,omitempty"` // Optional tool definitions (Anthropic models only)
ToolChoice *string `json:"tool_choice,omitempty"` // Optional tool choice ("auto", "any", "none")
🤖 Prompt for AI Agents
In core/schemas/api/bedrock.go around lines 217 to 253, the BedrockChatRequest
struct uses Mistral-specific message types and Anthropic-specific tool types
together, which may confuse maintainers. Add clear comments above each field to
specify which model family or provider the field is intended for, clarifying
usage for future readers and maintainers.
a926824
to
90a3b20
Compare
* feat: new api package and updated providers and integrations to use typed structs * feat: added bedrock types to apo * fix: switched from json to sonic
90a3b20
to
0332637
Compare
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.
Actionable comments posted: 12
🔭 Outside diff range comments (4)
transports/bifrost-http/integrations/genai/types.go (2)
348-385
: EnsureParams
is never nil before assigning Tools
bifrostReq.Params
is assumed non-nil when assigningTools
, but that only holds ifconvertGenerationConfigToParams
was called earlier.
Make the guarantee explicit to avoid future regressions.- if len(tools) > 0 { - bifrostReq.Params.Tools = tools - } + if len(tools) > 0 { + if bifrostReq.Params == nil { // defensive – cheap + bifrostReq.Params = &schemas.ModelParameters{} + } + bifrostReq.Params.Tools = tools + }
398-405
: Minor: skip allocatingparams
when nothing is set
convertGenerationConfigToParams
always allocates a struct even when every field is zero.
Returningnil
in that case would save an unnecessary allocation and keep the calling code simpler.
Not critical, but worth considering.transports/bifrost-http/plugins/logging/main.go (2)
226-231
: Compilation error –for range 1000
is invalid
range
cannot iterate over an integer literal. This will not compile.-// Prewarm the pools for better performance at startup -for range 1000 { +// Pre-warm the pools for better performance at startup +for i := 0; i < 1000; i++ { plugin.logMsgPool.Put(&LogMessage{}) plugin.updateDataPool.Put(&UpdateLogData{}) plugin.streamDataPool.Put(&StreamUpdateData{}) }
519-522
: Redundant nil-check on slice
req.Params.Tools
is now a slice, not a pointer.
The nil-check still works butlen(req.Params.Tools) > 0
would better convey intent (non-empty list).
♻️ Duplicate comments (7)
core/schemas/api/bedrock.go (1)
217-253
: Add documentation to clarify provider-specific field usage.The struct mixes Mistral-specific message types with Anthropic-specific tool types, which could be confusing for maintainers. The past review comment correctly identified this issue.
Consider adding comprehensive documentation:
// BedrockChatRequest represents the unified request structure for Bedrock's chat completion API. // This typed struct optimizes JSON marshalling performance and supports various models. +// +// Note: This struct serves as a unified interface for multiple model families: +// - Messages: Uses Mistral format but is compatible with all Bedrock chat models +// - Tools: Currently only supported by Anthropic models (Claude) +// - Other providers (Llama, etc.) will ignore unsupported fields type BedrockChatRequest struct {core/providers/anthropic.go (1)
21-23
: Good fix for the MaxTokens default value issue.The introduction of
DEFAULT_MAX_TOKENS = 4096
properly addresses the previous concern about settingMaxTokens
to 0. This ensures that Anthropic's API always receives a valid positive value for max_tokens, preventing potential API errors.Also applies to: 139-143, 240-243
core/schemas/api/anthropic.go (5)
32-36
: Extract inline Usage struct for consistency.The inline Usage struct should be replaced with the already-defined
AnthropicUsage
type to improve consistency and reduce duplication.
44-51
: Extract complex inline Content struct.The inline Content struct is complex with many fields. Consider extracting it as a named type for better maintainability.
55-58
: Replace inline Usage struct with AnthropicUsage type.Similar to the previous Usage struct, this should use the already-defined
AnthropicUsage
type.
69-69
: Inconsistent usage types between streaming and non-streaming responses.Streaming responses use
schemas.LLMUsage
while non-streaming responses use inline structs. This inconsistency could lead to confusion and mapping issues.Also applies to: 83-83
158-162
: Potentially unused AnthropicImageContent type.The
AnthropicImageContent
type appears to be redundant as it's not used in the codebase.AnthropicImageSource
serves the same purpose for the Anthropic API.Consider removing this type if it's not used elsewhere:
-type AnthropicImageContent struct { - Type ImageContentType `json:"type"` - URL string `json:"url"` - MediaType string `json:"media_type,omitempty"` -}
if input.VoiceConfig.Voice == nil { | ||
return nil | ||
} |
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.
🛠️ Refactor suggestion
Consider returning an error instead of nil.
Returning nil when voice is missing could cause nil pointer dereferences in callers. Consider returning an error to make the validation failure explicit.
-func buildOpenAISpeechRequest(model string, input *schemas.SpeechInput, params *schemas.ModelParameters) *api.OpenAISpeechRequest {
+func buildOpenAISpeechRequest(model string, input *schemas.SpeechInput, params *schemas.ModelParameters) (*api.OpenAISpeechRequest, error) {
// Set default response format if not provided
responseFormat := input.ResponseFormat
if responseFormat == "" {
responseFormat = "mp3"
}
// Validate voice is provided
if input.VoiceConfig.Voice == nil {
- return nil
+ return nil, fmt.Errorf("voice is required for speech synthesis")
}
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In core/providers/openai.go around lines 188 to 190, the function currently
returns nil when input.VoiceConfig.Voice is nil, which can lead to nil pointer
dereferences in callers. Modify the code to return an explicit error indicating
that the voice configuration is missing instead of returning nil. This will make
the validation failure clear and prevent potential runtime panics.
EncodingFormat *string `json:"encoding_format,omitempty"` // Format for embedding output (e.g., "float", "base64") | ||
Dimensions *int `json:"dimensions,omitempty"` // Number of dimensions for embedding output | ||
User *string `json:"user,omitempty"` // User identifier for tracking | ||
User *string `json:"user,omitempty"` |
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.
🧹 Nitpick (assertive)
Removed comment on User field
The comment for the User
field was removed. If this was intentional, consider whether the field is self-explanatory or if documentation should be maintained elsewhere.
🤖 Prompt for AI Agents
In core/schemas/bifrost.go at line 177, the comment for the User field was
removed. Review whether the User field is self-explanatory; if not, add a clear
and concise comment describing its purpose to maintain code documentation and
clarity.
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.
Actionable comments posted: 2
♻️ Duplicate comments (5)
core/providers/openai.go (1)
188-190
: Nil return is properly handled by callersWhile returning an error directly would be cleaner, the current implementation is acceptable since all callers (lines 630 and 696) check for nil and return appropriate errors.
core/schemas/api/bedrock.go (1)
241-251
: Consider clarifying the provider-specific field usage.The
BedrockChatRequest
struct mixes Mistral-specific message types with Anthropic-specific tool types. While this may work for Bedrock's unified API, it could be confusing for maintainers. Consider adding documentation to clarify which fields are used with which model families.// BedrockChatRequest represents the unified request structure for Bedrock's chat completion API. // This typed struct optimizes JSON marshalling performance and supports various models. type BedrockChatRequest struct { - Messages []BedrockMistralChatMessage `json:"messages"` // Formatted messages - Tools []BedrockAnthropicToolCall `json:"tools,omitempty"` // Optional tool definitions + Messages []BedrockMistralChatMessage `json:"messages"` // Formatted messages (used by all models) + Tools []BedrockAnthropicToolCall `json:"tools,omitempty"` // Optional tool definitions (Anthropic models only) ToolChoice *string `json:"tool_choice,omitempty"` // Optional tool choice ("auto", "any", "none")core/providers/anthropic.go (1)
276-291
: Potential nil pointer dereference when accessing Function.Name.At line 286, the code accesses
params.ToolChoice.ToolChoiceStruct.Function.Name
without checking ifFunction
is nil first. This could cause a panic ifFunction
is not set.Apply this fix:
} else if params.ToolChoice.ToolChoiceStruct != nil { toolChoice := &api.AnthropicToolChoice{ Type: params.ToolChoice.ToolChoiceStruct.Type, } - if params.ToolChoice.ToolChoiceStruct.Function.Name != "" { + if params.ToolChoice.ToolChoiceStruct.Function != nil && params.ToolChoice.ToolChoiceStruct.Function.Name != "" { toolChoice.Name = ¶ms.ToolChoice.ToolChoiceStruct.Function.Name } request.ToolChoice = toolChoice }core/schemas/api/anthropic.go (2)
57-80
: Inconsistent usage types between streaming and non-streaming responses.The streaming structs (
AnthropicStreamEvent
andAnthropicStreamMessage
) useschemas.LLMUsage
for their usage fields, while non-streaming responses (AnthropicTextResponse
andAnthropicChatResponse
) useAnthropicUsage
. This inconsistency could lead to confusion and mapping issues when converting between formats.Consider using a consistent approach - either use
AnthropicUsage
everywhere orschemas.LLMUsage
everywhere.
94-158
: Multiple redundant image handling types need consolidation or clarification.There are three similar types handling image content:
AnthropicImageSource
- contains type, media type, data, and URLURLTypeInfo
- contains type, media type, and data URL infoAnthropicImageContent
- contains type, URL, and media typeThis redundancy could lead to confusion about which type to use in different contexts. Consider:
- Consolidating these into fewer types if they serve the same purpose
- Adding clear documentation to distinguish their specific use cases
- Using type aliases if they represent the same data in different contexts
Refactor Provider APIs with Type-Safe Structs
This PR introduces a major refactoring of the provider APIs to use type-safe structs instead of generic maps. The changes improve type safety, performance, and maintainability across the codebase.
api
package incore/schemas/api
to house provider-specific API types*[]schemas.Tool
to[]schemas.Tool
json
tosonic
for better performanceThese changes make the code more maintainable and type-safe while preserving backward compatibility with existing API contracts.