[bong-water-water-bong]

Issue #625: 1bit forward-serve (the route 1bit serve -m <NPU model dir> --device npu runs
in-process) decodes with a plain argmax and constrains nothing. It parsed
max_completion_tokens/max_tokens and ignored everything else the OpenAI API puts next to a
completion: temperature, top_p, top_k, seed, stop and response_format were accepted and
silently dropped, and grep -ril 'gbnf|grammar' app npu tools found nothing.

This adds the controls, and constrained output, by mirroring what this tree's llama.cpp already
does rather than inventing a sampler.

What

request field	behaviour
temperature	0 or absent = argmax, as before. Above 0 divides the logits before the draw
top_k	keep the k highest logits (0/absent = all)
top_p	nucleus sampling (1/absent = all), min_keep candidates always stay
seed	same seed + same request = same text; absent or -1 seeds from the clock
stop	up to 4 strings, matched across token boundaries; finish_reason becomes stop
grammar	a GBNF grammar (llama.cpp's format, third_party/llama.cpp/grammars/*.gbnf) the answer cannot leave
response_format	{"type":"json_object"} = the answer must be JSON. {"type":"json_schema"} is a 400

• app/npu_sampling.h — top_k, then top_p, then temperature, then the cumulative-sum draw that
  third_party/llama.cpp/common/sampling.cpp's chain ends with (src/llama-sampler.cpp's
  llama_sampler_top_k_apply / top_p_apply / temp_apply / dist_apply semantics, including
  k <= 0 and p >= 1 turning a stage off, and the max-logit-subtracted softmax). The chain order is
  the one third_party/llama.cpp/common/common.h:260-270 defaults to — top_k, top_p, temperature —
  and temperature is a division of logits (llama-sampler.cpp:1822, logit /= temp), which is why
  top_p's distribution is a scratch vector and not a write-back over the candidates. llama.cpp's own
  sampler needs a llama_context and a llama_vocab; this route has raw logits and
  npu::Tokenizer, and llama.cpp is only built under ONEBIT_HRX, so the semantics are repeated
  here instead of linked. Plus stop: find_stop with stop_holdback, so a stop sequence split
  across two tokens still matches and no held-back text is ever dropped.
• app/npu_grammar.h — GBNF: llama.cpp's rule model (one flat element vector per rule, ALT
  and END between alternatives, the S{m,n}/S* rewrite rules, char classes, escapes, ( )
  groups, # comments) compiled to a byte-level pushdown automaton (src/llama-grammar.cpp's
  stack discipline: a frame points at a E_CHAR or a rule reference, never at ALT/END; an
  empty alternative leaves the caller's frame; an empty stack is a complete match).
  response_format {"type":"json_object"} compiles third_party/llama.cpp/grammars/json.gbnf
  verbatim. json_schema is a 400 — the schema-to-GBNF converter (common/json-schema-to-grammar.cpp)
  lives in llama.cpp's common/, which this route does not link, and hand-rolling one is the
  larger change the issue says to leave for later.
• app/forward_serve.cpp — the decode loop samples instead of argmax'ing, tests each vocabulary
  token against the grammar state (Grammar::next_bytes rejects almost every token on its first
  byte, so only the survivors pay a full feed()), holds back the stop-sequence tail, and returns
  finish_reason length when the token budget ran out vs stop for an end-of-sequence token, a
  stop sequence or a grammar that allows nothing more. It was hard-coded to "stop" before.
• docs/npu.md — "## Serving" → "### Sampling and structured output": the fields, the default,
  the finish reasons, and where the semantics come from.

Greedy is the default and the parser stays backward compatible: a request that names none of
these fields takes the same argmax path as before (argmax with the same lowest-index tie-break),
and a field the route cannot honour is a 400 whose error.message says why
(temperature: must be zero or more (zero decodes greedily), gbnf: unterminated literal at line 1, response_format: json_schema needs a schema-to-GBNF converter this route does not have; ...)
rather than another silent ignore.

One change to the device path, for the verification below

