[TRTLLM-12228][fix] ADP dummy type mismatch and collective deadlock - #13460
[TRTLLM-12228][fix] ADP dummy type mismatch and collective deadlock#13460Shixiaowei02 wants to merge 4 commits into
Conversation
|
/bot run --add-multi-gpu-test --disable-fail-fast |
9a52f21 to
6fd59a0
Compare
|
/bot run --add-multi-gpu-test --disable-fail-fast |
|
PR_Github #45498 [ run ] triggered by Bot. Commit: |
|
PR_Github #45499 [ run ] triggered by Bot. Commit: |
|
PR_Github #45498 [ run ] completed with state |
|
PR_Github #45499 [ run ] completed with state
|
|
/bot --help |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand. Details
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
d16d211 to
cdcf930
Compare
|
/bot run --add-multi-gpu-test --disable-fail-fast |
|
PR_Github #45574 [ run ] triggered by Bot. Commit: |
6e0313c to
c6a60e3
Compare
|
/bot run --add-multi-gpu-test --disable-fail-fast |
|
PR_Github #45590 [ run ] triggered by Bot. Commit: |
|
PR_Github #45590 [ run ] completed with state
|
📝 WalkthroughWalkthroughThis PR introduces stateful control for ADP dummy request insertion and enhances scheduling diagnostics. Changes add role-locking logic to track dummy request type (context vs. generation), update schedulability accounting to exclude certain request states, provide structured diagnostics for scheduling failures, and expand test coverage for ADP-related behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tensorrt_llm/_torch/pyexecutor/request_utils.py (1)
83-105: Narrow the diagnostic helper’s exception handling.This path catches
Exceptionthree times and silently drops two of them, so a brokenindex_mapper/kv_cache_managerprobe will look like “KV cache full” instead of showing that the diagnostic itself failed. Please catch the specific expected failures here and append a fallback marker instead ofpass.As per coding guidelines, "When using Python try-except blocks, limit the except clause to the smallest set of errors possible."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tensorrt_llm/_torch/pyexecutor/request_utils.py` around lines 83 - 105, The enum conversion and diagnostic probes are too broadly swallowing all Exceptions; narrow them and record fallback markers instead of silent pass: for LlmRequestState(req.state_value) catch the specific enum/validation error (e.g. ValueError) and set name to a clear fallback like f"state={req.state_value} (invalid)"; for the index_mapper probes (idx_mapper.num_free_slots() and idx_mapper.size()) catch only expected probe errors (e.g. AttributeError, TypeError, RuntimeError) and append a marker such as ", indexmapper_probe_failed" when they fail; likewise for kv_cache_manager.get_num_free_blocks() catch only expected probe errors (AttributeError, TypeError, RuntimeError) and append ", free_blocks_probe_failed" instead of silently passing. Ensure you reference LlmRequestState, idx_mapper, index_mapper.num_free_slots, index_mapper.size, and kv_cache_manager.get_num_free_blocks when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tensorrt_llm/_torch/pyexecutor/py_executor.py`:
- Around line 2845-2857: The ADP dummy role
(_adp_dummy_is_gen/_adp_dummy_role_locked) is being set only after per-rank
routing from validated_requests, causing idle ranks to default to GEN dummies;
change the logic to determine and lock the ADP dummy role before routing by
deriving it from executor configuration or broadcasting a role decision via
kv_cache_transceiver when enable_attention_dp is true (and kv_cache_transceiver
is not None), so that _pad_attention_dp_dummy_request() sees the correct role
even if a CTX worker has no local requests; ensure the new early path sets
_adp_dummy_is_gen and _adp_dummy_role_locked consistently and falls back only if
no broadcast/config value is available, and keep existing checks that reference
validated_requests as a secondary source.
In `@tests/unittest/_torch/executor/test_py_executor.py`:
- Around line 453-470: Tests currently duplicate the role-lock logic in a local
_run_role_lock function instead of exercising the real implementation; replace
the duplication by either invoking the production code path
(PyExecutor._fetch_and_activate_new_requests) with a PyExecutor instance and
minimal mocks/stubs for stub.enable_attention_dp, stub.kv_cache_transceiver, and
validated_requests (or by patching the surrounding interactions) so the real
block runs, or extract the role-lock block into a shared helper (e.g.,
PyExecutor._apply_role_lock or a module-level function) and update both
production to call that helper and tests to call the helper directly; target
symbols: _run_role_lock (remove), PyExecutor._fetch_and_activate_new_requests
(call), and LlmRequestType (use existing enums in tests) to locate the logic to
exercise.
---
Nitpick comments:
In `@tensorrt_llm/_torch/pyexecutor/request_utils.py`:
- Around line 83-105: The enum conversion and diagnostic probes are too broadly
swallowing all Exceptions; narrow them and record fallback markers instead of
silent pass: for LlmRequestState(req.state_value) catch the specific
enum/validation error (e.g. ValueError) and set name to a clear fallback like
f"state={req.state_value} (invalid)"; for the index_mapper probes
(idx_mapper.num_free_slots() and idx_mapper.size()) catch only expected probe
errors (e.g. AttributeError, TypeError, RuntimeError) and append a marker such
as ", indexmapper_probe_failed" when they fail; likewise for
kv_cache_manager.get_num_free_blocks() catch only expected probe errors
(AttributeError, TypeError, RuntimeError) and append ",
free_blocks_probe_failed" instead of silently passing. Ensure you reference
LlmRequestState, idx_mapper, index_mapper.num_free_slots, index_mapper.size, and
kv_cache_manager.get_num_free_blocks when making these changes.
🪄 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: 1879cd2e-0997-4f88-a3c8-29ed00552115
📒 Files selected for processing (4)
tensorrt_llm/_torch/pyexecutor/py_executor.pytensorrt_llm/_torch/pyexecutor/request_utils.pytests/unittest/_torch/executor/test_benchmark_disagg.pytests/unittest/_torch/executor/test_py_executor.py
|
/bot run --add-multi-gpu-test --disable-fail-fast |
|
PR_Github #47197 [ run ] triggered by Bot. Commit: |
|
PR_Github #47198 [ run ] triggered by Bot. Commit: |
c97ff1a to
46ea67b
Compare
|
/bot run --add-multi-gpu-test --disable-fail-fast |
|
PR_Github #47297 [ run ] triggered by Bot. Commit: |
|
PR_Github #47297 [ run ] completed with state |
|
/bot run --add-multi-gpu-test --disable-fail-fast |
|
PR_Github #47299 [ run ] triggered by Bot. Commit: |
Signed-off-by: Shixiaowei02 <39303645+Shixiaowei02@users.noreply.github.com>
Signed-off-by: Shixiaowei02 <39303645+Shixiaowei02@users.noreply.github.com>
Signed-off-by: Shixiaowei02 <39303645+Shixiaowei02@users.noreply.github.com>
46ea67b to
287c6f6
Compare
|
/bot run --add-multi-gpu-test --disable-fail-fast |
|
PR_Github #47371 [ run ] triggered by Bot. Commit: |
|
PR_Github #47371 [ run ] completed with state
|
Cherry-picked from #13460 Signed-off-by: Fanrong Li <lfr-0531@users.noreply.github.com>
Cherry-picked from NVIDIA#13460 Signed-off-by: Fanrong Li <lfr-0531@users.noreply.github.com>
Cherry-picked from NVIDIA#13460 Signed-off-by: Fanrong Li <lfr-0531@users.noreply.github.com>
Cherry-picked from NVIDIA#13460 Signed-off-by: Fanrong Li <lfr-0531@users.noreply.github.com>
@coderabbitai summary
Description
Stabilizes disaggregated serving when ADP is enabled.
max_num_tokensso the MoE all-to-all sees aligned token counts across ranks.tp_gatherin response enqueue no longer deadlocks. Includes matching transceiver/NIXL plumbing and a disagg+ADP accuracy test.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)
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.