Repository navigation
fix(layer-stream): prefetch embed_tokens at the backward tail, not the next step's prime (#975) - #1178
Conversation
…e next step's prime (MakazhanAlpamys#975) LargeLayerBufferPool holds embed_tokens and an untied lm_head in one shared slot. _prime() reloaded embed_tokens right as the next step's forward started, paying a full head-sized H2D copy with nothing to overlap it against (~33ms on the record in MakazhanAlpamys#975 for a synthetic 70B-shaped fixture). I made StreamPrefetcher also fire that same load at the tail of the backward pass, once the checkpoint recompute for decoder layer 0 confirms lm_head's own backward Node has already been dispatched (the autograd engine reaches it first, since it sits between the loss and every decoder layer) — so refilling the slot here cannot invalidate a saved-tensor version check that already ran. _prime() keeps its unconditional load as a fallback for step 0 and any forward that never reaches layer 0's backward (eval); on the hot path it is now a same-owner no-op. I verified this on CPU: the existing streaming suite (486 tests) is unaffected, and I added a new test file confirming the backward-tail fires, the slot already holds embed_tokens by the time prime() asks for it from the second step on, and 2 training steps complete without an autograd version error (the same failure class MakazhanAlpamys#1049 hits for a different reason). I also confirmed the DPO/KTO untied-preference version error from MakazhanAlpamys#1049 reproduces identically with and without this change, so it is pre-existing and not something I introduced here. I have not verified whether this actually reduces the measured stall from MakazhanAlpamys#975. That needs the wait() bracket in layer0_wait.py run on hardware that can load the cold-70B fixture (an RTX 5070 or similar); I built and tested this on a 4GB card that cannot.
9fc8ea5 to
3dcd7e7
Compare
…tching layer streaming's own CPU-only convention I pinned device="cpu" in _build_untied_wrapper for exact float32 arithmetic, but on a macOS CI runner where MPS is the accelerator that produces a device mismatch (RuntimeError: Passed CPU tensor to MPS op) in every test that actually runs a training step, unrelated to the fix itself. test_v07204.py already guards every CPU-pinned streaming test with skipif(_mps_is_the_accelerator()), which I missed when I wrote this file — I'm applying the same guard here at the class level, since all four tests build through the same CPU-pinned wrapper. Confirmed against the failing CI run: the one test that doesn't call train() (test_untied_fixture_actually_shares_one_slot) passed on macOS; the three that do all failed with the same MPS/CPU mismatch, which is exactly what this guard is meant to skip.
MakazhanAlpamys
left a comment
There was a problem hiding this comment.
Advisory review of the draft at 5223a0de.
Thank you for turning (b) into code so quickly. I ran the measurement you could not, and the result is good news for (b). Everything below was run on the RTX 5070 Laptop GPU (8 GB, driver 616.92, Windows 11, Python 3.12.10, torch 2.14.0+cu130, bitsandbytes 0.50.2, transformers 5.17.0, peft 0.20.0, trl 0.29.1), on your head merged with current main (it merges cleanly, and cleanly with #1061's head too).
What holds
Correctness. I ran real trainer.train() steps (4 per arm, identical LoRA init loaded into every arm, dropout 0) and compared the loss tensor of every step, every LoRA gradient at on_pre_optimizer_step, and the final adapter weights with torch.equal:
- CPU fp32: untied SFT on this PR is bit-identical to
mainand to a resident run (48 of 48 gradient tensors). The same holds for untied SFT atgradient_accumulation_steps: 2, for untied ORPO and SimPO (also equal to resident), and for tied SFT and tied DPO. Tied checkpoints have no large pool, so the new callback isNonethere. - Untied DPO and KTO crash the same way on
mainand on this PR: #1049, in step 1's backward, before your code runs. You were right about that. With #1061 merged in, untied DPO and KTO on #1061 + this PR are bit-identical to #1061 alone and to resident. - CUDA bf16: untied SFT, two runs of
mainand two of this PR, all four bit-identical. Untied DPO on #1061 + this PR is bit-identical to #1061 alone, and tied SFT is unchanged.
Scheduling. I traced every large-layer load. The prefetch fires exactly once per backward. From step 2 on, _prime's load is a same-owner no-op. With gradient_accumulation_steps: 2 it fires at the tail of every micro-batch's backward, which is right, because every micro-batch's forward starts with the lookup. A no-grad forward never fires it, and an eval forward after training finds the embedding already in the slot.
Tests vs mutations (CPU, as CI runs them): reverting the wiring at layer_stream_runtime.py:2520 fails 2 of your 4 tests. So do deleting the call at :1055 and dropping the direction check at :1050. The last one is worth knowing about: the prefetch then fires in the forward, and the flag silently blocks the backward one. Removing the flag reset in prime() fails 1. Firing the refill before the head's backward (a grad hook on the head output) fails 3, with the modified by an inplace operation error, so test_two_steps... does its job. Calling the prefetch twice, or dropping the once-per-pass flag, survives. Both are equivalent mutants, because load_async's owner check makes a repeat a no-op. Against main your file gives 2 failed / 2 passed.
The measurement #1174 asked for (arms a and b). The cold synthetic-70B fixture from #975 does not fit what I could run here, so I built a smaller one with the committed synth_checkpoint.py: untied Llama, hidden 2048, 8 layers, vocab 128256. That makes the shared slot 501.0 MiB bf16. Decoder NF4, seq 512, batch 1, LoRA r8 on q/k/v/o, PagedAdamW8bit. I used the committed stream_probe.Instruments with layer0_wait's labelling, so these are the same wait() brackets as gate-971 §8.
Both arms ran in one process. Arm A is your tree with backward_tail_prefetch set to None, which is exactly the old prefetcher, and a run on unmodified main confirms that. Arm B is your change. The schedule: 3 warm-up steps discarded, then five interleaved A/B blocks, each with 1 transition step discarded and 2 measured. That gives 10 measured steps per arm.
| configuration | embed wait() median, A → B |
step median, A → B |
|---|---|---|
| disk tier (AsyncDiskSource, read_ahead 2), sync every step | 22.22 → 0.15 ms | 306.6 → 291.6 ms |
| same, repeated | 22.22 → 0.16 ms | 302.8 → 296.9 ms |
| disk tier, no per-step sync | 22.67 → 0.19 ms | 316.9 → 296.3 ms |
| RAM tier (pinned), sync every step | 18.57 → 0.13 ms | 257.7 → 242.4 ms |
| RAM tier, no per-step sync | 18.72 → 0.12 ms | 252.1 → 242.3 ms |
control: unmodified main, both labels |
22.31 / 22.61 ms | 310.6 / 316.7 ms |
The embedding's wait() goes to essentially zero in every configuration. On the disk tier the step's total wait drops from 38.08 to 15.33 ms. Step time falls by 5.9–20.6 ms (2–6.5%) in all five runs. Identical code differs by about 6 ms between labels, though, so the step-time gain is real but close to the noise floor on this fixture. The head's forward-tail wait (10.6–14.6 ms) and the decoder waits (~1.0 ms) are unchanged, as expected. I did not measure (c).
Blocking
-
Three of the four new tests fail on any machine with a CUDA GPU.
_build_untied_wrapperpinsdevice="cpu"(test file line 45) and then callstrainer.train().TrainingArgumentsstill picks CUDA, so the batches land oncuda:0and the lookup fails withExpected all tensors to be on the same device, but got index is on cuda:0, different from other tensors on cpu. On the 5070 the file gives 3 failed / 1 passed. CI is green only because its runners have no GPU. None of these tests compares floats, so the line-55 reason for pinning the device does not apply. Fix:device="cuda" if cuda_available() else "cpu", which is what_build_streamed_wrapperand #1061's train-step test do. With that one change, all 4 pass on the 5070, and reverting the wiring still fails 2 of them there. -
The safety comment names the wrong guard, and the guard that actually protects the new refill point is untested. The comment at
:1037-1048rests on the version counter: "dispatch, not GPU completion, is what the saved-tensor version counter checks against". That is true of the CPU-side check. It says nothing about the GPU reading the slot. When the CPU reaches layer 0, the head's backward GEMM can still be queued on the compute stream. What orders the refill after it isstream.wait_stream(torch.cuda.current_stream())inLargeLayerBufferPool.load_async(:951). When layer 0 is reloaded during the backward, the same call at:860is also queued earlier on the same copy stream.I checked this by execution. The setup: an untied 2-layer fixture with 2 buffers, so layer 0 is not reloaded in the backward; a
torch.cuda._sleep(2e8)enqueued from a grad hook on the head's output, so the compute stream lags the CPU inside the backward (the sleep was still running when your prefetch fired, in steps 2–4); 4 real trainer steps; bf16. The results:- this PR with the lag is bit-identical to
main mainwith the lag and:951's wait removed is still bit-identical, because onmainnothing refills the slot inside the backward- this PR with the lag and
:951's wait removed diverges from step 2 on: layer-0v_projlora_Amax |Δ| 0.068,q_projlora_B1.3e-3, and the losses follow
So after this PR,
:951is the only thing standing between the new refill point and silently wrong gradients on a GPU. Please:- rewrite the comment to say that.
- add a CUDA-only test (
skipifwithout CUDA) built on that setup. It should compare against the same run withbackward_tail_prefetch=None, and it must fail when:951's wait is removed. It will only run on machines with a GPU, but it is the one test that pins the hazard this change introduces.
- this PR with the lag is bit-identical to
-
The load-count test asserts the right number for the wrong reason. Lines 137–139 say "5, not 6: one fewer than two full steps … would cost". Line 126 says "without the backward-tail prefetch turning into an extra, unplanned load". On
main, the same 2 steps make 4 real large-layer loads, not 6: the test fails there withassert 4 == 5. Both trees make 2 loads per step in steady state. This PR adds exactly one pertrain(), the last backward's prefetch, which nothing uses unless a forward follows. I measured 8 → 9 at 4 steps, and 16 → 17 at GA 2 and for DPO. Please rewrite the comment to say main = 4, PR = 5, and why. -
Before this leaves draft:
- add a changelog fragment,
changelog.d/<current baseline>/1178.changed.md, crediting(#975 by @VanshSharmaPES in #1178). - add one sentence to
docs/performance-and-quantization.md:354, which still says the embedding "is loaded for the model input". It is now reloaded at the tail of the previous step's backward. - state the measured magnitude at the top of the description, as #975's acceptance criteria ask: about 19–23 ms of embedding wait removed per step on this fixture, still about 0.1% of a cold 70B step.
- add a changelog fragment,
Non-blocking
- Preference losses get part of the benefit. The prefetch fires once per backward. DPO's no-grad reference forward still pays a real embedding load at
_prime, and so do KTO's three extra forwards. That is 1 of 2 embedding waits removed per DPO step, and 1 of 4 per KTO step. Worth one line in the description. - What is left for (c). With (b) in, the remaining large-layer wait is the head's forward-tail load, 10.6–14.6 ms per step here. That is now what (c)'s second slot would buy, for 501 MiB here and about 1 GiB on an 8B-class head. The embedding's bytes in the slot are dead once the lookup has run, so the head load could also move earlier in the forward, at zero VRAM. I would measure that before (c). This belongs on #1174, not in this PR.
- The last step's copy is the only load that nothing ever waits on. It is harmless while the slot outlives it. If you want every load to be consumed again, synchronize the copy stream in
StreamRuntime.close(). backward_tail_prefetchedis redundant. Layer 0 is recomputed once per backward. Keeping it as documentation is fine._prime's comment. "a frozen embedding with no LoRA reaching it" would be clearer as "a backward that never reaches layer 0". That is the actual condition under which the fallback is the only load.
Nits
- Line 101 calls
large_pool.load_async(embed_key, runtime.source, None). That re-tests the pool's existing owner check, not_prime. The load-count test already covers_prime. - Once #1061 lands,
_build_streamed_wrapper(..., tie=False)can replace the local helper. The two PRs merge cleanly, and together they are bit-exact for DPO and KTO. - For whoever re-runs
layer0_wait.pyfor #975: after this change, the embedding's copy bracket is recorded in the previous step'scopies, and the last step carries one extra. Comparestallswith gate-971 §8, not the copy lists.
…celerator, not a hardcoded CPU I pinned device="cpu" in _build_untied_wrapper, but on a machine with a CUDA GPU TrainingArguments still picks CUDA, so the batches land on cuda:0 and the embedding lookup fails on a device mismatch. Three of the four tests failed that way on an RTX 5070; CI only passed because its runners have no GPU. None of these tests compares floats, so the exact CPU arithmetic I was pinning for buys nothing here. I now pick "cuda" if cuda_available() else "cpu", the same way _build_streamed_wrapper does. The MPS skip stays, since without CUDA the helper still falls back to the CPU, and I reworded its comment to say that instead of the float-pinning reason.
…refill after the head's backward My comment in StreamPrefetcher.advance justified the backward-tail refill with the saved-tensor version counter. That is true of the CPU-side check, but it says nothing about the GPU: when the CPU reaches layer 0 the head's backward GEMM can still be queued on the compute stream, reading the shared slot. What orders the refill after it is stream.wait_stream(torch.cuda.current_stream()) in LargeLayerBufferPool.load_async. The comment now names both guards and says the second is the one protecting the GPU, and that removing it gives silently wrong gradients from the second step on. Comment only, no behaviour change.
|
Thanks for the thorough review and for running the measurement I couldn't. I've pushed two commits so far and am working through the rest.
Still open: the CUDA-only test that fails when that wait is removed (I'll write it from your setup, but I can't run it here, so I'll say so in the PR), the load-count comment ( |
MakazhanAlpamys
left a comment
There was a problem hiding this comment.
Advisory review of the draft at 2be74739.
Thank you for turning both pushes around quickly. I re-ran everything on your head merged with main at dd9a09fd, on the same RTX 5070 as last time, and with CUDA hidden, which is what CI sees. While I was reviewing, #1031 landed in layer_stream_runtime.py. Your branch merges cleanly with it, so I repeated the test file, the mutations and the streaming suite on 1c053a8c, and everything below still holds.
Closed
Item 1: the tests on a CUDA machine. With the 5070 visible your file now gives 4 passed (41 s), and 4 passed with CUDA hidden. The mutation results are the same on both:
- reverting the wiring at
:2530fails 2 tests; - deleting the call at
:1065fails 2; - dropping the direction check fails 2;
- removing the flag reset in
prime()fails 1; - firing the refill before the head's backward fails 3.
Against main the file gives 2 failed / 2 passed. The 31 streaming test files give 1253 passed, 13 skipped and 0 failed on the 5070, and 1193 passed, 73 skipped and 0 failed with CUDA hidden.
Item 2, the comment half. The new comment names the right guard, and I re-checked what it says by execution rather than by reading. The setup was the 2-layer untied fixture with 2 buffers, bf16, and a torch.cuda._sleep(1e9) enqueued from a grad hook on the head's output. The sleep makes the compute stream lag the host inside the backward; at steps 2–4 it was still running when your prefetch fired. Over 4 real trainer steps:
- this PR is bit-identical to
mainin two runs: 32 of 32 LoRA gradients, every loss, and the final adapter; - with the
wait_streamat:951removed, both runs diverge at step 2: all 8 gradients, worst layer-0v_projlora_B|Δ| 0.40, and the losses follow from step 3; mainwith the same wait removed stays bit-identical, because nothing refills the slot inside its backward.
So the comment is right that :951 is the only thing protecting the GPU.
Blocking (before this leaves draft)
-
The CUDA-only test for
:951is still missing, and the recipe I gave you last time would be a flaky test. With the wait removed, all 4 tests pass on the 5070 as well as on CPU, so nothing detects that removal on any machine.The flakiness comes from host load. I first ran the lag recipe with a smaller sleep while the host was busy with another test run. In 2 of 3 lag runs the sleep had already finished at every prefetch, so the race never opened, and "wait removed" came out bit-identical to your PR. A test built that way would have passed with the guard deleted.
A pool-level test does not depend on how fast the host is. I checked this one on the 5070: 5 of 5 pass with the guard and 5 of 5 fail with it removed, about a second each, and it held while the host was loaded:
class _PinnedSource: def __init__(self, tensors): self.tensors = tensors def get(self, idx, key): return self.tensors[key] @pytest.mark.gpu def test_refill_waits_for_a_read_still_queued_on_the_compute_stream(): import torch from soup_cli.utils.layer_stream_runtime import LargeLayerBufferPool shape = (1024, 1024) pool = LargeLayerBufferPool( {"lm_head": (shape, "float32"), "embed_tokens": (shape, "float32")}, {"lm_head": 0, "embed_tokens": 1}, device="cuda", ) head = torch.full(shape, 1.0).pin_memory() source = _PinnedSource({"lm_head": head, "embed_tokens": torch.full(shape, 2.0).pin_memory()}) copy_stream = torch.cuda.Stream() pool.load_async("lm_head", source, copy_stream) slot = pool.wait("lm_head") torch.cuda.synchronize() torch.cuda._sleep(int(1e9)) # the compute stream lags, like a queued head backward seen = slot.clone() # a read of the slot queued behind it pool.load_async("embed_tokens", source, copy_stream) # the #975 refill torch.cuda.synchronize() assert torch.equal(seen, head.cuda())
The host issues the refill microseconds after the read is queued, while the sleep takes about a second. So the race is always open, and the test only passes if
load_asyncorders its copy after the read.If you also want a trainer-level test, it has to assert that the race actually opened, for example by recording
event.query()on the sleep when the prefetch fires. Otherwise a slow host turns it into a test that always passes.About your note that you can't run the CUDA side on a 4 GB card: this is not a VRAM limit. The fixture is 2 layers × hidden 64, and the test above needs about 12 MB. Your 3050 can run both, as long as your torch is a CUDA build. Check with
python -c "import torch; print(torch.cuda.is_available())"; on Windows, pip installs the CPU wheel unless you pass the PyTorch index. -
The load-count comment (test lines 132 and 137–145) is unchanged. The assertion is right but the reasoning is not: on
mainthe same 2 steps make 4 real loads, and the file fails there withassert 4 == 5. This PR adds exactly one load pertrain(), the last backward's prefetch. Please say main = 4, PR = 5, and why. -
Changelog, docs and description.
- Add the changelog fragment. Since #1126 the baseline is 0.75.1, so the file is
changelog.d/0.75.1/1178.changed.md, crediting(#975 by @VanshSharmaPES in #1178). - Add the sentence at
docs/performance-and-quantization.md:354, which still says the embedding "is loaded for the model input". - Update the description. It still says "performance NOT measured"; the measured numbers are in my last review (embedding wait 19–23 ms → ~0.15 ms on that fixture). It also still gives only the version-counter argument, which your new comment now says does not protect the GPU.
- Add the changelog fragment. Since #1126 the baseline is 0.75.1, so the file is
Non-blocking
- The items from my last review are unchanged: preference losses get part of the benefit; the head's forward-tail wait is what (c) would now buy; a copy-stream sync in
close(); the redundant flag; and the_primecomment wording at:2587–2591. You listed the last one as still to do.
Nits
- The test module's docstring (lines 6–11) still gives only the CPU-side argument, "cannot invalidate a saved-tensor version check that already ran". Please point it at the GPU guard as well.
- The new comment says gradients come out wrong "from the second step on". That matches what I measured: step 1 never raced in any of the 7 lag runs. But the reason is that the first step's host-side overhead outlasts the lag, not anything in the code, and step 1's refill is issued the same way. I would drop the step qualifier.
- Line 107 still re-tests the pool's existing owner check rather than
_prime.
…l, fix the load-count reasoning, add changelog and docs This works through the second review on MakazhanAlpamys#1178. I added a pool-level GPU test for the wait_stream in LargeLayerBufferPool.load_async. It holds the compute stream back with torch.cuda._sleep, queues a read of the slot behind it, then issues the refill; the read must still see the head's bytes. It does not depend on host speed, unlike the trainer-level recipe I had planned, which would pass with the guard deleted whenever the sleep had already finished. I cannot run it here (my torch is a CPU build), so it is skipped on this machine; it follows the codeowner's version, which they checked 5 of 5 with the guard and 5 of 5 failing without it. I rewrote the load-count comment: main makes 4 real large-layer loads over the two steps, this change makes 5, and the extra one is the last backward's prefetch, which no later forward consumes. I dropped the assertion that only re-tested the pool's owner check, and pointed the module docstring at the GPU guard as well as the CPU one. In layer_stream_runtime.py I reworded _prime's comment to "a backward that never reached layer 0" and dropped the "from the second step on" qualifier from the wait_stream comment. I also added the changelog fragment and one sentence in docs/performance-and-quantization.md saying the embedding is reloaded at the tail of the backward pass.
|
Thanks for the second pass, and for catching that the lag recipe I'd planned would be flaky on a busy host. I've pushed Blocking 1, the CUDA-only test. I added your pool-level test as written: the compute stream is held back with Blocking 2, the load-count comment. Rewritten: Blocking 3.
Nits, taken:
Checks on the merged tree (CPU): ruff is clean, the changelog validator passes, and the streaming, docs and changelog suites give 602 passed, 35 skipped, 0 failed. Merging Not taken: the copy-stream sync in |
|
@VanshSharmaPES, thank you. Every item from the last review is addressed. Three things stand out:
The description now leads with the measured magnitude, the fragment and the docs sentence are in, and merging What I ran. All of it was on CPU with
#1141 and #1143, the other two open drafts in Before this leaves draft: one run of the GPU test, both ways. That is the only open item. The test matches the recipe line for line; only the embedding tensor got its own name. It is not deselected by default (only
Please post the counts, the card and the torch build. The test needs about 12 MB, so the 3050 is plenty. On Windows, pip installs the CPU wheel unless you point it at the PyTorch CUDA index for a CUDA version your driver supports; After that, mark it ready for review. The two non-blocking items you left, the copy-stream sync in CI on |
|
Here are the GPU counts you asked for, on Card and build: RTX 3050 Laptop GPU, torch
Earlier I got a first mutation attempt wrong: the script raised before writing, so its three passes were against unmodified code and proved nothing. I redid it and only counted runs where the guard was really gone. I've also opened the prototype for moving the head's load earlier as a separate draft, #1258, branched from this head as you asked. Nothing in it changes what this PR does. |
|
@VanshSharmaPES, thank you for getting a CUDA build onto the 3050 and running this yourself. Thank you also for saying that your first mutation attempt ran against unmodified code, instead of counting its three passes. That note is why the other counts can be trusted. Your run closes the one item left open on 2026-09-24. The committed GPU test:
You also named the card, the torch build and the driver. Patching Current state, checked today:
The GPU evidence on record is your run. No GPU was available for this check, so I did not re-run it. The next step is yours: mark it ready for review. #1258 is branched from this head, so it can only land after this one. |
|
@VanshSharmaPES, thank you for marking this ready, and for the 3050 run that made it possible. What is good here, specifically
One blocking item: the tests pin that the refill fires in the backward, not where
On any model with three or more layers, the two placements differ. The same 4-layer recording with that line removed gives: The head-sized copy is now issued ahead of layer 0's backward load. On CUDA, that load then queues behind it on the copy stream both pools share, so the wait moves into the backward instead of going away. It also changes the order of A 4-layer version of your trainer test catches it. Paste it into def test_the_refill_fires_once_per_backward_at_layer_0(self, tmp_path, monkeypatch):
"""Where in the backward the refill fires, not only that it fires.
On two layers the first recompute that walks downward IS layer 0, so a
refill moved to the start of the backward passes every other test here.
On a real model that would queue the whole head-sized copy in front of
the backward's decoder copies on the shared copy stream, and change the
order of `source.get()` calls, which #975 relies on being unchanged.
Four layers tell the two apart.
"""
wrapper, _, _ = _build_untied_wrapper(tmp_path, monkeypatch, n_layers=4)
prefetcher = wrapper._stream_runtime.prefetcher
refill = prefetcher.backward_tail_prefetch
fired_at = []
def _recording_refill():
fired_at.append((prefetcher.direction, prefetcher.prev))
refill()
prefetcher.backward_tail_prefetch = _recording_refill
wrapper.trainer.args.max_steps = 2
wrapper.trainer.train()
wrapper._close_stream_runtime()
# (direction, layer) at each call: once per step, walking down, at layer 0.
assert fired_at == [(-1, 0), (-1, 0)], fired_atHere is what I got with it in your file, on CPU, with your branch merged with current
The method is clean under One nit The module docstring (lines 23-25) says What I ran Everything ran on CPU with
On the GPU side, your 3050 run is the evidence for the For #1258, once this lands If this PR is squash-merged, a plain I tried that on a local squash of this PR, and the runtime file and both test files came out byte-identical to CI: all 13 required checks pass on |
…ot only that it fires On a 2-layer fixture the first backward recompute that walks downward IS layer 0, so removing the 'idx == 0' check from the trigger passed every test here: the refill still fired once per backward, just potentially at the wrong layer. On a real model that would queue the head-sized copy ahead of the backward's decoder copies on the shared copy stream instead of after them, and it would change the order of source.get() calls, which MakazhanAlpamys#975 relies on staying unchanged. I added a 4-layer test that records (direction, layer) at every call and asserts it fires walking down, at layer 0, once per step. Removing the idx == 0 check now fails it. I checked two more of the previously-reported mutations against the new test and got the same results as before: the once-per-pass flag check is still an equivalent mutant, and removing the flag reset in prime() still fails 2. I also corrected the module docstring: _build_streamed_wrapper has taken tie since MakazhanAlpamys#1061, and this file predates that.
…'s load is issued (MakazhanAlpamys#1174) I wrote this so the 5070 run for MakazhanAlpamys#1258 is one command. It flips StreamPrefetcher.head_prefetch_layer between blocks in one process on one model, using the committed stream_probe.Instruments and layer0_wait's labelling, so the wait() brackets are the same ones as gate-971 section 8. The number to judge an arm by is the step's total wait, with per-group medians (embedding, head, layer 0, other decoder layers) to show where a change went. It covers both tiers, --read-ahead, and a preference-shaped step (a no-grad forward first) for the disk-tier case at read-ahead 3. The arm order rotates each block. On a tree without head_prefetch_layer it exits with a message instead of comparing an arm with itself, which I checked against MakazhanAlpamys#1178's head. I ran it on an RTX 3050 with a 4-layer untied 8B-shaped fixture on the RAM tier and on the disk tier with the preference step at read-ahead 3, to check that it completes and that the switch takes effect. Those are 4 samples per arm on a different card, so I am not reporting them as a result.
|
Thanks for the exact repro on two layers, and for the test to tell the two placements apart. Pushed The blocking item. I added your test verbatim, on a 4-layer fixture: it records The nit. The module docstring no longer claims What I ran, on
For #1258: once this is squash-merged, I'll rebase with |
|
Merged as |
…s and yi111 #1103 (@umran666), #1281 (@Blizzeq), #1178 (@VanshSharmaPES), #1117 (@Srinivasan8888), #1257 (@wanjinhao1, first merge), #1284 (@Yi-111-a, first merge), #1304 (@kok-o), #1115 and #1144 (@Amix29), #1077 (@HarshRajSinghania). Display names from `gh api users/<login>`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ookup (MakazhanAlpamys#1174 part 2) This is the zero-VRAM alternative to a second slot (option (c) on MakazhanAlpamys#1174), to be measured before it. With MakazhanAlpamys#1178 the embedding is reloaded at the tail of the previous backward, so what is left of the large-layer wait is the head's forward-tail load. It is issued from StreamPrefetcher.advance() at the last decoder layer, before that layer's functional_call, so the copy hides behind one layer of compute. The embedding's bytes in the shared slot are dead once the lookup has run, and the lookup precedes layer 0's advance, so I added head_prefetch_layer and default it to 0, the earliest point. None restores the last-layer timing, and it is a plain attribute, so one process can run both timings. The refill is ordered after the queued embedding lookup by the wait_stream in LargeLayerBufferPool.load_async, the same guard MakazhanAlpamys#1178 relies on. I added a pool-level GPU test where the queued read is the embedding lookup. I checked on CPU and on an RTX 3050 that the two timings are bit-identical for untied sft (also at gradient_accumulation_steps 2), orpo, simpo, dpo and kto and for tied sft, that the real load sequence and count are the same in both, and that an eval forward followed by a training step behaves the same. I have not measured whether this is faster. That needs the wait() bracket on a GPU, judged by the step's total wait on both the RAM and the disk tier. Two things it may not do: the head's copy queues ahead of the next decoder copy on the single copy stream, and on the disk tier a head get() mid-forward replans the reader's lookahead. Both are unmeasured.
…'s load is issued (MakazhanAlpamys#1174) I wrote this so the 5070 run for MakazhanAlpamys#1258 is one command. It flips StreamPrefetcher.head_prefetch_layer between blocks in one process on one model, using the committed stream_probe.Instruments and layer0_wait's labelling, so the wait() brackets are the same ones as gate-971 section 8. The number to judge an arm by is the step's total wait, with per-group medians (embedding, head, layer 0, other decoder layers) to show where a change went. It covers both tiers, --read-ahead, and a preference-shaped step (a no-grad forward first) for the disk-tier case at read-ahead 3. The arm order rotates each block. On a tree without head_prefetch_layer it exits with a message instead of comparing an arm with itself, which I checked against MakazhanAlpamys#1178's head. I ran it on an RTX 3050 with a 4-layer untied 8B-shaped fixture on the RAM tier and on the disk tier with the preference step at read-ahead 3, to check that it completes and that the switch takes effect. Those are 4 samples per arm on a different card, so I am not reporting them as a result.
) (#1313) `data.val_split` (default 0.1) withheld that share of the rows on every run. Every trainer wrapper then passed those rows on as `eval_dataset`, but none set `eval_strategy`, whose Hugging Face default is "no", so `Trainer.evaluate()` never ran. The held-out rows were used for nothing, and the validation-loss sinks from #713 (the Val loss row, the tracker's `val_loss` and `TrainEvent.val_loss`) never received a value on the transformers backend. A shared helper, `training_eval_kwargs` in the new `utils/eval_schedule.py`, is threaded through all 16 wrappers that receive a split: sft, pretrain, dpo, ipo, kto, orpo, simpo, bco, grpo, distill, classifier, embedding, reward_model, prm, moe_lora_routing and asr. Each evaluates a non-empty split at the end of every epoch, or every `training.eval_steps` optimizer steps. `training.eval_steps` is a new field; it must be an int of at least 1, and a YAML bool is refused. The evaluation runs at the resolved per-device train batch, not Hugging Face's 8. The embedding trainer gains a `prediction_step`, because the default one crashed on its anchor/positive batches. The generation-based tasks (grpo, ppo, online_dpo) no longer withhold rows by default. `soup train` (including `--dry-run`), `soup sweep` and `soup cost` use every row and print a one-line note. grpo holds the split out and evaluates it only when `training.eval_steps` is set: - It evaluates at the largest multiple of `num_generations` that fits in the train batch, because TRL refuses an eval batch that splits a group of completions; a train batch of 3 with 2 generations used to raise at setup. - Its evaluation rewards are kept out of the reward-hack detector, the mitigation controller and the echo-trap detector: the signal buffer they share is paused for the whole `trainer.evaluate` call. - The docs say what grpo records as `eval_loss`: TRL's policy objective on the held-out completions, not a likelihood. The held-out reward is `eval_reward`. `training.eval_steps` is refused at config load on ppo, online_dpo and unlearn, on `backend: mlx`, and with `val_split: 0` on local files or remote URIs. MLX is otherwise unchanged. What users notice: runs now evaluate the held-out split at the end of every epoch and show a Val loss, which costs a few percent more time. grpo, ppo and online_dpo configs now train on every row instead of 90%, so their results are not directly comparable across this change. Verified: - The first CI run failed in all nine test cells, and both causes were in this PR's own tests. A subprocess `.stdout` assertion tripped the #1068 ANSI guard. The layer-streaming evaluation test built on CPU while transformers put the model on MPS on the macOS runners. Both are fixed: the assertion goes through `_plain`, and the streaming test now runs only where transformers' default device is the CPU its wrapper streams on. That is test_v07200.py's MPS guard, extended to a visible CUDA card, with both edges pinned by a test. - The second CI run failed in every test cell on one assertion, #833's guard against a private CUDA probe in a test module: the device helper called `torch.cuda.is_available()`. It now reads the suite's shared `conftest.cuda_available`, and the condition test pins that probe; making the helper ignore CUDA fails the condition test. - The six `soup train --dry-run` count tests read the real card, so on a dev box with a GPU the hardware-fit gate refused before the counts printed (green on CPU CI). They now stub the two probes the way a CPU runner answers them, as the #1212 tests do: 58 passed with an RTX 5070 visible, where 6 failed before. - A new test pins the remote-URI refusal. - On the branch rebased onto main with #1316 and #1281: - the #1223 files, the ANSI guard and 61 other neighbour files: 5,611 passed, 14 skipped, 0 failed; - `test_v05311.py` in its own process: 89 passed; - the MLX files in their own process: 52 passed. - Merged with main after #1178, the streaming, snapshot and #1223 files: 681 passed. - Without the fix, the three original test files give 102 failed / 12 passed on main. With the grpo-isolation and whole-group commits reverted they give 24 failed / 90 passed, including the leak itself: under TRL generation reuse the detector read the held-out rewards (40, 40, 40, 40) instead of the training rewards (7, 7, 4, 4). - 26 distinct targeted mutations all fail a test. - ruff is clean. Closes #1223.
…ookup (#1174) (#1258) On an untied streamed checkpoint, `embed_tokens` and `lm_head` share one device slot. Since #1178 the embedding is reloaded at the tail of the previous backward, so what was left of the large-layer wait was the head's forward load. `StreamPrefetcher.advance()` issued that load at the last decoder layer, where it could hide behind only that one layer's compute. The head's load is now issued from layer 0's `advance()`, right after the embedding lookup: the embedding's bytes in the slot are dead once the lookup has run. - `StreamPrefetcher.head_prefetch_layer` defaults to 0 when there is a large pool, and `None` restores the old timing. It is a validated property (None, or an int in [0, n_layers); a bool is refused; a refused assignment keeps the old value), so a harness can switch timings between steps in one process. - The refill is ordered after the queued lookup by the `wait_stream` in `LargeLayerBufferPool.load_async`, which a new pool-level GPU test pins. - `benchmarks/harness/head_prefetch_ab.py` is an interleaved A/B driver with a row in the benchmarks harness table. It has an A/A control through `--arms last,7` and rotates the arm order. After every block it writes a JSON with the GPU facts (NVIDIA driver and PCIe link included), every command-line argument, and the commit of the imported `soup_cli` tree, marked `-dirty` for uncommitted changes under `src/`. `--shards` reuses an existing shard store. What users notice: less waiting per step on untied streamed checkpoints, and nothing else. Loss, gradients and adapter weights are bit-identical between the two timings, and tied checkpoints are unchanged. On an RTX 5070 Laptop GPU (8-layer untied fixture, hidden 2048, 501 MiB shared slot, NF4, sequence 512, 10 recorded steps per arm), the median total wait per step fell: - from 11.96 to 4.50 ms on the RAM tier (A/A spread 0.11 ms); - from 15.83 to 1.42 ms on the disk tier (A/A spread 0.74 ms); - from 57.29 to 29.16 ms for a preference-shaped step on the disk tier at read-ahead 3. Every layer-0 sample was below every last-layer sample. The other decoder layers now wait longer (0.78 to 4.06 ms on the RAM tier), which gives back part of the saving, and layer 0 was the best of layers 0 to 3. Verified: - Per-step gradients and final adapters are bit-identical between the two timings for SFT, SFT at gradient_accumulation_steps 2, ORPO, SimPO, DPO, KTO and tied SFT, checked against a same-timing control with lora_B randomised. - On the tree merged with main, the #975, #1174, provenance, changelog-fragment, merge-freshness and benchmarks-index tests give 112 passed, 2 skipped (the GPU tests), 0 failed. - All 8 mutations of the new validation are killed. - All 11 mutations of the driver's provenance code are killed, including hard-coding `git_sha` in the payload and dropping `--shards`. A CPU test now runs the driver's real `main()` with only the GPU-bound pieces faked. - On the RTX 5070, both pool GPU tests pass 3 of 3, and fail 3 of 3 with the `wait_stream` removed. Closes #1174.
…e shared slot (#975) (#1781) `benchmarks/gate-971-async-nvme-source.md` gets two dated correction notes for the embedding entry (#975): one under the opening of section 8 and one under the finding that attributes the embedding stall to the single-member-group behaviour. Both say the same thing. `embed_tokens` and `lm_head` share one device slot, so each step's `_prime()` re-copies the whole matrix host to device with nothing to overlap it (a copy, not a read; fixed by #1178), which supersedes the earlier cause. The stall figures, the record's other findings and the 0.07-0.12% materiality ruling stand. Docs only: no source change, no fragment. Closes #975
Draft. On @MakazhanAlpamys's RTX 5070 run this removes about 19-23 ms of embedding wait per step (about 22 ms down to about 0.15 ms on the disk tier), on a fixture with a 501 MiB shared slot. That is still only about 0.1% of a cold 70B step, so this is latency hygiene rather than a throughput fix. This implements option (b) from #1174.
What I changed
LargeLayerBufferPoolholdsembed_tokensand an untiedlm_headin one shared slot._prime()(the forward-pre-hook) reloadedembed_tokensright as the next step's forward started, a full head-sized H2D copy with nothing to overlap it against (#975).I made
StreamPrefetcheralso fire that same load at the tail of the backward pass, when the checkpoint recompute for decoder layer 0 confirms the step's backward walk is done._prime()keeps its unconditional load as a fallback (step 0, and any forward that follows a backward that never reached layer 0); from the second step on it is a same-owner no-op.Why the refill is safe
Two separate guards, and only the second protects the GPU:
lm_head's backward Node sits between the loss and every decoder layer, so the autograd engine has already dispatched it, and the saved-tensor version check happens at dispatch. The embedding's own backward reads indices, never the weight values, so it never checks this slot's version.stream.wait_stream(torch.cuda.current_stream())inLargeLayerBufferPool.load_async. Without it the refill overwrites bytes the queued backward has not read, and the gradients come out silently wrong. That wait is now the only thing protecting this refill point, and a GPU-only test pins it.Measured (by @MakazhanAlpamys, RTX 5070 Laptop, not by me)
On an untied Llama with hidden 2048, 8 layers, vocab 128256 (a 501 MiB slot), decoder NF4, seq 512, batch 1:
wait()median, before to afterThe embedding wait goes to essentially zero in every configuration. The 2-6.5% step-time change is real but close to that fixture's noise floor (identical code differs by about 6 ms between labels). The head's forward-tail wait (10.6-14.6 ms) and the decoder waits are unchanged. Option (c) was not measured.
Their correctness runs: untied SFT is bit-identical to
mainon CPU fp32 and CUDA bf16; untied DPO and KTO with #1061 are bit-identical to #1061 alone and to resident.What to know
_prime, and so do KTO's extra forwards: 1 of 2 embedding waits removed per DPO step, 1 of 4 per KTO step.train(). Steady state is 2 large-layer loads per step in both trees, but the last backward's prefetch is never consumed by a later forward, somainmakes 4 loads over 2 steps and this makes 5.What I verified
ruff checkclean.pytest -k "stream or 1049 or 975 or docs or changelog"): 602 passed, 35 skipped, 0 failed, after mergingmain.tests/test_issue975_backward_tail_embed_prefetch.py: the backward-tail fires, the slot already holdsembed_tokensafter step 1, 2 steps complete with no autograd error, and the real-load count is 5.What I have not run
The GPU-only test for the
wait_streamguard (test_refill_waits_for_a_read_still_queued_on_the_compute_stream). My torch is a CPU build, so it skips on my machine. It follows the version @MakazhanAlpamys verified on the 5070: 5 of 5 pass with the guard, 5 of 5 fail with it removed.