refactor: consolidated all provider request schemas#198
refactor: consolidated all provider request schemas#198Pratham-Mishra04 wants to merge 3 commits intomainfrom
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| 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.
🛠️ 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.
🧹 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.
🧹 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.
Pratham-Mishra04
force-pushed
the
test/request-typed-structs
branch
from
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
Pratham-Mishra04
force-pushed
the
test/request-typed-structs
branch
from
90a3b20 to
0332637
Compare
Pratham-Mishra04
changed the title
Pratham-Mishra04
changed the title
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (4)
transports/bifrost-http/integrations/genai/types.go (2)
348-385: EnsureParamsis never nil before assigning Tools
bifrostReq.Paramsis assumed non-nil when assigningTools, but that only holds ifconvertGenerationConfigToParamswas 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 allocatingparamswhen nothing is set
convertGenerationConfigToParamsalways allocates a struct even when every field is zero.
Returningnilin 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 1000is invalid
rangecannot 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.Toolsis now a slice, not a pointer.
The nil-check still works butlen(req.Params.Tools) > 0would 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 = 4096properly addresses the previous concern about settingMaxTokensto 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
AnthropicUsagetype 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
AnthropicUsagetype.
69-69: Inconsistent usage types between streaming and non-streaming responses.Streaming responses use
schemas.LLMUsagewhile 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
AnthropicImageContenttype appears to be redundant as it's not used in the codebase.AnthropicImageSourceserves 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.
🛠️ 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.
🧹 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.
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
BedrockChatRequeststruct 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.Namewithout checking ifFunctionis nil first. This could cause a panic ifFunctionis 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 (
AnthropicStreamEventandAnthropicStreamMessage) useschemas.LLMUsagefor their usage fields, while non-streaming responses (AnthropicTextResponseandAnthropicChatResponse) useAnthropicUsage. This inconsistency could lead to confusion and mapping issues when converting between formats.Consider using a consistent approach - either use
AnthropicUsageeverywhere orschemas.LLMUsageeverywhere.
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
2 participants
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.
apipackage incore/schemas/apito house provider-specific API types*[]schemas.Toolto[]schemas.Tooljsontosonicfor better performanceThese changes make the code more maintainable and type-safe while preserving backward compatibility with existing API contracts.