run_forward_serve now opens xrt::device only when it is going to use it: with
NPU_INFER_HOST_GEMM=1 it calls Q4nxNpuForward::init_host instead
(npu/forward/include/q4nx_forward.h:64-70, the offline mode that "validates the whole forward
against the Python reference offline" and never touches the device). This box has no
/dev/accel*, and that mode is what lets the three runs below exercise the real model, the real
tokenizer and the real decode loop here. With the variable unset the code path is unchanged.
Say the word and I will drop that hunk and re-verify on the box that has the NPU.

How tested

Device-free, and it runs in CI now: tests/npu_sampling_test.cpp, registered as ctest
npu_sampling next to http_guard/think_split, no model and no NPU, and its target is listed in
.github/workflows/ci.yml's explicit build list — the list was missing it, so CI built everything
else and reported 22 - npu_sampling (Not Run), i.e. 98% tests passed, 1 tests failed out of 43
without ever executing the test. That line is fixed in this commit.

$ cmake -S . -B build-npu -DONEBIT_NPU=ON -DONEBIT_NPU_HOST=ON \
    -DONEBIT_NPU_RT_LIB=.../npu/libnpu_rt_api.a -DCMAKE_BUILD_TYPE=Release
$ cmake --build build-npu --target onebit npu_sampling_test -j14
[100%] Built target onebit
[100%] Built target npu_sampling_test
$ ./build-npu/npu_sampling_test
npu_sampling: ok
$ ctest --test-dir build-npu
100% tests passed, 0 tests failed out of 44
$ python3 tools/copyright.py --check     # CI's own check, exit 0

Those are the re-run numbers at the current head, 815408e: 44/44 in the build-npu tree, with
22 - npu_sampling among the 44 and passing (0.00 sec). A local reproduction of CI's own
configure (cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=gcc-16 -DCMAKE_CXX_COMPILER=g++-16, no -DONEBIT_NPU, the same target list as ci.yml; the two
FetchContent dependencies were pointed at the sources already fetched here rather than re-cloned)
gives 100% tests passed, 0 tests failed out of 43 with npu_sampling executed and npu_repack_path
skipped — 43, not 44, because that tree has no ONEBIT_NPU tests. (The reviewer's CI log is that same
tree before this commit: 22 - npu_sampling (Not Run) and 98% tests passed, 1 tests failed out of 43.)

The test covers: greedy == argmax including the tie by lowest id; temperature 1 able to draw the
second logit; top_k 1 == greedy and top_k 2 restricting the choice; top_p 0.5 keeping only the
head; the draw matching the hand-computed distribution over 20000 draws per case —
softmax({10,0,0})'s 4.6e-5 tail token, softmax({ln 3, 0, 0}) = {0.6, 0.2, 0.2}, and 9/11 at
temperature 0.5; the same seed giving the same 12-token sequence and another seed not; a token mask honoured
and an empty mask returning -1; stop matching (position, longest-at-a-tie, holdback, split across
tokens); every proper prefix of {"a": [1, true, "x"]} alive and only the full string accepted,
with malformed inputs dead; GBNF alternation, [a-z]+, [^a-c], "a"{2,3}, "a"*, non-ASCII
literals as UTF-8, and the parse errors as 400 text; and the request parser's defaults, overrides
and every 400 message.

Model runs: Qwen3-0.6B-NPU2 (model.q4nx, 653 MB) + tokenizer.json, host GEMM, one server,
this commit. Exact commands and outputs (the text/finish_reason of each answer, whole bodies
are longer):

$ NPU_INFER_HOST_GEMM=1 build-npu/1bit forward-serve -m /tmp/npu625/model/model.q4nx \
    --kernels /tmp/npu625/kernels -p 18080
1bit forward-serve: model ready on http://127.0.0.1:18080 (full ELF)

1. Greedy, no sampling fields — the command from the issue, text identical to the pre-change
binary (the pre-change binary answered "text":" Paris. The capital", finish_reason "stop";
only the finish reason changed, see above):

$ curl -sS localhost:18080/v1/completions -d '{"model":"m","prompt":"The capital of France is","max_tokens":8}'
{"text": " Paris. The capital of France is also", "finish_reason": "length"}

$ curl -sS localhost:18080/v1/completions -d '{"model":"m","prompt":"The capital of France is","max_tokens":4}'
{"choices":[{"finish_reason":"length","index":0,"text":" Paris. The capital"}, ...]}

2. Sampled, reproducible by seed, and a different seed changes it:

$ curl -sS localhost:18080/v1/completions -d '{"model":"m","prompt":"The capital of France is","max_tokens":8,
    "temperature":0.7,"top_k":40,"top_p":0.95,"seed":42}'    # run 1
{"text": " ___ in its country (the former president", "finish_reason": "length"}
   # run 2, same seed and body
{"text": " ___ in its country (the former president", "finish_reason": "length"}

