feat: bedrock - support Titan image multimodal embedding model#3637
feat: bedrock - support Titan image multimodal embedding model#3637serhiimeverc wants to merge 7 commits into
Conversation
…dels Promotes inputImage to a first-class field on BedrockTitanEmbeddingRequest and routes amazon.titan-embed-image-* and amazon.nova-2-multimodal-embeddings-* through the Titan converter. Accepts image-only requests when Input is nil.
📝 WalkthroughWalkthroughAdds Titan multimodal embedding support: makes Bedrock Titan embedding accept optional text or base64 image input, recognizes amazon.titan-embed-image models, implements presence validation and inputImage lifting/filtering in request conversion, and adds tests covering new behaviors. ChangesBedrock Titan Multimodal Embedding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Confidence Score: 5/5Safe to merge — changes are narrowly scoped to Titan image embedding routing and serialization with no risk to existing text-only or Cohere paths. The routing addition is a simple string-contains check that returns the same 'titan' value as the text case, leaving all previously working paths untouched. The No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "[fix] core/bedrock - require non-empty s..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/providers/bedrock/embedding.go`:
- Around line 17-24: The current validation treats inputImage as present if the
key exists in bifrostReq.Params.ExtraParams but later only accepts string
values, which can let non-usable inputs pass; update the checks around
bifrostReq.Params.ExtraParams["inputImage"] (and the subsequent extraction logic
referenced in the same file) to require the value is a non-empty string (type
assert to string and ensure len>0) before setting hasImage or using it as the
Titan input, and treat any missing, non-string, or empty-string value as absent
so the "no input text or image provided for embedding" error is correctly
raised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32977117-e702-49cc-be50-e63fe5c90470
📒 Files selected for processing (3)
core/providers/bedrock/embedding.gocore/providers/bedrock/embedding_test.gocore/providers/bedrock/types.go
Greptile/CodeRabbit P1: hasImage was set on key existence alone, but the
lifting code only accepts string values. A non-string or empty inputImage
with no text passed validation, then produced an empty {} wire body to
AWS instead of a clean Bifrost-level error.
Type-assert string and require non-empty at the validation point so
non-string/empty values are treated as absent, matching the lifting
contract. Adds tests for image-only + non-string and image-only +
empty-string rejection paths.
Summary
Bedrock embedding model detection only recognized Titan text and Cohere, causing the Titan image embedding model to be rejected as unsupported. Additionally, the existing
inputImagepassthrough was carried inExtraParams(taggedjson:"-") and only reached the wire body when the caller setBifrostContextKeyPassthroughExtraParams— which the Bedrock provider never sets — so image-only requests silently shipped withoutinputImageto AWS.This PR extends model-type routing so Titan image embeddings reach the Titan request path, promotes
inputImageto a first-class field so the wire body always includes it, and adjusts validation so image-only requests are accepted.Changes
Model routing updates (
embedding.go)DetermineEmbeddingModelTypenow mapsamazon.titan-embed-image*→titan.inputImagepromoted to first-class field (types.go,embedding.go)InputImagefield withjson:"inputImage,omitempty"onBedrockTitanEmbeddingRequest.InputTextswitched toomitempty(image-only requests legitimately have no text).Params.ExtraParams["inputImage"]into the typed field when it's a string. Non-string values stay inExtraParamsfor backward-safe passthrough.ExtraParamsforwarding loop alongsidenormalize.Validation accepts image-only requests (
embedding.go)Input == nilguard rejectedParams.ExtraParams["inputImage"]before thehasImagecheck could run.Input.Text/Input.Texts.Targeted unit coverage (
embedding_test.go)DetermineEmbeddingModelType: titan text / titan image / cohere / unsupported.TestToBedrockTitanEmbeddingRequest_ImageOnly: emptyInput+ image produces wire body withinputImageand withoutinputText.TestToBedrockTitanEmbeddingRequest_TextAndImage: both fields appear in the wire body.TestToBedrockTitanEmbeddingRequestLiftsInputImageAndNormalize: bothExtraParamskeys are lifted to typed fields.TestToBedrockTitanEmbeddingRequest_NonStringInputImage: non-string values stay inExtraParams(backward-safe passthrough).TestToBedrockTitanEmbeddingRequest_NilInputWithImage: nilInput+ image succeeds with the expected wire body.TestToBedrockTitanEmbeddingRequest_NilInputWithoutImage/_RejectsNoInput: unified error for no-input cases.Notes
{"inputText":"..."}exactly as before.BedrockToolUse(struct tag column re-alignment) is agofmtside-effect of adding the longerInputImagecomment in the same file — column alignment is context-sensitive. Reverting would re-introduce agofmt -lfailure.amazon.nova-*-multimodal-embeddings-v1:0) are intentionally out of scope — they use amessages-array envelope (similar to Converse), not the flat Titan shape, so they need their own request struct, converter, and response parser. Tracked separately.Type of change
Affected areas
How to test
Expected: all cases pass, including titan image routing, image-only request acceptance, nil-Input handling, and
inputImagefirst-class field lifting.Breaking changes
Related issues
Closes #3632.
Security considerations
No new auth surfaces or secret handling changes. Change is limited to model-name routing, request serialization, and unit coverage.
Checklist
docs/contributing/README.mdand followed the guidelines