gitadmin/foxhunt
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

feat(sp20): Phase 3 Task 3.1 — errata for redundant hold_cost_scale_compute_kernel

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
jgrusewski
2026-05-10 01:19:05 +02:00
parent e1aef53373
commit a5ee49e8ab
1 changed files with 193 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

193
docs/superpowers/plans/2026-05-09-sp19-20-wr-first.md
View File

@@ -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<f32>` (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.