perf(ds4): overlap q5 verification and stabilize 16K - #575
Conversation
5417326 to
e24ad65
Compare
8bc7cdf to
39dd741
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 17 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
e24ad65 to
4ce3b82
Compare
3240627 to
805d5a7
Compare
b5d3515 to
3244b9c
Compare
bae0822 to
c358b9d
Compare
|
@cubic-dev-ai review the current head after the branch rebase. Please focus on correctness, maintainability, security, and updated harness paths. |
@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 28 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@cubic-dev-ai review the current head. All 12 findings from the previous review were addressed with focused regression coverage; please re-check correctness, maintainability, and security. |
@davide221 I have started the AI code review. It will take a few minutes to complete. |
50f9f2b to
ceaf788
Compare
There was a problem hiding this comment.
7 issues found and verified against the latest diff
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="harness/qualification/deepseek4/analyze_rocprof_overlap.py">
<violation number="1" location="harness/qualification/deepseek4/analyze_rocprof_overlap.py:135">
P3: `--top` is the only numeric option not range-validated; every other flag (floats, `--timeline-max`, `--timeline-merge-gap-us`) rejects invalid values. A negative `--top` slips through and, because the top-kernel section is gated only by `args.top > 0`, it silently disables `top_kernels_*` output. Validate `--top >= 0` alongside the other numeric options so an invalid setting fails loudly instead of changing behavior without notice.</violation>
<violation number="2" location="harness/qualification/deepseek4/analyze_rocprof_overlap.py:182">
P2: Finite extreme values still crash the analyzer with an uncaught `OverflowError` during nanosecond conversion. Validate the scaled values, or catch the conversion overflow and reject these options through `parser.error()` before processing the trace.</violation>
<violation number="3" location="harness/qualification/deepseek4/analyze_rocprof_overlap.py:235">
P2: A tiny positive `--bin-ms` can turn a normal trace into an effectively unbounded one-nanosecond output loop. Reject bin sizes that would exceed a fixed maximum number of bins before entering the loop.</violation>
</file>
<file name="server/deps/llama.cpp/ggml/src/ggml.c">
<violation number="1" location="server/deps/llama.cpp/ggml/src/ggml.c:8427">
P2: When `global_ids` has dimensions beyond route and token, this constructor creates an output for every plane but the balanced-owner kernel processes only the first plane, leaving the remaining IDs uninitialized. Reject non-2D route tensors here, as `ggml_ds4_moe_align_ids` already does, or add full multidimensional support.</violation>
</file>
<file name="server/src/common/moe_hybrid_ffn_eval.cpp">
<violation number="1" location="server/src/common/moe_hybrid_ffn_eval.cpp:853">
P1: When dynamic route balance is enabled with a CPU cold-expert backend, the secondary balanced-ID node runs on CPU and aborts because `GGML_OP_MOE_FUSED` has no CPU implementation. Guard this mode on backend capability or provide a CPU owner-ID implementation before routing the node to the peer.</violation>
</file>
<file name="harness/qualification/deepseek4/qualify_ds4_q5_amd.sh">
<violation number="1" location="harness/qualification/deepseek4/qualify_ds4_q5_amd.sh:368">
P2: When `PROFILED_SERVER_BIN` is missing or non-executable, this forwarding loop lets qualification reach GPU setup and then fail when the profiler wrapper launches the server. Validate `PROFILED_SERVER_BIN` as an executable during preflight, alongside `SERVER_BIN`, so invalid profiler runs fail before changing performance levels.
(Based on your team's feedback about validating environment-provided binaries.) .</violation>
</file>
<file name="server/src/deepseek4/deepseek4_fused_verify.inc">
<violation number="1" location="server/src/deepseek4/deepseek4_fused_verify.inc:185">
P3: When dynamic route balance is enabled, `preferred_hot` is already `layer.hot_local_by_global` (selected above), so this fallback assigns `hot` the exact value it already holds — a no-op. The guard only has an effect on the static path where `preferred_hot` is the phase-specific `decode_hot_local_by_global`. Gate the fallback on `!dynamic_route_balance` (or drop it in dynamic mode) so the dead assignment isn't silently carried in the balance path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| [](int32_t local) { return local < 0; })) { | ||
| return false; | ||
| } | ||
| owner.local_ids = track(ggml_ds4_moe_balanced_owner_ids( |
There was a problem hiding this comment.
P1: When dynamic route balance is enabled with a CPU cold-expert backend, the secondary balanced-ID node runs on CPU and aborts because GGML_OP_MOE_FUSED has no CPU implementation. Guard this mode on backend capability or provide a CPU owner-ID implementation before routing the node to the peer.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/common/moe_hybrid_ffn_eval.cpp, line 853:
<comment>When dynamic route balance is enabled with a CPU cold-expert backend, the secondary balanced-ID node runs on CPU and aborts because `GGML_OP_MOE_FUSED` has no CPU implementation. Guard this mode on backend capability or provide a CPU owner-ID implementation before routing the node to the peer.</comment>
<file context>
@@ -784,6 +842,24 @@ static bool build_moe_owner_remap(
+ [](int32_t local) { return local < 0; })) {
+ return false;
+ }
+ owner.local_ids = track(ggml_ds4_moe_balanced_owner_ids(
+ ctx, global_ids, router_weights,
+ *owner.local_lut, *owner.valid_lut,
</file context>
| merged = {agent: merge_intervals(intervals_by_agent[agent]) for agent in agents} | ||
| overlap = intersect_intervals(merged[agents[0]], merged[agents[1]]) | ||
|
|
||
| window_start = trace_start + int(args.window_start_s * 1e9) |
There was a problem hiding this comment.
P2: Finite extreme values still crash the analyzer with an uncaught OverflowError during nanosecond conversion. Validate the scaled values, or catch the conversion overflow and reject these options through parser.error() before processing the trace.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At harness/qualification/deepseek4/analyze_rocprof_overlap.py, line 182:
<comment>Finite extreme values still crash the analyzer with an uncaught `OverflowError` during nanosecond conversion. Validate the scaled values, or catch the conversion overflow and reject these options through `parser.error()` before processing the trace.</comment>
<file context>
@@ -0,0 +1,282 @@
+ merged = {agent: merge_intervals(intervals_by_agent[agent]) for agent in agents}
+ overlap = intersect_intervals(merged[agents[0]], merged[agents[1]])
+
+ window_start = trace_start + int(args.window_start_s * 1e9)
+ requested_end = (
+ trace_start + int(args.window_end_s * 1e9)
</file context>
| f"either_busy_s={union_ns/1e9:.3f}" | ||
| ) | ||
|
|
||
| bin_ns = max(1, int(args.bin_ms * 1e6)) |
There was a problem hiding this comment.
P2: A tiny positive --bin-ms can turn a normal trace into an effectively unbounded one-nanosecond output loop. Reject bin sizes that would exceed a fixed maximum number of bins before entering the loop.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At harness/qualification/deepseek4/analyze_rocprof_overlap.py, line 235:
<comment>A tiny positive `--bin-ms` can turn a normal trace into an effectively unbounded one-nanosecond output loop. Reject bin sizes that would exceed a fixed maximum number of bins before entering the loop.</comment>
<file context>
@@ -0,0 +1,282 @@
+ f"either_busy_s={union_ns/1e9:.3f}"
+ )
+
+ bin_ns = max(1, int(args.bin_ms * 1e6))
+ print("bin_start_s,agent1_busy_pct,agent2_busy_pct,both_busy_pct")
+ cursor = window_start
</file context>
| GGML_ASSERT(router_weights->type == GGML_TYPE_F32); | ||
| GGML_ASSERT(local_id_lut->type == GGML_TYPE_I32); | ||
| GGML_ASSERT(main_candidate_lut->type == GGML_TYPE_F32); | ||
| GGML_ASSERT(ggml_are_same_shape(global_ids, router_weights)); |
There was a problem hiding this comment.
P2: When global_ids has dimensions beyond route and token, this constructor creates an output for every plane but the balanced-owner kernel processes only the first plane, leaving the remaining IDs uninitialized. Reject non-2D route tensors here, as ggml_ds4_moe_align_ids already does, or add full multidimensional support.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/deps/llama.cpp/ggml/src/ggml.c, line 8427:
<comment>When `global_ids` has dimensions beyond route and token, this constructor creates an output for every plane but the balanced-owner kernel processes only the first plane, leaving the remaining IDs uninitialized. Reject non-2D route tensors here, as `ggml_ds4_moe_align_ids` already does, or add full multidimensional support.</comment>
<file context>
@@ -8412,6 +8412,43 @@ struct ggml_tensor * ggml_ds4_moe_align_ids(
+ GGML_ASSERT(router_weights->type == GGML_TYPE_F32);
+ GGML_ASSERT(local_id_lut->type == GGML_TYPE_I32);
+ GGML_ASSERT(main_candidate_lut->type == GGML_TYPE_F32);
+ GGML_ASSERT(ggml_are_same_shape(global_ids, router_weights));
+ GGML_ASSERT(ggml_nelements(local_id_lut) ==
+ ggml_nelements(main_candidate_lut));
</file context>
| GGML_ASSERT(ggml_are_same_shape(global_ids, router_weights)); | |
| GGML_ASSERT(ggml_n_dims(global_ids) == 2); | |
| GGML_ASSERT(ggml_are_same_shape(global_ids, router_weights)); |
|
|
||
| # Preserve only the explicit profiler-wrapper controls across env -i. Ordinary | ||
| # qualification runs leave these unset and retain the exact established env. | ||
| for profiler_var in PROFILED_SERVER_BIN ROCPROF_OUTPUT_DIR \ |
There was a problem hiding this comment.
P2: When PROFILED_SERVER_BIN is missing or non-executable, this forwarding loop lets qualification reach GPU setup and then fail when the profiler wrapper launches the server. Validate PROFILED_SERVER_BIN as an executable during preflight, alongside SERVER_BIN, so invalid profiler runs fail before changing performance levels.
(Based on your team's feedback about validating environment-provided binaries.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At harness/qualification/deepseek4/qualify_ds4_q5_amd.sh, line 368:
<comment>When `PROFILED_SERVER_BIN` is missing or non-executable, this forwarding loop lets qualification reach GPU setup and then fail when the profiler wrapper launches the server. Validate `PROFILED_SERVER_BIN` as an executable during preflight, alongside `SERVER_BIN`, so invalid profiler runs fail before changing performance levels.
(Based on your team's feedback about validating environment-provided binaries.) .</comment>
<file context>
@@ -275,6 +336,42 @@ for visibility_var in HIP_VISIBLE_DEVICES ROCR_VISIBLE_DEVICES; do
+
+# Preserve only the explicit profiler-wrapper controls across env -i. Ordinary
+# qualification runs leave these unset and retain the exact established env.
+for profiler_var in PROFILED_SERVER_BIN ROCPROF_OUTPUT_DIR \
+ ROCPROF_START_SECONDS ROCPROF_DURATION_SECONDS; do
+ if [[ -n "${!profiler_var:-}" ]]; then
</file context>
| // A phase-specific placement may move a resident expert to the | ||
| // peer only when the peer owns a copy. Otherwise retain its | ||
| // physical main-owner route so no selected expert is dropped. | ||
| if (hot < 0 && cold < 0) { |
There was a problem hiding this comment.
P3: When dynamic route balance is enabled, preferred_hot is already layer.hot_local_by_global (selected above), so this fallback assigns hot the exact value it already holds — a no-op. The guard only has an effect on the static path where preferred_hot is the phase-specific decode_hot_local_by_global. Gate the fallback on !dynamic_route_balance (or drop it in dynamic mode) so the dead assignment isn't silently carried in the balance path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/deepseek4/deepseek4_fused_verify.inc, line 185:
<comment>When dynamic route balance is enabled, `preferred_hot` is already `layer.hot_local_by_global` (selected above), so this fallback assigns `hot` the exact value it already holds — a no-op. The guard only has an effect on the static path where `preferred_hot` is the phase-specific `decode_hot_local_by_global`. Gate the fallback on `!dynamic_route_balance` (or drop it in dynamic mode) so the dead assignment isn't silently carried in the balance path.</comment>
<file context>
@@ -166,20 +166,38 @@ static void ds4_fused_verify_refresh_hybrid_luts(
+ // A phase-specific placement may move a resident expert to the
+ // peer only when the peer owns a copy. Otherwise retain its
+ // physical main-owner route so no selected expert is dropped.
+ if (hot < 0 && cold < 0) {
+ hot = layer.hot_local_by_global[(size_t) ie];
+ }
</file context>
| parser.add_argument("--bin-ms", type=float, default=1000.0) | ||
| parser.add_argument("--window-start-s", type=float, default=0.0) | ||
| parser.add_argument("--window-end-s", type=float) | ||
| parser.add_argument("--top", type=int, default=12) |
There was a problem hiding this comment.
P3: --top is the only numeric option not range-validated; every other flag (floats, --timeline-max, --timeline-merge-gap-us) rejects invalid values. A negative --top slips through and, because the top-kernel section is gated only by args.top > 0, it silently disables top_kernels_* output. Validate --top >= 0 alongside the other numeric options so an invalid setting fails loudly instead of changing behavior without notice.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At harness/qualification/deepseek4/analyze_rocprof_overlap.py, line 135:
<comment>`--top` is the only numeric option not range-validated; every other flag (floats, `--timeline-max`, `--timeline-merge-gap-us`) rejects invalid values. A negative `--top` slips through and, because the top-kernel section is gated only by `args.top > 0`, it silently disables `top_kernels_*` output. Validate `--top >= 0` alongside the other numeric options so an invalid setting fails loudly instead of changing behavior without notice.</comment>
<file context>
@@ -0,0 +1,282 @@
+ parser.add_argument("--bin-ms", type=float, default=1000.0)
+ parser.add_argument("--window-start-s", type=float, default=0.0)
+ parser.add_argument("--window-end-s", type=float)
+ parser.add_argument("--top", type=int, default=12)
+ parser.add_argument(
+ "--timeline-max", type=int, default=0,
</file context>
1 participant
Summary
This PR is stacked on #569.
Validation
Configuration: R9700 target plus Strix Halo expert device, q=5, four routed experts, two warmups and three measured 128-token replies per context.
All 25 responses matched the expected deterministic hash. Peak target-GPU memory use was 31.089 GiB.
test_deepseek4_unit: passedtest_server_unit: 322 passed, 0 failedgit diff --check: passed