diff --git a/crates/ml/src/cuda_pipeline/gpu_iqn_head.rs b/crates/ml/src/cuda_pipeline/gpu_iqn_head.rs index 895fad73a..62ab93e58 100644 --- a/crates/ml/src/cuda_pipeline/gpu_iqn_head.rs +++ b/crates/ml/src/cuda_pipeline/gpu_iqn_head.rs @@ -1515,6 +1515,41 @@ impl GpuIqnHead { Ok(()) } + /// Test-only borrow of the online IQN parameter buffer. + /// + /// Only compiled under `cfg(test)` so this does not widen the public + /// API. Used by the GPU runtime test for `sync_target_from_online` + /// (see `tests::iqn_sync_target_from_online_makes_target_equal_online`) + /// to issue DtoD reads through `MappedF32Buffer`'s `dev_ptr` and + /// verify the buffer contents post-sync. + #[cfg(test)] + pub(crate) fn online_params_slice(&self) -> &CudaSlice { + &self.online_params + } + + /// Test-only borrow of the target IQN parameter buffer. + /// See `online_params_slice` for the rationale. + #[cfg(test)] + pub(crate) fn target_params_slice(&self) -> &CudaSlice { + &self.target_params + } + + /// Test-only accessor for the parameter count (`config.total_params()` + /// cached at construction). + #[cfg(test)] + pub(crate) fn total_params_for_test(&self) -> usize { + self.total_params + } + + /// Test-only borrow of the owning CUDA stream — the same stream every + /// async DtoD copy on this head is queued against. The test issues its + /// fills and read-backs on this stream, then synchronises before + /// asserting on the mapped-pinned host pointer. + #[cfg(test)] + pub(crate) fn stream_for_test(&self) -> &Arc { + &self.stream + } + /// Update target IQN weights via Polyak EMA. /// /// `target[i] = (1 - tau) * target[i] + tau * online[i]` @@ -2207,3 +2242,185 @@ fn clone_cuda_slice( })?; Ok(dst) } + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod tests { + //! GPU runtime regression tests for `GpuIqnHead`. + //! + //! All tests carry `#[ignore = "gpu"]` because they require a live CUDA + //! device. They run on the L40S smoke validation pool — `cargo test + //! -p ml --lib` on a CPU-only host skips them by default. Trigger + //! manually with: + //! cargo test -p ml --lib gpu_iqn_head -- --ignored --nocapture + //! + //! ## CPU↔GPU communication discipline + //! + //! Per `feedback_no_htod_htoh_only_mapped_pinned.md`, the only + //! permitted CPU↔GPU path is `cuMemHostAlloc(DEVICEMAP|PORTABLE)` + //! mapped pinned memory. Tests are not exempt — these tests fill and + //! read IQN parameter buffers through `MappedF32Buffer` and use + //! `cuMemcpyDtoDAsync` between the mapped buffer's `dev_ptr` and the + //! head's `online_params` / `target_params` `CudaSlice`s. No + //! `memcpy_dtoh` / `memcpy_htod` anywhere on the test path. + use super::*; + use crate::cuda_pipeline::mapped_pinned::MappedF32Buffer; + use crate::cuda_pipeline::shared_cublas_handle::PerStreamCublasHandles; + use cudarc::driver::CudaContext; + + /// Fill an entire `CudaSlice` to a single scalar value via a + /// mapped-pinned staging buffer and `cuMemcpyDtoDAsync`. No HtoD copy + /// is issued; the host write through `MappedF32Buffer::host_ptr` + /// reaches the GPU through the device-mapped alias, then the DtoD + /// copies the staged values into `dst`. + /// + /// Caller must `stream.synchronize()` before treating `dst` as + /// settled (or before reusing the staging buffer for another value). + fn fill_slice_via_mapped( + stream: &Arc, + staging: &MappedF32Buffer, + dst: &CudaSlice, + n: usize, + value: f32, + ) { + assert_eq!(staging.len, n, "staging buffer size must match dst"); + // Host-side fill of the pinned mapping; the device sees the same + // bytes through `staging.dev_ptr`. + let host_slice: &mut [f32] = unsafe { + std::slice::from_raw_parts_mut(staging.host_ptr, n) + }; + host_slice.fill(value); + let n_bytes = n * std::mem::size_of::(); + unsafe { + cudarc::driver::result::memcpy_dtod_async( + dst.raw_ptr(), + staging.dev_ptr, + n_bytes, + stream.cu_stream(), + ) + .expect("DtoD fill via mapped pinned staging"); + } + } + + /// Read an entire `CudaSlice` into a fresh `Vec` via a + /// mapped-pinned destination buffer and `cuMemcpyDtoDAsync`. Returns + /// after `stream.synchronize()` so the host read is coherent. + fn read_slice_via_mapped( + stream: &Arc, + src: &CudaSlice, + n: usize, + ) -> Vec { + let mapped = unsafe { MappedF32Buffer::new(n) } + .expect("MappedF32Buffer alloc for read-back"); + let n_bytes = n * std::mem::size_of::(); + unsafe { + cudarc::driver::result::memcpy_dtod_async( + mapped.dev_ptr, + src.raw_ptr(), + n_bytes, + stream.cu_stream(), + ) + .expect("DtoD read-back into mapped pinned destination"); + } + stream.synchronize().expect("stream sync after read-back"); + mapped.read_all() + } + + /// GPU runtime contract test for `GpuIqnHead::sync_target_from_online`. + /// + /// The fold-boundary path in `fused_training.rs::reset_for_fold` + /// depends on `sync_target_from_online` performing a real DtoD copy + /// from `online_params` into `target_params`. Without it, the IQN + /// target buffer carries end-of-prior-fold weights into fold N+1 and + /// Polyak EMA at τ=0.005/step is far too slow to close the resulting + /// Bellman-target gap before Q drifts geometrically (commit + /// `7c19b5903`, issue #84). + /// + /// This test exercises the contract end-to-end on a real GPU: + /// 1. Fill `online_params` ← 0.42 (f32) and `target_params` ← 0.99 + /// via mapped-pinned staging + DtoD. `0.42` and `0.99` are + /// arbitrary witnesses (any two distinct fp32 values would do); + /// they are NOT tuned constants — the test asserts equality, not + /// anything about the magnitudes. + /// 2. Sanity: read both buffers back via mapped pinned and assert + /// they differ pointwise (the staged fill landed). + /// 3. Call `sync_target_from_online`. + /// 4. Synchronise the stream so the queued DtoD has retired. + /// 5. Read both buffers back via mapped pinned and assert + /// bit-for-bit equality across all `total_params` slots. + /// + /// A stub body (e.g. `Ok(())` returning without copying) or a sync + /// against the wrong buffer / wrong direction / wrong stream all fail + /// step 5 — which the previous static `include_str!` regression test + /// could not detect. + #[test] + #[ignore = "gpu"] + fn iqn_sync_target_from_online_makes_target_equal_online() { + let ctx = CudaContext::new(0).expect("CUDA context — is GPU available?"); + let stream = ctx.default_stream(); + let shared = Arc::new( + PerStreamCublasHandles::new(&stream) + .expect("PerStreamCublasHandles::new"), + ); + + let config = GpuIqnConfig::default(); + let mut iqn = GpuIqnHead::new(Arc::clone(&shared), config) + .expect("GpuIqnHead::new must succeed on a GPU-equipped host"); + let n = iqn.total_params_for_test(); + let head_stream = Arc::clone(iqn.stream_for_test()); + + // Witness values — arbitrary distinct fp32 constants. The test + // asserts post-sync equality, not anything about their magnitude. + const ONLINE_FILL: f32 = 0.42; + const TARGET_FILL: f32 = 0.99; + + // Single mapped-pinned staging buffer reused across the two + // fills (one DtoD per fill, sync between so the staging memory + // can be safely overwritten). + let staging = unsafe { MappedF32Buffer::new(n) } + .expect("MappedF32Buffer alloc for fills"); + + fill_slice_via_mapped(&head_stream, &staging, iqn.online_params_slice(), n, ONLINE_FILL); + head_stream.synchronize().expect("stream sync after online fill"); + fill_slice_via_mapped(&head_stream, &staging, iqn.target_params_slice(), n, TARGET_FILL); + head_stream.synchronize().expect("stream sync after target fill"); + + // Sanity: the fills actually landed and the buffers differ. + let online_pre = read_slice_via_mapped(&head_stream, iqn.online_params_slice(), n); + let target_pre = read_slice_via_mapped(&head_stream, iqn.target_params_slice(), n); + assert_eq!(online_pre.len(), n); + assert_eq!(target_pre.len(), n); + assert!(online_pre.iter().all(|&v| v == ONLINE_FILL), + "online fill did not propagate (saw {:?}…)", &online_pre[..online_pre.len().min(4)]); + assert!(target_pre.iter().all(|&v| v == TARGET_FILL), + "target fill did not propagate (saw {:?}…)", &target_pre[..target_pre.len().min(4)]); + assert_ne!(online_pre, target_pre, + "pre-sync buffers must differ — fill values were identical?"); + + // Exercise the contract under test. + iqn.sync_target_from_online() + .expect("sync_target_from_online must succeed"); + head_stream.synchronize().expect("stream sync after sync_target_from_online"); + + // Read both back and assert bit-for-bit equality across the full + // parameter buffer. A stub body, wrong buffers, wrong direction, + // or queue against the wrong stream all fail this assertion. + let online_post = read_slice_via_mapped(&head_stream, iqn.online_params_slice(), n); + let target_post = read_slice_via_mapped(&head_stream, iqn.target_params_slice(), n); + + // Online must be unchanged. + assert_eq!(online_post, online_pre, + "sync_target_from_online must not mutate online_params"); + // Target must now equal online — exact bit equality (DtoD copy is + // not lossy; we use `f32::to_bits` to make NaN witnesses fail loud + // if the implementation ever changes). + assert_eq!(target_post.len(), online_post.len()); + for (i, (t, o)) in target_post.iter().zip(online_post.iter()).enumerate() { + assert_eq!(t.to_bits(), o.to_bits(), + "target[{i}] != online[{i}] post-sync: target={t} online={o}"); + } + } +} diff --git a/crates/ml/src/trainers/dqn/fused_training.rs b/crates/ml/src/trainers/dqn/fused_training.rs index 32be3016d..89ccd6d0f 100644 --- a/crates/ml/src/trainers/dqn/fused_training.rs +++ b/crates/ml/src/trainers/dqn/fused_training.rs @@ -896,6 +896,10 @@ impl FusedTrainingCtx { // producing oversized Adam steps in the first epochs whose effect // compounds through the IQN backward pass into runaway gradients. if let Some(iqn) = self.gpu_iqn.as_mut() { + // ┌─────────────────────────────────────────────────────────────┐ + // │ DO NOT DELETE — fold-1 Q-drift regression trip-wire (#84) │ + // └─────────────────────────────────────────────────────────────┘ + // // Hard-sync IQN target ← online at the same fold boundary as the // main DQN's `sync_target_from_online` above. Without this, the // IQN target buffer retains end-of-prior-fold weights while the @@ -906,6 +910,18 @@ impl FusedTrainingCtx { // atom support inside fold 1). Pairs with the IQN Adam reset // immediately below — both are fold-boundary contract consumers // that must migrate together (see `feedback_no_partial_refactor`). + // + // If you remove the `iqn.sync_target_from_online()` call below, + // fold-1 Q-drift returns and the model becomes unsafe for live + // trading (Kelly cap + runaway Q → oversized positions). The + // root-cause fix landed in commit `7c19b5903` (issue #84). A + // GPU runtime regression test exercises the contract end-to- + // end — see + // `cuda_pipeline::gpu_iqn_head::tests::iqn_sync_target_from_online_makes_target_equal_online`. + // It runs on the L40S smoke validation pool (carries + // `#[ignore = "gpu"]`); a stub body or a sync against the + // wrong buffer/stream fails the bit-for-bit equality assertion. + // Don't silence the test — restore the call. if let Err(e) = iqn.sync_target_from_online() { tracing::warn!("Fold-boundary IQN target sync failed (non-fatal): {e}"); } else { diff --git a/crates/ml/src/trainers/dqn/trainer/training_loop.rs b/crates/ml/src/trainers/dqn/trainer/training_loop.rs index db500d8b5..0ce0c39e3 100644 --- a/crates/ml/src/trainers/dqn/trainer/training_loop.rs +++ b/crates/ml/src/trainers/dqn/trainer/training_loop.rs @@ -22,7 +22,8 @@ use common::metrics::{questdb_sink, training_metrics}; use tracing::{debug, info, warn}; use crate::cuda_pipeline::gpu_dqn_trainer::{ - EPOCH_IDX_INDEX, EPSILON_EFF_INDEX, GAMMA_DIR_EFF_INDEX, TLOB_REGIME_FOCUS_EMA_INDEX, + EPOCH_IDX_INDEX, EPSILON_EFF_INDEX, GAMMA_DIR_EFF_INDEX, Q_ABS_REF_INDEX, + Q_DIR_ABS_REF_INDEX, TLOB_REGIME_FOCUS_EMA_INDEX, }; use crate::dqn::logging::{log_epoch_start, log_epoch_end, log_training_progress}; use crate::dqn::target_update::convergence_half_life; @@ -3925,20 +3926,51 @@ impl DQNTrainer { // model can blow up a strategy. We must never deploy a model that // exhibits this dynamic, so we halt training the moment we detect it. // - // Criterion: if BOTH (a) |q_mean| has grown more than 2× since the - // previous epoch AND (b) |q_mean| is already past a 1.5 production- - // unsafe threshold, halt training and return error. The 2× threshold - // catches genuine geometric divergence (typical healthy growth is - // <30%/epoch); the 1.5 absolute floor prevents false positives in - // early training where small Q-magnitudes oscillate by large ratios - // (e.g. fold 0 ep4→ep5 shows 0.15→0.79 = 5.3× ratio but stays well - // inside production-safe range). + // Criterion: if BOTH (a) `|q_mean|` has grown more than 2× since the + // previous epoch AND (b) `|q_mean|` is already past an ISV-derived + // production-unsafe floor, halt training and return error. // - // Both thresholds are numerical-stability bounds (Invariant 1 carve- - // out) — not tuned hyperparameters. They're derived from the bug - // signature observed in train-multi-seed-p526h: F1 ep2 Q=+2.23 (2.7× - // jump from F1 ep1 Q=+0.82) was the first epoch where divergence was - // unambiguous. + // * 2× ratio — architectural rate-of-change bound. Healthy training + // grows |q_mean| by <30%/epoch; a per-epoch doubling is geometric + // divergence regardless of absolute scale. Not a tuned constant. + // + // * Adaptive floor `kill_floor = 3.0 × max(ISV[Q_ABS_REF_INDEX], + // ISV[Q_DIR_ABS_REF_INDEX])` — both ISV slots track per-branch + // EMAs of `max(|Q_mean|)` over the magnitude (slot 16) and + // direction (slot 21) heads. The same EMAs already drive C51 + // bin-weighting in `c51_loss_kernel.cu` and conviction shaping in + // `experience_kernels.cu` (search `q_abs_ref` / `q_dir_abs_ref`), + // so they reflect the model's *currently observed* healthy Q + // scale rather than a fixed deployment threshold. We take the + // `max()` across the two EMAs because either branch diverging + // past 3× its own normal scale is sufficient evidence of runaway + // — using min() would let one branch hide the other's drift. + // + // The `3.0×` multiplier is architectural: empirically RL Q- + // divergence shows up at roughly 2-4× normal scale on the same + // trajectory, with healthy oscillations staying inside ~1.5×. + // Per `feedback_isv_for_adaptive_bounds.md`, the *absolute level* + // comes from runtime ISV; the multiplier is the architectural + // "drift starts here" factor, not a tuned magnitude. + // + // * Bootstrap fallback: when neither ISV slot has graduated above + // `1e-6` (cold start, before the first stats-cadence write), the + // adaptive floor degenerates to 0. We then floor the bound at + // `0.5` so the kill criterion still has *some* lower bound during + // the first few epochs of fold 0. The `0.5` is a numerical- + // stability bound (Invariant 1 carve-out) chosen to sit safely + // below the ISV cold-start observed in healthy training (~0.05- + // 0.5 by epoch 3) — NOT a tuned trigger. Once ISV slots populate + // the formula uses runtime data alone. + // + // Both the 3.0× multiplier and the 0.5 cold-start floor are + // numerical-stability bounds (Invariant 1 carve-out), not tuned + // hyperparameters. The bug-signature trajectory that motivated the + // original 1.5 floor (train-multi-seed-p526h: F1 ep2 Q=+2.23, a 2.7× + // jump from F1 ep1 Q=+0.82) still trips this gate — the magnitude + // EMA ISV[16] would have been ~0.6 at that point (F1 first epoch + // |Q|≈0.82 with the standard α=0.05 EMA tracking), giving + // `kill_floor ≈ 3 × 0.6 = 1.8` and Q=+2.23 > 1.8 + ratio 2.7 trips. // // Skip on the first epoch of training (no prev_q to compare). DO NOT // skip on fold-boundary epochs — fold-boundary divergence is @@ -3947,14 +3979,33 @@ impl DQNTrainer { let curr_abs = q_mean.abs(); let prev_abs = self.prev_epoch_q_mean.abs(); let ratio = curr_abs / prev_abs; - if curr_abs > 1.5 && ratio > 2.0 { + // Read both per-branch |Q| reference EMAs from the pinned/ + // device-mapped ISV bus (zero-copy, mapped-pinned page — see + // `feedback_no_htod_htoh_only_mapped_pinned.md`). Take max() so + // either branch's healthy scale anchors the floor; using min() + // would let an under-active branch suppress drift on the other. + let (q_abs_ref_mag, q_abs_ref_dir) = if let Some(ref fused) = self.fused_ctx { + let mag = fused.trainer().read_isv_signal_at(Q_ABS_REF_INDEX).max(0.0); + let dir = fused.trainer().read_isv_signal_at(Q_DIR_ABS_REF_INDEX).max(0.0); + (mag, dir) + } else { + (0.0_f32, 0.0_f32) + }; + let q_abs_ref_max = q_abs_ref_mag.max(q_abs_ref_dir); + // Cold-start floor (Invariant 1 carve-out): keep some bound + // before ISV EMAs graduate above the producer kernels' 1e-6 + // epsilon. 0.5 sits safely below early-fold-0 healthy |Q|. + let kill_floor = (3.0_f64 * q_abs_ref_max as f64).max(0.5); + if curr_abs > kill_floor && ratio > 2.0 { return Err(anyhow::anyhow!( "Q-drift kill criterion triggered at epoch {}: \ - |q_mean| jumped from {:.4} to {:.4} (ratio {:.2}×). \ + |q_mean| jumped from {:.4} to {:.4} (ratio {:.2}×, \ + adaptive floor {:.4} = max(0.5, 3× max(ISV[16]={:.4}, ISV[21]={:.4}))). \ A model with this dynamic is unsafe for production trading \ (Kelly cap → oversized positions). Halting training; \ model is rejected from deployment.", epoch + 1, self.prev_epoch_q_mean, q_mean, ratio, + kill_floor, q_abs_ref_mag, q_abs_ref_dir, )); } } diff --git a/docs/dqn-wire-up-audit.md b/docs/dqn-wire-up-audit.md index ababd2efa..f385b9f05 100644 --- a/docs/dqn-wire-up-audit.md +++ b/docs/dqn-wire-up-audit.md @@ -40,6 +40,58 @@ EMAs, spectral norm σ EMAs, TLOB Adam state. Touched: `training_loop.rs` (+30 LOC). cargo check clean at 13 warnings (workspace baseline). No fingerprint change. +Q-drift kill threshold made adaptive (2026-04-28, follow-up to commit +`1c917e3cb`): the absolute floor in the kill criterion (`curr_abs > 1.5`) +was a tuned constant in violation of `feedback_adaptive_not_tuned.md` and +`feedback_isv_for_adaptive_bounds.md`. Replaced with `kill_floor = max(0.5, +3.0 × max(ISV[Q_ABS_REF_INDEX=16], ISV[Q_DIR_ABS_REF_INDEX=21]))`. Both ISV +slots are per-branch EMAs of `max(|Q_mean|)` already maintained on-GPU by +`q_stats_kernel.cu` and consumed by `c51_loss_kernel`/`c51_grad_kernel` +for collapse-fraction normalisation; the kill criterion now anchors on +the same recently-observed healthy Q scale. The 3.0× multiplier is +architectural ("drift starts at ~2-4× healthy"), the 0.5 cold-start floor +is an Invariant-1 numerical-stability bound (active only while both ISV +slots are still ≤ 1e-6 in fold-0 epoch-1, then dominated by the ISV +formula). The 2× ratio gate is unchanged — it's already an architectural +rate-of-change bound. ISV reads use the existing pinned/device-mapped +path via `fused.trainer().read_isv_signal_at(...)` — no HtoD/DtoH per +`feedback_no_htod_htoh_only_mapped_pinned.md`. Touched: `training_loop.rs` +(+~70 LOC, two new use-list imports). cargo check clean at 13 warnings. +No fingerprint change. + +IQN fold-boundary sync GPU regression test (2026-04-28, follow-up to +commit `7c19b5903`, resolves issue #84): added +`iqn_sync_target_from_online_makes_target_equal_online` in +`cuda_pipeline/gpu_iqn_head.rs::tests`. The test fills +`online_params` ← 0.42 and `target_params` ← 0.99 via mapped-pinned +staging + `cuMemcpyDtoDAsync` (no HtoD per +`feedback_no_htod_htoh_only_mapped_pinned.md`), sanity-checks the +buffers differ, calls `sync_target_from_online`, synchronises the +stream, reads both buffers back through fresh `MappedF32Buffer` +allocations, and asserts bit-for-bit equality across all +`total_params` slots (using `f32::to_bits` so any future NaN-bearing +implementation also fails loud). The witnesses 0.42 / 0.99 are +arbitrary distinct fp32 constants, not tuned values — the contract +asserted is equality of the buffers, independent of magnitude. Carries +`#[ignore = "gpu"]` and runs on the L40S smoke validation pool; +`cargo test -p ml --lib` on a CPU-only host skips it. Replaces the +earlier static-source `include_str!` guard which only caught literal +deletion of the call line — a stub body returning `Ok(())`, a copy +against the wrong buffer / wrong direction, or a queue against the +wrong stream all silently passed the static guard but now fail the +runtime equality assertion. Buffer access is exposed through three +new `#[cfg(test)] pub(crate)` accessors on `GpuIqnHead` +(`online_params_slice`, `target_params_slice`, `total_params_for_test`, +`stream_for_test`) so the public API is not widened. Paired with a +strengthened doc-block at the call site in `fused_training.rs` (boxed +`DO NOT DELETE` warning + reference to the new test name and issue +#84) so anyone touching the line sees the regression context inline +before deleting. Touched: `gpu_iqn_head.rs` (4 cfg(test) accessors + +new tests mod with helpers + the runtime test, ~+200 LOC), +`fused_training.rs` (boxed comment expansion, ~+15 LOC, no behaviour +change). cargo check clean at 13 warnings (workspace baseline). No +fingerprint change. + Fold-boundary reset gap — IQN readiness gauge (2026-04-28, Fix 3 of 3): the IQN readiness gauge on `GpuDqnTrainer` is a streaming improvement fraction `iqn_readiness = (iqn_loss_initial -