Conversation
Summary by CodeRabbit
WalkthroughA new configuration flag, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Backend
participant Provider
User->>UI: Toggle "Send Back Raw Response" in provider form
UI->>Backend: Submit provider config (send_back_raw_response)
Backend->>Provider: Initialize with sendBackRawResponse flag
User->>Backend: Make completion request
Backend->>Provider: Call ChatCompletion/TextCompletion
Provider->>Provider: Conditionally include raw API response based on flag
Provider-->>Backend: Return BifrostResponse (with/without raw response)
Backend-->>User: Return response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
✨ 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 (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
ui/components/config/provider-form.tsx (2)
183-196: AddsendBackRawResponseto the useEffect dependency array.The
sendBackRawResponsefield is missing from the dependency array, which means changes to this field alone won't mark the form as dirty, preventing users from saving.- }, [selectedProvider, keys, networkConfig, performanceConfig, metaConfig, proxyConfig, initialState, keysRequired]) + }, [selectedProvider, keys, networkConfig, performanceConfig, metaConfig, proxyConfig, sendBackRawResponse, initialState, keysRequired])
236-250: IncludesendBackRawResponsein new provider creation.The
sendBackRawResponsefield is included for provider updates but missing from new provider creation, causing inconsistent behavior.meta_config: metaConfig, proxy_config: proxyConfig, + send_back_raw_response: sendBackRawResponse, }core/providers/ollama.go (1)
80-86: Critical: Missing initialization of sendBackRawResponse field.The
sendBackRawResponsefield is declared but not initialized in the constructor. This will cause the field to always befalse, preventing the raw response feature from working.Apply this fix to initialize the field from the provider configuration:
return &OllamaProvider{ - logger: logger, - client: client, - streamClient: streamClient, - networkConfig: config.NetworkConfig, + logger: logger, + client: client, + streamClient: streamClient, + networkConfig: config.NetworkConfig, + sendBackRawResponse: config.SendBackRawResponse, }, nilcore/providers/sgl.go (1)
80-86: Critical: Missing initialization of sendBackRawResponse field.The
sendBackRawResponsefield is declared but not initialized in the constructor, causing the raw response feature to be non-functional.Apply this fix to initialize the field:
return &SGLProvider{ - logger: logger, - client: client, - streamClient: streamClient, - networkConfig: config.NetworkConfig, + logger: logger, + client: client, + streamClient: streamClient, + networkConfig: config.NetworkConfig, + sendBackRawResponse: config.SendBackRawResponse, }, nil
Pratham-Mishra04
force-pushed
the
07-29-feat_added_toggle_for_include_raw_response
branch
2 times, most recently
from
71ad22f to
eb6ae77
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
core/providers/ollama.go (1)
80-85: Missing field initialization in constructor.The
sendBackRawResponsefield is declared in the struct but not initialized from the configuration. This will cause the field to default tofalseregardless of the user's configuration setting.Add the missing field initialization:
return &OllamaProvider{ logger: logger, client: client, streamClient: streamClient, networkConfig: config.NetworkConfig, + sendBackRawResponse: config.SendBackRawResponse, }, nilcore/providers/sgl.go (1)
80-85: Missing field initialization in constructor.The
sendBackRawResponsefield is declared in the struct but not initialized from the configuration, causing the same issue as in the Ollama provider.Add the missing field initialization:
return &SGLProvider{ logger: logger, client: client, streamClient: streamClient, networkConfig: config.NetworkConfig, + sendBackRawResponse: config.SendBackRawResponse, }, nil
♻️ Duplicate comments (1)
ui/components/config/provider-form.tsx (1)
924-937: Fix accessibility issue with the label element.The label element needs proper association with the control for screen readers.
- <label className="text-sm font-medium">Include Raw Response</label> + <label htmlFor="sendBackRawResponse" className="text-sm font-medium">Include Raw Response</label> <p className="text-muted-foreground text-xs"> Include the raw provider response alongside the parsed response for debugging and advanced use cases </p> </div> <Switch + id="sendBackRawResponse" checked={sendBackRawResponse} onCheckedChange={(checked) => updateField('sendBackRawResponse', checked)} />
Pratham-Mishra04
force-pushed
the
07-29-feat_added_toggle_for_include_raw_response
branch
from
eb6ae77 to
2fe3ada
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
ui/components/config/provider-form.tsx (1)
924-937: Fix accessibility issue with the label element.The label element needs proper association with the control for screen readers.
- <label className="text-sm font-medium">Include Raw Response</label> + <label htmlFor="sendBackRawResponse" className="text-sm font-medium">Include Raw Response</label> <p className="text-muted-foreground text-xs"> Include the raw provider response alongside the parsed response for debugging and advanced use cases </p> </div> <Switch + id="sendBackRawResponse" checked={sendBackRawResponse} onCheckedChange={(checked) => updateField('sendBackRawResponse', checked)} />core/providers/mistral.go (1)
33-51: Response pool refactoring already approved in previous review.Also applies to: 81-81
Pratham-Mishra04
force-pushed
the
07-29-feat_added_toggle_for_include_raw_response
branch
from
2fe3ada to
80f516a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
core/providers/openai.go (2)
170-176: Returning a pooled object afterdefer releasecauses use-after-recycle
responseis released back toopenAIResponsePoolby the deferred call before the caller receives it, so the caller will read a struct that may already be reused or zeroed by another goroutine – classic data corruption bug.- response := acquireOpenAIResponse() - defer releaseOpenAIResponse(response) +response := acquireOpenAIResponse() +// Do **not** release here; the caller now owns the object.If you want pooling without corruption, release only in paths where the object is not returned, or redesign to copy the data into a fresh struct before release.
857-876: Avoid double unmarshal when raw response is disabled
responseBodyis unmarshaled intorawResponseeven whensendBackRawResponseis false, incurring unnecessary CPU cost.- // Parse raw response for RawResponse field - var rawResponse interface{} - if err := sonic.Unmarshal(responseBody, &rawResponse); err != nil { - return nil, newBifrostOperationError(schemas.ErrProviderDecodeRaw, err, schemas.OpenAI) - } + var rawResponse interface{} + if provider.sendBackRawResponse { + if err := sonic.Unmarshal(responseBody, &rawResponse); err != nil { + return nil, newBifrostOperationError(schemas.ErrProviderDecodeRaw, err, schemas.OpenAI) + } + }Then keep the existing conditional assignment.
♻️ Duplicate comments (2)
ui/components/config/provider-form.tsx (1)
924-937: Fix accessibility issue with the label element.The label element needs proper association with the Switch control for screen readers.
Apply this fix to resolve the accessibility issue:
- <label className="text-sm font-medium">Include Raw Response</label> + <label htmlFor="sendBackRawResponse" className="text-sm font-medium">Include Raw Response</label> <p className="text-muted-foreground text-xs"> Include the raw provider response alongside the parsed response for debugging and advanced use cases </p> </div> <Switch + id="sendBackRawResponse" checked={sendBackRawResponse} onCheckedChange={(checked) => updateField('sendBackRawResponse', checked)} />core/providers/mistral.go (1)
33-51: Good refactoring to unify response handling.The migration from provider-specific
MistralResponseto the sharedschemas.BifrostResponsereduces code duplication and improves consistency across providers.
Merge activity
|
akshaydeo
changed the base branch from
07-29-enhancement_serialisation_optimisations
to
graphite-base/196
akshaydeo
dismissed stale reviews from coderabbitai[bot] and themself
The base branch was changed.
akshaydeo
force-pushed
the
07-29-feat_added_toggle_for_include_raw_response
branch
from
80f516a to
01ade14
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
core/providers/mistral.go (1)
288-288: Embedding method doesn't respect the sendBackRawResponse flag.The Embedding method always includes the raw response in line 288, which is inconsistent with the PR objectives and the implementation in other methods. It should conditionally set the raw response based on
provider.sendBackRawResponse.Apply this fix:
- ExtraFields: schemas.BifrostResponseExtraFields{ - Provider: schemas.Mistral, - RawResponse: rawResponse, - }, + ExtraFields: schemas.BifrostResponseExtraFields{ + Provider: schemas.Mistral, + },Then after creating the response, add:
+ if provider.sendBackRawResponse { + bifrostResponse.ExtraFields.RawResponse = rawResponse + }
♻️ Duplicate comments (2)
core/providers/bedrock.go (1)
841-848: Consider usinghandleProviderResponseutility for consistency.While the current implementation works correctly, it's inconsistent with the
ChatCompletionmethod which uses thehandleProviderResponseutility function. For consistency across the codebase, consider refactoring this to use the same utility pattern.Apply this diff to align with the consistent pattern:
- // Parse raw response if enabled - if provider.sendBackRawResponse { - var rawResponse interface{} - if err := sonic.Unmarshal(body, &rawResponse); err != nil { - return nil, newBifrostOperationError("error parsing raw response", err, schemas.Bedrock) - } - bifrostResponse.ExtraFields.RawResponse = rawResponse + // Handle raw response if enabled + rawResponse, bifrostErr := handleProviderResponse(body, &struct{}{}, provider.sendBackRawResponse) + if bifrostErr != nil { + return nil, bifrostErr + } + if provider.sendBackRawResponse { + bifrostResponse.ExtraFields.RawResponse = rawResponse }ui/components/config/provider-form.tsx (1)
924-937: Fix accessibility issue with the label element.The label element needs proper association with the control for screen readers.
- <label className="text-sm font-medium">Include Raw Response</label> + <label htmlFor="sendBackRawResponse" className="text-sm font-medium">Include Raw Response</label> <p className="text-muted-foreground text-xs"> Include the raw provider response alongside the parsed response for debugging and advanced use cases </p> </div> <Switch + id="sendBackRawResponse" checked={sendBackRawResponse} onCheckedChange={(checked) => updateField('sendBackRawResponse', checked)} />
# Make Raw Response Optional in Provider Responses This PR adds a configuration option to control whether raw provider responses are included in Bifrost responses. Previously, raw responses were always included, which could increase response size and bandwidth usage unnecessarily. Key changes: - Added `send_back_raw_response` boolean flag to provider configuration - Modified all providers to respect this setting when processing responses - Updated the UI to include a toggle for this option in the provider configuration form - Optimized response handling to avoid unnecessary JSON parsing when raw responses aren't needed - Standardized response object pools across providers to use the common `BifrostResponse` type This change improves performance by avoiding unnecessary JSON parsing and reduces response sizes when raw responses aren't needed, while maintaining backward compatibility for users who rely on the raw response data.
2 participants
Make Raw Response Optional in Provider Responses
This PR adds a configuration option to control whether raw provider responses are included in Bifrost responses. Previously, raw responses were always included, which could increase response size and bandwidth usage unnecessarily.
Key changes:
send_back_raw_responseboolean flag to provider configurationBifrostResponsetypeThis change improves performance by avoiding unnecessary JSON parsing and reduces response sizes when raw responses aren't needed, while maintaining backward compatibility for users who rely on the raw response data.