Repository navigation
fix(bedrock/rerank): populate document text when return_documents is true - #38007
AbhAy120204 wants to merge 2 commits into
Conversation
Greptile SummaryThe PR propagates the original rerank documents and
Confidence Score: 4/5The PR appears safe to merge, with only non-blocking repository-convention cleanup needed in the transformer and tests. The synchronous and asynchronous Bedrock paths consistently propagate the new inputs and correctly backfill valid string documents; the remaining concern is avoidable mutation and class-level test patching. Files Needing Attention: litellm/llms/bedrock/rerank/transformation.py; tests/test_litellm/llms/bedrock/rerank/test_bedrock_rerank_return_documents.py
|
| Filename | Overview |
|---|---|
| litellm/llms/bedrock/rerank/handler.py | Threads documents and the effective return-documents setting through both Bedrock response paths without leaving an inconsistent in-repository caller. |
| litellm/llms/bedrock/rerank/transformation.py | Backfills string documents safely by validated result index, but uses mutable construction contrary to repository conventions. |
| tests/test_litellm/llms/bedrock/rerank/test_bedrock_rerank_return_documents.py | Covers enabled, disabled, and asynchronous behavior, while relying on class-level monkeypatching rather than dependency injection. |
Reviews (1): Last reviewed commit: "fix(bedrock/rerank): populate document t..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
a2553b8 to
366e8f2
Compare
|
The code-quality failure is on the pre-existing check_workflow_startup_safety step which is failing on litellm_internal_staging itself (the .github/workflows/test-unit.yml file has an unrelated job-timeout vs pytest-timeout mismatch introduced by upstream commits). This PR touches no workflow files. |
Two cases @kimnamu probed on BerriAI#38006 while comparing this PR with BerriAI#38007, now pinned so neither can regress silently: - A dict document with no `text` key reaches Bedrock as a `jsonDocument` and `RerankResponseDocument` carries only `text`, so there is nowhere to put it. No back-fill, but no crash: the caller still gets index and score. This is the one case where the reported `KeyError: 'document'` survives, so it is a documented boundary rather than an oversight. - A non-integer `index` is tolerated rather than raising, matching main's behaviour (pydantic coerces "2" to 2 upstream). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…true Bedrock's rerank API only returns index and relevanceScore, never the document text. When return_documents is true (the default), LiteLLM's Cohere-compatible surface must back-fill each result's document.text from the original input documents by index. The response transformer was ignoring return_documents entirely, so document was always absent. Mirrors the existing HuggingFace rerank config which handles the same provider limitation the same way. Fixes BerriAI#38006
366e8f2 to
5a64738
Compare
…e tests - Restore arerank's original positional signature (prepared_request, timeout, client) and add documents/return_documents as optional kwargs, so downstream code calling arerank directly is unaffected - Pin the three defensive scenarios reviewers raised: dict documents without a text key, non-integer index, out-of-range index. Each produces a result with only index/score and never raises.
|
Thanks @AbhAy120204 for turning both of those around the same morning, and for pinning them with tests rather than only fixing them. I re-ran
One thing landed underneath since then. #35419 ( Keeping all three kwargs is the whole resolution. On that merged tree (staging
The end-to-end shape is unchanged after the merge: One consequence of #35419 before you resolve: the pre-fix positional arity is already gone upstream. After the merge the signature is Merged-tree probe output |
TLDR
Problem this solves:
return_documents— results never carrydocument.textreturn_documents=True) get a KeyError onresults[i]["document"]How it solves it:
documentsandreturn_documentsinto the response transformerdocument.textfrom the original input list by index, same as the HuggingFace rerank configUser Flow
Before: a RAG pipeline using Bedrock rerank to retrieve ranked source passages gets no document text back
litellm.rerank(model="bedrock/arn:...", query="...", documents=["doc 0", "doc 1", ...], return_documents=True)results: [{"index": 1, "relevance_score": 0.95}], nodocumentfieldresults[0]["document"]["text"]and getKeyError: 'document'After: the same call returns document text in each result
results: [{"index": 1, "relevance_score": 0.95, "document": {"text": "doc 1"}}]results[0]["document"]["text"]returns the text of the highest-ranked source documentRelevant issues
Fixes #38006
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/llms/bedrock/rerank/ -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
This is a pure SDK transformation bug: Bedrock's API only returns
{index, relevanceScore}and never echoes document text. The fix is client-side back-fill by index, confirmed by the regression test below. No live Bedrock call is needed to reproduce or verify — the transformer is fully exercisable without a provider.Shared setup:
uv run pytest tests/test_litellm/llms/bedrock/rerank/test_bedrock_rerank_return_documents.py -vBefore (ff02d5c)
FAILED test_bedrock_rerank_return_documents_true_populates_document_textAssertionError: return_documents=True must populate document, got {'index': 2, 'relevance_score': 0.95}FAILED test_bedrock_rerank_return_documents_true_asyncKeyError: 'document'2 failed, 1 passedAfter (8fb4545)
PASSED test_bedrock_rerank_return_documents_false_omits_document_fieldPASSED test_bedrock_rerank_return_documents_true_asyncPASSED test_bedrock_rerank_return_documents_true_populates_document_text3 passed in 1.08sType
🐛 Bug Fix
Caveats (if any)
documentslist. This matches the existing HuggingFace rerank config which handles the same provider limitation the same way (litellm/llms/huggingface/rerank/transformation.py:234-247)/v1/rerankroutes througharerankwherereturn_documentsdefaults toNone→ back-fill on, so callers who never set the flag will start getting document text. For 50 documents of ~1 KB passages the serialized response goes from ~2 KB to ~54 KB. Worth a release notetextkey are sent to Bedrock asjsonDocument, andRerankResponseDocumentcarries onlytext. Those results have no back-fill target and come back with onlyindexandrelevance_score(no crash). The HuggingFace rerank config has the same limitationFinal Attestation