Repository navigation
perf(ds4): accelerate long context and add PFlash - #664
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@cubic-dev-ai Please re-review the full current PR diff against main, especially the four findings from your previous review. Review only; do not generate or push fixes. Your check on the merge commit explicitly skipped AI review. The updated branch preserves current main and adds these corrections:
Fresh lucebox8 verification of the current code (ROCm 7.2.4, gfx1151, HIP graphs enabled): clean build; 451/451 server tests; 80/80 saved-row cases; feature-gate, dispatch, fused-combine, and ROCm format tests pass; 8/8 paged-attention tests pass. The latest commit only documents the supported PFlash placements. The PR remains draft. Fresh GitHub CI is running. A real-model adaptive retrieval smoke recovered the expected values but failed the strict five-line formatting check; that result is not counted as a pass, and an AR control is being checked. No new Vulkan speed or universal byte-identity claim is being made. |
@davide221 I have started the AI code review. It will take a few minutes to complete. |
|
Current-head runtime qualification update (lucebox8, gfx1151, ROCm 7.2.4,
The strict validator was not weakened. This isolates the difference to the Fresh local unit/kernel suites still pass, including 80 saved-row cases and The fixes are pushed and current main is integrated; the PR remains draft and |
There was a problem hiding this comment.
All reported issues were addressed across 13 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 13 files
Requires human review: Auto-approval blocked by 2 unresolved issues from a previous review of this commit.
Re-trigger cubic
Bind fused attention rotations to runtime positions during graph replay. Retain standalone forward-Q RoPE in cached decode and verifier graphs while preserving sparse flash attention, F16 KV, fused inverse RoPE, and uncached prefill fusion. Add width 1-5 F16/F32 replay and standalone-RoPE regression coverage, plus invalid-position contract checks. Document the narrow correctness qualification without claiming universal identity or revalidated long-context performance.
|
@cubic please review the new bug-fix commit The fix:
Validation on lucebox8, Strix Halo gfx1151, ROCm 7.2.4, HIP graphs enabled:
This fixes the reproduced failure, not a proof of universal byte identity. The first internal numerical divergence in fused forward-Q RoPE has not been localized; the guarded path is the tested fix. Sparse verification remains opt-in. Full 128K performance retention and a matched normal/ROCprof comparison remain unverified. The previous green CI/Cubic results belong to |
@davide221 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/docs/DS4.md">
<violation number="1" location="server/docs/DS4.md:565">
P3: The sparse-flash paragraph implies the switch to sparse top-k attention applies generically per layer, but the code only enables it for lane_q==1 or ratio==4 (deepseek4_fused_verify.inc). For batched verification lanes with other ratios, the verifier stays on explicit attention regardless of row savings. Restrict the doc to say the switch applies only in single-lane or ratio-4 layouts, since saved raw rows force explicit attention for other batched lanes.</violation>
</file>
<file name="server/src/deepseek4/deepseek4_graph.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_graph.cpp:2359">
P1: When cached verification wraps and preserved raw suffix rows are appended, this condition still enables the sparse flash path. Keep that batched layout on explicit attention, and enable sparse attention only when no preserved-row suffix is present.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Keep raw-row snapshots aligned with the current ring indices on cached graph replay, without disabling sparse F16 attention. Add 72 exact replay checks over widths 2-5 and F16/F32 storage, including ring wraps and 128K positions. Clarify the sparse verifier layout qualification.
|
Updated #664 with current main's #716 disk-prefix-cache integration and a targeted cached-verifier replay fix. No sparse-attention default was changed. @cubic-dev-ai please review the new head, especially the preserved-row report and the new regression. The saved suffix was already included by The fix gathers the few overwritten rows using runtime I32 indices corresponding to the I64 ring-write indices, before the writes execute. It preserves the cache dtype and retains sparse attention, native F16 KV, packed indexing, and adaptive verification. It does not add per-ring-position graph-cache entries or convert the entire KV cache. Fresh lucebox8 / gfx1151 / ROCm 7.2.4 / HIP-graphs-ON verification:
The sparse-verifier docs now explicitly limit batched sparse selection to ratio-4 layouts. Coarse-selection batched layouts remain explicit. The two earlier P3 performance follow-ups retain their previously documented non-blocking disposition; neither is claimed implemented here. This is correctness qualification, not a new throughput claim. The earlier 8K off/on measurements belong to the previous head. The 123K baseline was stopped after memory pressure and swapping; no long-context result is claimed. The PR is not merged, and fresh CI/review must be assessed on this head. Artifacts: |
@davide221 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/src/deepseek4/deepseek4_backend.cpp">
<violation number="1" location="server/src/deepseek4/deepseek4_backend.cpp:2986">
P2: When a `compress` command arrives through the daemon/string protocol, `handle_compress` builds `CompressRequest` without setting `drafter_gpu` or `residency_action`, so the ~1.5 GB PFlash drafter always loads on GPU 0 and stays resident (`KeepLoaded`) until the next park/free_drafter. The new typed `compress_batch` path honors `ReleaseAfterUse` and `drafter_gpu`; the string path cannot express either, so request-scoped residency and multi-GPU drafter placement are silently unavailable over the daemon protocol.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 15 files
Requires human review: Auto-approval blocked by 5 unresolved issues from previous reviews.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
|
Pushed
New lb8/gfx1151 validation: full DS4 suite passes, including the new cases, 72/72 saved-row replay, 80/80 saved-row attention, and 20/20 RoPE replay. Server tests 453/453, feature gate 1/1. Clean scoped diff and matching local/remote executable-source hashes. Binary SHA256: The previous revision's 8K live adaptive sparse F16 retrieval passed 5/5 with the known-good response hash; the final revision is being checked separately. The attempted 123K request stopped at the conservative cumulative swap-out guard (35,949 pages, about 140 MiB; about 11 GiB still available), while a limited CPU test build was also running. It is not a valid long-context correctness or performance result and is not an OOM finding. No new Vulkan or long-context speed claim is made. Fresh CI/review must be assessed on this new head. The RTX 3090 runner is confirmed offline; no checks were disabled. No merge or deployment performed. |
@davide221 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 15 files
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
|
Final revision: Validation summary:
The 123K attempts stopped at their swap guards, including the final isolated attempt; none produced a valid long-context result. This is not a fresh long-context performance or Vulkan-parity qualification. Sparse verification remains experimental/opt-in. All temporary test servers are stopped; other people's workloads were not interrupted. Artifacts remain in |
Merge-ready update — 2026-09-08
Current head:
13648dba4528ad592f85a766cde4cfc0ff6fb581. Main through561a7e9d0(#716 DS4 disk-prefix-cache integration) is merged without dropping its snapshot or disk-cache changes. The subsequent main change, #719, touches only README.md; GitHub still reports no merge conflicts. Cubic review is complete, with all discussions resolved after inspection; this is not a formal Cubic approval. Code review is complete, with the hardware-CI exceptions below explicitly retained. No merge has been performed.df780faf7676ccc768737cb23d0534555c6e4785382a7f225d4842e82dcffb87. Adaptive DSpark (cap 4), all six experts, sparse F16 fused verification active, prefix/PFlash caches off. One cold request with chunk 512 is correctness evidence, not a matched throughput comparison.Performance boundaries: previous-head 8K off/on medians were 30.05 → 33.00 tok/s decode; these are not measurements of this new head. The 123K off-profile baseline and subsequent guarded correctness attempts produced no valid result because of swapping. The final isolated request stopped at ~1.066 GiB cumulative swap-out with ~10.53 GiB still available, so no OOM diagnosis or long-context correctness/throughput result is claimed. No fresh long-context gain, universal byte-identity guarantee, or Vulkan-parity claim is made. All temporary test servers are stopped.
New CI run · Final review-fix evidence
Previous cached-RoPE fix — 2026-09-08
Previous head:
cbeea1f78925578be37e3d6b8d00e37378294786. The tested sparse-verification fix is now pushed; fresh CI and Cubic review are required. This is not a merge or performance-parity declaration.Prior-head review — 2026-09-07
Previously reviewed head:
059086abbd7a80ce84ab4e048aeabcc97f089eb4. Main was integrated at that revision. This PR does not include changes unique to #667. The CI and review results in this section predate the new fix.Rollout boundary
Keep
DFLASH_DS4_SPARSE_DECODE_FLASH=0(the default is off) until broader workload and performance qualification. The sparse example inserver/docs/DS4.mdremains an experimental opt-in. The prior head failed strict five-line formatting with sparse F16/F32 verification. The new cached-RoPE fix resolves that reproduced failure in two repeated F16 adaptive runs; it does not establish universal output identity or qualify every context/checkpoint.AR and adaptive DSpark q<=4 with fused explicit attention both pass the same 7,680-token strict retrieval task and produce identical response hashes. This single-task result is not a universal byte-identity guarantee. PFlash is lossy, optional, and supported for monolithic serving only; do not enable it for true-context or exact-retrieval performance comparisons.
Previous full-suite verification (
059086abb)Clean ROCm 7.2.4 / gfx1151 build with HIP graphs enabled; 451/451 server tests; 80/80 saved-row GPU cases (q=1..5, 2K/30K compressed rows, F16/F32, mask/direct selector); feature-gate, ROCm-format, packed-indexer, dispatch and fused-combine checks; 8/8 paged-attention checks. The two-device HC subtest skips with only Strix exposed. No warning-free claim.
Final real-model PFlash/FlowKV lifecycle smoke: two successful HTTP requests with correct short answers, 2,578 target tokens → 1,288 effective tokens, one target park/restore cycle, and one compression-content cache hit on the second turn. This used exact prefill and request-scoped drafter residency. The initial harness assumed the whole-prompt log format and two fresh compressions; its post-run assertion was corrected by separately validating the actual multi-turn FlowKV route against both saved responses and lifecycle logs. No kernel/server failure occurred, and the temporary server was stopped afterward.
CI run · Matched runtime controls
Changes
Historical measurements — before the current main integration
The original branch reported 122,879 prompt tokens, 256 output tokens and six experts: client decode 17.513 → 22.513 tok/s (+28.6%), unchanged output SHA, zero swap. The cited Vulkan reference was 32.60 tok/s. These are historical results, not remeasured current-head performance and not a parity claim.
The original PFlash smoke reported 8,192 target tokens → 1,739 effective tokens with 2.18 s Qwen scoring. Compression changes effective context and must not be used to claim matched-context speed.