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

guard+cleanup(cuda): DtoD-via-pinned pre-commit guard + delete orphan HER

Browse Source 
Two related changes installing the structural guard against the SP6 Pearl 5
IQN τ failure mode (root cause fixed at facbf76eb for that one site) and
removing the only remaining orphan callers of the broken pattern.

The bug class. The mapped_pinned::{upload,clone_to_device}_{f32,i32}_via_pinned
helpers are named to suggest "no HtoD per feedback_no_htod_htoh_only_mapped_pinned"
but their bodies do MappedXBuffer::new() + memcpy_dtod_async() +
stream.synchronize(). The DtoD copy and synchronize are both forbidden
inside CUDA Graph capture (CUDA_ERROR_STREAM_CAPTURE_INVALIDATED) and add a
host stall otherwise. The canonical pattern is MappedXBuffer stored directly
+ write_from_slice + kernel reads via .dev_ptr, used by SP4 portfolio_state
and SP6 IQN τ at facbf76eb.

Guard. New check_no_dtod_via_pinned in pre-commit-hook.sh rejects any
staged .rs file calling upload_(f32|i32)_via_pinned or
clone_to_device_(f32|i32)_via_pinned, except mapped_pinned.rs itself. Per
feedback_no_hiding: no suppression marker. Also fixes a pre-existing
silent-skip bug: the gpu-hotpath-guard.sh invocation used
$(cd "$(dirname "$0")" && pwd) which resolved to .git/hooks/ (the symlink's
directory) instead of scripts/, so the guard never ran. Replaced with
readlink -f "$0" + an explicit "guard missing" error branch — silent skip
is worse than no guard.

Orphan deletion. gpu_her.rs carried legacy relabel_batch, generate_random_donors
(CPU), HerBatch, slice_clone_f32, slice_clone_i32 — zero production callers
(verified via grep). Production uses relabel_batch_with_strategy +
generate_random_donors_gpu. The orphan held the only upload_i32_via_pinned
callers in the codebase; per feedback_no_hiding the right fix is delete.

Scope. Eliminates 2 of 47 production *_via_pinned call sites. Remaining 45
across 14 files are cold-path init — graph-capture-fragile and host-stalling
but not breaking operationally. Guard enforces no new calls; existing 45
migrate in subsequent atomic per-buffer commits. After all 45 are converted,
the four helpers themselves get deleted from mapped_pinned.rs.

Validation. Smoke smoke-test-82fjk at facbf76eb succeeded — magnitude
differentiation restored (q_full=0.462 > q_half=0.409 > q_quarter=0.350 vs
baseline frozen Pascal-triangle 0.225/0.280/0.495), eval distribution
unfrozen (eq=0.596, eh=0.404, ef=0.000 vs baseline single-action collapse).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
jgrusewski
2026-05-02 10:12:23 +02:00
parent facbf76eb5
commit 5275932f4c
3 changed files with 86 additions and 213 deletions
Download Patch File Download Diff File Expand all files Collapse all files

216
crates/ml/src/cuda_pipeline/gpu_her.rs
View File

