[None][perf] Reduce OpenAI stream postprocess overhead - #14708
Conversation
📝 WalkthroughWalkthroughAdded stream response metadata tracking to chat and completion streaming processors. Both ChangesStream Response Metadata Tracking
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unittest/llmapi/test_llm.py (1)
2578-2578: ⚡ Quick winAdd an explicit
Nonereturn type on the test function.Please annotate the test signature to keep function typing consistent with repo rules.
Suggested change
-def test_chat_stream_post_processor_reuses_stream_metadata(): +def test_chat_stream_post_processor_reuses_stream_metadata() -> None:As per coding guidelines: "Static type checking with mypy is opt-in by submodule; always annotate functions with return types (use
Noneif function does not return)".🤖 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_llm.py` at line 2578, The test function test_chat_stream_post_processor_reuses_stream_metadata lacks an explicit return type; update its signature to include a return annotation of None (i.e., def test_chat_stream_post_processor_reuses_stream_metadata() -> None:) to satisfy the repo's typing rule requiring all functions to declare return types.
🤖 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 `@tests/unittest/llmapi/test_llm.py`:
- Line 2578: The test function
test_chat_stream_post_processor_reuses_stream_metadata lacks an explicit return
type; update its signature to include a return annotation of None (i.e., def
test_chat_stream_post_processor_reuses_stream_metadata() -> None:) to satisfy
the repo's typing rule requiring all functions to declare return types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: df80d662-dbba-4264-ada5-341cc18b0933
📒 Files selected for processing (2)
tensorrt_llm/serve/postprocess_handlers.pytests/unittest/llmapi/test_llm.py
|
/bot run |
|
PR_Github #50971 [ run ] triggered by Bot. Commit: |
|
PR_Github #50971 [ run ] completed with state
|
|
/bot run |
|
PR_Github #51125 [ run ] triggered by Bot. Commit: |
|
PR_Github #51125 [ run ] completed with state
|
Streaming responses created fresh metadata for each chunk when callers did not pass it explicitly. High-concurrency workloads can emit hundreds of thousands of chunks, which makes UUID generation, and time lookups part of the CPU hot path. Reuse stream metadata per request. Signed-off-by: William Zhang <133824995+2ez4bz@users.noreply.github.com>
Signed-off-by: William Zhang <133824995+2ez4bz@users.noreply.github.com>
Signed-off-by: William Zhang <133824995+2ez4bz@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #51387 [ run ] triggered by Bot. Commit: |
|
PR_Github #51387 [ run ] completed with state
|
|
/bot run |
|
PR_Github #51493 [ run ] triggered by Bot. Commit: |
|
PR_Github #51493 [ run ] completed with state
|
|
/bot run |
|
PR_Github #51636 [ run ] triggered by Bot. Commit: |
|
PR_Github #51636 [ run ] completed with state
|
|
/bot run |
|
PR_Github #51682 [ run ] triggered by Bot. Commit: |
|
PR_Github #51682 [ run ] completed with state
|
|
/bot run |
|
PR_Github #51700 [ run ] triggered by Bot. Commit: |
|
PR_Github #51700 [ run ] completed with state
|
|
/bot run |
|
PR_Github #51746 [ run ] triggered by Bot. Commit: |
|
PR_Github #51746 [ run ] completed with state |
Summary by CodeRabbit
Bug Fixes
Tests
Description
Streaming responses created fresh metadata for each chunk when callers did not pass it explicitly. High-concurrency workloads can emit hundreds of thousands of chunks, which makes UUID generation, and time lookups part of the CPU hot path.
Reuse stream metadata per request.
Details
Prior to this, the stream postprocessors created each SSE chunk (e.g.
ChatCompletionStreamResponse) without passingidorcreated, thus calling their factoriesuuid.uuid64andtime.time, respectively, so every streamed chunk got fresh metadata.Semantically, this was wrong - all chunks for one streamed OpenAI response should have the same
idandcreatedtimestamp.As a side-effect, this also benefits performance: at high concurrency on lower-tier CPUs, this change can by itself lead to a ~5% ITL decrease.
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.