Repository navigation
npu: the log-mel front-end, checked against the reference extractor (refs #1783) - #2175
bong-water-water-bong wants to merge 1 commit into
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 0877400: CHANGES
Reviewed head 0877400 (refs #1783). This is the first verdict. It is stacked on #2173 (base main-whisper-exec), which is stacked on #2154.
Its own change is +5 lines in CMakeLists.txt: the audio_mel row. #2133 (open, reviewed READY) already registers that row together with the other audio tests. The tree this PR builds inherits the stack's red copyright step:
missing copyright notice: tests/audio_mel_test.cpp
missing copyright notice: tests/audio_ops_test.cpp
missing copyright notice: tests/onnx_graph_test.cpp
The stack below it carries #1931's pre-review code. As described on #2173, its audio_exec.cpp drops #2133's audio_parallel.h, so a throwing unit reaches std::terminate again.
Suggested: close the three-PR stack (#2154, #2173, this one) in favour of #2133, or re-cut any split from #2133's files as they are.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 0877400
Canonical line for the CHANGES already on this head, with both findings reproduced here — a block the gate cannot parse is not a block, and this one is written
VERDICT at 0877400: CHANGES.
1. The registration collides with #2133, and I found it in both files:
#2175 CMakeLists.txt:226 add_executable(audio_mel_test tests/audio_mel_test.cpp)
#2133 CMakeLists.txt:1317 add_executable(audio_mel_test tests/audio_mel_test.cpp)
Both PRs add the same target from the same source file. Landing both is a duplicate add_executable for one name — the shape #1983's set-diff rule exists for, and the reason
whichever lands second has to be rebased rather than merged as-is.
2. The copyright step fails on this head, and the misses are the audio headers themselves:
$ python3 tools/copyright.py --check
missing copyright notice: npu/audio_exec.cpp
missing copyright notice: npu/audio_exec.h
missing copyright notice: npu/forward/include/audio_mel.h
missing copyright notice: npu/forward/include/audio_ops.h
$ rc = 1
So the build red is not inherited from the parent alone: the mel header this PR exists to test is itself one of the files without a notice. Fixing only the parent's notices
would leave this PR red.
3. And it stacks on #2173, whose audio_exec.cpp drops #include "audio_parallel.h" (grep -c audio_parallel.h npu/audio_exec.cpp → 0 on that head), i.e. the exception path
#2133 carries and both mutant arms protect. A PR whose base undoes a fix cannot land before the base is fixed.
Where I land, matching the prior reviewer: this content belongs in #2133, which already registers audio_mel_test and carries the front-end work with its reference-extractor
check. If #1783 needs the mel front-end reviewable on its own, split it from #2133's files — the split then inherits the notice headers and the audio_parallel.h seam instead of
re-deriving files without them.
Not re-run: the mel front-end against the reference extractor (needs the pack). My check is the registration collision, the copyright red, and the base's missing seam.
|
VERDICT: CHANGES at The mechanism behind the copyright red, measured — it is a short header, not a missing one, and that makes it a 15-line fix rather than a mystery. I said in my review that Both files start with What that changes: the fix is to paste the block (the 14 lines What it does not change: the registration collision with #2133 stands ( |
… 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
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 0877400
#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.
Third slice of #1783 (F8), stacked on #2173 (reader + executor). Refs #1783.
The one part of the pipeline that is not in the pack. The encoder's graph starts at the mel window, so the numbers here have to match the reference extractor's or every downstream measurement is against the wrong input.
npu/forward/include/audio_mel.h— hann-windowed STFT (n_fft 400, hop 160, reflect padding, periodic hann astorch.hann_windowis by default), power spectrum, a built slaney-normalised mel filterbank (librosa's scale:f_sp = 200/3Hz linear below 1 kHz,logstep = ln(6.4)/27above), then log10 with a floor and Whisper'smax − 8dynamic-range clamp plus(x+4)/4.Checked against transformers'
WhisperFeatureExtractor(feature_size=80)on 30 s of three summed sinusoids (440 / 1250 / 3100 Hz), both sides building the signal from the same formula:The silence arm is the one that matters for robustness: power 0 →
log10(1e-10) = −10, the max is that too, so themax−8clamp never bites and the scale lands on(−10+4)/4 = −1.5. My first expectation there said1.0— the test caught its author, which is the argument for exact-number arms rather than "looks reasonable".Two judgement calls, stated so they can be overruled: the filterbank is code, not a shipped table (an 80×201 constant is 64 KB nobody can review; the reference agrees with the formula, and flipping to a table is a one-file change with the same tests). And the front-end is not wired into a serve path — nothing in the engine consumes it yet; the message tool (#2173's next slice) will.
ctest -R audio_melpasses (5 s); fresh configure rc 0.