[bong-water-water-bong]

closes #937

Root cause of an unmeasurable row, not a bug in the forward. /v1/completions built its prompt from a string only, and app/forward_serve.cpp prepends BOS on the chat route alone (if (chat && e.add_bos && e.bos_id >= 0)), while llama.cpp's /completion prepends BOS by default. Two engines on the same weights were therefore fed two different prompts, and the comparison measured the prompt construction:

Zyphra/ZR1-1.5B, container weights both sides, 5 raw prompts, greedy 64	forward	llama.cpp
prompt tokens	10, 5, 7, 24, 13	11, 6, 8, 25, 14 (exactly one short)
identical continuations	0/5 — first divergence at token 0,0,3,0,1
reference's token at those points	—	near-certain: −0.0, −0.0017 logprob

The forward's counts equal llama.cpp /tokenize with add_special:false on every prompt, i.e. the difference is the BOS token and nothing else — never quantization noise.

Change. llama.cpp's /completion takes prompt as text or as an array of token ids; this adds the id form to /v1/completions (app/npu_prompt_ids.h). Supplied ids are used exactly, nothing is tokenized and no BOS is inserted on top: the caller is stating the prompt, not asking for it to be derived.

After — same model, same prompts, ids to both engines:

• prompt tokens 11, 6, 8, 25, 14 == 11, 6, 8, 25, 14
• byte-identical text 3/5; identical token sequences 2/5, over 64 generated tokens each
• the 2 real divergences are .\n\n (reference, margin 0.39 and 0.32 logprob) against .\n (forward, 1.13 / 1.30 under the reference's own distribution): whitespace variants at a near-tie, i.e. the residual is host-GEMM arithmetic, not the prompt

Refusals are 400s that name the position, never a clamped or dropped value — checked over HTTP on the running server:

{"prompt":[]}            400 bad request: prompt: an empty token-id array cannot be evaluated
{"prompt":[1,"2"]}       400 bad request: prompt[1] is not an integer token id
{"prompt":[999999999]}   400 bad request: prompt[0] = 999999999 is outside the vocabulary (0..151664)
{"prompt":{"a":1}}       400 bad request: prompt must be a string or an array of token ids
{"prompt":[151646,785]}  200 text_completion, prompt_tokens 2

An out-of-vocabulary id is refused because the embedding lookup indexes by id. OpenAI's array-of-strings batch form is refused by the same rule. String prompts and the chat route are untouched (prompt_is_ids is false for the chat route, so its BOS behaviour is unchanged).

Tests. tests/npu_prompt_ids_test.cpp pins the acceptance rules and every refusal, device-free and model-free; npu_prompt_ids: ok, and tools/copyright.py --check is clean. Measured with ~/lb/zr1-route-compare.py against 1bit forward-serve (host GEMM) and llama.cpp on the container's own weights (tools/q4nx_to_gguf.py write).

Not measured here: the device path (ZR1's four ELFs do not exist on this box) and the chat route for a non-ChatML checkpoint, which is the separate template issue.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 9f78e60)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 937 - Partially compliant Compliant requirements: The PR implements support for token IDs in the prompt field for /v1/completions The PR ensures that when token IDs are provided, no tokenization or BOS insertion occurs The PR includes proper validation of token IDs to prevent out-of-vocabulary errors The PR updates documentation to reflect the new capability Non-compliant requirements: The PR does not include a comprehensive test suite for all edge cases of token ID handling Requires further human verification: The actual performance and correctness of the implementation on the target hardware (Zyphra/ZR1-1.5B) needs to be verified The parity run results should be confirmed to match the claimed improvements

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Prompt ID Handling Logic The new logic for handling prompt IDs introduces a conditional path that may be error-prone. Specifically, when prompt_is_ids is true, the prompt variable is not used but the code still references it in the lambda capture, which could lead to confusion or potential issues if the variable is accessed elsewhere in the future. The code should ensure that prompt is only captured in the lambda when it's actually needed. auto run = [&e, &req, prompt, prompt_ids, prompt_is_ids, max_tokens, chat, &opts, gram, &stats]( const std::function<bool(const std::string&, const std::vector<Emitted>&)>& on_text) { std::string finish = "stop"; // The tokenizer is const and keeps no per-call state, so it runs before the lock. A caller // that sent ids already stated the positions: they are used exactly, with no BOS inserted // on top (inserting one there would re-introduce the one-token prompt difference an id // prompt exists to remove). std::vector<int> ids = prompt_is_ids ? prompt_ids : e.tok->encode(prompt); Validation Clarity The validation logic in prompt_ids_from_body throws exceptions with detailed messages, which is good for debugging. However, the error messages could be more specific about what constitutes a valid token ID range, especially for edge cases like the maximum vocabulary size. Consider adding a note about the expected range to improve clarity for users. if (id < 0 || (vocab > 0 && id >= vocab)) throw std::runtime_error("prompt[" + std::to_string(i) + "] = " + std::to_string(id) + " is outside the vocabulary (0.." + std::to_string(vocab - 1) + ")");

[bong-water-water-bong]

VERDICT: READY on content at 660cfce67. The CI build and two Analyze jobs are still running at this head, so
the tprs-ready label stays off until they are green; nothing in this review depends on them.

The parser, built and run here (header-only, no CMake, no model, no device — which is itself the claim):

$ g++ -std=c++17 -I app -I npu -I <nlohmann> tests/npu_prompt_ids_test.cpp -o /tmp/npu_prompt_ids_test
$ /tmp/npu_prompt_ids_test
npu_prompt_ids: ok                                     rc=0

The assertions are the interesting ones: a string returns false and leaves the vector untouched; an id array is
kept in order including BOS 151646 (the whole reason for the shape); one id is accepted; missing / empty /
[1,"2"] / [1,2.5] / [1,true] / [1,-1] / [1,151936] / {"prompt":{"a":1}} are each refused naming the
position; 151935 (last id) is accepted; and vocab <= 0 disables the range check.

The wiring, read against the rest of the handler. e.tok->size() is the right bound — int(id_to_text_.size()),
"one past the largest id, added tokens included" (npu/tokenizer.h:48) — so a special token like BOS is legal and an
id equal to the size is refused, which is what the 400s say. Three things I checked because they are where this
shape usually leaks:

• No BOS on the id path, and the chat route is untouched: if (!prompt_is_ids && chat && e.add_bos …)
  (forward_serve.cpp:218), prompt_is_ids is only ever set on the non-chat branch (:161-163).
• The context guard still applies to ids: the string heuristic is skipped, but room = max_context - ids.size()
  plus if (room < 1) throw "prompt longer than the context" (:223-224) runs before the lock, so a huge array is
  a 400, not a crash.
• The prefix cache is keyed on the ids, not on the now-empty prompt string (e.prefix->lookup(ids) at
  :236, insert(ids, …) at :283). Had it been keyed on the text, every id prompt would have collided on ""
  and a parity run would have been comparing a resumed state — that is the one place I expected to find a bug and
  it is correct.

Refusal messages come out as the body's examples (bad request: prompt[1] is not an integer token id,
prompt[0] = 999999999 is outside the vocabulary (0..151664)), since the parser throws inside the try that
prefixes bad request: .

Not verified here, and it is the PR's own evidence: the before/after parity table, the live 400/200 calls over
HTTP, and the "reference's token was near-certain" logprob column. Those need both servers on the container weights
(~/lb/zr1-route-compare.py, forward-serve host-GEMM + llama.cpp with tools/q4nx_to_gguf.py write), and starting
1bit forward-serve on this box can want the NPU device that the owner's Bonsai serve holds — I did not take that
lock for a review. The unit test plus the handler reading are what I can stand behind; the table is the lane's.

Nits, none a gate:

1. add_dependencies(npu_sampling_test npu_prompt_ids_test) copies the existing ci.yml workaround with its #635
   comment. Fine, but it makes the new test's build depend on the sampling test's target; if that ever changes,
   this one silently stops being built too.
2. prompt stays empty when the ids path is taken. Nothing reads it downstream today (stats.prompt_tokens comes
   from ids.size()), but any future log or trace that prints prompt would print nothing for an id request.
3. The docs example is "prompt": [151646, 785, 6722, 315, 9625, 374] with no note that 151646 is this model's
   BOS — a copy-paste against another container is an out-of-vocabulary 400 rather than a silent wrong prompt,
   which is the right failure, but a half-line would save the confusion.

[bong-water-water-bong]

CI is now green at 660cfce67 (build + Analyze ×5 + CodeQL + pr_agent SUCCESS, hw_gate SKIPPED, mergeStateStatus CLEAN), so the tprs-ready label is on. The VERDICT: READY above stands at this head.

[bong-water-water-bong]

VERDICT: READY at 9f78e6030. This head is comments and docs only (git diff 660cfce67 9f78e6030: +9/−5, two
files, zero statements changed), so the review above carries unchanged — the parser test still builds and runs
standalone (npu_prompt_ids: ok), and the wiring (!prompt_is_ids && chat && e.add_bos, ids path keyed into the
prefix cache, room < 1 for oversized id arrays) is untouched.

Both nits I raised are addressed exactly: the prompt variable now carries a comment at its declaration that it is
empty on the ids path and that nothing reads it (with the "do not log it as the request's text" warning — that was
the point, it is a trap for the next person, not a bug), and docs/npu.md now states that 151646 is
ZR1-1.5B's BOS and that pasting another container's is the out-of-vocabulary 400 rather than a silently
different prompt. Nit 1 (add_dependencies(npu_sampling_test npu_prompt_ids_test)) is still there, which is fine —
it is the same workaround the other device-free tests use.

CI at this head is re-running (mergeStateStatus BLOCKED, build and Analyze queued); the label stays on, and I
will confirm at this head when it settles.

[github-actions]

Persistent review updated to latest commit 9f78e60

[bong-water-water-bong]

CI is green at 9f78e6030 (build + Analyze ×5 + CodeQL + pr_agent SUCCESS, hw_gate SKIPPED), so the READY above stands at this head and the tprs-ready label is current.