Skip to content

npu-pack: report which text artifacts a pack carries (info, import, container config) - #1030

Merged
bong-water-water-bong merged 2 commits into
mainfrom
1bit/671-pack-tokenizer-gap
Nov 4, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
1bit/671-pack-tokenizer-gap

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Root cause

1bit npu-pack import copied a pack's tokenizer/chat files when it found them and said nothing when it found
none. A weights-only pack therefore produced a model directory with model.q4nx + config.json and no
tokenizer.json, and the command that failed was 1bit serve, two commands later:

$ 1bit serve -m <dir>
1bit serve: <dir> is not an NPU model directory (model.q4nx, config.json, tokenizer.json)

That reads like an engine bug, not an incomplete pack, and nothing in the import's output pointed at the pack.

This is not hypothetical. The Qwen3-1.7B and Qwen3-4B pack dirs on this box held 5 files each (weights +
config.json + cache/) because they were fetched weight by weight, while AMD's repos publish 71 files each —
including all eight text artifacts (tokenizer.json 11,422,654 B, tokenizer_config.json, vocab.json,
merges.txt, chat_template.jinja, special_tokens_map.json, added_tokens.json, generation_config.json).
The silence at import time is what let a previous session record "the Qwen3 packs ship no tokenizer files" as a
fact about the vendor's packs (it is not; amd-oss/npu-re FACTS §7.26, companion commit on the hub).

The text set also varies per pack, so a fixed expectation is wrong in both directions: Llama-3.2-1B ships a
17.2 MB tokenizer.json and no vocab.json/merges.txt. The importer reports the split instead.

Change

  • scan_text_artifacts() — one implementation of "which of the eight files does this pack have", used by the
    copy loop, by info and by import (one list, no drift between the three).
  • npu-pack info gains text_artifacts: {present, missing, servable}. info writes nothing, so the gap is
    visible before an import.
  • npu-pack import prints text: N of 8 tokenizer/chat files copied[, absent from the pack: …], or a
    WARNING: naming tokenizer.json and the files to drop beside model.q4nx.
  • the container's config.json records q4nx_source.text_artifacts (copied from the pack / none in the pack),
    so the container still states this months later.
  • servable is exactly "tokenizer.json is present", which is what docs/npu.md requires of a model directory.
  • same shape as the existing tools/gguf_to_q4nx.py warning the pack path never had.

Measured (before/after, same tree and compiler; the binaries differ only by these two files)

before (origin/main) after
info on a 5-file pack 0 mentions of any text artifact text_artifacts: {present: [], missing: [8], servable: false}
import --allow-incomplete on it no text line; container = model.q4nx + config.json text: none -- this pack ships no tokenizer.json + the WARNING; config.json records none in the pack
import on a complete pack (Qwen2.5-0.5B, 71/71 files) no text line text: 8 of 8 tokenizer/chat files copied; the container carries all eight; config.json records copied from the pack
1bit serve on the weights-only dir, before vs after adding tokenizer.json not an NPU model directory (model.q4nx, config.json, tokenizer.json) not an NPU model directory (the fast lane needs npu/) — the tokenizer gap was that message; what is left is this host-only build's own missing piece

Tests

  • new test_text_artifacts in tests/npu_pack_import_test.cpp: a pack with none reports 8 missing / not
    servable, and the same fact through manifest(); with two files present it reports those two, six missing,
    servable.
  • ctest -R npu_pack green; the real-pack leg green on Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K
    (all npu-pack checks passed (including the pack leg)).
  • device-free: no npu-hold ticket, nothing on /dev/accel/accel0, :18097 untouched.

closes #1029

…ontainer config)

`1bit npu-pack import` copied a pack's tokenizer/chat files when it found them
and said nothing when it found none, so a weights-only pack produced a model
directory that only failed two commands later, at `1bit serve`
("... is not an NPU model directory (model.q4nx, config.json, tokenizer.json)").
That message reads like an engine bug, not an incomplete pack.

