A CUDA fallback is a property of the call, not of the tensor - #849
Conversation
|
The change itself is right, and the framing is the part I would keep even if the code changed: a boolean cannot express "this width did not fit", and the degenerate case — fail at S=1, record 1, It cannot merge as it stands: the branch carries 16 compiled objects.
This is not your mistake. If you rebase after #868 lands they will fall out of On the verification limit you named: agreed, and thank you for naming it rather than implying coverage you did not have. @ThefloorMiner's repro is the one that settles it. |
|
Follow-up: #868 is on
Worth saying plainly, because my earlier comment made it sound like your mistake and it was not: the same thing happened to me, on this repo, about ten minutes after I wrote that. I ran Once it is rebased I will merge it. The change is right and I have said so on the substance: recording S instead of a boolean, with a genuine device fault falling out as @ThefloorMiner's repro is still the only thing that can confirm the runtime behaviour, and your PR says so — which is the right way round. |
Closes the permanence half of JustVugg#767. @JustVugg's framing there: A one-off allocation failure taking a device out for good, with no way back short of a restart, is the actual defect. The 2 GB per-device headroom being too small for a realistic prompt is what triggers it; the permanence is what makes it bad. The failure this guards is almost always a scratch cudaMalloc under memory pressure, and that pressure scales with S. A boolean cannot say that: it records "this tensor is broken" when what happened is "this width did not fit". So record the width instead: if (coli_cuda_matmul(...)) { w->cuda_fail_s = 0; return; } w->cuda_fail_s = S; and admit a call when S is narrower than the narrowest failure seen. A prefill chunk that OOMs at S=512 no longer condemns decode at S=1, which is exactly the reported symptom -- "throughput collapses and does not recover until restart" -- and it recovers without a restart. A genuine device fault fails at S=1, records 1, and is never retried, because S < 1 is never true. The old permanent behaviour falls out of the general rule as a special case rather than needing a branch of its own: no retry counter, no threshold, no generation number, no error-code classification, and no change to the DLL ABI. The two operator-facing notices said "disabled" and "this run did NOT use the GPU for them". That is no longer true, so they now describe the fallback per width and say that narrower calls still try the GPU -- which is also the "say it out loud" item from the same comment. NOT VERIFIED ON A GPU. I have no CUDA device; triggering this needs a real OOM. CPU build and a -DCOLI_CUDA syntax build are clean and make test-c passes, but the runtime behaviour wants confirmation from someone who can reproduce JustVugg#767.
ccffe3d to
0830495
Compare
|
Rebased and clean — One correction, because it will bite the next person and it nearly bit me: the rebase alone did not clear them. Your first comment had it right and the follow-up was too optimistic.
Same trap as #835, where #868 covers 12 of the 16
Happy to send the one-line follow-up ( And it was my discipline problem tooThank you for saying it happened to you as well, but I have no excuse here: I used #800 is rebased and cleaned the same way. It also picked up 16 new variables that landed in |
#868 closed most of this: c/COLI_V4_UNIT_*.o covers the twelve objects the amalgamated deepseek_v4 Makefile leaves next to the sources. Four more go somewhere else. c/Makefile:895: V4_OWN_DIR = build/ownership so `make check` also writes build/ownership/COLI_V4_UNIT_{RUNTIME,CONFIG, ST,NATIVE_QUANT}.o, which no rule matched. On current dev a clean checkout plus `make check` still leaves `?? c/build/` in git status -- one `git add -A` away from the same accident #868 was written to stop, and the one I made on #849 and #800. Ignoring c/build/ wholesale rather than the four names: nothing under it is tracked, and it is a build output directory, so a rule per object would need editing every time the ownership suite grows. Worth stating because it caught me on #849: an ignore rule does not untrack what is already committed. #868 made the twelve invisible in git status while leaving them in the tree; git rm --cached is what removed them. Same shape as #835. This prevents the next one, it does not clean up an existing one.
Closes the permanence half of #767. Your framing there, which this takes literally:
The mistake in the old code
The failure this guards is almost always a scratch
cudaMallocunder memory pressure, and that pressure scales with S. A boolean cannot express that. It records "this tensor is broken" when what actually happened is "this width did not fit" — so a prefill chunk at S=512 condemns decode at S=1, on a card that has plenty of room by then.The fix: record the width
and admit a call when
!w->cuda_fail_s || S < w->cuda_fail_s.That is the whole change. What falls out of it:
S < 1is never true → never retried. The old permanent behaviour, as a special case of the general rule rather than a branch of its ownNo retry counter, no threshold, no generation number, no error-code classification, and no change to the DLL ABI —
cuda_failedhad exactly four references in the tree.The notices were lying after this change
They said
disabledandthis run did NOT use the GPU for them. Both become false once a fallback is per-width, so they now describe what actually happens and state that narrower calls still try the GPU — decode can recover even when prefill does not. That is also the "say it out loud" item from the same comment: the previous text named the mechanism, not the consequence.What this does not do
Your third item —
autoreserving a constant 2 GB where the requirement scales with S — is not addressed here. That is the trigger, and it wants a scratch model plus measurement on real hardware. This PR only stops the trigger from being permanent. #687, #766 and #759 are also untouched.Verification, and its limit
-DCOLI_CUDAsyntax build clean, zero warningsmake test-cpassesI have no CUDA device, so the runtime behaviour is unverified. Triggering this path needs a real OOM. The change is a pure branch-condition change on the C side and I have reasoned through the cases above, but reasoning is not a measurement and I would rather say so than imply otherwise.
@ThefloorMiner — you reported #767 and have the 4× A6000. The check is the original repro:
CUDA_EXPERT_GB=auto, a ~7,000-token prompt, then confirm decode returns to the GPU instead of staying on CPU until restart. If it does not, this is wrong and I would like to know.