[None][fix] Fix disagg gen-consensus deadlock under pipeline parallelism - #15595
[None][fix] Fix disagg gen-consensus deadlock under pipeline parallelism#15595Shixiaowei02 wants to merge 1 commit into
Conversation
f2d9422 to
c493c61
Compare
|
/bot run --add-multi-gpu-test --disable-fail-fast |
📝 WalkthroughWalkthroughFixes two disaggregated-generation collective communication issues: adds a ChangesDisaggregated generation deadlock and consensus fixes
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 |
|
PR_Github #55516 [ run ] triggered by Bot. Commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tensorrt_llm/_torch/disaggregation/transceiver.py`:
- Around line 363-365: In transceiver.py, the call site in the consensus flow is
using getattr(self._dist, "pp_allgather"), which triggers Ruff B009 and is
unnecessary. Update the _consensus_outcome invocation to access
self._dist.pp_allgather directly, keeping the same arguments and behavior while
removing the dynamic attribute lookup.
In `@tensorrt_llm/_torch/pyexecutor/py_executor.py`:
- Around line 1934-1940: The propagated disagg transfer flag in PyExecutor’s
scheduling path is missing newly started gen-init transfers, because it is
computed before _prepare_disagg_gen_init() can begin receives and before the
next poll loop reuses it. Update the logic around
disagg_gen_transfer_in_progress and
SerializableSchedulerOutput.from_scheduler_result in py_executor.py so the flag
also reflects any fitting_disagg_gen_init_requests that just started transfer,
ensuring the next status check is not skipped.
🪄 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: 115e5aba-1b55-4544-be00-7bf9a880410e
📒 Files selected for processing (3)
tensorrt_llm/_torch/disaggregation/transceiver.pytensorrt_llm/_torch/pyexecutor/py_executor.pytensorrt_llm/_torch/pyexecutor/scheduler/scheduler.py
|
PR_Github #55516 [ run ] completed with state
|
d35e52d to
fe65a28
Compare
|
/bot run --add-multi-gpu-test --disable-fail-fast |
|
PR_Github #55731 [ run ] triggered by Bot. Commit: |
bf95f26 to
42864f4
Compare
The disaggregated generation server intermittently hangs with the Python cache transceiver under pipeline parallelism (py_transceiver=True). Two independent causes, both in the per-iteration consensus collectives: 1. Participation divergence. _check_disagg_gen_transfer_status gated the gen transfer-status group allgather on a per-rank predicate (any in-transmission request). Across pipeline stages the in-transmission set differs, so some ranks enter the allgather while peers skip it and advance to the next collective -> cross-collective deadlock. Fix: propagate the scheduling rank's flag on the existing schedule broadcast (SerializableSchedulerOutput) so every PP rank enters/skips together. 2. Cross-communicator ordering. _gen_consensus/_gen_consensus_outcome used a single WORLD allgather while the ctx path uses a TP-then-PP two-stage. WORLD is a different communicator from TP/PP and MPI does not order collectives across communicators, so under pipeline drift the gen WORLD allgather deadlocks against the ctx/PP subgroup allgather. Fix: route the gen consensus through the same TP-then-PP two-stage as ctx (tp x pp == world, identical result) so all disagg consensus uses the ordered TP/PP communicators. No new collective is introduced (both fixes ride existing communication). The C++ transceiver path and the non-PP loop are unaffected. Validated on 8xB200 (Qwen3-Next, TP4 ctx / TP2-PP2 gen): 10/10 runs pass; plain upstream hangs intermittently. Signed-off-by: Shixiaowei02 <39303645+Shixiaowei02@users.noreply.github.com>
42864f4 to
9b3e53e
Compare
|
/bot run --add-multi-gpu-test --disable-fail-fast |
|
PR_Github #55786 [ run ] triggered by Bot. Commit: |
|
PR_Github #55731 [ run ] completed with state |
|
PR_Github #55786 [ run ] completed with state
|
|
duplicated with #15356 |
Description
This pull request introduces important changes to the pipeline and parallelism (PP/TP) consensus and scheduling logic for disaggregated generation in the distributed execution engine. The main focus is on improving synchronization and preventing deadlocks in collective operations by ensuring all pipeline parallel (PP) ranks make consistent decisions about entering or skipping group collectives, particularly during disaggregated generation KV cache transfers.
The most important changes are:
Consensus and Synchronization Improvements:
_gen_consensusand_gen_consensus_outcomeintransceiver.pyto use a two-stage tensor-parallel (TP) then pipeline-parallel (PP) consensus instead of a single world allgather when attention data parallelism is disabled, preventing deadlocks caused by cross-communicator collectives. [1] [2]KV Cache Transfer Coordination:
disagg_gen_transfer_in_progressflag toSerializableSchedulerOutputinscheduler.pyto propagate the scheduling rank's transfer status to all PP ranks via the existing schedule broadcast, ensuring all ranks enter or skip the transfer-status allgather together. [1] [2]_pp_schedule_and_propagateinpy_executor.pyto set and propagate thedisagg_gen_transfer_in_progressflag, and to adopt this flag on every PP rank for consistent collective participation. [1] [2]_check_disagg_gen_transfer_statusinpy_executor.pyto gate the group allgather on the propagated flag, avoiding divergent per-rank decisions that could lead to deadlock.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.Summary by CodeRabbit