perf(dflash): add opt-in exact prefill bands - #590
Open
cheese-cakee wants to merge 21 commits into
Open
Conversation
cheese-cakee
marked this pull request as ready for review
August 18, 2026 16:00
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
40 issues found across 170 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/server/http_server.cpp">
<violation number="1">
P1: When a local request uses `tools` with a non-null `tool_choice`, this branch routes it to the concurrent scheduler, which never passes the tool-choice hint to the backend. Preserve the classic path for these requests or add equivalent tool-choice inputs to the sequence-engine contract.</violation>
</file>
<file name="server/src/server/scheduler.cpp">
<violation number="1">
P1: When concurrent serving is enabled, this admits the raw rendered prompt and bypasses `prepare_prompt`, so configured FlowKV/PFlash prompt processing is silently ignored. Prepare the effective prompt through the same request pipeline before admission, or reject unsupported prompt-processing modes explicitly.</violation>
</file>
<file name="server/src/qwen35/qwen35_backend.h">
<violation number="1">
P1: When a hybrid `Qwen35MoeBackend` is configured with `max_concurrency > 1`, this inherited accessor exposes `Qwen35SeqEngine` even though that engine bypasses the MoE hybrid execution path. Return `nullptr` or reject concurrent serving for MoE backends until the engine supports `hybrid_forward_*`.</violation>
</file>
<file name="harness/benchmarks/concurrency/canonical_concurrent_benchmark.py">
<violation number="1">
P2: When any request in a wave fails, this condition skips retirement waiting and immediately starts the next wave. Abort before advancing or wait for every known response ID so a failed stream cannot leave scheduler work overlapping the next measurement.</violation>
<violation number="2">
P2: The canonical DDTree launcher uses `--ddtree-budget 22`, but this proof computes acceptance with a fixed `16 * steps` denominator. Derive the denominator from the actual candidates offered per step, including shortened or adaptive steps, and record that telemetry.
(Based on your team's feedback about offered-candidate acceptance denominators.)</violation>
<violation number="3">
P2: When one successful request has no output timing, `statistics.median` receives `None` alongside numeric timings and the runner exits with an exception. Filter absent timings and only publish the median/max when every successful request has a numeric value.</violation>
</file>
<file name="server/src/qwen35/graph_builders.cpp">
<violation number="1">
P2: When a caller supplies a positive `n_prefill_segments` count with a null `prefill_segments` array, this loop crashes instead of returning the builder's expected `false` failure. Reject a null segment array before iterating, and validate the segment count as non-negative.</violation>
</file>
<file name="harness/benchmarks/concurrency/run_qwen36_concurrency.sh">
<violation number="1">
P2: When `PREFILL_FIRST_BURST_STEPS` is positive, the Luce cases claim to measure a TTFT policy that the server never consumes. Add the server-side policy implementation or remove/reject this unsupported benchmark option before publishing its results.</violation>
<violation number="2">
P2: When `IDLE_PREFILL_TOKENS` exceeds 4096 with the default zero burst steps, this runner accepts a benchmark combination the protocol says is invalid. Reject that combination or require a positive `PREFILL_FIRST_BURST_STEPS` before launching cases.</violation>
<violation number="3">
P2: When a screen selects only one engine, the unconditional binary checks still require the unused engine and prevent valid Luce-only or llama-only runs. Validate each server binary only when its variant is selected.</violation>
<violation number="4">
P2: On a host without `awk` or `ldd`, the runner fails after creating the output rather than reporting a missing dependency up front. Include both commands in the preflight dependency check.</violation>
</file>
<file name="harness/benchmarks/concurrency/summarize_concurrency.py">
<violation number="1">
P2: A DDTree report with positive aggregate proof but no per-response telemetry passes this check and publishes AL/acceptance. Validate every measured request's `ddtree_metrics` and positive step count before using the aggregate proof, so missing concurrency records fail closed.</violation>
<violation number="2">
P2: When every ragged report omits `selected_output_set_sha256`, the summary claims `Stable output: yes` and still publishes comparison deltas. Treat missing or non-string output hashes as unknown (`n/a`) or reject the reports before calculating stability and deltas.</violation>
</file>
<file name="harness/benchmarks/concurrency/run_qwen36_canonical_concurrency.sh">
<violation number="1">
P2: Every case rereads the full target and optional draft just to write metadata, making repeated runs unnecessarily I/O-heavy and potentially perturbing file-cache state between cases. Hash each artifact once before the case loops and reuse the resulting digests.</violation>
<violation number="2">
P2: When a DDTree run receives a non-Q8_0 drafter, the metadata still claims `draft_quant: Q8_0` because the runner validates readability only. Validate the GGUF quantization or record the detected value so benchmark reports cannot mislabel the measured recipe.</violation>
</file>
<file name="server/src/deepseek4/deepseek4_dspark_spec.cpp">
<violation number="1">
P2: When `DFLASH_DS4_Q5_VERIFY=1` uses fused verification, this enables q=5 without providing the output storage required by the wide graph. The verifier silently splits q=5 into q=4 plus q=1, adding work instead of using the opt-in wide path; enable `keep_logits_` for q=5 or pass an output buffer.</violation>
</file>
<file name="harness/benchmarks/deepseek4/ds4_publication_decode_client.py">
<violation number="1">
P2: When an SSE `data:` line is malformed, this branch drops its content and can still mark the run successful. Return a failed record on JSON decode errors so dropped chunks cannot enter the throughput summary.</violation>
<violation number="2">
P2: The client includes the first output token in the numerator even though timing starts when that token arrives, overstating every reported decode rate by one token. Use `(completion_tokens - 1) / decode_s`, matching the repository's established decode metric.</violation>
</file>
<file name="harness/benchmarks/concurrency/concurrent_benchmark.py">
<violation number="1">
P2: When the server returns a boolean or negative usage count, these checks mark it complete and the benchmark aggregates invalid accounting; negative prompt counts can pass the fixed-token gate and produce misleading rates. Validate both usage counts as non-boolean, non-negative integers before storing them, matching `native_prefill_values()`.</violation>
</file>
<file name="server/src/qwen35/qwen35_target_graph.cpp">
<violation number="1">
P2: When `in.q_capture` is enabled for a ragged paged step, this branch leaves `q_fa_out` null and the per-layer `cache.q_cap` remains stale. Assign `*q_fa_out = Qfa` before calling `paged_read`.</violation>
</file>
<file name="server/src/common/moe_hybrid_placement.cpp">
<violation number="1">
P2: When critical-path placement runs with many layers and unequal expert sizes, this exact frontier can grow combinatorially and make model initialization consume excessive memory and time. Bound or quantize the frontier, or use a bounded knapsack/greedy algorithm so placement startup remains predictable.</violation>
</file>
<file name="harness/qualification/deepseek4/analyze_rocprof_overlap.py">
<violation number="1">
P2: When `--timeline-max` is enabled, the timeline header emits four comma-separated fields, but every burst row emits three. Keep the header at three columns and print `merge_gap_us` separately so CSV consumers can parse the timeline.</violation>
<violation number="2">
P2: With a finite but enormous option such as `--window-start-s 1e308`, this conversion raises `OverflowError` after the finite check passes. Validate unit-scaled values before converting, or report the range error through `argparse`.</violation>
</file>
<file name="server/test/test_paged_attention.cpp">
<violation number="1">
P2: This test does not actually force the partitioned path. ggml-cuda reads GGML_CUDA_PAGED_ATTN_FORCE_PARTITIONS once into a function-local static (`static const int forced_partitions = [](){ ... }();` in paged-attn.cu:855-859) on the first paged-attn compute. PartitionedPathMatchesReference is registered first and runs several full computes with the env unset, so the static is captured as 0 before DirectPathMatchesReference sets the ScopedEnvVar. Setting the env at runtime here has no effect on the already-captured value, so the "direct" case runs the default partition path and no longer pins the partitioned path that the old CTest invocation (env set before process start) exercised.</violation>
<violation number="2">
P3: The reference mirror does not actually treat a negative causal position as a padding row the way the kernel does. When query_positions[seq] < 0 on a row whose physical slot is in range, the code sets kv_seq_len = position + 1; for position < -1 that is negative and `std::vector<float> scores(kv_seq_len)` becomes undefined behavior instead of "leave zero output" as the comment claims. The kernel treats any negative query position as a dead row (valid_query = false) regardless of slot validity. The current test only pairs the -1 position with the out-of-range padding slot (which is skipped by the earlier `continue`), so the divergence is untested and latent; clamp the position to >= 0 to match the kernel's dead-row semantics.</violation>
</file>
<file name="server/tests/test_server_parallel.py">
<violation number="1">
P2: When --max-concurrency is in 33..64, test_parallel_more_than_slots crashes the whole suite instead of testing. It sets count = 2 * n and passes it to make_math_prompts, which raises ValueError for count > 64. The suite advertises support for up to 64 streams (the main() range check and the other tests accept 64), so this is inconsistent. Cap count at 64 (use min(2*n, 64)) or add a skip for n > 32 so the suite doesn't abort with a traceback.</violation>
</file>
<file name="harness/qualification/deepseek4/qualify_ds4_q5_amd.sh">
<violation number="1">
P2: When this launcher runs from cron, a container, or another environment without `HOME`, `set -u` aborts at server-env construction before the qualification starts. Use a default for `HOME` (or validate it explicitly) before expanding it.</violation>
</file>
<file name="server/test/smoke_load_draft.cpp">
<violation number="1">
P2: The new tensor validation checks are placed at the end of the test, but `w.fc`, `w.hidden_norm`, and `w.layers[0]` are all dereferenced earlier (the `w.fc->ne[0]`, `w.hidden_norm->ne[0]`, `w.layers[0].wq->ne[0]`, and `ggml_backend_tensor_get(w.hidden_norm, ...)` calls above). If any of these were null/empty, the code would already crash with undefined behavior before `CHECK_NOT_NULL(w.fc)` is reached, so these checks can never catch the exact condition they guard and offer no real protection. Move them immediately after the successful load (before any tensor iteration/printing) so a null tensor yields a clean assertion failure instead of a segfault.</violation>
</file>
<file name="server/src/server/server_status.h">
<violation number="1">
P3: When concurrent slots are simultaneously prefilling and decoding, the status page emits `badge-mixed` without a matching CSS rule, so the new phase loses its phase styling. Add a `.badge-mixed` style to the status page.</violation>
</file>
<file name="harness/benchmarks/concurrency/generate_prompts.py">
<violation number="1">
P3: When `MAX_TOKENS` is overridden, the generated `he-raw` manifest still records `max_tokens: 128` while the benchmark sends the override. Remove this unused field or populate it from the same runtime setting so manifests describe the measured workload.</violation>
</file>
<file name="server/src/qwen35/graph_builders.h">
<violation number="1">
P3: When `n_prefill_tokens > 0`, `seq_slot` is ignored; each `QwenPrefillSegment::seq_slot` selects the recurrent-state slab. Document that `seq_slot` applies only to non-segment steps.</violation>
</file>
<file name="server/test/CppUnitTestFramework.hpp">
<violation number="1">
P3: The message passed to SKIP (e.g. "CUDA device unavailable") is silently discarded: the catch sets only test_skipped and ConsoleLogger::SkipTest prints just the test name, so TestSkippedException::m_message is never surfaced and callers can't see why a test was skipped. Surface the reason (e.g. pass the exception message to SkipTest or log it before clearing).</violation>
<violation number="2">
P3: For a test skipped via SKIP, ExitTest (which resets m_indent_level to 0) is never called and SkipTest does not reset it, while EnterTest already incremented it. Each skipped test leaks one indent level, so indentation of all subsequent verbose/section/assert logging for the rest of the run drifts. Reset m_indent_level in the skipped branch, e.g. in SkipTest.</violation>
</file>
<file name="server/test/test_recurrent_snapshot.cpp">
<violation number="1">
P3: When the CPU backend is unavailable, the preceding `CHECK(backend != nullptr)` records a failure but the immediately following `SKIP(...)` throws `TestSkippedException`. In `CppUnitTestFramework::TestRegistry::Run`, a caught `TestSkippedException` marks the test skipped and takes precedence over `HaveChecksFailed()`, so the recorded failure is discarded and the test is reported as a skip (exit 77, not a CI failure). The old `if (!backend) return 1;` failed hard instead. Same applies to the `ctx` and `buffer` guards. Use `REQUIRE` for these environment preconditions so a genuinely broken setup fails the test rather than silently skipping it.</violation>
</file>
<file name="optimizations/paged_attention/CONCURRENCY_OPT_QWEN36_EXPERIMENTS.md">
<violation number="1">
P3: This newly added file is a Qwen3.6 concurrency/experiments ledger, but the PR is a DeepSeek4 opt-in exact-prefill-bands change. Nothing in the ledger covers DeepSeek4 exact prefill, and none of the PR's 9 product/test files or its DS4 feature is referenced here. Unrelated documentation mixed into a focused PR makes review and history harder to reason about; move this ledger to a separate PR (or a Qwen-concurrency doc update).</violation>
</file>
<file name="server/test/smoke_load_target.cpp">
<violation number="1">
P3: The added null checks run after the code has already dereferenced the same pointers. The spot-check `printf` block above uses `w.tok_embd->ne[0]`, `w.output->type`, and `w.out_norm->type`, and the `print_layer` loop reads `w.layers[il]` — so if any of these were null, the test would crash or read invalid memory before `CHECK_NOT_NULL` ever executes. The checks therefore cannot actually catch the null case they appear to validate. Move these guards before the dereferences and use `REQUIRE` so they abort the test with a clean message instead of crashing.</violation>
</file>
<file name="server/test/test_seq_slot_manager.cpp">
<violation number="1">
P3: The comment "Invalid asks are hard errors, not busy" is not actually verified. The two checks here only assert `!is_admitted(...)`; a `busy` result also fails `is_admitted`, so a regression where `admit` returns `busy` for an empty or oversize prompt would still pass this block. Assert the concrete status (`SeqEngine::AdmitResult::Status::failed`) and `!is_busy(...)` so the claimed classification is enforced, as the later "never-fits" block does.</violation>
</file>
<file name="server/test/test_flashprefill_kernels.cpp">
<violation number="1">
P2: This conversion registers the kernel validation as an always-run CppUnit/CTest case, but the numerical checks only print PASS/FAIL and never assert. A kernel regression (mean diff ≥ 1e-2, attention diff ≥ 5e-2, or a slow/broken e2e path) will print FAIL while the test still returns success, so CI stays green on a broken flashprefill kernel. Since the whole purpose of this test is numerical validation, gate each check on a REQUIRE (e.g. REQUIRE(max_diff_mean < 1e-2f) and REQUIRE(max_diff_attn < 5e-2f)).</violation>
</file>
<file name="server/test/test_feature_gate.cpp">
<violation number="1">
P3: All ~20 gate tests are now folded into a single TEST_CASE (feature_gate_suite), so the framework can no longer report or filter each rule cluster independently. The old RUN_TEST harness reported each function as its own named test. Create one TEST_CASE per test function (or at least per cluster) so a failure is attributed to a specific gate rule and a subset can be run via a keyword.</violation>
</file>
<file name="README.md">
<violation number="1">
P3: The new "Recommended Setups" DeepSeek rows recommend `--ds4-prefill exact` and the DSpark/fused-verify flags but never enable `DFLASH_DS4_EXACT_PREFILL_BANDS`, the headline feature this PR adds. Without it, `--ds4-prefill exact` runs the q1 single-token default, so users following the documented recommended DeepSeek config never get the q2–q4 exact prefill bands the PR exists to provide (per the PR description the bands activate only with `DFLASH_DS4_EXACT_PREFILL_BANDS=1`). Add the flag to the recommended DeepSeek rows (and note it in the README), or explicitly document that the recommended setup intentionally stays on the q1 default.</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
This was referenced Aug 19, 2026
Contributor
|
i'm wondering: should cc @davide221 |
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
2 participants
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.
Summary
Adds an opt-in exact-prefill band scheduler for DeepSeek-V4 while keeping tokenwise q1 exact prefill as the safe default.
DFLASH_DS4_EXACT_PREFILL_BANDS=1;Classification: primarily an opt-in performance feature, with correctness fixes needed to preserve exact behavior. It is not a general bug-fix PR.
Fresh matched performance
Freshly remeasured on Lucebox6 at the published PR head
42d5c3e7f6672a840e2e1c4cbaf036a653f12804. The comparison used one matched dual-GPU heterogeneous topology, exact prefill,chunk=4, the same model and prompt, caches disabled, two warmups, and five measured requests per launch.The pooled q1 reference is 16.395 tok/s. Exact bands are 5.95% faster at 17.37 tok/s. Both q1 brackets agree, every response produced SHA-256
cd5cb9fb5ac3c4f4007e8b41d117da21622439cd05c1728f3e82f90e4f869dad, and every request was cache-cold.Evidence subset:
/home/cheese/pr590-pr633-report-requal2-20260825T140000ZPR590_SHA256SUMSSHA-256:dc6b7340b5b1e044350c7024aef03d570723cf3c767364cf6df7b3dda532ed8cPlacement result from Lucebox4
The earlier cache-cold candidate placement screen remains useful as a separate system result:
Candidate heterogeneous / candidate Strix-only was 3.3084x. This compares hardware placements; it is not the speedup from exact bands. Within-topology outputs were stable, while the historical screen did not establish cross-topology output parity.
Historical evidence:
/home/cheese/pr590/evidence/t07-goal-combined-3605716a-20260818-r4Seal SHA-256:
11bbd6a66f7d549c0a80f886234d047920938d442906c41438210d107100e3b5Correctness and validation
gfx1151;gfx1201, with HIP graphs enabled;test_deepseek4_unit: PASS on physical gfx1201 and gfx1151;Live status
42d5c3e7f6672a840e2e1c4cbaf036a653f1280499ab4cebd331310adcc37c5ab89e30323b3ddf27main.No merge-ready claim is made until the branch is reconciled and the resulting head is checked.