Repository navigation
build: --target 1bit builds the CLI instead of finding a stale file and exiting 0 (Fixes #1993) - #1996
Closed
bong-water-water-bong wants to merge 20 commits into
Closed
build: --target 1bit builds the CLI instead of finding a stale file and exiting 0 (Fixes #1993)#1996bong-water-water-bong wants to merge 20 commits into
--target 1bit builds the CLI instead of finding a stale file and exiting 0 (Fixes #1993)#1996bong-water-water-bong wants to merge 20 commits into
Conversation
…ken table (WIP, engine#1725)
The gemma-4 1.9 components carry, beside `embed_tokens.weight`, a PER-LAYER table and a projection norm:
embedding.embed_tokens_per_layer.weight (262144, 8960) f32, external (= 35 layers x 256 head_dim)
embedding.per_layer_projection_norm.weight (256,) f32
The decoder consumes them as `per_layer_inputs`, so a container with only the token table gives the decoder less than it
asks for -- which is why the previous commit refused such a component by name. This adds the reader, so the next slice can
write the container section the forward path will load.
EmbedPerLayer { table [V][layers*head_dim] row-major, norm [width], V, width } + `ok()`;
read_embedding_per_layer(pack_dir, why) resolves the component exactly as the token half does -- genai_config.json's
model.embedding.filename (relative only, no ".."), external data beside the GRAPH rather than at the pack root -- reads
f32, refuses any other dtype by name rather than mis-decoding it, and says so when the projection norm is missing
(the table is still usable, but a consumer expecting a projected row would silently take raw ones).
WIP: no container section and no forward consumer yet; that is the next slice, with its test. Builds clean
(`npu_pack_import_test` rc 0). No vendor code or layout: only the pack's own ONNX names and dims are read.
…(WIP, engine#1725)
Second half of the reader in the previous commit: `import_pack()` now writes the per-layer table and its projection norm
beside the token table --
model.embed_tokens_per_layer.weight BF16 {V, layers*head_dim} (from the component's f32)
model.per_layer_projection_norm.weight BF16 {layers*head_dim}
-- and the slice-1 refusal in `read_embedding_component()` is relaxed to match: a component that carries
`embed_tokens_per_layer` is no longer refused by that reader, because the mechanism it was waiting for now exists.
Measured on the real pack with this change (`1bit npu-pack info ~/scratch-packs/pi-demo/e2b`, loader built from main):
before: the pack has no model.embed_tokens.weight, and the component carries embedding.embed_tokens_per_layer.weight,
i.e. a PER-LAYER embedding table ... the per-layer embedding mechanism is not implemented here yet
after: rc 0, a full plan -- dims hidden 1536 / 35L / 8 heads / 1 kv / head_dim 256 / IM 6144 / vocab 262144,
423 tensors, and 350 `incomplete` entries that all say the same NEXT thing:
"layer blocks not located: the epsilon headers do not repeat with a single period"
So the embedding half is no longer what blocks a gemma-4 pack: the next wall is the Token .fconst block layout, whose rule
row cannot be measured for gemma-4 (no 1.8 twin exists) -- the `txn_bins` preformat route designed on engine#1503.
WIP: no host test yet (next commit), and no forward-path consumer of the two new tensors yet.
… that made it fail first (engine#1725)
Test for the two WIP commits before it: `read_embedding_per_layer()` reads the component's
`embed_tokens_per_layer` (V x layers*head_dim) and `per_layer_projection_norm` (width) as f32, resolving both through
genai_config.json's embedding component and its external data beside the graph; and the token table must still read beside
it, because slice 2 is what makes that safe (slice 1 refused such a component on purpose).
Two fixture mistakes are worth the comment they now carry, because both produce a fixture that looks right:
* `std::vector<float> norm(size_t(width))` is a FUNCTION DECLARATION (most vexing parse);
* `std::vector<float> norm{size_t(width)}` is an initializer_list of ONE element = width, so the table is 1 value wide
and the reader correctly reports "declares more bytes than its data carries" for the norm.
The two-arg form is the only one that does what it looks like.
Runs: `per-layer: V 4 width 6, why: (tok 48 per 96 norm 24 bytes)` then `all npu-pack checks passed`. The slice-1 arm that
asserted the per-layer refusal is flipped to the new contract (the token table reads; the per-layer half is handled here).
…by three numbers (engine#1725)
The band the per-layer embedding feeds is `out[k] = table[id][k] * norm[k]` over the layer's own slice of a
layers*head_dim-wide row; that is the whole arithmetic, so it lives in a header-only function with no container and no
device (`npu/forward/include/per_layer_embed.h`) and a test that can be read as the spec:
row0 {1,2,3,4} * norm {2,2,0.5,0.25} -> {2,4,1.5,1}
row1 {-1,0.5,0,8} * same norm -> {-2,1,0,2} (a different id picks a different row)
null norm -> the row passes through, because a MISSING projection is not a ZERO one
id out of range, or no table -> output untouched (the loader can hand a short container)
Runs: `per-layer gather: row0 -> 2.00 4.00 1.50 1.00` then all checks pass.
Also in this commit: the test target gains `npu/forward/include` on its include path (it had only the engine's own), and a
comment records the trap I hit twice in this file: `std::vector<T> v(size_t(n))` is a FUNCTION DECLARATION, and
`std::vector<T> v{size_t(n)}` is an initializer_list of one element — only the two-arg form does what it looks like.
…/complete() (engine#1725, WIP)
Tracing the component's graph end to end (not its node list) showed the per-layer input is
( rmsnorm((inputs_embeds @ per_layer_model_projection.weight_t_Q4) * H**-0.5, per_layer_projection_norm) + 16*table[id] ) / sqrt(2)
so the table and its norm are only TWO of the three tensors: the context projection is the third, and without it the
per-layer input is `16*table/sqrt(2)` -- plausible, and wrong at every layer. The component carries it quantised
(`per_layer_model_projection.weight_t_Q4` [8960,48,16], `..._t_scales`, packed `..._t_zero_point`), and the loader wants it
dequantised as `model.per_layer_model_proj.weight_layer{l}` [P][H] BF16 per layer.
* `read_embedding_per_layer(pack_dir, hidden, why)` now dequantises that projection through the file's own
`decode_matmulnbits()` (same decoder the rest of the importer uses) and holds it as [NL][P][H] f32; `hidden` is a
parameter because it comes from the pack's geometry, not from the component.
* `ok()` is what the reader guarantees (table + norm); `complete()` additionally requires the projection. A component
with the first but not the second imports the table and is refused by the LOADER on `per_layer_model_proj.weight_layer0`,
which is the honest place to refuse -- the importer does not know whether the decoder declares per_layer_inputs.
Measured on `amd/gemma-4-e2b-it-mobius-int4_rai_1.9.0_npu`'s component: table 262144x8960 f32, norm 256 (shared by all 35
layers -- the graph reshapes to [.., 35, 256] around the norm), projection K=1536 N=8960 block 32 int4.
WIP: the container still writes the two old names; the rename to the loader's names and the projection emission are the next
commit, together with the forward's BF16 band path for the table.
… loads (engine#1725, WIP)
The forward's `build_ple_inputs()`/`load_ple_block()` already implement this family's arithmetic -- I traced the component's
graph and it is line for line the same: `context = rmsnorm((emb @ W_l) * H**-0.5, norm)`, `identity = 16 * table[id]`,
`per_layer_input = (context + identity)/sqrt(2)` -- but it reads different names than the importer wrote. This commit closes
that gap:
model.per_layer_token_embd.weight BF16 {V, NL*P} (was model.embed_tokens_per_layer.weight)
model.per_layer_proj_norm.weight BF16 {P} (was model.per_layer_projection_norm.weight)
model.per_layer_model_proj.weight_layer{l} BF16 {P, H}, l = 0..NL-1 (was absent)
The token table is **streamed**, never materialised: `read_per_layer_table_band()` maps the component's external data and
converts 32 rows at a time from f32 to BF16 straight into the container blob, because the full table is V x NL*P f32 =
9.4 GiB on gemma-4-e2b and a converting copy of that would be the largest allocation in the import.
Two bugs of my own found while writing it, both worth the comments they carry:
* the first version of the band reader did `memcpy(dst, src, n * 4)` into a `uint16_t*` of n elements -- 4 bytes per
element into a 2-byte-per-element buffer, i.e. a 2x overflow. Removed before it ran;
* the projection matrices were being dequantised but never written; `pl.complete()` (table + norm + projection) is what
now gates the per-layer emission, and a component with `ok()` but not `complete()` still writes the table, leaving the
refusal to the LOADER (`missing model.per_layer_model_proj.weight_layer0`), which is the honest place.
All existing checks pass; copyright rc 0. WIP: the forward still lacks a RAW-BF16 band path in `load_ple_block` (it handles
plain int8 + scale, or the chunked container formats), so this container is one loader branch away from being readable.
`load_ple_block()` handled two shapes: plain int8 + a per-32-column `.scale`, and the chunked container formats. The multi-component packs state this table as f32 and the importer converts it to BF16 when it writes the container -- a converting copy of the whole V x NL*P table would be 9.4 GiB on gemma-4-e2b -- so the container's table is raw BF16 and neither branch matched it. The band is the same shape the int8 branch handles (32 rows x cols), with a 2-byte element instead of a code, so this is a loop of `bf16_to_f32` over the band, chosen by data_size: a multiple of the BF16 band and NOT the plain-int8 size. That ordering matters -- `plain_bytes` (one byte per element) and a BF16 band differ by a factor of two, and testing int8 first would take a BF16 table whose size happens to equal `plain_bytes` for another geometry. With this, a container written by the importer (#1725's `83c323b`) is readable end to end on the embedding side: table (banded), norm (256), and the per-layer context projections.
…el's arithmetic (engine#1725)
The previous commit on this branch added `per_layer_gather()` computing `table[id] * norm`, from a reading of the component's
node LIST. Tracing the component's graph end to end showed something else:
per_layer_inputs = ( rmsnorm((inputs_embeds @ per_layer_model_projection.weight_t_Q4) * H**-0.5,
per_layer_projection_norm) + 16*table[id] ) / sqrt(2)
-- the norm applies to the projected CONTEXT, the table row is ADDED (times 16 = sqrt(256)), and the sum is scaled by
1/sqrt(2). That is exactly what the forward's `build_ple_inputs()` already computes, so the primitive was not merely
unused, it was wrong, and a pure function with a passing test is exactly the wrong thing to leave in a tree for someone to
call later.
Deleted: `npu/forward/include/per_layer_embed.h`, its test, and the test target's include line it needed. The container and
loader work that DOES match the graph stays (earlier commits on this branch): the three PLE tensors written under the names
the forward reads, the streamed BF16 table, and `load_ple_block()`'s raw-BF16 band path.
Deleting beats keeping a wrong primitive as documentation: the retraction is recorded on engine#1725 with the graph, which
is where someone changing this will look.
…engine#1503)
`fconst19_detect()` requires ONE stride for the whole file. The mobius line does not have one -- measured on
gemma-4-e2b: 35 blocks, each opening with 8 anchors (8 heads x 4 B = 32-byte header), sizes 21.87 MiB to 288.06 MiB --
so the detect fails and the pack was reported the same way as a file that is simply not a 1.9 fconst at all.
* `fconst19_block_starts()` is the run scan on its own (exactly `layers` runs of `heads*4` anchor bytes), so a caller can
tell the two cases apart before the stride test;
* when the runs are there but the strides differ, the refusal now NAMES the structure: how many blocks, where the first
one starts, and the size range in KiB, plus why the rule table cannot be the answer ("one stride per file" is the
table's model) and what the answer is (the per-layer block table; the anchors give the starts, and each block's size
follows that layer's own projection shapes).
On the real pack:
1bit npu-pack: … this IS a Ryzen AI 1.9 Token .fconst (35 blocks of 32 B headers at 2564864 B) but its blocks are NOT
uniform: 22395 KiB to 294973 KiB, i.e. a variable-stride container. The rule table describes one stride per file, so no
row can fit it; this family (mobius/MatFormer) needs the per-layer block table instead …
Crafted arm: four blocks with different body sizes are refused by name (`variable stride: refused by name
(variable-stride container)`), so the distinction is pinned without a 1.3 GiB pack.
…ot by arithmetic on the size Review of #1825 caught a real collision class in my BF16-band branch: it separated the two layouts with data_size >= (block+1)*bf16_bytes && data_size % bf16_bytes == 0 && data_size != plain_bytes and every multiple of the band satisfies the first two, so a plain table whose byte count happens to divide would be read as 32-row BF16 bands -- "plausible and wrong", the failure my own comment warns about. The tree already decides this way elsewhere (`model.c` does `strcmp(dtype, "I8")` on the same field) and `q4nx_dequant.h`'s rule is that an ambiguous tile count is refused rather than resolved by which test happens to pass. * BF16 is taken only when the container SAYS BF16, and then its size must be whole 32-row bands (else refused); * plain I8 (or Q4) keeps the existing int8 + .scale path; * any other DECLARED dtype is refused by name, and so is an EMPTY one -- a container written by this importer always states a dtype (`w.add()` writes it for every tensor), so "no dtype" means a foreign or truncated container, not a layout this loader may guess. `model.c` populates `dtype` for every tensor entry that carries the key, which is what makes the branch sound rather than merely tidier; that is stated in the comment next to the test.
…to switch the block on (engine#1725)
The per-layer tensors are useless without the two numbers the forward's semantics reader gates the whole block on
(`hidden_size_per_layer_input > 0`, q4nx_semantics.c). gemma-4 states them in model_config.json's text_config
(256 and 262144); the container's own config.json did not carry them, so an import would write the table, the norm and the
per-layer projections and then the forward would skip the block silently -- the worst failure shape, because the container
looks complete.
* when the component is `complete()` (table + norm + context projection), the container's config.json gains
`hidden_size_per_layer_input` = P and `vocab_size_per_layer_input` = V, taken from the COMPONENT (where they are
measured) rather than from the pack's config, which may not state them at all;
* the three Gemma-4 layer-shape switches the same reader consults -- `num_kv_shared_layers`, `use_double_wide_mlp`,
`extra_layer_norms` -- are carried through when model_config.json's text_config states them, and left absent otherwise
so the reader's defaults apply.
The read happens at the config-writing site, which lives in a different writer from the one that emits the tensors: one
metadata parse (names, dims, the quantised projection header -- the 9.4 GiB table is NOT read there) rather than plumbing a
value across two functions.
…the executor (engine#1931)
…ime (engine#1931)
… against onnxruntime (engine#1931)
… reference decode (engine#1931)
…sured at last (engine#1931)
… and exiting 0 (engine#1993)
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
Collaborator
Author
There was a problem hiding this comment.
VERDICT at 2d63df9: READY
The fix is one line and it is the right one: add_custom_target(1bit DEPENDS onebit) means --target 1bit builds the CLI instead of matching a stale file of that name. A target that shadows a filename is the classic CMake trap — make 1bit finds 1bit on disk, decides it is up to date, and does nothing, so a build "succeeds" without compiling anything.
One thing to know before merging, and it is the reason this PR is 15 files rather than 2: git merge-base --is-ancestor confirms this branch contains #1931's branch — the Whisper front-end (audio_exec, audio_mel, audio_ops, onnx_graph, whisper_transcribe) is stacked underneath. So merging this lands #1931's work as well, and #1931 is still open. Two ways that goes right: merge #1931 first (then this is the clean one-line diff it looks like), or merge this and close #1931 explicitly so the work is not double-counted.
I readied #1931 at 61426f5 earlier and the same caveat applies to both: READY on scope and green CI, not a certification that the wide front-end surface is correct — the known-answer arms and the graph arm carry weight, the driver code is what a run would have to confirm.
CI green.
Collaborator
Author
|
VERDICT: CHANGES
Separately, and this is not paperwork: the PR's headline is I did not review the audio/onnx additions themselves (+2,210 across |
Collaborator
Author
|
VERDICT at 2d63df9: CHANGES - This is a second blocker, independent of the ci_targets CHANGES above, and that one stands as written. I agree with it. The defect, on this headNinja's targets are file paths, and #1993 is a Makefiles-only bug: under Ninja,
A fix measured on the real engine (main a0f17ac)set_target_properties(onebit PROPERTIES OUTPUT_NAME 1bit)
if(CMAKE_GENERATOR MATCHES "Makefiles") # Ninja already has a rule for the file; a second one breaks build.ninja
add_custom_target(1bit)
add_dependencies(1bit onebit)
endif()
ScopeThe PR also carries 14 files of audio work ( Not run: the PR's audio tests. I reviewed only the #1993 change. |
Collaborator
Author
|
Closing in favour of #2007, which refuses I opened this two lines of CMake ago believing the right fix was to make the spelling build the CLI. #2007's comment has the better argument: an alias keeps the wrong name in everyone's fingers and in the logs, while a refusal that names Verified on #2007: refusal text, |
This was referenced Nov 10, 2026
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.
Fixes #1993. Two lines plus a comment, no behaviour change to the binary.
The CLI's target is
onebitand1bitis only its output name. Because a file called1bitalready exists in the build tree,cmake --build <dir> --target 1bittreats the name as an up-to-date file target — it prints nothing, exits 0, and leaves whatever binary was there:With the target:
Both spellings work afterwards —
--target onebitis untouched, which is what every existing script uses, so nothing changes for them.Why I care enough to send two lines: the failure mode is a false verification, not a failed build. During a review I rebuilt "main", ran an import against a real 2.5 GiB pack, watched it succeed, and only later noticed the binary predated the checkout by seven hours — the run was clean because it was a different binary from a different branch. In a fleet where "rebuilt and re-ran" is the currency of every review verdict, a build command that silently does nothing is worse than one that fails.
Test:
npu_pack_import_test tests/goldenpasses at this head (all npu-pack checks passed -- pack leg SKIPPED);--target onebitand--target 1bitboth build; the binary is byte-for-byte the same build as before the change (the custom target adds a dependency edge, nothing else).I considered the alternative — documenting "always use
--target onebit, and checkls -lagainst the commit" — and rejected it: that relies on every author remembering, and the failure is silent by construction. A target costs two lines and removes the trap.