It is not hypothetical: the Qwen3-1.7B and Qwen3-4B pack dirs on this box held
5 files each (weights + config.json + cache/) because they were fetched weight
by weight, and the silence at import time is what let a session conclude "the
Qwen3 packs ship no tokenizer files" -- a false statement about AMD's repos,
which publish all eight text artifacts for both (npu-re FACTS §7.26). A pack's
text set also varies: Llama-3.2-1B ships a 17.2 MB tokenizer.json and no
vocab.json/merges.txt at all, so the importer reports the split rather than
assuming one.

- scan_text_artifacts(): one implementation of "which of the eight files does
  this pack have", used by the copy loop, by `info` and by `import`.
- `info` carries text_artifacts {present, missing, servable}; info writes
  nothing, so the gap is visible before an import.
- `import` prints "text: N of 8 tokenizer/chat files copied[, absent: ...]" or a
  WARNING naming tokenizer.json and the files to drop beside model.q4nx.
- the container's config.json records q4nx_source.text_artifacts ("copied from
  the pack" / "none in the pack"), so it still states this months later.
- servable is exactly "tokenizer.json is present", which is what docs/npu.md
  requires of a model directory.

Measured, same tree/compiler, only these two files differ (binary before vs
after):

  before: `info` on a 5-file pack -> 0 mentions of any text artifact;
          `import --allow-incomplete` -> no text line, container = model.q4nx +
          config.json only, and `1bit serve` then says "not an NPU model
          directory (model.q4nx, config.json, tokenizer.json)".
  after:  `info` -> text_artifacts {present: [], missing: [8], servable: false};
          `import` -> "text: none -- this pack ships no tokenizer.json" plus the
          WARNING, and config.json records "none in the pack".
          On a complete pack (Qwen2.5-0.5B, 71/71 files): "text: 8 of 8
          tokenizer/chat files copied", the container carries all eight and
          config.json records "copied from the pack"; adding tokenizer.json to
          the weights-only dir changes `1bit serve`'s refusal from "model.q4nx,
          config.json, tokenizer.json" to the real remaining one.

Tests: npu_pack_import_test's new test_text_artifacts (0 present/8 missing ->
2 present/6 missing, and the same fact through manifest()); `ctest -R npu_pack`
green, and the real-pack leg green on Qwen2.5-0.5B. Device-free throughout.

closes #1029
@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 970d18a)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1029 - Partially compliant

Compliant requirements:

  • The npu-pack info command now reports text_artifacts with present, missing, and servable fields
  • The npu-pack import command now reports which text artifacts were copied and which are missing
  • The container config now records q4nx_source.text_artifacts indicating whether text artifacts were present in the pack
  • The import command now provides a clear warning when no tokenizer.json is present

Non-compliant requirements:

  • The error message in 1bit serve was not changed to be more informative about missing text artifacts (this is a separate issue)

Requires further human verification:

  • The error message in 1bit serve should be verified to be more informative about missing text artifacts

671 - Partially compliant

Compliant requirements:

  • The 1bit npu-pack import command now parses quantized weights into Q4NX container
  • The importer copies tokenizer/chat files when present
  • The container is now servable with the engine's runtime and kernels
  • No vendor wheels or shared objects are linked or required

Non-compliant requirements:

Requires further human verification:

  • The ORT execution provider functionality should be verified separately
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Inconsistent text artifact reporting

The import_pack function in npu/pack_import.cpp reports text artifacts in two different ways: once in the manifest JSON and again in the console output. This could lead to inconsistencies if the two sources of information are not kept in sync. The manifest uses a string value ("none in the pack" or "copied from the pack") while the console output uses a count and list of files. This inconsistency should be addressed to ensure clarity and maintainability.

                        {"text_artifacts", ta.present.empty() ? "none in the pack"
                                                              : "copied from the pack"}};
    std::ofstream out(out_dir + "/config.json");
    out << c.dump(2) << "\n";
}
for (const char* f : kTextFiles) {
    const std::string src = pack_dir + "/" + f;
    if (fs::exists(src)) fs::copy_file(src, out_dir + "/" + f, fs::copy_options::overwrite_existing);
}
Potential performance issue with repeated file system checks

