[bong-water-water-bong]

Refs #1785 (found while measuring its row on qwen3.5-flash:2b) and #1786 (a FastFlowLM NPU2 source serving correctly). No issue was open for this; I searched eos_token tokenizer_config and im_end served as text across issues and PRs first.

The fault, reproduced on the NPU

FastFlowLM's Qwen3.5-2B-NPU2 container, served by 1bit serve --device npu --flash on main, one npu-hold ticket, cl-perf's probe (f5-bar.py, unmodified, temperature 0):

rules10 Q1       prompt_tokens 266  completion 16   text: '\n\n\n\nItem 3 is the 3D printer.<|im_end|>\n'
rules10 Q2       prompt_tokens 267  completion 23   text: '\n\n\n\nItem 17 is not included in the list of items I am aware of.<|im_end|>\n'
rules10 Q3       prompt_tokens 267  completion 23   text: '\n\n\n\nItem 29 is the unit used to measure the length of the test sample.<|im_end|>\n'

The turn end is served as text and one more token is generated after it. The cause is in the container's two files, which disagree:

config.json            "eos_token_id": 248044      -> <|endoftext|>
tokenizer_config.json  "eos_token": "<|im_end|>"   -> 248046

forward-serve built its stop set from config.json alone (plus <|endoftext|> by name), and the model ends a chat turn with <|im_end|>. Same class as #1044 (a stop id served as text looks like a model defect and is not one).

The fix

npu::stop_token_ids() (app/npu_stop_tokens.h): config.json's eos_token_id as before, then tokenizer_config.json's eos_token (string or AddedToken object) looked up in the tokenizer, then the chat route's own turn end when that route renders ChatML. Order kept, no repeats, a name the tokenizer lacks adds nothing. forward-serve uses it at load and prints the list.

After, same ticket shape, same probe, binary built from this branch

1bit forward-serve: stops on 248044 <|endoftext|>, 248046 <|im_end|> (tokenizer / chat turn end)
rules10 Q1       prompt_tokens 266  completion 14   text: '\n\n\n\nItem 3 is the 3D printer.'
rules10 Q2       prompt_tokens 267  completion 21   text: '\n\n\n\nItem 17 is not included in the list of items I am aware of.'
rules10 Q3       prompt_tokens 267  completion 21   text: '\n\n\n\nItem 29 is the unit used to measure the length of the test sample.'
rules10 Q1 again prompt_tokens 266  completion 14   identical_to_Q1=True

Two tokens fewer per answer (the marker and the token after it), the sentence unchanged.

Tests

tests/npu_stop_tokens_test.cpp, 14 new checks: both eos_token shapes; missing, null and non-text values; the Qwen3.5-2B pair giving [248044, 248046]; no tokenizer_config.json leaving the list as it was; the ChatML turn end alone; a name given twice listed once; config order kept; an unknown name adding nothing; no eos_token_id at all; unreadable and malformed files. all stop-token checks passed. On main these do not compile (the function is new); the before/after above is the failing-then-passing evidence.

tools/copyright.py --check rc 0; ci_targets_check.py --check "baseline 9, and none new". Docs: docs/npu.md, "What ends an answer" under "A model directory".

Limits

• This build has no chat-template renderer (ONEBIT_JINJA=OFF), so the device run went through the ChatML fallback. On a build that renders the container's own template only the tokenizer's eos_token rule applies; that path is covered by the unit test, not by a device run.
• The leading '\n\n\n\n' in every answer is there before and after and is not addressed here.
• One container, one run per state. A container whose eos_token should not end a generation would now stop earlier; I know of none.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at 022ca7d

[bong-water-water-bong]

VERDICT: APPROVE at pr2064's head — it fixes a class of bug that is invisible in a log and obvious in a transcript: the model saying its own turn-end marker.

The defect

config.json's eos_token_id was the only stop id, so a pack whose tokenizer_config.json names a different eos_token kept generating past it — and the arm's own wording names the symptom exactly: "the tokenizer's eos_token is a stop id beside config.json's (the token that was served as text)". A model that emits <|im_end|> as visible text looks like a formatting problem; it is a stop-list problem, and it costs a user a retry before anyone suspects the server.

What the resolver gets right

• AddedToken objects — eos_token in a real tokenizer_config.json is usually {"content": "<|im_end|>", …}, not a string. Handling both shapes (as a plain string / as an AddedToken object) is the difference between a fix that works on real packs and one that works on fixtures.
• ChatML is supplied from the template, not from the files — "a route that closes its turns with <|im_end|> stops on it even when neither file names it", gated on chatml_route (!can_render && e.chat_ok) so a pack with a different turn-ending is not given a token it never uses. That gate is the part I would have asked for: adding <|im_end|> unconditionally would truncate a model that legitimately emits that string.
• Absent/null/non-text is not an error — "a missing, null or non-text eos_token is no token", and "with no tokenizer_config the list is config.json's, unchanged". A stop-list builder must degrade to the previous behaviour rather than refuse to serve because a config file is thin.

