Repository navigation
Conversation
aget_text_embedding_batch fans out per-text and per-sub-batch calls with plain asyncio.gather, which propagates the first exception but leaves the other tasks running detached. Nothing awaits or cancels them, so a single rejected text orphans in-flight embedding requests for every sibling: the provider has already done billable work whose result is discarded. Add gather_cancel_on_error to async_utils: it futures the coroutines, gathers them, and on any failure cancels the survivors and drains them before re-raising. Use it in BaseEmbedding and BaseSparseEmbedding at both fan-out levels, including the tqdm progress path, which has no return_exceptions switch of its own. Adds regressions for dense and sparse embeddings at both levels plus a direct helper test; the tests fail on the previous code and pass with this change.
The show_progress branch goes through tqdm_asyncio.gather, which has no return_exceptions switch, so it needs its own cancel-and-drain coverage. Both the failure and the success case are pinned here.
6 of 14 tasks
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.
What this PR does
BaseEmbedding.aget_text_embedding_batchfans out embedding work at two levels and both used a bareasyncio.gather:_aget_text_embeddings;aget_text_embedding_batch.asyncio.gatherpropagates the first exception to the caller but leaves the remaining tasks running detached: nothing awaits them and nothing cancels them. For embeddings that is worse than a plain crash, because the provider has already done billable work on the sibling requests, and that work — and its vectors — is silently discarded. Any integration that relies on the default fan-out instead of a native batch call inherits both gaps.This PR adds
gather_cancel_on_errortollama_index.core.async_utilsand uses it at both levels inBaseEmbeddingandBaseSparseEmbedding:Cancelling and then draining before re-raising keeps the original exception as the one the caller sees, and guarantees no orphaned coroutine survives the failure.
BaseExceptionis caught (not justException) so an outer cancellation of the whole call also cleans up its children instead of abandoning them.The
show_progress=Truebranch goes throughtqdm_asyncio.gather, which has noreturn_exceptionsswitch (it is built onas_completed), so that branch collects the tasks first and applies the same cancel-and-drain on failure.Scope
_aget_text_embeddings(both dense and sparse) and the sub-batch fan-out inaget_text_embedding_batch(both dense and sparse).run_jobs/num_workers > 1is untouched: that helper manages its own worker pool and is used in many other places, so changing it belongs in a separate PR.Tests
New file
llama-index-core/tests/embeddings/test_async_batch_cancellation.pycovers dense and sparse embeddings at both fan-out levels plus the helper itself, asserting that the failing text propagates its exception while every sibling reportsCancelledErrorinstead of completing.pytest tests/embeddings/test_async_batch_cancellation.py -q(with this change)8 passed5 failed, 3 passed(siblings complete instead of being cancelled, including on theshow_progresspath)pytest tests/embeddings tests/base -q122 passed, 4 pre-existing errors from a Windows temp-dir permission issue unrelated to this changeruff check/ruff format --checkon the changed filesRegression scope: I ran
pytest tests/embeddings tests/ingestion tests/indices -qwith and without the source change and diffed the failing-test sets. The pre-existing failures are identical in both runs (missing optional dependencies and Windows temp-dir permissions in this environment); the only delta is the five new regressions, which fail without the change and pass with it. No test that passes onmainstarts failing here.Package version bumped to
0.14.26.AI assistance disclosure
Prepared with an AI coding agent, per the repository's guidelines. I reviewed the diff, wrote the regressions first and confirmed they fail without the source change, and ran the test and lint commands above.
Refs #22312