Skip to content

release-check: a second, long-then-short NPU turn, because one short prompt cannot see #911 - #923

Closed
bong-water-water-bong wants to merge 2 commits into
mainfrom
release/npu-multiturn-row
Closed

bong-water-water-bong wants to merge 2 commits into
mainfrom
release/npu-multiturn-row

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Root cause (why a row is needed at all)

Engine #911, from the c17 board on 3 Nov 13:22 (engine ca1d42f, package c21): chat-npu passed at 96.6 tok/s, then the next request on the same route hung — 0 chunks, lemond's WARNING: Backend did not send [DONE] marker, and aie2_legacy_tdr_detect: TDR timeout detected in the same second (board dmesg.log / lemond.log, evidence in ~/briefs/NPU-MULTITURN-TDR-2026-11-03.md and ~/briefs/RELEASE-C17-VERDICT.md).

The gate could not see it: chat-npu is one non-streaming, 48-token, /no_think prompt. One prompt cannot tell "the route works" from "the route works once".

The change

os/mini/release-check.sh only — a new row chat-npu-multiturn, run when the NPU and an NPU model are present:

  • turn 1: a long prompt (~70 words, the board's own repro), streamed, enable_thinking:false;
  • turn 2: a short follow-up carrying the history, streamed, same flags;
  • it counts deltas of content and reasoning_content, not the [DONE] marker — lemond appends [DONE] itself when the backend never sent one, so a row grepping for [DONE] would have passed this exact failure (measured in the board's own lemond.log);
  • a new aie2_legacy_tdr_detect line during the two turns fails the row even when chunks arrive; a turn that reaches the 240 s cap fails naming the cap;
  • the decision is a pure function (npu_turns_verdict) so --self-test drives it, like lemonade_verdict (os: prove the API key is enforced in 1bit-release-check, do not infer it #569);
  • SKIP wording mirrors chat-npu (no /dev/accel/accel*, or no NPU entry listed), so a VM is unchanged.

Measured (before / after)

~/wt/release-rc/rowproof-npu-multiturn.sh — extracts the block verbatim and drives four arms:

arm backend verdict
healthy streams both turns PASS — "turn 1 3 chunks, turn 2 3 chunks"
board turn 1 streams, turn 2 answers 200 + [DONE], zero deltas FAIL — "turn 2 delivered 0 chunks after a long first turn (engine #911 …)"
hang turn 2 never answers FAIL — "turn 2 never finished (curl/timeout rc 28 = the cap)"
device the real NPU: 1bit serve -m ~/models/npu/Qwen3-0.6B --device npu, under npu-hold PASS — turn 1 192 chunks, turn 2 15 chunks, no TDR

sh -n, busybox ash -n and os/mini/release-check.sh --self-test (7 new cases: both turns answer / 0 chunks / TDR with chunks / turn-2 timeout / silent turn 1 / transport failure / empty TDR count) are green; tools/copyright.py --check rc 0.

Notes for the fixer of #911

On this box the direct route answered both turns (192 + 15 chunks, no TDR), so the hang is not simply "any second request". The board's router log has the difference in two adjacent lines, both from the same load:

13:20:29.352 (Router) Auto-tune ctx_size resolved to 32768
13:20:29.600 (Process) dx: kernel set serves up to 4096 cached tokens (128 attention blocks)

i.e. lemond allocated 8x the context the dx kernel set says it serves. Worth testing --ctx-size 4096 (or a cap in the router) on the board repro. This row goes through the lemond API, so it will exercise whatever the router decides.

Refs #911 — this closes the gate gap the issue names, not the defect itself (no closes).

…prompt cannot see #911

The c17 board (3 Nov 13:22, engine #911) served the NPU chat row at 96.6 tok/s and then hung on the NEXT
request: 0 chunks, lemond's "Backend did not send [DONE]", and an amdxdna TDR in the same second. The row that
would have caught it did not exist - chat-npu is one non-streaming 48-token /no_think prompt, which cannot tell
"the route works" from "the route works once".

chat-npu-multiturn sends a long prompt (~70 words, the board's own repro) and then a short follow-up carrying the
history, both streamed, thinking off. It counts DELTAS of content and reasoning_content, not the [DONE] marker:
lemond appends [DONE] itself when the backend never sent one, so a row grepping for it would have passed this exact
failure (measured in the board's lemond.log). A new aie2_legacy_tdr_detect line during the two turns fails the row
even when chunks arrive, and a turn that hits the 240 s cap fails naming the cap.

The decision is a pure function (npu_turns_verdict) so --self-test drives it - 7 new cases: both turns answer, the
board's 0 chunks, a TDR with chunks, a turn-2 timeout, a silent first turn, a transport failure, an empty TDR count.
Measured: ./rowproof-npu-multiturn.sh, four arms - a healthy stream PASS, the board's 200+[DONE]+no-deltas FAIL
naming "0 chunks", a hung turn 2 FAIL naming the cap, and the REAL NPU through '1bit serve --device npu' PASS
(turn 1 192 chunks, turn 2 15 chunks, no TDR). sh -n, busybox ash -n and release-check --self-test green.

Refs #911 (this is the gate gap it named, not the fix).
@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 6e81045)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

911 - Partially compliant

Compliant requirements:

  • Add a long-prompt-then-follow-up row to the release check's NPU chat

Non-compliant requirements:

  • Reproduce on Ubuntu with the c21 package: the long prompt, then a short one, on this route
  • Separate the TDR from any thinking artifact
  • Root-cause and fix it

Requires further human verification:

  • Reproduce on Ubuntu with the c21 package: the long prompt, then a short one, on this route
  • Separate the TDR from any thinking artifact
  • Root-cause and fix it

569 - Partially compliant

Compliant requirements:

  • Decide the row from HTTP status codes, not from whether a body came back
  • The verdict is a pure function (lemonade_verdict <keyed status> <keyed body> <keyless status>)
  • Add a --self-test that asserts seven cases including the unprepared-stick one
  • The script gained an --self-test that asserts seven cases — including the unprepared-stick one (200 + list with the key, empty body without it) and the "no server at all" case

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

Potential TDR Detection Flaw

The TDR detection logic in chat-npu-multiturn may not be robust enough to distinguish between a TDR that occurred during the test run and one that was already present in the system. Specifically, the check _tdr0=$(_TDR) captures the TDR count before the test, but if a TDR had already occurred, it might be missed or misinterpreted in the delta calculation.

_TDR() { { dmesg 2>/dev/null || journalctl -k 2>/dev/null || true; } | grep -c 'aie2_legacy_tdr_detect' 2>/dev/null || true; }
MT_CAP=${MT_CAP:-240}   # seam: the proof drives the timeout arm with a short cap
_NW=$(mktemp -d)
_tdr0=$(_TDR)
_long='Explain in three sentences why a compiler for an accelerator array must choose the tile shape before it schedules the loop nest, then sketch the trade off between a wider tile and a deeper pipeline for a matrix multiply whose intra layer memory is fixed.'
printf '{"model":"%s","messages":[{\"role\":\"user\",\"content\":\"%s\"}],"max_tokens":192,"temperature":0,"stream":true,"chat_template_kwargs":{"enable_thinking":false}}' \
    "$NPU" "$_long" > "$_NW/t1.json"
_post() {  # <body file> <out file> -> transport rc
    if command -v curl > /dev/null 2>&1; then
        timeout $((MT_CAP + 20)) curl -sS -N --connect-timeout 5 --max-time "$MT_CAP" -o "$2" \
            -H 'Content-Type: application/json' -H "Authorization: Bearer $KEY" \
            --data-binary "@$1" "$API/api/v1/chat/completions" 2>/dev/null
    else
        timeout $((MT_CAP + 20)) wget -q -T "$MT_CAP" -t 1 -O "$2" --header='Content-Type: application/json' \
            --header="Authorization: Bearer $KEY" --post-file="$1" "$API/api/v1/chat/completions" 2>/dev/null
    fi
}
_deltas() { grep -o '"content"[[:space:]]*:[[:space:]]*"[^"]' "$1" 2>/dev/null | wc -l | tr -d ' '; }
_post "$_NW/t1.json" "$_NW/t1"
_dc=$(_deltas "$_NW/t1")
_dr=$(grep -o '"reasoning_content"[[:space:]]*:[[:space:]]*"[^"]' "$_NW/t1" 2>/dev/null | wc -l | tr -d ' ')
_t1=$((_dc + _dr))
if [ "$_t1" -gt 0 ]; then
    _ans=$(sed -n 's/.*"content"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p' "$_NW/t1" 2>/dev/null | tr -s '[:space:]' ' ' | head -c 400)
    [ -n "$_ans" ] || _ans=$(sed -n 's/.*"reasoning_content"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p' "$_NW/t1" 2>/dev/null | tr -s '[:space:]' ' ' | head -c 400)
    _ans=$(printf '%s' "$_ans" | tr -d '"\\' | tr -d '\000-\037')
    printf '{"model":"%s","messages":[{\"role\":\"user\",\"content\":\"%s\"},{\"role\":\"assistant\",\"content\":\"%s\"},{\"role\":\"user\",\"content\":\"In one short sentence: what came first in what you just described?\"}],"max_tokens":96,"temperature":0,"stream":true,"chat_template_kwargs":{"enable_thinking":false}}' \
        "$NPU" "$_long" "$_ans" > "$_NW/t2.json"
else
    cp "$_NW/t1.json" "$_NW/t2.json"   # turn 1 said nothing; turn 2 has nothing to carry
fi
_post "$_NW/t2.json" "$_NW/t2"; _rc=$?
_t2c=$(_deltas "$_NW/t2")
_t2r=$(grep -o '"reasoning_content"[[:space:]]*:[[:space:]]*"[^"]' "$_NW/t2" 2>/dev/null | wc -l | tr -d ' ')
_t2=$((_t2c + _t2r))
_tdr1=$(_TDR)
case "$_tdr0" in ''|*[!0-9]*) _tdr0=0 ;; esac
case "$_tdr1" in ''|*[!0-9]*) _tdr1=$_tdr0 ;; esac
_v=$(npu_turns_verdict "$_t1" "$_t2" "$_rc" "$((_tdr1 - _tdr0))")
Incomplete Error Handling for TDR Counting

The code for counting TDRs uses grep -c 'aie2_legacy_tdr_detect' which may not be robust against variations in log message formats or partial matches. If the log format changes slightly, the TDR detection could fail silently.

_TDR() { { dmesg 2>/dev/null || journalctl -k 2>/dev/null || true; } | grep -c 'aie2_legacy_tdr_detect' 2>/dev/null || true; }

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at c69af2850 — the keyword item is fixed; the row still sends a request sequence the demo does not, and that is the shape that failed.

Fixed, and thank you for the explicit wording: Refs #911 — this closes the gate gap the issue names, not the defect itself (no closes), the title carries no closing keyword, and #911 is still OPEN. That half is closed.

Still open — the history assumption. The row's turn 2 carries the conversation:

os/mini/release-check.sh  (this head, ~:663)
printf '{"model":"%s","messages":[{"role":"user","content":"%s"},{"role":"assistant","content":"%s"},{"role":"user","content":"In one short sentence: …"}]…

but the page that produced #911 sends one user message and keeps nothing:

os/v3/puppy/embed/opt/1bit/webapp/index.html:122-126   (main)
fetch(api + "/api/v1/chat/completions", { …,
  body: JSON.stringify({ model: m, messages: [{ role: "user", content: text }],
                         max_tokens: 512, temperature: 0, stream: true }) })

ask() clears the input, appends to out.textContent for display, and has no localStorage/sessionStorage/history
array anywhere in the file (grep -n 'localStorage\|sessionStorage\|push(\|history\[' → nothing). And the failure text you
quote is that page's own status line — done: 0 chunks in 5.9 s is stat.textContent = "done: " + chunks + " chunks in …".
So the failing request was a fresh single-message request issued while the route still held the previous 512-token turn,
not a request that replayed the answer.

That difference is load-bearing, not cosmetic: with history, turn 2's prompt is ~192 tokens re-processed; without it, turn 2
is a ~15-token prompt against whatever state the first turn left in the runtime — which is exactly where your own root-cause
note points (ctx_size 32768 from the router versus the dx kernel set's 4096 cached tokens). A row that always sends the
long history cannot see the second shape, and the arm you ran on the device (turn 1 192 chunks, turn 2 15 chunks, no TDR)
is evidence about the history shape only. As written the row can be green while the demo still hangs — the same class as
#906, one level up.

Two asks:

  1. Turn 2 = the page's body: messages: [{role:"user", content:"<short question>"}], no replayed assistant turn. If the
    history variant is worth keeping, make it a second arm clearly labelled as not the page's shape — the gate row should
    be the shape the demo will send.
  2. max_tokens: the page sends 512 for its turn; the row sends 192 (turn 1) and 96 (turn 2). If 192 is a deliberate
    cost choice, say so in the comment next to it, because "the board's own repro" and the demo's own cap are 512.

Nothing else needs re-review on my side: the [DONE]-marker point is right and well argued (lemond appending it is why a
marker-grep row would have passed this exact failure), the TDR delta across the pair is the right instrument, an unreadable
dmesg is said out loud rather than counted clean, and the 7 npu_turns_verdict self-test cases cover the decision's
branches. #922 must not be merged — its title still carries closes #911, and this PR supersedes it.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 6e81045

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superseded by #922, which is now READY and covers both shapes: its arm A is this PR's missing piece — a single-message, no-history follow-up, exactly what os/v3/puppy/embed/opt/1bit/webapp/index.html:122-126 sends — and its arm B is the history shape this row sends. Merge #922 and close this; both add a row to the same file, so merging both would duplicate the coverage and conflict on the hunks.

The head move (6e810455b) is a merge of main only; the row's request bodies are unchanged from c69af2850, so my CHANGES on it stands as written.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Superseded by #922 (merged 18:43Z, chat-npu-followup), which does strictly more than this row and is what ships in c22: a 512-token turn first, then both follow-up shapes (the page's own — a short prompt with no history — and the stricter one with that turn replayed as history), with the dmesg TDR count compared across all three requests, and its own pure followup_verdict.

Keeping the two would mean two rows asserting the same thing with different wording on every stick, so this one goes. What is worth carrying over from it, and is in the #911 record either way:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant