Repository navigation
gguf, model: gguf-inventory refuses a top-k above the expert count by its keys - #153
Conversation
… its keys The routed bytes per token are the sum over MoE blocks of exps bytes * top-k / expert count. The sum ran in u128 and both reports cast it to u64. A top-k above the expert count selects more than every expert, so the sum can pass the tensor bytes: a header with expert_count 1, expert_used_count 2 and one F32 tensor of dims [2^61, 1] in block 0 gives 2^64, which stdout and Markdown printed as 0 with exit 0, in debug and release builds alike. gguf-inventory now stops with exit 1 before any metric when expert_used_count is greater than expert_count, naming both keys with their values. A header with expert_count 0 and a top-k above 0 is refused the same way. With top-k at most the expert count, each block adds at most its exps bytes, so the sum is a part of the total that known_bytes already checks and fits u64. routed_per_token is a u64, built with one try_from, and the two reports print it without a cast. The two other u128 products narrowed with as u64 in the model crate, CallAcc::scaled and the per-type share in record_rows, go through u64::try_from with the reason in expect, as docs/rust-quality.md R5 asks. Their factor is a part of a whole, so the share is at most ns. The test runs the binary on a synthetic header with both reports requested: top-k 2 over 1 expert and top-k 1 over 0 experts exit 1 with the message and write neither report; top-k 2 over 2 experts reports 2^63 bytes in both, and top-k 0 over 0 experts reports no routed figure. The synthetic header comes from the shared write_gguf_kvs.
github-actions
Bot
added
the
model
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: CLEAN: The guard prevents routed-byte overflow, and the profile conversions preserve valid time shares.
Non-blocking
⚠ Linux profile evidence: crates/model/src/ops.rs:2020. The PR reports cross-compilation but no runtime model tests. Attach just gate-profile output from x86_64 Linux to exercise record_rows and CallAcc::scaled during profiled decode.
Checked
Duplicates: 3 new functions checked; no duplicated owner. Dryer reported no pairs.
Complexity: crapper flagged no function; the metadata guard adds one error branch.
Silent failure: 4 sites followed. Invalid top-k exits 1 before output; valid sums and profile shares fit u64; unavailable metrics report n/a.
Tests: the regression protects crates/gguf/src/bin/gguf-inventory.rs:294. Removing the guard or excluding zero experts fails the exit-code assertion; changing > to >= fails the equality success assertion. The PR supplies debug/release FAIL-first output for the first two mutants.
Gates: no pins or bands moved; one synthetic regression added.
Performance: inventory is a tool path; model conversions run only during profiling and add no allocation, lock, copy or launch.
Adversarial: maximum u64 counts keep the product within u128; exact and partial sums remain bounded by checked total bytes. The deepseek2 and qwen3moe metadata readers reject excessive top-k.
Escape catalog: no patterns matched. No ledger lessons are in force.
Notes
Static review only; no builds or tests were run during this review.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
|
On the non-blocking Linux profile item: |
|
The review is done and the contribution check passed, so I'm merging this. |
1 participant
What and why
Closes #152.
gguf-inventoryreports the routed bytes per token as the sum over MoE blocks ofexps bytes * top-k / expert count. It computed the sum inu128and both reports cast it tou64(routed_per_tokenincrates/gguf/src/bin/gguf-inventory.rs, in the stdout report and in the Markdown table). A top-k above the expert count selects more than every expert, so the sum can pass the tensor bytes and then passu64. A header withexpert_count1,expert_used_count2 and one F32 tensor of dims[2^61, 1]namedblk.0.ffn_up_exps.weightneeds 2^63 bytes for the tensor and 2^64 bytes per token. Both reports printed0, and the command exited 0. A debug build did the same, becauseasdoes not check.The change, one commit:
known_byteshas summed the tensors, and before any metric is computed, the command stops with exit 1 whenexpert_used_countis greater thanexpert_count. The message names both keys and their values:gguf-inventory: deepseek2.expert_used_count 2 is greater than deepseek2.expert_count 1. This is the cause of the overflow: a top-k above the expert count has no meaning, so the report has nothing true to print for it. A header withexpert_count0 and a top-k above 0 is refused the same way; before, it printedn/afor the routed bytes.bytes * k / ec <= bytes). The sum over blocks is then a part of the total thatknown_bytesalready checks (gguf, engram, model, gpu-gates: a tensor whose bytes or data range pass u64 is refused by name #149), so it fitsu64.routed_per_tokenis now au64built with oneu64::try_from, and the two reports print it without a cast. I usedexpectthere, with the bound in the message, because a failure would mean the guard above it is gone; the FAIL-first run below shows what that looks like.routed_per_tokenstates the bound, and the module doc says the refusal.CallAcc::scaledincrates/model/src/profile.rsand the per-type share inrecord_rowsincrates/model/src/ops.rsnarrowed au128product withas u64. They go throughu64::try_from(..).expect(..)now, as R5 indocs/rust-quality.mdasks. See the next section for why this is not a fix of a reachable overflow.I did not add a second check at the
routed_per_tokenconversion. The bound holds for any top-k the guard lets through, so a checked conversion that returns an error would be a branch that nothing can reach.Same defect elsewhere
routed_per_tokenwas the only narrowing cast of a header-derived byte count left ingguf-inventory. The rest of itsascasts are widening (u64andu32tou128), a float conversion for theGiBcolumn (gib), or in the test that sizes one block of each type. The byte sums are parts ofknown_bytes, as #149 states. I searched the other crates foru128and forexpert_used_countconsumers: none of them narrows au128byte count. Other crates do sizen_usedexperts (for examplecrates/gpu/src/arch/deepseek2/dispatch.rs:1096andcrates/placement/src/placement/workstation.rs:341), but from a loaded model's shapes withu64arithmetic, and I did not change them.The two
as u64after au128product in the model crate scale a profile's nanoseconds by a part of a whole: the factor is at most 1, so the result is at mostnsand no header value reaches them. They were not a bug. I changed them anyway so that no narrowing cast of au128product is left in host code, which R5 asks for, and theexpectmessages state why the conversion cannot fail.record_union_rowsin the same file already has this form.bloomery-modeldoes not build on macOS (bloomery-threadsuses Linux-onlylibcaffinity calls), so a test on the Mac cannot run these two lines. The lint step ofjust mac-staticcompiles them forx86_64-unknown-linux-gnu, and I did not run the model crate's tests on a Linux node.Code shape
u64.u128result type is gone; nothing else was made unused.cargo fmtandcargo clippyare clean on what I touched; the lint count did not go up.Duplication found or removed (
path:line), or "none": none added. The test header writerwrite_gguf_kvsincrates/gguf/tests/inventory.rstakes the metadata list, and bothwrite_ggufand the newmoe_headercall it.What I ran
The Mac runs are on an Apple M4 Pro (macOS 27.0.1,
rustc 1.100.0-nightly (e457a7b0d 2026-08-27)).just mac-staticran on an Apple M5 with the same rustc.The test is
a_top_k_above_the_expert_count_is_refused_by_nameincrates/gguf/tests/inventory.rs. It writes the header above (moe_header) and runs thegguf-inventorybinary with--markdown. It checks that top-k 2 over 1 expert and top-k 1 over 0 experts each exit 1 with exactly the message above, print nothing on stdout and write no Markdown file. It then runs top-k 2 over 2 experts and checks that both reports show 2^63 bytes (9,223,372,036,854,775,808) as the routed bytes per token, so the guard does not refuse what the cast could not break. Last, it runs top-k 0 over 0 experts and checks that the command succeeds and printsrouted experts/token = n/a. The refusals run before the successes because a success writes the Markdown file.FAIL-first. Each mutant is made in a scratch copy of the head (
git archive HEAD) with the test kept, and run in a debug and a release build.if let (Some(ec), Some(k)) = (expert_count, top_k) && k > ec { ... }block is deleted and the newu64::try_fromstays. The command panics in the conversion instead of printing 0, and the test fails on exit code 101 in both builds:k > ecchanged tok > ec && ec > 0(a line added to theif letchain). Top-k 2 over 1 expert is still refused, and top-k 1 over 0 experts now exits 0 withn/a, so the test fails on its second refusal in both builds:gguf-inventory.rsreplaced by the base version (git show cff55213:crates/gguf/src/bin/gguf-inventory.rs). The first refusal fails with exit 0 and a report that printsrouted experts/token = 0 bytes, in both builds:(
...marks the rest of the printed report, cut here.)With the fix, debug and release:
(the
--releaseruns print the same two lines).just mac-static upstream/mainat the head18482677on the M5: ok in 121 s. The base for that run wasupstream/mainat4a9419ea, two docs-only commits ahead ofcff55213, which the branch is based on. fmt-check has 0 diff blocks; lint has 41 warnings, the ratchet's 41 intools/lint-ratchet.txt, and compilesbloomery-modelforx86_64-unknown-linux-gnu. Combos ran 20 shapes, 0 red, 28 skipped as out of scope. check-arch, check-comments, check-defaults, check-levers, check-loads, check-ptx-names, check-records, check-recipes, check-rustflags, check-unsafe and check-waits pass.Where the test runs: CI's t0 tier tests none of
bloomery-gguf.gate-1-1runs the gguf tests (unit, integration and thegguf-inventorybin), so it covers this one. I did not rungate-1-1on a Linux node: the change is in a binary and a test that build the same on macOS, and the unit tests of thegguflibrary that do not build on macOS (libc::RUSAGE_THREADinsplit_tests) are not touched.Hardware (if the change is for a machine)
Not a machine-specific change.
AI help
AI 코딩 에이전트가 코드를 작성한 후 설명을 썼고, 테스트를 마쳤습니다. diff 리뷰와 PR 초안 리뷰는 제가 직접 했습니다.