[None][perf] serve: always use msgspec msgpack for disagg orchestrator->worker body#16873
[None][perf] serve: always use msgspec msgpack for disagg orchestrator->worker body#16873Tabrizian wants to merge 1 commit into
Conversation
|
/bot run --disable-fail-fast |
WalkthroughThe orchestrator-to-worker relay now always uses ChangesOrchestrator-to-worker MessagePack transport
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In
`@docs/source/blogs/tech_blog/blog26_DeepSeek_V4_on_NVIDIA_Blackwell_Model_Specific_and_Agentic_Workload_Optimizations_in_TensorRT-LLM.md`:
- Line 431: Update the relay serialization description near the “msgspec”
transport claim to remove the assertion that request-body processing is
eliminated entirely. State only that the internal path eliminates JSON encoding
and parsing, while preserving the surrounding contrast with the external OpenAI
API.
In `@tensorrt_llm/serve/openai_server.py`:
- Line 125: Add explicit return annotations to the new async json method and the
adjacent request/route hook functions around it, using their actual concrete
return types and annotating non-returning hooks with None; do not use Any.
In `@tests/unittest/disaggregated/test_disagg_openai_client.py`:
- Around line 149-176: The existing test_request_body_is_msgpack coverage only
validates the outgoing client payload; add server-side tests covering
_MsgspecRequest and _MsgspecRoute. Verify requests marked with X-TRTLLM-Msgpack
are decoded from msgpack, while requests without that marker use the plain JSON
fallback, preserving the expected parsed request behavior in both paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 51fc0f40-730b-4841-b80a-7842848d584a
📒 Files selected for processing (4)
docs/source/blogs/tech_blog/blog26_DeepSeek_V4_on_NVIDIA_Blackwell_Model_Specific_and_Agentic_Workload_Optimizations_in_TensorRT-LLM.mdtensorrt_llm/serve/openai_client.pytensorrt_llm/serve/openai_server.pytests/unittest/disaggregated/test_disagg_openai_client.py
|
PR_Github #61709 [ run ] triggered by Bot. Commit: |
…r->worker body Make the msgspec MessagePack transport for the disaggregated orchestrator->worker request body unconditional and drop the TRTLLM_SERVE_ENABLE_MSGSPEC opt-in flag. msgspec is already a hard dependency (requirements.txt -> install_requires), so the guarded import and its ImportError shim are no longer needed. The worker still keys off the X-TRTLLM-Msgpack header to choose the decoder, so external OpenAI-API clients keep the unchanged stdlib-json path. Signed-off-by: Iman Tabrizian <itabrizian@nvidia.com>
53b5954 to
5a8d2b7
Compare
|
PR_Github #61709 [ run ] completed with state
|
Dev Engineer Review
msgspecMessagePack for internal orchestrator-to-worker request forwarding.QA Engineer Review
No test changes.
Description
Follow-up to #16126, which introduced the
msgspecMessagePack transport for thedisaggregated orchestrator→worker request body behind the
TRTLLM_SERVE_ENABLE_MSGSPECopt-in flag.This PR makes that transport unconditional and removes the environment variable.
openai_client.py: the orchestrator always encodes the forwarded body asmsgpack and tags it with
X-TRTLLM-Msgpack: 1. Themodel_dump_jsonfallbackbranch is gone.
openai_server.py:_MsgspecRequest/_MsgspecRoutemove to module scope(they were nested inside the
if _MSGSPEC_ENABLED:block) and the route classis always installed on the FastAPI app.
import msgspecunconditionally.msgspecis already a harddependency — it is in
requirements.txt, whichsetup.pyfeeds intoinstall_requires— so the guarded import and itsImportErrorshim wereredundant.
Behavior for external clients is unchanged: the worker keys off the
X-TRTLLM-Msgpackheader, so anything without that header (i.e. everyOpenAI-API client) still goes through the stdlib
json.loadspath.Content-Typestays
application/jsonso FastAPI keeps routing bodies throughRequest.json().The tech blog paragraph describing this path as "opt-in" is updated to match.
Test Coverage
No new tests. This PR removes a configuration branch rather than adding a code
path — the msgpack transport itself is unchanged from #16126, it is just no
longer gated.
Existing coverage now exercises the msgpack path by default:
tests/unittest/disaggregated/test_disagg_openai_client.py(registered inl0_a10.yml) covers the orchestrator send/retry/streaming paths, which nowalways serialize via msgpack.
end-to-end; the previously opt-in path is the one they now run.
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.