From 2937da8898a1b86dfcc25782edb730beb173ec95 Mon Sep 17 00:00:00 2001 From: jgrusewski Date: Mon, 11 May 2026 08:18:11 +0200 Subject: [PATCH] fix(sp21): inflated return + missing best.safetensors at training end (atomic) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two smoke-driven fixes for issues surfaced by smoke v1 (train-grfcw). Fix 1: inflated Return (financials.rs) - Prior `exp(sum(log(1+r_i))) - 1` produced `Return=+8.730e19%` on ~4M step_returns/epoch. Math correct but metric meaningless. - Fix: ANNUALIZED compounded return (CAGR). For n_returns ≥ bars_per_year, scale log_growth by `bars_per_year / n_returns` then exp. Short rollouts (tests/warmup) fall back to total compounded. Clamped to log-space `[-23, +20]` for display sanity. - Smoke v1 epoch 3 with fix: 24.7% annualized (was +e19%). - Documented v1→v2→v3 history in comment. Fix 2: missing dqn_fold{N}_best.safetensors (training_loop.rs) - Smoke v1 evaluate phase failed with "Failed to load DQN checkpoint" for fold 0 and fold 1. - Root cause: async best-worker swallowed errors non-fatally; with 3 epochs and checkpoint_frequency=10, periodic never fired; if async failed, NO checkpoint existed. - Fix: guaranteed final save at training end. After async drain, restore_best_gpu_params + serialize + sync callback(is_best=true). Idempotent if async already wrote; authoritative if it failed. All errors here non-fatal (training succeeded; eval reports its own missing-ckpt at proper boundary). Files changed: - crates/ml/src/trainers/dqn/financials.rs: v3 annualized return - crates/ml/src/trainers/dqn/trainer/training_loop.rs: final save - docs/dqn-wire-up-audit.md: 2026-05-11 audit entry Verification (passing): - cargo check -p ml --tests --features cuda: 0 errors - cargo test -p ml --lib financials: 7/7 - cargo test -p ml --lib sp21_isv_slots: 4/4 - sp20_aggregate_inputs_test: 12/12 - sp20_phase1_4_wireup_test: 2/2 - sp20_emas_compute_test: 4/4 - sp20_controllers_compute_test: 7/7 - sp21_per_trade_predicted_q_test: 3/3 Total: 39 tests, 0 failures. Smoke v2 (train-rl5x2) is on commit ad99b79e0 and won't have these fixes. A v3 dispatch after this commit validates both fixes end- to-end. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/ml/src/trainers/dqn/financials.rs | 76 +++++++--- .../src/trainers/dqn/trainer/training_loop.rs | 68 ++++++++- docs/dqn-wire-up-audit.md | 131 ++++++++++++++++++ 3 files changed, 252 insertions(+), 23 deletions(-) diff --git a/crates/ml/src/trainers/dqn/financials.rs b/crates/ml/src/trainers/dqn/financials.rs index 5f98906fc..2e3c6f1d3 100644 --- a/crates/ml/src/trainers/dqn/financials.rs +++ b/crates/ml/src/trainers/dqn/financials.rs @@ -58,23 +58,40 @@ pub(crate) fn compute_epoch_financials( 0.0 }; - // Total return: actual cumulative compounded portfolio return over the - // rollout — the standard meaning of "Return". Uses log-space sum to avoid - // f64 overflow on long rollouts and clamps step returns at -100% (loss - // of all capital) so a single extreme bar can't push (1+r) to ≤ 0. + // Total return: ANNUALIZED compounded portfolio return (CAGR-style). // - // Previously this was `mean_per_bar × bars_per_year` ("annualized mean - // arithmetic return") which produced misleading numbers like -51,234% - // when per-bar mean was -52 bps × 98,280 bars/year. Multiplying any - // moderately negative per-bar mean by ~100K creates a value that LOOKS - // like portfolio collapse but is actually a label artefact — the formula - // assumed compounding when the metric used arithmetic annualization. + // **Inflation fix (2026-05-11)** — the prior commit's "total compounded + // return over the rollout" formula produced absurd magnitudes for long + // training rollouts. With ~4M step_returns per epoch and even tiny + // positive per-bar mean (~1e-5), the compounded product grew to e19 + // and beyond (smoke v1 cycle 3: `Return=+8.730e19%`). Math was correct + // but the metric had no operational meaning — no real trader holds + // 4M-bar positions, so "compounded total return over 4M bars" is a + // label artefact. // - // The compounded form below cannot exceed -100% (clamped at log-space - // step floor) and matches what a trader would actually realise from - // following the policy over the rollout. Annualization is left to - // downstream callers if needed (multiply by bars_per_year/n_returns). - let bars_per_year = bars_per_day * 252.0; // retained for trade-annualization below + // The annualized formula below normalizes by bars_per_year, producing + // a per-year compounded growth rate (CAGR): + // annualized = exp(log_growth × bars_per_year / n_returns) − 1 + // + // This is the standard portfolio-management metric: regardless of + // rollout length, the value answers "what annualized return is the + // policy producing?" For the smoke v1 cycle 3 numbers (log_growth ≈ + // 45.93, n_returns = 4_096_000, bars_per_year ≈ 19_656 for 5-min + // bars): annualized = exp(45.93 × 19_656 / 4_096_000) − 1 ≈ 24.7% — + // a reasonable, interpretable number. + // + // History (do not regress to either earlier form): + // v1 (pre-2026-04): mean_per_bar × bars_per_year (arithmetic + // annualization) — produced -51,234% on negative per-bar means. + // v2 (2026-04..2026-05-11): log_growth.exp() − 1 (total compounded, + // no annualization) — produced +e19% on long rollouts. + // v3 (this commit, 2026-05-11): annualized compounded — bounded by + // construction and operationally meaningful. + // + // Lower bound: -100% (log_growth.exp() − 1 ≥ -1 when log_growth ≥ + // log(1e-10) ≈ -23 per the per-bar clamp). Upper bound: practically + // capped by the annualization exponent ÷ rollout-length normalization. + let bars_per_year = bars_per_day * 252.0; let returns_slice = &trade_stats.step_returns; let n_returns = returns_slice.len(); let total_return = if n_returns > 0 { @@ -87,8 +104,33 @@ pub(crate) fn compute_epoch_financials( if g.is_finite() { g.ln() } else { 0.0 } }) .sum(); - if log_growth.is_finite() { - log_growth.exp() - 1.0 + // Annualization factor: bars_per_year / n_returns. For an N-bar + // rollout this scales log_growth to an equivalent per-year + // log-growth, then exp() gives the CAGR. + // + // Short-rollout guard: when n_returns < bars_per_year the + // annualization factor exceeds 1, which would extrapolate a + // brief positive run to absurd annualized magnitudes (a 5-bar + // test with 1.5% gain would annualize to e8% on 1-min bars). + // For sub-year rollouts, report TOTAL compounded growth + // instead — same `log_growth.exp() − 1` as the v2 formula, + // bounded by the per-bar floor to ±100% per the inflation + // audit's WR=46% honest meter discipline. + let n_returns_f = n_returns as f64; + if log_growth.is_finite() && n_returns_f > 0.0 && bars_per_year > 0.0 { + let log_scaled = if n_returns_f >= bars_per_year { + // Production rollout (typically ~4M bars vs ~20K bars/year) + // — annualize for CAGR semantics. + log_growth * bars_per_year / n_returns_f + } else { + // Short rollout (tests, warmup) — report total compounded. + log_growth + }; + // Sanity bounds: lower at -23 (≈ -100% by construction + // from per-bar `(1+r).max(1e-10)` floor), upper at +20 + // (≈ exp(20) − 1 ≈ +4.85e8% — beyond this magnitude the + // signal is more likely a numerical artefact than reality). + log_scaled.clamp(-23.0, 20.0).exp() - 1.0 } else { 0.0 } diff --git a/crates/ml/src/trainers/dqn/trainer/training_loop.rs b/crates/ml/src/trainers/dqn/trainer/training_loop.rs index 6bc85a755..365f87132 100644 --- a/crates/ml/src/trainers/dqn/trainer/training_loop.rs +++ b/crates/ml/src/trainers/dqn/trainer/training_loop.rs @@ -1774,6 +1774,62 @@ impl DQNTrainer { // the "best" file half-written. self.await_pending_checkpoint_handles().await; + // **Checkpoint-save reliability fix (2026-05-11)** — guarantee a + // `_best.safetensors` exists after every training run. + // + // Prior behaviour: the async best-checkpoint worker only fires + // when `epoch_sharpe > best_sharpe` (per + // `handle_epoch_checkpoints_and_early_stopping`). Even when it + // fires, `await_pending_checkpoint_handles` only logs worker + // errors (non-fatal). If the worker silently failed OR the + // first epoch produced negative Sharpe AND every subsequent + // epoch was worse (degenerate but possible cold-start path), + // NO `_best.safetensors` would be written. The smoke v1 + // (train-grfcw) hit this failure mode — the evaluate phase + // got `Failed to load DQN checkpoint: dqn_fold{0,1}_best.safetensors` + // for both folds. + // + // Fix: at end of training, ALWAYS restore best GPU params (idempotent + // — if `best_epoch == 0` is the only available snapshot, that's + // already the current state) and synchronously write a final + // `_best.safetensors` via `checkpoint_callback(is_best=true)`. + // If the async worker already wrote the same bytes, this overwrites + // with identical content (no harm). If the async worker failed, + // this is the authoritative save. + // + // Errors here are logged but non-fatal — the training run already + // succeeded; a save failure shouldn't poison the returned metrics. + // Downstream `evaluate_baseline` will report a missing-checkpoint + // error at its own load site, which is the right failure boundary. + if let Err(e) = self.restore_best_gpu_params() { + tracing::warn!( + "Final best-params restore failed (non-fatal, using current params): {e:#}" + ); + } + match self.serialize_model().await { + Ok(checkpoint_data) => { + let final_epoch = self.best_epoch.max(1); + let cb_lock = checkpoint_callback.lock(); + match cb_lock { + Ok(mut cb) => match cb(final_epoch, checkpoint_data, true) { + Ok(path) => info!( + "Final best checkpoint saved at training end: {} (best_sharpe={:.4}, best_epoch={})", + path, self.best_sharpe, self.best_epoch + ), + Err(e) => tracing::warn!( + "Final best checkpoint save failed (non-fatal): {e:#}" + ), + }, + Err(e) => tracing::warn!( + "Final best checkpoint callback poisoned (non-fatal): {e}" + ), + } + } + Err(e) => tracing::warn!( + "Final model serialization failed (non-fatal — no best.safetensors will be written this run): {e:#}" + ), + } + let training_duration = start_time.elapsed(); let metrics = self.create_final_metrics( @@ -6986,12 +7042,12 @@ impl DQNTrainer { "dqn", "current", financials.buy_pct, financials.sell_pct, financials.hold_pct, ); - // `total_return` is log-space cumulative growth across every - // per-bar step_return (financials.rs:80-94). With ~4M step - // returns in a fold-convergence run, even sub-bps positive - // bars compound to absurd magnitudes (observed: 1.93e37%). - // Math is correct; display needs scientific notation to render - // compactly without hiding the magnitude. + // `total_return` is the ANNUALIZED compounded CAGR (post + // 2026-05-11 inflation fix in financials.rs). Bounded by + // construction in `[-100%, +4.85e8%]` via log-space + // clamps at [-23, +20]. Typical values should fall in + // `[-50%, +500%]` for a healthy policy. The {+:.3e} format + // preserves precision across the bounded range. info!( "Epoch {}/{}: Sharpe={:.2} Sharpe_raw={:.6} WinRate={:.1}% MaxDD={:.3}% PF={:.2} Return={:+.3e}% Trades={}", epoch + 1, self.hyperparams.epochs, diff --git a/docs/dqn-wire-up-audit.md b/docs/dqn-wire-up-audit.md index 61b17efd9..684a4852c 100644 --- a/docs/dqn-wire-up-audit.md +++ b/docs/dqn-wire-up-audit.md @@ -14967,6 +14967,137 @@ the ceiling adaptively. No other controllers saturated in the enrichment controllers (only Invariant 1 stability carve-outs on bound-on-bound formulas, which terminate the recursion). +## 2026-05-11 — smoke v1 follow-up fixes: inflated return + missing best.safetensors (atomic) + +### Scope (atomic single commit, two smoke-driven fixes) + +Smoke v1 (train-grfcw, commit `1d2dd38a1`) surfaced two issues +unrelated to the SP21 cascade itself. Fixed together because they +share the same triggering smoke and are operationally complementary +(both block downstream eval). + +### Fix 1: inflated `Return=+e19%` (financials.rs) + +**Symptom**: smoke v1 epoch 3 logged `Return=+8.730e19%`. Prior +two epochs: `+2.963e2%` (epoch 1), `+7.023e1%` (epoch 2). The +trend was monotonically inflating — by epoch 3 the number was +operationally meaningless. + +**Root cause**: `compute_epoch_financials` v2 formula (from a +prior commit) computed TOTAL compounded return as +`exp(sum(log(1+r_i))) - 1`. For ~4M step_returns per epoch +with even tiny positive per-bar mean (~1e-5), the compounded +product grew to `e19` and beyond. Math was correct but the +metric had no operational meaning — no real trader holds 4M-bar +positions; "compounded total return over 4M bars" is a label +artefact, same root pathology as `pre-2026-04` v1's +"mean × bars_per_year" arithmetic annualization producing +-51,234% on negative per-bar means. + +**Fix**: change semantic to ANNUALIZED compounded return (CAGR). + +```rust +let log_scaled = if n_returns_f >= bars_per_year { + log_growth * bars_per_year / n_returns_f // production rollout → annualize +} else { + log_growth // short rollout (tests) → total +}; +log_scaled.clamp(-23.0, 20.0).exp() - 1.0 // sanity bounds +``` + +Smoke v1 epoch 3 with the fix: +- log_growth=45.93, n_returns=4_096_000, bars_per_year≈19_656 +- log_scaled = 45.93 × 19_656 / 4_096_000 = 0.220 +- exp(0.220) − 1 = 24.7% annualized — interpretable. + +Short-rollout fallback preserves test semantics (test_mixed_trades +expects `total_return > 0.0` on 5-bar input → 1.5% total compounded, +unchanged behaviour). + +History documented in code comment: +- v1 (pre-2026-04): `mean_per_bar × bars_per_year` → -51,234% bug +- v2 (2026-04..2026-05-11): `log_growth.exp() − 1` → +e19% bug +- v3 (this commit): annualized compounded with sanity bounds → sane + +### Fix 2: missing dqn_fold{N}_best.safetensors (training_loop.rs) + +**Symptom**: smoke v1 evaluate phase failed with: +``` +Fold 0 evaluation failed: Failed to load DQN checkpoint: + /workspace/output/dqn_fold0_best.safetensors +Fold 1 evaluation failed: Failed to load DQN checkpoint: + /workspace/output/dqn_fold1_best.safetensors +``` + +**Root cause**: best-checkpoint save fires only on +`epoch_sharpe > self.best_sharpe`. The async worker that writes +the bytes is spawned with `spawn_blocking`; failures are logged +non-fatally by `await_pending_checkpoint_handles`. If the worker +silently failed for any reason (memcpy_dtod_async error, +safetensors serialize error, fs::write race), NO checkpoint gets +written. Periodic checkpoint (every 10 epochs by default) doesn't +fire in 3-epoch smoke runs. + +**Fix**: add a guaranteed FINAL save at end of training. After +`await_pending_checkpoint_handles`: + +```rust +self.restore_best_gpu_params()?; +let bytes = self.serialize_model().await?; +checkpoint_callback.lock()?(best_epoch, bytes, true /* is_best */)?; +``` + +If the async worker already wrote the same bytes, this overwrites +identical content (idempotent). If async failed, this is the +authoritative save. All errors here are `tracing::warn!` (non-fatal) +since the training run itself already succeeded — the save failure +shouldn't poison the returned `TrainingMetrics`. Downstream +`evaluate_baseline` reports its own missing-checkpoint at load +time, which is the proper failure boundary. + +### Files changed + +| File | Status | Purpose | +|------|--------|---------| +| `crates/ml/src/trainers/dqn/financials.rs` | annualized return | v3 formula: `exp(log_growth × bars_per_year / n_returns).clamp(-23, +20) - 1`; short-rollout fallback to total compounded; explicit history in comment | +| `crates/ml/src/trainers/dqn/trainer/training_loop.rs` | final best save | After `await_pending_checkpoint_handles`, restore best params + serialize + sync callback with `is_best=true`. Display comment updated to reflect annualized semantic | +| `docs/dqn-wire-up-audit.md` | This entry | 2026-05-11 audit log | + +### Pearls + invariants honoured + +- `feedback_no_partial_refactor` — both fixes ship atomically; + they're complementary smoke-driven follow-ups. +- `feedback_no_stubs` — `restore_best_gpu_params` already existed + but was only called on PLATEAU_EXHAUSTED; now also on normal + training completion. No stub addition. +- `feedback_kill_runs_on_anomaly_quickly` — both issues blocked + downstream eval (the natural smoke-validation gate); fixing + them at the boundary keeps the smoke loop closed. +- `project_metric_pipeline_inflation_audit.md` — Fix 1 closes the + "Return=+1.5e29% from scale mismatch" entry in that audit (the + scale mismatch was the OLDER v1 bug; v2 was already a refactor; + v3 is the operationally correct CAGR). + +### Verification + +``` +cargo check -p ml --tests --features cuda: 0 errors +cargo test -p ml --lib financials: 7/7 +cargo test -p ml --lib sp21_isv_slots: 4/4 +sp20_aggregate_inputs_test: 12/12 +sp20_phase1_4_wireup_test: 2/2 +sp20_emas_compute_test: 4/4 +sp20_controllers_compute_test: 7/7 +sp21_per_trade_predicted_q_test: 3/3 +``` + +Total: 39 tests, 0 failures. Smoke v3 dispatch after this commit +will validate both fixes: +- Smoke epoch Return should display in `[-100%, +500%]` range + (typical) instead of `+e19%`. +- evaluate phase should find `dqn_fold{0,1,2}_best.safetensors` + for each fold and proceed without "Failed to load DQN checkpoint". + ## 2026-05-11 — SP21 T2.2 Phase 7.5: E8 curriculum_weights → per-segment PER insert priority boost (atomic) ### Scope (atomic single commit)