perf(deepseek): overlap decode prepare and reclaim - #147
Conversation
📝 WalkthroughWalkthroughThe serving worker now supports asynchronous prepared decode with split device dispatch and output reclamation. DeepSeek V4 uses ping-pong buffers and persistent generation-aware MTP device state. Prefill finalization initializes state, and tests cover ordering, reuse, EOS handling, and reclamation. ChangesAsynchronous MTP decode
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ServingWorker
participant PyptoExecutor
participant DeepSeekV4ModelRunner
participant DeviceKernel
participant ResultReclaimer
ServingWorker->>PyptoExecutor: Prepare decode snapshot
PyptoExecutor->>DeepSeekV4ModelRunner: Prepare slot-local inputs
ServingWorker->>PyptoExecutor: Dispatch prepared decode
PyptoExecutor->>DeepSeekV4ModelRunner: Launch device work
DeepSeekV4ModelRunner->>DeviceKernel: Execute fused MTP decode
DeviceKernel->>DeepSeekV4ModelRunner: Update persistent device state
ServingWorker->>ResultReclaimer: Queue pending result
ResultReclaimer->>PyptoExecutor: Reclaim prepared decode
PyptoExecutor->>DeepSeekV4ModelRunner: Read committed outputs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4ee99b4 to
1f7b270
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
pypto_serving/model/deepseek/npu_runner.py (4)
3602-3620: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRead
_mtp_request_statesunder_mtp_state_lock.Line 3608 uses
self._mtp_request_states.get(request_id)without the lock. The sibling helper_require_mtp_request_statetakes_mtp_state_lockfor the same read, andrelease_finished_requestspops from the dict on the worker output lane while this method runs on the prepare lane.The read itself is atomic, so the dict cannot be corrupted. Use the lock for consistency with the other accessors and to keep the state lookup and the
tail_slot_id/generationreads in one critical section.🤖 Prompt for 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. In `@pypto_serving/model/deepseek/npu_runner.py` around lines 3602 - 3620, Protect the MTP state lookup and related field reads in the loop around `_mtp_request_states` with `_mtp_state_lock`. Keep the lookup, `tail_slot_id`, and any `generation` access in the same critical section, matching `_require_mtp_request_state`, while preserving the existing missing-state and unreserved-tail behavior.
2636-2666: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the state collection in
_reclaim_mtp_decode.Line 2662 appends
pending.states[len(states)], which depends on the current length of the list being built. The loop variablerequest_idis never used, which Ruff reports as B007 at line 2645.Zip
pending.statesinto the same loop and drop the localstateslist.♻️ Proposed refactor
accepted_counts_list = [] accepted = [] - states = [] - for request_id, rank, local_row in zip( - inputs.request_ids, + states = pending.states + for rank, local_row in zip( inputs.ranks, inputs.local_rows, strict=True, ): row_start = local_row * decode_seq main_tokens = pending.sampled_ids[ rank, row_start : row_start + decode_seq, 0, ].tolist() accepted_count = int( pending.accepted_counts[rank, local_row].item() ) accepted_counts_list.append(accepted_count) accepted.append([int(token) for token in main_tokens[:accepted_count]]) - states.append(pending.states[len(states)])🤖 Prompt for 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. In `@pypto_serving/model/deepseek/npu_runner.py` around lines 2636 - 2666, Update _reclaim_mtp_decode to iterate over pending.states alongside inputs.request_ids, inputs.ranks, and inputs.local_rows in the existing strict zip, append accepted tokens using the provided state directly, and remove the temporary states list and pending.states[len(states)] lookup. Rename the unused request_id loop variable appropriately so Ruff no longer reports B007.Source: Linters/SAST tools
3777-3785: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
_DeepSeekV4DecodeSharedBufferspinssampled_idsandtensorsto slot 0.Both fields reference
self._decode_input_slots[0]._execute_main_decodeusesdecode_buffers.sampled_ids, so the autoregressive prepared path writes slot 0 even whenprepared.buffer_slotis 1.This is safe today because
supports_async_decode_reclaimreturnsself._compiled.enable_mtp, so the non-MTP path never splits dispatch and reclaim and the device lane serializes dispatch with the result read. The coupling is implicit and breaks if split reclaim is extended to the non-MTP path.Select the active slot in
_execute_main_decodefromprepared.buffer_slot, or document why slot 0 is correct there.🤖 Prompt for 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. In `@pypto_serving/model/deepseek/npu_runner.py` around lines 3777 - 3785, The _DeepSeekV4DecodeSharedBuffers construction hardcodes sampled_ids and tensors from decode input slot 0, while _execute_main_decode may operate on another prepared slot. Select the active decode input slot using prepared.buffer_slot when constructing or consuming these fields, preserving the existing buffer-slot selection for the autoregressive prepared path.
3109-3130: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the duplicate DeepSeek V4 vocab-size constant.
_require_decode_logits_buffer()returns the cached allocation without checking the requested size._require_decode_logits_buffer(DEEPSEEK_V4_VOCAB_SIZE)and_require_decode_logits_buffer(model.config.vocab_size)currently use the same constant, but keeping both makes the size assumption less clear. Usemodel.config.vocab_sizehere with the same validation thatload_lm_head_weights()applies.🤖 Prompt for 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. In `@pypto_serving/model/deepseek/npu_runner.py` around lines 3109 - 3130, Update _bind_prepared_mtp_dispatch to request the logits buffer using model.config.vocab_size instead of DEEPSEEK_V4_VOCAB_SIZE, matching the validation and size source used by load_lm_head_weights(). Keep the existing buffer retrieval and dispatch argument construction unchanged.
🤖 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 `@pypto_serving/model/deepseek/npu_runner.py`:
- Around line 2179-2183: Validate buffer_slot explicitly in the decode buffer
access near _ensure_decode_buffers, rejecting values below 0 as well as values
above the available slot range before indexing _decode_input_slots. Preserve the
existing ValueError message/handling for invalid slots and the normal staged
lookup for slots 0 and 1.
- Around line 3663-3786: Update _decode_input_slots and the step-slot ownership
in WorkerProcess._pipelined_busy_loop so the number of reusable decode slots
matches the maximum in-flight device steps. Ensure dispatch does not replace or
recycle a slot until reclaim has finished reading all captured input and output
tensors, rather than relying on cmd.step_id % 2 when two-step overlap can reuse
a slot.
In `@pypto_serving/serving/server/serving_worker.py`:
- Around line 277-297: Update
pypto_serving/serving/server/serving_worker.py:277-297 around the StepCommand
registration and _reclaim_pending_decode lifecycle to remove the outdated
FIFO-ordering claim and ensure reclaiming step N cannot remove the _req_cache
entry registered for step N+1, using step/generation identity validation. Update
pypto_serving/model/deepseek/npu_runner.py:1696-1706 to correct the
FIFO-device-lane comment and verify that the relevant kernel rejects descriptors
whose generation no longer matches the recycled slot, enforcing that validation
if necessary.
- Around line 390-408: Update _reclaim_pending_decode so
_release_finished_request_state(work.cmd.finished_request_ids) executes in a
finally block, regardless of whether reclaim_prepared_decode or
_consume_decode_result raises. Preserve the existing successful StepResult path
and exception logging/error response while ensuring finished request state is
always released.
- Around line 269-276: Update the decode-exception path in
_device_execution_loop to stop writing directly to self.output_queue; wrap the
encoded StepResult error in the marker type _CompletedStepOutput and enqueue it
through output_work_queue so the output lane preserves FIFO ordering with
earlier steps. Ensure the output lane forwards the completed failure result
using the existing ordered-output handling.
- Around line 678-709: Prevent _prepare_step_command from preparing decode work
when the executor supports async decode preparation but not device decode
embeddings, since the placeholder batch would bind hidden_states to token zero.
Add an explicit validation failure before _make_decode_batch, or alternatively
update _late_bind_prepared_decode_batch to re-bind hidden_states whenever
token_ids are replaced; preserve correct embeddings for all prepared decode
batches.
---
Nitpick comments:
In `@pypto_serving/model/deepseek/npu_runner.py`:
- Around line 3602-3620: Protect the MTP state lookup and related field reads in
the loop around `_mtp_request_states` with `_mtp_state_lock`. Keep the lookup,
`tail_slot_id`, and any `generation` access in the same critical section,
matching `_require_mtp_request_state`, while preserving the existing
missing-state and unreserved-tail behavior.
- Around line 2636-2666: Update _reclaim_mtp_decode to iterate over
pending.states alongside inputs.request_ids, inputs.ranks, and inputs.local_rows
in the existing strict zip, append accepted tokens using the provided state
directly, and remove the temporary states list and pending.states[len(states)]
lookup. Rename the unused request_id loop variable appropriately so Ruff no
longer reports B007.
- Around line 3777-3785: The _DeepSeekV4DecodeSharedBuffers construction
hardcodes sampled_ids and tensors from decode input slot 0, while
_execute_main_decode may operate on another prepared slot. Select the active
decode input slot using prepared.buffer_slot when constructing or consuming
these fields, preserving the existing buffer-slot selection for the
autoregressive prepared path.
- Around line 3109-3130: Update _bind_prepared_mtp_dispatch to request the
logits buffer using model.config.vocab_size instead of DEEPSEEK_V4_VOCAB_SIZE,
matching the validation and size source used by load_lm_head_weights(). Keep the
existing buffer retrieval and dispatch argument construction unchanged.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c3bc95f-dda7-4a96-bb66-a2215f845ea1
📒 Files selected for processing (12)
.agents/skills/profile-dsv4-serving-strace/scripts/analyze_profile.pydocs/dev/model/deepseek-v4.mdpypto-libpypto_serving/model/common/executor/executor.pypypto_serving/model/common/executor/pypto_executor.pypypto_serving/model/deepseek/npu_executor.pypypto_serving/model/deepseek/npu_runner.pypypto_serving/serving/server/serving_worker.pytests/unit/model/deepseek/test_model_components.pytests/unit/serving/device_sampling_fakes.pytests/unit/serving/engine/test_async_pipeline.pytests/unit/serving/server/test_worker_step_protocol.py
ed19881 to
974068c
Compare
974068c to
d53ea9b
Compare
d53ea9b to
11714f3
Compare
11714f3 to
b5c9ade
Compare
b5c9ade to
a109161
Compare
Summary
Why
The asynchronous scheduler could queue future decode steps, but host preparation and output processing still serialized the device lane. Persistent device state removes the prior-token dependency, and independent FIFO reclaim allows the next prepared invocation to launch without waiting for scheduler-visible output processing.
PR #139 reduced serial host gaps by memoizing request assignments, static metadata, and complete L3 argument tuples. Those caches were tied to stable request/rank layouts, had narrow hit conditions, and required extra invalidation logic. Once prepare runs concurrently with the much longer device invocation, that work is no longer on the critical path. This PR therefore prefers normal per-step construction on the prepare lane over topology- and lifecycle-sensitive caches.
Performance: before vs after this PR
The baseline is the final synchronous decode path from PR #139 (
a74a514, merged asb1a35ab). The optimized result is this PR after removing the temporary cache shortcuts (1f7b270, rebased without semantic changes asa3a19e0). Both profiles used the same model, prompt, exactly 20 output tokens, fused one-L2 MTP decode, PTOAS 0.54, host STRACE with device STRACE disabled, and had MTP acceptance 10/11 (90.91%).The key framework result is the 91.5% reduction in the exposed cross-round host gap. All ten successor prepares completed before the preceding device invocation ended; the minimum remaining device-execution margin was 27.054 ms. Reclaim ran on the independent output lane and no longer blocked the next device dispatch.
The baseline was task
task_20260805_024605_15915111219on devices 8-15. The optimized run was tasktask_20260810_034449_263105568on devices0,2,4,6,8,10,12,14. Because the physical device sets differ, the 3.8%-4.5% decode-duration changes are directional; the lane ordering and exposed-gap reduction are the primary evidence for this PR. HTTP wall time is intentionally excluded because prefill compilation/runtime variance dominated it.Baseline artifact:
artifacts/dsv4-profile-hostopt-rebased-p1c66-r3165-lc46d-ptoas054-20260805in the PR #139 checkout.Optimized artifact:
artifacts/dsv4-serving-profile-20260810-even-cleanup.Current validation
a109161onorigin/main@0033990python -m pytest tests/unit -q— 139 passed after review fixesThe NPU profile was captured on the pre-rebase equivalent implementation (
1f7b270). The rebase resolved the new upstreamKernelCompilerintegration and worker execution conflict; the current commit has host validation but has not been rerun on NPU.Review hardening
Earlier multi-request validation
Four sequential requests with 20, 17, 13, and 9 output tokens completed, and two staggered four-request waves completed all eight requested outputs. In that trace, 34/34 steady transitions overlapped prepare(N+1) with dispatch(N) and reclaim(N) with dispatch(N+1).
The two waves used different packed-prefill groupings (1+3 and 3+1). Greedy output matched between waves for 2/4 prompts; two prompts differed despite matching output-token counts. That batch-shape-dependent precision observation remains a follow-up and is not claimed as a precision pass for the current commit.
Depends on hw-native-sys/pypto-lib#917.