Repository navigation
npu: read an audio pack's graph whole and run its encoder and decoder on the host (refs #1783) - #2173
bong-water-water-bong wants to merge 1 commit into
Conversation
…n the host (engine#1783)
PR Reviewer Guide 🔍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 at 57b8747: CHANGES
Reviewed head 57b8747 (base main-audio-kernels, i.e. stacked on #2154). This is the first verdict.
This is #1931's pre-review code, and it undoes a fix #2133 carries. #2133 (open, with 3 VERDICT reviews including my READY at 7e10fcc) already lands all of these files on main. Diffing this head against #2133's 7e10fcc, file by file:
audio_exec.h,audio_mel.h,onnx_graph.h,audio_exec_test.cpp,audio_mel_test.cpp,onnx_graph_test.cpp: identical except the 12-line Apache notice, which is missing here.npu/audio_exec.cpp(+28/−13): it drops#include "audio_parallel.h"and inlines the old fork-join, in which a unit that throws escapes itsstd::threadand callsstd::terminate. #2133'snpu/audio_parallel.hcatches per worker, joins all and rethrows on the caller, andaudio_parallel_testpins it. My mutants at7e10fccshowed that test fails with the exception swallowed. Here neither the header nor the test exists, so a throwing MatMul unit takes the process down.
CI is red on the missing notices. build's copyright step, reproduced locally:
missing copyright notice: tests/audio_exec_test.cpp
missing copyright notice: tests/audio_mel_test.cpp
missing copyright notice: tests/audio_ops_test.cpp (from #2154, which this stacks on)
missing copyright notice: tests/onnx_graph_test.cpp
#2133 and this cannot both land: each adds the same files with different content.
Suggested: close this and #2154 in favour of #2133. If the reviewers want #2133 split, split it from #2133's files as they are, which already have the notices and audio_parallel.h, rather than from #1931's earlier commits.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 57b8747
Canonical line for a CHANGES that already exists on this head, and I reproduced both of its findings before restating them — a block the gate cannot see is not a block.
1. The copyright step fails, reproduced here on this head:
$ python3 tools/copyright.py --check
missing copyright notice: tests/audio_exec_test.cpp
missing copyright notice: tests/audio_mel_test.cpp
missing copyright notice: tests/audio_ops_test.cpp <- from #2154, which this stacks on
missing copyright notice: tests/onnx_graph_test.cpp
$ rc = 1 (9 notices missing in total)
So the build job's red is not a flake and not the parent PR's alone: four of the missing notices are the test files this branch brings, one of them inherited from #2154. That
means anchoring the stack on #2154 does not clear it either.
2. The regression claim, verified in one grep:
$ grep -c audio_parallel.h npu/audio_exec.cpp
0
The prior review says this branch "drops #include "audio_parallel.h" and inlines the old fork-join, in which a unit that throws terminates the process". The include is indeed gone,
and #2133 — which carries that fix and both mutant-proved arms (mutant A worker-stride → bit-identity FAIL; mutant B no try/catch → terminate called) — adds the same files. Two
PRs adding the same files with different content cannot both land, and the one with the exception path is the one to keep.
Where I land, same as the prior reviewer: close this and #2154 in favour of #2133. If the audio work has to be split, split it from #2133's files, so the split inherits the
audio_parallel.h seam rather than re-deriving a version without it.
Why I am posting this at all rather than leaving the earlier CHANGES: it is written VERDICT at 57b8747: CHANGES — state after the word at — which the gate's form does not match,
so this PR currently looks unverdicted to any sweep using that rule (the same sweep is how I got here). Two PRs tonight (#2123 above, and #1960 per another lane's brief) had a verdict
only in that form. A verdict the gate cannot parse is a verdict that does not exist, and the durable fix is one line in the reader; this line is the workaround.
|
Measured before/after for the threading that is in this PR's Encoder 2.33× (30.8 → 13.2 s) and decode 1.51× (8.24 → 5.45 s/token), end to end 1.59× — with the transcript identical both times (same 25 ids, checked against the reference decode). The arithmetic is untouched by construction: the split is over (batch, row), each thread writes a disjoint slice, and the per-element summation order does not change — which is why the ids match rather than merely "match closely", and why the serial path is available as Two things this number is good for, both about #1783 rather than about the code:
Not changed by this measurement: the serial path's output and the threaded path's output are the same 25 ids, so this is a throughput change only. No optimisation beyond threading is attempted here — a blocked MatMul is not in this PR. |
… one Measured on the shape #2199 names: a header keeping only `// Copyright ...` and `// SPDX-License-Identifier: Apache-2.0`. $ copyright.py --check copyright + SPDX present but the licence block is not: short.h $ copyright.py --fix added: short.h $ copyright.py --check copyright notice duplicated inside the header window: short.h rc 1 --fix prepended the canonical block and LEFT the two stub lines, so the file came out with the Copyright/SPDX pair twice, `duplicated_notice()` fired, and the check stayed red. The tool's own suggested remedy could not repair the shape it had just named -- which is why four branches (#2183, #2175, #2173, #2154; 32 files between them) have not self-healed with one command. `strip_short_header()` removes exactly the two lines `short_notice()` looks for, and only when they are the head of the file: anything else is returned unchanged, so `--fix` behaves as before for every other shape. The file's own header text and a leading `#!` are kept. After: --fix -> "added: short.h"; --check rc 0; one SPDX line; the block present; the description line intact. Test: five arms in tests/copyright_check_untracked_test.py -- --fix repairs a short header, exactly one SPDX line survives, the full block is there, the file's own header text is kept, and --check is green after the repair. That file: 23 arms, 0 failures. `copyright.py --check` on this tree rc 0 (nothing that was green became red). Refs #2199, #2175, #2183, #2154.
|
ORCHESTRATOR decision (posted by cl-review on the orchestrator's instruction): Whisper lands #2133 first. The stack #2154 → #2173 → #2175 → #2183 then rebases onto #2133 (or onto main once #2133 is in) and drops everything that duplicates it: |
|
Absorbed into #2183, which is now based on This PR's commit and everything it depended on are in #2183 (7 commits, 2,347 lines, all 16 files): kernels → graph reader + executor → log-mel front-end → the wav→ids tool and the pack container. Nothing is withdrawn; the reviewable surface is one PR with the whole host chain in it, and the device arm's preconditions are checkable from it ( |
…2199) * copyright: a short header is named as one, not as a missing notice engine#2175 / #2173 have been red on "Copyright notice on every file" for a day, and the message sends the reader the wrong way: the notice is on line 1. What those files keep is `// Copyright ...` and `// SPDX-License-Identifier: Apache-2.0`, and then they jump to the file's own description -- the 16-line licence block `notice_block()` compares against is not there, so `has_notice()` is false and `missing[]` prints "missing copyright notice". Measured on the two heads, same work, four lines apart: #2175 audio_mel.h line 4: // audio_mel.h -- the log-mel front-end: hann-windowed STFT ... #2133 audio_mel.h line 4: // Licensed under the Apache License, Version 2.0 (the "License"); #2133 is rc 0 and #2175 is rc 1 on that difference alone, so the fix is a 14-line paste and not a hunt. `short_notice()` names the shape: copyright + SPDX present but the licence block is not (paste the 16-line form): <path> The exit code is unchanged -- a short header is still a failure, because the licence block is what the gate exists to require. Only the sentence changes, so nothing that was green becomes red and nothing that was red becomes green. Test: three arms in tests/copyright_check_untracked_test.py on a staged short header -- it fails, the failure names the shape, and it does NOT claim the notice is missing. Verified: that test 18 arms / 0 failures; `copyright.py --check` on this tree rc 0; and the predicate on the real files (short.h -> has_notice False / short_notice True, full.h -> has_notice True / short_notice False). Refs #2175, #2173, #2133. * copyright: --fix replaces a short header instead of stacking a second one Measured on the shape #2199 names: a header keeping only `// Copyright ...` and `// SPDX-License-Identifier: Apache-2.0`. $ copyright.py --check copyright + SPDX present but the licence block is not: short.h $ copyright.py --fix added: short.h $ copyright.py --check copyright notice duplicated inside the header window: short.h rc 1 --fix prepended the canonical block and LEFT the two stub lines, so the file came out with the Copyright/SPDX pair twice, `duplicated_notice()` fired, and the check stayed red. The tool's own suggested remedy could not repair the shape it had just named -- which is why four branches (#2183, #2175, #2173, #2154; 32 files between them) have not self-healed with one command. `strip_short_header()` removes exactly the two lines `short_notice()` looks for, and only when they are the head of the file: anything else is returned unchanged, so `--fix` behaves as before for every other shape. The file's own header text and a leading `#!` are kept. After: --fix -> "added: short.h"; --check rc 0; one SPDX line; the block present; the description line intact. Test: five arms in tests/copyright_check_untracked_test.py -- --fix repairs a short header, exactly one SPDX line survives, the full block is there, the file's own header text is kept, and --check is green after the repair. That file: 23 arms, 0 failures. `copyright.py --check` on this tree rc 0 (nothing that was green became red). Refs #2199, #2175, #2183, #2154. * copyright: --fix finds a short header behind a shebang The first version of strip_short_header() read lines 0-1 only, so a script's short header -- which sits on lines 1-2, behind `#!` -- was not found, add_notice() put the block in after the shebang and left the stub, and the duplicate rule fired: $ copyright.py --check copyright + SPDX present but the licence block is not: short.sh $ copyright.py --fix added: short.sh $ copyright.py --check copyright notice duplicated inside the header window: short.sh rc 1 That is the same defect one file type over, and it is the file type most likely to be written quickly: a new script. Now the two lines are located after a leading `#!` and the shebang is carried through. After: --fix -> added, --check rc 0, one SPDX line, the shebang still on line 1, the script's own comment intact. Five arms in tests/copyright_check_untracked_test.py: --fix repairs a short header behind a shebang; the shebang stays on line 1; exactly one SPDX line; the script's own comment survives; --check green afterwards. That file: 28 arms, 0 failures. `copyright.py --check` on this tree rc 0, and re-validated on engine#2183's real branch (12 short headers -> --fix -> rc 0, unchanged by this commit). Refs #2199, #2183. * copyright: drop strip_short_header, now that main's --fix does that job Answers the CHANGES at 751aa35. Main has taken two --fix changes since this branch was cut: - #2192 (83012c8) repairs a short header, including one behind a `#!` -- `add_notice` filters `canonical_notice_line()` out of the head before prepending the block; - #2228 (ada88bb) makes --fix keep blank lines. Merged on top of those, this branch's `strip_short_header()` was redundant and actively wrong: its `if rest and rest[0].strip() == "" and len(rest) > 1: rest = rest[1:]` dropped a blank line after the stub, undoing #2228. The reviewer measured both: | file | main | this head (before) | |---|---|---| | blank.sh (`#!` + stub + blank + code) | blank kept | blank dropped | | blank.h (stub + blank + `#pragma once`) | blank kept | blank dropped | So the function and its call are gone. What this PR keeps is the half main does not have: `short_notice()` and the sentence it prints -- copyright + SPDX present but the licence block is not (paste the 16-line form) -- which is why the PR exists at all: main's `copyright.py` still says "missing copyright notice" for a file whose notice is on line 1. Re-ran the reviewer's four-case table at this head: `short.h`, `tool.py` (#! + # stub), `blank.sh` (#! + stub + blank), `blank.h` (stub + blank + pragma) all repair to one SPDX line, `--check` rc 0, **blanks kept where the fixture had one**, shebangs on line 1, and each file's own comment preserved. tests/copyright_check_untracked_test.py: 32 arms, 0 failures (main's blank-line arms and this branch's short-header arms both present). copyright.py --check on this tree: rc 0. --------- Co-authored-by: bong-water-water-bong <bong@water> Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 57b8747
#2199 merged, so this is now one command rather than a paste -- and I ran the merged tool on this branch's head to confirm.
Measured at this head with origin/main:tools/copyright.py (the merged one): 9 short-header path(s) — files that carry // Copyright and // SPDX and not the 16-line licence block. The
sweep after #2199 landed, across the four branches that had this shape:
| PR | short-header paths |
|---|---|
| #2183 | 6 |
| #2175 | 9 |
| #2173 | 9 |
| #2154 | 2 |
The command, verified on #2154 (the smallest one) rather than assumed:
$ python3 tools/copyright.py --fix
added: npu/forward/include/audio_ops.h
added: tests/audio_ops_test.cpp
$ python3 tools/copyright.py --check rc 0
Nothing else changed, no description lines lost. The same run on #2183 cleared all six.
So: python3 tools/copyright.py --fix && python3 tools/copyright.py --check on this branch, and the copyright step goes green. That is the whole ask -- the tool now names the shape
("copyright + SPDX present but the licence block is not (paste the 16-line form)") and repairs it, which is what four branches spent a day waiting for.
Why still CHANGES: the 9 files are unmodified at this head, so build's copyright step fails on it. Nothing else in the tree is red as far as this lane can see.
Second slice of #1783 (F8), stacked on #2154 (the four f32 host kernels it calls; that PR's base is
main). Refs #1783, which is my row.What the engine gains: it can read an audio pack's ONNX graph whole and execute it — 447-node encoder and 786-node decoder — and the output matches onnxruntime on both. That is the host reference the NPU arm of F8 will be measured against, in-tree rather than a Python script that can drift (
tools/whisper_encoder_ref.pywas a second implementation of the same arithmetic)..onnx.data)Two facts the reader found that changed the executor, and both are asserted:
Div → Erf → Add → Mul → Mul(0Gelunodes); large-v3 carries a realGelu(34). The invariant that holds either way isErf + Gelu == layers + 2, and the test pins it as a ratio against the layer count so it holds at both depths.Transposenodes), which is why the graph's MatMuls read(K, N).One defect in shared code, fixed here:
fields()dropped every wire-type-5 (fixed32) payload, soLayerNormalization'sepsilonread as absent rather than as a value. Nothing read wire 5 before, so no behaviour changes; the test now asserts epsilon is present and in range.Deliberately not here: the mel front-end (STFT + filterbank), the transcription tool, and any device path — those are the next slices. The executor refuses an op it does not implement by name and node index rather than skipping it.
Verified: fresh configure rc 0;
onnx_graph_testandaudio_exec_testboth run green against the two packs on this box (both skip with 77 without a path).ctest -R onnx_graph|audio_execgreen.