[None][fix] Enforce Responses conversation history capacity - #15043
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR centralizes conversation capacity trimming in ChangesConversation History Trimming Refactor
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tensorrt_llm/serve/responses_utils.py (1)
372-377: ⚡ Quick winAdd parameter type annotations to new helper methods.
The new helpers introduce untyped parameters; please type
conversation_id(and align_pop_conversation/_pop_conversation_by_conversation_idsignatures consistently) to match repo typing standards.Suggested patch
- def _pop_conversation(self, resp_id) -> None: + def _pop_conversation(self, resp_id: str) -> None: @@ - def _trim_conversation(self, conversation_id) -> None: + def _trim_conversation(self, conversation_id: str) -> None: while len(self.conversations[conversation_id] ) > self.conversation_capacity: self._pop_conversation_by_conversation_id(conversation_id) - def _pop_conversation_by_conversation_id(self, conversation_id) -> None: + def _pop_conversation_by_conversation_id(self, conversation_id: str) -> None:As per coding guidelines, “Always annotate functions; make the return type
Noneif the function does not return anything.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/serve/responses_utils.py` around lines 372 - 377, The helper methods lack parameter type annotations; update _trim_conversation(conversation_id) and _pop_conversation_by_conversation_id(conversation_id) to annotate conversation_id with the same type used by the existing _pop_conversation signature (e.g., the repo's ConversationId type or Union[str, int]) and keep the return type as -> None; ensure both signatures are consistent with each other and the project's typing conventions.Source: Coding guidelines
tests/unittest/llmapi/test_responses_utils.py (1)
57-60: ⚡ Quick winStrengthen trimming assertions to verify latest output is preserved, not only capped.
Both tests currently assert only
len(conversation) <= conversation_capacity; they can miss over-trimming regressions that drop the newly appended assistant output. Add explicit checks that"final"/"next"remains in the stored history after trimming.Suggested patch
@@ conversation = await store.get_conversation_history("resp_1") assert len(conversation) <= store.conversation_capacity + assert any( + msg.get("role") == "assistant" and msg.get("content") == "final" + for msg in conversation + ) @@ conversation = await store.get_conversation_history("resp_next") assert len(conversation) <= store.conversation_capacity + assert any( + msg.get("role") == "assistant" and msg.get("content") == "next" + for msg in conversation + ) assert ( store.response_to_conversation["resp_next"] == store.response_to_conversation["resp_prev"] )As per coding guidelines for
tests/**, coverage review should be actionable and explicit about sufficiency; this is a targeted follow-up to make trimming coverage sufficient for data-retention behavior.Also applies to: 86-89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/llmapi/test_responses_utils.py` around lines 57 - 60, The test currently only checks trimming by asserting len(conversation) <= store.conversation_capacity; update the assertions to also verify the most-recent assistant output is retained after trimming by checking that the saved conversation (from await store.get_conversation_history("resp_1") and the analogous call in the second test) contains the expected assistant message text ("final" in the first case and "next" in the second). Keep the existing length check, then add an explicit membership/assertion against conversation entries (or their text field) to ensure the latest assistant output is present after trimming.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tensorrt_llm/serve/responses_utils.py`:
- Around line 372-377: The helper methods lack parameter type annotations;
update _trim_conversation(conversation_id) and
_pop_conversation_by_conversation_id(conversation_id) to annotate
conversation_id with the same type used by the existing _pop_conversation
signature (e.g., the repo's ConversationId type or Union[str, int]) and keep the
return type as -> None; ensure both signatures are consistent with each other
and the project's typing conventions.
In `@tests/unittest/llmapi/test_responses_utils.py`:
- Around line 57-60: The test currently only checks trimming by asserting
len(conversation) <= store.conversation_capacity; update the assertions to also
verify the most-recent assistant output is retained after trimming by checking
that the saved conversation (from await store.get_conversation_history("resp_1")
and the analogous call in the second test) contains the expected assistant
message text ("final" in the first case and "next" in the second). Keep the
existing length check, then add an explicit membership/assertion against
conversation entries (or their text field) to ensure the latest assistant output
is present after trimming.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 22a070f0-e030-4632-8673-40623139fa76
📒 Files selected for processing (2)
tensorrt_llm/serve/responses_utils.pytests/unittest/llmapi/test_responses_utils.py
hchings
left a comment
hchings
left a comment
There was a problem hiding this comment.
LGTM. Thanks for catching this issue.
|
/bot run |
|
PR_Github #53477 [ run ] triggered by Bot. Commit: |
|
PR_Github #53477 [ run ] completed with state
|
4ad810d to
3910b63
Compare
|
/bot run |
|
PR_Github #53692 [ run ] triggered by Bot. Commit: |
|
PR_Github #53692 [ run ] completed with state
|
|
/bot run |
|
PR_Github #54233 [ run ] triggered by Bot. Commit: |
|
PR_Github #54233 [ run ] completed with state
|
|
/bot run |
|
PR_Github #54743 [ run ] triggered by Bot. Commit: |
|
PR_Github #54743 [ run ] completed with state
|
3910b63 to
eb06536
Compare
|
/bot run |
|
@hchings Thank you for your attention Erin. |
|
PR_Github #55333 [ run ] triggered by Bot. Commit: |
|
PR_Github #55333 [ run ] completed with state
|
eb06536 to
be7ab43
Compare
|
/bot run |
|
PR_Github #55898 [ run ] triggered by Bot. Commit: |
|
PR_Github #55898 [ run ] completed with state
|
|
Not sure why CI is still failing. Let me know if I need to do anything. |
|
/bot run --disable-fail-fast |
|
PR_Github #63084 [ run ] triggered by Bot. Commit: |
|
PR_Github #63084 [ run ] completed with state
|
Head branch was pushed to by a user without write access
2bed890 to
01122f8
Compare
|
Since CI failed, I just rebased the PR. |
zhaoyangwang-nvidia
left a comment
zhaoyangwang-nvidia
left a comment
There was a problem hiding this comment.
The root-cause analysis is right — trimming keyed on an unmapped resp_id made the while loop a no-op (and the capacity cap a no-op with it). Keying on conversation_id after the mapping is established is the correct fix, and the two new tests cover both paths. Approving; two non-blocking nits inline.
One heads-up: the PR description says pytest wasn't run locally (no pytest in that env). Could you confirm CI is green on tests/unittest/llmapi/test_responses_utils.py before merge? mergeable_state is currently blocked.
|
/bot run --disable-fail-fast |
|
PR_Github #63708 [ run ] triggered by Bot. Commit: |
|
PR_Github #63708 [ run ] completed with state
|
|
As Blossom is failing, let me know if there is anything I can do. |
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
1ceb77e to
6b8372a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run --disable-fail-fast |
pcastonguay
added
ci: post-merge approved
|
/bot run --disable-fail-fast |
|
Hi @fallintoplace - CI had 11 tests failure unrelated to this MR. I've retriggered one. TRTLLM CI is unstable lately and as soon as we get a green one we can merge. |
|
PR_Github #64393 [ run ] triggered by Bot. Commit: |
|
PR_Github #64393 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
Please do not rebase for now, as I'm trying to reuse CI stages. |
|
PR_Github #64695 [ run ] triggered by Bot. Commit: |
|
Okay. I will keep an eye on this. |
|
PR_Github #64695 [ run ] completed with state |
|
CI is green, please don't rebase. Should only lack one more approval from runtime devs. |
5 participants
Description
ConversationHistoryStoreis meant to cap the number of stored messages per conversation, but two store paths were trimming through a response id that either was not mapped yet or skipped trimming after appending final output messages.This changes trimming to operate on the conversation id after the store path has selected the conversation. That keeps normal stored Responses requests within
conversation_capacityand avoids the previous-response append path spinning without making progress.Test Coverage
tests/unittest/llmapi/test_responses_utils.pyfor the pre-stored request path and previous-response append path.pre-commit run --files tensorrt_llm/serve/responses_utils.py tests/unittest/llmapi/test_responses_utils.pypython3 -m py_compile tests/unittest/llmapi/test_responses_utils.pypython3 -m pytest tests/unittest/llmapi/test_responses_utils.pybecause this local Python environment does not havepytestinstalled.PR Checklist
Summary by CodeRabbit
Refactor
ConversationHistoryStoreto trim conversations byconversation_id.Tests
test_store_response_trims_pre_stored_request_conversation().test_store_response_trims_previous_response_conversation(monkeypatch).Dev Engineer Review
_trim_conversation(conversation_id)._pop_conversationdelegates deletion to_pop_conversation_by_conversation_id.QA Engineer Review
test_store_response_trims_pre_stored_request_conversation().test_store_response_trims_previous_response_conversation(monkeypatch).tests/integration/test_lists/.test-db/orqa/coverage entries were added.pytestwas unavailable.