feat(py/plugins/amazon-bedrock): add embedders (Titan, Cohere v3, Nova) - #6010
feat(py/plugins/amazon-bedrock): add embedders (Titan, Cohere v3, Nova)#6010hilariie wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements Amazon Bedrock embedders using the InvokeModel API for the Genkit Python plugin, porting functionality from the Go plugin with support for Titan, Cohere, and Nova embedding models. The review feedback highlights opportunities to improve robustness and performance, including adding defensive checks for media URLs to prevent potential AttributeError crashes, explicitly rejecting unsupported remote image URLs instead of silently skipping them, optimizing Cohere batch processing to run chunks concurrently using asyncio.gather, and evaluating the fail-fast behavior of concurrent document embedding.
974de8f to
b9e26a2
Compare
b9e26a2 to
af83c8c
Compare
af83c8c to
4c81831
Compare
cabljac
left a comment
There was a problem hiding this comment.
Embedders look good, the per-family split reads clearly and the batching and cancellation behaviour is well covered by the tests. Two nits inline, neither blocking.
|
Both issues fixed Transport now isinstance-checks the parsed body so a non-object raises INTERNAL instead of an AttributeError downstream, docstring updated to match. For resolve I added looks_like_embedding_model, an 'embed' substring check, and swapped both guards to it. Unknown embedding families now fail UNIMPLEMENTED by name. is_embedding_model still gates what gets listed. |
…a-2) Embedding models go through InvokeModel with a per-provider JSON body, so routing is by model ID. Diverges from the Go plugin in a few places; see the PR body.
…the first failure
json.loads can hand back a list or a scalar, and the four payload.get() callers would raise AttributeError on it instead of a GenkitError.
…he chat path An embedding ID from a family with no entry in the routing table used to resolve as a Converse model and fail with a Bedrock validation error.
568de9b to
fa0e6b6
Compare
Why
Slice 5 of #5820: embedders. Stacked on #5970. Embedding isn't part of Converse (bedrock-runtime has no embedding operation), so embedders go through InvokeModel with a raw per-provider JSON body. So this slice routes by model ID instead of reusing the Converse converters.
Changes
New
embedders.py: Titan text (one call per document), Titan multimodal (text and/or image in one call), Cohere v3 (text batched 96 per call) and Nova-2, plusinvoke_modelon the transport seam. Per-document calls fan out under a semaphore of 10 and reassemble by index; a batch cancels its outstanding calls on the first failure, matching Go'scontext.WithCancel, and reports the lowest-indexed failure among the calls that ran.resolve()gains an embedder branch and a cross-guard, so an embedding model ID no longer resolves as a Converse chat model, andlist_actionsonly lists whatresolvecan serve.Four divergences from the Go plugin, all checked against AWS:
amazon.nova-2-multimodal-embeddings-v1:0SINGLE_EMBEDDING schema. Go registersamazon.nova-embed-text-v1:0, which doesn't exist, sends a Titan-shaped body, and routes on a substring that can't match the real ID.imagesfield for v3, butget-foundation-modelreportsinputModalities: [TEXT]and a live image call returnsInvalid parameter combination. Go's image path can't work whatever the encoding.cohere.embed, not barecohere, which would swallowcohere.rerank-*andcohere.command-*.messageis raised. Bedrock reports input errors there on a 200, and Go drops it.cohere.embed-v4is recognised and fails UNIMPLEMENTED: different request schema, and where Cohere image embedding will land. Per-request options aren't wired yet, so Cohere is pinned toinput_type: search_document.Verification
Unit tests pin each family's request body, Cohere's 96-document chunking, ordering when calls finish out of order, and the concurrency cap. Live tests (
BEDROCK_LIVE_TESTS=1) hit all six models against real Bedrock, Nova-2 in us-east-1. The sample app gainsembed_text,embed_batch,embed_similarityandembed_image.Worth a closer look
invoke_modelis new in this slice and slice 6 will reuse it, so flagging it while it is still cheap to change.It returns the parsed body: the boto3 call, the
body.read()and thejson.loadsall happen inside oneto_threadhop. The read has to be off the loop regardless since botocore classes InvokeModel as a streaming-output operation, so it hands back an unreadStreamingBodyandread()blocks on the socket. Keeping the parse in that same hop means the decode stays off the loop too, which is cheap for a 1 KB vector but less so for the base64 image bodies slice 6 pulls back.The tradeoff is that the codec sits on both sides of the seam:
embedders.pydoes thejson.dumpsgoing in, transport does thejson.loadscoming out. Would like your read on whether that is the right home for the parse. Worth stating that there is no Go precedent to lean on here as aws-sdk-go-v2 materialises the InvokeModel body as[]bytebefore the caller sees it, so Go has no read to place and no seam to place it across. Would like your read on whether that is the right home for the parse.Screenshots