Skip to content

qwen4_exp: warn when ROWINV runs without the in-pass checkpoint - #210

Merged
AzeezIsh merged 2 commits into
mainfrom
fix/rowinv-needs-midchunk-ckpt
Oct 27, 2026
Merged

AzeezIsh merged 2 commits into
mainfrom
fix/rowinv-needs-midchunk-ckpt

Conversation

@AzeezIsh

Copy link
Copy Markdown
Contributor

Problem

ATLAS_QWEN4EXP_PREFILL_ROWINV=1 turns off the tail-checkpoint split, because a block-grid cut is off the 64-row GDN grid. It relies on ATLAS_QWEN4EXP_PREFILL_MIDCHUNK_CKPT=1 to capture the checkpoint in-pass instead. With ROWINV alone, no tail SSM checkpoint is ever saved, so every warm turn recomputes its whole prompt. Nothing reports this; the only trace is "Prefix cache hit … but no SSM snapshot" at INFO.

Measured on Flash-Next FP4-Local, agentic-webserver 50, GB10, prefix cache on:

profile checkpoints / intermediate hits TTFT p50 s/turn
default 1338 / 299 632 ms 12.43
ROWINV + MULTI + MULTI_CACHED + CODISPATCH 0 / 0 3062 ms 13.97
same + MIDCHUNK_CKPT — / 307 510 ms 11.76

Change

pc_policy::rowinv_tail_unsplit(in_pass) replaces the inline qwen4exp_rowinv::on() at the tail-split site and returns the same value. When ROWINV is on and the in-pass checkpoint is off, it logs one WARN. The guard is the existing split condition: last chunk, SSM model, snapshots on, prefix cache active. No behaviour change.

Verified (sparky GB10)

  • cargo fmt --check, cargo clippy --release -p spark-model -- -D warnings, cargo test -p spark-model --lib pc_policy (11 passed).
  • Live, two-turn chat: ROWINV alone → 1 warning, 0 intermediate hits; ROWINV + MIDCHUNK_CKPT → 0 warnings, 1 intermediate hit.

🤖 Generated with Claude Code

ATLAS_QWEN4EXP_PREFILL_ROWINV=1 disables the tail-checkpoint split (a
block-grid cut is off the 64-row GDN grid) and relies on
ATLAS_QWEN4EXP_PREFILL_MIDCHUNK_CKPT=1 to capture the checkpoint in-pass.
With ROWINV alone no tail SSM checkpoint is ever saved, so every warm
turn recomputes its whole prompt, silently. Measured on Flash-Next
FP4-Local agentic-webserver 50 (GB10): 0 checkpoints, TTFT p50
632 -> 3062 ms, 13.97 vs 12.43 s/turn. With MIDCHUNK_CKPT added: 307
intermediate hits, 11.76 s/turn.

Warn once at the first last-chunk where it applies. No behaviour change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@maplepaladin73

Copy link
Copy Markdown
Contributor

Nice catch on the silent no-checkpoint case. One suggestion: rowinv_tail_unsplit has no test, and none of the existing spark-model lib tests reach it. You can't drive it directly in a test because qwen4exp_rowinv::on() reads ATLAS_QWEN4EXP_PREFILL_ROWINV once per process (OnceLock in spark_runtime::qwen4exp_prefill_rowinv) and the warn guard is a function-local static Once.

The patch below changes no behaviour. It moves the body into rowinv_tail_unsplit_with(rowinv, in_pass, &Once, emit): the wrapper keeps the env read, the static guard and the same tracing::warn! text. It then adds two tests to the existing pc_policy_tests.rs, so pc_policy.rs stays under the 500-line cap (456 → 467):

  • the truth table: the return value equals ROWINV for all four (rowinv, in_pass) cases, and the warn fires only for rowinv && !in_pass and only once across repeated calls;
  • the guard is shared: an in_pass call doesn't consume the once-guard, and the warning isn't repeated.

The patch applies cleanly to 1d51b6db. Results there, built with the GPU kernels compiled on an x86 box: cargo test -p spark-model --lib rowinv 3 passed, and the full cargo test -p spark-model --lib 1874 passed / 0 failed / 89 ignored (1872 before + the 2 new). cargo fmt --all --check is clean; clippy on spark-model --tests and the file-size-cap script were clean on the same patch on our #163-rebased tree.

