Fix merge_embed_responses dropping embedding types absent from first batch - #811
Open
Sehastrajit-S wants to merge 1 commit into
Open
Sehastrajit-S wants to merge 1 commit into
Sehastrajit-S wants to merge 1 commit into
Conversation
…batch merge_embed_responses() determined which embedding fields to merge by inspecting only the first EmbeddingsByTypeEmbedResponse. If a later batch returned an embedding type (e.g. int8) that the first batch didn't include, those values were silently dropped from the merged result instead of being preserved. Fixes cohere-ai#796.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
merge_embed_responses()decided which embedding fields to merge by inspecting only the firstEmbeddingsByTypeEmbedResponsein the list. If a later batch returned an embedding type (e.g.int8) that the first batch didn't include, those values were silently dropped from the merged result instead of being preserved.Fixes #796.
Change
In
src/cohere/utils.py, the field-selection check now looks across all batches (any(... for embedding_by_type in embeddings_by_type)) instead of only the first one, so a type present in any batch survives the merge.Test plan
test_merge_embeddings_by_type_preserves_type_absent_from_first_responseintests/test_embed_utils.py, mirroring the repro from Batched embeddings drop types absent from the first response #796.poetry run pytest(embedding utils tests + full non-network suite): all passing.poetry run ruff check/poetry run mypyclean on the touched files.Note
Low Risk
Small, targeted change to batch merge logic for embeddings-by-type, with a regression test; affects embed client batching behavior when types differ across batches.
Overview
Fixes
merge_embed_responsesso embedding-by-type merges no longer drop types that appear only in later batches.Field selection used to depend on the first response only; it now keeps any field that is non-
Noneon at least one batch when building the mergedembeddings_by_typeresult (e.g.int8from a second batch is preserved even if the first batch had onlyfloat_).Adds
test_merge_embeddings_by_type_preserves_type_absent_from_first_responseto lock in that behavior (issue #796).Reviewed by Cursor Bugbot for commit bb499c7. Bugbot is set up for automated code reviews on this repo. Configure here.