Repository navigation
[PP+PD] Fix #38206 Reduce bootstrap consensus latency - #38959
Conversation
|
/tag-and-rerun-ci |
|
/rerun-failed-ci |
| # chunk can still be pending, and concluding now would drop it. | ||
| if ( | ||
| status == KVPoll.Success | ||
| and self.kv_mgr._staging_outstanding.get(self.bootstrap_room, 0) > 0 |
There was a problem hiding this comment.
Thanks for these changes, the logic reads much cleaner this way.
One thing I'm not sure about: _staging_outstanding here is PP0-local, while check_status_pp_consensus is deciding on the replicated request_status of all ranks. If some rank k>0 publishes Success with a deferred staging chunk still pending, PP0 has no way to see that, right?
It doesn't bite today since nothing is sent before pop_bootstrapped, but I think it will once transfer consensus moves onto the store. Maybe the transfer thread should hold update_status(Success) until _staging_outstanding drops to 0, so request_status alone is the source of truth? That would also let poll and poll_pp_consensus share one body. Curious what you think.
There was a problem hiding this comment.
Maybe the transfer thread should hold update_status(Success) until _staging_outstanding drops to 0, so request_status alone is the source of truth?
Thanks for your feedback.
That is exactly what I tought. With the change you mentioned, i.e. making request_status the single source of truth, it will make code cleaner.
I think we can do that in a follow-up PR. This PR focuses on only improving TTFT by removing bootstrap consensus.
|
Resolved conflicts against master. |
Begin (19/53):
python3 /home/runner/work/sglang/sglang/test/registered/unit/managers/test_scheduler_hicache_events.py
.
.
/home/runner/work/sglang/sglang/python/sglang/srt/layers/quantization/awq/awq.py:53: UserWarning: Only CUDA, HIP and XPU support AWQ currently.
warnings.warn(f"Only CUDA, HIP and XPU support AWQ currently.")
/home/runner/work/sglang/sglang/python/sglang/srt/layers/quantization/gguf.py:69: UserWarning: Only CUDA, MUSA and NPU support GGUF quantization currently.
warnings.warn(f"Only CUDA, MUSA and NPU support GGUF quantization currently.")
..E
======================================================================
ERROR: test_pp_prefill_drains_before_admission (__main__.TestSchedulerHiCacheEvents)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/runner/work/sglang/sglang/test/registered/unit/managers/test_scheduler_hicache_events.py", line 134, in test_pp_prefill_drains_before_admission
s.event_loop_pp_disagg_prefill()
File "/opt/hostedtoolcache/Python/3.10.21/x64/lib/python3.10/site-packages/torch/utils/_contextlib.py", line 124, in decorate_context
return func(*args, **kwargs)
File "/home/runner/work/sglang/sglang/python/sglang/srt/managers/scheduler_pp_mixin.py", line 266, in event_loop_pp_disagg_prefill
self.process_bootstrapped_queue()
File "/home/runner/work/sglang/sglang/python/sglang/srt/managers/scheduler_pp_mixin.py", line 599, in process_bootstrapped_queue
reqs = self.disagg_prefill_bootstrap_queue.pop_bootstrapped()
AttributeError: 'Scheduler' object has no attribute 'disagg_prefill_bootstrap_queue'
----------------------------------------------------------------------
Ran 3 tests in 0.053s
FAILED (errors=1)
.
.
End (19/53):
filename='/home/runner/work/sglang/sglang/test/registered/unit/managers/test_scheduler_hicache_events.py', elapsed=9, estimated_time=2.0
. |
OK. Fixed. |
| assert get_disagg().optimistic_prefill_attempts == 0, ( | ||
| "PP prefill is incompatible with optimistic prefill" | ||
| ) |
There was a problem hiding this comment.
No need, just need to know optimistic prefill is not on your roadmap, and then it is fine. Since this is in the hot path, let us remove this assertion.
There was a problem hiding this comment.
OK. Not in roadmap.
|
/tag-and-rerun-ci |
|
/rerun-failed-ci |
Resolve conflicts: - common/conn.py: keep both the PPConsensusStore import and main's common.bootstrap imports. - mooncake/conn.py: keep the PR's poll()/_poll_with_status() split and carry main's deferred-decode-allocation Success-hold condition into _poll_with_status() so poll_pp_consensus() honors it too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Thanks for the PR. Requested changes:
- Don't rely on queue symmetry across PP ranks.
pop_bootstrappedapplies PP0'spollsto the localself.queueby position, and returns early on an empty queue beforepp_sync_polls. If queues diverge (e.g.continueafterensure_metadata_buffer/finalize_bootstrapfails on one rank), polls attach to the wrong request orrecv_objectblocks. Send rids with the polls, or at least assertlen(polls) == len(self.queue). - Replace
pickleinPPConsensusStorewith msgspec. The payload is only ints, and the PULL socket is TCP-bound and unauthenticated. - Add a scheduler-level test for abort/failure with PP>1.
|
OK. I see. Thanks for reviewing, Shangming and Lzy17. |
Co-author: @inkcherry @huangtingwei9988
Motivation
In current implementation, PP+PD bootstrap ready takes a full round-trip from PP0 to PP(n-1). This contributes to TTFT in large pipeline deployement, e.g.
pp_size=8.This has been reported in #34815 and analyzed in depth in #38206. PR #36613 was a different solution to the issue.
This PR proposes an alternative approach. It introduces
PPConsensusStore, which is simply aDictshared among PP ranks. All PP ranks update their bootstrap status into the map. PP0 collects others bootstrap status and determine bootstrap readiness. PP0 broadcast its decision to other PP ranks usingpp_sync. This completely removes two-round consensus for bootstrap readiness.This PR is working in progress. Here are limitations:
Only works with mooncake transfer engine. Other transfer engine is not fixed yet.TCPStorelacks of batch and non-blocking operation. We may need to implement our own store.Only bootstrap consensus is removed. Other consensus processes are kept unchanged.. This will be done in follow-up PRs.Discussion and advices are welcome.
Modifications
PPConsensusStore, which helps PP0 to gather boostrap ready events from other PP ranks.MooncakeKVSender.poll_pp_consensus. This function collects bootstrap events of other PP ranks, andpop_bootstrapped, PP0 callspoll_pp_consensusto collect bootstrap status from others, and then propagate decision to other PP ranks.Accuracy Tests
Tested SWE bench verfieid with DSv4 flash, pd disaggregation, P pp_size=4 tp_size=1, D pp_size=1 tp_size=4.
Baseline: 387/500
This PR: 385/500
Details
Speed Tests and Profiling
I tested dsv4 flash with PD and no HiCache. The benchmark shows reduciton on TTFT by a single round of prefill (~1.4s).
Details
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ciCI States
Latest PR Test (Base): ✅ Run #37272668112⚠️ Not enabled -- add
Latest PR Test (Extra):
run-ci-extralabel to opt in.Latest PR Test (AMD ROCm 10): ❌ Run #37272668001