From d4d12abab2df5681abe925159deda09f0f2291d8 Mon Sep 17 00:00:00 2001 From: jgrusewski Date: Fri, 1 May 2026 22:35:18 +0200 Subject: [PATCH] docs(sp5): close two minor review findings before A5 Two minor code-review nits accumulated across A1-A4 reviews; closing them in a single docs/comment-only commit before Layer A continues. 1. tests/sp5_producer_unit_tests.rs module docstring (A4 review): the file header still claimed it covered only A1's two kernels even though A2/A3/A4 had each appended their tests. Updated header to enumerate all 9 tests across A1-A4 (+ 1 grad_cosine_sim test landed in 4da6b34d7) so a reader cold to the file can locate each producer's coverage without scanning the body. 2. gpu_dqn_trainer.rs:2854 stale field comment `[B, TOTAL_ACTIONS(11)]` (A1 review): comment was left at 11 since the pre-4-direction layout but the actual slot count is 13 (4 dir + 3 mag + 3 ord + 3 urg). Updated to `[B, TOTAL_ACTIONS(13)]` with the per-branch breakdown inline so the field declaration matches the SP5 producer docstrings (e.g. q_branch_stats_kernel comments at line 277, 282). Comment-only changes; no behavior change. cargo check + cargo test --no-run both clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../ml/src/cuda_pipeline/gpu_dqn_trainer.rs | 2 +- crates/ml/tests/sp5_producer_unit_tests.rs | 22 ++++++++++++++----- docs/dqn-wire-up-audit.md | 2 ++ 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs b/crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs index 348d48d39..03e8c0514 100644 --- a/crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs +++ b/crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs @@ -2851,7 +2851,7 @@ pub struct GpuDqnTrainer { mse_loss_dev_ptr: u64, // ── Forward-only Q-value output ───────────────────────────────── - q_out_buf: CudaSlice, // [B, TOTAL_ACTIONS(11)] + q_out_buf: CudaSlice, // [B, TOTAL_ACTIONS(13)] — 4 direction + 3 magnitude + 3 order + 3 urgency /// Snapshot buffers for td_errors + per_sample_loss during eval replay. /// Prevents graph_forward replay from corrupting PER priorities. eval_td_snapshot: CudaSlice, diff --git a/crates/ml/tests/sp5_producer_unit_tests.rs b/crates/ml/tests/sp5_producer_unit_tests.rs index 4da1b94a9..5fe6c84ec 100644 --- a/crates/ml/tests/sp5_producer_unit_tests.rs +++ b/crates/ml/tests/sp5_producer_unit_tests.rs @@ -1,12 +1,24 @@ #![allow(unsafe_code)] // CUDA kernel launch + mapped-pinned memory. -//! SP5 Task A1 producer kernel unit tests. +//! SP5 Layer A producer kernel unit tests. //! -//! Tests cover the two SP5 Task A1 CUDA kernels: -//! 1. `q_branch_stats_update`: per-branch Q-statistics reduction. -//! 2. `pearl_1_atom_update`: Pearl 1 atom-span headroom controller. +//! Tests cover the SP5 Layer A CUDA kernels across Tasks A1–A4: //! -//! Both tests use analytically-known synthetic inputs with no CPU reference +//! Task A1 (Pearl 1 atom-span + Q-stats source): +//! 1. `sp5_q_branch_stats_uniform_input_matches_analytical_ground_truth` +//! 2. `pearl_1_atom_update_v_half_tracks_max_abs_dev_with_bootstrap_headroom` +//! Task A2 (Pearl 3 NoisyNet σ): +//! 3. `pearl_3_sigma_bootstrap_at_target_entropy` +//! 4. `pearl_3_sigma_fraction_increases_when_entropy_below_target` +//! Task A3 (Pearl 2 loss budget): +//! 5. `pearl_2_budget_flat_regime_iqn_floor_c51_takes_remainder` +//! 6. `pearl_2_budget_sharp_regime_iqn_dominates` +//! Task A4 (Pearl 4 per-group Adam β/β/ε + auxiliary cosine-sim): +//! 7. `pearl_4_adam_hparams_high_stability_yields_long_memory` +//! 8. `pearl_4_adam_hparams_low_stability_yields_short_memory` +//! 9. `grad_cosine_sim_per_group_dot_norm_and_writeback` +//! +//! All tests use analytically-known synthetic inputs with no CPU reference //! implementation, per `feedback_no_cpu_test_fallbacks.md`. All mapped-pinned //! memory (no DtoH), all `#[ignore]`-gated for GPU. //! diff --git a/docs/dqn-wire-up-audit.md b/docs/dqn-wire-up-audit.md index c8fd41da0..740a423dc 100644 --- a/docs/dqn-wire-up-audit.md +++ b/docs/dqn-wire-up-audit.md @@ -2,6 +2,8 @@ **Status:** Populated during Plan 1 Task 6 (A.5 orphan audit). Updated on every commit per Invariant 7. +SP5 docs/comment-only fix-up — close two minor review findings before Layer A Task A5 (2026-05-01): two minor code-review nits accumulated across A1-A4 reviews; closed in a single docs/comment-only commit. (1) `tests/sp5_producer_unit_tests.rs` module docstring (A4 review) updated to enumerate all 9 SP5 Layer A tests (q_branch_stats + pearl_1 + pearl_3 × 2 + pearl_2 × 2 + pearl_4 × 2 + grad_cosine_sim) so a reader cold to the file can locate each producer's coverage without scanning the body. (2) `gpu_dqn_trainer.rs:2854` stale field comment `// [B, TOTAL_ACTIONS(11)]` (A1 review) updated to `// [B, TOTAL_ACTIONS(13)] — 4 direction + 3 magnitude + 3 order + 3 urgency` so the field declaration matches the SP5 producer docstrings (e.g. q_branch_stats_kernel comments at line 277/282) and the 4-branch action layout established in commit `2fb30f098`. Comment-only changes; no behavior change. cargo check + cargo test --no-run both clean (11 pre-existing warnings, none new). + SP5 Task A4 fix-up — grad_cosine_sim_update unit test added (2026-05-01): code-quality review caught that `pearl_4_adam_hparams_kernel` had direct GPU tests but the auxiliary `grad_cosine_sim_kernel` had none. Test 7 and Test 8 launched the hparams kernel directly with pre-baked cosine inputs, never exercising the per-group reduction (dot + 2× L2 norm sums) or the writeback (grad_curr → grad_prev for next step's comparison). The writeback in particular is load-bearing for cross-step cosine evolution — if broken, every subsequent step's cosine_sim is computed against a stale or mis-aligned previous gradient. Fix adds Test 9 `grad_cosine_sim_per_group_dot_norm_and_writeback` with 8 analytically-known synthetic group cases: identical (cos=+1), orthogonal (0), antiparallel (-1), Pythagorean (cos=+1, |c|=5), cold-start curr (zero grad → cos=0, |c|=0), cold-start prev (Pearl A sentinel state → cos=0, |c|=1), and 2 repeats. NO CPU oracle — expected values are unit-vector cosines from the kernel formula. Writeback verified by reading grad_prev_buf after the kernel runs and asserting bit-identity with grad_curr_buf for all 32 elements. Touched: `crates/ml/tests/sp5_producer_unit_tests.rs` (+1 cubin const + 1 loader + 120 LOC test). cargo test --no-run clean. SP5 Task A4 — Pearl 4 per-group Adam β1/β2/ε GPU producers (Layer A, 2026-05-01): two new CUDA kernels (`grad_cosine_sim_kernel.cu`, `pearl_4_adam_hparams_kernel.cu`) and one `MappedF32Buffer` (`grad_prev_buf_per_group`) land as Layer A additive producers that feed ISV slots [226..250) with per-param-group Adam hyperparameters. `grad_cosine_sim_update`: single-block 8-thread kernel (one thread per SP4 param group: DqnTrunk/Value/Branches/IQN/IqlHigh/IqlLow/Attn/Curiosity), reads `grad_buf[total_params]` + `grad_prev_buf[total_params]` + `group_param_offsets[9]` (prefix sums in element units); computes `dot = Σ gc·gp`, `norm_curr_sq = Σ gc²`, `norm_prev_sq = Σ gp²` per group; writes `cos_sim[8]→scratch[131..139)` and `l2_norm[8]→scratch[139..147)`; writebacks `grad_curr → grad_prev_buf` for the next step's comparison (same thread — no race). `pearl_4_adam_hparams_update`: single-block 8-thread kernel, reads `cos_sim[8]` + `l2_norm[8]` from scratch; clips stability to `[0, 1]`; computes `β1 = 0.85 + 0.10×stability ∈ [0.85, 0.95]`, `β2 = 0.99 + 0.0095×stability ∈ [0.99, 0.9995]`, `ε = clamp(grad_norm × 1e-7, 1e-10, 1e-6)`; writes `β1[8]→scratch[147..155)`, `β2[8]→scratch[155..163)`, `ε[8]→scratch[163..171)`. Structural envelopes (Invariant 1 anchors per spec): β1∈[0.85, 0.95], β2∈[0.99, 0.9995], ε∈[1e-10, 1e-6]. `launch_sp5_pearl_4_adam_hparams` fires 24 `launch_apply_pearls` calls: β1[8] → ISV[226..234), β2[8] → ISV[234..242), ε[8] → ISV[242..250). Uses `ALPHA_META_PEARL_4=5e-4` (half SP4 default 1e-3) per theoretical-caveat mitigation — slow β change rate reduces instability risk from adaptive β breaking Adam's constant-β convergence proof. Pearl A sentinel 0 at fold boundary fires first-observation replacement (cosine_sim=0 → low-stability envelope endpoints). Aux groups 3-7 (IQN/IqlHigh/IqlLow/Attn/Curiosity) have non-contiguous grad allocations; zero-width sentinel offsets `[tp, tp, tp, tp, tp]` in `group_param_offsets_buf` prevent the kernel inner loop from executing for those groups, yielding cosine_sim=0 → β1/β2 start at low-stability floors. `apply_pearls_kernel.cu` signature gained `float alpha_meta` parameter (migration of all 18 call sites atomic per `feedback_no_partial_refactor.md`: 1 in `gpu_experience_collector.rs`, 17 in `gpu_dqn_trainer.rs`). Buffer growth: `producer_step_scratch_buf` 131→171 slots (SP5_SCRATCH_TOTAL); 5 new scratch constants `SCRATCH_PEARL_4_{COSINE=131, L2=139, BETA1=147, BETA2=155, EPS=163}`. StateResetRegistry gains 4 new FoldReset entries: `sp5_adam_beta1` (ISV[226..234)), `sp5_adam_beta2` (ISV[234..242)), `sp5_adam_eps` (ISV[242..250)), `sp5_grad_prev_buf` (grad_prev_buf_per_group — zero at fold boundary so cosine_sim=0 fires Pearl A bootstrap). Wire-up in training_loop.rs: `launch_sp5_pearl_4_adam_hparams()` called immediately after `launch_sp5_pearl_2_budget()` with `tracing::warn!` on error. Two GPU-only `#[ignore = "requires GPU"]` unit tests: (7) high-stability (cosine_sim=1.0 → β1=0.95, β2=0.9995, ε within envelope); (8) low-stability (cosine_sim=0.0 → β1=0.85, β2=0.99, ε within envelope). No CPU compute, no HtoD/HtoH, no atomicAdd, no stubs, no consumer migration. Touched: `cuda_pipeline/grad_cosine_sim_kernel.cu` (new), `cuda_pipeline/pearl_4_adam_hparams_kernel.cu` (new), `cuda_pipeline/apply_pearls_kernel.cu` (+alpha_meta param), `cuda_pipeline/sp4_wiener_ema.rs` (+alpha_meta arg), `cuda_pipeline/gpu_experience_collector.rs` (+1 call site ALPHA_META arg), `build.rs` (+2 cubin entries), `cuda_pipeline/gpu_dqn_trainer.rs` (SP5_SCRATCH_TOTAL 131→171 + 5 new constants + 2 static cubins + 4 struct fields + constructor allocs + struct initializer + 17 call-site ALPHA_META args + launch method), `trainers/dqn/state_reset_registry.rs` (+4 FoldReset entries), `trainers/dqn/trainer/training_loop.rs` (+5 LOC after Pearl 2 launch), `tests/sp5_producer_unit_tests.rs` (+2 GPU-only tests + cubin constant + loader helper). Hard rules: feedback_no_cpu_compute_strict, feedback_no_atomicadd, feedback_no_htod_htoh_only_mapped_pinned, feedback_no_stubs, feedback_no_partial_refactor (apply_pearls 18 call sites all migrated).