Repository navigation
Conversation
MakazhanAlpamys
left a comment
There was a problem hiding this comment.
Advisory review of the draft at 632c3fd8. Keep it in draft. Route 1 from #840 is the right route, and leaving the schema gate, docs and fragment until the grid exists is right too. But the instrument under-reads the real step, and that is the direction #840's criterion 2 forbids.
What holds:
- The probe builds the real loss, not an imitation. Forward hooks show the same forwards and shapes as the real step:
- DPO: a policy pass with grad plus the
refadapter without grad, at[2, seq]; - KTO: KL, policy, ref and ref-KL;
- ORPO/SimPO: one concatenated forward.
- DPO: a policy pass with grad plus the
- Dropping the rejected half of the batch is caught.
- Nothing new is reachable by a user: the schema still refuses the probe for
task != sft, so no preference plan is ever built.
Measured on an RTX 5070 8 GB (sm_120), torch 2.14.0+cu130, transformers 5.17.0, trl 0.29.1, peft 0.20.0, accelerate 1.15.0. The model is a tiny Llama (vocab 32000, hidden 256, 2 layers), streamed in bf16 with LoRA r4 on q/v, fed rows that fill max_length. I compared the probe's peak with the real first training_step peak, both max_memory_allocated, with no other GPU work on the card. One toy model and two shapes, so these are a new baseline, not comparable with the old 3050 record.
1. Blocking, the core defect: the probe does not run the forward the Trainer will use. It calls trainer.compute_loss(...) from setup(), before _inner_training_loop runs accelerator.prepare(self.model). Under bf16 native AMP, prepare replaces model.forward with autocast plus convert_outputs_to_fp32 (accelerate/accelerator.py:1824-1835). So the probe sees bf16 logits with autocast off, while the real step has fp32 logits with autocast on. Probe peak / real-step peak:
| loss | seq 512 | seq 2048 |
|---|---|---|
| DPO | 0.664 | 0.666 |
| ORPO | 0.405 | 0.372 |
| SimPO | 0.404 | 0.372 |
| KTO (batch 2) | 0.523 | 0.508 |
- Control: the existing SFT causal-LM probe on the same box and model reads 0.997 / 1.112.
- Attribution: your probe with
model.forwardwrapped exactly as accelerate wraps it reads 0.94-0.99. The wrapped DPO probe reads 646.3 MB, the same as the real step's 646.3 MB, so the gap is entirely the missing wrapper. - Fix: run the probe through the forward the Trainer will use, then restore it, and pin it with a CUDA test asserting that the probe and the real step see the same logits dtype and autocast state. The logits term dominates here, and it is larger still on 128k-152k vocabularies.
2. Blocking: the stretch truncates, and the truncation hides real overruns. _resize_probe_tensor cuts batches longer than max_length. But on trl 0.29.1, Soup's SimPO and KTO wrappers never call enforce_preference_sequence_limit (DPO and ORPO call it 3 times each), and TRL's own truncation lets rows run past max_length:
- CPO truncates the response to
max_length - longer_response_length, which overruns whenever the prompt is longer than the longer response; - KTO slices with a negative bound once the prompt alone reaches
max_length.
Real steps against the probe:
| loss | max_length | real step | probe | probe / real |
|---|---|---|---|---|
| SimPO | 512 | [2, 1025] |
[2, 512] |
0.218 |
| SimPO | 2048 | [2, 4097] |
[2, 2048] |
0.190 |
| KTO | 512 | [2, 1024] |
[2, 512] |
0.282 |
| KTO | 2048 | [2, 4096] |
[2, 2048] |
0.259 |
The truncated SimPO batch had no completion tokens left, so its log-probs were NaN, and the instrument accepted a NaN loss as a valid measurement. Never truncate: either measure the batch as TRL produced it, or refuse and name the overrun. Treat a non-finite loss as failed. The overrun itself predates this PR; I am filing it separately.
3. Blocking: the probe writes into the trainer state the run then reports. All four compute_loss implementations store metrics.
- A one-step DPO run logs
num_tokens 2048with the probe and1024without it, and that counter is cumulative, so the error is permanent. - ORPO/SimPO's first logged rewards are the mean of the probe batch and the real step.
Snapshot and restore the metric stores and the token counter around the probe. Also observed, not claimed as harm: the CPU RNG advances across the probe, which runs after the Trainer's seeding point.
4. Blocking: the deferred gate fails open. With the probe on, a predicted miss up to 4x is demoted from refusal to advice. The plan is set in one place and consumed in another, and nothing checks that it was consumed. So a wrapper without the hook, or with the hook in a branch that does not run, trains with no gate at all: disabling the DPO hook, or moving it before materialize_meta_adapter_copy, leaves every test green. Refuse in train() while _pending_stream_vram_probe is still set, and replace the source-grep test with a behavioural one through the real setup(); the tests/test_v07204.py harness can do that.
5. Blocking before this leaves draft: the new instrument has no direct tests. The one hook test monkeypatches it out. These mutations all survive:
- no backward;
- peak read from
max_memory_reserved; - dropping
_zero_probe_grads; - a zero-filled attention-mask extension;
- not closing the stream runtime when building the batch raises;
- routing preference back to the causal-LM step.
Mirror the real-device class in test_issue349_measured_fit.py:435-535, and add a dispatch test.
Non-blocking:
- The plan's
rowsis only a label: the measured row count is whatever the first dataloader batch holds, which is fewer thanbatch_sizeon a small dataset. Assert the batch's leading dimension. - Every probe run leaves ~33.5 MB resident: the autograd thread's cuBLAS workspace, freed by
torch._C._cuda_clearCublasWorkspaces(). It predates this PR, since the SFT probe does the same, but the grid should account for it. - Nits: the SFT messages now read "a sft streaming step at 1 model rows". The deleted comments on the three-outcome policy (None / failed / oom) were its only documentation.
Before this leaves draft: fix 1-5, then run #840's grid on a named card, publish it under benchmarks/, and widen the gate one loss at a time, together with the docs and the fragment.
632c3fd to
bb5b559
Compare
|
Implemented review blockers 1–5 at The probe now uses Accelerate’s training-time AMP/output wrapper and restores it; refuses overlength and partial batches; rejects non-finite loss; restores metrics, cumulative tokens, RNG and model state; and all four trainers fail closed if setup leaves a pending plan. Direct instrument and real-setup regressions were added. Local focused result: 111 passed, 35 skipped, Ruff clean, diff-check clean. The PR remains draft because the named-CUDA acceptance grid required by #840 cannot be executed or claimed on this M4 Max. |
|
@Amix29 thanks, this is a solid round. Blockers 1-5 from my review of What holds, by execution
Tests I ran
Small things to fold in before the grid run. All of them can be tested on CPU.
What has to happen before a full review
All 13 required checks pass on |
bb5b559 to
0a3e647
Compare
|
Folded in all six CPU-review follow-ups at
Local M4 Max result on the six review files: 116 passed, 34 skipped. Ruff (including preview E30) and Keeping the PR in draft: I have not run or claimed #840's named-CUDA acceptance grid, and the requested #1196-before-grid ordering still applies. |
|
@Amix29 thank you. This round closes all six CPU follow-ups from my review of
Everything below ran at Tests I ran
Mutations: 28 against those six files, 24 killed
Three small gaps, none blocking. Four mutations survive, and three of them are restore paths that nothing pins yet:
These three tests kill all three mutations. On your head they pass, 3/3: from types import SimpleNamespace
import pytest
torch = pytest.importorskip("torch")
def test_an_attribute_the_probe_creates_is_removed_again(monkeypatch):
from soup_cli.trainer.stream_setup import StreamingSetupMixin, _ProbePlan
from soup_cli.utils.layer_stream_runtime import StepPeak
class Runtime:
def close(self):
pass
class Trainer:
def __init__(self):
self._metrics = {"train": {"loss": [1.0]}}
def get_train_dataloader(self):
return [{"input_ids": torch.tensor([[1, 2, 3], [1, 4, 5]]),
"attention_mask": torch.ones((2, 3), dtype=torch.long)}]
def compute_loss(self, model, batch):
self._stored_metrics = {"train": {"rewards": [9.0]}} # created by the loss
return model.weight.sum()
class Wrapper(StreamingSetupMixin):
pass
wrapper = Wrapper()
wrapper.model = torch.nn.Linear(4, 4, bias=False)
wrapper.trainer = Trainer()
wrapper.device = "cuda"
wrapper._stream_runtime = Runtime()
wrapper._pending_stream_vram_probe = _ProbePlan(
task="dpo", batch_size=1, rows=2, seq_len=8, vocab_size=64,
predicted_bytes=100, available_bytes=1_000,
)
def fake_measure(model, *, step, rows, seq_len, device):
step()
return StepPeak(peak_bytes=200, reserved_bytes=220, seconds=0.01,
rows=rows, seq_len=seq_len)
monkeypatch.setattr(
"soup_cli.utils.layer_stream_runtime.measure_loss_step_peak_bytes", fake_measure
)
before = set(vars(wrapper.trainer))
wrapper._run_pending_stream_vram_probe()
assert "_stored_metrics" not in vars(wrapper.trainer)
assert set(vars(wrapper.trainer)) == before
def _autocast_cpu(monkeypatch):
monkeypatch.setattr(
"accelerate.utils.modeling.get_mixed_precision_context_manager",
lambda *_args, **_kwargs: torch.autocast("cpu", dtype=torch.bfloat16),
)
return SimpleNamespace(accelerator=SimpleNamespace(native_amp=True, autocast_handler=None))
def test_an_instance_forward_is_put_back(monkeypatch):
from soup_cli.trainer.stream_setup import _trainer_forward_for_probe
trainer = _autocast_cpu(monkeypatch)
model = torch.nn.Linear(8, 8)
def instance_forward(x):
return torch.nn.Linear.forward(model, x)
model.forward = instance_forward
with _trainer_forward_for_probe(trainer, model):
assert model(torch.randn(2, 8)).dtype == torch.float32
assert vars(model)["forward"] is not instance_forward
assert vars(model)["forward"] is instance_forward
def test_the_forward_is_put_back_when_the_step_raises(monkeypatch):
from soup_cli.trainer.stream_setup import _trainer_forward_for_probe
trainer = _autocast_cpu(monkeypatch)
model = torch.nn.Linear(8, 8)
with pytest.raises(RuntimeError, match="step failed"):
with _trainer_forward_for_probe(trainer, model):
assert "forward" in vars(model)
raise RuntimeError("step failed")
assert "forward" not in vars(model)The fourth survivor is cosmetic: For later, not for now: when the gate widens, the refusal at What remains before this leaves draft. None of it is yours to do until the grid exists.
If you fold the three tests in before step 2, the card only has to run the final head once. |
0a3e647 to
3a62c0e
Compare
|
Folded in all three suggested restore-path tests plus the cosmetic label-tail pin at
Six-file local result: 120 passed, 34 skipped on the M4 Max. Ruff, preview E30 and |
|
@Amix29 thank you.
Your label test does one more thing than I asked for. It also checks that the original columns keep their I also checked the rebase. Everything below ran at Tests I ran
Mutations: 29 against your six files, 29 killed. That covers the six items from my review of
Nothing survives. On CI. The required checks on What remains before this leaves draft. Nothing from you until the grid exists, and the order has not changed:
|
|
@Amix29, G1 is done on the card. RTX 5070, on G2, the #840 probe-vs-real-step grid and its gate record, is still to come. It needs the harness, which is not in the tree yet, and it waits for #1196 as agreed, so nothing changes on your side for now. When the harness is ready, the grid runs on the same card. |
|
@Amix29, a follow-up on G1. I re-ran it today on the RTX 5070, on
I also ran the same 19 with your source changes reverted to That is expected, and it is not a gap in your tests. The 19 GPU tests predate this PR:
So G1 shows that your change breaks nothing in a real streamed DPO, ORPO, SimPO or KTO step on CUDA. It does not show G2 is unchanged. The #840 grid waits for #1196, which is still in review, and for the harness. Nothing is needed from you until then. One request that would make the GPU run count for this PR: a |
|
Added the requested real-device preference-probe test at The two new Local M4 Max validation: the six review files give 135 passed, 21 expected skips; the final focused pair gives 87 passed, 16 expected skips. Full Ruff and |
|
@Amix29 thank you for turning the GPU request around the same day. The test drives the real thing in the right order: a wrapper built through the real What I ran. RTX 5070 Laptop (sm_120), torch 2.14.0+cu130, trl 0.29.1, peft 0.20.0, accelerate 1.15.0, on
Why. On this card, cuBLAS holds a 32 MiB workspace per handle, the allocator counts it, and a training step uses two handles: the forward thread's and the autograd thread's. The fitted formula has no term for that. For a model this small, its prediction is almost entirely
So the step itself costs about 0.6 MB, and the instrument reads it correctly. The assertion cannot hold at this shape on this card, and "within the fitted bound" was my wording, so that part is on me. A replacement I ran on the card. It keeps your structure and your per-call assertions, adds one warm-up probe so the workspaces are already allocated, and then takes the resident allocation as the baseline. It measures at seq 32 and at seq 64, and it bounds the growth above the resident set by the fitted prediction: @pytest.mark.gpu(reason="the preference-loss peak instrument needs CUDA")
class TestPreferenceLossProbeOnRealHardware:
"""Drive #840's new instrument through real TRL DPO and KTO losses on CUDA.
The first backward in a process creates the autograd thread's cuBLAS
workspace (32 MiB per handle on this card) and it stays allocated, so a
first measurement also counts memory the step does not own. One warm-up
probe makes every later measurement start from the same resident set.
"""
@pytest.mark.parametrize("task", ("dpo", "kto"))
def test_real_loss_peak_moves_with_the_shape_and_fits_the_fitted_bound(
self, tmp_path, monkeypatch, task
):
import gc
import torch
from soup_cli.trainer.stream_setup import (
_preference_probe_rows,
_stretch_preference_probe_batch,
_trainer_forward_for_probe,
)
from soup_cli.utils import layer_stream as layer_stream_module
from soup_cli.utils.layer_stream_runtime import measure_loss_step_peak_bytes
predicted = []
real_estimate = layer_stream_module.estimate_stream_peak_vram
def capture_estimate(**kwargs):
value = real_estimate(**kwargs)
predicted.append(value)
return value
monkeypatch.setattr(layer_stream_module, "estimate_stream_peak_vram", capture_estimate)
wrapper = trainer = model = raw = None
try:
wrapper, _, _ = _build_streamed_wrapper(tmp_path, monkeypatch, task=task, n_layers=2)
assert len(predicted) == 1
trainer = wrapper.trainer
model = wrapper.model
raw = next(iter(trainer.get_train_dataloader()))
def probe(seq_len):
batch = _stretch_preference_probe_batch(raw, seq_len=seq_len)
rows = _preference_probe_rows(batch)
batch = _batch_on(model, batch)
model.train()
with _trainer_forward_for_probe(trainer, model):
peak = measure_loss_step_peak_bytes(
model,
step=lambda: trainer.compute_loss(model, batch),
rows=rows,
seq_len=seq_len,
device="cuda",
)
assert peak is not None and not peak.oom and not peak.failed
assert peak.rows == rows and peak.seq_len == seq_len and peak.seconds > 0
assert all(parameter.grad is None for parameter in model.parameters())
return peak
probe(64) # warm-up: creates the per-thread cuBLAS workspaces
torch.cuda.synchronize()
resident = torch.cuda.memory_allocated()
short = probe(32)
full = probe(64)
assert resident < short.peak_bytes < full.peak_bytes, (
f"{task}: resident {resident}, seq 32 {short.peak_bytes}, "
f"seq 64 {full.peak_bytes}"
)
assert full.peak_bytes - resident <= predicted[0], (
f"{task}: the step added {full.peak_bytes - resident} bytes above "
f"the resident {resident}, against a fitted bound of {predicted[0]}"
)
finally:
if wrapper is not None:
wrapper._close_stream_runtime()
raw = model = trainer = wrapper = None
gc.collect()
torch.cuda.empty_cache()It replaces your class in
For comparison, your current class fails under every one of those mutations, but only because it already fails at the head. So on this card it cannot tell a working instrument from a broken one yet. One small CPU gap, optional. Deleting the def test_loss_instrument_resets_the_peak_counter_before_the_step(monkeypatch):
import torch
from soup_cli.utils.layer_stream_runtime import measure_loss_step_peak_bytes
events = []
monkeypatch.setattr(torch.cuda, "is_available", lambda: True)
monkeypatch.setattr(torch.cuda, "synchronize", lambda *args: None)
monkeypatch.setattr(
torch.cuda, "reset_peak_memory_stats", lambda *args: events.append("reset")
)
monkeypatch.setattr(torch.cuda, "empty_cache", lambda: None)
monkeypatch.setattr(torch.cuda, "max_memory_allocated", lambda *args: 123)
monkeypatch.setattr(torch.cuda, "max_memory_reserved", lambda *args: 456)
model = torch.nn.Linear(4, 1)
x = torch.randn(3, 4)
def step():
events.append("step")
return model(x).square().mean()
peak = measure_loss_step_peak_bytes(model, step=step, rows=3, seq_len=1, device="cuda")
assert peak is not None and not peak.failed
assert events == ["reset", "step"]CI. The required checks on What remains before this leaves draft.
|
|
Replaced the two failing CUDA assertions with the resident-baseline protocol you validated, at head The DPO/KTO rows now warm up once to create the persistent cuBLAS workspaces, measure the resident allocation, require peak growth from seq 32 to seq 64, and compare only I also added the optional CPU contract that proves Local M4 Max validation across the six review files: 136 passed, 21 expected skips. Full Ruff and |
|
@Amix29, thank you for turning the GPU test around the same day, and for using the resident-baseline version as written. What I checked at
What has to happen before it leaves draft. Nothing from you until these exist:
Thank you for how carefully this has been iterated: every round has closed what was asked with a test that goes red when the code is broken. |
dfc4531 to
4d5ce35
Compare
|
Rebased this PR onto current New head: Local validation:
No CUDA result is claimed; this remains the CPU/MPS/MLX-side implementation and regression coverage. |
|
@Amix29, thank you for the rebase. You resolved the What I checked at
What still has to happen before this leaves draft. None of it is owed by you today.
You don't need to rebase again before #1196 lands. Thank you for keeping every round this careful. |
Refs #840
What
This remains a draft: the implementation and review fixes are present, while the issue-required CUDA acceptance grid remains deliberately unclaimed.
trainer.compute_lossthrough the same temporary Accelerate native-AMP/FP32-output forward wrapper that the real training loop prepares, then restores the original dispatch;train()methods if a deferred probe was not consumed;Real-device regression added after G1
Two
gpurows now build real tiny streamed Llama trainers for DPO and KTO, use their actual TRL batches, and drivemeasure_loss_step_peak_bytesthrough the production AMP wrapper. A warm-up creates the persistent per-thread cuBLAS workspaces; the test then measures seq 32 and seq 64 from the same resident baseline, requires peak growth with shape, and bounds only the step's incremental allocation by the fitted prediction. It also pins correct shape metadata, positive duration and gradient cleanup.The CPU contract now verifies that
reset_peak_memory_statsruns before the measured loss step, so CI catches a stale high-water mark even without CUDA.Deliberate non-change
The schema still refuses
training.stream_vram_probeon preference tasks. #840 requires the CUDA grid before enabling losses one by one; this M4 Max cannot supply that evidence.Validation on Apple M4 Max
git diff --checkclean.Hardware follow-up required before ready-for-review
max_length >= 4096, KTO batch >= 2, recording probe peak, real-step peak, prediction and cost;No CUDA result is claimed from this Mac.