Repository navigation
perf(layer-stream): issue the head's load right after the embedding lookup (#1174) - #1258
Conversation
I added changelog.d/0.75.1/1258.changed.md, crediting MakazhanAlpamys#1174 by @VanshSharmaPES in MakazhanAlpamys#1258. It says the speed effect is not measured yet, since that needs the step's total wait on a GPU on both tiers.
|
@VanshSharmaPES, thank you for turning the go-ahead on #1174 into a prototype within hours, and for bringing all the evidence that list asked for. Three things stand out:
What I ranEverything ran on CPU (
Your two hazards, checked on CPU1. The shared copy stream. I recorded the order in which copies reach the two pools on a 4-layer untied fixture (RAM tier). Both pools share one copy stream on CUDA, so this is the order they would queue in there.
Your reading holds: layer 2's copy queues behind the head's. One addition: at every SFT step start, the head now also queues behind 2. The disk tier. At the default depth I also saw no difference in decoder misses. At
What it showed:
None of this calls for a change to Before this leaves draft
Non-blocking
CI: all 13 required checks pass on Not run here, because it needs a GPU:
|
|
Thanks for the thorough pass, and for comparing the two trees and not only the two arms. I've pushed The range check. You're right that the harness switch is an assignment, so it skipped the constructor's check and a bad value would only surface at the head's forward as "scheduler bug". The docs sentence. Checks on Left as is:
Your note about the bit-identity class taking 8 of 8 on #1178's head is fair. I'll keep the split you described: those tests guard against regressions, and the trigger and installed-default tests pin the move itself. I'm also planning to write the interleaved A/B script for the 5070 run, covering both tiers, total stall, and DPO at |
…'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.
|
Pushed It flips On a tree without I ran it on my 3050 on the RAM tier and on the disk tier with the preference step at read-ahead 3, with a 4-layer untied fixture I generated locally, only to confirm it completes and the switch takes effect. That's 4 samples per arm on a different card, so I'm not reporting those numbers as a result, and the synthetic generator has no hidden-2048 shape to match your fixture anyway. It should be ready to point at whatever fixture the 5070 run uses. Example invocations are in the module docstring. |
|
@VanshSharmaPES, thank you for the driver. It turns the 5070 run into commands instead of a procedure. Three things about it stand out:
What I checked (on CPU; nothing here ran on a GPU)
One correction to the rebase plan On #1178 you wrote that you would run That form replays your five commits cleanly. Afterwards Before this is ready for review
CI: |
|
@VanshSharmaPES, a quick follow-up. #1178 merged as I tried the command against the real merge commit, on today's It replays your five commits with no conflicts. What is left on your side, all from my last comment:
They can all go in one push. The default and the fragment still follow the RTX 5070 number. That run is on my side, and nothing above waits for it. If any of this is in your way, or you are busy this week, just say so here. |
…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.
…aming wires (MakazhanAlpamys#1174 part 2) My arms set head_prefetch_layer themselves, so reverting the wiring in install_streaming still passed. I added tests for the untied default (0) and the tied case (no large pool, None), and confirmed the reverted wiring now fails one of them.
I added changelog.d/0.75.1/1258.changed.md, crediting MakazhanAlpamys#1174 by @VanshSharmaPES in MakazhanAlpamys#1258. It says the speed effect is not measured yet, since that needs the step's total wait on a GPU on both tiers.
…cument the head's timing (MakazhanAlpamys#1174) The harness switch is an attribute assignment, which skipped the range check I had only in the constructor, so a bad value surfaced at the head's forward as a large-layer scheduler bug that names the wrong cause. I turned head_prefetch_layer into a property whose setter runs the same check, and it now also refuses bool, since True would otherwise pass as layer 1. I added tests that an assignment is refused and leaves the old value, and that valid values are accepted. I also updated docs/performance-and-quantization.md, which still said the shared slot is reused for the head after the last decoder layer. It now says the head's copy is issued right after the embedding lookup.
…'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.
…ha for head_prefetch_ab.py (MakazhanAlpamys#1258) Per review: give head_prefetch_ab.py a row in the harness table like every other harness script has, and record soup_cli_file plus git_sha in the JSON payload so a run says which tree it measured, the way stream_probe's meta has since MakazhanAlpamys#974.
b1c595c to
7736564
Compare
MakazhanAlpamys
left a comment
There was a problem hiding this comment.
@VanshSharmaPES, thank you for doing all three items in one push. Three things about it stand out:
- The rebase is exact.
git range-diffshows all five commits replayed as identical patches (5 of 5=).layer_stream_runtime.py, your test file and the fragment are byte-identical tob1c595c9, so the RTX 5070 run below measured exactly the code we reviewed. - The setter holds up. I broke it eight different ways, and every one turns your file red. Your own
if False:mutation fails exactly the 12 refusal cases, as you said. - The driver worked on the card. It took one command per configuration, and the
last,7A/A arms gave the noise floor the decision needed.
The RTX 5070 result
The run used your new head merged with today's main (bb92aa2c) on #1178's 8-layer untied fixture: hidden 2048, vocabulary 128,256, a 501 MiB shared slot, NF4, sequence 512, batch 1. It kept the driver's default schedule, which gives 10 recorded steps per arm. Median total wait per step, in ms:
| run | last |
0 |
change | A/A spread |
|---|---|---|---|---|
| RAM tier, SFT | 11.96 | 4.50 | −7.46 | 0.11 |
| disk tier, SFT, read-ahead 2 | 15.83 | 1.42 | −14.41 | 0.74 |
| disk tier, preference step, read-ahead 3 | 57.29 | 29.16 | −28.13 | — |
- Every sample separates. In all three runs, every layer-0 sample is below every last-layer sample.
- The head's wait is gone, and the decoder layers pay part of it back. The head's own wait drops to 0.1–0.2 ms. As you predicted, the other decoder layers now wait longer, because a decoder copy queues behind the head's on the shared copy stream: 0.78 → 4.06 ms on the RAM tier and 1.03 → 9.24 ms in the preference step. On disk SFT there is no change (0.91 → 0.93). That gives back part of the head's saving, not all of it.
- Layer 0 is the best place, so the default stays at
0. A RAM-tier sweep oflast,0,1,2,3gave 12.49 / 3.99 / 4.80 / 4.97 / 5.06 ms. The sweep ran on the pre-rebase head, whoselayer_stream_runtime.pyis byte-identical. - The GPU guard is pinned on the card. Both pool GPU tests pass on the new head, 3 of 3 runs. With the
wait_streamremoved fromLargeLayerBufferPool.load_async, both fail, 3 of 3.
Step time moved the same way (medians 230.0 → 222.9, 294.4 → 286.9 and 397.5 → 381.3 ms). Step time is noisy on this box, though, so the wait is the number to quote.
Before this leaves draft
1. The fragment. changelog.d/0.75.1/1258.changed.md still says "The speed effect is not measured yet: it needs the step's total wait() on a GPU, on both the RAM and the disk tier." It can now say what was measured. A suggested replacement for that sentence:
On an RTX 5070 Laptop GPU, with an 8-layer untied checkpoint (hidden 2048, a 501 MiB shared slot, NF4, sequence 512), the step's median total
wait()fell from 11.96 to 4.50 ms on the RAM tier, from 15.83 to 1.42 ms on the disk tier, and from 57.29 to 29.16 ms on the disk tier atstream_read_ahead: 3for a step that runs a no-grad forward first. A decoder layer's copy now queues behind the head's on the shared copy stream, so the other decoder layers wait longer (0.78 to 4.06 ms on the RAM tier), which gives back part of the saving; layer 0 was the best of layers 0 to 3.
2. The NVIDIA driver version is lost in every new JSON (benchmarks/harness/head_prefetch_ab.py:297-299).
- The cause.
stream_probe.gpu_facts()returns its own"driver"key, which holds the NVIDIA version. The payload spreads it first and then sets"driver": "benchmarks/harness/head_prefetch_ab.py", and the later key wins. - The evidence. In the five JSONs from the card,
616.92appears nowhere. In the seven JSONs fromb1c595c9it is atgpu.driver. - A second effect.
gpualso changed from a dict to the bare device name, so old and new records no longer have the same shape.
One correction on my side, because it led to this. On 10:16Z I wrote that only --label could say which tree a run measured. That was wrong. gpu_facts() has recorded soup_cli_file since #991 (the #974 work), and your b1c595c9 payload already had it under gpu. So the smallest fix is to put the nesting back and keep your new key beside it:
"driver": "benchmarks/harness/head_prefetch_ab.py",
# the card, its NVIDIA driver and PCIe link, torch, and soup_cli_file
"gpu": stream_probe.gpu_facts(device),
"git_sha": _source_sha(),Those two items are all I would hold this on.
Worth doing in the same push
3. git_sha names the driver's checkout, not the tree that was measured (head_prefetch_ab.py:106-129).
- What happens.
_source_sha()runs git inPath(__file__).parents[2]. WithPYTHONPATHpointing at another tree, it recorded24ba7690(the driver's checkout) whilesoup_cliwas imported frombb92aa2c. - A precedent in the repo.
benchmarks/harness/variant2_gate.py:213-256resolves the commit fromsoup_cli.__file__withgit rev-parse --show-topleveland appends-dirty, which also covers a run on a tree with an uncommitted change.unknownfor a tree with no.git, as on the card, is honest and fine. - A test. This one is in the style of
tests/test_issue379_variant2_gate.py::TestSourceCommit. It fails 3 of 3 on77365646, and passes 3 of 3 with the nesting fix above plus a_source_sha()written the wayvariant2_gate.pywrites it:
"""#1174: the head-prefetch A/B driver's JSON must say which card, driver and tree it measured."""
import importlib.util
import shutil
import subprocess
from pathlib import Path
from types import SimpleNamespace
import pytest
_HARNESS = Path(__file__).resolve().parents[1] / "benchmarks" / "harness" / "head_prefetch_ab.py"
def _load_driver():
spec = importlib.util.spec_from_file_location("head_prefetch_ab", _HARNESS)
assert spec is not None and spec.loader is not None
module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module)
return module
_driver = _load_driver()
@pytest.mark.parametrize("status", ["", " M src/soup_cli/utils/layer_stream_runtime.py\n"])
def test_git_sha_names_the_imported_tree_and_marks_it_dirty(monkeypatch, tmp_path, status):
"""The driver and the soup_cli it measures can come from different checkouts, so
the commit is resolved from ``soup_cli.__file__``, not from this file."""
import soup_cli
package_dir = Path(soup_cli.__file__).resolve().parent
calls = []
def fake_run(command, *, cwd, **kwargs):
calls.append((list(command[1:]), Path(cwd)))
if command[1:] == ["rev-parse", "--show-toplevel"]:
return SimpleNamespace(stdout=f"{tmp_path}\n", returncode=0)
if command[1:] == ["rev-parse", "HEAD"]:
return SimpleNamespace(stdout="a" * 40 + "\n", returncode=0)
return SimpleNamespace(stdout=status, returncode=0)
monkeypatch.setattr(shutil, "which", lambda name: "git")
monkeypatch.setattr(subprocess, "run", fake_run)
assert _driver._source_sha() == "a" * 40 + ("-dirty" if status else "")
assert calls[0] == (["rev-parse", "--show-toplevel"], package_dir)
def test_the_gpu_facts_stay_nested_so_the_nvidia_driver_survives():
"""``gpu_facts`` returns a ``driver`` key (the NVIDIA version); spreading it into a
payload that also sets ``"driver"`` to the script path overwrites it."""
text = _HARNESS.read_text(encoding="utf-8")
assert '"gpu": stream_probe.gpu_facts(device)' in text
assert "**stream_probe.gpu_facts(" not in text4. Warm the shard cache first, and keep paths short on Windows. The card run hit two things that belong in the driver's docstring:
- Sharding in the same process slows both arms. The first process that also built the NF4 shards measured both arms about twice as slow as later processes: the last-layer head wait was 27.2 ms, against 10.9–11.5 ms in the five later RAM-tier processes. Its change (−16.45 ms) therefore overstated the effect; the three later RAM-tier A/B runs agree at −7.46 to −8.49 ms. The docstring should say to build the shards once and discard that run.
- A deep weights path fails on Windows. It stops with
WinError 206(MAX_PATH), becauseresolve_shard_dirnames the shard directory after the weights path, up to 128 characters. That islayer_shard.py's behaviour, not your driver's. - One change fixes both. The
--shardspassthrough from my earlier list now covers both cases.stream_probe.build()already readsargs.shards, and the driver hard-codesshards=None(:208).
Nits (optional)
ruff formatwould rewritehead_prefetch_ab.py:333-334and the setter's condition (layer_stream_runtime.py:1027-1031). CI runs onlyruff check, and that is clean.test_an_assignment_is_checked_like_the_constructor_and_leaves_the_valuestarts from0. ForFalse, the "value unchanged" check therefore cannot tell a storedFalsefrom the old0, and that one case survives a store-before-check mutation. Starting from2closes the gap.
What I ran
Everything here ran on CPU with the GPU hidden, on your head merged with today's main. That merged tree is the tree the card measured (48a012ac).
-
Tests. The #975 and #1174 files, the changelog-fragment and merge-freshness tests, and the two
benchmarks/README.mdindex tests: 104 passed, 2 skipped (the two GPU tests), 0 failed.ruff checkis clean, and all 185 fragments validate. -
Your file on
mainwithout your branch. 24 failed, 11 passed, 1 skipped. The 11 that pass are the regression guards we discussed before. -
Eight mutations of the setter, all killed:
mutation result the whole check off ( if False)12 failed boolrefusal dropped3 failed intcheck dropped3 failed upper bound <=2 failed lower bound dropped 2 failed value stored before the check 6 failed constructor bypasses the property 5 failed getter always None15 failed -
Three mutations of the provenance. I dropped
gpu_facts, hard-codedgit_shaand made_source_sha()always returnunknown. None of them fails a test, because no test runs the driver yet. The test in item 3 is for that. -
Merges. Your branch merges cleanly with #1141 and #1143, and with the other open PRs that add rows to
benchmarks/README.md.
CI: all 13 required checks on 77365646 are still queued.
Once items 1 and 2 are in and the checks pass on that head, nothing else on my list stands between this and review.
…oup_cli's tree, and add --shards (MakazhanAlpamys#1258) I broke two things when I recorded provenance: spreading gpu_facts() at the payload's top level let my own "driver" key (the script path) overwrite its "driver" key (the NVIDIA version), and _source_sha() resolved the commit from this file's own location instead of the soup_cli tree it actually measured. I put gpu_facts() back under a nested "gpu" key and rewrote _source_sha() the way variant2_gate.py's is, from soup_cli.__file__, with a -dirty suffix. I also add a --shards passthrough: sharding in the measured process slows every arm (my RTX 5070 run showed roughly 2x on the process that also sharded), and it can trip Windows' MAX_PATH once the weights path is long. I filled in the changelog fragment with the RTX 5070 numbers now that the measurement is in, and added a test file covering the two provenance bugs.
|
Thanks for catching both of those, and for the correction on where the tree-provenance fix should live — I hadn't noticed the spread was clobbering gpu_facts()'s own driver key. The lost NVIDIA driver. Fixed as you suggested: gpu_facts() is nested back under "gpu", and git_sha sits beside it. gpu is a dict again, so old and new records have the same shape. git_sha named the wrong tree. Rewrote _source_sha() the way variant2_gate.py's is — resolves --show-toplevel and HEAD from soup_cli.file's directory, not from this driver's own file, and appends -dirty off git status --porcelain --untracked-files=normal. unknown on a tree with no .git, same as before. Test. Added tests/test_issue1174_head_prefetch_ab_provenance.py, close to your sketch: the parametrized dirty/clean case against a faked subprocess.run, plus two unknown-path cases (no toplevel, no git binary), plus a source-text check that "gpu": stream_probe.gpu_facts(device) is present and **stream_probe.gpu_facts( is not, so a future spread-back regression fails loudly instead of silently. --shards. Added, wired through to stream_probe.build(). Docstring now says to shard once, discard that process's numbers, and point --shards at the sharded directory afterward — covers both the ~2x slower first process and the Windows MAX_PATH case. The fragment. Took your suggested replacement for the "not measured yet" sentence verbatim. Checks on c7cafea: ruff check clean; on CPU, pytest -k "stream or 1049 or 975 or 1174 or docs or changelog" gives 677 passed, 13 skipped, 0 failed, plus the new provenance file at 5 passed on its own. Left as nits, per your list: the ruff format rewrite and the setter test's starting value (0 → 2). Happy to take those too if you'd rather not carry them. |
…hanAlpamys#1258) ruff format on the setter's condition and the driver's one long print, and I start the assignment-checked-like-the-constructor test from layer 2 instead of 0, so a stored False can't be confused with an unchanged 0.
|
Picked up both nits in c14e45c, on top of c7cafea: ruff format. Applied to the setter's condition (layer_stream_runtime.py:1027-1031) and the driver's one long print (head_prefetch_ab.py). I left the unrelated StreamedLargeLayer.init reformat ruff format also wants — that one's outside what you flagged. The setter test's starting value. test_an_assignment_is_checked_like_the_constructor_and_leaves_the_value now starts from head_prefetch_layer=2 instead of 0, so a store-before-check mutation storing bad=False can't hide behind Python's False == 0 — the "value unchanged" assertion now actually distinguishes them. Checks on c14e45c: ruff check clean, ruff format --diff clean on every file this PR touches (the StreamedLargeLayer.init hunk aside), and on CPU pytest -k "stream or 1049 or 975 or 1174 or docs or changelog" gives 677 passed, 13 skipped, 0 failed. |
MakazhanAlpamys
left a comment
There was a problem hiding this comment.
@VanshSharmaPES, thank you. All four items are in c7cafeaa, pushed within minutes of the review, and both optional nits are in c14e45cf. Your commit message says plainly what broke and why, which makes a review like this quick. Three things stand out:
- The new test file pins both bugs. Put the top-level spread back, or resolve the commit from the driver's own file again, and it fails. It also covers
-dirtyand bothunknownpaths. - The fragment is exact. I recomputed every number in it from the raw JSONs of the run: 11.96 → 4.50, 15.83 → 1.42, 57.29 → 29.16, the decoder rise 0.78 → 4.06, the 501 MiB slot, 8 layers, sequence 512, and layer 0 as the best of 0–3. Each one matches.
- The nits are real fixes, not just style. The setter's reformat changes no code (the AST of
StreamPrefetcherand of the driver is identical before and after). Starting the assignment test from2means a store-before-check mutation now fails 7 cases instead of 6,Falseincluded.
What I ran
Everything ran on CPU with the GPU hidden, on c14e45cf merged with today's main (bb92aa2c). After main moved to f76c8afd, I ran the test set again and got the same counts.
Tests. The #975 and #1174 files, your new provenance file, the changelog-fragment and merge-freshness tests, and the two benchmarks/README.md index tests: 109 passed, 2 skipped (the two GPU tests), 0 failed. ruff check is clean. ruff format --check is clean on the driver and both test files; the one hunk left in layer_stream_runtime.py is main's own.
The driver's real main(). The payload is built inside main() behind the CUDA check, so I faked only the GPU-bound pieces: stream_probe, layer0_wait, the bitsandbytes optimizer, and the three torch calls made with device="cuda". I faked gpu_facts with its real key set; everything else ran as your code.
- On
c14e45cf,gpuis a dict andgpu.driveris616.92.git_shais the merged tree's commit, and--shards D:/fake/untied-8l-shardsreachesbuild()and the JSON'sshard_dir. - The same run on
77365646givesgpuas a bare string, with no driver version anywhere.
_source_sha() with real git.
- A
git archiveextract (no.git) givesunknown. - The clean worktree gives its commit.
- A tracked edit under
src/gives-dirty.
Eight mutations of the setter, all killed (12, 3, 3, 2, 2, 7, 5 and 15 failures). The store-before-check one is the 7, which was 6 before your change.
Nine mutations of the provenance code, five killed:
| mutation | result |
|---|---|
gpu_facts spread at the top level again |
1 failed |
gpu_facts dropped |
1 failed |
_source_sha() never reports the commit |
2 failed |
| commit resolved from the driver's own file again | 2 failed |
-dirty dropped |
1 failed |
Two of those five survived at 77365646: dropping gpu_facts, and _source_sha() never reporting the commit. The four that still pass are listed below, and none of them is something I would hold this on.
From my side this is ready. Nothing on my list is left, so you can take it out of draft. It then waits only on the 13 required checks, which are queued on c14e45cf right now.
Optional follow-ups (none of them holds the merge)
1. Two sentences in the docstring (head_prefetch_ab.py:31-39).
- Who measured it. "I saw a last-layer head wait of 27.2 ms…" reads as your measurement, but those numbers came from the RTX 5070 run on this PR. Since
benchmarks/is the tracked record, it is worth saying so. - What caused it. "The sharding itself runs on the same card as the arms, so a process that also shards measures both arms slower" states a cause. NF4 quantisation does run on the card (
layer_shard.py:905-924), but it finishes before the first step, and why the arms were slower afterwards was not isolated.
Something like:
The RTX 5070 run on #1258 measured a last-layer head wait of 27.2 ms in the process that also built the NF4 shards, against 10.9-11.5 ms in five later processes; the cause was not isolated. Build the shards once and discard that process's numbers, or point
--shardsat an already-sharded directory.
2. Two error branches have no test. Dropping the 40-hex check changes nothing your tests can see, and neither does narrowing except (OSError, subprocess.SubprocessError) to OSError.
The second one matters in practice. The card run used a git archive tree, where git rev-parse --show-toplevel exits 128, and with check=True that is a CalledProcessError.
The two tests below pass on c14e45cf, and each fails one of those mutations. They are ready to append to your provenance file. They use its _driver, SimpleNamespace, Any and Path, plus import subprocess:
def test_source_sha_is_unknown_when_git_exits_non_zero(monkeypatch: pytest.MonkeyPatch) -> None:
"""A tree with no ``.git`` (a ``git archive`` extract) makes ``git rev-parse
--show-toplevel`` exit 128; with ``check=True`` that is a ``CalledProcessError``."""
def fake_run(command: list[str], *, cwd: Path, **kwargs: object) -> Any:
raise subprocess.CalledProcessError(128, command)
monkeypatch.setattr(_driver.subprocess, "run", fake_run)
assert _driver._source_sha() == "unknown"
def test_source_sha_does_not_record_a_head_that_is_not_a_commit(
monkeypatch: pytest.MonkeyPatch, tmp_path: Path
) -> None:
def fake_run(command: list[str], *, cwd: Path, **kwargs: object) -> Any:
if command == ["git", "rev-parse", "--show-toplevel"]:
return SimpleNamespace(stdout=f"{tmp_path}\n")
if command == ["git", "rev-parse", "HEAD"]:
return SimpleNamespace(stdout="not-a-commit\n")
return SimpleNamespace(stdout="")
monkeypatch.setattr(_driver.subprocess, "run", fake_run)
assert _driver._source_sha() == "unknown"3. The payload wiring itself is still untested. The other two survivors are hard-coding "git_sha": "unknown" in the payload and reverting shards=cli.shards to None. Both live in main() behind the CUDA check. A CPU test that runs main() with the GPU-bound pieces faked would cover them, the way tests/test_issue379_variant2_gate.py already runs its harness's main().
4. The record does not say how it was configured. The JSON has tier, read-ahead, sequence, batch and step shape. It has no quant, lora_r, lora_targets, buffers, seeds or no_pin, so the fragment's "NF4" cannot be read from the record itself. Adding "args": vars(cli) to the payload would capture all of them.
5. -dirty also fires on an untracked output file. With --untracked-files=normal, an untracked ab_ram.json at the repo root turns a clean tree into …-dirty. The docstring's --out ab_ram.json, run from the root, does exactly that from the second run on. It errs in the safe direction, and variant2_gate.py behaves the same way. Adding -- src to the status call, or writing outputs outside the tree, would avoid it.
…refetch driver (MakazhanAlpamys#1258) I attributed the 27.2 ms sharding-process number to a general observation when it was specific to the RTX 5070 run on this PR, and I stated a cause (sharding itself slowing the arms) I hadn't isolated. Reworded to say who measured it and drop the unverified cause. _source_sha()'s dirty check now scopes to -- src, so an untracked --out file left at the repo root doesn't turn a clean tree dirty. Added two more _source_sha() tests: a non-zero git exit (a git-archive tree, which is what the RTX 5070 run's card actually had) and a HEAD that isn't a 40-hex commit. Both fail without the fix. Added a CPU test that runs the driver's real main() with only the GPU-bound pieces faked (the model, runtime, stream_probe, layer0_wait, the bitsandbytes optimizer), so the payload wiring itself is covered: a hard-coded git_sha or a shards silently reverted to None would now fail a real run of main(), not just a source-text check. Added "args": vars(cli) to the payload, so a record's own JSON says how it was configured (quant, lora_r, seeds, etc.) instead of relying on the changelog fragment's word for it.
|
Picked up all five optional follow-ups too, in d7f5551 on top of c14e45c:
Checks on d7f5551: ruff check clean, ruff format --diff clean on every file this PR touches, and on CPU pytest -k "stream or 1049 or 975 or 1174 or docs or changelog" gives 680 passed, 13 skipped, 0 failed (8 of those in the extended provenance file alone). |
MakazhanAlpamys
left a comment
There was a problem hiding this comment.
@VanshSharmaPES, thank you for taking all five. The main() test is the one I'd point others at: it runs the driver's real code path on CPU, so the JSON is now checked as it is written, not as the source reads.
I verified each of the five by running it on d7f55514 merged with today's main (f76c8afd), on CPU with the GPU hidden:
- The docstring now credits the RTX 5070 run on this PR and says the cause was not isolated. The first-person "I saw" and the stated cause are gone. The only code changes in the driver are the
-- srcscope and the"args"key; everything else in the diff is docstring or comment text. - The two
_source_sha()tests pass. Without the 40-hex check, and with theexceptnarrowed toOSError, one of them fails in each case. - The
main()test passes. Hard-coding"git_sha"in the payload, revertingshards=cli.shards, and dropping"args"each fail it. "args": a realmain()run writes all 21 flags, includingquant: nf4,lora_r,lora_targets,buffers, both seeds,no_pinandshards.-- src, checked with real git:- an untracked
ab_ram.jsonat the repo root no longer marks the tree dirty; - an untracked file or a tracked edit under
src/still does; - a
git archivetree still givesunknown.
- an untracked
Tests: the #975 and #1174 files, your provenance file (8 of 8), the changelog-fragment and merge-freshness tests, and the two benchmarks/README.md index tests give 112 passed, 2 skipped (the GPU tests), 0 failed. ruff check and ruff format --check are clean.
Mutations: all 9 of the provenance mutations from my last review fail your tests now; four of them passed before this commit. Two more that I added also fail: dropping "args" and unscoping the git status call. That is 11 of 11.
A footnote on my own suggestion, and no change is needed here: -- src also stops a local edit to benchmarks/harness/*.py from marking the tree dirty. When the driver and soup_cli come from the same checkout, -- src benchmarks/harness would cover the measuring scripts too.
Nothing is open on my side. The PR is still a draft; once it is marked ready, it waits only on the 13 required checks, which are queued on d7f55514.
|
Merged as |
…pped cases on MPS (#1355) (#1375) The streamed test builders chose their device with `"cuda" if cuda_available() else "cpu"` and never picked `mps`. On Apple Silicon the model was built on the CPU while `TrainingArguments` picked `mps:0` by itself, so the first training step failed on a device mismatch. 32 streamed cases carried an "MPS is untested" skip, and #1258 had just added three more classes with the same skip. CI therefore never ran a streamed backward pass, optimizer step or checkpoint save on MPS, a path Soup ships as experimental. - `tests/conftest.py` gains `mps_is_the_accelerator()` (cached, like `cuda_available`) and `accelerator_device()` (cuda, else mps, else cpu). The four builder sites use it, and the three module-local `_mps_is_the_accelerator` copies are gone, so the helper lives only in conftest, as the #833 rule requires. - The skip is removed from the 32 cases and from #1174's three classes (`test_issue1174_head_prefetch_after_lookup.py`, 14 cases), which now run on the macOS cells. Both bitsandbytes MPS skips and the MPS-only parity control are unchanged. The CPU-pinned bit-exact cases get no `TrainingArguments` CPU pin, because none of them reaches a trainer step. - `tests/test_issue1355_accelerator_device.py` pins the resolver's order (cuda, then mps, then cpu, including a torch with no `torch.backends.mps`) on every machine, not only on a Mac or a GPU box. - Test-only: nothing a user runs changes, so there is no changelog fragment. Verified: - The six files, the #833 guard, #1174 and the new test give 485 passed / 23 skipped with the GPU hidden, on the branch and merged with `main`. On `main` they give 480 / 23, and the only difference is the five new cases. - The resolver test fails on `main` and catches every resolver mutation: always `"cpu"` (3 failures), ignoring CUDA (2), `main`'s expression (1) and the MPS probe ignoring CUDA (1). Forcing the MPS probe on a machine without MPS fails 67 cases, including the 32 and #1174's 14, and turns on the six bitsandbytes skips. - GitHub's macOS 3.11 runner ran the 32 on MPS on an earlier push with the same first commit: 32 passed. - The author ran the same set on an M3 Ultra (481 passed / 27 skipped) and the 32 on an RTX 4090 D. Closes #1355.
Streamed NF4 on CUDA always dequantised the whole weight and ran `F.linear` (the #331 repair), while resident NF4 runs bitsandbytes' own dispatch, which picks a fused 4-bit GEMM at small M. On sm_120 the two paths give different results, so streamed NF4 was not bit-exact against resident NF4 there (#776: 4.88e-4 in fp16, 3.9e-3 in bf16). Under the 2026-09-24 ruling, native bitsandbytes `gemm_4bit` is the reference. What changed: - `_can_use_checkpoint_visible_nf4_gemm` makes bitsandbytes' own decision at call time: its `_gemm_4bit_custom_max_m` cap, the `K % blocksize` check and `_gemm_4bit_use_custom_fn`, with M counted over every leading dimension. If those private names move, it routes to the Function, which stays exact and may be slower. A CPU-visible test turns CI red on such a bitsandbytes release. - Where bitsandbytes picks its fused GEMM, the streamed layer runs that GEMM through a checkpoint-visible autograd Function. The packed weight and every quant-state tensor go through `save_for_backward`, so non-reentrant checkpointing recomputes them after the pool reloads the right layer. Backward checks that the slot still belongs to the layer, and refuses if it does not. - Everywhere else the dequantise + `F.linear` path stays, so training-size M pays no third dequantisation. - #841: the streamed layer caches its substitution mapping and its `Params4bit` / `QuantState` views per pool-buffer mapping. - #968's tests now pin patched == unpatched native bitsandbytes at a fused M, with a counter proving the Function ran. `test_issue385` compares against a plain bitsandbytes resident model. What users notice: streamed NF4 training on CUDA matches resident NF4 bit for bit, sm_120 included. At small M the fused arm keeps packed aliases instead of dense weights, so it uses less VRAM. Backward still dequantises, because bitsandbytes has no transposed 4-bit GEMM, so #842's throughput ceiling stays open. Verified on an RTX 5070 Laptop (sm_120), torch 2.14.0+cu130, bitsandbytes 0.50.2, on the head merged with main: - the 19 `gpu` tests pass, including `test_issue385` `[nf4]` at exactly 0.0 in fp16 and bf16; - each of the ruling's three mutations turns tests red: gate off (7), recompute through dequantise + `F.linear` (1), fused arm sent back to `bnb.matmul_4bit` (3); - with `layer_stream_runtime.py` taken from main, `[nf4]` reads 4.8828125e-04 / 3.90625e-03 again. On CPU: the six review files give 188 passed / 22 skipped, the neighbouring streaming files including #1258's give 352 passed / 6 skipped, and 7 of 7 mutations are killed. Refs #842. #841 was folded into #842 and is already closed.
Draft, branched from #1178's head (
77dc04e). #1178 has to land first, and this branch will need a rebase once it does. Review only the last three commits (2afd7d4,ca8c6e5,d9fc2a1); the rest is #1178. I have not measured whether this is faster, and I can't: it needs the 5070.This is part 2 of #1174: the zero-VRAM alternative to a second slot (option (c)), to be measured before it.
What I changed
With #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 into the shared slot.
tail_prefetchfires fromStreamPrefetcher.advance()at the last decoder layer, before that layer'sfunctional_call, so the copy hides behind one layer of compute.The embedding's bytes in the slot are dead once the lookup has run, and the lookup precedes layer 0's
advance, so I addedStreamPrefetcher.head_prefetch_layerand default it to 0, the earliest point.Nonerestores the last-layer timing. It is a plain attribute, so one process can run both timings, the waybackward_tail_prefetch=Nonegave arm A on #1178:The refill is ordered after the queued embedding lookup by the
wait_streaminLargeLayerBufferPool.load_async, the same guard #1178 relies on.Evidence (CPU, and an RTX 3050 with torch 2.14.0+cu130)
Bit-equality. Per-step gradients and the final adapter weights are
torch.equalbetween the two timings for untied SFT (also atgradient_accumulation_steps: 2), ORPO, SimPO, DPO and KTO, and for tied SFT, where there is no large pool and the switch does nothing. The same timing run twice is a control, so the comparison is not noise. I randomiselora_Bfirst, since KTO's gradients in bf16 were all zero without it and the comparison passed while checking nothing.Load counts. The real large-layer load sequence and count are identical in both timings: 5 over two steps. The head load's
previs 0 in arm B and the last layer in arm A, so the move really happens. An eval forward followed by a training step gives 2 loads after the eval and 5 after the step, in both timings.Mutations of the main change (CPU, 21-test version of the file):
advance_prime, before the lookupprime()install_streamingidx == head_layertoidx >= head_layerPool-level GPU test where the queued read is the embedding lookup (
@pytest.mark.gpu, skipped without CUDA). On the 3050 it passed 5 of 5 with thewait_streamand failed 5 of 5 with it patched out.The whole new file passes on CUDA (23 passed), and with
tests/test_issue975_backward_tail_embed_prefetch.pyit is 28 passed. On CPU:ruffis clean, andpytest -k "stream or 1049 or 975 or 1174 or docs or changelog"gives 624 passed, 36 skipped, 0 failed. mypy reports the same errors as on fix(layer-stream): prefetch embed_tokens at the backward tail, not the next step's prime (#975) #1178's file.What I checked about your two hazards (not measured)
getmoves from after layer 3 to right after layer 1's prefetch (4-layer untied fixture), so layer 2's copy now queues behind it, as you predicted. Whether that costs more than it saves needs the GPU.source.get()over 3 steps onAsyncDiskSource, on a tiny CPU fixture, three runs per arm. The move added exactly one demand miss, on the head at step 0 (arm A: 6 misses, arm B: 7-8). The decoder-layer misses were the same in both arms, and steady state was identical. This says nothing about real timing, and it is why the disk tier has to be measured. The change needs no edit to_plan_queue,_claim_slotorget.What I have not done
stalls) on both the RAM and the disk tier. If layer 2's wait grows, two other placements are worth trying, andhead_prefetch_layeraccepts them: a later layer, or a stream of its own for the head.prime()layer-0 change. I left that for its own issue.d9fc2a1).