Fix partial failures in AWS OpenSearch bulk inserts - #855
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: norrishuang The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| ) | ||
| total_inserted += inserted | ||
| if error is not None: | ||
| raise error |
There was a problem hiding this comment.
vectordb_bench/backend/clients/aws_opensearch/aws_opensearch.py line:303
Medium ---- _insert_with_single_client now raises OpenSearchBulkInsertError instead of returning (total_inserted, error), which bypasses the framework contract that VectorDB.insert_embeddings returns tuple[int, Exception] (api.py) and that concurrent_runner._insert_batch_with_retry uses getattr(error, "non_retryable", False) on the RETURNED error to short-circuit retries. The newly declared non_retryable=True flag is therefore never consulted, partial insert counts are lost to the runner, and this diverges from the OSS sibling client, which returns (count, error) from its insert path. Suggest returning (total_inserted, error) and letting the runner decide, or documenting why raising is required.
There was a problem hiding this comment.
Fixed in 1a89780. _insert_with_single_client now returns (total_inserted, error) again, preserving the VectorDB contract and the partial count. OpenSearchBulkInsertError.non_retryable is now consumed by the runner as intended.
| try: | ||
| self.client.bulk(body=insert_data) | ||
| total_inserted += len(batch_embeddings) | ||
| response = client.bulk(body=pending_data) |
There was a problem hiding this comment.
vectordb_bench/backend/clients/aws_opensearch/aws_opensearch.py line:358
Medium ---- On a request-level exception the entire pending_data is retried even though the server may already have applied part of the batch (e.g. a timeout after server-side processing). For OpenSearch Serverless the index action carries no _id (auto-generated), so re-sending an already-applied document creates a duplicate with a different _id but the same _source.id, inflating index counts and corrupting recall - exactly the data-integrity problem this PR is meant to fix. Consider documenting the ambiguity or making the retry idempotent for the auto-_id path.
There was a problem hiding this comment.
Fixed in 1a89780. For the AOSS auto-ID path, request-level HTTP 429 remains retryable because the request was explicitly rejected, while ambiguous failures such as connection timeouts now return a non-retryable error without resending the batch. Response-level item failures are still retried selectively. Provisioned paths with deterministic IDs retain request retries.
| log.error(str(error)) | ||
| return total_inserted, error | ||
|
|
||
| error = OpenSearchBulkInsertError(f"Bulk insert for {context} exhausted its retry loop") |
There was a problem hiding this comment.
vectordb_bench/backend/clients/aws_opensearch/aws_opensearch.py line:412
Low ---- The trailing error = OpenSearchBulkInsertError("...exhausted its retry loop") followed by return total_inserted, error is unreachable: every iteration of the for attempt in range(1, BULK_MAX_ATTEMPTS + 1) loop returns (or continues) on all 30 iterations, so control never falls through to this block. Remove the dead code or make the loop-exit the single error-construction point.
There was a problem hiding this comment.
Fixed in 1a89780. The unreachable fallback was removed. Terminal request/item failures now set final_error, break the loop, and use one reachable error logging/return point after the loop.
| except Exception as retry_e: | ||
| log.warning(f"Retry failed for batch: {retry_e!s}") | ||
| return total_inserted, retry_e | ||
| request_error = e |
There was a problem hiding this comment.
vectordb_bench/backend/clients/aws_opensearch/aws_opensearch.py line:360
Low ---- The request-exception retry branch (client.bulk raising, lines 357-382) has no test coverage; all six new tests exercise response-level errors only. A test that makes client.bulk raise on the first N attempts would lock in the backoff and counting behavior of this branch, which is the most common real-world failure mode (timeouts, connection errors).
There was a problem hiding this comment.
Fixed in 1a89780. Added request-exception coverage for two request-level 429 failures followed by success (including 2s/4s backoff assertions), plus an AOSS connection-timeout case that verifies the ambiguous auto-ID request is not resent. The focused test file now has 8 passing tests.
3 participants
What changed
VectorDB.insert_embeddingscontractOpenSearchBulkInsertErrorwhen any documents remain rejected after all attemptslabels_datawhen falling back to a single clientWhy
The OpenSearch bulk API can return HTTP 200 while individual documents fail with
errors: true. The previous implementation counted the entire batch as inserted without inspecting item statuses, allowing incomplete datasets to be used for benchmark and recall results.For OpenSearch Serverless, response-level item failures and request-level HTTP 429 responses are safe to retry. Other request exceptions can have an ambiguous outcome because AOSS uses auto-generated document IDs; those failures are returned as non-retryable instead of risking duplicate documents.
Retry timing
With 30 total attempts there are at most 29 waits. The
2, 4, 8, 16, 32, 60...schedule covers about 25 minutes of sustained throttling before the operation fails hard.Testing
python3 -m pytest tests/test_aws_opensearch.py -q(8 passed)black --checkon the changed implementation and testsruff checkon the changed implementation and tests