Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces structured configuration schemas (EmbeddingConfigSchema and VertexEmbeddingConfigSchema) using Pydantic for Google GenAI and Vertex AI embedders. It also implements batching for text embedding requests and updates multimodal embedding to process multiple documents sequentially rather than rejecting them. The feedback recommends executing both text embedding batches and multimodal embedding requests concurrently using asyncio.gather with a semaphore to reduce latency, which also requires importing asyncio.
cabljac
left a comment
There was a problem hiding this comment.
The :predict request_dict assertions and the reverse-completion ordering test are the right shape. One fix, rest inline.
_run_bounded doesn't cancel its children when the outer generate() is cancelled. asyncio.wait doesn't propagate, so on abort or timeout the in-flight calls keep running and billing. The FIRST_EXCEPTION path is fine, it's just the outer-cancel case. BaseException handler, cancel pending, gather with return_exceptions, re-raise. Floor is 3.10 so no TaskGroup. Or drop the concurrency, which #6187 doesn't ask for anyway, and it goes away.
One that can't anchor inline: google.py:533 still types config as dict[str, object] while this PR exports the two schemas, so passing a typed instance is a pyright error even though _parse_options handles it. The dict typing predates this (#6104), the mismatch doesn't.
Also, not this PR: Go batches gemini-embedding-001 at 250 (embedder.go:35) against the Vertex doc's "each request can only include a single input text". Your 1 is right, Go's wrong.
…lled Also warn when a Gemini API embedder drops mimeType or autoTruncate.
|
Resolved all comments in a256a5a |
cabljac
left a comment
There was a problem hiding this comment.
The cancellation fix and its test are right, and the dropped-option warning is what I was after. One item from the last round is still open, it wasn't inline-anchored so easy to miss. Two nits inline.
google.py:533 still types config as dict[str, object] | None. Passing an EmbeddingConfigSchema instance is a pyright error even though _parse_options accepts it. EmbedderRef.config is Any, so widening to EmbeddingConfigSchema | dict[str, object] | None is enough. Same type on family_embedder_ref in _model_refs.py:110.
| extra = options.model_extra or {} | ||
| dropped = [name for name in VERTEX_ONLY_OPTION_NAMES if name in extra] | ||
| if dropped: | ||
| logger.warning( |
There was a problem hiding this comment.
nit: this fires on every generate(). An indexing loop with mimeType in the ref config emits one warning per call. Warn-once per embedder instance would be quieter. Fine to leave if you prefer.
There was a problem hiding this comment.
Leaving this one. _create_embedder_action builds a fresh Embedder inside _run on every request, so a per-instance flag would still warn every call. Happy to add a process-wide once if you would rather have it.
|
Sorry, missed that one last round. Done in |
Fixes #6187
Summary
Embedder options are validated through a typed schema that accepts both snake_case and camelCase keys (
taskType/task_type,outputDimensionality/output_dimensionality,title,version), tolerates unknown keys, and is advertised ascustomOptionsper backend. Vertex embedders additionally acceptmimeTypeandautoTruncate. Invalid options raiseGenkitErrorwithINVALID_ARGUMENTnaming the field.Requests are chunked and reassembled in input order: 100 contents per call on the Gemini API, 250 on Vertex text-embedding models, and 1 on Vertex
gemini-*models, which accept a single input text per request. The multimodal path sends one:predictcall per document instead of rejecting multi-document requests.EmbeddingTaskTypegainsCODE_RETRIEVAL_QUERY.