fix(search): batch missing-embedding calls to avoid API batch-size limits#1542
Closed
octo-patch wants to merge 1 commit into
Closed
fix(search): batch missing-embedding calls to avoid API batch-size limits#1542octo-patch wants to merge 1 commit into
octo-patch wants to merge 1 commit into
Conversation
…mits When _extract_embeddings encounters many documents without cached embeddings it previously called embedder.embed(all_missing) in one shot. Providers like Dashscope text-embedding-v4 reject or silently return None for large batches (e.g. 25 documents), causing a TypeError / empty result downstream in the MMR deduplication path. Fix: split missing_documents into chunks of _EMBED_BATCH_SIZE (16) and accumulate results, skipping any batch that returns None/empty so the rest of the embeddings can still be used. Fixes MemTensor#1482 Co-Authored-By: Octopus <liyuan851277048@icloud.com>
Collaborator
|
Automated Test Results: PASSED Cloud test-engine rerun against
Manual code review is still required before merge. |
Collaborator
|
Thanks for the contribution. This PR addresses the same issue as #2041 ( We are going to continue the review in #2041 because it uses a smaller batch size, preserves per-batch index mapping, and includes a regression test for the missing-embedding batching behavior. To keep the review queue focused, I’m closing this PR as a duplicate of #2041. We can carry any useful hardening ideas from this PR into the #2041 review if needed. |
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.
Fixes #1482
Problem
_extract_embeddingsinsearch_handler.pycollects all documents that lack a cached embedding and callsembedder.embed(all_missing)in a single shot. Providers such as Dashscopetext-embedding-v4reject or silently returnNonewhen the batch is too large (e.g. 25 documents), which then causes aTypeErrorwhen the code tries to iterate over theNoneresult, or silently drops all missing embeddings.The warning message before this:
followed by a crash or silent failure in the MMR deduplication path.
Solution
Split
missing_documentsinto chunks of_EMBED_BATCH_SIZE(16) and callembed()for each chunk, extending a combined result list. Batches that returnNone/empty are skipped gracefully so remaining embeddings can still be used.Testing
TypeError; now completes successfully using 2 batches of 16 and 9.