Repository navigation
feat(stream): make NF4 fused forward checkpoint-visible (#842) - #1143
MakazhanAlpamys merged 6 commits into
Conversation
MakazhanAlpamys
left a comment
There was a problem hiding this comment.
Advisory review of the draft at c326a396. Keep it in draft. The mechanism is sound, but the largest effect is missing from the description.
What actually changes. On CUDA, install_dequant_forward no longer dequantises: its forward now runs bitsandbytes' own gemm_4bit dispatch, the same one native MatMul4Bit runs. So the streamed NF4 reference moves from "dequant + F.linear" (#331 / #857) to "native bitsandbytes". #842's own "Honest limits" section says that change should be decided deliberately, not arrive as a side effect. The PR also deletes #857's warning ("not assumed to be numerically free") at the moment it makes the switch.
Measured on an RTX 5070 with a 4-layer / 2-buffer streamed model, bit for bit:
- With the PR: streamed == native resident NF4, for the logits and every LoRA gradient, in bf16 and fp16, at seq 12 and 128.
- On
main: streamed != native at seq 12 (logits 4.883e-3 / grads 1.826e-3 in bf16).
So the #776 symptom disappears because streaming moves onto bnb's native dispatch, not because anyone settled which path is right. I will give a ruling on that choice separately. Please don't build further on either answer until then.
1. Blocking: two CUDA tests go red. test_issue968::TestTheTwoBitsandbytesFourBitPathsDisagree::test_the_paths_diverge_by_the_measured_magnitude[float16|bfloat16] fails with "the two bitsandbytes 4-bit paths now AGREE … assert 0.0 > 0.0", on this head and on the tree merged with main. Clean main gives 167 passed / 0 failed on the same files. #968's patched arm is resident + install_dequant_forward, which is now the same dispatch as native. CI cannot see this, because the runners have no GPU. There are two ways out:
- Accept the new reference. Then #968 needs rewriting, and these still say the streamed path always dequantises:
test_v07202.py:1916,test_v07300.py:35("WHY THIS IS NOT A NUMERICS CHANGE"),:233and:296. - Keep
install_dequant_forward's arithmetic, and put the Function on its own path.
2. Blocking: the tests do not pin the CUDA change. All 6 new tests are CPU-only. On CPU, gemm_4bit is bnb's default dequantize_4bit + F.linear, so "fused vs dequant exact parity" compares the old arithmetic with itself. At 16x16, bnb falls back even on CUDA (K % 64 != 0). Re-run on CUDA at fused shapes (64x64 and 4096x4096, M=3), parity is False, with a bf16 forward gap up to 1.25e-1. Three mutations survive:
- Gating the new route off entirely: the CUDA files get greener (173 passed / 0 failed).
- Recomputing with dequant +
F.linearduring checkpoint recompute: a gradient probe shows LoRA grads off by 1.28e-3 (bf16, seq 12), and nothing in the repo notices. - Routing the CUDA branch back to
bnb.matmul_4bit, the #331 route itself:TestStreamedNF4AvoidsMatMul4Bitbuilds a CPU model, where that branch is unreachable. A counter on a CUDA streamed step readsmatmul_4bit= 40 under this mutation, against 0 on the PR.
Needed as gpu-marked tests:
- (i) the
matmul_4bitcounter on a CUDA streamed model, plus a counter on the Function path; - (ii) recycled-vs-private gradient comparison through the same Function at a fused shape (K a multiple of 64, M inside the fused window), with the plain-
ctxarm that must fail; - (iii) streamed-vs-resident logits and LoRA grads at a fused M.
3. Blocking: it is slower at training shapes, on the RAM tier #842 targets.
Dequantisations per checkpointed forward + recompute + backward of one 8B q projection (bf16):
- 3 on the PR vs 2 on
mainat M=512 and 2048; - 1 vs 2 at M=16 and 64.
The fused window on this card: for 8B, q / o / down up to M=96, k / v up to M=8, gate / up up to M=256; for 70B, every projection up to M=256.
Timing one checkpointed 7-projection decoder region (indicative only, the card was shared at the time), new/old:
- 8B: 1.16-1.22 at M=16, 0.88-0.94 at 64, 1.03-1.19 from 128 to 2048;
- 70B: 0.82 / 0.88 / 1.02 / 1.08 at M=16 / 64 / 256 / 512.
Peak VRAM for the same region goes down: 8B by 260-301 MiB, 70B by 1.08-1.18 GiB. The Function saves packed aliases, where the old path held every dense W from recompute to backward; that is plausibly most of #331's +650.9 MiB. So this trades time for VRAM. It needs #842's gate, or it should route through the Function only where bnb picks the fused kernel, with the trade stated.
What holds:
- New == native bnb bit for bit at every shape, M and dtype, and dX is identical across new, old and native.
- The fused kernel is deterministic.
- Recompute and the original forward take the same bnb path, because the dispatch is a cached pure function of device, dtype and shape. So the shipped code has no "gradient for a different function" defect.
- The #841 per-layer view cache is correct and pinned (its test fails on
mainatassert second is first), and bf16 is now covered.
A new invariant worth writing down. The Function saves pool aliases and dequantises them in backward. Gradients are right only because checkpoint recompute calls pool.wait() first, and the prefetcher is 1-ahead with n >= 2, so no slot is refilled between a layer's recompute and the end of its backward. A deeper lookahead would break this silently. Add a comment, and an ownership assertion in backward.
Once it leaves draft it needs a changed fragment and docs. docs/performance-and-quantization.md:343 says streamed NF4 is bit-exact vs resident "through the same bitsandbytes kernels"; it must say which path CUDA streaming takes, and state the VRAM/time trade. None of #842's acceptance criteria are met yet, as the draft says.
Nits:
backwardcomputesgrad_xandgrad_biasregardless ofctx.needs_input_grad.- The gate does an op lookup on every call.
- The Function is not autocast-aware (
custom_fwd).
|
The ruling I said I would give separately. The reference for streamed NF4 is native bitsandbytes It does not merge until these three are done. (a) Remove the slowdown at training sizes. At training M the Function adds a third dequantisation per checkpointed step (3 vs 2 at M=512 and 2048), and my 8B region ran 3-19% slower from M=128 up (indicative, on a shared card). Route through the Function only where bitsandbytes itself picks the fused kernel. Everywhere else bitsandbytes runs
(b) CUDA tests. Mark them
Between them these must kill the three mutations from my review: the counter catches the (c) Rewrite the #968 tests against the new reference. The rest of my review still stands: the slot-ownership assertion in |
c326a39 to
8158f84
Compare
|
Implemented the maintainer ruling and remaining review items at The Function now runs only when bitsandbytes itself selects the custom GEMM; training-M fallback stays on the old dequant+linear path. Missing private dispatch symbols fail safely toward the Function. Slot ownership, autocast decorators, Local non-GPU validation: 133 passed, 11 skipped plus the rewritten #968 GPU module collected as 8 skipped; Ruff and diff-check clean. The PR stays draft until the RTX 5070 acceptance run is performed. |
|
@Amix29 thanks for turning the ruling around this quickly. The routing is what the ruling asked for. Here is what I checked at What is good, by execution
Tests I ran
Please fix before the GPU run
What the ruling means for this PR now
Your description also lists #842's own measurements (throughput, peak VRAM, the large-layer alias control) as still open. Those belong to the issue's acceptance criteria. The required checks on |
|
Addressed the 2026-09-25 review at head 8ea2f2d, rebased onto current main. The fragment now satisfies the release validator and credits #842/#1143; #385 compares streamed NF4 with native resident bitsandbytes; both private dispatch symbols are pinned on CPU-visible CI where the CUDA ops module imports; a CPU routing grid pins dispatch/max-M/alignment; and the GPU streamed-vs-resident test derives its expected route from bitsandbytes on the actual card. Local validation: 181 passed, 24 expected platform/GPU skips; Ruff and git diff --check clean. No CUDA result is claimed from this Mac. |
8158f84 to
8ea2f2d
Compare
|
@Amix29 thank you. This round closes the three items from my review of
I ran everything below at Tests I ran
Mutations: 19 against those files, 12 killed
Three gaps worth closing on CPU before the card run, so the card only has to run the final head once. None blocks the draft.
A cosmetic fourth: the nested What remains before this leaves draft
|
8ea2f2d to
a62fae0
Compare
a62fae0 to
433b35a
Compare
|
Closed the three CPU gaps from the latest review at
Local M4 Max validation: 185 passed, 24 expected platform/GPU skips across the six focused files; the #842 module alone is 15 passed, 5 skipped. Ruff, preview E30, and |
|
@Amix29 thank you.
I also checked the rebase. Everything below ran at Tests I ran
Mutations: 20 against those files, 18 killed
One optional addition: the check's refusing branch. Your test proves that the check is attached and stays silent for the right owner. Nothing yet proves that def test_the_production_owner_check_refuses_a_recycled_slot(tmp_path):
from test_v07202 import _nf4_stream
import soup_cli.utils.layer_stream_runtime as runtime_module
model, runtime, _, _, _ = _nf4_stream(tmp_path, n_layers=4, buffers=2)
layer = runtime_module.decoder_owner(model).layers[0]
slot = runtime.pool.slot_for(layer.idx)
runtime.pool.owner[slot] = layer.idx
layer._assert_nf4_slot_owner() # the right owner: silent
runtime.pool.owner[slot] = layer.idx + 2 # layer 2 has refilled the slot
with pytest.raises(RuntimeError, match="recycled weight slot"):
layer._assert_nf4_slot_owner()CI. The required checks on What remains before this leaves draft. Nothing more is needed from you unless you want to add the test above. The rest needs the RTX 5070, not your Mac:
If you do add the test, please add it before that run, so the card only runs the final head once. |
|
Added the optional production refusal pin before the CUDA run at The new test builds the real 4-layer / 2-buffer NF4 stream, calls the production Local M4 Max validation:
The PR body is refreshed. The draft now has no remaining CPU-side review item; the RTX 5070 acceptance run remains external, and no CUDA result is claimed here. |
|
@Amix29 thank you. I checked it by breaking the code, not by reading the test:
Every mutation was undone, and Tests I ran (Windows, CPU with CUDA hidden, bitsandbytes 0.50.2):
CI. The required checks on What remains. Nothing CPU-side. This PR now waits only for the CUDA acceptance run on the RTX 5070, on this head merged with |
|
@Amix29, both GPU gates are done, on
Every mutation turns at least one G1 test red, which was the bar. One thing worth knowing: exactly one of the 19 tests sees the recompute mutation. From my side this is ready to merge. Mark it ready for review when you want it in, and it merges once the required checks are green on that head. #1258 still overlaps this PR in |
|
@Amix29, thanks for marking this ready. #1258 merged first ( |
e7e6476 to
e1bcb66
Compare
|
Rebased on current Focused validation across the #842/NF4 compatibility files: 166 passed, 23 expected GPU/platform skips. Full Ruff and |
MakazhanAlpamys
left a comment
There was a problem hiding this comment.
@Amix29 thank you for the quick rebase. I checked it before running anything on the card:
git range-diffagainst the pre-rebase series shows five of your six commits unchanged. The sixth differs only in the docs context line, because #1258 rewrote the "How it works" paragraph that your NF4 paragraph follows. Your paragraph still reads correctly after it.- The PR's diff over
src/,tests/andchangelog.d/has the same stable patch-id before and after the rebase. So the code, the tests and the fragment are byte-for-byte what ran on the card on 09-27. #1258's change tolayer_stream_runtime.py(the head-prefetch timing) does not overlap your hunks.
On the RTX 5070 (sm_120, torch 2.14.0+cu130, bitsandbytes 0.50.2), on e1bcb669 merged with current main (dccc8680). The merge is clean.
- G1, the 19-item
-m gpucommand: 19 passed, none skipped. That includestest_issue385's[nf4]cases at exactly 0.0 in fp16 and bf16, and your 4-layer / 2-buffer streamed-vs-unpatched-resident test at the fused and the training M. - G2, one mutation at a time, each restored byte for byte afterwards:
- gate off: 7 failed;
- recompute through
dequantize_4bit+F.linear: 1 failed, again only the[fused-m]streamed-vs-resident test. The logits still match, and layer 0'sq_proj.lora_Agrad differs, -1.3513e-06 vs -1.3619e-06, the same digits as before the rebase; - fused arm sent back to
bnb.matmul_4bit: 3 failed, caught by thematmul_4bitcounter (52 calls where 0 are allowed) and the Function counter.
- Without the fix, meaning
layer_stream_runtime.pytaken frommainwith your tests kept,test_issue385[nf4]reads 4.8828125e-04 in fp16 and 3.90625e-03 in bf16. Those are the #776 numbers exactly, so on this card your change is what brings them to 0.0.
On CPU (Windows, CUDA hidden), on the same merged tree:
- The six review files (
test_issue842,test_v07300,test_v07202,test_issue968,test_issue385,test_issue487): 188 passed, 22 skipped, the same 210 tests as before the rebase. - The streaming files around it: #1258's two
test_issue1174_*files,test_issue975…, bothtest_issue379_*files (they driveinstall_dequant_forward),test_issue361_nf4_throughputandtest_v07200. 352 passed, 6 skipped. - Seven mutations on the rebased code, all killed: the production owner check that never refuses, the owner-check attachment deleted,
save_for_backwardreplaced by plainctxattributes, the view cache never hit, the gate off, M taken fromx.shape[0], and the fragment withoutin #1143. Each was undone, andgit statuswas empty afterwards. - Ruff and
git diff --checkare clean. Your new test file fails at collection onmain, as it should.
This is ready to merge. It now waits only on the 13 required checks, which have been queued since 14:53Z and have not started yet. It closes no issue: #841 is already closed as folded into #842, and #842's own measurement criteria stay open there.
|
Merged as |
Refs #842
Refs #841
What
This PR implements checkpoint-visible streamed NF4 and folds the #841 pooled-view cache into the same branch. It follows the maintainer ruling that native bitsandbytes is the numerical reference.
dequantize_4bitplusF.linearwhen bitsandbytes selects that fallback;save_for_backward, restores autocast state, skips unneeded gradient work, and asserts stream-slot ownership before backward reads pooled aliases;Params4bitandQuantStatesubstitution views by pool mapping identity without retaining another weight copy;Validation after rebasing on #1258
git diff --checkclean.CUDA boundary
On the pre-rebase head, the maintainer's RTX 5070 G1 suite passed 19/19 and all three G2 mutations were killed. Because the requested rebase on #1258 rewrote
layer_stream_runtime.py, those same G1 and G2 gates must be rerun on the current head before merge. No CUDA result is claimed from this M4 Max.The backward still dequantises because bitsandbytes exposes no deterministic transposed 4-bit GEMM, so the full no-dense-W ceiling remains open at issue level.