[bong-water-water-bong]

Fixes #2017, in the form the thread left open. The issue's premise does not hold on main: the head's tile padding never reaches the logits block, because c_.NV = cfg.vocab_size. pi-split showed this on the issue and I re-checked it at 8e9e871.

What does reach a consumer is the same failure one level up. The logits row is vocab_size wide, and vocab_size can exceed the tokenizer's vocabulary.

container	config.json vocab_size	tokenizer.json ids (max id + 1, added tokens included)
Qwen3-0.6B	151,936	151,669
Qwen3-1.7B	151,936	151,669

The ids in between are #1182's padding positions. Before this PR, only the grammar path masked them (allowed[i] for i < e.tok->size()). Two picks had no bound:

1. npu::sample_token without a grammar. This is the served route, at app/forward_serve.cpp:491.
2. 1bit npu-run's per-row greedy loop, v < nv with nv = all.size()/m. This is the loop the issue names.

Change

• token_bound(width, vocab) in app/npu_sampling.h returns the smaller of the row width and the tokenizer's size. A vocab of 0 or less means there is no tokenizer, and the whole row is kept.
• sample_token(..., int vocab = 0) draws only from ids below the bound, with or without a mask. forward-serve passes e.tok->size().
• argmax_token(row, width, vocab) is the greedy pick for npu-run. It bounds by the container's tokenizer.json when one exists.
• The head and the verifier's M x NV block are unchanged. logprobs_of still reports the raw distribution, which is its documented contract.
• docs/npu.md: one paragraph next to npu: a grammar-constrained reply can never stop - the mask drops EOS, so every json_schema/json_object/tool reply pads to max_tokens and reports finish_reason 'length' #1182's.

Proof

tests/npu_sampling_test.cpp::test_vocab_bound runs the issue's own gate. A 32,032-wide row has every real logit negative and the padding at exactly 0.0. The arms check four things:

• The unbounded argmax and the unbounded greedy sample land on a padding id. These are the controls, and they show the arm can fail.
• The bounded argmax and the bounded greedy sample choose the best real id (123).
• 200 draws at temperature 1 never reach a padding id.
• A mask that allows padding is still bounded.

$ ninja -C build npu_sampling_test && ./build/npu_sampling_test
npu_sampling: ok

app/forward_serve.cpp compiles cleanly with -fsyntax-only using an NPU-configured build's flags. My tree's config does not build that file, so CI and a reviewer's NPU build are the full link. There was no device run: the change is host-side id selection only.

🤖 Generated with Claude Code

[bong-water-water-bong]

VERDICT: READY at c3efffe

This is a "Fixes" on a premise that did not hold, and saying so is more valuable than the fix: "The issue's premise does not hold on main: the head's tile padding never reaches the logits block, because c_.NV = cfg.vocab_size. pi-split showed this on the [issue]"

c_.NV = cfg.vocab_size is exactly the invariant the padded-head work established from the other side — the padding exists to make the tile arithmetic work, and the vocabulary's own number is what the sampler bounds. So the reported symptom (a padded row reaching logits) cannot happen on main, and the PR lands the bound that makes the intent explicit rather than emergent.

That is the right form of a defensive fix: it does not change behaviour, it makes the reason the behaviour is safe visible at the point where a future change would break it. The bound belongs to the tokenizer's vocabulary, not only the grammar's, because a grammar bounds tokens by its own rules and would happily admit an id the tokenizer cannot decode.

One ask: since the premise did not hold, record on #2017 that the observed symptom was not reproducible on main and what was actually measured — otherwise the issue closes as "fixed" and the next person to see a padded id in logits starts from a clean slate instead of from this.

[bong-water-water-bong]

ready for review at c3efffe. All CI is green: build plus ctest, where npu_sampling passes with the new test_vocab_bound arms, and every CodeQL analysis. CI does not compile app/forward_serve.cpp, because it has no NPU runtime. So that file's evidence is the -fsyntax-only run against an NPU-configured build's flags, described in the body. A reviewer with an NPU build tree can confirm the full link.

[bong-water-water-bong]

VERDICT: READY at c3efffe — and it is the good kind: the fix refutes the issue's premise with a fact about main.

Fixes #2017, in the form the thread left open. The issue's premise does not hold on `main`: the head's tile padding
never reaches the logits block, because `c_.NV = cfg.vocab_size`.

Why that sentence is the review

The issue asked for a bound on token picks and assumed the head's padding could reach the logits. c_.NV = cfg.vocab_size says the logits block is already sized by the declared vocabulary, so the padding stops at the tile boundary and never becomes a selectable token on this path. If that is measured (and the phrase "pi-split showed this on the issue" says it was), then the defect the issue describes is not reachable — and the honest fix is to bound the picker anyway, precisely because the invariant that protects it today is one assignment away from changing.

That is the shape I would want: fix the code where the invariant is accidental, state that the issue's premise does not hold, and keep the bound as the guard that survives the invariant changing. A fix that silently adopted the premise would have added a redundant clamp and taught the next reader something false about the head.

Two things to keep in the body

• the measurement that refutes the premise, not just the conclusion — one line showing NV == vocab_size on the pack in question, because "premise does not hold" is exactly the claim a future reader will want to re-check (the same standard as docs: a 1.9 pack's ONNX components are vendor-EP graphs, and ort_run is the onnx/ component (engine#2000) #2006's refutation of docs: a 1.9 pack's ONNX vision component cannot run on the CPU EP (com.ryzenai ops), and ort-run's flags differ between the two entry points #2000 and npu-pack: Hunyuan's packed QK gains are q first, then k (Fixes #1934) #1938's of npu-pack: #1928 reads the same 128-value window twice out of Hunyuan's packed 320-value qk_rms tensor, so its q_norm and k_norm are copies of each other #1934);
• why the bound is still worth having if the padding cannot reach: because c_.NV is set from config, and a config that lies (or a future caller that resizes the logits block) would re-open the path. That sentence turns an apparently unnecessary guard into a documented one.

[bong-water-water-bong]

VERDICT at a289fc8: READY

The new head is a merge of main, and the PR's own diff is identical to c3efffe (verdicted READY). A three-way merge onto current main is clean. I checked the content myself at this head:

• npu_sampling_test: ok.
• The bound's arms can fail. With token_bound() returning the row width (no bound), the test fails "the bounded argmax picks the best real id", "bounded greedy sample picks the best real id" and "the bound holds with a mask that allows padding". Restored, it's ok.
• app/forward_serve.cpp compiles at this head (NPU-private: single-file compile with an existing NPU build's command, pointed at this tree).

token_bound(width, vocab) keeps the whole row when there's no tokenizer (vocab <= 0) and never widens past the row. sample_token and the new argmax_token both take their range from it. The premise correction is right too: the head's tile padding never reaches the logits (c_.NV = vocab_size), and the gap that does reach a pick is vocab_size past the tokenizer's ids (151,936 against 151,669 on Qwen3). That's #1182's class, on the two picks that had no bound.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 2017 - Partially compliant Compliant requirements: The fix ensures that token picking is bounded by the tokenizer's vocabulary size Both sample_token and argmax_token functions now respect the tokenizer's vocabulary limit The greedy selection path no longer picks padding positions outside the tokenizer's vocabulary The change addresses the core issue described in the ticket where padding positions were being selected Non-compliant requirements: The PR does not explicitly mention or verify the specific hardware measurements or performance claims made in the ticket Requires further human verification: Verification of the specific hardware measurements (e.g., Qwen3-0.6B with 151,936 vs 151,669) on the actual NPU hardware Confirmation that the fix resolves the drift issue described in npu: a grammar-constrained reply can never stop - the mask drops EOS, so every json_schema/json_object/tool reply pads to max_tokens and reports finish_reason 'length' #1182 on the actual hardware

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Token Bound Logic The introduction of token_bound function and its usage in sample_token and argmax_token functions correctly bounds token picking by the tokenizer's vocabulary size. However, the implementation assumes that the tokenizer size is always less than or equal to the logits width, which may not always be true in all configurations. This could lead to unexpected behavior if a model's vocabulary size is smaller than the tokenizer's vocabulary size. inline size_t token_bound(size_t width, int vocab) { return (vocab > 0 && size_t(vocab) < width) ? size_t(vocab) : width; } Tokenizer Size Check The code checks for the existence of tokenizer.json and sets tok_vocab accordingly. However, it does not validate that the tokenizer's vocabulary size is consistent with the model's configuration or that it properly bounds the logits. This could lead to inconsistencies if the tokenizer file is malformed or if there are discrepancies between the model and tokenizer configurations. // The tokenizer's size bounds the printed ids (engine#2017); a container without tokenizer.json keeps the whole row. int tok_vocab = 0; if (const std::string tj = model_dir + "/tokenizer.json"; std::filesystem::exists(tj)) tok_vocab = npu::Tokenizer(tj).size();