return finish reason in bedrock error chunk #1143
Conversation
|
Description🔄 What Changed
🔍 Impact of the ChangeThis change improves the handling of streaming errors from Bedrock by ensuring that a 📁 Total Files Changed2 files were changed in this pull request. 🧪 Test AddedNo explicit tests were mentioned or added in this pull request description. The PR body states "This is not a fix for #1142, the stream error is still consumed silently, but this change returns finish_reason which is required for the last streaming chunk", implying a functional enhancement rather than a bug fix with specific test cases. 🔒Security VulnerabilitiesNo security vulnerabilities were detected in the provided code changes. MotivationThis change returns the Type of Change
How Has This Been Tested?
Screenshots (if applicable)N/A Checklist
Related IssuesN/A Tip Quality Recommendations
Sequence DiagramsequenceDiagram
participant User
participant GatewayAPI as Gateway API (Bedrock ChatComplete)
participant BedrockService as Bedrock Service
participant BedrockChatCompleteStreamChunkTransform as BedrockChatCompleteStreamChunkTransform
participant getBedrockErrorChunk as getBedrockErrorChunk
User->>GatewayAPI: Initiate Chat Completion Stream Request
GatewayAPI->>BedrockService: Forward Request
BedrockService-->>GatewayAPI: Stream Chunks (JSON)
GatewayAPI->>BedrockChatCompleteStreamChunkTransform: Process Stream Chunk (responseChunk)
alt Bedrock Error Message Received
BedrockChatCompleteStreamChunkTransform->>BedrockChatCompleteStreamChunkTransform: Parse responseChunk (parsedChunk)
BedrockChatCompleteStreamChunkTransform-->>BedrockChatCompleteStreamChunkTransform: Check if parsedChunk.message exists
activate BedrockChatCompleteStreamChunkTransform
BedrockChatCompleteStreamChunkTransform->>getBedrockErrorChunk: Call getBedrockErrorChunk(fallbackId, model)
activate getBedrockErrorChunk
getBedrockErrorChunk-->>BedrockChatCompleteStreamChunkTransform: Return formatted error chunk (data: {...finish_reason: 'stop'...}
\ndata: [DONE])
deactivate getBedrockErrorChunk
BedrockChatCompleteStreamChunkTransform-->>GatewayAPI: Return Error Stream Chunk
deactivate BedrockChatCompleteStreamChunkTransform
else Standard Stream Chunk
BedrockChatCompleteStreamChunkTransform->>BedrockChatCompleteStreamChunkTransform: Parse responseChunk (parsedChunk)
BedrockChatCompleteStreamChunkTransform-->>BedrockChatCompleteStreamChunkTransform: Check for parsedChunk.stopReason
activate BedrockChatCompleteStreamChunkTransform
BedrockChatCompleteStreamChunkTransform-->>BedrockChatCompleteStreamChunkTransform: Process delta, tool_calls, etc.
BedrockChatCompleteStreamChunkTransform-->>BedrockChatCompleteStreamChunkTransform: Ensure finish_reason: null for non-error chunks
BedrockChatCompleteStreamChunkTransform-->>GatewayAPI: Return Transformed Stream Chunk
deactivate BedrockChatCompleteStreamChunkTransform
end
GatewayAPI-->>User: Stream Transformed Chunks
|
![matter-code-review[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
| const parsedChunk: BedrockChatCompleteStreamChunk = JSON.parse(responseChunk); | ||
| console.log(JSON.stringify(parsedChunk, null, 2)); | ||
| if (parsedChunk.message) { | ||
| return getBedrockErrorChunk(fallbackId, gatewayRequest.model || ''); | ||
| } |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: The console.log statement should not be included in production code.
Fix: Remove the debugging console.log statement.
Impact: Prevents unnecessary logging in production and keeps the logs clean.
| const parsedChunk: BedrockChatCompleteStreamChunk = JSON.parse(responseChunk); | |
| console.log(JSON.stringify(parsedChunk, null, 2)); | |
| if (parsedChunk.message) { | |
| return getBedrockErrorChunk(fallbackId, gatewayRequest.model || ''); | |
| } | |
| const parsedChunk: BedrockChatCompleteStreamChunk = JSON.parse(responseChunk); | |
| if (parsedChunk.message) { | |
| return getBedrockErrorChunk(fallbackId, gatewayRequest.model || ''); | |
| } |
| export const getBedrockErrorChunk = (id: string, model: string) => { | ||
| return [ | ||
| `data: ${JSON.stringify({ | ||
| id, | ||
| object: 'chat.completion.chunk', | ||
| created: Math.floor(Date.now() / 1000), | ||
| model: model, | ||
| provider: BEDROCK, | ||
| choices: [ | ||
| { | ||
| index: 0, | ||
| delta: { | ||
| role: 'assistant', | ||
| finish_reason: 'stop', | ||
| }, | ||
| }, | ||
| ], | ||
| })}\n\n`, | ||
| `data: [DONE]\n\n`, | ||
| ]; |
There was a problem hiding this comment.
⚡️ Performance Improvement
Issue: The error chunk doesn't include the actual error message from Bedrock, which could be useful for debugging.
Fix: Pass and include the error message in the response.
Impact: Improves error reporting and makes debugging easier.
| export const getBedrockErrorChunk = (id: string, model: string) => { | |
| return [ | |
| `data: ${JSON.stringify({ | |
| id, | |
| object: 'chat.completion.chunk', | |
| created: Math.floor(Date.now() / 1000), | |
| model: model, | |
| provider: BEDROCK, | |
| choices: [ | |
| { | |
| index: 0, | |
| delta: { | |
| role: 'assistant', | |
| finish_reason: 'stop', | |
| }, | |
| }, | |
| ], | |
| })}\n\n`, | |
| `data: [DONE]\n\n`, | |
| ]; | |
| export const getBedrockErrorChunk = (id: string, model: string, errorMessage?: string) => { | |
| return [ | |
| `data: ${JSON.stringify({ | |
| id, | |
| object: 'chat.completion.chunk', | |
| created: Math.floor(Date.now() / 1000), | |
| model: model, | |
| provider: BEDROCK, | |
| choices: [ | |
| { | |
| index: 0, | |
| delta: { | |
| role: 'assistant', | |
| finish_reason: 'stop', | |
| }, | |
| error: errorMessage, | |
| }, | |
| ], | |
| })}\ | |
| \ | |
| `, | |
| `data: [DONE]\ | |
| \ | |
| `, | |
| ]; |
| if (parsedChunk.message) { | ||
| return getBedrockErrorChunk(fallbackId, gatewayRequest.model || ''); | ||
| } |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: The error message from Bedrock is not being passed to the error chunk function.
Fix: Pass the error message to the getBedrockErrorChunk function.
Impact: Provides more context in the error response for better debugging.
| if (parsedChunk.message) { | |
| return getBedrockErrorChunk(fallbackId, gatewayRequest.model || ''); | |
| } | |
| if (parsedChunk.message) { | |
| return getBedrockErrorChunk(fallbackId, gatewayRequest.model || '', parsedChunk.message); | |
| } |
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
|
Glad to see this, has been super frustrating using Portkey with Bedrock |
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
3 participants
This is not a fix for #1142 , the stream error is still consumed silently, but this change returns finish reason which is required for the last streaming chunk