[None][fix] Enforce Responses conversation history capacity - #15043
[None][fix] Enforce Responses conversation history capacity#15043fallintoplace wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR refactors ChangesConversation History Trimming Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Our CI is not stable lately. |
be7ab43 to
d4daeb2
Compare
|
PR_Github #58312 [ run ] triggered by Bot. Commit: |
|
PR_Github #58312 [ run ] completed with state
|
|
CI is failing again, sadly. If this was related to this PR, I will have a look. |
|
Hi @fallintoplace can you rebase and check whether the following two cases are related to your MRs? They doesn't look related to me from a quick glance. We have some CI fixes in latest main branch.
|
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
Head branch was pushed to by a user without write access
d4daeb2 to
db5bb7c
Compare
3 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
Tests