gitadmin/foxhunt
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(dqn): SP1 Phase A — slot 24/25 buffer correction + dormancy reconciliation

Browse Source 
Three review-driven fixes:

1. Slot 24/25 cite the actual production buffers d_value_logits_buf
   (gpu_dqn_trainer.rs:3123) and d_adv_logits_buf (:3125) — the f32
   atomicAdd buffers that launch_c51_grad writes — not the staging
   buffers at :3127/:3129. Pointer expressions exist at the launch
   site (:17707/:17708), making new accessors optional. Task 3 summary
   and priority-list entries updated to match.

2. Explicit plan-supersession note inserted immediately above the
   per-slot table: the audit's per-slot allocation supersedes the plan
   Task 2 step 6 name table. The plan's allocation was placeholder;
   the audit's is grounded in per-kernel inspection. Task 2 should use
   the audit's names (d_value_logits_buf, d_adv_logits_buf, iqn_trunk_m,
   iqn_d_h_s2_ptr, d_branch_logits_buf, etc.).

3. Summary suspicion-ranking row #3 reconciled with slot-28 dormancy:
   iqn_backward_per_sample has no Rust caller (verified via
   grep crates/ml/src/), so row #3 is re-pointed at the production
   kernel iqn_quantile_huber_loss (iqn_dual_head_kernel.cu:1346-1413,
   loaded at gpu_iqn_head.rs:2114). Same unsafe-write pattern (no
   isfinite guard at line 1410's d_q_online[idx] = qw*d_huber/Q),
   now attributed to the live path. apply_iqn_trunk_gradient at #1
   stands — its reasoning (orchestrator consuming iqn_d_h_s2_ptr) is
   unchanged by which specific kernel writes that buffer.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
jgrusewski
2026-04-29 23:56:30 +02:00
parent 8fd9cc0603
commit 2406ebf2f7
1 changed files with 10 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
docs/dqn-backward-nan-audit.md
View File

@@ -477,7 +477,7 @@ Ranked by cumulative severity (number of unsafe patterns × severity), with `app
|---|---|---|---|---|---|
| 1 | `apply_iqn_trunk_gradient` (orchestrator + `encoder_backward_chain` cuBLAS leg + saxpy producer) | 0 | 3 | low–medium | Prior NEVER-CLOSED 8% step-2 NaN; `dqn_saxpy_f32_kernel` has no `isfinite` guard; F1 slot-6 fire pattern matches. |
| 2 | `bw_d_h_s2` accumulator path (Rust, 8 producers SAXPY/DtoD) | 0 | 2 | low (diagnostic) | Buffer received by `encoder_backward_chain`; one bad producer poisons the whole aux pass. |
| 3 | `iqn_backward_per_sample` + `iqn_weight_grad_reduce` | 0 | 1 | low | `save_dL_dq` written without `isfinite`; reduce loops have NO guard at lines 750-762 (existing guards at 689/712 demonstrate the precedent). |
| 3 | `iqn_quantile_huber_loss` (production IQN backward) | 0 | 1 | low | DORMANCY RECONCILIATION (2026-04-29): the previously-listed `iqn_backward_per_sample` + `iqn_weight_grad_reduce` pair is NOT loaded by any Rust caller (verified via `grep -rn "iqn_backward_per_sample" crates/ml/src/` — only the `.cu` definition matches; `gpu_iqn_head.rs:2099-2120` loads `iqn_loss_reduce`, `iqn_grad_norm_phase{1,2}`, `iqn_adam_kernel`, `iqn_quantile_huber_loss`, etc., but never `iqn_backward_per_sample`). The production IQN backward path is `iqn_quantile_huber_loss` (`iqn_dual_head_kernel.cu:1346-1413`), which writes `d_q_online[idx] = qw * d_huber / Q` at line 1410 with NO `isfinite` guard on `d_huber`, `qw`, or the inputs `online_val`/`target_val`. This f32 buffer binds Rust-side to `d_branch_logits_buf` (slot 28). The same unsafe-write pattern applies (no inline `isfinite`), now correctly attributed to the live kernel. The dormant-kernel `save_dL_dq` discussion in the kernel section is retained as historical documentation of a non-loaded path. Rank #1 (`apply_iqn_trunk_gradient`) still stands — its reasoning (orchestrator consuming `iqn_d_h_s2_ptr = gpu_iqn_head::d_h_s2_buf`) is unchanged by which specific kernel writes that buffer; the orchestrator consumes whatever the production path produced. |
| 4 | `c51_grad_kernel` | 0 | 1 | low | Reads ISV slots 12-21 every step; if any slot is NaN at fold boundary, the multiplicative bin_weight propagates NaN. |
| 5 | `apply_ensemble_diversity_backward` | 0 | 1 | low | Same suspect-class as IQN trunk (cuBLAS GEMMs + saxpy); currently dormant in production config. |
| 6 | Bottleneck Linear backward | 0 | 0 | N/A | Stride-padded SGEMM bug is FIXED (session_2026-04-05 finding #3); slot-6 + lda variants in place. Diagnostic slot 32 still useful. |
@@ -507,8 +507,8 @@ Plus the consumer + name-table sites already enumerated in Task 2's plan body:
1. **Slot 26** — `iqn_trunk_m` post-`encoder_backward_chain` in `apply_iqn_trunk_gradient` (highest suspicion — prior investigation).
2. **Slot 27** — `iqn_d_h_s2_ptr` post-DtoD copy into `bw_d_h_s2` (companion to slot 26; locates per-sample IQN backward as source).
3. **Slot 33** — `bw_d_h_s2` AFTER `backward_full` returns, BEFORE `encoder_backward_chain` begins (the main-path entry-point check).
4. **Slot 24** — `d_value_logits` post-`c51_grad_kernel`.
5. **Slot 25** — `d_adv_logits` post-`c51_grad_kernel`.
4. **Slot 24** — `d_value_logits_buf` post-`c51_grad_kernel` (the f32 atomicAdd buffer at `gpu_dqn_trainer.rs:3123`, NOT the staging `d_value_logits` at :3127).
5. **Slot 25** — `d_adv_logits_buf` post-`c51_grad_kernel` (the f32 atomicAdd buffer at `gpu_dqn_trainer.rs:3125`, NOT the staging `d_adv_logits` at :3129).
6. **Slot 34** — `bw_d_h_s2` AFTER `apply_aux_heads_backward` SAXPY (separates aux-head poisoning from main-path).
7. **Slot 35** — `bw_d_h_s2` AFTER `apply_iqn_trunk_gradient` DtoD copy (symmetric with slot 27 — distinguishes graph-capture DtoD edge cases).
8. **Slot 28** — `save_dL_dq` post-`iqn_backward_per_sample` (locates IQN per-sample backward as NaN seed for the weight-grad reduce path).
@@ -519,14 +519,18 @@ Plus the consumer + name-table sites already enumerated in Task 2's plan body:
If any of slots 24–35 are pre-empted by Phase B's actual implementation budget (e.g. cudarc wrapper limits or `nan_flags_buf` sizing constraints), the deferred two slots (29, 31) can be returned to the budget as the lowest-priority drops. Slots 26, 27, 33 are NON-NEGOTIABLE — without those three the prior-investigation hypothesis cannot be confirmed or refuted from one smoke run.
### Plan supersession note
The SP1 plan's Task 2 step 6 name table at `docs/superpowers/plans/2026-04-29-numerical-stability-sp1-f1-nan-root-cause.md:354-366` predates this audit's per-kernel inspection and uses placeholder names (`iqn_trunk_grad_out`, `iqn_aux_grad_out`, `c51_d_current_lp`, `c51_d_projected`, `cql_grad`, `ens_grad_out`, `aux_nb_dh_s2`, `aux_rg_dh_s2`, `bw_d_h_s2_pre_saxpy`, `bw_d_h_s2_post_saxpy`, `mse_grad`, `bottleneck_grad`). **This audit's per-slot table below supersedes the plan's name table for slots 24–35.** When Task 2 lands the `nan_flags_buf` 24→48 expansion, the slot-name strings should be the audit's names (the actual production buffer names: `d_value_logits_buf`, `d_adv_logits_buf`, `iqn_trunk_m`, `iqn_d_h_s2_ptr`, `d_branch_logits_buf`, `cql_d_value_logits`, `aux_dh_s2_nb_buf`, `ensemble_d_logits_buf`, `bn_d_concat_buf`, `bw_d_h_s2` ×3 call-sites), not the plan's. The plan's slot purposes are conceptually right but the buffer-name mapping was wrong; the audit's allocation maps each slot to the actual production buffer that the live code path writes.
## Per-slot Rust buffer-pointer expressions (Task 4 inputs)
Concrete handle to pass to `check_nan_f32(buf_ptr, len, flag_idx)` for each of slots 24–35. The "Owner struct" column is the Rust struct that owns the buffer (`GpuDqnTrainer` for everything except slot 31 which lives on `FusedDqnTraining`). Verified 2026-04-29 against `crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs`, `crates/ml/src/cuda_pipeline/gpu_iqn_head.rs`, `crates/ml/src/cuda_pipeline/gpu_aux_heads.rs`, and `crates/ml/src/trainers/dqn/fused_training.rs`.
| Slot | Buffer name (Rust) | Owner struct | Handle status | Rust expression |
|---|---|---|---|---|
| 24 | `d_value_logits` | `GpuDqnTrainer` | needs new accessor | private field at `gpu_dqn_trainer.rs:3127` (`CudaSlice<f32>`); add `pub(crate) fn d_value_logits_ptr(&self) -> u64 { self.d_value_logits.raw_ptr() }`. Length: `self.d_value_logits.len()`. |
| 25 | `d_adv_logits` | `GpuDqnTrainer` | needs new accessor | private field at `gpu_dqn_trainer.rs:3129` (`CudaSlice<f32>`); add `pub(crate) fn d_adv_logits_ptr(&self) -> u64 { self.d_adv_logits.raw_ptr() }`. Length: `self.d_adv_logits.len()`. |
| 24 | `d_value_logits_buf` (NOT `d_value_logits` — see note) | `GpuDqnTrainer` | accessor optional (pointer expression already constructed at launch site) | private field at `gpu_dqn_trainer.rs:3123` (`CudaSlice<f32>`, "f32 for native atomicAdd"). This is the f32 atomicAdd buffer that `c51_grad_kernel` writes via the launch site at `gpu_dqn_trainer.rs:17707` (`let d_value_logits_buf_ptr = self.d_value_logits_buf.raw_ptr();`). The check can therefore be inlined into `launch_c51_grad` immediately after the kernel launch (reusing the existing `d_value_logits_buf_ptr` local), OR — symmetric with the other slots — `pub(crate) fn d_value_logits_buf_ptr(&self) -> u64 { self.d_value_logits_buf.raw_ptr() }` can be added. Length: `self.d_value_logits_buf.len()` (= `B × NA`). NOTE: `d_value_logits` at line 3127 is an unrelated *staging* buffer for the backward pass; `c51_grad_kernel` does not write to it. |
| 25 | `d_adv_logits_buf` (NOT `d_adv_logits` — see note) | `GpuDqnTrainer` | accessor optional (pointer expression already constructed at launch site) | private field at `gpu_dqn_trainer.rs:3125` (`CudaSlice<f32>`, "f32 for native atomicAdd"). This is the f32 atomicAdd buffer that `c51_grad_kernel` writes via the launch site at `gpu_dqn_trainer.rs:17708` (`let d_adv_logits_buf_ptr = self.d_adv_logits_buf.raw_ptr();`). Same inline-vs-accessor choice as slot 24. Length: `self.d_adv_logits_buf.len()` (= `B × (b0+b1+b2+b3) × NA`). NOTE: `d_adv_logits` at line 3129 is an unrelated *staging* buffer for the backward pass; `c51_grad_kernel` does not write to it. |
| 26 | `iqn_trunk_m` | `GpuDqnTrainer` | already exists in `ptrs` | `self.ptrs.iqn_trunk_m`. Length: `trunk_grad_total` (per-call computed as the sum of `param_sizes[0..13)`; same value used at `gpu_dqn_trainer.rs:7014` for the SAXPY count). |
| 27 | `iqn_d_h_s2_ptr` (= `gpu_iqn_head::d_h_s2_buf`) | `GpuIqnHead` | needs new accessor | already passed in as a `u64` arg at `gpu_dqn_trainer.rs:6841`; the source field is `d_h_s2_buf` allocated at `gpu_iqn_head.rs:467`. Add `pub(crate) fn d_h_s2_buf_ptr(&self) -> u64 { self.d_h_s2_buf.raw_ptr() }` on `GpuIqnHead`, OR — simpler — call `check_nan_f32(iqn_d_h_s2_ptr, b * SH2, 27)` inside `apply_iqn_trunk_gradient` between line 6880 (the DtoD copy) and line 6891 (the consume). Length: `B × hidden_dim` = `B × SH2 = B × 128`. |
| 28 | `d_branch_logits_buf` (NOT `save_dL_dq` — see note) | `GpuIqnHead` | needs new accessor | private field at `gpu_iqn_head.rs:305`; add `pub(crate) fn d_branch_logits_buf_ptr(&self) -> u64 { self.d_branch_logits_buf.raw_ptr() }`. Length: `tba * bq` (= `total_branch_atoms × (B * IQN_NUM_QUANTILES)`). NOTE: the audit's earlier "save_dL_dq" reference is the kernel-internal name used inside `iqn_dual_head_kernel.cu:514` of the dormant `iqn_backward_per_sample` kernel. The PRODUCTION IQN backward path is `iqn_quantile_huber_loss` (kernel at `iqn_dual_head_kernel.cu:1346`), whose Rust-side dL/dq output is `d_branch_logits_buf`. Phase B Task 4 must instrument this buffer; the audit-text reference to `iqn_backward_per_sample` is documenting a non-loaded kernel. |
@@ -539,7 +543,7 @@ Concrete handle to pass to `check_nan_f32(buf_ptr, len, flag_idx)` for each of s
| 35 | `bw_d_h_s2` (post-IQN DtoD) | `GpuDqnTrainer` | already exists in `ptrs` | `self.ptrs.bw_d_h_s2`. Same buffer as slots 33/34; distinction is again call site. Length: `B × SH2`. |
**Summary for Task 3** (accessor additions):
- `GpuDqnTrainer` (in `gpu_dqn_trainer.rs`): add 4 accessors — `d_value_logits_ptr`, `d_adv_logits_ptr`, `aux_dh_s2_nb_buf_ptr`, plus `cql_d_value_logits_ptr` if slot 29 is un-deferred.
- `GpuDqnTrainer` (in `gpu_dqn_trainer.rs`): add `aux_dh_s2_nb_buf_ptr` accessor (slot 30); plus `cql_d_value_logits_ptr` if slot 29 is un-deferred. Slots 24/25 (`d_value_logits_buf` / `d_adv_logits_buf`) do NOT need new accessors — the pointer expressions already exist as locals (`d_value_logits_buf_ptr` / `d_adv_logits_buf_ptr`) inside `launch_c51_grad` at `gpu_dqn_trainer.rs:17707-17708`, so the slot-24/25 NaN check can be inlined post-launch in that function. (If symmetric accessors are preferred for orchestrator-side checks, add `d_value_logits_buf_ptr` / `d_adv_logits_buf_ptr` reading the `_buf` fields at lines 3123/3125, NOT the staging fields at 3127/3129.)
- `GpuIqnHead` (in `gpu_iqn_head.rs`): add up to 2 accessors — `d_h_s2_buf_ptr` (slot 27, optional if the check is inlined into `apply_iqn_trunk_gradient`) and `d_branch_logits_buf_ptr` (slot 28, required).
- `FusedDqnTraining` (in `fused_training.rs`): add `ensemble_d_logits_buf_ptr` if slot 31 is un-deferred.
- No accessor needed for slots 26, 32, 33, 34, 35 — already accessible via `self.ptrs.*` or an existing `pub(crate)` accessor.