$ curl -sS localhost:18080/v1/completions -d '{"model":"m","prompt":"The capital of France is","max_tokens":8,
    "temperature":0.7,"seed":7}'
{"text": " Paris. The capital of the United States", "finish_reason": "length"}

(The 0.6B base model rambles at temperature 0.7; temperature 0.01 on the same prompt still
answers Paris. The capital of France is also, which is the greedy text, so the draw is not
mangling the head of the distribution.)

3. Grammar-constrained, on the same prompt:

$ curl -sS localhost:18080/v1/completions -d '{"model":"m","prompt":"The capital of France is","max_tokens":16,
    "grammar":"root ::= \" Paris\" | \" Lyon\" | \" Marseille\""}'
{"text": " Paris", "finish_reason": "length"}

$ curl -sS localhost:18080/v1/completions -d '{"model":"m","prompt":"Q: name the capital of France, as JSON. A:",
    "max_tokens":32,"grammar":"root ::= \"{\\\"city\\\": \\\"\" [A-Za-z]+ \"\\\"}\""}'
{"text": "{\"city\": \"Paris\"}", "finish_reason": "length"}

$ curl -sS localhost:18080/v1/completions -d '{"model":"m","prompt":"The JSON describing Paris is ",
    "max_tokens":40,"response_format":{"type":"json_object"}}'
{"text": "{\"city\": \"Paris\", \"country\": \"France\", \"population\": 2000000, \"currency\": \"EUR",
 "region\": \"Europe\", \"latitude\": ", "finish_reason": "length"}

4. Stop sequences, and the stream route still frames:

$ curl -sS localhost:18080/v1/completions -d '{"model":"m","prompt":"The capital of France is",
    "max_tokens":20,"stop":[" of France"]}'
{"text": " Paris. The capital", "finish_reason": "stop"}

$ curl -sS -N localhost:18080/v1/completions -d '{"model":"m","prompt":"The capital of France is",
    "max_tokens":4,"stream":true}'
data: {"choices":[{"finish_reason":null,"index":0,"text":" Paris"}], ... "object":"chat.completion.chunk"}

5. The 400s (whole bodies):

{"error":{"message":"temperature: must be zero or more (zero decodes greedily)"}}                  [400]
{"error":{"message":"gbnf: unterminated literal at line 1"}}                                        [400]
{"error":{"message":"stop: at most 4 sequences"}}                                                   [400]
{"error":{"message":"top_p: must be above zero and at most 1 (1 turns it off)"}}                    [400]
{"error":{"message":"response_format: json_schema needs a schema-to-GBNF converter this route does
 not have; send grammar (GBNF) or type json_object"}}                                               [400]

Review follow-up: the three defects

Reviewer's list, one by one. Head 815408e.

1. The sampler applied temperature to probabilities. top_p wrote its softmax back over the
candidate array (x.logit = exp(x.logit - max_l)), so the temperature stage that follows divided
exp(probability) by T. exp() of a value in [0, 1] is flat to within e^-1, so the draw was
nearly uniform over whatever top_k/top_p had kept, and every tail token that survived got
~1/n_kept instead of its ~0 probability. Measured on {10, 0, 0} at temperature 1 with
top_p = 0.5, min_keep = 3 (so the 4.6e-5 tail tokens are kept and drawable), 20000 draws:

draws of the 4.6e-5 token	old chain	new chain	softmax says
logits {10, 0, 0}	4275	1	0.91 in 20000
logits {ln 3, 0, 0}, 0.2 token	5084	3960	4000

The fix keeps the logits: the top_p distribution lives in a std::vector<double> prob scratch
vector and c[i].logit stays a logit, so the chain is top_k on logits → top_p on the distribution
computed from them → temperature dividing the survivors → cumulative-sum draw. Top_p's softmax is
taken on the logits as they stand at that point, i.e. before the temperature, which is exactly what
llama.cpp's chain does (top_p_apply softmaxes cur_p; temp comes after top_p in
common/common.h:260-270) — at T = 1, the temperature the tests use, the two readings coincide.
New test test_sampler_distribution in the same file: three cases of 20000 draws against
hand-computed softmax probabilities. It fails on the old header with four FAIL lines and passes on
the new one.

2. CI never ran the test. .github/workflows/ci.yml's Build step has an explicit target list
(ctest runs whatever binaries exist, so a target missing from that list reports "Not Run"). The whole
diff is the target name, typed next to the neighbouring NPU tests:

-            npu_model_bounds_test npu_forward_loader_test tokenizer_linear_test hrx_placement_test hrx_cwsr_test -j
+            npu_model_bounds_test npu_forward_loader_test npu_sampling_test \
+            tokenizer_linear_test hrx_placement_test hrx_cwsr_test -j

3. The body overclaimed. The (...) in CI and 44/44 lines were wrong as wired; the section
above now states the command, the tree it ran in and the number that tree really reports (44/44 in
build-npu; 43/43 in the ONEBIT_NPU=OFF tree CI configures, where the test is #22).

The RFC question, for the maintainer

CONTRIBUTING.md asks for an RFC before "a change to 1bit serve's command line or HTTP API that
existing users would notice". This is additive — no field, no flag, the same greedy text when
nobody asks for anything — so I read it as the pull-request half, and #625 asks for the change. If
you would rather see it as a serving-mode change, say so and I will open the discussion and park
this branch until it is accepted.

Not touched: the private add-on route's repetition_penalty (app/unified.cpp,
npu/generate.cpp) is a different decode path and still does what docs/npu.md says; this route
has no repetition penalty, before or after. No kernel module, systemd unit, llama-server or
1bit-canonical was restarted, and the model runs above are short and finished.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 45acb70)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 625 - Partially compliant Compliant requirements: Parse response_format (json_object, json_schema) plus temperature and top_p in app/forward_serve.cpp, and reject what is not implemented with a 400 rather than ignoring it Mask logits before the argmax and advance the grammar state after the sampled token Handle the two traps: all tokens masked → softmax/NaN and per-request state leaking Non-compliant requirements: GBNF/grammar passthrough (FLM calls theirs "coming later" too), lazy grammars, and tool-call parsing — a JSON-schema path with the two traps handled is the useful subset (this is out of scope for the first cut) Requires further human verification: Verification that the grammar state is properly reset on every exit path (this was mentioned in the ticket but not explicitly implemented in the diff)

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Grammar State Reset The PR introduces grammar constraint handling but does not clearly show that the grammar state is properly reset on every exit path. This is critical to prevent state leakage between requests, as mentioned in the ticket. The code initializes a grammar_state variable but does not explicitly show how it's reset on all exit paths (e.g., early returns, exceptions). npu::Grammar::State grammar_state; Temperature Division Precision The temperature scaling in sample_token divides logits by temperature and then applies softmax. This is correct, but the implementation uses double for intermediate calculations and then converts back to float. While this is generally fine, it's worth noting that the precision handling could potentially lead to subtle differences in token selection compared to the exact llama.cpp implementation, especially in edge cases with very small or very large logits. float max_l = c.front().logit; for (const auto& x : c) max_l = std::max(max_l, x.logit); double sum = 0.0; for (auto& x : c) { x.logit = float(std::exp(double(x.logit - max_l) / double(p.temperature))); sum += x.logit; } std::uniform_real_distribution<double> dist(0.0, 1.0); const double target = sum * dist(rng); double run = 0.0; for (const auto& x : c) { run += x.logit; if (run >= target) return x.id; } return c.back().id; } UTF-8 Handling in Grammar Parsing The grammar parser handles UTF-8 literals, but the implementation assumes that all non-ASCII characters are encoded in UTF-8. While this is standard, it's important to ensure that the parser correctly handles invalid UTF-8 sequences and that the matching logic is robust against malformed input. The code does check for invalid UTF-8 but doesn't explicitly state how it handles such cases in the context of grammar matching. inline uint32_t GrammarParser::parse_utf8() { const unsigned char lead = (unsigned char)src_[pos_++]; int n = 0; uint32_t cp = 0; if ((lead & 0xE0) == 0xC0) { n = 1; cp = lead & 0x1F; } else if ((lead & 0xF0) == 0xE0) { n = 2; cp = lead & 0x0F; } else if ((lead & 0xF8) == 0xF0) { n = 3; cp = lead & 0x07; } else { fail("invalid UTF-8 in a literal"); } for (int i = 0; i < n; i++) { if (pos_ >= src_.size()) fail("truncated UTF-8 in a literal"); const unsigned char c = (unsigned char)src_[pos_]; if ((c & 0xC0) != 0x80) fail("invalid UTF-8 in a literal"); cp = (cp << 6) | (c & 0x3F); pos_++; } return cp; }

[github-actions]

Persistent review updated to latest commit 815408e

[github-actions]

Persistent review updated to latest commit 45acb70