Repository navigation
[Fix] dspark: Hand the overlap readback a snapshot of the folded accept, not views (#42465) - #45207
Open
feixiangpeng wants to merge 7 commits into
Open
feixiangpeng wants to merge 7 commits into
feixiangpeng wants to merge 7 commits into
Conversation
…ept (sgl-project#42465) The folded accept writes fixed epilogue buffers inside the target-verify graph, and the next replay rewrites them. accept_and_finalize returned read_accept(bs) views, which the DSPARK worker put into GenerationBatchResult.accept_lens / next_token_ids. The overlap scheduler reads those back on copy_stream after copy_stream.wait_stream(forward_stream) and launches the next forward without waiting for the copy, so when the copy engine is busy (HiCache write-through D2H of a long prefix) the readback runs after replay N+1 and returns step N+1's accept lengths. Each TP rank races on its own, so ranks committed different token counts and deadlocked. Add DsparkVerifyEpilogue.snapshot_accept: clones of the accept buffers made on the forward stream right after the replay, so stream order pins them to step N and async_d2h's record_stream keeps them alive for the readback. accept_and_finalize returns the snapshot on the folded path. read_accept keeps returning views because the in-graph _accept writes through them (build_accept_outs(out=self.read_accept(bs))); cloning there, as sgl-project#42812 does, leaves the static buffers stale on current main. Tests: a CPU case in test_dspark_accept_tail.py (fails on main, passes with the fix) and a 1-GPU test that replays the two-stream interleaving deterministically. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
feixiangpeng
requested review from
Qiaolin-Yu,
Ying1123,
hnyls2002,
merrymercy and
xiezhq-hermann
as code owners
October 19, 2026 17:15
This comment has been minimized.
This comment has been minimized.
Review against .claude/rules/comment-style.md: the snapshot_accept docstring carried the bug narrative and the issue number, the scheduler comment read as history, and the test docstrings were paragraphs. Each now states the constraint in two or three lines; the rationale stays in the PR description. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ck too sgl-project#45450 added block_accept_lens_buf, written by the static accept tail and handed to GenerationBatchResult as a view of the graph buffer; the overlap readback copies it on copy_stream after the next replay may have rewritten it, the same hazard snapshot_accept covers for the other accept outputs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
#42465: DeepSeek-V4.1-Flash with
--speculative-algorithm DSPARKand HiCachewrite_throughdeadlocks across TP ranks. WithSGLANG_ENABLE_RANK_CONSENSUS_CHECKER=1the ranks diverge oncache_finished_req ... owned_kv_lenby 1-2 tokens, so each rank inserts a different prefix and the write-through backups no longer line up.The mechanism, as @divyvasal found: the folded accept runs inside the target-verify CUDA graph and writes fixed epilogue buffers (
commit_lens_buf,out_tokens_buf, ...) that the next replay rewrites.accept_and_finalize(folded_accept=True)returnedread_accept(bs), i.e. views of those buffers, and the DSPARK worker put them intoGenerationBatchResult.accept_lens/next_token_ids. Under the overlap scheduler_copy_result_to_cpuissues that readback oncopy_streamaftercopy_stream.wait_stream(forward_stream)and then launches forward N+1 without waiting for the copy. When the copy engine is busy, for example behind the write-through device-to-host copy of a ~540K-token prefix, the readback runs after replay N+1 and reads step N+1's accept lengths. Each rank races on its own, so ranks commit different token counts. The in-graph accept is broadcast from rank 0, so the ranks agree on the device; only the host readback diverges.Modifications
dspark_verify.py:DsparkVerifyEpilogue.snapshot_accept(bs)returns clones of the six accept buffers. The clones are made on the forward stream right after the replay, so stream order pins them to step N, andasync_d2h'srecord_streamkeeps them alive until the readback has run.TargetVerifyExecutor.accept_and_finalizereturns the snapshot on the folded path.read_acceptkeeps returning views, because the in-graph_acceptwrites through them (build_accept_outs(..., out=self.read_accept(bs))).scheduler.py: comment in_copy_result_to_cpuupdated; no behavior change (the DSpark inline copy under Confidential Computing is kept as validated).Relation to #42812
#42812 clones inside
read_accept. That was right at its base (264d1c2, 2026-10-06), where_acceptwrote the buffers directly. Since #43790 (2026-10-12)_acceptpassesread_accept(bs)as its in-graph destination, so cloning there makes the graph write into throwaway clones and the static buffers never update. On today's main the existingtest_dspark_accept_tail.py::test_graph_accept_matches_eager_greedyfails with #42812's hunk applied (4 subtests). This PR keeps the alias where the graph needs it and snapshots where the views escape. @shanemort1982 reported the same breakage on #42812 on 2026-10-19 and proposed the same split (views accessor as the write target, clones for the reader); this PR keepsread_acceptas the views so the in-graph call site is unchanged and addssnapshot_acceptfor the reader.Accuracy / tests
test/registered/unit/spec/test_dspark_accept_tail.py::TestFoldedAcceptSnapshot(CPU): the foldedaccept_and_finalizeoutput must not alias the epilogue buffers and must keep its values after the buffers are rewritten. Fails on main (AssertionError: True is not false : correct_len), passes with the fix. The existing parity case still passes.test/registered/unit/spec/test_dspark_overlap_readback_streams.py(1-GPU): deterministic replay of the interleaving with real streams andasync_d2h: a spin kernel on copy_stream stands in for the busy copy engine, forward_stream rewrites the buffers for step N+1; views read step N+1 (the hazard), the snapshot reads step N. On an H200 (main 5cbe237 + this change): both cases pass; against the pre-fixdspark_verify.pythe view case still reproduces the hazard and the snapshot case fails. The fulltest/registered/unit/spec/sweep passes (308 tests, 176 subtests). Re-run on 2026-10-19 against main 8e7601a: same outcomes, sweep 311 passed, 182 subtests.Checklist
CI States
Latest PR Test (Base): ❌ Run #40166228725
Latest PR Test (Extra): ❌ Run #40166228717
Latest PR Test (AMD ROCm 10): ❌ Run #40166228711