feat: add OpenAI to AWS Bedrock embeddings translation#1969
feat: add OpenAI to AWS Bedrock embeddings translation#1969Flgado wants to merge 5 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Joao Folgado <jfolgado94@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for translating OpenAI embedding requests to AWS Bedrock's Titan embedding models. The changes are well-structured, following existing patterns for translators and including comprehensive test coverage. The new request/response schemas, translator logic, and endpoint wiring are all correctly implemented.
I have one suggestion regarding the error handling logic for passthrough errors to ensure consistency and prevent potential loss of header information. Otherwise, the implementation is excellent.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1969 +/- ##
==========================================
+ Coverage 84.21% 84.38% +0.16%
==========================================
Files 128 129 +1
Lines 17832 18030 +198
==========================================
+ Hits 15018 15215 +197
Misses 1871 1871
- Partials 943 944 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…span recording test and error path coverage. Signed-off-by: Joao Folgado <jfolgado94@gmail.com>
Signed-off-by: Joao Folgado <jfolgado94@gmail.com>
Signed-off-by: Joao Folgado <jfolgado94@gmail.com>
Description
This PR adds translation support for the OpenAI
/v1/embeddingsendpoint targeting AWS Bedrock Titan Embed Text models (amazon.titan-embed-text-v1and `amazon.titan-embed-text-v2:0).It follows the same patterns established by the existing openai_awsbedrock.go translator (JSON decoding strategy, error handling, path header construction via url.PathEscape, span recording).
Changes:
TitanEmbeddingRequestand `TitanEmbeddingResponse schema structs covering both Titan v1 and v2 fields (InputText, dimensions, normalize, embeddingTypes).NewEmbeddingOpenAIToAWSBedrockTranslatorinto the APISchemaAWSBedrock embedding path.What is NOT supported in this PR:
Special notes/questions for reviewers
Two design decisions I'd like maintainer input on before considering follow-up work:
1. Should normalize be user-configurable via vendor fields?
Currently normalize (Titan v2 only) is always left unset, using Titan's default (true). The GCP embeddings translator exposes a similar per-provider option via
GCPVertexAIEmbeddingVendorFields.TaskType. Should we follow the same pattern and add anAWSBedrockTitanEmbeddingVendorFields struct with normalize? Or is leaving it as the default acceptable. Same question for the embeddingsByType.2. Should embeddingsByType be surfaced in the response?
Titan v2 can return quantized binary vectors under embeddingsByType alongside the standard float embedding. This PR always discards it and returns only the float array. If the community wants to expose this, it would require either a vendor extension on EmbeddingResponse or a new field there is no OpenAI equivalent. Should this be in scope, and if so, what is the preferred approach?
Amazon Titan Embeddings docs for reference: https://docs.aws.amazon.com/bedrock/latest/userguide/model-parameters-titan-embed-text.html