From a5ee49e8ab3b6d7c06c7f01f2ffeadd57c84fb5d Mon Sep 17 00:00:00 2001 From: jgrusewski Date: Sun, 10 May 2026 01:19:05 +0200 Subject: [PATCH] =?UTF-8?q?feat(sp20):=20Phase=203=20Task=203.1=20?= =?UTF-8?q?=E2=80=94=20errata=20for=20redundant=20hold=5Fcost=5Fscale=5Fco?= =?UTF-8?q?mpute=5Fkernel?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The plan asks for a separate `hold_cost_scale_compute_kernel.cu` to host the two-sided multiplicative ramp 1.05/0.95 controller for `HOLD_COST_SCALE_INDEX = 513`. Reality: the controller is already implemented inside Phase 1.3's `sp20_controllers_compute_kernel.cu` lines 175-194 (block 6 of the 6-controller fused kernel) and tested end-to-end at `sp20_controllers_compute_test::hold_cost_scale_two_ sided_ramp` covering all 5 cases (ramp up, ramp down, deadband, upper- clamp, lower-clamp). Splitting it back into a separate kernel would: 1. Break the in-thread `tgt` reuse optimization documented in the fused kernel header (sp20_controllers_compute_kernel.cu:73-78); forces 1 extra ISV read per step. 2. Add separate launch + cubin + Rust launcher boilerplate for ~20 LoC of kernel logic. 3. Force the production caller (gpu_experience_collector.rs:6549) to make TWO controller-kernel launches per step. Per `feedback_no_partial_refactor` ("when a contract changes, every consumer migrates atomically") + `feedback_no_quickfixes`, the correct response is errata, not a partial-refactor split. This commit also documents 4 additional Phase 3 plan-vs-reality gaps that emerged during implementation review (Gaps 8-11): - Gap 8: Task 3.3's target_hold_pct controller is already wired in Phase 1.3 + has a unit test; Task 3.3 commit will add a behavioral integration test, NO production code changes. - Gap 9: hold_baseline_buffer layout is `[N_envs, 30]` row-major (per-env), NOT a single global ring as the spec phrasing implied — the kernel is per-env-parallel and per-trade attribution requires per-env stride. - Gap 10: `PS_ENTRY_BAR` does NOT exist; `PS_HOLD_TIME` (slot 10) + `segment_hold_time` (already in scope at trade close) suffice. No new state slot needed — keeps PS_STRIDE = 43 contract stable. - Gap 11: Task 3.4 plan refers to `GpuBatchPtrs` but that struct is in `crates/ml-dqn/src/gpu_replay_buffer.rs`. The collector struct is `GpuExperienceBatch`. Both need the new `aux_conf` field + the ring-buffer scatter/gather column in between. Plan-level errata only; no code touched in this commit. Implementation of the actual Phase 3 work continues in Tasks 3.2 / 3.3 / 3.4. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../plans/2026-05-09-sp19-20-wr-first.md | 193 ++++++++++++++++++ 1 file changed, 193 insertions(+) diff --git a/docs/superpowers/plans/2026-05-09-sp19-20-wr-first.md b/docs/superpowers/plans/2026-05-09-sp19-20-wr-first.md index f983639d4..f1c55ebb6 100644 --- a/docs/superpowers/plans/2026-05-09-sp19-20-wr-first.md +++ b/docs/superpowers/plans/2026-05-09-sp19-20-wr-first.md @@ -2087,3 +2087,196 @@ Task 2.2 entry under "Deleted: the `min_hold_*` kernel-arg trio". End of Phase 2 errata. Phase 3 / 3.2 / 4 / 5 / 6 / 7 of the plan remain as specified; future tasks may add additional errata sections as deviations are made during implementation. + +--- + +## Plan Accuracy Errata — Phase 3 (appended 2026-05-10) + +The following deviations from the original Phase 3 plan are documented +here for future implementers. + +### Gap 7: Task 3.1 (`hold_cost_scale_compute_kernel.cu`) is REDUNDANT + +The plan calls for a new dedicated `hold_cost_scale_compute_kernel.cu` ++ `hold_cost_scale_compute.rs` launcher pair to host the two-sided +multiplicative ramp `1.05/0.95` controller for `HOLD_COST_SCALE_INDEX`. +Spec §4.2 ("Implementation") similarly says "1 new producer kernel +`hold_cost_scale_compute_kernel.cu` (~50 LoC, follows SP4 Pearls +A+D)". + +Reality: the controller was already implemented inside Phase 1.3's +`sp20_controllers_compute_kernel.cu` lines 175-194 (block 6 of the 6- +controller fused kernel). The two-sided ramp: + +```cuda +const float upper = tgt + 0.05f; +const float lower = tgt - 0.05f; +float new_scale; +if (hold_pct_ema > upper) new_scale = prev_cost_scale * 1.05f; +else if (hold_pct_ema < lower) new_scale = prev_cost_scale * 0.95f; +else new_scale = prev_cost_scale; +new_scale = fmaxf(0.01f, fminf(new_scale, 0.5f)); +isv[isv_idx_hold_cost_scale] = new_scale; +``` + +is the exact spec §4.2 / plan §3.1 algorithm, and the per-launch GPU +oracle test `sp20_controllers_compute_test::hold_cost_scale_two_ +sided_ramp` already covers all 5 cases (ramp up, ramp down, deadband, +upper-clamp, lower-clamp). + +The `sp20_controllers_compute` kernel module-level docstring (lines +37-46 + line 73-78) explicitly notes this ordering decision: +"TARGET_HOLD_PCT MUST be computed BEFORE HOLD_COST_SCALE because the +controller for HOLD_COST_SCALE reads `tgt`. We compute TARGET_HOLD_PCT +into a local `tgt`, write it to ISV, then use the same local for the +HOLD_COST_SCALE controller — avoids a redundant ISV read-back." + +**Decision**: SKIP Task 3.1's separate-kernel creation. The controller +is fully wired and tested. + +**Rationale**: per `feedback_no_partial_refactor`'s "atomic migration" ++ `feedback_no_quickfixes`'s "every issue gets a proper fix per +established patterns", splitting the controller back into a separate +kernel would: + +1. Break the documented ordering optimization (in-thread `tgt` reuse) + forcing 1 extra ISV read per step in the HOLD_COST_SCALE kernel. +2. Add a separate launch + test cubin + Rust launcher boilerplate for + ~20 LoC of kernel logic — a net code-bloat regression. +3. Force the production caller (gpu_experience_collector.rs:6549) to + make TWO controller-kernel launches per step instead of one. + +The "follows SP4 Pearls A+D" rationale in spec §4.2 (which suggested +sibling-kernel placement) was a pre-design recommendation; the actual +fused kernel in Phase 1.3 is already SP4-Pearls-A-compliant (single- +block, single-thread, mapped-pinned ISV write with __threadfence_ +system after writes — see `sp20_controllers_compute_kernel.cu:196- +199`). + +Task 3.1's commit is therefore an errata-only commit (this entry). + +### Gap 8: Task 3.3 (`target_hold_pct` controller) was already wired in Phase 1.3 + +The plan states: "Already inside Task 1.3 for the other controllers; +this task verifies the wire path is complete and adds a behavioral +test that target_hold_pct adapts to aux confidence distribution." + +Reality confirmed: `TARGET_HOLD_PCT_INDEX = 514` IS computed in +`sp20_controllers_compute_kernel.cu:164-173`: + +```cuda +float tgt; +{ + const float raw = 0.8f - aux_p50_ema * 1.5f; + tgt = fmaxf(0.1f, fminf(raw, 0.8f)); + isv[isv_idx_target_hold_pct] = tgt; +} +``` + +Direct mid-launch unit test exists at +`sp20_controllers_compute_test::target_hold_pct_inverse_relation` +covering aux_p50 ∈ {0.0, 0.5, 0.2}. The plan-specified behavioral +spot-checks (aux_conf_p50 = 0.05 → target ≈ 0.725; aux_conf_p50 = 0.4 +→ target ≈ 0.2) are NEW values that exercise the same formula at +different operating points. + +**Decision**: Task 3.3's atomic commit ADDS an integration-style +behavioral test (`target_hold_pct_behavioral_spot_checks`) that pumps +the full Path C chain (Stats → Aggregate → EMAs → Controllers) with +synthetic aux_logits chosen so `aux_conf_p50_ema` lands at exactly +0.05 / 0.40 after one launch, then asserts the resulting +`ISV[TARGET_HOLD_PCT_INDEX]` matches the spec §4.2 self-stabilizing +properties. NO production code changes. + +**Rationale**: `pearl_tests_must_prove_not_lock_observations` says +"assert invariants, not observed values". The spec §4.2 explicitly +calls out the `aux_conf_p50_ema = 0.05 → target_hold_pct = 0.725` +operating point as a **self-stabilizing-property invariant** ("Aux +untrained warmup pushes cost_scale toward floor 0.01, no explicit +warmup gate needed"). Asserting that operating point holds end-to-end +through the full producer chain is invariant-style, not observed- +value-style. + +### Gap 9: `hold_baseline_buffer` per-env layout NOT in plan + +The plan specifies `hold_baseline_buffer[i % HOLD_BUFFER_SIZE]` (line +1333) and "circular buffer of size 30" (spec §4.2 line 215), implying +a single global ring. But each env has its own concurrent open trade, +so a single global ring would interleave reward bars across envs and +break the per-trade `sum_over_trade_range` semantic. + +**Decision**: layout is `[N_envs, HOLD_BUFFER_SIZE = 30]` f32 row- +major. Each env writes to `[env, current_t % 30]` per bar; at trade +close, sums backwards `min(HOLD_BUFFER_SIZE, segment_hold_time)` slots +in its own row (via `current_t` index mod-30 walking backwards). + +**Rationale**: the spec §4.2 phrasing was implicit single-thread but +the kernel is per-env-parallel. The per-env stride preserves the +trade-attribution semantic without atomicAdd contention (each thread +writes only its own row). + +### Gap 10: `trade_open_bar` tracking — existing `PS_HOLD_TIME` suffices + +The plan's Step 5 says "trade_open_bar, trade_close_bar" args to a +`sum_hold_baseline_buffer` helper. The user's brief explicitly asked +to flag whether `PS_ENTRY_BAR` exists. + +Reality: there is NO `PS_ENTRY_BAR` slot in `state_layout.cuh`; the +PS_* slots top out at PS_PRE_ENTRY_CONVICTION_VAR_EMA = 42 (PS_STRIDE += 43). The closest existing tracking is `PS_HOLD_TIME` (slot 10) — +"consecutive steps with position", incremented per bar inside the +helper and reset to 0 at trade close. + +The trade-close branch already captures `saved_hold_time` BEFORE the +trade-physics helper resets `hold_time` to 0 (experience_kernels.cu: +2618), and exposes it as `segment_hold_time` (line 2994). This is +exactly the trade-duration value needed. + +**Decision**: NO new state slot or per-env trade-open-bar tile. The +`hold_baseline_buffer` consumer at the trade-close site reads +`current_t` (current bar) + `segment_hold_time` (already in scope) and +walks backwards `min(HOLD_BUFFER_SIZE, segment_hold_time)` slots from +`(current_t - 1) % HOLD_BUFFER_SIZE` (the previous bar's write index). + +**Rationale**: per `feedback_no_partial_refactor`, adding a new state +slot is justifiable only when no existing slot suffices. +`segment_hold_time` plus `current_t` is sufficient — no architectural +gap. Skipping the new tile keeps the PS_STRIDE = 43 contract stable +and avoids the StateResetRegistry / ISV layout fingerprint churn the +Task 2.4 errata flagged as a deferral cost. + +### Gap 11: `GpuBatchPtrs` vs `GpuExperienceBatch` naming + +Plan Task 3.4 specifies adding `aux_conf_buf: u64` to `GpuBatchPtrs`. +That struct exists in `crates/ml-dqn/src/gpu_replay_buffer.rs:27` (NOT +in `crates/ml/src/cuda_pipeline/gpu_experience_collector.rs` as the +plan implies). The collector-side struct is `GpuExperienceBatch` +(line 445 of `gpu_experience_collector.rs`). + +The full data flow has TWO struct boundaries: + +``` +ExperienceCollector → GpuExperienceBatch → insert_batch() → ring → sample() → GpuBatchPtrs → Trainer + (collector struct) (replay struct) +``` + +**Decision**: add `aux_conf` field to BOTH structs: + - `GpuExperienceBatch.aux_conf: CudaSlice` (collector output) + - `GpuBatchPtrs.aux_conf_ptr: u64` (replay sampler output) + +Plus the ring-buffer storage column + scatter/gather kernel calls +inside `gpu_replay_buffer.rs:insert_batch` / `sample_proportional`, +mirroring the existing `aux_sign_labels` i32 column wiring (B0 +plumbing pattern). + +**Rationale**: Phase 5 (Aux→Q gate) needs `aux_conf` at the SAMPLED +state, not the current state — the gate computes `gate = +sigmoid((aux_conf - threshold) / temp)` inside the Bellman target, +which fires at replay time. The plumbing must reach that consumer via +the replay buffer, not just the collector batch. This requires +threading both struct boundaries. + +--- + +End of Phase 3 errata. Phase 4 / 5 / 6 / 7 of the plan remain as +specified.