diff --git a/crates/spark-model/src/model/trait_impl/prefill_b/pc_policy.rs b/crates/spark-model/src/model/trait_impl/prefill_b/pc_policy.rs
index 68c56380..df0e25ac 100644
--- a/crates/spark-model/src/model/trait_impl/prefill_b/pc_policy.rs
+++ b/crates/spark-model/src/model/trait_impl/prefill_b/pc_policy.rs
@@ -220,16 +220,27 @@ pub(in crate::model) fn tail_split_disabled() -> bool {
 /// FP4-Local agentic-webserver, 0 checkpoints, TTFT p50 632 -> 3062 ms.
 /// Warns once in that case.
 pub(in crate::model) fn rowinv_tail_unsplit(in_pass: bool) -> bool {
+    static WARN: std::sync::Once = std::sync::Once::new();
     let rowinv = crate::layers::ops::qwen4exp_rowinv::on();
+    rowinv_tail_unsplit_with(rowinv, in_pass, &WARN, || {
+        tracing::warn!(
+            "ATLAS_QWEN4EXP_PREFILL_ROWINV=1 without \
+             ATLAS_QWEN4EXP_PREFILL_MIDCHUNK_CKPT=1 saves no tail SSM checkpoint: \
+             warm turns will recompute their whole prompt"
+        )
+    })
+}
+
+/// [`rowinv_tail_unsplit`] with the flag, the once-guard and the warning
+/// passed in, so tests can drive it without the read-once env flag.
+fn rowinv_tail_unsplit_with(
+    rowinv: bool,
+    in_pass: bool,
+    warn: &std::sync::Once,
+    emit: impl FnOnce(),
+) -> bool {
     if rowinv && !in_pass {
-        static WARN: std::sync::Once = std::sync::Once::new();
-        WARN.call_once(|| {
-            tracing::warn!(
-                "ATLAS_QWEN4EXP_PREFILL_ROWINV=1 without \
-                 ATLAS_QWEN4EXP_PREFILL_MIDCHUNK_CKPT=1 saves no tail SSM checkpoint: \
-                 warm turns will recompute their whole prompt"
-            )
-        });
+        warn.call_once(emit);
     }
     rowinv
 }
diff --git a/crates/spark-model/src/model/trait_impl/prefill_b/pc_policy_tests.rs b/crates/spark-model/src/model/trait_impl/prefill_b/pc_policy_tests.rs
index eee53ac3..0a1845ae 100644
--- a/crates/spark-model/src/model/trait_impl/prefill_b/pc_policy_tests.rs
+++ b/crates/spark-model/src/model/trait_impl/prefill_b/pc_policy_tests.rs
@@ -5,7 +5,7 @@
 
 use super::{
     Agreed, agree_restore, branch_checkpoint_at, branch_split_at, layer_write_floor, replay_floor,
-    tail_cut,
+    rowinv_tail_unsplit_with, tail_cut,
 };
 
 const BS: usize = 16;
@@ -207,3 +207,44 @@ fn replay_floor_covers_the_replayed_matched_rows() {
     assert_eq!(replay_floor(true, 256, 256, 256, 100), 0);
     assert_eq!(replay_floor(false, 64, 0, 0, 70), 0);
 }
+
+/// `ATLAS_QWEN4EXP_PREFILL_ROWINV` decides alone whether the last chunk
+/// runs unsplit; `in_pass` only gates the warning, which fires for ROWINV
+/// without the in-pass checkpoint and at most once per guard.
+#[test]
+fn rowinv_tail_unsplit_truth_table_and_warn_once() {
+    use std::cell::Cell;
+    use std::sync::Once;
+    for (rowinv, in_pass, unsplit, warns) in [
+        (false, false, false, 0),
+        (false, true, false, 0),
+        (true, true, true, 0),
+        (true, false, true, 1),
+    ] {
+        let guard = Once::new();
+        let fired = Cell::new(0);
+        for _ in 0..3 {
+            let got =
+                rowinv_tail_unsplit_with(rowinv, in_pass, &guard, || fired.set(fired.get() + 1));
+            assert_eq!(got, unsplit, "rowinv={rowinv} in_pass={in_pass}");
+        }
+        assert_eq!(fired.get(), warns, "rowinv={rowinv} in_pass={in_pass}");
+    }
+}
+
+/// The once-guard is shared across `in_pass` values: a warning already
+/// given is not repeated, and an `in_pass` call never consumes it.
+#[test]
+fn rowinv_tail_unsplit_warn_guard_is_shared() {
+    use std::cell::Cell;
+    use std::sync::Once;
+    let guard = Once::new();
+    let fired = Cell::new(0);
+    let bump = || fired.set(fired.get() + 1);
+    assert!(rowinv_tail_unsplit_with(true, true, &guard, bump));
+    assert!(!guard.is_completed());
+    assert!(rowinv_tail_unsplit_with(true, false, &guard, bump));
+    assert!(rowinv_tail_unsplit_with(true, true, &guard, bump));
+    assert!(rowinv_tail_unsplit_with(true, false, &guard, bump));
+    assert_eq!(fired.get(), 1);
+}

This comment was prepared with AI assistance (OpenClaw agent) and reviewed by me.

Co-Authored-By: maplepaladin73 <215220483+maplepaladin73@users.noreply.github.com>

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@AzeezIsh

Copy link
Copy Markdown
Contributor Author

Thanks @maplepaladin73. I applied your patch verbatim as d221f72 (rowinv_tail_unsplit_with + the two tests), with you as co-author. pc_policy.rs is 456 → 467 lines. On sparky (GB10): cargo test -p spark-model --lib rowinv 3 passed, pc_policy 13 passed; fmt (pinned 1.93.1 + stable) and clippy are clean. This PR now rides integration train #212, which is being gated on FP4-Local (agentic 50 + BFCL ST-995) and merges once it passes.

@AzeezIsh
AzeezIsh merged commit c3bf9aa into main Oct 27, 2026
20 of 23 checks passed
AzeezIsh added a commit that referenced this pull request Oct 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants