feat: fmt=8 (fp8-e4m3) decode on the kv_b absorb path, CPU and CUDA - #1102
Conversation
77d8939 to
0282193
Compare
|
Rebased on 0282193. More coming to build out support for the FP8 checkpoint-faithful container and full logprobs instrumentation support when using the OpenAI API. |
|
Rebased onto What changed, by origin
Re-verified on
Preflight's advisory about POSIX-only test constructs refers to Authored by Claude Fable 5.1 in Claude Code, analysis in partnership with @monotophic |
0282193 to
619d51f
Compare
|
Authored by Claude Fable 5.1 in Claude Code, analysis in partnership with @monotophic Rebased onto current Files needing conflict resolution, all caused by dev motion since the last rebase:
Re-verified on @JustVugg: Checking in on the viability of this PR. Is there interest in merging the fmt=8 absorb decode path? The format itself is already in via the mint tooling; this PR closes the one path that still refuses it. I have a personal interest in having the option to preserve the published checkpoint quality for the resident spine for GLM 5.2/5.3 and also a full checkpoint quality container for a quant degradation benchmark. The quant performance/cost experiments are also the driver for my associated PRs that build out proper instrumentation for the OpenAI API endpoint. I've got everything I need integrated and working at my end so my container experiments are not blocked by this stuff sitting unmerged, but I would prefer not to be investing in an isolated fork and rebasing regularly to avoid drifting too far from what you are doing here. Steering input is welcome, my hope is that these contributions would be generally useful for anyone wanting to use Colibri for serious work, including model tuning/development and research. |
|
Authored by Claude Fable 5.1 in Claude Code, analysis in partnership with @monotophic Field evidence for the CPU fmt=8 absorb path in this PR, measured on real fmt=8 containers. Identity gate (2026-09-07, this PR's build
Control: the Repeated 2026-09-15 with this PR integrated alongside #1353 and #1355–#1357 on Not measured: CUDA runtime behaviour of the fmt=8 absorb kernels (no CUDA host in these runs; the kernels are verified by compilation and the HIP syntax lane only). Serving rate on the CPU absorb path was ~0.31 tok/s on a GB10 for we-v1, so these are identity measurements, not throughput ones. Records: F-5 gate |
619d51f to
f25ac45
Compare
|
Authored by Claude Fable 5.1 in Claude Code, analysis in partnership with @monotophic Rebased onto current One file needed conflict resolution, caused by dev motion since the last rebase: Re-verified on |
|
Reviewed, and this is close. The CPU arm mirrors One change before I merge, and it is wording rather than code. The behavioural-contract bullet says fmt=8 kv_b decodes bit-consistently with the reference on both backends. The CUDA side does not, and your own comment in Worth knowing for sequencing rather than for you to fix: the roughly seventy lines of Makefile dependency churn will conflict with most other open PRs touching that file, so this wants to land either before them or well after. |
|
Authored by Claude Fable 5.1 in Claude Code, analysis in partnership with @monotophic Agreed, and fixed in the PR body — wording only, no push.
Head is still @JustVugg: Understood on the Makefile sequencing. Knowing that this is getting review is sufficient to keep me focused on making sure this is ready to merge when it fits your broader priorities. Thanks! |
|
Heads-up on an interaction with #1356, which is in review now: #1356 and this PR both add entries to Whichever lands second, we re-merge One related note, if this PR lands first,
|
The CUDA backend needs the same 128-column block edge and block count the CPU decoder uses. Sharing them through quant.h is not possible: that header pulls in the whole CPU kernel set, which backend_cuda.cu must not compile. Move the two definitions into a minimal header both sides include, so there is one definition of the fp8 block geometry rather than a constant repeated per backend. Every prerequisite list whose translation unit reaches the new header gains it beside quant.h, in both build files. One rule, tests/test_qwen36_dnproj_batch, names quant.h but is left alone: qwen36.c does not include quant.h, so that translation unit never reaches fp8_format.h and listing it would declare a dependency that does not exist. That claim is now enforced rather than asserted. The existing Makefile prerequisite test walks only an engine's own #include lines, so a header reached THROUGH another one is invisible to it -- which is exactly fp8_format.h's shape, since quant.h includes it and no engine .c does. A new case closes the include graph for this header: drop fp8_format.h from a rule whose engine reaches it and the test names that rule. It is scoped to this header deliberately; the same closure over every header reports seven rules that predate this change, which the case documents as a separate fix rather than failing on arrival for reasons this change did not cause. Pointers to the old location (the fmt=8 entry in docs/FORMATS.md, comments in colibri.c and backend_metal.mm) now name fp8_format.h. FORMATS.md's source references drop their line numbers and cite symbols instead: those anchors went stale twice, and a stale line number reads as a verification that was not performed.
qt_addrow and qt_matvec_rows had no fmt=8 case, so an fp8-e4m3 kv_b_proj fell through to the per-row-scale tail, read t->s[row] past the end of a block-scale array and dereferenced a NULL q4. Both now decode fp8 directly, taking the block scale from [ceil(O/128), ceil(I/128)] exactly as matmul_fp8 does. qt_matvec_rows' arm mirrors that kernel statement for statement, including the f32-partial-per-block, double-across-blocks accumulation. qt_addrow is an axpy, so it mirrors the block-scale indexing only and folds the coefficient into the scale, the convention its fmt=4 arm has always used. A NaN block scale is refused by name, with the block that carried it: it poisons the whole block and every accumulator downstream of it, and these functions cannot repair it. A ZERO block scale is not refused -- it is valid data. Block scales are amax/448, so an all-zero block legitimately carries one, and decoding it as zeros is the correct answer; unlike on the GPU there is no table here that could be unwritten, because the CPU decoder reads e4m3 from a compile-time constant. The check costs one branch per block, not per element, and both properties are pinned: a NaN scale must refuse through either function, a zero scale must decode to zeros through both. The format guard below the new arms keeps its condition; only its message changes, to say that fmt=8 now returns above it. layer_cuda_shard_kvb gains a format allowlist. fmt 5/6/7 previously computed an int4 row stride against a group-scaled layout, took a non-NULL q4 and uploaded garbage silently; they are now refused by name before any pointer or stride is used, with a notice bounded to one line per format per process.
weight_at and absorb_scale gain fmt=8 branches so the attention absorb path decodes fp8 on the GPU, with the block-scale geometry of the dense fmt=8 matmul and of matmul_fp8. The surviving 128 literals are pinned to the shared constant by a file-scope assertion. The e4m3 table is per-device while the flag that gates fmt=8 uploads is process-wide, so the two must not drift. coli_cuda_shutdown clears the flag; coli_cuda_init never writes it. That is sufficient, and this states the mechanism rather than asserting the conclusion: init will not rebuild contexts underneath a live device set. A re-init naming the same set returns success and leaves the contexts -- and the table published to them -- untouched; one naming a different set is refused before anything is rebuilt. So the set cannot widen past what the last publish covered without passing through shutdown. The two decisions that gate rests on are factored into pure predicates in backend_cuda.h, which the backend calls at the real sites rather than keeping a second copy. tests/test_cuda_lut_gate.c pins them, and the resulting state machine, on a plain CPU build with no GPU and no CUDA toolchain: previously nothing about this gate ran anywhere except on a CUDA host, which is how three sites in the tree came to assert init semantics that no longer held. The lifecycle test in tests/test_fp8_cuda.cu now asserts all three edges, including that a same-set re-init keeps the flag -- requiring a republish there would force one that buys nothing. absorb_scale's own contract comment named fmt=4 as the only format without a per-row scale; it now names fmt=8's per-block layout as the other exception, since this change adds that branch inside the same function.
…ession test_fp8_serve_batch_e2e.py pins the fmt=8 kv_b batched-serve decode end to end against a real container, and is a named SKIP when none is present. Its child environment is allowlist-scrubbed: ambient engine knobs are stripped, read-only model-location settings pass, documented legacy aliases are stripped alongside their primaries, and the ratified non-absorb arm passes value-restricted. The CUDA absorb path is opt-in: the test INJECTS the exact engine bundle that path needs rather than letting it arrive ambiently, then witnesses the engine's boot report and fails outright on the resident-dense-on-CPU line, so a misconfigured lane cannot bank a vacuous green. That witness caught a first attempt which value-allowed the bundle through the scrub instead of injecting it. test_fp8_refusal_canary.py runs in CI without a container: it pins that each absorb function keeps at least one refusal matching the end-to-end matcher -- existential on purpose, since an additional differently-worded guard is new coverage rather than drift -- and that the matcher stays selective against both a synthetic near-miss and the real in-tree sibling. test_e8x4g64_mint_load.py and test_e8x4g64_loader.c pin the container class through the real conversion CLI and the real C loader at toy scale, plus the duplicate-tensor-name refusal on a tool-produced container. The C harness takes a container directory on argv, so it is excluded from the gate list and driven by the Python test; it gains a Makefile rule anyway, so that it compiles under the suite's own flags instead of only the driver's hand-copied list, which is what its no-warnings assertion had been vouching for. The driver no longer swallows a failed OpenMP probe on macOS: silently compiling a different binary from the one the Makefile builds is what made that assertion misleading. The external-files table in the repack tool now cites symbols instead of line numbers. Those anchors had rotted twice while the table claimed to be verified against the current tree, and a stale line number reads as a verification that was not performed.
f25ac45 to
ea8a7e4
Compare
|
Rebased onto
I dropped the init-side reset. Keeping it would have left a comment that is false on the merged tree. The shutdown-side reset stays, and its comment now names Until now that argument lived only in comments, and when Second place: On the Two things did change in the CPU arms on re-reading them: a NaN block scale is now refused by name, with the block it came from, instead of propagating through the accumulator; and a zero block scale is explicitly kept valid and decoded as zeros, because block scales are Gates: CPU decode-identity check, re-run on the rebased binary (direct CUDA, on a GB10 with CUDA 13: the One thing is yours to call. Authored by Claude Opus 5 in Claude Code, analysis in partnership with @monotophic |
…g test comment Five accuracy items from the r7 audit (AUDIT_1102_r7_ea8a7e4d.md, findings F1-F5), none touching the decode path. docs/FORMATS.md's Metal fused-gate paragraph (F1) still named a colibri.c line number for each of five symbols and cited a non-upstream branch (kvb/fmt-gate-notice-r4) as its source of truth, even though the "Sources for all rows" table just below it had already been de-anchored. All five are now symbol references in the same style as that table, and the branch name is gone. repack_fp8_passthrough.py's EXTERNAL-files table (F2) de-anchored its line numbers everywhere the preamble above it promises, except one: the Engine.__init__ reference still carried a line number (previously wrong per r6 F4, and edited to a different wrong number in r7). It is now a bare symbol reference, matching the rest of the block. tests/test_e8x4g64_loader$(EXE) (F3) had a rule and a comment claiming it compiles under the suite's own CFLAGS so its warnings are caught, but the rule was unreachable from every target -- test-c, test, check, and all -- since TEST_EXCLUDE drops it from TEST_BINS and nothing else names it as a prerequisite. Added e8x4g64-loader-check, a phony build-only target shaped like qwen38-tier-engine-check, so `make e8x4g64-loader-check` actually builds it under those flags on demand. check itself is untouched: the new target is not a prerequisite of check/test/test-c/all, so this adds no time to `make check`. test_qt_addrow.c's block-scale comment (F4) said a ZERO fmt=8 scale is "refused by name" alongside NaN. It is not: colibri.c is explicit that a zero scale is valid and must decode to zeros, and test_fmt8_zero_scale_decodes_to_zeros twenty lines below already asserts exactly that. The comment now describes only the NaN guard the two calls below it exercise, and points at that assertion for the zero case. fp8_format.h (F5) is added beside quant.h/backend_cuda.cu on two of the three Makefile rules the audit named: tests/test_kimi_cuda_expert$(EXE) (kimi_k3.c includes quant.h directly, which pulls in fp8_format.h) and tests/bench_cuda_resident_batch$(EXE) (backend_cuda.cu -- a separate translation unit built into the same binary -- includes fp8_format.h directly, a miss no quant.h-based framing could see). The third, tests/test_qwen36_dnproj_batch$(EXE), is deliberately left alone: full preprocessing under both its build variants (plain and the -DCOLI_DNPROJ_REAL_CUDA alternate) confirms qwen36.c never reaches quant.h or fp8_format.h at all, exactly as 6826c00's own commit message already documented when it introduced fp8_format.h. The audit's framing of this rule as a regression does not hold up under that check; the pre-existing quant.h entry on that rule is itself already a no-op and is not this change's to fix. Not in scope: the fmt=8 NaN exit(1)-inside-omp policy, disclosed in the r7 thread note and left for a later round.
|
Pushed two small commits on top of the previous head. A review pass I ran after the last push found five accuracy problems in the documentation and build rules, none in code paths: One thing worth naming rather than changing: the NaN block-scale refusal exits the process from inside the OpenMP region, where |
…get.h (JustVugg#1712) next to this branch's header
|
Merged current dev into your branch: the only conflicts were Makefile prerequisite lines (dev added serve_budget.h in #1712 and the build-flags stamp in Makefile.deepseek-v4 in #1707), resolved as the union with fp8_format.h kept. On the merge, colibri builds without warnings and test_qt_addrow, test_shard_kvb_refuse, test_cuda_lut_gate, test_cuda_fmt_guard, test_makefile_deps and the V4 build-flags test pass. The summary now matches the CUDA comment, thank you. On the NaN block-scale refusal: accepted as policy, a poisoned scale on a format that was refused outright until now should stop loudly. Merging once CI is green; it goes into 1.12.1. |
…ot.h and the new CPU-only test rules next to this branch's sse41_kernels.h and SSE4.1 gates
2 participants
Authored by Fable 5.1 in Claude Code, analysis in partnership with @monotophic
Context. This PR is one of four independent contributions derived from a single locally-verified working tree (fp8 container support, scoring-evidence tooling, server API hardening, and batched group scoring). This one carries the fp8 container line: the fmt=8 absorb decode path, the mint tool's tests, and their end-to-end test. It stands alone — nothing else in the set needs to land for this to be complete, and merging or declining the others does not affect it. The others will be proposed separately, each with its own evidence.
Checkpoint-faithful FP8 containers stamp
kv_b_projas fmt=8 (raw e4m3 bytes + one f32 scale per 128×128 block). The absorb decode path had no fmt=8 branch: CPUqt_addrow/qt_matvec_rowsrefused loudly, and the CUDA absorb gate refused before any kernel ran — an FP8 container could load but not decode attention. This adds the fmt=8 branch to both backends, mirroringmatmul_fp8's block-scale indexing exactly, plus an explicit named skip (with the absorb path called out) where kv_b GPU sharding legitimately cannot serve fmt=8. The block geometry (FP8_BLOCK,fp8_nblk) moves to a shared header,fp8_format.h, so the CPU branch, the CUDA kernels, and the tests read one definition site.The dense fmt=8 matmul path already exists on
dev; this PR's own new content is the kv_b/absorb decode arms (CPU and CUDA), the shared block-geometry header, and their tests. Platform coverage at this head: CPU is native for both dense and absorb fmt=8. CUDA is native for both and is device-proven end-to-end on GB10: the two-slot batched-serve CUDA-absorb arm passed underCUDA_DENSE=1+COLI_CUDA_ATTN=1, with the engine's own boot line[CUDA] mode: routed experts + resident dense tensorscaptured as a positive witness (kv_b CUDA-eligible, absorb path selected); the same test fails, on the same hardware, when that boot line readsresident dense on CPU. Metal has a real, unit-tested fmt=8 decode kernel that no live dispatch path calls, so every fmt=8 matmul on a Metal build continues to fall back to CPU by existing dispatch design — unchanged by this PR; Vulkan has no fmt=8 arm and continues to refuse it at upload, also unchanged. Two other engines in this tree (glm53.c,kimi_k3.c) share the identicalkv_b_proj-absorb attention shape and are attachable to the same feature but are not wired to it — a separate follow-on, not a gap here.Behavioral contract
layer_cuda_shard_kvbrefuses un-shardable kv_b formats BY NAME (notice + skip; the absorb path serves them) instead of proceeding on a NULL pointer by accident — and the same allowlist closes two pre-existing silent misreads (fmt=5 and fmt=6 computed a wrong row-byte stride with a non-NULLq4).f0f8dfc4), so the device set cannot widen past the last publish without passing through shutdown; the process-wide-flag-vs-per-device-table hole is closed by that pair. The two decisions are pure predicates inbackend_cuda.h, called at the real sites, and a host-side test pins the lifecycle with no GPU: shutdown clears, a same-set re-init keeps, a different-set re-init is refused.make checkstanding alone.Scope note @JustVugg — observability deliberately left out. During verification we used a temporary log line to prove the absorb branch executes on the serve path, then removed it to keep this change minimal; the end-to-end test now captures the engine's own boot-mode line instead. A permanent, env-gated absorb-path debug line is a reasonable follow-on if you'd find it useful — happy to add it to this PR or a follow-up at your request.
Capstone matrix (evidence as originally re-run on 2026-09-03; the branch was since re-derived onto
dev9e152d49asea8a7e4d, and the evidence re-run at that head — per-commit gates, the HIP lane, the CPU decode-identity check on two containers, and the CUDA arms on a GB10 — is in the comment of 22 September)qt_addrowbit-exact;qt_matvec_rows≈6e-8 relative (float accumulation order)make cuda-teston GB10: the fmt=8 absorb-kernel oracle, the fmt trap test, and the LUT-lifecycle pin all pass (oa-lane/results/spark1/cells/cuda_test/)row_bytessweep, with a negative control (wrongngformula fails as expected)BITE CONFIRMED: qt_addrow: unsupported fmt=8 …(oa-lane/results/{strix1,spark1}/cells/cell_old/)colibri.c: 2 FAILED at exactly the fmt=8 refusal pins, fmt=6 pins unchanged; the CUDA sizing probe's negative control fails on a wrong block-count formulaCUDA=1shard-refuse guard run on GB10:layer_cuda_shard_kvb refusal probe: ok(oa-lane/results/spark1/cells/shard_kvb_refuse/)routed experts + resident dense tensorscaptured; the negative twin (CPU-dense boot line) fails the same test on the same box (oa-lane/results/spark1/cells/cell_new_cuda_absorb/,…/negative_polarity_ce501dd/). The witness proves CUDA eligibility and path selection; kernel execution follows from the source-verified dispatch chain (no silent-fallback branch between selection and launch) rather than a separate artifacttest_cuda_lut_gate(host-side, no CUDA toolchain) pins all three edges — shutdown clears, same-set re-init keeps, different-set re-init refused — and the same edges ran on a GB10 intest_fp8_cuda's lifecycle phaseea8a7e4d, where the lane now also builds and runs the shard-refuse test against the CUDA backend — see the comment of 22 Septembermake METAL=1: zero warnings at every commit in the series;metal-testok under bothCOLI_METAL_RESSETstatesFuller matrix, review record, and origin accounting
Review record: four bisectable commits (shared header → CPU engine → CUDA engine → tests), each built from
make cleanand passing the full suite standing alone; each commit reviewed by an independent blind validator and a deep auditor, with fix rounds closed at primary source; the assembled program then passed a program-head deep audit and a final pre-push verification. One review finding is worth naming: an earlier version of the CUDA-absorb end-to-end test went green without ever reaching the CUDA absorb path (the eligibility knob was stripped by the test's own env hygiene). The test now injects the knob, asserts the engine's boot-mode line, and echoes what it observed on pass and fail, so that class of vacuous green cannot recur.Origin accounting for a re-derive onto current dev: the decode branches and CUDA arms are byte-identical or offset-only carries of the previously reviewed content; two pre-existing defects found during re-derivation are fixed here and disclosed above (the fmt=5/6 shard-allowlist misreads, and a missing
fp8_format.hdependency inMakefile.deepseek-v4that left five DeepSeek-V4 objects stale against a block-size edit); the LUT re-init hole an earlier revision closed on the init side is now closed by dev's own init refusal together with the shutdown-side reset this PR keeps, pinned by the host-side lifecycle test and by the GB10 one; a few stale line-number anchors indocs/FORMATS.mdand the repack tool's docstring were corrected while those files were open. The end-to-end test and the refusal canary diverge from the earlier hardened lineage by exactly the reviewed hardening described above.The mint→load regression test is armed only where torch/numpy/safetensors are installed (it names its skip otherwise); upstream CI does not install torch, so it SKIPs there and runs on our fleet. Compiled-binary digests for the CUDA build varied run-to-run on identical sources (nvcc non-determinism; the gcc build was byte-stable) — disclosed as a reproducibility note, not a behavior.
The fmt=8/fmt=6 scale-byte VRAM accounting interaction disclosed in the original submission was fixed separately in #1100 (merged 2026-08-19), which is in this PR's base; the diagnostic counters are correct here without further change.
Style: changed lines were held to the file's measured local idiom via a diff-scoped consistency check; no surrounding code was reformatted.
Durable vs current-state: decode branches, refusals, and the LUT gate are durable; GB10/sm_121 timings, skip counts, and warning counts are current-state (2026-09-03 at base
387653f; re-derived 2026-09-22 onto base9e152d49asea8a7e4d, evidence in the comment of 22 September).