Repository navigation
forward-serve: --dump-logits writes the raw distribution, so a KLD comparison is possible at all - #940
Conversation
…mparison is possible at all The OpenAI API caps top_logprobs at 100, and `1bit npu-run --dump-logits` drives the old lane family (layer_ctx1.elf / layer_pdi) rather than the per-shape full_i8_* family every current container uses, so neither route can produce a vocabulary-wide distribution for an imported pack. The dump is taken immediately before sample_token(), which takes logits by const reference, so it is the model's own distribution -- before temperature, top-k/top-p and the GBNF mask narrow it. Off by default; a bad directory is refused at startup rather than ignored. Measured against AMD's recorded distribution for the same pack and prompts (engine#671 gate b): KL(AMD||ours) = 0.0003 / 0.0006 / 0.0025 nats, argmax agreeing at every recorded position.
|
Addendum: the same measurement re-run from this branch's own build ( |
PR Reviewer Guide 🔍(Review updated until commit 41eb612)Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 41eb61291. The dump is taken from the right place, it compiles with the real NPU flags, and the
oracle half of the proof table checks out from the artefact. Two notes, neither a gate.
It compiles where CI does not. app/forward_serve.cpp at this head, compiled with the project's own
ONEBIT_NPU flags (taken from an existing -DONEBIT_NPU=ON compile_commands.json, source tree pointed at this
worktree): rc 0, including the new --dump-logits block. tools/copyright.py --check rc 0.
The distribution is the model's, not the sampler's. sample_token(const std::vector<float> & logits, …) takes
it by const reference (app/npu_sampling.h:88) and the dump copy runs immediately before the call, with the grammar
mask passed separately (gram ? &allowed : nullptr), so the file is pre-temperature, pre-top-k/p and pre-GBNF. The
top_logprobs claim is the code's own: npu_sampling.h:295 is literally
"top_logprobs: must be 0 to " + kMaxTopLogprobs, and the cap is why the API route cannot serve gate (b).
The AMD side of the table verifies from ~/ogaload/window/oracle_logits.npz (which is on the box):
prompt 0 argmax 6722 softmax p(argmax) = 0.9987 top3 (6722 .9987, 3146 .0003, 4226 .0001)
prompt 1 argmax 1156 softmax p = 0.9936 top3 (1156 .9936, 2326 .0052, 10250 .0007)
prompt 2 argmax 12884 softmax p = 0.9512 top3 (12884 .9512, 6303 .0224, 9237 .0093)
6722 / 1156 / 12884 and 0.999 / 0.994 / 0.951 are the table's AMD column exactly, and out_i == logits_i
byte for byte in the npz, so the two names in that file are one distribution. What I could not reproduce is the
KL column (0.0003 / 0.0006 / 0.0025) and our probabilities: see the first note.
Note 1 (the one that costs this tool a device token): forward-serve takes the NPU device lock even in
host-GEMM mode. take_npu_device_lock(streams) is at app/forward_serve.cpp:752, and the NPU_INFER_HOST_GEMM
check that decides whether a device is opened at all is at :758. So the very run this flag exists for — the
host-GEMM reference dump — cannot start while another process holds the lock. It could not here: the owner's Bonsai
serve holds /tmp/1bit-npu-device.lock, so I did not run it (lane rule), which is why the KL column above is yours
and not mine. Moving the host_gemm decision above the lock, or giving it an explicit opt-out, would make the
reference dump runnable beside a serve — and it is exactly the kind of run that should not need the device.
Note 2 (small): the std::fopen in the loop is silent on failure — a directory that goes away mid-run produces
a clean answer and no logits. The startup check covers the ordinary case, so a one-line warning there would only
matter for a long run.
Everything else is right: off by default (dump_logits empty ⇒ nothing written), a bad directory refused at
startup rather than ignored, one step_<n>.bin per generated position, and the file is vocabulary × float32.
mergeStateStatus BLOCKED is CI only (build and pr_agent were still running); the code review above is at this
head.
|
CI is green at |
|
Persistent review updated to latest commit 41eb612 |
What
forward-servegains an off-by-default--dump-logits <dir>flag that writes each generated position's rawfloat32 distribution over the whole vocabulary as
step_<n>.bin.Root cause / why it is needed
Measuring a KL divergence against another stack (engine#671 gate (b)) needs the distribution, and neither existing route
can produce it:
top_logprobs—{"error":{"message":"top_logprobs: must be 0 to 100"}};1bit npu-run --dump-logitsdoes dump logits, but it drives the old lane family (layer_ctx1.elf,layer_pdi),not the
full_i8_*per-shape family that every current container uses, so on an imported OGA pack it fails at loadwith
cannot read …/layer_ctx1.elf.The dump is taken immediately before
sample_token(), which takes logits byconstreference, so what lands on disk isthe model's own distribution — before temperature, top-k/top-p and the GBNF mask narrow it.
Proof (measured, not asserted)
app/forward_serve.cpp, one PR, no behaviour change when the flag is absent (nothing is written unless asked; a baddirectory is refused at startup rather than silently ignored).
Against AMD's recorded distribution for the same pack and the same prompts (
~/ogaload/window/oracle_logits.npz,captured through OGA on AMD's own NPU stack), our imported container in host-GEMM mode:
Alignment was proven rather than assumed: AMD's capture is the distribution after its first generated token, so the
comparison uses our
step_1and requires ourstep_0argmax to equal AMD's first output token (785) — it does, on allthree prompts. (My own first run compared
step_0and reported 11-21 nats, which is what a one-position offset lookslike, and is the reason the alignment check is now printed on every row.)
Checks: builds with
ONEBIT_NPU=ON,ninja onebitclean, existingctestNPU set unaffected;tools/copyright.py --checkon the touched file. Closes nothing on its own — it is the vehicle for #671 gate (b).Notes
the docstring in the struct says so.