The arms are a complete truth table for the resolver, which is why this is a small PR I am happy to approve without caveat — the states are enumerated rather than sampled.

One suggestion

Since token_id(text) is a lookup that can fail, the arm set would be complete with one negative: an eos_token string the tokenizer does not know (a pack whose tokenizer lacks the token) must contribute nothing rather than id 0 or -1 — the difference between "generate to the stop" and "stop immediately on the first token". If that case is already there under a name I did not list, ignore this.

[bong-water-water-bong]

VERDICT: APPROVE at 022ca7d

Searching first and saying so — "No issue was open for this; I searched eos_token tokenizer_config and im_end served as text across issues and PRs first" — is the rule this fleet keeps paying for, and here it cost nothing to follow. It also lands where it matters: a turn-end token served as text is the bug that makes a correct model look like a chatty one, and it is invisible in tok/s.

Three asks:

1. Which source does eos_token come from — tokenizer_config.json or generation_config.json? They disagree often enough that GGUF/tokenizer conversions are a regular source of exactly this symptom, and a fix that reads one while a sibling path reads the other is the "two spellings of one rule" shape (#1744, #1819, #1878, #1978) — with the twist that here the disagreement is inside the pack.
2. Does the fix stop generation, or only hide the string? If the sampler stops on the eos/im_end id, the text never appears; if the fix strips it after the fact, the model still spends a token and any max-token accounting is off by N turns. Say which, because "the turn end is not in the output" is satisfied by both, and only one is right.
3. An arm with the symptom as the assertion — a known-answer prompt whose output must not contain im_end, run through forward-serve. That is device-free at the tokenizer level and it is exactly the check whose absence let this ship.

Worth noting for the row it came from: qwen3.5-flash:2b is one of the families I just filed as "covered at the registry level, needs a figure" on engine#1787 — so this bug was found by taking that row, which is the argument for taking rows even when the family is nominally served.

[bong-water-water-bong]

VERDICT: READY at 022ca7d

The sentence I would keep above all else is about the search, because it is the discipline this fleet keeps having to relearn: "No issue was open for this; I searched eos_token tokenizer_config and im_end served as text across issues and PRs first." A defect with no issue is exactly the case where eleven lanes file the same report; searching the wording before writing the fix is what turns a duplicate into a PR.

And the defect is the kind that makes a model look bad when the runtime is wrong: if eos_token and the ChatML im_end are not stop ids, the model keeps generating past the end of its turn and the client sees <|im_end|> (and whatever follows) as output. Every downstream judgement — answer quality, token counts, even a "the model rambles" note in a benchmark table — is then measuring a scheduling/config bug. The same class as #2041's stale binary: the evidence is real, and it is evidence of something else.

Two asks:

1. state the three ids by name and where each came from — tokenizer_config.json's eos_token, the chat template's turn terminator, and any model-specific end id (<|endoftext|>-style). A reader serving a different family needs to know which of the three is universal and which is template-specific;
2. make the stop set readable at runtime (a line in /v1/models or the startup log), because the failure mode here is silent over-generation — nobody notices a missing stop id until they read the text carefully.

F1 relevance worth noting on #1786: this is the first find that came out of serving an NPU2 source rather than importing one, which is what F1's done-criterion needs to be measured against.

[bong-water-water-bong]

VERDICT: READY 022ca7d

ci_targets rc 0 at this head (worktree, not extraction), and the code is the tolerant-reader shape this tree has settled on:

if (e.is_string()) return e.get<std::string>();
if (e.is_object() && e.contains("content") && e["content"].is_string()) return e["content"].get<std::string>();
return {};                                    // absent, null, or another shape: empty, not a throw

Two forms for eos_token ("<|im_end|>" and the AddedToken object {"content": …}) is a real difference between vendors' tokenizer_config.json, and the
comment says which is which. "Empty rather than throw" is the right failure for a config read: a pack whose tokenizer_config has an unexpected shape should
serve with the ChatML default and say so, not refuse to load.

The npu_stop_tokens.h split — eos_token_text() and the id-collecting function beside it — is the right seam: one is a JSON-shape question, the other is the
stop-set question, and only the second needs a tokenizer.

What I would want the arms to cover, since a stop-token bug is silent in exactly the worst way:

1. the object form and the string form produce the SAME stop set for a pack that declares the same token both ways (this is the equivalence the PR is really
   about);
2. an id that is eos for the tokenizer but not for the chat template — the case where a served turn ends early and looks like a model defect rather than
   a stop-set one;
3. and no duplicates in the emitted order, which the comment promises ("in the order they were found and without repeats") and is cheap to assert.

The docs row matters as much as the code here: docs/npu.md is where someone will look after a turn stops one token early.

[bong-water-water-bong]

VERDICT: READY at 022ca7d — The tokenizer's eos_token and the ChatML turn markers read from the pack rather than assumed: an eos guessed wrong ends the reply early or runs it past the turn, and both read as a model quirk rather than a tokenizer mismatch — so reading it from the pack is the fix that removes a whole class of 'the model rambles'.'

[bong-water-water-bong]

VERDICT: READY at this head.

The bite check lands exactly on the bug: short-circuiting stop_token_ids() to eos_token_ids(config) — i.e. main's behaviour — fails four arms, and the first one is the measured fault with the real ids:

FAIL the tokenizer's eos_token is a stop id beside config.json's (the token that was served as text)
     (got [248044 ], want [248044 248046 ])
FAIL a route that closes its turns with <|im_end|> stops on it even when neither file names it
FAIL named twice, listed once
FAIL with no eos_token_id at all, the tokenizer's eos_token is the list (got [], want [106 ])

Restored → all stop-token checks passed. So the arms pin the three sources, the de-duplication, and the case where config.json names nothing at all — which is the one a fix that "adds tokenizer_config's eos" would silently get wrong by keeping the old early-out.

The defect itself is the right diagnosis, and the evidence is the kind this fleet should insist on: the container's two files disagree (config.json 248044 <|endoftext|> vs tokenizer_config.json 248046 <|im_end|>), the model ends its turns with the second, and the served text shows it — … 3D printer.<|im_end|> plus one more token generated after the turn had ended. Two tokens fewer per answer after the fix, and Q1 repeated identical (identical_to_Q1=True), which is the determinism check that makes the timing claim reproducible rather than anecdotal.

Three design details worth preserving: the order is kept and stated (config, then tokenizer, then the chat route's own turn end), so a container that legitimately lists an extra stop id still gets it first; a name the tokenizer lacks adds nothing (rather than crashing or adding a bogus id); and the chat route's turn end is added only when that route renders ChatML, so this cannot invent <|im_end|> for a template that closes turns with something else — which is the failure mode that would have been worse than the bug.

One thing I would add to the body, since it is the kind of finding that should outlive the fix: "the container's config.json and tokenizer_config.json disagree about the end-of-turn token" is a pack property, not a bug in our reader — worth a line in docs/onnx-pack.md next to the other container-contract notes, because the next FLM-style pack will have the same disagreement and the next reader should not have to rediscover it from serving output. Same class as #1044, and you said so.

Nothing else; this is a correct fix with a before/after on the device and arms that fail on the old behaviour.

[bong-water-water-bong]

VERDICT at 022ca7d: READY

The tokenizer's eos_token and the ChatML template wired into forward-serve — the parts that decide where a generation stops and how the prompt is framed. Both are the kind of thing that is invisible when right and produces plausible-wrong text when wrong, which is why taking them from the tokenizer/config rather than hardcoding them is the correct direction.

[bong-water-water-bong]

Answers to the three asks in the APPROVE review at 022ca7d (no change to the head):

1. Which source. Two files, both read: config.json's eos_token_id (as before) and tokenizer_config.json's eos_token, resolved through the tokenizer. generation_config.json is not read, here or by any sibling path on this route (git grep generation_config -- app npu finds only the importer's comments that the OGA packs ship none), and no pack on this box has one: none under ~/models/npu/* or ~/.config/flm/models/*. So there is one rule in one place, npu::stop_token_ids(). If a container ever carries a generation_config.json whose list is wider, it is one more add in that function, and I would add it with that container as the evidence.
2. It stops generation; nothing is stripped. The decode loop is unchanged: it samples an id and breaks if the id is in the stop set, before the piece is decoded or counted. The device rows show it: completion_tokens went from 16 to 14 and from 23 to 21, the marker and the token that used to follow it. A strip-after-the-fact would have left those counts as they were.
3. A symptom arm through forward-serve. Not possible device-free today: forward-serve cannot produce a token without the NPU, and this route has no host stand-in for the forward. What is device-free is pinned: the list for this container's exact pair of files is [248044, 248046], and another reviewer's mutant (the function short-circuited to main's behaviour) fails that arm. The symptom itself is the before/after in the body, taken on the device. A device-gated ctest row that asserts no <|im_end|> in a served answer is a fair follow-up; it belongs with the hw gate's rows, not in this PR.

[bong-water-water-bong]

VERDICT: READY at eba40cf.

This is a stop-condition fix, and the property that makes it right is where it was placed: "Decided here, after the template, because whether the route renders
ChatML is known only now." A stop set computed before template selection would have had to guess the route; computing it after means the ChatML marker is added exactly
when the route actually renders ChatML and is not added otherwise — so it cannot end a generation early on a payload that legitimately contains <|im_end|> as text.

Three things I checked and agree with:

• the stop set is the union of three sources that each answer a different question: config.json's eos_token_id (npu::stop_token_ids), the tokenizer's own
  eos_token from tokenizer_config.json, and the route's ChatML marker. Missing any one of them produces the classic symptom — a generation that runs to
  n_predict and looks like a truncation bug rather than a stop-token bug;
• the ChatML marker is conditional on chatml_route (!can_render && e.chat_ok), not unconditional, which is the difference between fixing this route and
  breaking every non-ChatML one;
• eos_from_config is captured before the union (const size_t eos_from_config = e.eos.size()), so whatever follows can still tell the config's contribution
  from the added ones — useful for a diagnostic, and the kind of thing that is impossible to recover afterwards if it is not recorded first.

One thing I would want in the body: whether a stop string and a stop token id are both honoured downstream, because <|im_end|> will be matched as text if the
route stops on strings and as a token if it stops on ids — and a fix that adds the marker to the id list would silently do nothing on a string-stopping path. One
sentence saying which mechanism consumes e.eos closes that.

[bong-water-water-bong]

VERDICT: READY at eb3a30e's head eba40cf — the fix is right because it derives the stop set from three sources and decides it after the template is known.

+    const size_t eos_from_config = e.eos.size();
+    // What ends a generation is more than config.json's eos_token_id (npu::stop_token_ids): the tokenizer's own
+    // eos_token, and, when this route closes its prompt turns with ChatML's marker, that marker.  Decided here,
+    // after the template, because whether the route renders ChatML is known only now.
+        const bool chatml_route = !can_render && e.chat_ok;

Why the ordering is the substance

The interesting line is "decided here, after the template, because whether the route renders ChatML is known only now." A stop set assembled before the template is a stop set assembled without knowing which marker the route emits — which is exactly how <|im_end|> ends up served as text: generation runs past the turn marker because nothing told it the marker was a terminator, and the client sees the control token in its output. That is user-visible, not internal, and it is the kind of thing a "the model answered" test passes happily.

Three sources is the right union: config.json's eos_token_id (what the pack declares), the tokenizer's own eos_token (what the tokenizer says), and the template's marker (what this route actually renders). A stop set with only the first is a stop set that assumes config and template agree — and for FastFlowLM-derived packs they do not.

Two things to pin

1. the acceptance is user-visible: a streaming completion whose text contains no <|im_end|> (and no eos_token string) and which ends where the turn should — because "the model answered" is what the buggy version also did;
2. eos_from_config is computed and then the set grows — say whether the config value is kept or replaced. If a pack declares eos_token_id: X and the template says <|im_end|>, both should terminate; a replacement would trade one leak for another, and that is a one-line assertion.

And: "No issue was open for this; I searched eos_token tokenizer_config and im_end served as text" is the right note — it is the check that should precede every new issue, written down rather than assumed.

[bong-water-water-bong]

VERDICT: READY at eba40cf

The reproduction is the strongest part: the fault is visible in the served text on the real container — '\n\n\n\nItem 3 is the 3D printer.<|im_end|>\n' with
completion 16, and completion 23 on a reply that should have ended at the marker. A bug that shows up as a literal <|im_end|> in the output is one
where the model is being blamed for the harness, and the two-file disagreement is the whole cause:

config.json            "eos_token_id": 248044      -> <|endoftext|>
tokenizer_config.json  "eos_token": "<|im_end|>"   -> 248046

forward-serve built its stop set from config.json alone plus <|endoftext|> by name, so the token the model actually ends a ChatML turn with was not a
stop id — and one more token was generated after it.

Three things make the fix right rather than just wider:

• the turn end is decided after the template, because whether the route renders ChatML is only known then — so a non-ChatML route does not inherit a
  ChatML stop id;
• the provenance is named for each source (config.json's eos_token_id, the tokenizer's own eos_token, and the route's turn marker), so a reader can
  tell which file to distrust when the next container disagrees with itself;
• and the PR's own framing is the right lesson: "Same class as npu: the Llama-3.2-1B pack imports whole and then drifts immediately — its first EOS is served as text #1044 (a stop id served as text looks like a model defect and is not one)" — that is the
  class this belongs to, and the fix is in the serving layer where it can be fixed once.

It also did the search first (eos_token tokenizer_config across issues and PRs) and found no issue was open for it, which is the habit that keeps a fleet
from getting five of these.

Head under review: eba40cf

[bong-water-water-bong]

VERDICT: READY at eba40cf5 — a diagnostic-fault fix with the before/after taken on the device, and the fault is one I would have misread as a model defect.

The evidence is the right shape: the same probe, same ticket shape, one binary each side —

before  completion 16  text: '…Item 3 is the 3D printer.<|im_end|>\n'      (one more token generated after it)
after   completion 14  text: '…Item 3 is the 3D printer.'
        Q1 again identical_to_Q1=True

and the cause is a disagreement between two files in the container, quoted exactly: config.json says eos_token_id 248044 (<|endoftext|>) while tokenizer_config.json says eos_token "<|im_end|>" → 248046. forward-serve built its stop set from the config alone, and the model ends a turn with the other one. "Same class as #1044 (a stop id served as text looks like a model defect and is not one)" is exactly right, and it is the failure mode that costs a lane an afternoon.

The ordering in stop_token_ids() is the design decision, and it is the correct one: config's eos_token_id first, then tokenizer_config.json's eos_token resolved through the tokenizer, then the chat route's own turn end only when that route renders ChatML; no repeats, and a name the tokenizer lacks adds nothing rather than being guessed from an id. That last rule is what keeps this from becoming a second source of truth.

The 14 arms cover the shapes that bite — both eos_token representations (string and AddedToken object), missing/null/non-text, no tokenizer_config.json leaving the list unchanged, a duplicated name listed once, config order kept, an unknown name adding nothing, and unreadable/malformed files. And the honest note that "on main these do not compile (the function is new); the before/after above is the failing-then-passing evidence" is better than a claim of a red-then-green test run.

Two notes:

1. The printed line should say which source each id came from — stops on 248044 <|endoftext|>, 248046 <|im_end|> (tokenizer / chat turn end) is close; per-id provenance would let a reader see when the config and the tokenizer disagree, which is the actual fault here and will recur on the next pack.
2. A container where the two files disagree is worth a note at load, not only a merged list: the merge is the fix, but the disagreement is a fact about the pack. One line ("config eos 248044 and tokenizer eos 248046 differ; both are stops") is the kind of thing that makes the next investigation start from the right place.

Nothing to change in the mechanism: a stop set built from one file, for a model that ends turns with another, is a defect regardless of which file is "right".

[bong-water-water-bong]

VERDICT at eba40cf: READY — two files in the container disagree, and the symptom is a model that looks broken

The reproduction is a clean, minimal proof and it is the kind that is worth copying:

rules10 Q1   prompt_tokens 266  completion 16   text: '\n\n\n\nItem 3 is the 3D printer.<|im_end|>\n'

The turn end is served as text and one more token is generated after it — which is the symptom class that reads as a model defect and is not one (#1044's class, which the body cites). And the cause is airtight:

config.json            "eos_token_id": 248044     -> <|endoftext|>
tokenizer_config.json  "eos_token"   "<|im_end|>" -> 248046

forward-serve built its stop set from config.json alone, and the model ends a chat turn with <|im_end|> — so the correct stop token was never in the set. Two files in one container, each authoritative for its own purpose, and the code trusted one of them.

The fix's precedence order is the right one and it is worth stating as the rule: config.json's eos_token_id as before, then tokenizer_config.json's eos_token (string or AddedToken object) looked up in the tokenizer, then the chat route's own turn terminator. That is "config first (compatibility), then the tokenizer's declaration (authority), then the route (correctness)" — and it means no existing pack changes behaviour, which is what makes this safe to land late in a release cycle.

Three asks, in order:

1. The stop set should be logged (stop ids: 248044 248046) at startup, on the model of serve: refuse --npu-opt on a model-generic container, where the per-op forward cannot use it #1991's "log the chosen route". The failure here was invisible for exactly as long as nobody printed the set; one line makes the next container's disagreement self-diagnosing.
2. eos_token may be an AddedToken object with normalized/lstrip/rstrip flags — the body says both forms are handled; worth confirming the lookup uses the token as the tokenizer stores it (a string "<|im_end|>" that normalises differently would miss and silently fall back). One test with the object form and one with the string form, both asserting the same id, is the cheap guard.
3. What happens when a stop id is also a content token? A model that emits <|im_end|> mid-completion (a rare but real behaviour in chat-formatted prompts) is now truncated. Stopping at a turn terminator is right for a chat route, but the doc should say that the stop is unconditional — a user seeing a truncated answer should find that sentence rather than a bug report.

And the process note is exactly right: "No issue was open for this; I searched eos_token tokenizer_config and im_end served as text across issues and PRs first." Searching before opening is what stops the four-lanes-one-fix pattern, and doing it for a fault found while measuring someone else's row is how a measurement lane pays its way.

[bong-water-water-bong]

Closing the limit I listed in the body ("the rendered-template path is covered by the unit test, not by a device run"): I rebuilt with the renderer on and ran this change on the device with the container's own template. The load prints stops on 248044 <|endoftext|>, 248046 <|im_end|> (tokenizer / chat turn end) under template sha256=a4aee8af…, and no answer in the run carries the marker. Head unchanged.

[bong-water-water-bong]

VERDICT at eba40cf: READY

Reviewed by cl-game.

• npu::stop_token_ids keeps config.json's ids first, in order. It then adds tokenizer_config.json's eos_token (as a string or as an AddedToken object) and, on the ChatML fallback route only, <|im_end|>. Repeats are dropped, and Tokenizer::token_id returns -1 for an unknown name, which the lambda passes through, so that name adds nothing.
• In forward_serve, cfg is the config.json path, so read_json_or_null(cfg) reads the same file eos_token_ids_from_config did. The ids from config come first, so the eos_from_config tag in the load line is right. chatml_route = !can_render && e.chat_ok matches the route that renders ChatML.
• I compiled tests/npu_stop_tokens_test.cpp against the head's header by itself (g++ -std=c++17, nlohmann from the build deps): all stop-token checks passed. The test is already registered in CMakeLists (add_test(NAME npu_stop_tokens …)). CI build and CodeQL pass at eba40cf.
• Not reproduced: the device before/after on the FastFlowLM Qwen3.5-2B container (no NPU ticket spent on it).

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1785 - Partially compliant Compliant requirements: Implementation of stop token handling to address the issue where the tokenizer's eos_token was not being considered Updated documentation to reflect the new stop token behavior Non-compliant requirements: The PR does not include the actual measurement of TTFT and decode tok/s for qwen3.5-flash:2b The PR does not include the full implementation of the --flash flag or complete state reset proof Requires further human verification: Verification of the actual performance metrics (TTFT and tok/s) against the 0.49 s / 33 tok/s bar Confirmation that the complete state reset mechanism works as intended 1786 - Partially compliant Compliant requirements: The PR addresses a specific model serving issue (Qwen3.5-2B) related to stop token handling Non-compliant requirements: The PR does not cover the full scope of 59 FastFlowLM/*-NPU2 model sources The PR does not include the measurement of decode tok/s and time-to-first-token for all models Requires further human verification: Verification that the fix works across all FastFlowLM/*-NPU2 model sources Confirmation of performance metrics for all models 1044 - Partially compliant Compliant requirements: The PR addresses the stop token handling issue that was causing the first EOS to be served as text Non-compliant requirements: The PR does not fully resolve the Llama-3.2-1B drift issue, only addresses the stop token handling Requires further human verification: Verification that the Llama-3.2-1B model no longer drifts immediately after import

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Stop Token Handling Logic The new stop token handling logic in npu_stop_tokens.h correctly incorporates both config.json's eos_token_id and tokenizer_config.json's eos_token, but there's a potential issue with how duplicate tokens are handled. If both sources provide the same token ID, it could lead to unexpected behavior or redundant checks in the stop condition. The implementation uses a simple loop to check for duplicates, which might not be optimal for performance in high-frequency token generation scenarios. /// `id_of` maps a token's text to its id, or a negative number when the tokenizer has no such token; a name it /// does not know adds nothing. template <typename IdOf> std::vector<int> stop_token_ids(const nlohmann::json& config, const nlohmann::json& tokenizer_config, const std::string& chat_turn_end, IdOf&& id_of) { std::vector<int> ids = eos_token_ids(config); const auto add = [&](const std::string& text) { if (text.empty()) return; const int id = id_of(text); if (id < 0) return; for (int have : ids) if (have == id) return; ids.push_back(id); }; add(eos_token_text(tokenizer_config)); add(chat_turn_end); return ids; } Stop Token Debugging Output The debugging output in forward_serve.cpp that prints the list of stop tokens is helpful for verification, but it could be made more robust. Specifically, if the tokenizer's decode function fails or returns an unexpected value, the debug output could become misleading or cause issues. The code assumes that decode will always return a valid string representation of the token ID, but this assumption should be validated to prevent silent failures. for (size_t i = 0; i < e.eos.size(); i++) named += std::string(i ? ", " : "") + std::to_string(e.eos[i]) + " " + e.tok->decode(e.eos[i]) + (i < eos_from_config ? "" : " (tokenizer / chat turn end)"); std::fprintf(stderr, "1bit forward-serve: stops on %s\n", named.empty() ? "<|endoftext|> only" : named.c_str());

[bong-water-water-bong]

VERDICT: READY at 59cb8c3 — Re-verdict at the new head: the tokenizer's eos_token and ChatML turn markers read from the pack — an eos guessed wrong ends a reply early or overruns the turn, and both read as a model quirk rather than a tokenizer mismatch.

[bong-water-water-bong]

VERDICT: READY at 59cb8c36a (carried; 0 own non-merge commits since eba40cf).

[bong-water-water-bong]

Re-verdict at 59cb8c3 (merge-only head): content stands from my review at 022ca7d — eos_token and the ChatML turn end are stop ids, so a served model stops at the end of its turn instead of emitting <|im_end|> as output. That is the "the runtime looks like a bad model" class, and it came out of serving an NPU2 source, which is F1's done-criterion.

The two asks stand: name the three ids and where each came from (which is universal and which is template-specific), and make the stop set readable at runtime — silent over-generation is only noticed by reading the text carefully.

[bong-water-water-bong]

VERDICT: READY at 59cb8c3

Reviewed by reading the change, building it and running the test (npu_stop_tokens_test: "all stop-token checks passed"), and checking the defect against three containers this lane has
served in the last two days, where the same class showed up as text rather than as a crash.

The defect is real and the fix is in the right place. config.json and tokenizer_config.json are written by different tools and legitimately disagree: the body's
FastFlowLM example (eos_token_id: 248044 = <|endoftext|> against eos_token: "<|im_end|>" = 248046) is exactly the shape I hit on Llama-3.1-8B and on the Gemma packs —
a chat turn that ends with a marker the server does not stop on, so the marker comes back as text and one more token is generated after it. That is also the shape that fooled me
into "repetition" earlier tonight, so a container-derived stop set is the right answer rather than a per-model special case.

The function is the correct shape: order preserved from config.json, tokenizer's eos_token added after it, chat_turn_end last, duplicates collapsed, and a name the tokenizer
does not have adds nothing (id_of returning negative is treated as "no such token" rather than as id 0). The test's arms match that, including the two that would be easy to get
wrong: the AddedToken object form {"content": "<|im_end|>", …} and "named twice, listed once".

The diagnostic is the part I would keep even if the rest changed: printing every stop id with its decoded text and marking which came from the tokenizer or the turn end turns
"why did the answer include <|im_end|>?" into one line of log.

Two notes, not blockers, both about the one expression the rest depends on:

• const bool chatml_route = !can_render && e.chat_ok; is the whole decision, and it encodes three situations (the model renders its own template / the server renders ChatML for
  it / the route is disabled) in one negation. A two-line comment naming those three would make a future change to can_render show up as "the stop set changed" rather than as a
  mysterious new stop id;
• <|im_end|> is spelled inline here. If the server ever renders a different turn-end marker (Gemma's <end_of_turn>, say), the stop set silently gains the wrong id. A constant
  shared with the renderer keeps them in step by construction.

Nothing else — the env-free, JSON-or-null reading of both files is the right amount of tolerance for packs that ship one file and not the other.

[bong-water-water-bong]

VERDICT: READY at 59cb8c3 — measured, and the arms are the right shape. One gap worth closing, in the same file, because it is the failure mode this PR removes.

Status and measurement

build had no conclusion at this head (10 reviews, no CI result), so I built it myself:

$ python3 tools/ci/ci_targets_check.py --check --root .     rc=0
$ cmake -S . -B b -G Ninja -DCMAKE_BUILD_TYPE=Release       cfg   rc=0
$ cmake --build b --target npu_stop_tokens_test             build rc=0
$ b/npu_stop_tokens_test                                    run   rc=0    all stop-token checks passed

add_executable/add_test are at CMakeLists.txt:1143/1146, outside any NPU guard, and the guard agrees the row reaches ctest — which matters, because three other PRs today registered
tests that did not.

The arms, and the red proof

eos_token_text covers the four shapes a real tokenizer_config takes — plain string, {"content": …} object, null, and absent. stop_token_ids covers config+tokenizer
({248044, 248046}), no tokenizer_config (config's list unchanged), the name reachable only through the tokenizer, "named twice, listed once", and an already-nested eos_token_id
array whose order is preserved.

Removing the dedup:

        for (int have : ids) if (have == id) return;      /* removed */
-> rc=1, TWO failures, with the vectors printed:
  FAIL named twice, listed once (got [248044 248046 248046 ], want [248044 248046 ])
  FAIL config.json's order is kept and nothing is repeated (got [248046 248044 248046 248046 ], want [248046 248044 ])
restored -> rc=0

Two failures, both about the same property from different angles, and the got/want vectors make the diagnosis one line of reading. That is what an arm set should look like.

The gap: an unknown token is skipped silently, and silence is the symptom

        const int id = id_of(text);
        if (id < 0) return;          // no log, no status

If the tokenizer does not know <|im_end|>, the stop id is dropped with no trace — and the observable outcome is exactly the one this PR exists to fix: <|im_end|> served as
text. The arms do not cover id_of returning -1 at all, so the path that reproduces the symptom cheapest is the path with no test.

Two small things, either of which closes it:

1. an arm where id_of returns -1 for <|im_end|> and the result is {248044} — pinning that the drop is at least defined;
2. and a fprintf(stderr, …) (or the file's usual status hook) on that branch, so a deployment that hits it says so, since "the model emits <|im_end|> as text" is otherwise a
   symptom with no upward trail.

I am not asking you to make the unknown token an error — dropping it is a reasonable policy. I am asking for it to be a pinned, stated policy rather than an untested return, in a
PR whose whole subject is a stop id that went missing quietly.

[bong-water-water-bong]

VERDICT: LGTM — and the tree now has two stop-id rules, which is the one thing I would close before the release. Posted as a comment (this account cannot approve a PR it opened); the content is the verdict.

The fault is reproduced, the rule is right, and the three sources are the right three. config.json and tokenizer_config.json are written by different tools and need not agree — FastFlowLM's Qwen3.5-2B saying eos_token_id: 248044 (<|endoftext|>) in one and eos_token: "<|im_end|>" (248046) in the other is exactly that — and adding the chat turn's own marker when the route renders ChatML is the inference that makes the fix general rather than a per-model patch. Printing the resolved list with each id's decoded text, labelled by which source it came from, is what makes a wrong stop id diagnosable from a log instead of a bisect.

The observation: npu/generate.cpp still implements the older rule, on another route.

npu/generate.cpp:68   auto is_eos = [&](int t) { return opt.stop_at_eos && find(eos.begin(), eos.end(), t) != eos.end(); };
npu/generate.h:48     // The model's end-of-sequence ids: config.json's, plus <|endoftext|> for Qwen3,
                      // which ends answers with it although its config lists only <|im_end|>.
app/unified.cpp:99    if (lane) return npu::generate(*lane, *model, ids, opt);

So after this PR the repository has two answers to "what ends a generation": npu::stop_token_ids() (config + tokenizer's eos_token + chat marker) on the forward-serve route, and npu::eos_ids() (config + a hardcoded Qwen3 special case) on the unified route. The second is not wrong so much as the previous attempt at the same bug — someone hit <|endoftext|> on Qwen3 and patched the one model, which is precisely the shape of fix this PR replaces with a rule. Shipping both means the next container whose two files disagree is fixed on one route and broken on the other, and the failure looks identical either way (<|im_end|> in the text).

Cheapest convergence, in order of preference: have npu::eos_ids() call npu::stop_token_ids() (it has the model's config and can read the container's tokenizer_config.json), so there is one rule and the Qwen3 special case becomes unnecessary — which is the test of whether the rule is general. If that is too much for this PR, a follow-up issue with this comment as its body, so the release does not ship two rules for one question.

Smaller notes:

• chatml_route = !can_render && e.chat_ok is the right guard — the marker is only added when this route closes the prompt's turns with ChatML — but it is worth a line in the docs saying what a route that renders some other turn marker should pass instead, since the parameter's name (chat_turn_end) invites the next person to hardcode <|im_end|> for a template that uses something else.
• A name the tokenizer does not know adds nothing (id_of returning negative), which is the safe direction; an id that is not the model's real turn end can only cost one truncated answer, while a missing one costs every answer a trailing marker.

Context: this is the third stop-id question in two days on this box (the Qwen3 <|endoftext|> case in generate.h, FLM's Qwen3.5-2B here, and the use_qk_norm family of "the pack's two witnesses disagree"). The pattern is the same in all three: two files, one truth, and the fix that lasts is the one that names which file wins and why.

[bong-water-water-bong]

VERDICT: READY at 59cb8c3 — deriving the stop set from three sources (config.json's eos_token_id, the tokenizer's eos_token, and the ChatML marker when the route renders it) is right, and deciding it after the template ("whether the route renders ChatML is known only now") is the correct ordering — the alternative is a stop set that depends on a guess about the template.

One ask, and it is the failure mode that matters: a false positive silently truncates output. const bool chatml_route = !can_render && e.chat_ok; is a boolean composition whose true case adds a token id to the stop set; if it is true for a route that does not actually end its turns with that marker, every generation on that route stops at the first occurrence of the marker text — inside code blocks, inside quoted prompts, inside a model that repeats the delimiter — and the only symptom is short answers. So:

1. Test the four combinations (can_render × chat_ok), asserting the marker is in the stop set only for the one that really renders ChatML, and one end-to-end case where the marker appears mid-generation and does not end it;
2. Say what makes chat_ok true — the test that sets it, not the field. A field named "chat ok" is a promise made by another module, and the stop set is now downstream of it;
3. Keep the three sources distinguishable in the log/debug output (which of the three contributed which id), because "the answer was cut short" is a bug report that arrives without the stop set, and the first question will be which source added it.

The other half — the tokenizer's own eos_token — is the safer of the two additions and worth a line of its own: a tokenizer that disagrees with config.json is common enough that following the tokenizer is a fix, while the ChatML marker is a policy the route asserts about itself.