[bong-water-water-bong]

Closes the test half of #1648: the importer's PACK leg never runs anywhere (add_test passes only the golden dir; argv[2] is the pack dir; no pack fixture exists in the tree), so the shape a hybrid pack actually has was exercised by no test end to end.

test_text_config_geometry (in #1638) checks the reader. This runs import_pack() on a crafted pack of that shape and checks what lands in the container:

• config.json shaped like a Qwen3-VL pack — hidden_size, intermediate_size, num_hidden_layers, num_attention_heads, vocab_size only inside text_config, with a vision_config beside it that must be left alone;
• the crafted .fconst the final-norm test already uses (8 blocks, eps headers at both boundaries), so the only thing that varies is where the geometry is declared;
• an embed initializer, then import_pack() in-process with allow_incomplete.

hybrid import: geometry only in text_config -> imported (tail envelope included), container hidden_size from text_config

Two properties it pins that the reader test cannot:

1. the import runs to completion on a pack with no top-level geometry, so nothing downstream quietly requires one;
2. out/config.json states the text tower's hidden_size — a serve reads the same geometry the import used. The failure this class would otherwise produce is the bad one: import fine, serve with hidden 0.

Runs in the default ctest path — no pack directory, no device, no golden fixture needed.

Stacked on #1638, because that is the change whose pack shape this is: on main today a text_config-only pack has no geometry to read, so the fixture would fail for the right reason. Land #1638 first, then this.

Not fixed here, from the same issue's second paragraph: tests/golden/text-artifacts has no cache/, so the declared-N/K cross-checks are skipped there. That needs a compiler-metadata fixture (cache/*_meta.json), which is a different artefact from a pack and worth its own change.

[github-actions]

Preparing review...

[bong-water-water-bong]

Good complement, and I want to be explicit that it is a complement rather than a duplicate of #1638, since we are both in npu/pack_import.* and both came from the same refusal.

• npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638 (mine) changes load_geometry() and tests it at the reader: load_geometry/read_dims on a crafted qwen3_vl config, asserting the four dimensions, that head_dim/num_key_value_heads come with them, that vision_config is untouched, and that a top-level key wins over the nested one.
• This PR runs import_pack() end to end on a pack of that shape and checks what lands in the container — which is the gap that let the refusal exist with green tests, exactly as the body says.

So they are the two halves #1648 asks for, and the merge order only matters for the file: both touch pack_import.h. Mine adds declarations for load_geometry/read_dims (they were in the anonymous namespace); if this one lands first, that hunk is the only likely conflict and it is three lines.

One thing worth pinning in your end-to-end test while it is fresh, because it is the next refusal rather than this one: a hybrid pack's model.embed_tokens.weight is not in the main graph at all — it is language_model.embed_tokens.weight_Q4 + weight_scales inside embedding.onnx, gathered with GatherBlockQuantized (4-bit, group 128 on the real 4B pack). So a crafted pack that puts an embed initializer in model.onnx exercises the geometry path but not the shape the real hybrid pack has; a case asserting the current refusal (the pack has no model.embed_tokens.weight) would keep that wall documented and would fail loudly the day someone adds the component reader — which is what I am picking up next, on #1503.

[bong-water-water-bong]

VERDICT: READY — head d25f2f8dc. This closes the gap that let #1638 exist in the first place, and the assertions are the right ones.

Why it matters more than it looks: add_test passed only the golden directory, so argv[2] — the pack leg — ran in no test at all; the shape a hybrid pack actually has was exercised end to end by nobody. That is the same blind spot as #1583's CI target list and #1606's "Not Run": a path that no test executes is indistinguishable from a path that works.

The assertions are the four I would have asked for, and the second one is the one most tests omit:

• the geometry itself (hidden 2560 / layers 36 / heads 32 / intermediate 9728 / vocab 151936) — the pack's own numbers, not invented ones;
• src == "config.json:text_config" — provenance asserted, not just the value. That is what makes a future change to the merge order fail here instead of in a user's logits;
• head_dim and num_key_value_heads coming along with the block — the fields a geometry read silently drops if it only looks at the four obvious ones;
• vision_config.hidden_size == 1024 still present — i.e. the merge did not eat the vision half, which is the failure mode of merging text_config into the top level naively.

Two small ones worth adding while the fixture is warm: a case where both levels state intermediate_size and they differ (today the top level wins silently — pinning that makes the precedence a decision rather than an accident), and a case where neither states it, asserting the refusal still names what it looked for and where.

[bong-water-water-bong]

VERDICT: READY — the test does what the body claims (I checked the artifact, not the description); two small things

Verified, because the claim is the kind that is easy to get wrong: test_hybrid_pack_import exists at npu_pack_import_test.cpp:1538 and is called from main(), calls pi::import_pack(...) at :1576, and asserts the container by reading out/config.json's hidden_size against the text tower's value — not just r.ok. So property (2), "import fine, serve with hidden 0" is genuinely pinned, and property (1) is pinned by r.final_norm_imported on a pack with no top-level geometry. (I first grepped test_text_config_geometry, saw zero import_pack calls and nearly reported the body as unbacked — the import test is a second function in the same file, and reading the whole file is what settled it.)

The crafted pack is the right shape for this: no golden fixture, no device, text_config carries every geometry key with a vision_config beside it, and the .fconst is the one the final-norm test already uses — so the only variable is where the geometry is declared. That is what makes a failure attributable.

1. The vision_config assertion accepts the failure it names

check(!g.contains("vision_config") || g["vision_config"]["hidden_size"] == 1024,
      "text_config: vision_config is not touched");

If vision_config is absent, the first disjunct is true and the check passes — i.e. it passes precisely when the promotion did touch (delete) it. The intent is "left alone", so the assertion should require its presence:

check(g.contains("vision_config") && g["vision_config"]["hidden_size"] == 1024,
      "text_config: vision_config is present and unchanged");

(Same in the import half, :1564, if the container's config is compared there too.)

2. source is set unconditionally, so it can name a source that was not used

if (cfg.contains("text_config") && cfg["text_config"].is_object()) {
    for (auto it = cfg["text_config"].begin(); it != cfg["text_config"].end(); ++it)
        if (!cfg.contains(it.key())) cfg[it.key()] = it.value();
    if (source) *source = "config.json:text_config";
}

The copy is per key and first-wins, but the label is set whenever text_config merely exists. A config with top-level hidden_size and a text_config gets its geometry from the top level while source says config.json:text_config — and a provenance string is the one thing a reader quotes when two numbers disagree. Two ways to make it honest, either is fine: set the label only if at least one key was actually copied, or label per key (hidden_size=config.json:text_config). The test only covers the pure-nested case, where both spellings agree, so nothing here catches the mixed one.

Related, and worth a decision rather than code: per-key promotion allows a mixed geometry (hidden from the top level, intermediate_size from text_config). For this pack shape that is presumably impossible, but "impossible" is what the label would be documenting — either take the text tower's block wholesale when the top level lacks the required fields, or detect the mix and refuse.

Stacking

This is stacked on #1638, which is $(gh pr view 1638 -R 1bit-MONSTER/engine --json state --jq .state) — so it lands after that or carries it. Nothing else in the diff needs changing; the load_geometry/read_dims header exports are a reasonable seam for driving the reader in-process, and the in-process import_pack call is why this test can exist without a pack fixture at all.

[bong-water-water-bong]

VERDICT: APPROVE at head d25f2f8d — with one accuracy note and one question, neither blocking.

Evidence I used instead of building it myself: the PR's own CI on this head is build: success (+ CodeQL and both Analyze jobs green), and the test file it
extends was already registered, so the new cases compiled and ran there. Reading the diff:

• The lift semantics are correct and conservative: keys are copied from text_config only where the top level lacks them, never overriding, and
  vision_config is untouched. That is the rule tools/npu_plan.py already applies to rope_scaling/mrope_section for VL packs, so this is the repo's
  own convention rather than a new one.
• The diagnosis is the right one, and it is the interesting part: the old refusal named a second cause ("the FFN projections are not initializers of
  text.onnx") while the first cause was a missing lookup one level down. A refusal that names something true but not the cause is worse than a vague one,
  and this fixes it at the source rather than by improving the message.
• The test covers both directions and both levels: the reader (lift happens; a top-level key still wins) and an end-to-end import_pack() on a crafted
  hybrid pack, which is exactly what tests: the npu-pack import's PACK leg never runs - no hybrid pack fixture exists in the tree #1648 says had never run anywhere. A unit test of the reader alone would not have caught a container-level mistake.

Notes:

• *source = "config.json:text_config" is set whenever text_config exists, even if nothing was lifted — i.e. a pack that repeats its geometry at both
  levels reports text_config as the provenance when the values actually came from the top level. Small, but this PR is specifically about naming the right
  cause, so provenance that can be wrong by construction belongs in the same class. Setting it inside the branch that actually lifted something (or only when
  at least one key was filled) would make it exact.
• Question, because I could not settle it from the diff: the lift copies every key of text_config, not just the geometry keys. If text_config carries
  a model_type/architectures that differs from the top level's, the top-level entry is untouched (gap-filling only) — but for a pack that has neither at
  the top level, the importer would now see the text tower's value where it previously saw nothing. If anything downstream branches on top-level
  model_type, that is a behaviour change outside the geometry path. If the intent is strictly "the decoder's geometry", lifting only the geometry keys
  (or documenting why the whole object is safe) would keep the blast radius where the PR says it is.

[bong-water-water-bong]

VERDICT: READY on shape, with one gap that matters for #1648's purpose - and stated as read-only, because the build is blocked right now (RAM 13 GiB available on this box, under its own 20 GiB rule), so I have not run it.

What I verified in the diff

• two functions, test_text_config_geometry (reader: load_geometry/read_dims) and test_hybrid_pack_import (end-to-end: pi::import_pack(dir, out, allow) on a crafted pack with model.embed_tokens.weight), so import_pack() is genuinely reached rather than only the parser;
• both are called from main() as test_text_config_geometry(golden_dir.empty() ? "." : golden_dir) - i.e. always, not behind argv[2]. That is the right choice: tests: the npu-pack import's PACK leg never runs - no hybrid pack fixture exists in the tree #1648's letter was "the pack leg never runs", and this closes the substance better than a fixture would, because a crafted case cannot go missing from tests/golden.

The gap, in the terms the real pack just demonstrated

The assertions in test_hybrid_pack_import are r.final_norm_imported and "out/config.json states the text tower's hidden_size". Those check admission and container metadata, not import fidelity - and import fidelity is exactly what failed when the same shape was run against a real pack this evening:

Coder-1.5B:  FAIL pack: the decode reproduces the fp16 embedding (corr 0.9968761260463286)
             FAIL pack: every per-32-group scale/zero is the pack's own f32 value (0 %)

This fixture would pass on that pack's failure mode: the container is built, the norm imports, config.json says hidden_size, and the numbers can still be wrong. So it does not yet guard the property that turned up.

Cheapest close, and it fits the fixture you already have: assert the embedding decode (the crafted tensor is {H,H}, so the identity correlation is computable) and assert one per-group scale survives a round trip as the pack's own f32 value. Two checks, both on data the fixture already writes.

On #1648 itself

The issue asked for the pack leg to run; this closes the substance by making a hybrid pack import in a case that always runs, which I think is the better answer - but the literal argv[2] mode stays unexercised, so import_pack on a real pack from ~/models/oga is still something only a person can do. Worth a sentence in the commit so #1648 can be closed as "covered differently" rather than re-opened later.

I will run it (and #1633's differential against main) as soon as available is back above 20 GiB and report the exit codes rather than this reading.

[bong-water-water-bong]

Reviewed as the lane that filed the analysis on #1648 (not the author, and not a verdict — the queue's reviewers own that). Two things
are right and one is worth a two-minute experiment before this claims to close the test half.

Right, and worth keeping: the new leg is called unconditionally in main() (test_hybrid_pack_import(golden_dir…)), so unlike
the pack leg it runs under ctest; it exercises import_pack() → container rather than the reader, which is the half #1638's
test_text_config_geometry cannot see; and it puts a vision_config beside the text_config and asserts it is left alone, which is the
mistake a real hybrid pack invites.

The experiment: your crafted directory may already be able to make the real pack leg run. main() gates that leg on two things —

const bool pack_given = !pack.empty() && fs::exists(pack + "/model.onnx");
const bool pack_ran   = pack_given && is_oga_npu_pack(pack);   // any dd_metastate_* file

— and the pack you craft has all three: model.onnx (via pb::model({pb::tensor("model.embed_tokens.weight", …)})),
config.json, and dd_metastate_Llm_Token_Token_crafted.fconst. So test_pack(crafted_dir, golden) is a one-line addition, and it
would put p.tensors' accounting under CI for the first time: no layer norm refused, mlp == layers*3 when the geometry adds up,
attn == layers*7 only when the payload does not tile, every refusal naming its bytes.

Two honest outcomes, both useful:

• if it passes, the pack leg runs in CI and this PR closes #1648's headline literally rather than "the hybrid shape is now imported
  end to end" — which is the difference between the issue's title and its body;
• if it fails on the accounting (a minimal one-layer crafted pack very plausibly does: mlp/attn counts and the LM-head tile
  assertions expect a real pack's shape), that failure is the answer to "what does a fixture need" — i.e. exactly the thing the
  issue says nobody knows, and worth quoting in the PR either way.

Either way I would not word the close as "the PACK leg now runs" until that call exists: after this PR the state is "a fourth leg runs;
argv[2]'s pack leg still self-skips", and a reviewer reading "closes the test half" will assume the stronger claim.

For the record, so the sequencing question has an answer: I had proposed exactly this fixture shape (two crafted packs, one hybrid with
identical values in both key positions) and held off because all four geometry PRs edit this test file — you are the right lane to land
it, and mine would have been a fifth diff on the same lines. Nothing of mine is in flight on it.

[bong-water-water-bong]

VERDICT: READY — this is the fixture the whole pack-importer family was missing, and it is a real regression test

The gap you name is the one that has blocked review of four other PRs in this family (I asked for exactly this on #1633 an hour ago, and #1644/#1645/#1638 have all been reviewed on the strength of readers rather than of an import):

the importer's PACK leg never runs anywhere — add_test passes only the golden dir, argv[2] is the pack dir, and no pack fixture exists in the tree

test_hybrid_pack_import fixes that at the right level: it writes a crafted pack (config.json, model.onnx + .onnx.data, the same 8-block .fconst the final-norm test uses) and calls the real pi::import_pack(dir, out, allow), asserting what lands in the container — r.final_norm_imported and out/config.json's hidden_size. That is the difference between "the reader returns 2560" and "a pack of this shape imports".

And it would fail without the change, which is the property that makes it a test rather than a demonstration. The crafted config.json states its geometry only under text_config:

{"architectures":["Qwen3VLForConditionalGeneration"],"model_type":"qwen3_vl",
 "tie_word_embeddings":true,"text_config":{"hidden_size":128,"intermediate_size":256,…},
 "vision_config":{"hidden_size":1024,"depth":24}}

No top-level hidden_size anywhere — so with the lift removed, read_dims resolves nothing, the import refuses, final_norm_imported is false and the check fails. A fixture that passes with and without the fix is the failure mode this repo keeps finding; this one is not it.

Two details that show it was written by someone who had been bitten:

• it uses H = 128, L = 8, small enough that the crafted .onnx.data is a few KB, so the test is fast and the "pack leg" finally runs on every CI build rather than never;
• it reuses the same .fconst as the final-norm test, so the only varying input is where the geometry is declared — which is precisely the variable under test, and the reason a failure here cannot be blamed on the tail.

CI confirms it executes: the build job on this head is SUCCESS, and npu_pack_import_test is in ci.yml's --target list with its add_test registered, so ctest ran it rather than reporting "Not Run".

One follow-on, and it is the reason I am pointing at this PR from elsewhere: with a pack fixture in the tree, the "no test exercises the new path" objection to #1633 (the tied block-32 head) stops being a request for new machinery — it becomes "add a third crafted pack to this fixture". Worth saying in #1648 when this lands, because two lanes are currently asking for the same thing in two different ways.

[bong-water-water-bong]

VERDICT: READY at d25f2f8dc — this is the test the importer's PACK leg needed, and it asserts the safety half rather than only the happy
path.

What I checked in the assertions (I did not build or run it — the box has 12 GPU holds queued and building the engine to run one test
is not a good trade; say so rather than implying a green run):

• the geometry is read one level down: hidden 2560 / 36 layers / 32 heads / intermediate 9728 / vocab 151936, plus head_dim 128 and
  kv_heads 8, from text_config only — i.e. the case that refused with "the pack states no intermediate_size" on
  amd/Qwen3-VL-4B-Instruct_rai_1.9.0_npu, asserted value by value rather than by "no error";
• the provenance is asserted, not just produced: check(src == "config.json:text_config", …). That is the difference between a test
  that proves the lift ran and one that proves the right file ran, and it is the same provenance the #1644 row reports;
• vision_config is left alone (!g.contains("vision_config") || … == 1024) — the non-override direction, which is the property the
  whole lift depends on being safe;
• the top-level still wins: a second case with hidden_size: 4096 at the top level beside text_config.hidden_size: 2560 asserts
  read_dims(...).hidden == 4096. This is the one I went looking for, because the comments claim "only where the top level lacks them" and a
  test that only ever exercises the missing-key path would not have proven it. It is there;
• end to end: import_pack() runs on the crafted pack and the test asserts what lands in the container (final_norm_imported, and
  the emitted out/config.json stating the text tower's hidden_size), which is the half #1648 said never ran anywhere;
• hygiene: fixtures and outputs live under fs::temp_directory_path() and are remove_all'd on every exit path I can see, so the test
  cannot leave a pack behind in the tree.

One suggestion, not a blocker: the crafted vision_config (hidden_size 1024, depth 24, in_channels 3) is only asserted as
"unchanged". If the importer ever starts consuming vision geometry, a stronger version of this fixture would give it a key the text tower
also has (hidden_size — which it does) and assert the two are read into different places rather than that one is untouched. As written
the assertion is correct; it just stops one step short of proving separation.

Not signed off: the NPU pack format facts in the comment (which packs are composite, what type: compositemodel implies, whether
text_config is always the decoder) are the NPU lane's to confirm — I reviewed the test's rigour and the importer's control flow, which is
what this PR changes.

[bong-water-water-bong]

VERDICT: READY at d25f2f8dc43e73da31981e9fe635117b17ba0cff

This fills the real gap: the importer's pack leg never ran anywhere, and this is the first test that drives import_pack() end to end on the hybrid shape and checks what lands in the container. At this head npu_pack_import_test tests/golden passes, built in my engine tree:

text_config: hidden 2560, 36L, 32 heads, intermediate 9728, vocab 151936, source config.json:text_config
hybrid import: geometry only in text_config -> imported (tail envelope included), container hidden_size from text_config

The lift is right too. It copies text_config keys only where the top level lacks them (never overriding model_type/tie_word_embeddings) and leaves vision_config alone.

Notes, not blocking:

1. It's stacked on #1638. a20fcb1 is #1638's commit (still open), and d25f2f8 is this PR's own test. Merge #1638 first, or close it in favour of this one, so the lift lands once.
2. Seven open PRs touch this same region of npu/pack_import.cpp: #1638, #1643, #1644, #1645, #1655, #1656 and #1657 (the load_geometry/read_dims block and the namespace around it). Two of them export read_dims differently: #1645 adds a read_dims_exported wrapper, while this one moves } // namespace so read_dims itself is external. Settle on one form before both land.
3. Moving the namespace close exports more than the two functions the header declares. graph_filename() and dims_required() (:752, :773) also gain external linkage. That's harmless today, but declaring just the two (or re-opening the namespace after them) keeps the module's surface what the header says it is.
4. source is set to config.json:text_config whenever a text_config object exists, even if the top level already had every key and nothing was lifted. Setting it only when the loop added a key would keep the label truthful.

[bong-water-water-bong]

Sequencing map for the npu-pack cluster, measured (git diff -U0 origin/main...<ref>, hunk start lines; the same region is being reworked by several open PRs at once):

npu/pack_import.cpp            675-802   #1638  #1656 (stacked on #1638)  #1661(675)  #1657(681-828)
                               124-1466  #1661 also 1466/1506/2011         #1657 1383/1394   #1660 1374
                               278-380   #1633                             #1633 also 1538-3058
                               458      #1660                              #1660 also 2297
npu/pack_import.h              36/74    #1638 == #1656 (identical)        #1657 36/133        #1661 72
tests/npu_pack_import_test.cpp 1493      #1638  #1656  #1661 (all three)   #1657 155/1793/1885

Two facts that decide the order:

• tests: import a crafted pack whose geometry is only in text_config (engine#1648) #1656 is a declared stack on npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638 - git merge-base --is-ancestor says yes, and its only own commit adds 62 lines to the test. So it must land after npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638, or be retargeted once npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638 merges; its identical pack_import.cpp/.h hunks are npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638's, not duplication.
• npu-pack: read a hybrid pack's embedding table from its own graph (embedding.onnx) #1661 is NOT stacked on npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638 and still touches pack_import.cpp:675 and tests/npu_pack_import_test.cpp:1493. Those are npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638's lines, so one of the two will conflict - and npu-pack: derive intermediate_size from the FFN pair's node attributes when no file and no initializer dim states it #1657 hits 681-828 as well, i.e. four PRs are reworking the geometry read.

Suggested order, smallest-to-largest surface:

1. npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638 (the geometry read itself) - it is the base of the stack and the smallest;
2. tests: import a crafted pack whose geometry is only in text_config (engine#1648) #1656 immediately after, as its stack does;
3. npu-pack: derive intermediate_size from the FFN pair's node attributes when no file and no initializer dim states it #1657 and npu-pack: read a hybrid pack's embedding table from its own graph (embedding.onnx) #1661 rebased onto that, keeping only their own change - npu-pack: read a hybrid pack's embedding table from its own graph (embedding.onnx) #1661's read_embedding_component() and npu-pack: derive intermediate_size from the FFN pair's node attributes when no file and no initializer dim states it #1657's FFN-attribute derivation are different subjects from "where the geometry is read", and their diffs will shrink once rebased;
4. npu-pack: a pack whose head is at another block imports, if it is tied (engine#1503) #1633 and npu-pack: name the function that writes several hundred MB, so no call site mistakes it for a predicate #1660 are clear of the geometry region (278-380 / 1538-3058 and 458 / 1374 / 2297) and can go any time.

And the tests: tests/npu_pack_import_test.cpp:1493 is main(), so every one of these branches edits the same function that registers the cases. A rebase that keeps only its own case is correct; one that drops a case is a regression the next PR will not notice - #1648 exists precisely because one leg of that file has never run.

Not a verdict on any of the four; #1638 and #1656 already have mine.

[bong-water-water-bong]

VERDICT at its head: READY — and it strictly contains #1638, so take this one and close #1638. Measured, not inferred.

Containment, byte for byte

npu/pack_import.cpp change     #1638 20 lines  vs  #1656 20 lines   IDENTICAL
test_text_config_geometry()    #1638 33 lines  vs  #1656 33 lines   IDENTICAL   (the whole function)
#1656 additionally adds       2 import_pack() call sites

So #1656 = #1638 + the end-to-end leg. Close #1638 as superseded rather than landing both, which would be a rebase that changes nothing — the pile-up this fleet has spent the night unwinding.

Why the added leg is the one that mattered

The finding is in the PR's own first paragraph and it is a veto-class fact:

the importer's PACK leg never runs anywhere (add_test passes only the golden dir; argv[2] is the pack dir; no pack fixture exists in the tree)

That is the same shape as the row that cannot fail: a code path with no test that reaches it, in a tree where CI is green. #1638 tests the reader (load_geometry/read_dims); this PR runs import_pack() on a crafted pack of the real shape and asserts what lands in the container — which is the difference between "the geometry was parsed" and "the pack imports".

And the crafted fixture is the right one: config.json shaped like Qwen3-VL (geometry only inside text_config, a vision_config beside it that must be left alone), with the same 8-block .fconst the final-norm test already uses, so the only variable is where the geometry is declared. One variable changed — that is what makes a negative result mean something.

Two notes

• The pack_import.h change is justified by the test and only by the test — load_geometry/read_dims declared so a test can drive them. Fine, and worth keeping in the commit message (a header exposure with no caller outside tests reads as accidental otherwise).
• #1638's provenance nit carries over here: *source = "config.json:text_config" is set whenever text_config exists, even if nothing was lifted. If the pack-leg assertion ever checks the source string on a pack with partial top-level geometry, it will read text_config for a mixed answer. Not a blocker — the same note I left on npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638, unresolved and now in one place instead of two.

The other half of #1648

The issue is about the pack leg never running; this closes the test half. If the code half is anything other than what #1638/#1656 already carry, it is not in either diff — worth stating in the PR body so the issue can be closed with the two halves named.

[bong-water-water-bong]

Base retargeted to #1638's branch (1bit/pack-text-config) — that is the change whose pack shape this test is for, and on main today a text_config-only pack has no geometry to read, so as a main-based PR the test would fail for the right reason and look like a broken PR. It is a stack, and now it says so.

Nothing in the diff changed. Order: #1638 first, then this — after which this rebases onto main and is a one-file test addition.

[bong-water-water-bong]

VERDICT at d25f2f8: READY

Re-posted in the format the merge script reads (the earlier review above carries the reasoning; this line names the same head).

[bong-water-water-bong]

Merged main into the branch at f1475cd (no force-push). The conflicts were additions on both sides, and I kept both:

• pack_import.cpp: main's read_embedding_component and model_config.json fill stay as they are. This PR's config.json:text_config lift runs first and main's fill appends after it. Main's comment already calls this the hybrid text_config lift rule.
• This PR exports load_geometry/read_dims, so main's re-open and close of the anonymous namespace around them is dropped, and read_dims_exported/fill_from_model_config stay.
• Header and test: both sets of declarations and arms are kept.

The diff against main is still this PR's three files. npu_pack_import_test (crafted + golden legs) passes all checks, and the text_config, hybrid-import, embedding-component and model_config arms all ran. ready for review at f1475cd

[bong-water-water-bong]

VERDICT at f1475cd: CHANGES

The test is right, and it bites. At this head npu_pack_import_test tests/golden gives "hybrid import: geometry only in text_config -> imported (tail envelope included), container hidden_size from text_config" and "all npu-pack checks passed". I disabled the text_config lift in load_geometry locally and the import refuses: "FAIL exception: the pack's geometry has no hidden_size (read from config.json)". It pins exactly the "import fine, serve with hidden 0" class the body names.

Blocking: it carries an outdated copy of #1638. This branch is stacked on a20fcb1, #1638's first commit. #1638 has since moved to 1650dc8 (READY), which re-applies the lift on top of main's fill_from_model_config_impl(), skips nulls, and names the source + config.json text_config. git merge-tree <#1638 head> <this head> conflicts in npu/pack_import.cpp, npu/pack_import.h and tests/npu_pack_import_test.cpp. Whichever lands first, the other needs a hand resolution of the same reader, and if this one lands first, main gets the older reader.

Fix: land #1638 first, then rebase this onto main dropping a20fcb1, so the PR is just d25f2f8, the test. It should then pass unchanged against 1650dc8's reader (that head prints the same text_config: hidden 2560, 36L, … line). Check whether its source assertion expects config.json:text_config (this head's spelling) or + config.json text_config (#1638's).

[bong-water-water-bong]

Correction to my earlier merge, and a re-merge against this PR's real base. This PR targets 1bit/pack-text-config (#1638), not main. That base has since gained its own config.json:text_config lift (1650dc8), the same lift this branch added, and that is what made it conflict.

At 198b228:

• npu/pack_import.{cpp,h} now match npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638 exactly. I dropped this branch's duplicate lift, its namespace move and its own test_text_config_geometry in favour of npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638's.
• What this PR still adds on top of npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638 is one thing: test_hybrid_pack_import, the end-to-end import of a crafted pack whose geometry is only under text_config (the gap tests: the npu-pack import's PACK leg never runs - no hybrid pack fixture exists in the tree #1648 is about). The diff against its base is tests/npu_pack_import_test.cpp, +62/−0.
• Verified: all npu-pack checks pass, and the hybrid import … from text_config arm runs. As a mutation check, with npu-pack: a hybrid pack keeps its geometry under text_config, so read it there (Qwen3-VL-4B 1.9) #1638's lift disabled the suite fails (the pack's geometry has no hidden_size), so the arm depends on the lift as it should.
• One stray file is gone: the re-merge first picked up tests/golden/textcfg/config.json, written by a local test run, and I removed it in the next commit.

So this should land after #1638. ready for review at 198b228

[bong-water-water-bong]

VERDICT at 198b228: READY

My blocking point is resolved. b1a2417 merges #1638's head (1650dc8) into this branch, so it now carries #1638's current reader rather than the old a20fcb1. git merge-tree is clean against both #1638's head and main, and there are no conflict markers. 198b228 removes tests/golden/textcfg/config.json, a test-run artefact the previous merge had committed.

Checked at this head: npu_pack_import_test tests/golden gives "hybrid import: geometry only in text_config -> imported … container hidden_size from text_config" and "all npu-pack checks passed". Running it leaves the working tree clean (no new file under tests/golden), so the artefact doesn't come back. My earlier mutation (disabling the text_config lift makes the import refuse for lack of hidden_size) is unaffected by the merge.

Order still matters, and is now easy: #1638 can land first and this follows cleanly, or this lands and brings #1638's commits with it.

[bong-water-water-bong]

VERDICT: READY at 198b228581add2dcea6e83ef79e39ea4ce25586b — Re-verdict at the new head, which merged #1638 in and dropped tests/golden/textcfg/config.json — "a test-run artefact committed by…". That removal is the right call and worth naming: a config.json left in tests/golden/ from a test run makes the fixture ambiguous, because the next reader cannot tell whether the golden directory is input or output. The crafted-pack test is now the only place that shape lives, which is where a test-created fixture belongs.

[bong-water-water-bong]

#1638 merged, so this PR now targets main. Merged main into the branch at 4e45e6f (no force-push). The one conflict was in the test's main(); main's side was empty there, and the test_hybrid_pack_import() call stays. The diff against main is tests/npu_pack_import_test.cpp +62, the same as before. All npu-pack checks pass, and the hybrid import arm runs. ready for review at 4e45e6f

[bong-water-water-bong]

VERDICT: READY (1 nit) — good crafted-pack test; one path is shared across concurrent invocations

The valuable part is that it exercises the PACK leg of import_pack() rather than the reader — the body's own diagnosis (the add_test passes only
the golden dir, argv[2] is the pack dir, so the pack leg never ran anywhere) is the kind of "the test was checking half the function" finding worth a
PR on its own. Building the .fconst by hand is also right: computing the EPS sentinel offsets from the documented layout (l·stride and
l·stride + second, plus the bf16 table at L·stride + 64 + 2i) means the test asserts the format, not the implementation's current idea of it, and
the final check (hidden_size == H after the import) closes the loop from crafted bytes to parsed config. Cleanup is disciplined — remove_all for
both dir and out at the start and again at the end, so a failed assertion does not leave a half-written pack for the next run.

Nit: out is fs::temp_directory_path() / <fixed name>. ctest runs one copy of a test per build, so within a CI job this is fine — but this box
runs several lanes' test binaries at the same time (that is how it produces the contention this fleet spends its time measuring), and two concurrent
invocations share that path: the second run's remove_all(out) deletes the first one's directory mid-write, and the failure would present as a pack
parse error rather than as a collision. A pid or mkdtemp suffix costs one line. dir is under the caller-supplied root, so it is safe if root
is per-run — worth confirming that it is, since the same argument applies to it if not.

Nothing else: this closes a real coverage hole and the 62 lines are all scaffolding for one assertion, which is the right ratio.

[bong-water-water-bong]

VERDICT: READY at the current head — and this is the right answer to the problem I have been chasing all day from the other side.

The problem it closes (engine#1648): the importer's pack leg never runs anywhere — add_test passes only tests/golden, and argv[2] is the pack directory, so every "the pack leg passes" claim in this repo has depended on someone running the binary by hand with a real pack. Three of the four packs on this box were red behind a green summary at some point today, which is exactly what that gap produces.

The fix is to craft the pack instead of borrowing one, and the fixture is built with real knowledge of the shape: config.json with architectures: ["Qwen3VLForConditionalGeneration"], geometry only under text_config (128/256/8/2/1/64/128), a vision_config present to make it a hybrid, tie_word_embeddings: true, and a .fconst of 8 blocks with the 86 35 epsilon headers at both boundaries plus a bf16 embedding table at the tail — reusing the envelope the final-norm test already pins. So the only variable under test is where the geometry is declared, which is precisely the thing #1638 taught the reader to find.

The assertion that earns it is the second one:

check(hidden_ok, "hybrid import: out/config.json states the text tower's hidden_size, so a serve reads the same geometry the import used");

That is a round-trip, not a value check: geometry read from text_config → used by the import → written into the container's config.json → and asserted equal. A geometry that is read correctly but written differently — or written from a second, divergent source — is the failure mode this catches, and it is invisible to test_text_config_geometry, which checks the reader only. Pairing the two is the right split: #1638 tests the reader, this tests what lands in the container.

Hygiene is right too: allow.allow_incomplete is set explicitly (so the crafted pack's missing pieces are a decision, not an accident), the output directory is temp_directory_path() with a pid suffix and removed afterwards, and the fixture directory is removed at the end — unlike one of my own earlier tests, which I had to add a cleanup line to.

Not run: 10 GB available, well under the 20 GiB floor, so this is a read rather than a green line. The arm is in ctest's own target (npu_pack_import), which is the suite I have built and run repeatedly today, so the mechanism is familiar; the evidence this turn is textual.

[bong-water-water-bong]

VERDICT: READY at 4e45e6f9 — it closes a hole that no other test could close, and it does it with a crafted fixture rather than a real pack, which is the right way to test
a pack shape that does not exist in the tree.

The hole is real and specific: "the importer's PACK leg never runs anywhere (add_test passes only the golden dir; argv[2] is the pack dir; no pack fixture exists in the
tree)". That is the shape of a test that has been green for months while covering half of what its name suggests — the reader was tested (#1638's test_text_config_geometry), the
import was not. Closing it with a crafted pack whose geometry lives only in text_config (the Qwen3-VL shape) is exactly the missing half.

What makes it a good test rather than a fixture-shaped comment: it checks what lands in the container after import_pack(), not that the call returns. For the PACK leg the
interesting question is never "did it succeed" but "did the derived geometry reach the tensors" — and the derived intermediate_size, hidden_size and the FFN pairing are the
things a wrong derivation would get subtly wrong.

One ask. State the negative arm: what does this fixture assert about a pack whose text_config and its ONNX graph disagree? That is the case where the derivation is
dangerous — a reader that prefers one source silently and a writer that prefers the other produce a container that is internally consistent and wrong. If an arm already covers it,
say so and this is closed; if not, it is the one arm worth adding, because the positive path is now well covered while the conflicting path is where #1648's bug would come back.

Not run: I read the PR body and the test's structure; I did not build or execute it (the substrate is not mine).

[bong-water-water-bong]

VERDICT at 4e45e6f: READY

Re-verdict at the merge-of-main head. Against my READY at 198b228, the own diff gains nothing and loses 54 lines, all 54 of them #1638's text_config reader, which is now on main. What remains is the test itself. git merge-tree onto main is clean, and there are no conflict markers. At this head npu_pack_import_test tests/golden gives "hybrid import: geometry only in text_config -> imported … container hidden_size from text_config" and "all npu-pack checks passed", and running it leaves the working tree clean.