Skip to content

fix(finetune): NF4 QLoRA per-epoch val_loss must reflect trained adapters (FALSIFY-CUDA-EVAL-ADAPTER-SYNC-001) - #2257

Merged
noahgift merged 2 commits into
mainfrom
fix/qlora-eval-adapter-sync-20260702
Jul 2, 2026
Merged

fix(finetune): NF4 QLoRA per-epoch val_loss must reflect trained adapters (FALSIFY-CUDA-EVAL-ADAPTER-SYNC-001)#2257
noahgift merged 2 commits into
mainfrom
fix/qlora-eval-adapter-sync-20260702

Conversation

@noahgift

@noahgift noahgift commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

The bug

On the CUDA path, train_step writes LoRA adapter deltas into the GPU-resident cuda_blocks, but InstructPipeline::evaluate() computes val_loss on the CPU via model.forward_with_lora(x, &self.lora_layers). The CPU lora_layers are only refreshed by sync_lora_to_cpu(), which was invoked exclusively inside save_checkpoint — never before evaluate() in the epoch loop (instruct_trainer.rs:243).

Consequence: per-epoch val_loss is byte-identical across every epoch and every run, so:

  • best_val_loss collapses to the epoch-0 constant
  • best_epoch freezes at 0 → the best/ checkpoint is stale-by-N-epochs
  • early stopping fires on a phantom plateau (the metric never moved)

Trained weights themselves were fine (final save syncs) — only the validation signal and the decisions it drives were dead.

The fix

evaluate() now calls sync_lora_to_cpu() before the CPU forward (takes &mut self), so val_loss reflects the current GPU-trained adapters. A #[cfg(not(feature = "cuda"))] no-op twin keeps the call unconditional — on CPU/WGPU paths lora_layers are updated in place and already current. This makes evaluate self-consistent for every caller, not just the trainer.

Falsifier — FALSIFY-CUDA-EVAL-ADAPTER-SYNC-001

Contract: contracts/finetune-eval-adapter-sync-v1.yaml (pv validate/pv lint clean). The probe injects a GPU-only adapter change via download→(set B nonzero)→upload — deliberately independent of the optimizer/clip path so it runs on a fresh, uncorrupted CUDA context — then asserts the next evaluate() differs.

Verified live on RTX 4090 (sm_89):

val_before val_after |Δ| result
RED (sync removed) 14.25047874 14.25047874 0.0 FAILED ✓
GREEN (sync present) 14.25047874 14.22203255 0.02844620 ok ✓

Testing

  • CPU suite: 7612 pass (3 pre-existing prune::snapshot_tests insta-snapshot failures — unrelated module, present on origin/main)
  • evaluate &mut self change: no other callers of InstructPipeline::evaluate exist workspace-wide; touched test bindings updated to mut
  • Falsifier is GPU-gated (--ignored, needs APR_PARITY_MODEL); not yet CI-enforced pending a CUDA runner

Scope bound

sync_lora_to_cpu reconciles Q and V adapters (the default QLoRA target set). Configs that train K/O/gate/up/down would still evaluate those partially stale — documented in the contract as a separate extension.

Surfaced (tracked separately, not in this PR)

While building the falsifier, two orthogonal GPU-training defects re-surfaced on sm_89 (both → CUDA_ERROR_ILLEGAL_ADDRESS): (A) the seq<32 batched-softmax partial-warp shfl.sync UB (a #2252 follow-up), and (B) the fused LoRA grad-clip PTX kernels (gradient_clip_gpu_scale/clip_scale_reduce/squared_sum_reduce) failing to JIT. Next thread: verify via the real apr finetune -m qlora CLI whether these degrade real training on current main.

🤖 Generated with Claude Code

…ters (FALSIFY-CUDA-EVAL-ADAPTER-SYNC-001)

On the CUDA path, `train_step` writes LoRA adapter deltas into the GPU-resident
`cuda_blocks`, but `InstructPipeline::evaluate()` computes val_loss on the CPU via
`model.forward_with_lora(x, &self.lora_layers)`. The CPU `lora_layers` are only
refreshed by `sync_lora_to_cpu()`, which was invoked exclusively inside
`save_checkpoint` — never before `evaluate()` in the epoch loop
(`instruct_trainer.rs:243`).

Consequence: per-epoch `val_loss` is byte-identical across every epoch and run,
so `best_val_loss` collapses to the epoch-0 constant, `best_epoch` freezes at 0
(the `best/` checkpoint is stale-by-N-epochs), and early stopping fires on a
phantom plateau. Trained weights themselves were fine — only the validation
signal (and the decisions it drives) were dead.

Fix: `evaluate()` calls `sync_lora_to_cpu()` before the CPU forward (now takes
`&mut self`), so val_loss reflects the current GPU-trained adapters. Adds a
`#[cfg(not(feature = "cuda"))]` no-op twin so the call is unconditional; on the
CPU/WGPU paths `lora_layers` are updated in place and already current. This makes
`evaluate` self-consistent for every caller, not just the trainer.

Falsifier FALSIFY-CUDA-EVAL-ADAPTER-SYNC-001 (contract finetune-eval-adapter-sync-v1.yaml):
injects a GPU-only adapter change via download→(set B nonzero)→upload — independent
of the optimizer/clip path — then asserts the next evaluate() differs. Verified live
on RTX 4090 (sm_89):
  RED  (sync removed): val_before == val_after == 14.25047874 (|Δ|=0.0)
  GREEN (sync present): 14.25047874 -> 14.22203255 (|Δ|=0.02844620)

GPU-gated (`--ignored`, needs APR_PARITY_MODEL); not yet CI-enforced pending a
CUDA runner. CPU suite: 7612 pass (3 pre-existing prune insta-snapshot failures,
unrelated module). Q/V-adapter scope bound documented in the contract.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@noahgift
noahgift enabled auto-merge July 2, 2026 16:42
@noahgift
noahgift added this pull request to the merge queue Jul 2, 2026
…iff-scoped mutation gap

The standalone qwen2_1_5b_config() helper in the (cuda+ignore-gated) eval-sync
falsifier was the only surviving diff-scoped mutant: cargo-mutants replaces it
with Default::default(), but its sole caller is the GPU-gated #[ignore] test that
CI cannot run, so nothing kills it. Inlining the config into the #[test] body
removes the mutable helper (cargo-mutants skips #[test] fns). The only remaining
diff mutant (evaluate -> Default::default()) is unviable — InstructBatchResult
does not derive Default — so 0 missed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@noahgift
noahgift removed this pull request from the merge queue due to a manual request Jul 2, 2026
@noahgift
noahgift enabled auto-merge July 2, 2026 17:28
@noahgift
noahgift added this pull request to the merge queue Jul 2, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 2, 2026
@noahgift
noahgift added this pull request to the merge queue Jul 2, 2026
Merged via the queue into main with commit 093641a Jul 2, 2026
10 checks passed
@noahgift
noahgift deleted the fix/qlora-eval-adapter-sync-20260702 branch July 2, 2026 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant