[bong-water-water-bong]

Fixes #2272. Host-side only — no device, no NPU ticket.

The defect

logprobs_of() normalised over every element of the row it was given:

for (const float v : logits) max_l = std::max(max_l, v);
for (const float v : logits) sum   += std::exp(double(v) - double(max_l));

A padded head stores rows past the vocabulary (engine#2026 — Qwen3: 151,936 rows against 151,669 ids), and those rows are not tokens. So the chosen id was always real while the softmax it was divided by included the padding's mass. sample_token() in the same header already bounds itself with token_bound(); only the report did not.

Why it hid: the shift is systematic per model rather than random, so it reads as a calibration difference, and every path-vs-path comparison — speculative vs plain, before vs after a refactor — agrees with itself while both are wrong.

The change

• logprobs_of(logits, vocab, chosen, top_n) takes the tokenizer's bound and uses the header's own token_bound() idiom for the normaliser, the max_l scan and the top_logprobs candidate list.
• The old three-argument form stays for a row that is the vocabulary, documented as such, so existing callers and tests keep their meaning.
• The one production call site passes e.tok->size() — the same bound sample_token() is given three lines away, which is the consistency that was missing.

Verified by a mutant, not by reading

With the bound ignored:

FAIL bounded greedy sample picks the best real id
FAIL the bound holds with a mask that allows padding
FAIL a padded row: the logprob is the vocabulary-only softmax, not the padding's
FAIL a padded row: no id at or past the vocabulary appears in top_logprobs

and with it, npu_sampling: ok. The two new arms build their expected value from a vocabulary-only softmax constructed in the test, not through the helper, so they cannot agree with a wrong helper by construction — which is the only kind of check that can catch a defect living in a shared function. The padded fixture gives the padding the largest logits (50.0f), so the shift cannot be mistaken for rounding.

Found while reviewing #2267, which passes an explicitly full-width row on the speculative path; that review's CHANGES stands and this fix covers both paths.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 2272 - Partially compliant Compliant requirements: Fix the logprobs normalisation to use the tokenizer's bound instead of the head's padded width Ensure that padding ids do not contribute to the softmax calculation Ensure that top_logprobs do not contain ids beyond the vocabulary Maintain backward compatibility for non-padded models Add tests to verify the fix Non-compliant requirements: None Requires further human verification: None

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Logprob Normalization Overhead The change correctly normalizes logprobs over the vocabulary bound instead of the full logits vector, which prevents padding IDs from affecting the softmax calculation. This addresses the core issue described in the ticket where padded-head models were incorrectly shifting logprob values due to including padding in the normalization. inline TokenLogprob logprobs_of(const std::vector<float>& logits, int vocab, int chosen, int top_n) { TokenLogprob out; out.id = chosen; if (logits.empty()) return out; const size_t n = token_bound(logits.size(), vocab); if (n == 0) return out; // log-sum-exp in double: exp() of a float logit and then log() of the sum loses the tail that // top_logprobs is read for. float max_l = logits[0]; for (size_t i = 0; i < n; i++) max_l = std::max(max_l, logits[i]); if (!std::isfinite(max_l)) return out; // a vocabulary of infinities reports nothing meaningful double sum = 0.0; for (size_t i = 0; i < n; i++) sum += std::exp(double(logits[i]) - double(max_l)); const double log_z = double(max_l) + std::log(sum); if (!std::isfinite(log_z)) return out; if (chosen >= 0 && size_t(chosen) < n) out.logprob = float(double(logits[size_t(chosen)]) - log_z); if (top_n <= 0) return out; Call Site Update The call site in forward_serve.cpp now correctly passes the tokenizer's size as the vocabulary bound to logprobs_of, ensuring consistency with how sample_token handles the bound. This aligns the production usage with the fix. held.emplace_back(Emitted{piece, npu::logprobs_of(logits, e.tok->size(), next, opts.sample.top_logprobs)}, piece.size());

[bong-water-water-bong]

VERDICT at 1adceaf: READY

Reviewed at 1adceaf5746604ac0429457df2e93259381f9590. It is on main (0 behind) and merges clean. The diff is 3 files, +40/-7.

Checked

• The bounded overload is consistent. logprobs_of(logits, vocab, chosen, top_n) applies token_bound() to every place a padded row could leak in: the max_l scan, the normaliser, the chosen range check and the top_logprobs candidates. vocab <= 0 falls back to the full width.
• The one call site is fixed. Main's only call (forward_serve.cpp:562) now passes e.tok->size(), the bound sample_token() already gets.
• Tests. npu_sampling_test reports ok in an ONEBIT_NPU host tree, and tools/copyright.py --check returns rc 0.
• Mutant. I normalised over logits.size() again, the bug itself. It fails "a padded row: the logprob is the vocabulary-only softmax, not the padding's". Restored, and the test passes again.

Note for whoever lands second of this PR and #2266/#2267 (not blocking here). The body says this fix "covers both paths". On main that is true: there is one call site. But #2267, still open on #2266's branch, adds a pointer overload, logprobs_of(const float* row, size_t width, int chosen, int top_n), and calls it with logits.size(), the head's full width. Its second argument is the width, where this PR's second argument is the vocabulary bound.

So after both land, the speculative path's logprobs still normalise over the padding unless the merge gives the pointer form the same bound (e.tok->size()). The two branches already conflict in app/npu_sampling.h and app/forward_serve.cpp (git merge-tree pr2273 pr2267), so this will surface as a hand resolution.

A safer shape is a single pointer overload taking (row, width, vocab, chosen, top_n). Then a caller cannot pass the width where the bound belongs.

My READY on #2267 at d56e13a said its speculative logprobs equal the plain loop's. They do, but both were normalised over the padding, which is this issue. I missed that both paths share the defect.

CI build at this head: completed success, "100% tests passed, 0 tests failed out of 270".

[bong-water-water-bong]

VERDICT: READY at 1adceaf

Built and ran it: npu_sampling: ok -- and the defensive shape of token_bound() is what I checked first, because a bound is where an off-by-one becomes a wrong answer rather than a crash:

inline size_t token_bound(size_t width, int vocab) {
    return (vocab > 0 && size_t(vocab) < width) ? size_t(vocab) : width;
}

It clamps in the safe direction only: a vocab larger than the row leaves the width alone (the caller's row is the authority), a vocab of 0 or negative leaves it alone, and only a genuinely narrower bound clamps. So a caller who passes the wrong (too large) vocabulary gets today's behaviour rather than an out-of-bounds read -- the failure mode this must not have.

The two arms are the defect and its visible consequence:

check(std::fabs(bounded.logprob - float(-log_z)) < 1e-6f, …)
check(!padding_reported, "a padded row: no id at or past the vocabulary appears in top_logprobs")

The second is the one worth having: the padding shifts every logprob, and it puts ids the tokenizer does not have into top_logprobs. Only the second is visible to a client; the first is invisible to any comparison between two paths sharing this helper -- which is exactly what the docstring says:

"those rows are not tokens. Normalising over them shifts every reported logprob by the padding's mass, which is model-dependent rather than a constant, and invisible to any comparison between two paths that share this helper"

"Model-dependent rather than a constant, and invisible to any comparison" is the clearest statement of why this class is dangerous that I have read tonight: a constant offset would cancel in a difference; padding mass does not, so the error survives every test that compares two paths.

And the concrete numbers are there: engine#2026's Qwen3 head, 151,936 rows against 151,669 ids -- 267 padding rows, from a defect I reviewed two days ago now showing up as a second symptom in a different layer. That is worth noticing: #2026 fixed the reader refusing a well-formed padded head, and this fixes what happens after it stops refusing.

One note: with n == 0 the function returns out (default). A vocab of 0 now reports a default logprob rather than a refusal -- worth one arm ("vocab 0 reports nothing rather than a logprob of 0"), since 0.0 is a plausible-looking value for a broken call.

Ran: npu_sampling_test. Not run: a live serve's logprobs response (no NPU ticket; the helper is host-side and that is where it is tested).