@@ -29,7 +29,7 @@
use std::sync::Arc;
use cudarc::driver::{CudaFunction, CudaSlice, CudaStream, DevicePtr, LaunchConfig, PushKernelArg};
use cudarc::driver::{CudaFunction, CudaSlice, CudaStream, LaunchConfig, PushKernelArg};
use tracing::info;
use crate::MLError;
@@ -91,28 +91,6 @@ impl GpuHerConfig {
}
}
// ---------------------------------------------------------------------------
// GPU-resident relabeled batch
// ---------------------------------------------------------------------------
/// GPU-resident HER relabeled batch -- staging buffer slices ready for
/// concatenation with the normal PER batch.
#[allow(missing_debug_implementations)]
pub struct HerBatch {
/// Relabeled states `[her_batch_size, state_dim]` -- f32 on device.
pub states: CudaSlice<f32>,
/// Relabeled next_states `[her_batch_size, state_dim]` -- f32 on device.
pub next_states: CudaSlice<f32>,
/// Actions (copied unchanged) `[her_batch_size]` -- i32 on device.
pub actions: CudaSlice<i32>,
/// Recomputed rewards `[her_batch_size]` -- f32 on device.
pub rewards: CudaSlice<f32>,
/// Dones (copied unchanged) `[her_batch_size]` -- f32 on device.
pub dones: CudaSlice<f32>,
/// Number of relabeled samples in this batch.
pub batch_size: usize,
}
// ---------------------------------------------------------------------------
// GpuHer
// ---------------------------------------------------------------------------
@@ -120,8 +98,9 @@ pub struct HerBatch {
/// GPU-accelerated Hindsight Experience Replay.
///
/// Owns pre-allocated staging buffers for relabeled experiences and the compiled
/// relabel kernel. Call [`relabel_batch`] to produce a [`HerBatch`] from the
/// GPU replay buffer's tensor storage.
/// relabel kernel. Production callers use [`GpuHer::relabel_batch_with_strategy`]
/// (Future/Final, GPU-native donor selection) and
/// [`GpuHer::generate_random_donors_gpu`] (Random strategy, GPU RNG).
#[allow(missing_debug_implementations)]
pub struct GpuHer {
/// Configuration.
@@ -229,136 +208,6 @@ impl GpuHer {
})
}
/// Generate a relabeled batch from GPU replay buffer storage.
///
/// 1. Accepts pre-generated source indices (PER-weighted) and random donor
/// indices (for Random strategy).
/// 2. Uploads index arrays to GPU.
/// 3. Launches the relabel kernel.
/// 4. Returns staging buffer references as a `HerBatch`.
///
/// # Arguments
///
/// * `buffer_states` - `[capacity, state_dim]` f32 on device (replay buffer storage)
/// * `buffer_next_states` - `[capacity, state_dim]` f32 on device
/// * `buffer_actions` - `[capacity]` i32 on device
/// * `buffer_rewards` - `[capacity]` f32 on device
/// * `buffer_dones` - `[capacity]` f32 on device
/// * `source_idx_host` - PER-sampled source indices (CPU)
/// * `donor_idx_host` - Donor indices (CPU, strategy-dependent)
/// * `buffer_size` - Current replay buffer occupancy
/// * `her_batch_size` - Number of experiences to relabel
pub fn relabel_batch(
&mut self,
buffer_states: &CudaSlice<f32>,
buffer_next_states: &CudaSlice<f32>,
buffer_actions: &CudaSlice<i32>,
buffer_rewards: &CudaSlice<f32>,
buffer_dones: &CudaSlice<f32>,
source_idx_host: &[i32],
donor_idx_host: &[i32],
buffer_size: usize,
her_batch_size: usize,
) -> Result<HerBatch, MLError> {
if her_batch_size == 0 {
return Err(MLError::InvalidInput(
"HER batch size must be > 0".to_owned(),
));
}
let max_her = self.config.her_batch_size();
if her_batch_size > max_her {
return Err(MLError::InvalidInput(format!(
"HER batch size {her_batch_size} exceeds pre-allocated max {max_her}"
)));
}
// Upload source and donor indices to GPU via mapped-pinned staging
// (per-relabel call; pinned staging is allocated+freed each call —
// acceptable on this cold path which runs once per epoch).
super::mapped_pinned::upload_i32_via_pinned(
&self.stream,
&source_idx_host[..her_batch_size],
&mut self.source_indices,
)
.map_err(|e| MLError::ModelError(format!("HER source_indices upload via pinned: {e}")))?;
super::mapped_pinned::upload_i32_via_pinned(
&self.stream,
&donor_idx_host[..her_batch_size],
&mut self.donor_indices,
)
.map_err(|e| MLError::ModelError(format!("HER donor_indices upload via pinned: {e}")))?;
// Launch relabel kernel: one warp (32 threads) per sample
let launch_config = LaunchConfig {
grid_dim: (her_batch_size as u32, 1, 1),
block_dim: (32, 1, 1),
shared_mem_bytes: 0,
};
let buffer_size_i32 = buffer_size as i32;
// Safety: all buffer pointers are valid GPU allocations on the same context.
// source_indices and donor_indices contain valid buffer indices in [0, buffer_size).
// Output buffers are pre-allocated with sufficient capacity.
let state_dim_i32 = ml_core::state_layout::STATE_DIM as i32;
unsafe {
self.stream
.launch_builder(&self.relabel_func)
.arg(buffer_states)
.arg(buffer_next_states)
.arg(buffer_actions)
.arg(buffer_rewards)
.arg(buffer_dones)
.arg(&self.source_indices)
.arg(&self.donor_indices)
.arg(&buffer_size_i32)
.arg(&mut self.out_states)
.arg(&mut self.out_next_states)
.arg(&mut self.out_actions)
.arg(&mut self.out_rewards)
.arg(&mut self.out_dones)
.arg(&state_dim_i32)
.launch(launch_config)
.map_err(|e| MLError::ModelError(format!("HER relabel kernel launch: {e}")))?;
}
// Return references to the staging buffers (data is on GPU, ready for concat)
// Clone the CudaSlice handles — they point to the same device memory.
// The caller will use these for D2D copies into the merged training batch.
let states_slice = slice_clone_f32(&self.stream, &self.out_states, her_batch_size * ml_core::state_layout::STATE_DIM)?;
let next_states_slice = slice_clone_f32(&self.stream, &self.out_next_states, her_batch_size * ml_core::state_layout::STATE_DIM)?;
let actions_slice = slice_clone_i32(&self.stream, &self.out_actions, her_batch_size)?;
let rewards_slice = slice_clone_f32(&self.stream, &self.out_rewards, her_batch_size)?;
let dones_slice = slice_clone_f32(&self.stream, &self.out_dones, her_batch_size)?;
Ok(HerBatch {
states: states_slice,
next_states: next_states_slice,
actions: actions_slice,
rewards: rewards_slice,
dones: dones_slice,
batch_size: her_batch_size,
})
}
/// Generate random donor indices for the Random strategy.
///
/// Each donor is a uniform random index in `[0, buffer_size)`.
/// This is done on CPU since it is O(her_batch_size) with trivial
/// per-element cost and avoids cuRAND setup.
pub fn generate_random_donors(
buffer_size: usize,
her_batch_size: usize,
) -> Vec<i32> {
use rand::Rng;
use rand::SeedableRng;
use rand::rngs::StdRng;
let mut rng = StdRng::seed_from_u64(crate::cuda_pipeline::mix_seed(0x4E4_5678));
let mut donors = Vec::with_capacity(her_batch_size);
donors.resize_with(her_batch_size, || rng.gen_range(0..buffer_size as i32));
donors
}
/// GPU-native donor selection for Future and Final strategies.
///
/// Dispatches to the appropriate episode-aware kernel based on the configured
@@ -573,63 +422,6 @@ fn alloc_i32(
})
}
/// Extract raw CUDA device pointer from a CudaSlice<f32>.
///
/// Same pattern as `raw_device_ptr` in `gpu_dqn_trainer.rs` -- wraps the
/// `SyncOnDrop` guard in `ManuallyDrop` to skip read event recording (safe
/// on same stream, same context).
fn raw_ptr_f32(slice: &CudaSlice<f32>, stream: &CudaStream) -> u64 {
let (ptr, guard) = slice.device_ptr(stream);
let _no_drop = std::mem::ManuallyDrop::new(guard);
ptr
}
/// Extract raw CUDA device pointer from a CudaSlice<i32>.
fn raw_ptr_i32(slice: &CudaSlice<i32>, stream: &CudaStream) -> u64 {
let (ptr, guard) = slice.device_ptr(stream);
let _no_drop = std::mem::ManuallyDrop::new(guard);
ptr
}
/// Clone a sub-range of a CudaSlice<f32> via async D2D memcpy.
fn slice_clone_f32(
stream: &Arc<CudaStream>,
src: &CudaSlice<f32>,
n: usize,
) -> Result<CudaSlice<f32>, MLError> {
let dst = alloc_f32(stream, n, "her_clone_f32")?;
let num_bytes = n * std::mem::size_of::<f32>();
let src_ptr = raw_ptr_f32(src, stream);
let dst_ptr = raw_ptr_f32(&dst, stream);
// Safety: both src and dst are valid device memory on the same context.
// n elements * 4 bytes does not exceed either allocation.
unsafe {
cudarc::driver::result::memcpy_dtod_async(
dst_ptr, src_ptr, num_bytes, stream.cu_stream(),
).map_err(|e| MLError::ModelError(format!("HER D2D clone f32: {e}")))?;
}
Ok(dst)
}
/// Clone a sub-range of a CudaSlice<i32> via async D2D memcpy.
fn slice_clone_i32(
stream: &Arc<CudaStream>,
src: &CudaSlice<i32>,
n: usize,
) -> Result<CudaSlice<i32>, MLError> {
let dst = alloc_i32(stream, n, "her_clone_i32")?;
let num_bytes = n * std::mem::size_of::<i32>();
let src_ptr = raw_ptr_i32(src, stream);
let dst_ptr = raw_ptr_i32(&dst, stream);
// Safety: both src and dst are valid device memory on the same context.
unsafe {
cudarc::driver::result::memcpy_dtod_async(
dst_ptr, src_ptr, num_bytes, stream.cu_stream(),
).map_err(|e| MLError::ModelError(format!("HER D2D clone i32: {e}")))?;
}
Ok(dst)
}
// ---------------------------------------------------------------------------
// Strategy parsing
// ---------------------------------------------------------------------------

23
docs/dqn-gpu-hot-path-audit.md
View File

@@ -245,3 +245,26 @@ Atom-stat block-sum reduction unchanged (warp shfl + shared-mem bank, determinis
Expected wall-clock saving on this kernel: ~33% of 207ms = ~70ms across the smoke run; on production batch=8192 × 1382 calls per fold, roughly proportional. Lower-bound estimate; actual saving depends on memory bandwidth vs compute mix — at low N (smoke) the kernel may be launch-bound rather than load-bound. nsys re-profile after deployment will quantify.
ABI unchanged; no Rust caller changes required.
### Fix 20 — DtoD-via-pinned guard + orphan HER deletion (2026-05-02)
`scripts/pre-commit-hook.sh` + `crates/ml/src/cuda_pipeline/gpu_her.rs`. Two related changes installing the structural guard against the SP6 Pearl 5 IQN τ failure mode (root cause fixed at `facbf76eb` for that one site) and removing the only remaining orphan callers of the broken pattern.
**The bug class.** The `mapped_pinned::{upload,clone_to_device}_{f32,i32}_via_pinned` helpers carry the `_via_pinned` suffix to suggest "no HtoD per `feedback_no_htod_htoh_only_mapped_pinned`", but their bodies do `MappedXBuffer::new()` (staging) + `memcpy_dtod_async()` + `stream.synchronize()`. The DtoD copy and synchronize are both forbidden inside CUDA Graph capture (`CUDA_ERROR_STREAM_CAPTURE_INVALIDATED` — exactly the failure pattern in pre-`facbf76eb` Pearl 5 smokes) and add a host stall otherwise. The "_via_pinned" name was a lie. The canonical pattern is `MappedXBuffer` field stored directly + `write_from_slice` for host writes + kernel reads via `.dev_ptr` — used by SP4 portfolio_state, SP4 grad_prev_buf_per_group, and SP6 IQN τ at `facbf76eb`.
**Guard.** New `check_no_dtod_via_pinned` function in `pre-commit-hook.sh` rejects any staged `.rs` file that calls `upload_(f32|i32)_via_pinned` or `clone_to_device_(f32|i32)_via_pinned`, with a single exception for `mapped_pinned.rs` itself (where the helpers currently live). Per `feedback_no_hiding`: no suppression marker. The error message points at the structural fix (rewrite to `MappedXBuffer` directly).
Also fixes a pre-existing silent skip: the `gpu-hotpath-guard.sh` invocation used `SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"` which resolved to `.git/hooks/` (the symlink's directory) instead of `scripts/` (the script's actual location), so the guard never ran. Replaced with `readlink -f "$0"` to resolve through the symlink chain. Added an explicit `else` branch that errors if the guard binary is missing — silent skip is worse than no guard.
**Orphan deletion.** `gpu_her.rs` carried legacy `pub fn relabel_batch(...)`, `pub fn generate_random_donors(...)` (CPU version), `pub struct HerBatch`, `fn slice_clone_f32`, `fn slice_clone_i32` — confirmed zero callers via:
```
grep -rn '\.relabel_batch(' --include='*.rs' | grep -v 'with_strategy\|_gpu\b'
grep -rn 'HerBatch\|slice_clone_f32\|slice_clone_i32' --include='*.rs'
grep -rn '\.generate_random_donors(' --include='*.rs' | grep -v '_gpu\b'
```
Production uses `relabel_batch_with_strategy(...)` (line 384, called at `fused_training.rs:1555`) which does pure-DtoD donor selection from a pre-existing GPU buffer — no `*_via_pinned` calls. The orphan held the only `upload_i32_via_pinned` callers in the codebase (lines 279, 285). Per `feedback_no_hiding` (wire-or-delete) the right fix is delete; conversion would have been adding `MappedI32Buffer` machinery to dead code.
Also removed the now-unused `DevicePtr` import and updated the module docstring header to reference `relabel_batch_with_strategy` + `generate_random_donors_gpu` (the surviving production paths).
**Scope.** This commit eliminates 2 of the 47 production `*_via_pinned` call sites (the two inside the deleted orphan). The remaining 45 sites across 14 files are cold-path init (constructors, per-fold setup, hyperopt adapters, walk-forward backtest setup) — they don't break operationally because they're outside any captured graph, but they're still graph-capture-fragile and host-stalling. The guard now enforces no NEW calls; the existing 45 will be migrated in subsequent atomic per-buffer commits. After all 45 are converted, the four helpers themselves get deleted from `mapped_pinned.rs`.
**Validation.** Smoke `smoke-test-82fjk` at the τ-buffer-fix HEAD `facbf76eb` succeeded (`phase=Succeeded progress=1/1`, 5-epoch L40S `multi_fold_convergence`). Magnitude differentiation restored (`q_full=0.462 > q_half=0.409 > q_quarter=0.350` vs baseline frozen Pascal-triangle 0.225/0.280/0.495). Eval distribution unfrozen (`eq=0.596, eh=0.404, ef=0.000` vs baseline single-action collapse). Validates that the SP5+SP6 architecture works once the τ buffer DtoD violation is gone.

60
scripts/pre-commit-hook.sh
View File

@@ -57,9 +57,15 @@ if [ -n "$STAGED_FILES" ]; then
fi
fi
# Resolve script directory by following the symlink to the real location.
# When invoked via .git/hooks/pre-commit symlink, $0 points at the symlink
# (.git/hooks/) — `dirname $0` would silently miss the sibling guard scripts
# in scripts/. `readlink -f` resolves through the symlink chain to the real path.
REAL_SCRIPT="$(readlink -f "$0")"
SCRIPT_DIR="$(cd "$(dirname "$REAL_SCRIPT")" && pwd)"
# GPU hot-path leak detection
echo "🔎 Checking for GPU→CPU leaks in hot paths..."
SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
if [ -x "$SCRIPT_DIR/gpu-hotpath-guard.sh" ]; then
if ! "$SCRIPT_DIR/gpu-hotpath-guard.sh" --staged; then
echo "⛔ GPU hot-path leak detected — fix or suppress with // gpu-ok: <reason>"
@@ -67,8 +73,59 @@ if [ -x "$SCRIPT_DIR/gpu-hotpath-guard.sh" ]; then
fi
echo " No GPU→CPU leaks in hot paths"
echo ""
else
echo "❌ Pre-commit guard missing: $SCRIPT_DIR/gpu-hotpath-guard.sh not found or not executable"
exit 1
fi
# DtoD-via-pinned guard: zero use of upload_*_via_pinned or
# clone_to_device_*_via_pinned anywhere in production code.
#
# These helpers internally do memcpy_dtod_async + stream.synchronize() — the
# "via_pinned" name is a lie. They cause CUDA_ERROR_STREAM_CAPTURE_INVALIDATED
# inside any captured graph and force a host-side stall otherwise. There is no
# legitimate use case: every CPU↔GPU communication path must use
# MappedF32Buffer / MappedI32Buffer directly (cuMemHostAlloc DEVICEMAP — host
# writes via host_ptr, kernel reads dev_ptr, zero copy).
#
# Per feedback_no_hiding: no suppression marker. If you find yourself wanting
# to suppress, rewrite the call site instead.
#
# Reference: gpu_iqn_head.rs Pearl 5 τ buffers (commit facbf76eb) — converted
# from CudaSlice + upload helper to MappedF32Buffer per-branch arrays.
check_no_dtod_via_pinned() {
local staged
staged=$(git diff --cached --name-only --diff-filter=ACM | grep '\.rs$' || true)
if [ -z "$staged" ]; then return 0; fi
local bad=""
while IFS= read -r f; do
[ -z "$f" ] && continue
# Only skip the helpers' definitions inside mapped_pinned.rs itself
# (where they currently live; the structural fix removes them entirely).
[[ "$f" == *"cuda_pipeline/mapped_pinned.rs" ]] && continue
local hits
hits=$(grep -nE 'upload_(f32|i32)_via_pinned|clone_to_device_(f32|i32)_via_pinned' "$f" 2>/dev/null \
| grep -vE '^[0-9]+:[[:space:]]*//' \
|| true)
if [ -n "$hits" ]; then
bad+="${bad:+$'\n'}$f"$'\n'"$hits"
fi
done <<< "$staged"
if [ -n "$bad" ]; then
echo "❌ DtoD-via-pinned guard violation: upload_*_via_pinned / clone_to_device_*_via_pinned"
echo " These helpers do memcpy_dtod_async + stream.synchronize() —"
echo " graph-capture-incompatible AND host-stalling. There is no"
echo " legitimate use. Rewrite to MappedF32Buffer / MappedI32Buffer"
echo " stored directly (host_ptr writes; kernel reads dev_ptr; no copy)."
echo " No suppression marker — fix the structure."
echo "$bad" | sed 's/^/ /'
return 1
fi
}
# DQN v2 Invariant 7 enforcement: component-adding commits must update audit docs.
check_audit_doc_updates() {
local staged=$(git diff --cached --name-only)
@@ -122,6 +179,7 @@ check_no_isv_migrations() {
check_audit_doc_updates || exit 1
check_no_todo_fixme || exit 1
check_no_isv_migrations || exit 1
check_no_dtod_via_pinned || exit 1
echo "✅ All pre-commit checks passed!"
echo ""