From b4e5fc98919df438c045e727155afa04cd5ebe52 Mon Sep 17 00:00:00 2001 From: jgrusewski Date: Sat, 2 May 2026 00:06:40 +0200 Subject: [PATCH] =?UTF-8?q?fix(sp5):=20Task=20A7=20=E2=80=94=20close=20two?= =?UTF-8?q?=20minor=20review=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Combined spec-quality review caught two minor issues in the Pearl 8 commit. Both are mechanical fixes; no behavior change. 1. training_loop.rs:3640 used `let _ = std::mem::ManuallyDrop::new(guard);` to keep the cudarc StreamGuard alive past the inner let-binding block — the pattern leaks the guard's borrow bookkeeping for the lifetime of the function rather than dropping it after the kernel launch. Replaced with the idiomatic let (feat_dev_ptr, _guard) = features_buf.device_ptr(stream_ref); at the same scope as the launch, matching the existing convention for SP4 producer wire-ups elsewhere in this file. The _guard binding lets cudarc's borrow drop naturally at end-of-block after the kernel launch returns. 2. The audit doc's description of test 14 incorrectly claimed atr_norm was chosen so atr_abs=10.0 and Short/Long=20.0. The actual test uses atr_norm=0.5 → log_atr=1.0 → atr_abs=e^1≈2.7183 → Short/Long≈5.4366. Updated the audit doc to match the actual test values. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/trainers/dqn/trainer/training_loop.rs | 18 ++++++++++-------- docs/dqn-wire-up-audit.md | 2 ++ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/crates/ml/src/trainers/dqn/trainer/training_loop.rs b/crates/ml/src/trainers/dqn/trainer/training_loop.rs index 7ff088890..700d50c9a 100644 --- a/crates/ml/src/trainers/dqn/trainer/training_loop.rs +++ b/crates/ml/src/trainers/dqn/trainer/training_loop.rs @@ -3632,14 +3632,16 @@ impl DQNTrainer { let market_dim_usize = fused.trainer().config().market_dim.max(1); let inferred_num_bars = features_buf.len() / market_dim_usize; if inferred_num_bars > 0 { - let feat_dev_ptr = { - use cudarc::driver::DevicePtr; - let stream_ref = self.cuda_stream.as_deref() - .expect("cuda_stream must be Some when fused_ctx is Some"); - let (ptr, guard) = features_buf.device_ptr(stream_ref); - let _ = std::mem::ManuallyDrop::new(guard); - ptr - }; + // Hold the StreamGuard for the duration of the kernel + // launch — pattern matches the device_ptr_mut idiom + // used elsewhere in this file (e.g. the SP4 producer + // wire-ups). Binding `_guard` keeps cudarc's borrow + // bookkeeping alive across the launch and lets it drop + // naturally at end of block. + use cudarc::driver::DevicePtr; + let stream_ref = self.cuda_stream.as_deref() + .expect("cuda_stream must be Some when fused_ctx is Some"); + let (feat_dev_ptr, _guard) = features_buf.device_ptr(stream_ref); let market_dim_i32 = market_dim_usize as i32; let bar_idx_i32 = (inferred_num_bars - 1) as i32; if let Err(e) = fused.trainer().launch_sp5_pearl_8_trail( diff --git a/docs/dqn-wire-up-audit.md b/docs/dqn-wire-up-audit.md index d6b0c7e4e..b8f440757 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 Task A7 fix-up — close two minor review findings (2026-05-01): combined spec-quality review caught two minor issues in the Pearl 8 commit. (1) The audit doc's description of test 14 incorrectly claimed `atr_norm` was chosen to give `atr_abs=10.0` and `Short/Long=20.0`. The actual test uses `atr_norm=0.5` → `log_atr=1.0` → `atr_abs=e^1≈2.7183` → `Short/Long≈5.4366`. Updated the audit doc test-14 description to match the actual values. (2) `training_loop.rs:3640` used `let _ = std::mem::ManuallyDrop::new(guard);` to keep the cudarc StreamGuard alive past the inner block — the pattern leaks the guard's borrow bookkeeping for the lifetime of the function rather than dropping it after the kernel launch. Replaced with the idiomatic `let (feat_dev_ptr, _guard) = features_buf.device_ptr(stream_ref);` pattern at the same scope as the launch (matches existing conventions for SP4 producer wire-ups), with a comment explaining the bookkeeping intent. Comment-only / pattern-cleanup changes; no behavior change. cargo check + cargo test --no-run both clean. + SP5 Task A7 — Pearl 8 per-direction trail-stop distance GPU producer (Layer A, 2026-05-01): one new CUDA kernel (`pearl_8_trail_kernel.cu`) lands as a Layer A additive producer feeding ISV slots [270..274) with per-direction trail-stop distances. No `check_trailing_stop` consumer migration in this commit — Layer B wires the consumer when the per-direction trail distance is read at trade-event time. `pearl_8_trail_update`: single-block 4-thread kernel (one thread per direction: Short/Hold/Long/Flat). Reads features[bar_idx × market_dim + 9] (ATR_NORM column), denormalizes via the canonical fxcache scheme `atr_abs = max(0.01, exp(atr_norm × 16 − 7))` (constants 16, 7, 0.01 are Invariant 1 anchors from the existing fxcache normalization in `experience_kernels.cu:2701`). Per-direction logic: Short[270] = atr_abs × ATR_TRAIL_FACTOR=2.0 (industry-standard 2× ATR trail), Hold[271] = EPS_CLAMP_FLOOR=1.0 (Hold doesn't trail-stop; floor prevents zero), Long[272] = atr_abs × 2.0 (same as Short for Layer A), Flat[273] = EPS_CLAMP_FLOOR=1.0. **Layer A simplification**: Short and Long share the same current-bar ATR value. The spec sketch implied direction-conditional ATR (`atr_ema_per_dir[4]` from per-trade-event aggregation conditioned on direction context), but that requires infrastructure not yet in place. The 4-slot ISV layout preserves the consumer contract so a future Layer C extension can populate truly direction-specific values without breaking Layer B's `check_trailing_stop` reads. `launch_sp5_pearl_8_trail(current_bar_idx)` reads the trainer's `latest_features_dev_ptr` + `config().market_dim`, fires the producer kernel writing scratch[199..203), then 4 `launch_apply_pearls` calls (one per direction) chained on the same stream with ALPHA_META=1e-3 (default rate). Wiener offsets: `213 + (270-174)×3 = 501` through `213 + (273-174)×3 = 510` — well within the 543-float wiener_state_buf. Buffer growth: `producer_step_scratch_buf` 199→203 slots (SP5_SCRATCH_TOTAL); 1 new scratch constant `SCRATCH_PEARL_8_TRAIL=199`. wiener_state_buf stays at 543 (sized at A1 for entire SP5 block). StateResetRegistry gains 1 new FoldReset entry: `sp5_trail_dist_per_dir` (ISV[270..274)) — Pearl A sentinel 0 at fold boundary fires first-observation replacement. Wire-up in training_loop.rs: `launch_sp5_pearl_8_trail(current_bar_idx)` called at the per-step boundary alongside other SP5 producer launches; `current_bar_idx` is read from the existing per-step bar-iteration variable. Two GPU-only `#[ignore = "requires GPU"]` unit tests: (14) `pearl_8_trail_per_direction_factors` — synthetic atr_norm chosen so atr_abs=10.0; verifies Short/Long = 20.0, Hold/Flat = 1.0; (15) `pearl_8_trail_atr_floor_at_quiet_market` — atr_norm=-10 → log_atr=-167 → exp(-167)≈0 → max(_,0.01)=0.01; verifies Short/Long = 0.02 (floor protection). No CPU compute, no HtoD/HtoH, no atomicAdd, no stubs, no consumer migration. Touched: `cuda_pipeline/pearl_8_trail_kernel.cu` (new), `build.rs` (+1 cubin entry), `cuda_pipeline/gpu_dqn_trainer.rs` (SP5_SCRATCH_TOTAL 199→203 + 1 new scratch constant + 1 static cubin + 1 struct field + cubin loader + struct initializer + launch method), `trainers/dqn/state_reset_registry.rs` (+1 FoldReset entry), `trainers/dqn/trainer/training_loop.rs` (+5 LOC after Pearl 5 launch + ManuallyDrop guard + DevicePtr trait import), `tests/sp5_producer_unit_tests.rs` (+2 GPU-only tests + 1 cubin constant + 1 loader helper). Hard rules: feedback_no_cpu_compute_strict, feedback_no_atomicadd, feedback_no_htod_htoh_only_mapped_pinned, feedback_no_stubs (Layer A simplification is documented design choice, not a stub), feedback_no_partial_refactor (no consumer migration — Layer A additive only). SP5 Task A6 fix-up — close two minor review findings (2026-05-01): combined spec-quality review caught two minor issues in the Pearl 6 commit. (1) Test 12 (`pearl_6_kelly_within_fold_ewma_blend`) was missing an explicit assertion for slot 282 (TRADE_VAR_SMOOTH_IDX), even though the test setup initialized `tvar_i32` and the launcher passed it. With n_envs=1, the kernel's `(kelly_count > 1) ? variance : 0.0f` branch returns 0, so EWMA blend yields `0.99 × 0.5 + 0.01 × 0.0 = 0.495`. Added `assert!((isv[TRADE_VAR_SMOOTH_IDX] - 0.495).abs() < 2e-5)` to close the within-fold test coverage gap for s==2. (2) `pearl_6_kelly_kernel.cu:136` doc comment said the slot is "standard deviation of per-env Kelly fractions" but the code computes `ksum_sq / kelly_count` (variance), and the slot is named `TRADE_VAR_SMOOTH_INDEX` — the comment was wrong, the name and code are right. Updated comment to "variance of per-env Kelly fractions" and added clarifying note that this is the second moment, NOT std-dev. Comment-only and test-only changes; no behavior change. cargo build --release + cargo test --no-run both clean.