[https://nvbugs/6388153][fix] Relay PP sample states synchronously on…#16170
[https://nvbugs/6388153][fix] Relay PP sample states synchronously on…#16170WeiHaocheng wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change modifies PyExecutor's pipeline-parallel sample-state relay behavior. The TLLM_PP_ASYNC_BROADCAST_SAMPLE_STATE default flips from "1" to "0", enabling synchronous inline relay by default. Queue setup, shutdown, forward-pass drainage, and executed-batch dequeue logic are adjusted to support both relay modes without deadlocks. ChangesPP Synchronous Sample-State Relay
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ExecutorLoopPP
participant RingBroadcast
participant ExecutedBatchQueue
participant NonZeroRank
alt async relay enabled
ExecutorLoopPP->>ExecutedBatchQueue: enqueue executed_batch
ExecutedBatchQueue->>RingBroadcast: background thread relay
NonZeroRank->>NonZeroRank: get() blocking on executed_batch_response_queue
else synchronous relay (default)
ExecutorLoopPP->>RingBroadcast: _ring_broadcast_sample_state(executed_batch)
ExecutorLoopPP->>ExecutorLoopPP: drain send_handles before forward
NonZeroRank->>NonZeroRank: get_nowait() and raise RuntimeError if missing
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
… the executor thread Signed-off-by: WeiHaocheng <20514172+WeiHaocheng@users.noreply.github.com>
354e901 to
5c7af1d
Compare
|
LGTM. Can merge after @yuxianq take a look. |
Signed-off-by: WeiHaocheng <20514172+WeiHaocheng@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #58597 [ run ] triggered by Bot. Commit: |
|
PR_Github #58597 [ run ] completed with state
|
The 1024-slot minimum exists to absorb the async relay thread's delivery lag. The synchronous relay retires each microbatch within its own iteration, so pipeline-depth many slots suffice — and the pre-forward drain then scans pp_size handles instead of 1024. Signed-off-by: WeiHaocheng <20514172+WeiHaocheng@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #58670 [ run ] triggered by Bot. Commit: |
Under pipeline parallelism, sample states are relayed between PP ranks
(last rank -> 0 -> 1 -> ...) by a dedicated background thread. The relay
thread needs the GIL to run: when the executor thread blocks inside a
GIL-holding native call that waits on GPU progress (e.g. a DeepGEMM JIT
kernel cold load whose lazy loading requires context synchronization),
the relay starves, the downstream rank stops advancing, its unlaunched
forward never pairs the in-flight NCCL p2p kernel the blocked rank's GPU
is waiting on, and the ranks deadlock.
Summary by CodeRabbit