Repository navigation
Conversation
|
|
Greptile SummaryThis PR restores Vertex AI rerank document text in normalized results when documents are requested and omits it when record details are disabled.
Confidence Score: 4/5The PR appears safe to merge functionally, with a non-blocking source-convention cleanup needed in the response transformer. The document mapping follows the shared rerank response shape and the request’s effective return_documents setting; the only accepted issue is the newly introduced mutable, non-Final implementation style and unnecessary explanatory comment. Files Needing Attention: litellm/llms/vertex_ai/rerank/transformation.py
|
| Filename | Overview |
|---|---|
| litellm/llms/vertex_ai/rerank/transformation.py | Correctly maps Vertex record content into normalized rerank documents, but the implementation violates repository immutability, Final-annotation, and comment conventions. |
| tests/test_litellm/llms/vertex_ai/rerank/test_vertex_ai_rerank_transformation.py | Adds focused coverage for document inclusion, omission, and transformed request behavior without weakening existing assertions. |
| tests/test_litellm/llms/vertex_ai/rerank/test_vertex_ai_rerank_integration.py | Extends the mocked integration flow to verify returned document text and omission when record details are disabled. |
Reviews (1): Last reviewed commit: "fix(vertex_ai): populate document text i..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| # Determine whether to return documents (defaults to True) |
There was a problem hiding this comment.
The new response path reassigns return_documents and result_item, mutates intermediate dictionaries, and adds a straightforward explanatory comment, violating the repository’s immutable, Final-annotated source conventions and making the response construction harder to reason about.
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!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Gentle follow-up for @krrishdholakia / @ishaan-jaff — all 77 CI checks are passing green with no merge conflicts. Happy to address any feedback if needed, otherwise ready for review/merge. Thank you! |
…_documents is true
e441d38 to
fcf23a1
Compare
|
Gentle follow-up for @krrishdholakia and @ishaan-jaff. All CI checks are passing green, and the branch is cleanly mergeable with full unit and integration test coverage for Vertex AI rerank response document mapping. Ready for review whenever convenient. Thank you! |
What does this PR do?
Fixes #38239
When calling
litellm.rerank()with Vertex AI semantic ranker models andreturn_documents=True(the default), the response transformer previously discarded the returnedcontentfield from each record.This change:
transform_rerank_response, checks whetherreturn_documentsis true (viaoptional_params,request_data,litellm_params, or defaultTrue) and populatesdocument={"text": record["content"]}whenrecord.get("content")is present.return_documents=False, thedocumentfield is omitted from results as expected.return_documents=Trueandreturn_documents=False.