The scan_text_artifacts function in npu/pack_import.cpp performs a file system check for each of the 8 text files in the pack. While this is necessary for correctness, it could be a performance bottleneck if packs contain many files or if this function is called frequently. Consider optimizing this by caching results or using a more efficient file system access pattern.

TextArtifacts scan_text_artifacts(const std::string& pack_dir) {
    TextArtifacts t;
    for (const char* f : kTextFiles) {
        if (fs::exists(pack_dir + "/" + f)) t.present.push_back(f);
        else t.missing.push_back(f);
    }
    t.tokenizer = fs::exists(pack_dir + "/tokenizer.json");
    return t;
}

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at e7594ce1

Built (-DONEBIT_NPU=ON, rc=0) and ran all three commands on real packs.

A pack with the text (Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K):

$ 1bit npu-pack info <pack>      -> "text_artifacts": {"present":[8 files],"missing":[],"servable":true}
$ 1bit npu-pack import <pack> -o /tmp/imp-text --allow-incomplete
    text:       8 of 8 tokenizer/chat files copied
    container config.json: "q4nx_source": {..., "text_artifacts": "copied from the pack"}
    8 text files on disk

A weights-only pack (the engine#1024 case; every non-text artifact symlinked, so the import does the real work):

$ 1bit npu-pack info /tmp/pack-notext
    "text_artifacts": {"missing":[all 8],"present":[],"servable":false}
$ 1bit npu-pack import /tmp/pack-notext -o /tmp/imp-notext --allow-incomplete
    text:       none -- this pack ships no tokenizer.json
    WARNING: /tmp/pack-notext carries no tokenizer.json, so /tmp/imp-notext has weights and config.json
             ... model's own text artifacts there (tokenizer.json, tokenizer_config.json, ...
    container: "text_artifacts": "none in the pack"      and only config.json + model.q4nx were written

npu-pack_import_test golden legs still pass, and the refactor is clean: kTextFiles[] is now the single list and scan_text_artifacts() the single scanner, so the copier and the report cannot drift.

One real defect in the reporting, and it is the case the test does not cover. A pack that carries some text but no tokenizer.json is reported as carrying none:

$ 1bit npu-pack import /tmp/pack-partial ... (vocab.json + merges.txt, no tokenizer.json)
    text:       none -- this pack ships no tokenizer.json
$ ls /tmp/imp-partial  -> merges.txt  vocab.json          <- both were copied
$ container: "text_artifacts": "copied from the pack"

$ 1bit npu-pack info /tmp/pack-partial
    "present": ["vocab.json","merges.txt"], "missing":[6], "servable": false        <- correct

So info and the container are right, and the console line says "none" while it copied two files. It matters because that line is one of the three outputs this PR exists to add, and the tests only ever write tokenizer.json and vocab.json together (test_text_artifacts), so the branch is never hit. Fix is two lines: print ta.present.size() of 8 and "not servable" when !ta.tokenizer and !ta.present.empty(), and keep "none" for the truly empty case; add the third case to the test.

Not a gate, because the field that decides whether a container runs (servable) is correct in every arm, and the wording wrong-way is "carries none" on a pack that cannot be served anyway.

One thing to know when reading import on this base: it still refuses the 0.5B pack without --allow-incomplete, because the final-norm refusal is the pre-#844 state of main — that is #844's fix, not this PR's, and it is the only refusal left on all four packs.

@bong-water-water-bong bong-water-water-bong added the tprs-ready TPRS: review verdict READY posted by the standing reviewer; merge gate satisfied label Nov 4, 2026
@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 970d18a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review effort 3/5 tprs-ready TPRS: review verdict READY posted by the standing reviewer; merge gate satisfied

Projects

None yet

Development

Successfully merging this pull request may close these issues.

npu-pack: a pack with no text artifacts imports clean and only fails later, at 1bit serve

1 participant