From 3ad5e011b97e2c1514f3d3adfaefb38f50049ef3 Mon Sep 17 00:00:00 2001 From: jgrusewski Date: Sat, 2 May 2026 01:27:40 +0200 Subject: [PATCH] =?UTF-8?q?fix(sp5):=20Layer=20B=20fix-up=20=E2=80=94=20cl?= =?UTF-8?q?ose=203=20review=20findings=20before=20Layer=20C?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comprehensive review of Layer B (commit 99367b9c6) caught one critical and two important findings. All three fix in one atomic commit. Critical: IQL Adam groups 4+5 (IqlHigh, IqlLow) missed the Pearl 4 migration. gpu_iql_trainer.rs::train_value_step still read self.config.beta1=0.9, self.config.beta2=0.999 at runtime. ISV[230] (adam_beta1(4)) and ISV[231] (adam_beta1(5)) were populated by the Layer A producer but unconsumed — partial refactor of the 8-group Pearl 4 contract. Fix mirrors the Curiosity pattern: 3 new iql_beta1/beta2/epsilon parameters threaded through the IQL Adam call, 2 ISV reads at each of the 2 train_value_step call sites in fused_training.rs. Important: consumer clamp envelopes were wider than the SP4 producer range (0.5..0.9999 vs producer's 0.85..0.95 for β1, etc.). At cold-start with ISV=0, this produced β1=0.5 (more aggressive momentum than the SP4-anchored 0.85 floor). Tightened all Pearl 4 consumer clamps to match the producer envelope: β1∈[0.85,0.95], β2∈[0.99,0.9995], ε∈[1e-10,1e-6]. Important: gpu_curiosity_trainer.rs:66-68 left dead ADAM_BETA1/BETA2/ EPS constants after the Pearl 4 migration. Removed. 8/8 Pearl 4 groups now ISV-driven (DqnTrunk, Value, Branches, IQN, IqlHigh, IqlLow, Attn, Curiosity). cargo check + cargo build + lib tests all clean. Co-Authored-By: Claude Sonnet 4.6 --- .../cuda_pipeline/gpu_curiosity_trainer.rs | 3 -- .../ml/src/cuda_pipeline/gpu_iql_trainer.rs | 12 +++-- crates/ml/src/trainers/dqn/fused_training.rs | 48 ++++++++++++------- .../src/trainers/dqn/trainer/training_loop.rs | 6 +-- docs/dqn-wire-up-audit.md | 19 ++++++++ 5 files changed, 62 insertions(+), 26 deletions(-) diff --git a/crates/ml/src/cuda_pipeline/gpu_curiosity_trainer.rs b/crates/ml/src/cuda_pipeline/gpu_curiosity_trainer.rs index e087b9c79..844ed031c 100644 --- a/crates/ml/src/cuda_pipeline/gpu_curiosity_trainer.rs +++ b/crates/ml/src/cuda_pipeline/gpu_curiosity_trainer.rs @@ -63,9 +63,6 @@ const CUR_B2_LEN: usize = CUR_OUTPUT; // [42] /// Adam optimizer hyperparameters. const ADAM_LR: f32 = 0.001; -const ADAM_BETA1: f32 = 0.9; -const ADAM_BETA2: f32 = 0.999; -const ADAM_EPS: f32 = 1e-8; // --------------------------------------------------------------------------- // Minimal cuBLAS context for the curiosity trainer diff --git a/crates/ml/src/cuda_pipeline/gpu_iql_trainer.rs b/crates/ml/src/cuda_pipeline/gpu_iql_trainer.rs index 712a8be8a..aed475b17 100644 --- a/crates/ml/src/cuda_pipeline/gpu_iql_trainer.rs +++ b/crates/ml/src/cuda_pipeline/gpu_iql_trainer.rs @@ -588,6 +588,12 @@ impl GpuIqlTrainer { /* SP4 Layer B (Mech 9): ISV-driven AdamW weight_decay rate. * Caller reads `ISV[WD_RATE[group]]` with `.max(0.0)` non-negativity. */ weight_decay_value: f32, + /* SP5 Pearl 4: per-group Adam β1/β2/ε read from ISV[ADAM_BETA1_BASE+g], + * ISV[ADAM_BETA2_BASE+g], ISV[ADAM_EPS_BASE+g] by FusedTrainingCtx + * and threaded through. g=4 for IqlHigh, g=5 for IqlLow. */ + iql_beta1: f32, + iql_beta2: f32, + iql_epsilon: f32, ) -> Result<(), MLError> { let b = self.config.batch_size; let h = self.config.value_hidden_dim; @@ -1036,9 +1042,9 @@ impl GpuIqlTrainer { unsafe { *self.t_pinned = self.adam_step; } let adam_blocks = (self.total_params + 255) / 256; let lr = self.config.lr; - let beta1 = self.config.beta1; - let beta2 = self.config.beta2; - let eps = self.config.epsilon; + let beta1 = iql_beta1; + let beta2 = iql_beta2; + let eps = iql_epsilon; // SP4 Layer B (Mech 9): AdamW weight_decay sourced from // ISV[WD_RATE[IqlHigh|IqlLow]] by the caller (`FusedTrainingCtx`) // and threaded through `train_value_step`. `.max(0.0)` upstream diff --git a/crates/ml/src/trainers/dqn/fused_training.rs b/crates/ml/src/trainers/dqn/fused_training.rs index e69c81ef5..aebff917a 100644 --- a/crates/ml/src/trainers/dqn/fused_training.rs +++ b/crates/ml/src/trainers/dqn/fused_training.rs @@ -1633,9 +1633,9 @@ impl FusedTrainingCtx { self.trainer.read_group_adam_bounds(crate::cuda_pipeline::ParamGroup::Attn); // SP5 Layer B (Pearl 4): TLOB shares ParamGroup::Attn=6. let tlob_g = crate::cuda_pipeline::ParamGroup::Attn.idx(); - let tlob_beta1 = self.trainer.read_isv_signal_at(ADAM_BETA1_BASE + tlob_g).max(0.5_f32).min(0.9999_f32); - let tlob_beta2 = self.trainer.read_isv_signal_at(ADAM_BETA2_BASE + tlob_g).max(0.9_f32).min(0.99999_f32); - let tlob_epsilon = self.trainer.read_isv_signal_at(ADAM_EPS_BASE + tlob_g).max(1e-12_f32); + let tlob_beta1 = self.trainer.read_isv_signal_at(ADAM_BETA1_BASE + tlob_g).clamp(0.85_f32, 0.95_f32); + let tlob_beta2 = self.trainer.read_isv_signal_at(ADAM_BETA2_BASE + tlob_g).clamp(0.99_f32, 0.9995_f32); + let tlob_epsilon = self.trainer.read_isv_signal_at(ADAM_EPS_BASE + tlob_g).clamp(1e-10_f32, 1e-6_f32); self.gpu_tlob .adam_step( self.trainer.config().lr, @@ -1942,11 +1942,25 @@ impl FusedTrainingCtx { self.trainer.read_group_adam_bounds(crate::cuda_pipeline::ParamGroup::IqlHigh); let (iql_low_clamp, iql_low_wd) = self.trainer.read_group_adam_bounds(crate::cuda_pipeline::ParamGroup::IqlLow); + // SP5 Pearl 4: per-group Adam β1/β2/ε for IqlHigh (group 4) and IqlLow (group 5). + // ISV[ADAM_BETA1_BASE+4=230], ISV[ADAM_BETA2_BASE+4=238], ISV[ADAM_EPS_BASE+4=246]. + // ISV[ADAM_BETA1_BASE+5=231], ISV[ADAM_BETA2_BASE+5=239], ISV[ADAM_EPS_BASE+5=247]. + // Clamps match SP4 producer envelope: β1∈[0.85,0.95], β2∈[0.99,0.9995], ε∈[1e-10,1e-6]. + let iql_high_g = crate::cuda_pipeline::ParamGroup::IqlHigh.idx(); + let iql_high_beta1 = self.trainer.read_isv_signal_at(ADAM_BETA1_BASE + iql_high_g).clamp(0.85_f32, 0.95_f32); + let iql_high_beta2 = self.trainer.read_isv_signal_at(ADAM_BETA2_BASE + iql_high_g).clamp(0.99_f32, 0.9995_f32); + let iql_high_epsilon = self.trainer.read_isv_signal_at(ADAM_EPS_BASE + iql_high_g).clamp(1e-10_f32, 1e-6_f32); + let iql_low_g = crate::cuda_pipeline::ParamGroup::IqlLow.idx(); + let iql_low_beta1 = self.trainer.read_isv_signal_at(ADAM_BETA1_BASE + iql_low_g).clamp(0.85_f32, 0.95_f32); + let iql_low_beta2 = self.trainer.read_isv_signal_at(ADAM_BETA2_BASE + iql_low_g).clamp(0.99_f32, 0.9995_f32); + let iql_low_epsilon = self.trainer.read_isv_signal_at(ADAM_EPS_BASE + iql_low_g).clamp(1e-10_f32, 1e-6_f32); let states_f32 = self.trainer.states_buf(); - self.gpu_iql.train_value_step(states_f32, iql_high_clamp, iql_high_wd) + self.gpu_iql.train_value_step(states_f32, iql_high_clamp, iql_high_wd, + iql_high_beta1, iql_high_beta2, iql_high_epsilon) .map_err(|e| anyhow::anyhow!("IQL high train: {e}"))?; let states_f32 = self.trainer.states_buf(); - self.gpu_iql_low.train_value_step(states_f32, iql_low_clamp, iql_low_wd) + self.gpu_iql_low.train_value_step(states_f32, iql_low_clamp, iql_low_wd, + iql_low_beta1, iql_low_beta2, iql_low_epsilon) .map_err(|e| anyhow::anyhow!("IQL low train: {e}"))?; // 4. Compute advantage weights (from high-tau V(s)). @@ -2059,9 +2073,9 @@ impl FusedTrainingCtx { self.trainer.read_group_adam_bounds(crate::cuda_pipeline::ParamGroup::Iqn); // SP5 Layer B (Pearl 4): per-group Adam β1/β2/ε for Iqn=3. let iqn_g = crate::cuda_pipeline::ParamGroup::Iqn.idx(); - let iqn_beta1 = self.trainer.read_isv_signal_at(ADAM_BETA1_BASE + iqn_g).max(0.5_f32).min(0.9999_f32); - let iqn_beta2 = self.trainer.read_isv_signal_at(ADAM_BETA2_BASE + iqn_g).max(0.9_f32).min(0.99999_f32); - let iqn_epsilon = self.trainer.read_isv_signal_at(ADAM_EPS_BASE + iqn_g).max(1e-12_f32); + let iqn_beta1 = self.trainer.read_isv_signal_at(ADAM_BETA1_BASE + iqn_g).clamp(0.85_f32, 0.95_f32); + let iqn_beta2 = self.trainer.read_isv_signal_at(ADAM_BETA2_BASE + iqn_g).clamp(0.99_f32, 0.9995_f32); + let iqn_epsilon = self.trainer.read_isv_signal_at(ADAM_EPS_BASE + iqn_g).clamp(1e-10_f32, 1e-6_f32); match iqn.execute_training_pipeline( online_h_s2, next_states_buf, &self.target_dueling, @@ -2149,9 +2163,9 @@ impl FusedTrainingCtx { self.trainer.read_group_adam_bounds(crate::cuda_pipeline::ParamGroup::Attn); // SP5 Layer B (Pearl 4): per-group Adam β1/β2/ε for Attn=6. let attn_g = crate::cuda_pipeline::ParamGroup::Attn.idx(); - let attn_beta1 = self.trainer.read_isv_signal_at(ADAM_BETA1_BASE + attn_g).max(0.5_f32).min(0.9999_f32); - let attn_beta2 = self.trainer.read_isv_signal_at(ADAM_BETA2_BASE + attn_g).max(0.9_f32).min(0.99999_f32); - let attn_epsilon = self.trainer.read_isv_signal_at(ADAM_EPS_BASE + attn_g).max(1e-12_f32); + let attn_beta1 = self.trainer.read_isv_signal_at(ADAM_BETA1_BASE + attn_g).clamp(0.85_f32, 0.95_f32); + let attn_beta2 = self.trainer.read_isv_signal_at(ADAM_BETA2_BASE + attn_g).clamp(0.99_f32, 0.9995_f32); + let attn_epsilon = self.trainer.read_isv_signal_at(ADAM_EPS_BASE + attn_g).clamp(1e-10_f32, 1e-6_f32); attn.adam_step(lr, mgn, Some(&attn_stream_ref), ws_override, attn_weight_clamp_max_abs, attn_weight_decay_value, attn_beta1, attn_beta2, attn_epsilon) @@ -2193,9 +2207,9 @@ impl FusedTrainingCtx { self.trainer.read_group_adam_bounds(crate::cuda_pipeline::ParamGroup::Attn); // SP5 Layer B (Pearl 4): per-group Adam β1/β2/ε for Attn=6. let attn_g = crate::cuda_pipeline::ParamGroup::Attn.idx(); - let attn_beta1 = self.trainer.read_isv_signal_at(ADAM_BETA1_BASE + attn_g).max(0.5_f32).min(0.9999_f32); - let attn_beta2 = self.trainer.read_isv_signal_at(ADAM_BETA2_BASE + attn_g).max(0.9_f32).min(0.99999_f32); - let attn_epsilon = self.trainer.read_isv_signal_at(ADAM_EPS_BASE + attn_g).max(1e-12_f32); + let attn_beta1 = self.trainer.read_isv_signal_at(ADAM_BETA1_BASE + attn_g).clamp(0.85_f32, 0.95_f32); + let attn_beta2 = self.trainer.read_isv_signal_at(ADAM_BETA2_BASE + attn_g).clamp(0.99_f32, 0.9995_f32); + let attn_epsilon = self.trainer.read_isv_signal_at(ADAM_EPS_BASE + attn_g).clamp(1e-10_f32, 1e-6_f32); attn.adam_step(lr, mgn, None, None, attn_weight_clamp_max_abs, attn_weight_decay_value, attn_beta1, attn_beta2, attn_epsilon) @@ -2213,9 +2227,9 @@ impl FusedTrainingCtx { self.trainer.read_group_adam_bounds(crate::cuda_pipeline::ParamGroup::Iqn); // SP5 Layer B (Pearl 4): per-group Adam β1/β2/ε for Iqn=3. let iqn_g = crate::cuda_pipeline::ParamGroup::Iqn.idx(); - let iqn_beta1 = self.trainer.read_isv_signal_at(ADAM_BETA1_BASE + iqn_g).max(0.5_f32).min(0.9999_f32); - let iqn_beta2 = self.trainer.read_isv_signal_at(ADAM_BETA2_BASE + iqn_g).max(0.9_f32).min(0.99999_f32); - let iqn_epsilon = self.trainer.read_isv_signal_at(ADAM_EPS_BASE + iqn_g).max(1e-12_f32); + let iqn_beta1 = self.trainer.read_isv_signal_at(ADAM_BETA1_BASE + iqn_g).clamp(0.85_f32, 0.95_f32); + let iqn_beta2 = self.trainer.read_isv_signal_at(ADAM_BETA2_BASE + iqn_g).clamp(0.99_f32, 0.9995_f32); + let iqn_epsilon = self.trainer.read_isv_signal_at(ADAM_EPS_BASE + iqn_g).clamp(1e-10_f32, 1e-6_f32); match iqn.execute_training_pipeline( online_h_s2, next_states_buf, &self.target_dueling, diff --git a/crates/ml/src/trainers/dqn/trainer/training_loop.rs b/crates/ml/src/trainers/dqn/trainer/training_loop.rs index c087b9993..f19ed77ce 100644 --- a/crates/ml/src/trainers/dqn/trainer/training_loop.rs +++ b/crates/ml/src/trainers/dqn/trainer/training_loop.rs @@ -1816,9 +1816,9 @@ impl DQNTrainer { let cur_g = crate::cuda_pipeline::ParamGroup::Curiosity.idx(); let (cur_beta1, cur_beta2, cur_epsilon) = if let Some(ref fused) = self.fused_ctx { ( - fused.read_isv_signal_at(SP5_ADAM_BETA1_BASE + cur_g).max(0.5_f32).min(0.9999_f32), - fused.read_isv_signal_at(SP5_ADAM_BETA2_BASE + cur_g).max(0.9_f32).min(0.99999_f32), - fused.read_isv_signal_at(SP5_ADAM_EPS_BASE + cur_g).max(1e-12_f32), + fused.read_isv_signal_at(SP5_ADAM_BETA1_BASE + cur_g).clamp(0.85_f32, 0.95_f32), + fused.read_isv_signal_at(SP5_ADAM_BETA2_BASE + cur_g).clamp(0.99_f32, 0.9995_f32), + fused.read_isv_signal_at(SP5_ADAM_EPS_BASE + cur_g).clamp(1e-10_f32, 1e-6_f32), ) } else { (0.9_f32, 0.999_f32, 1e-8_f32) diff --git a/docs/dqn-wire-up-audit.md b/docs/dqn-wire-up-audit.md index 5bb912b1f..8e15edbaa 100644 --- a/docs/dqn-wire-up-audit.md +++ b/docs/dqn-wire-up-audit.md @@ -3229,3 +3229,22 @@ All 11 consumer sites migrated in one atomic commit per `feedback_no_partial_ref - `cargo test -p ml --lib` → 930 pass, 14 pre-existing failures unchanged Refs: `feedback_no_partial_refactor`, `feedback_isv_for_adaptive_bounds`, `feedback_adaptive_not_tuned` + +### SP5 Layer B fix-up (2026-05-01) + +Three review findings on Layer B (commit `99367b9c6`) resolved in one atomic commit before Layer C validation. + +**Critical — IQL groups 4+5 complete Pearl 4 migration:** +`gpu_iql_trainer.rs::train_value_step` still read `self.config.beta1=0.9`, `self.config.beta2=0.999`, `self.config.epsilon` at runtime (lines 1039-1041). ISV[230] (adam_beta1(4)), ISV[231] (adam_beta1(5)), ISV[238], ISV[239], ISV[246], ISV[247] were populated by the Layer A producer but unconsumed — partial Pearl 4 contract with 6 of 8 groups migrated. Fix: `train_value_step` gains 3 new parameters (`iql_beta1`, `iql_beta2`, `iql_epsilon`); `fused_training.rs` reads ISV at both call sites using `ADAM_BETA1_BASE + iql_high_g/iql_low_g` and threads values through. Pearl 4 is now complete for all 8 param groups (DqnTrunk, DqnValue, DqnBranches, Iqn, IqlHigh, IqlLow, Attn, Curiosity). + +**Important — consumer clamp envelopes tightened to SP4 producer range:** +All Pearl 4 consumer clamps across `fused_training.rs` (IQN, ATTN, TLOB sites, new IQL sites) and `training_loop.rs` (Curiosity site) tightened from `max(0.5).min(0.9999)` / `max(0.9).min(0.99999)` / `max(1e-12)` to match the SP4 producer envelope: β1∈[0.85, 0.95], β2∈[0.99, 0.9995], ε∈[1e-10, 1e-6]. Cold-start floor is now 0.85 (not 0.5) for β1, eliminating the overly-aggressive momentum at ISV=0. + +**Important — dead constants removed:** +`gpu_curiosity_trainer.rs` lines 66-68: `const ADAM_BETA1: f32 = 0.9`, `const ADAM_BETA2: f32 = 0.999`, `const ADAM_EPS: f32 = 1e-8` removed. These were made dead by the Layer B Pearl 4 migration; the runtime path already read from threaded parameters. + +**Verification:** +- `cargo check -p ml --offline` → clean (12 warnings, all pre-existing) +- `cargo test -p ml --lib -- sp5 sp4 state_reset_registry` → 13 pass, 0 fail + +Refs: `feedback_no_partial_refactor`, `feedback_isv_for_adaptive_bounds`