NPU GGUF-on-NPU: converter preference, forward re-sync (MLA/MoE), streaming crash fix by bong-water-water-bong · Pull Request #181 · 1bit-MONSTER/engine

bong-water-water-bong · 2026-09-28T00:52:45Z

Three fixes on top of the merged GGUF-on-NPU route (#179), all verified on the real NPU. Goal: `1bit serve -m <model.gguf> --device npu` answers the anchor prompt for the six target arches.

1. `repack: prefer the canonical npu-infer converter` (`b8585ae`)

The resolver fell through to `1bit-MONSTER/third_party/FLM_Q4NX_Converter` (`0ecf09f38`), whose `llama.py` gates the q/k interleaved->paired reorder on `freq_base <= 20000`. MiniCPM5-1B is arch `llama` with `freq_base 5e6`, so the gate skipped the reorder and the forward decoded `" the"` instead of `" Paris"`.

converter	model.q4nx md5	forward token (anchor ids)
`1bit-MONSTER@0ecf09f38` (gated)	`217f0d4d8672786b8b58c42c73474d1c`	285 " the"
`np-model-generic@92f03365d` (canonical)	`685e6e4a5f17651931dea0ae894a86b4`	8181 " Paris"

The canonical md5 is byte-identical to the published FastFlowLM `MiniCPM5-1B-NPU2/model.q4nx`. The dir-cache provenance stamp re-resolves the default converter, so an existing stale dir is now reported stale (`--check-stamp` exits 1) and rebuilt.

2. `forward: re-sync from the canonical npu-infer forward` (`6dc8d8b`)

The engine port was an older snapshot: no MLA + MoE path at all (deepseek2/GLM), no `mla_attn`/`mla_rope`/`mla_expert`/`moe_mlp`, and `pack.c` lacked the QA/QB/KVA/DG/DU/DD design cases. Re-synced `q4nx_forward.{cpp,h}` + `q4nx_pack.{c,h}` from the canonical, re-adding the Apache header and keeping the engine's `lm_head_tiles_` cache (the canonical re-gathers the whole NV x H head with a T-float stride per token). The canonical already carries the MiniCPM4 scales, the LongRoPE divide fix and `NPU_INFER_ROPE_FACTOR_MUL`; the engine's only loss would have been `lm_head_tiles_`, which is preserved. Diff +700/-20; builds clean.

3. `forward-serve: fix the streaming path's dangling capture` (`01355a0`)

The SSE chunked-content-provider lambda captured the handler's locals by reference, but cpp-httplib runs the provider after the handler returns -> every streaming request read destroyed stack and the process aborted (`terminate ... std::system_error: Invalid argument`). The frames are now built inside the handler and handed to the provider by value (`shared_ptr` buffer + cursor).

Verified

`tests/serve_e2e.sh build-npu/1bit MiniCPM5-1B-Q4_K_M.gguf npu` -> PASS: /health 200, /v1/models names the alias, chat answers Paris, stream delivers 32 `data:` chunks.
The canonical repack + forward returns token 8181 " Paris" for the anchor ids.
CodeQL fix from #5ef0f71 (O_NOFOLLOW dump) is preserved through the re-sync (this PR is on top of it).

…k reorder) The engine's repack resolver fell through to ~/1bit-MONSTER/third_party/FLM_Q4NX_Converter (0ecf09f38), whose llama.py gates the q/k interleaved->paired reorder on freq_base <= 20000. MiniCPM5-1B is arch "llama" with freq_base 5e6, so that gate SKIPPED the reorder: the repacked q/k stayed interleaved, the forward rotated them with the paired convention, and the NPU decoded " the" instead of " Paris" (engine#124 goal, task-3 "llama" row). Resolve the canonical npu-infer/model-generic converter first. It applies the reorder for every arch "llama" (only skipping when the GGUF lacks llama.rope.dimension_count) and carries the minicpm module + config, the LongRoPE work and the MiniCPM4 scale fixes the forward was validated against. Verified: its output for MiniCPM5-1B-Q4_K_M is byte-identical to the published MiniCPM5-1B-NPU2 model.q4nx (md5 685e6e4a5f17651931dea0ae894a86b4, where the gated converter gives 217f0d4d8672786b8b58c42c73474d1c), and the model-generic forward on it returns token 8181 " Paris" for the anchor ids. The dir-cache provenance stamp already re-resolves the default converter, so an existing dir built by the gated converter is now correctly reported stale (--check-stamp exits 1) and rebuilt on the next serve.

The engine port was an older snapshot of np-model-generic/npu-infer's q4nx_forward.cpp: it lacked the entire MLA + MoE path (deepseek2/GLM-4.7-Flash, load_layer's mla_ branch, mla_attn/mla_rope/mla_expert/moe_mlp, the moe_router/gate_exps/share_* tensor set) and the pack.c design cases that go with it (QA/QB/KVA/DG/DU/DD). Copy the canonical forward.cpp, forward.h, pack.c and pack.h in, re-adding the Apache header the engine's copyright check requires and re-applying the engine's lm_head_tiles_ cache (the canonical gathers the whole NV x H head with a T-float stride every token; the engine builds the per-tile [H][T] blocks once). No engine-side logic beyond lm_head_tiles_ was lost: the canonical already carries the MiniCPM4 embedding/residual/logit scales, the LongRoPE divide fix and NPU_INFER_ROPE_FACTOR_MUL. Requires model.h's MoE descriptors and the I8Ctx resident-expert helpers, both already present in the engine. Builds clean; MiniCPM5-1B still runs on --device npu.

…ery stream) The SSE chunked-content-provider lambda captured the handler's locals (id, created, object, chat) by reference, but cpp-httplib invokes the provider AFTER the handler returns -- so every streaming request read destroyed stack and the process aborted with an uncaught std::system_error. It also ran the forward inside the provider, i.e. from httplib's write path. Build the SSE frames inside the handler (where the locals are alive), buffer them, and hand the provider a by-value (shared_ptr) buffer plus a cursor, so it owns everything it touches. Verified on MiniCPM5-1B --device npu: a streamed chat emits data: frames ('The', ' capital') and data: [DONE] with the server still healthy, where it previously aborted.

context7 · 2026-09-28T00:52:55Z

Docs7 for 1bit-monster/engine

Result	Status	Action
Deployment	➖ Not used	—
Content review	➖ Did not run. This site has no agent runs available this month. Wait for the monthly reset or check your Docs7 plan.	—

Commit 8a54e74

github-actions · 2026-09-28T00:54:34Z

PR Reviewer Guide 🔍

(Review updated until commit `8a54e74`)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 179 - Partially compliant Compliant requirements: GGUF-on-NPU serving route implemented Model repacking to Q4NX format with proper metadata Support for Qwen2.5-7B, MiniCPM4-8B, MiniCPM5-1B models Correct handling of MiniCPM4-8B specific scales (embedding_scale, residual_scale, logit_scale) Repack guard implemented to fail on critical scale-like scalars Cache-hit validation implemented logit_scale fidelity confirmed Non-compliant requirements: GLM-4.7-Flash support (not claimed as supported) SentencePiece/unigram tokenizer support for MiniCPM4-8B (falls back to bytes) Requires further human verification: Verification of the specific token outputs for all target models Confirmation that the repack guard correctly fails on critical scale-like scalars Validation that the cache-hit validation properly discards and rebuilds on failure
⏱️ Estimated effort to review: 5 🔵🔵🔵🔵🔵
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Potential for incorrect MLA attention implementation The MLA attention implementation in `mla_attn` function may have incorrect handling of the k_b and v_b tensor indexing. The comment mentions that the Q4NX grid for v_b is [kv_lora][v_head] = [512][256] and that this was pinned empirically against llama.cpp's tensor dump. However, the indexing logic in lines 1335-1336 and 1336-1337 may not correctly handle the tile grid mapping, which could lead to incorrect attention computation. This is particularly concerning as it's a critical path for deepseek2/GLM models. const float* wk = &w_kb_[(size_t)h * (size_t)kv_lora_ * (size_t)kb_rows_]; const float* wv = &w_vb_[(size_t)h * (size_t)kv_lora_ * (size_t)vh]; float* dk = &kn[old * (size_t)NH * kh + (size_t)h * kh]; float* dv = &vc[old * (size_t)NH * vh + (size_t)h * vh]; for (int n = 0; n < kh; n++) { float s = 0.0f; Incorrect weight caching logic for MLA layers The weight caching logic in `npu_gemm` function (lines 1200-1218) may not correctly handle the caching of weights for MLA layers. The code checks for `weight_cache_ && wkey >= 0` but doesn't explicitly check if the current layer is an MLA layer. This could lead to incorrect caching behavior for MLA layers where weights are not truly token-invariant. if (weight_cache_ && wkey >= 0 && cur_layer_ >= 0 && cur_layer_ < c_.NL && dev_) { if (int(wcache_.size()) <= cur_layer_) wcache_.resize(size_t(cur_layer_) + 1); auto& m = wcache_[size_t(cur_layer_)]; auto it = m.find(wkey); if (it == m.end()) { PackedWeight pw; pw.bo = ctx.make_weight_bo(dev_); float sout = 1.0f; ctx.packB_into(pw.bo, Bmat, K, N, sout, pw.scales); it = m.emplace(wkey, std::move(pw)).first; } slot = &it->second; gs = &slot->scales; } else { float sout = 1.0f; ctx.packB(0, Bmat, K, N, sout); ctx.quantize_async(A, 1, K, as); gs = &ctx.group_scales[0]; } Potential memory access violation in MoE implementation In the `moe_mlp` function, there's a potential memory access violation in the loop that processes selected experts (lines 1478-1511). The code accesses `exp_g_`, `exp_u_`, and `exp_d_` without checking if they are properly initialized or if the expert index `e` is within valid bounds. This could lead to a crash or incorrect behavior when processing MoE layers. for (int e : sel) { if (!mla_expert(exp_g_, exp_g_bytes_, e, expert_im_, H, g)) { err_ = "expert gate"; return false; } if (!mla_expert(exp_u_, exp_u_bytes_, e, expert_im_, H, u)) { err_ = "expert up"; return false; } if (!mla_expert(exp_d_, exp_d_bytes_, e, H, expert_im_, dd)) { err_ = "expert down"; return false; } // gate/up are GEMMs against xn (the dequantized matrices are weights, not // outputs): run them on the NPU through the G/U designs. { const Q4nxDesignGeom* gg = &designs_[i_g]; Q4nxProjections p{}; p.gate = g.data(); if (q4nx_assemble_B(gg, &p, B_.data()) != 0) { err_ = "assemble expert gate"; return false; } npu_gemm(ctx_[i_g], xn.data(), gg->K, gg->N, B_.data(), go); } { const Q4nxDesignGeom gu = &designs_[i_u]; Q4nxProjections p{}; p.up = u.data(); if (q4nx_assemble_B(gu, &p, B_.data()) != 0) { err_ = "assemble expert up"; return false; } npu_gemm(ctx_[i_u], xn.data(), gu->K, gu->N, B_.data(), uo); } std::vector<float> act((size_t)expert_im_); for (int i = 0; i < expert_im_; i++) act[i] = (go[i] / (1.0f + std::exp(-go[i]))) uo[i]; const Q4nxDesignGeom* gd = &designs_[i_d]; std::vector<float> ed; Q4nxProjections p{}; p.down = dd.data(); if (q4nx_assemble_B(gd, &p, B_.data()) != 0) { err_ = "assemble expert down"; return false; } npu_gemm(ctx_[i_d], act.data(), gd->K, gd->N, B_.data(), ed); if (dump) { for (float f : go) moe_g += f; for (float f : uo) moe_u += f; for (float f : act) moe_sw += f; for (float f : ed) moe_dn += f; } for (int i = 0; i < H; i++) d[i] += r[e] ed[i]; } Incorrect handling of LongRoPE scaling The LongRoPE scaling logic in `attention` function (lines 1640-1646) has a conditional check that may not correctly handle all cases. The code checks for `NPU_INFER_ROPE_FACTOR_MUL=1` to keep the old multiply behavior, but the comment suggests that the default behavior should be to divide the inv_freq. This could lead to incorrect RoPE application for models that rely on the new scaling method. // LongRoPE (MiniCPM4): ggml's rope applies the factor as // `rope_yarn(theta / ff, ...)` (ggml-cpu/ops.cpp // ggml_rope_cache_init), i.e. the angle is DIVIDED by // freq_factors. So divide the inv_freq here. Multiplying (the // engine route's 599ac77) blows the high dims up by up to 31x // and yields garbage logits. NPU_INFER_ROPE_FACTOR_MUL=1 keeps // the old multiply for A/B. Potential dangling reference in SSE streaming The SSE streaming implementation in `handle_generate` function (lines 200-234) uses `std::make_shared` to capture `chunk_tpl` and `chunks` by value, but the lambda function captures `chunks` by reference. This could potentially lead to a dangling reference if the shared pointer is not properly managed, although the code attempts to address this by moving the forward run into the handler. // Build the SSE frames while the handler still owns its locals. The // content provider below runs AFTER this handler returns, so it must // not capture anything by reference (id/created/object die here) -- // that was a dangling-reference crash (`std::system_error`). Run the // forward here too, so it is not invoked from httplib's write path. json chunk_tpl; chunk_tpl["id"] = id; chunk_tpl["object"] = "chat.completion.chunk"; chunk_tpl["created"] = created; chunk_tpl["model"] = e.id; auto chunks = std::make_shared<std::vector<std::string>>(); run([&](const std::string& t) { json chunk = chunk_tpl; if (chat) chunk["choices"] = json::array({{{"index", 0}, {"delta", {{"content", t}}}, {"finish_reason", nullptr}}}); else chunk["choices"] = json::array({{{"index", 0}, {"text", t}, {"finish_reason", nullptr}}}); chunks->push_back("data: " + chunk.dump() + "\n\n"); return true; }); auto cursor = std::make_shared<size_t>(0); res.set_chunked_content_provider("text/event-stream", [chunks, cursor](size_t, httplib::DataSink& sink) { while (cursor < chunks->size()) { const std::string& d = (chunks)[cursor]; if (!sink.write(d.data(), d.size())) return false; ++cursor; } const std::string done = "data: [DONE]\n\n"; sink.write(done.data(), done.size()); sink.done(); return true; });

⚠️ Review coverage: The following files were not included in this review because of the token budget:

scripts/repack_gguf.py

…ks for Q4_0 The NPU forward reads Q4_1 tiles (5120 B: 512 B bf16 scales + 512 B bf16 zeros + 4096 B packed int4). configs/qwen2.json -- the only config for a target arch -- shipped default_tensor_type Q4_0, which packs no zero half. The forward then decoded the entire model wrong, not slightly: '1bit serve -m qwen2.5-7b-instruct-q4_k_m-merged.gguf --device npu' answered a CJK token instead of ' Paris', while the published FastFlowLM Qwen2.5-7B-NPU2 dir answered correctly on the same forward. Repacking that same GGUF with default_tensor_type Q4_1 produces model.q4nx md5 f56e41b7cdc84f3c3a333ec0b96e03e9 -- byte-identical to the published dir (Q4_0: f89faa2f...). Every other target arch (llama, qwen3, minicpm) already ships Q4_1; only qwen2 and lfm2 shipped Q4_0. The repack now runs the converter from a patched copy when this arch's config asks for anything but Q4_1, so the engine's tile format no longer depends on a moving external checkout. The provenance stamp records q4nx_default_tensor_type=Q4_1 and --check-stamp compares it, so a dir built with Q4_0 is reported stale and rebuilt (verified: the old cache dir exits 1, a fresh dir exits 0) instead of being served silently.

github-actions · 2026-09-28T02:05:25Z

Persistent review updated to latest commit 0c87648

The converter emits a tokenizer.json whose model.type is "BPE" with an empty merge list and a ByteLevel pre-tokenizer for SentencePiece GGUFs (MiniCPM4-8B's tokenizer.ggml.model is "llama"), but the vocabulary is SPM pieces ("▁The", "▁capital", ...). Byte-level BPE on that vocabulary has no merges to apply, so '1bit serve --device npu' fell back to roughly one id per byte and the model could not be driven by text at all. Detect that shape (empty merges + "▁"-prefixed pieces) and encode with sentencepiece rules instead: spaces become U+2581 with a leading one, greedy longest-match over the vocabulary, and <0xNN> byte fallback for anything unmatched. Decode joins pieces, maps U+2581 back to a space and expands <0xNN>. Verified against the oracle ids for the anchor: the lane tokenizer now encodes "The capital of France is" to 1507 8107 1379 8360 1410, exactly the ids the MiniCPM4-8B NPU capture used. Byte-level BPE models are unaffected (Qwen3-0.6B still encodes it to 785 6722 315 9625 374).

github-actions · 2026-09-28T02:25:56Z

Persistent review updated to latest commit 868c45a

Every token re-dequantized the Q4NX (load_layer) and re-quantized it to int8 (I8Ctx::packB) for every GEMM -- both O(model) per token, so a served answer cost 12 min on Qwen2.5-7B and did not return in 35 min on MiniCPM4-8B. Token-INVARIANT projections now get their own weight BO, packed once on first use and reused through launch_async_with_bo; the per-token dequant and the f32->B assembly are skipped for a cached layer. Routed-expert weights vary per token and pass wkey < 0, so they keep re-packing. ELF mode had no kernel object to take a memory group from (k is null), so make_weight_bo now allocates through xrt::ext::bo like init_elf's mkbo. Verified: Qwen2.5-7B still answers ' Paris' and its capture drops 12m11s -> 5m11s; MiniCPM4-8B answers ' Paris' on --device npu in 22m18s (previously did not return).

github-actions · 2026-09-28T05:07:47Z

Persistent review updated to latest commit 6bbd144

The canonical npu-infer converter has no deepseek2 module, so a GLM-4.7-Flash repack failed outright. Resolution is now arch-aware: among the candidate checkouts, prefer one that actually ships q4nx/models/<arch>.py (the iso-build checkout has deepseek2.py and is what built the Q4NX the forward's MLA+MoE path was validated against), falling back to the default. --check-stamp re-resolves with the directory's own model_type so a GLM dir keeps comparing against the converter that can repack it. Also adds NPU_NO_WCACHE=1 to disable the packed-weight cache for A/B. NOTE: this makes the GLM route select the iso-build deepseek2.py, whose k_b grid is [20][16][5120] (16 tiles/head); the forward reads the re-converted [20][12][5120] (12 tiles/head) and segfaults on it. GLM therefore still needs the re-converted deepseek2 module before --device npu can answer; until then the route crashes instead of failing with 'no module'. The arch-aware selection is the enabler for that, and changes nothing for the other arches.

github-actions · 2026-09-28T05:23:27Z

Persistent review updated to latest commit 36f43ae

…ow a symlink CodeQL (6 high on #181): three debug dumps opened their env-named path with fopen(..., "wb"), which creates the file 0666 (less umask) and follows symlinks. They now go through open_debug_dump(): open(O_WRONLY|O_CREAT|O_TRUNC|O_NOFOLLOW, 0600) + fdopen, the pattern the hidden-state dump in the same file already used. Dumps only happen when a developer sets NPU_INFER_DUMP_ATTN / _LN / _L0 / _HEAD. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-09-28T05:28:30Z

Persistent review updated to latest commit 96721e5

…EBUG_DUMPS) NPU_INFER_DUMP_ATTN, _LN/_L0 and _HEAD write to a path taken from the environment (CodeQL: tainted path, 3 high on #181). Owner's call: compile them only when -DONEBIT_NPU_DEBUG_DUMPS=ON (defines NPU_INFER_DEBUG_DUMPS on onebit_npu_forward); release builds carry no env-named file writes. The stderr-only dumps and the fixed-path hidden-state dump are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-09-28T05:33:07Z

Persistent review updated to latest commit 6b97580

github-actions · 2026-09-28T05:35:09Z

Persistent review updated to latest commit 8a54e74

…UF on the NPU, 1BP; blog post (#214) README gets a Direction note (HRX with Loom kernels plus the NPU, as geramyL proposed; Vulkan stays the default until the RFC #213 gates are met) and current status lines: six GGUF architectures answer on the NPU (0.006-0.17 tok/s), DwarfStar reads 1BP, and Laya classifies conversations with its scorer on HRX at 15-16 ms. PORTING's NPU and Laya rows gain #179/#181/#209 and #187/#206/#208. The blog post covers the same. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

agent-872bf5 added 3 commits September 27, 2026 21:52

github-actions Bot added the Review effort 5/5 label Sep 28, 2026

github-advanced-security Bot found potential problems Sep 28, 2026

View reviewed changes

github-actions Bot added Review effort 4/5 and removed Review effort 5/5 labels Sep 28, 2026

github-actions Bot added Review effort 5/5 and removed Review effort 4/5 labels Sep 28, 2026

github-actions Bot added Review effort 4/5 and removed Review effort 5/5 labels Sep 28, 2026

github-advanced-security Bot found potential problems Sep 28, 2026

View reviewed changes

Comment thread npu/forward/src/q4nx_forward.cpp Fixed

Comment thread npu/forward/src/q4nx_forward.cpp Fixed

Comment thread npu/forward/src/q4nx_forward.cpp Fixed

github-actions Bot added Review effort 5/5 and removed Review effort 4/5 labels Sep 28, 2026

Merge branch 'main' into npu-gguf-fixes

8a54e74

bong-water-water-bong merged commit 02b5578 into main Sep 28, 2026
9 checks passed

bong-water-water-bong deleted the npu-gguf-fixes branch September 28, 2026 05:36

Conversation

bong-water-water-bong commented Sep 28, 2026

1. `repack: prefer the canonical npu-infer converter` (b8585ae)

2. `forward: re-sync from the canonical npu-infer forward` (6dc8d8b)

3. `forward-serve: fix the streaming path's dangling capture` (01355a0)

Verified

Uh oh!

context7 Bot commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 8a54e74)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 28, 2026

Uh oh!

github-actions Bot commented Sep 28, 2026

Uh oh!

github-actions Bot commented Sep 28, 2026

Uh oh!

github-actions Bot commented Sep 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 28, 2026

Uh oh!

github-actions Bot commented Sep 28, 2026

Uh oh!

github-actions Bot commented Sep 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

1. `repack: prefer the canonical npu-infer converter` (`b8585ae`)

2. `forward: re-sync from the canonical npu-infer forward` (`6dc8d8b`)

3. `forward-serve: fix the streaming path's dangling capture` (`01355a0`)

context7 Bot commented Sep 28, 2026 •

edited

Loading

github-actions Bot commented Sep 28, 2026 •

edited

Loading

(Review updated until commit `8a54e74`)