feat(payload-processing): add AWS Bedrock OpenAI chat completions sup…#601
feat(payload-processing): add AWS Bedrock OpenAI chat completions sup…#601srampal wants to merge 1 commit intoopendatahub-io:mainfrom
Conversation
…port - Add AWSBedrockOpenAI provider constant - Implement Bedrock translator for OpenAI-compatible API - Support configurable AWS regions (defaults to us-east-1) - Add API key injection with Bearer token format - Include comprehensive test suite (15 test cases) - Maintain full compatibility with existing providers
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: srampal The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@nirrozenbaum PTAL |
|
/retest |
| @@ -0,0 +1,106 @@ | |||
| /* | |||
| Copyright 2026 The opendatahub.io Authors. | |||
There was a problem hiding this comment.
nit: align with the other files in the repo
| Copyright 2026 The opendatahub.io Authors. | |
| Copyright 2026. |
| if region == "" { | ||
| region = awsDefaultRegion | ||
| } | ||
|
|
There was a problem hiding this comment.
this is not needed. called should set the region. and in the translator we assume it was set properly.
| // Validate this is a Chat Completions request | ||
| if _, hasMessages := body["messages"]; !hasMessages { | ||
| return nil, nil, nil, fmt.Errorf("only Chat Completions API is supported - 'messages' field required") | ||
| } |
There was a problem hiding this comment.
no need - basic assumptions it only OpenAI chat completions for now.
pls remove.
| // Validate this is a Chat Completions request | |
| if _, hasMessages := body["messages"]; !hasMessages { | |
| return nil, nil, nil, fmt.Errorf("only Chat Completions API is supported - 'messages' field required") | |
| } |
| headersToMutate = map[string]string{ | ||
| ":path": bedrockOpenAIPath, | ||
| "content-type": "application/json", | ||
| "host": fmt.Sprintf("bedrock-runtime.%s.amazonaws.com", t.region), |
There was a problem hiding this comment.
we are not supposed to set host here.
this should be part of MaaSModelRef CR, where the external model should declare:
provider (e.g., bedrock-openai) and endpoint (e.g., bedrock-runtime..amazonaws.com).
this shouldn't be part of the translator.
we should also remove the env var + default region vars.
| // GetRegion returns the AWS region configured for this translator | ||
| func (t *BedrockTranslator) GetRegion() string { | ||
| return t.region | ||
| } No newline at end of file |
There was a problem hiding this comment.
no need to get region.
| func createTestTranslator() *BedrockTranslator { | ||
| return NewBedrockTranslator() | ||
| } |
There was a problem hiding this comment.
nit: can we remove this function?
just call NewBedrockTranslator()
| assert.Nil(t, translatedBody, "body should not be mutated for Bedrock OpenAI-compatible API") | ||
| assert.Equal(t, "/v1/chat/completions", headers[":path"]) | ||
| assert.Equal(t, "application/json", headers["content-type"]) | ||
| assert.Equal(t, "bedrock-runtime.us-east-1.amazonaws.com", headers["host"]) |
There was a problem hiding this comment.
pls remove:
| assert.Equal(t, "bedrock-runtime.us-east-1.amazonaws.com", headers["host"]) |
| func TestTranslateRequest_WithRegion(t *testing.T) { | ||
| translator := NewBedrockTranslatorWithRegion("us-west-2") | ||
|
|
||
| body := map[string]any{ | ||
| "model": "nvidia.nemotron-nano-12b-v2", | ||
| "messages": []any{ | ||
| map[string]any{"role": "user", "content": "Hello"}, | ||
| }, | ||
| } | ||
|
|
||
| _, headers, _, err := translator.TranslateRequest(body) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, "bedrock-runtime.us-west-2.amazonaws.com", headers["host"]) | ||
| } |
There was a problem hiding this comment.
no need to add host.
previous test should cover the rest of functionality. can be removed.
| func TestTranslateRequest_WithRegion(t *testing.T) { | |
| translator := NewBedrockTranslatorWithRegion("us-west-2") | |
| body := map[string]any{ | |
| "model": "nvidia.nemotron-nano-12b-v2", | |
| "messages": []any{ | |
| map[string]any{"role": "user", "content": "Hello"}, | |
| }, | |
| } | |
| _, headers, _, err := translator.TranslateRequest(body) | |
| require.NoError(t, err) | |
| assert.Equal(t, "bedrock-runtime.us-west-2.amazonaws.com", headers["host"]) | |
| } |
| body := map[string]any{ | ||
| "model": "nvidia.nemotron-nano-12b-v2", | ||
| "messages": []any{ | ||
| map[string]any{"role": "system", "content": "You are helpful."}, | ||
| map[string]any{"role": "user", "content": "Hello"}, | ||
| }, | ||
| "temperature": 0.7, | ||
| "top_p": 0.9, | ||
| "max_tokens": 1000, | ||
| "stream": true, | ||
| "stop": []any{"END"}, | ||
| "n": 1, | ||
| "presence_penalty": 0.5, | ||
| "frequency_penalty": 0.3, | ||
| } |
There was a problem hiding this comment.
can you just move these fields to the first test and remove duplicated test?
| func TestTranslateRequest_MissingMessages(t *testing.T) { | ||
| body := map[string]any{ | ||
| "model": "nvidia.nemotron-nano-12b-v2", | ||
| } | ||
|
|
||
| _, _, _, err := createTestTranslator().TranslateRequest(body) | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "only Chat Completions API is supported") | ||
| } |
There was a problem hiding this comment.
remove pls
| func TestTranslateRequest_MissingMessages(t *testing.T) { | |
| body := map[string]any{ | |
| "model": "nvidia.nemotron-nano-12b-v2", | |
| } | |
| _, _, _, err := createTestTranslator().TranslateRequest(body) | |
| assert.Error(t, err) | |
| assert.Contains(t, err.Error(), "only Chat Completions API is supported") | |
| } |
| func TestTranslateRequest_LegacyCompletionsNotSupported(t *testing.T) { | ||
| body := map[string]any{ | ||
| "model": "nvidia.nemotron-nano-12b-v2", | ||
| "prompt": "Complete this sentence: The weather today is", | ||
| } | ||
|
|
||
| _, _, _, err := createTestTranslator().TranslateRequest(body) | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "only Chat Completions API is supported") | ||
| } |
There was a problem hiding this comment.
remove please.
| func TestTranslateRequest_LegacyCompletionsNotSupported(t *testing.T) { | |
| body := map[string]any{ | |
| "model": "nvidia.nemotron-nano-12b-v2", | |
| "prompt": "Complete this sentence: The weather today is", | |
| } | |
| _, _, _, err := createTestTranslator().TranslateRequest(body) | |
| assert.Error(t, err) | |
| assert.Contains(t, err.Error(), "only Chat Completions API is supported") | |
| } |
| func TestTranslateRequest_HeadersSet(t *testing.T) { | ||
| body := map[string]any{ | ||
| "model": "nvidia.nemotron-nano-12b-v2", | ||
| "messages": []any{map[string]any{"role": "user", "content": "Hi"}}, | ||
| } | ||
|
|
||
| _, headers, _, err := createTestTranslator().TranslateRequest(body) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Len(t, headers, 3) | ||
| assert.Contains(t, headers, ":path") | ||
| assert.Contains(t, headers, "content-type") | ||
| assert.Contains(t, headers, "host") | ||
| } |
There was a problem hiding this comment.
covered in first test, pls remove:
| func TestTranslateRequest_HeadersSet(t *testing.T) { | |
| body := map[string]any{ | |
| "model": "nvidia.nemotron-nano-12b-v2", | |
| "messages": []any{map[string]any{"role": "user", "content": "Hi"}}, | |
| } | |
| _, headers, _, err := createTestTranslator().TranslateRequest(body) | |
| require.NoError(t, err) | |
| assert.Len(t, headers, 3) | |
| assert.Contains(t, headers, ":path") | |
| assert.Contains(t, headers, "content-type") | |
| assert.Contains(t, headers, "host") | |
| } |
| func TestTranslateResponse_StreamingChunk(t *testing.T) { | ||
| body := map[string]any{ | ||
| "id": "chatcmpl-abc123", | ||
| "object": "chat.completion.chunk", | ||
| "created": 1700000000, | ||
| "model": "nvidia.nemotron-nano-12b-v2", | ||
| "choices": []any{ | ||
| map[string]any{ | ||
| "index": 0, | ||
| "delta": map[string]any{ | ||
| "content": "Hello", | ||
| }, | ||
| "finish_reason": nil, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| translatedBody, err := createTestTranslator().TranslateResponse(body, "nvidia.nemotron-nano-12b-v2") | ||
| require.NoError(t, err) | ||
| assert.Nil(t, translatedBody) | ||
| } |
There was a problem hiding this comment.
doesn't seem to be testing something new. can be removed.
| func TestProviderName(t *testing.T) { | ||
| assert.Equal(t, "awsbedrock-openai", provider.AWSBedrockOpenAI) | ||
| } |
There was a problem hiding this comment.
remove please. not relevant
| func TestProviderName(t *testing.T) { | |
| assert.Equal(t, "awsbedrock-openai", provider.AWSBedrockOpenAI) | |
| } |
| func TestGetRegion(t *testing.T) { | ||
| translator := NewBedrockTranslatorWithRegion("eu-west-1") | ||
| assert.Equal(t, "eu-west-1", translator.GetRegion()) | ||
| } | ||
|
|
||
| func TestNewBedrockTranslatorWithRegion_EmptyRegion(t *testing.T) { | ||
| translator := NewBedrockTranslatorWithRegion("") | ||
| assert.Equal(t, "us-east-1", translator.GetRegion()) // Should default | ||
| } No newline at end of file |
There was a problem hiding this comment.
remove pls:
| func TestGetRegion(t *testing.T) { | |
| translator := NewBedrockTranslatorWithRegion("eu-west-1") | |
| assert.Equal(t, "eu-west-1", translator.GetRegion()) | |
| } | |
| func TestNewBedrockTranslatorWithRegion_EmptyRegion(t *testing.T) { | |
| translator := NewBedrockTranslatorWithRegion("") | |
| assert.Equal(t, "us-east-1", translator.GetRegion()) // Should default | |
| } |
| func TestNewBedrockTranslator(t *testing.T) { | ||
| translator := NewBedrockTranslator() | ||
| assert.NotNil(t, translator) | ||
| assert.Equal(t, "us-east-1", translator.GetRegion()) // Default region |
There was a problem hiding this comment.
remove:
| assert.Equal(t, "us-east-1", translator.GetRegion()) // Default region |
nirrozenbaum
left a comment
nirrozenbaum
left a comment
There was a problem hiding this comment.
@srampal thanks.
I left minor comments.
most is about cleanup of unnecessary things.
otherwise looks good.
|
@srampal: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
|
@nirrozenbaum As discussed offline, this PR will be merged into the other repo (ai-gateway-payload-processing). Have made all the changes there so leaving this PR as -is for now. Let us review and merge in that other repo, after which we can close this PR here or update here again if needed. |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
2 participants
…port
Description
Note: The goal here is not to support the Bedrock Converse or Invoke APIs, not the openai responses api. Limiting this PR to just the direct OpenAI endpoint within Bedrock and only the chat completions api for a simple initial implementation. More api translation and interworking combinations can be added in future.
How Has This Been Tested?
All Unit tests pass. Additional e2e/ manual testing to be performed for full verification.
Merge criteria: