Repository navigation
Conversation
… set
Bedrock's rerank API returns only `index` and `relevanceScore` and never
echoes document text. LiteLLM exposes a Cohere-compatible interface where
`return_documents` means the transformer back-fills `document.text` from
the request's documents by index, but the Bedrock transformer ignored the
parameter entirely — so `results[i]["document"]` was always absent and
callers hit `KeyError: 'document'` no matter what they passed.
Thread the `RerankRequest` into `_transform_response` (sync and async
paths) and back-fill by index, the same way the HuggingFace rerank config
already handles this identical provider limitation. Both `str` and
`{"text": ...}` document shapes are supported. `request_data` is optional,
so an existing caller passing only the response keeps its behaviour.
Fixes BerriAI#38006
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ype gate The first cut accumulated results with `_results = []` plus `.append`, and defaulted the documents list to a `[]` literal. Both are mutable-collection constructions (LIT002), so the type-discipline gate counted two new violations against the budget. Build the list in one shot from a comprehension over a per-result helper, and let `None` — rather than an empty list — mean "nothing to back-fill from", which folds the opt-out and the no-request case into one check. Annotate the new helpers with read-only views (Sequence/Mapping) rather than list/dict so they do not add LIT001 either. Behaviour is unchanged; the tests are untouched and still pass. transformation.py now sits below its base count on every gated rule (LIT001 5<6, LIT002 5<6, LIT010 3<4) and handler.py is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR combines a Bedrock rerank response fix with broader CI, telemetry, spend-log session, and Responses API streaming changes
Confidence Score: 4/5The streaming regression should be fixed before merging because slow providers can withhold every initial Responses API event until their first token Both iterator modes now consume the upstream stream before yielding response.created, exposing otherwise healthy long-time-to-first-token requests to client idle and first-byte timeouts Files Needing Attention: litellm/responses/litellm_completion_transformation/streaming_iterator.py, tests/test_litellm/llms/bedrock/rerank/test_bedrock_rerank_return_documents.py
|
| Filename | Overview |
|---|---|
| litellm/responses/litellm_completion_transformation/streaming_iterator.py | Adds response-ID priming and expanded event assembly, but now waits for provider output before emitting initial stream events |
| litellm/llms/bedrock/rerank/transformation.py | Correctly restores requested source text by validated Bedrock result index while preserving opt-out behavior |
| litellm/llms/bedrock/rerank/handler.py | Threads the normalized rerank request through both synchronous and asynchronous response transformation |
| litellm/integrations/otel/plumbing/routing.py | Extends tenant tracer cache identity with fixed-region endpoints and confines dynamic credentials to the owning exporter |
| litellm/proxy/utils.py | Adds an event-driven spend-log flush wake-up used to improve Responses API session lookup availability |
| .github/workflows/test-postgres.yml | Ports proxy behavior, security, and migration suites from removed CircleCI jobs into a GitHub Actions matrix |
Comments Outside Diff (1)
-
litellm/responses/litellm_completion_transformation/streaming_iterator.py, line 369-375 (link)Initial stream events are blocked
When a provider has a long time to first token,
_aprime_response_idwaits for its first chunk before emittingresponse.created, causing client first-byte or idle timeouts.
Reviews (1): Last reviewed commit: "fix(bedrock): back-fill rerank document ..." | Re-trigger Greptile
| @@ -0,0 +1,70 @@ | |||
| """Bedrock never echoes document text, so the transformer must back-fill it.""" | |||
There was a problem hiding this comment.
Regression test file is fragmented
Move this bug-fix coverage into the existing mapped transformation.py test file and remove redundant explanatory comments to keep maintenance localized.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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>
kimnamu
left a comment
There was a problem hiding this comment.
Thanks for pinning the jsonDocument and non-integer index boundaries in 4c9d7f6, and for folding the sync/async measurement into the description. I re-ran the branch on top of today's litellm_internal_staging (02522a54) and the fix still does exactly what it should; the only thing between this PR and green is a one-hunk conflict that landed after you opened it.
What changed underneath: #35419 (bb227420, Aug 25) threads logging_obj through BedrockRerankHandler.arerank on the same lines this PR threads request_data, so GitHub now reports mergeable_state: dirty (same for #38007). Keeping both kwargs is the whole resolution; the arerank signature itself merges cleanly (logging_obj positional after prepared_request, request_data stays the trailing kwarg):
# litellm/llms/bedrock/rerank/handler.py, after merging litellm_internal_staging
return self.arerank(
prepared_request,
logging_obj=logging_obj, # from #35419
request_data=request_data, # this PR
timeout=timeout,
client=client if client is not None and isinstance(client, AsyncHTTPHandler) else None,
)Tests on the merged tree, tests/test_litellm/llms/bedrock/rerank/:
| Tree | Result |
|---|---|
litellm_internal_staging @ 02522a54, no PR |
10 passed |
| PR 4c9d7f6 merged into staging | 19 passed |
same, with litellm/llms/bedrock/rerank/ reverted to staging (PR tests kept) |
8 failed, 11 passed |
ruff check + ruff format --check on the touched files |
clean |
The one PR test that passes without the fix is test_no_request_data_leaves_response_untouched, which is the intended no-op path.
End to end, only HTTPHandler.post / AsyncHTTPHandler.post patched (real SigV4 signing with dummy keys, return_documents not passed):
=== litellm_internal_staging @ 02522a54 ===
rerank [{"index": 1, "document": null}, {"index": 0, "document": null}]
arerank [{"index": 1, "document": null}, {"index": 0, "document": null}]
=== PR 4c9d7f65 merged into staging ===
rerank [{"index": 1, "document": {"text": "beta passage"}}, {"index": 0, "document": {"text": "alpha passage"}}]
arerank [{"index": 1, "document": {"text": "beta passage"}}, {"index": 0, "document": {"text": "alpha passage"}}]
So the arerank path (what the proxy's /v1/rerank goes through) still back-fills after #35419's logging_obj plumbing, and the sync and async entry points agree with the flag unset, as measured on the issue.
Repro script
import asyncio, json, httpx, litellm
from unittest.mock import patch
from litellm.llms.custom_httpx.http_handler import HTTPHandler, AsyncHTTPHandler
BODY = {"results": [{"index": 1, "relevanceScore": 0.9}, {"index": 0, "relevanceScore": 0.1}]}
def fake(*a, **k):
return httpx.Response(200, json=BODY, request=httpx.Request("POST", "https://bedrock-agent-runtime.us-east-1.amazonaws.com/rerank"))
async def afake(*a, **k):
return fake()
kw = dict(model="bedrock/arn:aws:bedrock:us-east-1::foundation-model/cohere.rerank-v3-5:0", query="q",
documents=["alpha passage", "beta passage"], aws_region_name="us-east-1",
aws_access_key_id="AKIAIOSFODNN7EXAMPLE", aws_secret_access_key="wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY")
with patch.object(HTTPHandler, "post", side_effect=fake), patch.object(AsyncHTTPHandler, "post", side_effect=afake):
s = litellm.rerank(**kw)
a = asyncio.run(litellm.arerank(**kw))
for name, r in (("rerank ", s), ("arerank", a)):
print(name, json.dumps([{"index": x["index"], "document": x.get("document")} for x in r.results]))Nothing else from my side; once the branch is rebased or merged with staging this looks ready for a maintainer pass.
Fixes #38006.
The bug
Bedrock's rerank API returns only
indexandrelevanceScore— it never echoesdocument text. LiteLLM exposes a Cohere-compatible interface where
return_documentsmeans the response transformer back-fillsdocument.textfrom the request's documents by index.
BedrockRerankConfig._transform_responsenever saw the request at all:so it could not back-fill even in principle, and
return_documentswas ano-op.
results[i]["document"]was always absent, and a RAG pipeline usingBedrock rerank had to re-join ranked indices to source text by hand.
The fix
Thread the
RerankRequestthrough to_transform_response(both the sync pathand
arerank) and back-fill by index — the same approachHuggingFaceRerankConfigalready uses for this identical provider limitation.strand{"text": ...}.request_datais optional, so any existing caller passing only theresponse keeps today's behaviour rather than breaking.
degrades instead of crashing the call.
The
Nonedefault — settled in reviewRerankRequest.return_documentsdefaults toNone, notTrue, so I originallyraised this as a judgement call. @kimnamu measured it on the issue and the
answer is clear:
rerank(...)declaresreturn_documents: bool | None = Truewhile
arerank(...)declaresNoneand forwards it positionally, so thetransform layer sees
Trueon the sync path andNoneon the async path.Treating
Noneas back-fill is what keeps the two agreeing; treating it asopt-out splits them, and closing that split from the other side would change
what the async path sends to other providers (
fireworks_aiforwards the flagonly when it is not
None).Note this is a deliberate divergence from Cohere, whose v1 rerank defaults
return_documentsto false, and from the HuggingFace config here, which treatsabsent as
False. Bedrock ends up more generous than both. Say the word if youwould rather match them and accept the sync/async split.
Release note worth making
The proxy's
/v1/rerankruns throughroute_type="arerank", so proxy callerswho never set the flag are on the
Nonebranch and will start receivingdocument text where they previously got none. For 50 documents of ~1 KB
passages that is a serialized response of 2,217 → 54,107 bytes. That is the fix
working as intended, but it is a visible payload change worth calling out in
release notes.
Known boundary: JSON documents
A dict document with no
textkey is sent to Bedrock as ajsonDocument(
_transform_sources), andRerankResponseDocumentcarries onlytext, sothere is nowhere to put it. Those callers still see no
documentkey. There isa test pinning this explicitly — no back-fill, but no crash, so index and score
still come back. Serializing the JSON into
textwould close it, but thatinvents a shape neither Cohere nor the HuggingFace config produces, so I have
left it as a documented limitation rather than decide it inside a bug fix.
Verification
Seven tests in
tests/test_litellm/llms/bedrock/rerank/test_bedrock_rerank_return_documents.py:return_documents=TrueandNone, with rankedorder preserved (the response ranks index 1 above index 0, so this also pins
that text is matched by index rather than by position)
documententirely when the caller passesFalse{"text": ...}document dictsrequest_datais supplied6 of the 7 fail without the source change. The existing
test_bedrock_rerank_header_forwarding.py(which exercisesarerank) stillpasses — 14/14 in that directory — confirming the signature change is safe
on the async path.
ruff checkandruff format --checkclean on the touched files.Unrelated pre-existing failures
tests/test_litellm/llms/bedrock/has 10 failures on this branch, but the same10 fail on a clean checkout with nothing applied (verified by stashing) — mostly
in
test_anthropic_claude3_transformation.py, plustest_sigv4_matches_rust_golden_vectorandtest_mantle.py. Untouched by thisPR; flagging in case they are not already known.
On the red
code-qualitycheck: it is not from this PR. It fails on everyPR against
litellm_internal_stagingright now, including ones that touch noworkflow files, because three unit shards in
.github/workflows/test-unit.ymlcap the job below the startup-safety invariant. Fixed independently in #38046;
this PR needs no change for it.