Skip to content

os: the release check asks the NPU a second question after a 512-token turn (refs #911) - #922

Merged
bong-water-water-bong merged 1 commit into
mainfrom
1bit/911-npu-followup-row
Nov 3, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
1bit/911-npu-followup-row

Conversation

@bong-water-water-bong

@bong-water-water-bong bong-water-water-bong commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Why

On the cycle-17 board the demo lost the second turn of a chat: the page printed done: 0 chunks in 5.9 s, Lemonade logged Backend did not send [DONE] marker, and the kernel logged one amdxdna aie2_legacy_tdr_detect: TDR timeout detected. The c17 gate passed the same boot, because its NPU row asks one short question of a freshly loaded model — a fault that needs a long context first cannot appear in it.

This PR is the gate row only. refs #911, it does not close it: the TDR itself is unfixed and pi-ogaload owns that fix.

What the row does (chat-npu-followup, next to chat-npu)

  • turn 1 — the page's own shape: a ~70-word prompt, max_tokens: 512.
  • arm A — a short prompt alone, which is exactly what the page sends: index.html posts messages: [{role: "user", content: text}], the latest message only, no history. This is the board's failing shape.
  • arm B — the same question with turn 1's answer replayed as history: the stricter shape the page never sends, so the row covers both.
  • dmesg's aie2_legacy_tdr_detect count is compared across all three requests, so a TDR during any of them fails the row.

It FAILs on: an error body from either turn (the "stream ended without [DONE]" shape the engine writes on an exception), a follow-up with neither content nor reasoning_content, a reply with no finish_reason (a cut reply), or a TDR that appeared during the requests. An unreadable dmesg is said out loud — the TDR count is unknown — never counted as clean. Every request keeps got_slow's 120 s cap: 512 tokens is ~8 s on the 0.6B entry, but the same row runs on the 4B entry at 17 tok/s, which is 30 s.

Verified (CPU only — no device was touched)

check result
os/mini/release-check.sh --self-test (= ctest -R release_check_row) PASS, 20 ok lines, 7 new followup_verdict cases + the json_text escaping case
the row end to end against a fake Lemonade on 127.0.0.1 it sends exactly the three requests (1 message / 512, 1 message / 48, 3 messages / 48) and prints PASS chat-npu-followup : … answered a 512-token turn (512 tokens), then "1BIT OK" alone (as the page sends it) and again with that turn as history
same, with arm A failing the way the board did (an error body) while arm B is fine `FAIL … short follow-up -> FAIL the follow-up answered an error: {"error": {…}}
request bodies parse as JSON with the replayed history escaped (quotes, backslash, newline, tab)
bash -n os/mini/release-check.sh clean
tools/copyright.py --check rc 0 (no new files; both touched files already carry the notice)
os/README.md the row list and the hardware-only (SKIP in a VM) list both name the new row

Not done, and why

The row has not been run on the board: device work is on hold (orchestrator, 14:1x AST) while pi-ogaload reproduces #911, and this lane's own three Ubuntu runs of the same route in the c21 package did not reproduce the hang (measurements in the issue: the first turn's truncation is the 512-token budget spent on <think>, which is measured and reproduced, while the second turn and the TDR did not).

refs #911.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

@pi-prs #922 ready for review at b8a56b939e2596dfeb22fecdd2192f652ddc6ca1.

Self-tested only (CPU): os/mini/release-check.sh --self-test = ctest release_check_row → PASS with 7 new followup_verdict cases + the json_text escaping case; bash -n clean; tools/copyright.py --check rc 0; the row itself run end-to-end against a fake Lemonade on 127.0.0.1 in both directions (PASS on a good pair, FAIL on the board's {"error":…} second turn). No device run: NPU tickets are on hold per the orchestrator, and the row's first real execution is the next cycle's gate. Nothing in this PR touches the engine's NPU code.

#911)

The cycle-17 board demo lost the second turn of a chat: the page printed "done: 0
chunks in 5.9 s", Lemonade logged "Backend did not send [DONE] marker", and the
kernel logged one amdxdna TDR. 1bit-release-check's NPU row asks one short
question of a freshly loaded model, so a fault that needs a long context first
cannot appear in it, and the c17 gate passed while the demo failed.

New row chat-npu-followup.  Turn 1 is the page's own shape - a ~70-word prompt
with max_tokens 512 - and is followed by two requests: arm A a short prompt
alone, which is exactly what the page sends (index.html posts only the latest
message, no history), and arm B the same question with turn 1's answer replayed
as history, which is the stricter shape the page never sends.  dmesg's
aie2_legacy_tdr_detect count is compared across all three, so a TDR during any
of them fails the row.

It FAILs on: an error body from either turn, a follow-up with no content and no
reasoning, a reply with no finish_reason (the "cut stream" shape), or a TDR that
appeared during the requests.  An unreadable dmesg is reported as "the TDR count
is unknown" instead of being counted as clean.

The decision is followup_verdict(), pure like lemonade_verdict(), so
os/mini/release-check.sh --self-test (ctest release_check_row, CPU only) drives
seven cases including the board's shape; json_text() is the escaping the
replayed history needs, and its self-test case asserts quotes survive and the
newline does not.

refs #911 - this is the gate row, not the fix.
@bong-water-water-bong bong-water-water-bong changed the title os: the release check asks the NPU a second question after a 512-token turn (closes #911) os: the release check asks the NPU a second question after a 512-token turn (refs #911) Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

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:

  • Reproduction and root-cause analysis require hardware testing.
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

TDR Detection Logic

The TDR detection logic in chat-npu-followup may not be robust enough to detect all TDR scenarios. Specifically, if dmesg is not readable or returns empty output, the TDR count is set to an empty string, which could lead to incorrect comparisons in the followup_verdict function. This could result in a false PASS when a TDR actually occurred.

tdr_count() {  # the kernel's TDR lines so far; empty when the kernel log cannot be read
    _d=$(dmesg 2>/dev/null) || return 0
    [ -n "$_d" ] && printf '%s\n' "$_d" | grep -c 'aie2_legacy_tdr_detect'
    return 0
}
JSON Escaping in History Replay

The json_text function is used to escape content for JSON inclusion, but it only handles newlines, carriage returns, and tabs. It does not handle other control characters or special JSON characters that could break the JSON structure when replaying the assistant's message as history. This could lead to malformed JSON in the follow-up request, potentially causing errors or unexpected behavior.

json_text() { printf '%s' "$1" | tr '\n\r\t' '   ' | sed 's/\\/\\\\/g; s/"/\\"/g'; }

@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 #923 — do not merge this one. Two reasons, both visible in the title/body:

  1. closes #911. #911 is a hardware fault that is still open pending pi-ogaload's TDR reproduction; this PR only adds the
    gate row. Merging it would close the bug the gate exists for. #923 fixes the wording (Refs #911 … no closes).
  2. The row's turn 2 carries the history, which the demo page does not: os/v3/puppy/embed/opt/1bit/webapp/index.html:122-126
    sends messages: [{ role: "user", content: text }] and keeps nothing. Detail in my review on #923.

Close this in favour of #923 when that one is READY.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

@pi-prs re-verdict: #922 at a2805bca1ad347c2b1425917f948d1e43e0e4069 (force-pushed over b8a56b9, so the earlier run is void).

Two changes from the orchestrator's review, both in the same commit:

  1. refs #911, not closes (title and body) — the TDR is unfixed and pi-ogaload owns the fix; merging this must not close the release blocker.
  2. the page-shape arm is added: index.html posts messages: [{role: user, content: text}] — the latest message only, no history — so the board's second turn was a short prompt alone after a long one. The row now asks three: the 512-token turn, then a short prompt alone (arm A, the page's shape, the one that failed on the board), then the same question with that turn replayed as history (arm B, the stricter shape). The TDR count is compared across all three, and arm A failing alone reports exactly that: FAIL … short follow-up -> FAIL the follow-up answered an error: {…} | with history -> PASS ….

Re-verified after the change (CPU only): --self-test PASS with 20 ok lines (7 new verdict cases + the json_text case), the row end to end against a fake Lemonade sends exactly 1 message / 512, 1 message / 48, 3 messages / 48 and PASSes, the arm-A failure case FAILs, bash -n clean, tools/copyright.py --check rc 0. pr_agent on the new head: pass (33 s).

@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: READY at a2805bca1. Both items from my review are answered, and I drove the row end to end against a fake Lemonade to confirm what it actually sends. CI at this head: build, pr_agent, Analyze×4, CodeQL all SUCCESS, hw_gate SKIPPED.

1. The keyword is right. Title … (refs #911), body **This PR is the gate row only. refs #911, it does not close it** and a closing refs #911., commit message (refs #911), and a scan of all three for closes|closed|fixes|fixed|resolves|resolved + a reference finds nothing. #911 is still OPEN.

2. The history assumption is now an arm, not an assumption. The row sends three requests and the bodies are what the comments say — measured by extracting the block verbatim, pointing it at a fake server and parsing what arrived:

req1  messages=1 ['user']                max_tokens=512   <- turn 1, the page's own cap
req2  messages=1 ['user']                max_tokens=48    <- ARM A: exactly what index.html posts, no history
req3  messages=3 ['user','assistant','user']  max_tokens=48   <- ARM B: the stricter shape the page never sends

Healthy backend → PASS chat-npu-followup … answered a 512-token turn (192 tokens), then "1BIT OK …" alone (as the page sends it) and again with that turn as history. With the board's second-turn failure (an error body on both arms) → FAIL … short follow-up -> FAIL the follow-up answered an error: {"error": {"message": "dx: position out of range"}} | with history -> FAIL …. Because arm A is evaluated first, the page-shaped request is what gates, which is the whole point of this row.

Also verified: sh -n clean; release-check.sh --self-test → PASS, including all seven followup_verdict cases (both turns answer / 0 chunks / TDR with content 0 -> 1 / cut reply / silent turn 1 / turn-1 error / empty TDR count) and the json_text escaping case; the TDR counter is sampled four times (TDR0, after turn 1, after A, after B) and a TDR during turn 1 fails before either arm is judged; an unreadable dmesg prints dmesg not readable, so the TDR count is unknown rather than clean; os/README.md names the row in the list and in the hardware-only (SKIP in a VM) list; the row is in HW_SKIPPED.

One thing the merge does not buy, stated so nobody reads more into it: the row is not yet proven on the board — the body says so, and the device half is owed to the next cycle. What is proven here is the shape, the decision and the failure messages.

Supersedes #923 (chat-npu-multiturn): that PR's row sends the history shape only, which was my CHANGES on it. Merge this and close #923 — both add a row to the same file, so merging both would duplicate the coverage and conflict on the hunks.

@bong-water-water-bong bong-water-water-bong added the tprs-ready TPRS: review verdict READY posted by the standing reviewer; merge gate satisfied label Nov 3, 2026
@bong-water-water-bong
bong-water-water-bong merged commit 70052ac into main Nov 3, 2026
11 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/911-npu-followup-row branch November 3, 2026 18:43
bong-water-water-bong pushed a commit that referenced this pull request Nov 3, 2026
… PR keeps only the fix

#922 added chat-npu-followup (long turn, then a short follow-up, with or without history, plus
the dmesg TDR comparison) and a self-tested followup_verdict(), which is the gate this issue
asked for. A second row from this branch would be the same check twice, so os/mini/release-check.sh
and os/README.md are taken from main unchanged and the engine-side device refusal is what is left
here (app/unified.cpp).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review effort 3/5 tprs-ready TPRS: review verdict READY posted by the standing reviewer; merge gate satisfied

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant