Repository navigation
fix(model): recognize F8_E4M3 GGUF tensors from ggml v0.29.0 - #9795
Open
NotAFlightRisk wants to merge 1 commit into
Open
NotAFlightRisk wants to merge 1 commit into
NotAFlightRisk wants to merge 1 commit into
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
inspect-model-artifactdoesn't know GGUF tensor type 43 yet, so an FP8 model's weights come back asUNKNOWN_43with no byte length, and the tensor ranges and storage sizes drop to partial. ggml v0.29.0 addedGGML_TYPE_F8_E4M3 = 43on 4 November (llama.cpp v0.9.0 shipped it the same day), and llama.cpp's converter now keeps FP8 checkpoint weights in that type by default. This adds the missing row to the type table.No linked issue - it's a one-row table update.
Problem and expected behavior
Any GGUF with an F8_E4M3 tensor triggers it, e.g. one written by
convert_hf_to_gguf.pyfrom an FP8 checkpoint (its default--outtype autokeeps FP8 weights as they are).The table follows the ggml enum, which now has
GGML_TYPE_F8_E4M3 = 43, with a block size of 1 and a one-byteggml_fp8_e4m3_t. gguf-py in llama.cpp v0.9.0 lists it as(1, 1)as well. Expected: those tensors are namedF8_E4M3and get a derived byte length, the same as F16 or BF16.Change and scope
Adds
43: ["F8_E4M3", 1, 1]and moves the table's pin comment from ggmlda8e2a54to57e2c2ca(the v0.29.0 tag). Between those two commits theggml_typeenum only gains this one line, and everyQK*define and blocksizeofassertion inggml-common.his unchanged, so nothing else in the table moves. The Q8_1 note and the links indocs/model-artifacts.mdstay onda8e2a54, since the layouts they cite haven't changed.Scope decision: not applicable, it's an ordinary fix inside the existing GGUF inspection.
Contract and boundary impact
src/model/ModelTensorTypes.ts, read byGgufHeader.ts. Nothing else interprets tensor type IDs.typeis already a free string andtype_ida number.byte_lengthandstorage_validation: "consistent". Tensor data is still not interpreted.docs/public/product-catalog.json), package, or installation impact: none.Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.
Evidence and regression coverage
Tests added or updated: one F8_E4M3 row in the existing "retains established storage for stable GGUF types" test in
tests/boundary/model/modelArtifact.test.ts. It's 3 bytes at offset 64, so the expected total goes from 30 to 33.Base reproduction or other evidence: with main's table the new row fails with
type: "UNKNOWN_43",byte_length: nullandstorage_validation: "unknown". With this change it passes.User-visible CLI/MCP output (if applicable):
rea inspect-model-artifact fp8-demo.gguf --format json, on a GGUF with one F32 tensor and one 32-element F8_E4M3 tensor.Before (excerpt):
After (excerpt):
Remaining proof gaps: I haven't run it on a full converted FP8 model. The CLI file above is a 256-byte GGUF v3 I wrote to the spec, not one written by llama.cpp.
Boundary regressions: format assumptions, with the producer revision (ggml v0.29.0) named in the table comment. The test runs
ModelArtifactProvideron a written GGUF file, and the CLI run above covers the public path. Lifecycle regressions: not applicable.For evidence-bearing changes:
The byte length is still marked
derived, and type IDs rea doesn't know are still reported as unknown.Validation performed
Linux, Node.js 24.22.1, base
55b6d2c.npx vitest run tests/boundary/model/modelArtifact.test.ts -t "retains established storage": 1 failed with main'sModelTensorTypes.ts, 1 passed with this change.npx vitest runon the threetests/boundary/model/files: 145 passed.npm run check:fast: passed.npm run format:check: passed.node scripts/verify/architecture-guards.mjs: passed.npm run lint:typed: nothing reported on the two changed files.npm run build:cached, then therea inspect-model-artifactrun above, on main and on this branch.Compatibility, safety, and release
UNKNOWN_43are nowF8_E4M3.Author readiness
Human owner: @NotAFlightRisk
Before merge
type(scope): outcome.