Skip to content

Add NVIDIA Nemotron-3.5-ASR streaming multilingual backend - #10

Merged
Alex-Wengg merged 13 commits into
mainfrom
add-nemotron-streaming-support
Jun 29, 2026
Merged

Add NVIDIA Nemotron-3.5-ASR streaming multilingual backend#10
Alex-Wengg merged 13 commits into
mainfrom
add-nemotron-streaming-support

Conversation

@Alex-Wengg

@Alex-Wengg Alex-Wengg commented Jun 28, 2026

Copy link
Copy Markdown
Member

Closes #8.

Adds a new eddy::nemotron backend for NVIDIA's nemotron-3.5-asr-streaming-0.6b — a cache-aware streaming FastConformer-RNNT with prompt-conditioned multilingual decoding (40+ languages).

Why a separate module

The existing Parakeet path is a stateless overlapping-chunk encoder with TDT duration-bin decoding. Nemotron is fundamentally different — cache-aware streaming, plain RNNT, and an integer prompt_id for per-chunk language conditioning — so it gets its own backend rather than overloading Parakeet:

  • Cache-aware streaming encoder loop (carries cache_channel/cache_time/cache_len across chunks, att_context=[56,0]).
  • Integer prompt_id language conditioning via the encoder's prompt_kernel (see prompt_dictionary in metadata.json; auto=101).
  • Plain RNNT greedy decode (2× LSTM @ 640 → joint → vocab 13088, blank 13087).
  • Dedicated tokenizer: Nemotron emits standalone U+2581 word-boundary tokens, so text is rebuilt by concatenating raw pieces then mapping U+2581 → space (reusing Parakeet's per-piece Tokenizer::decode drops spaces).

Changes

  • include/eddy/models/nemotron/nemotron.hpp, src/models/nemotron/nemotron_openvino.cpp — OpenVINO backend (preprocessor / encoder / decoder / joint IR + vocab + metadata).
  • examples/cpp/nemotron_cli.cpp — CLI (--device, --lang, --model-dir).
  • include/eddy/eddy_c.h, src/eddy_c.cppC API for Nemotron (EddyNemotronModel, eddy_nemotron_create/_destroy/_infer_file/_infer_buffer/_free_result), mirroring the Parakeet C API for language bindings.
  • model_configs.hppnemotron-streaming (→ fp16/) and nemotron-streaming-int8 (→ int8/) entries on FluidInference/Nemotron-3.5-ASR-Streaming-Multilingual-0.6b-ov. Adds ModelConfig::repo_subdir so files download from a repo subfolder while staying flat in the local cache.
  • ensure_models.cpp — honor repo_subdir in the download URL (Parakeet unaffected; field defaults to empty). Namespace renamed eddy::parakeeteddy::model_utils (it's a model-agnostic download helper now used by Nemotron too); call sites updated.

Accuracy — FLEURS, full test splits, CPU, greedy, forced language

FP32 IR vs the FluidAudio CoreML reference (FP16 transcripts identical to FP32):

Lang Metric OpenVINO FP32 FluidAudio ref Δ n
en_us WER 11.78 12.09 −0.31 647
es_419 WER 6.99 9.01 −2.02 908
fr_fr WER 12.92 15.18 −2.26 676
cmn_hans_cn CER 21.05 24.54 −3.49 945
ja_jp CER 15.12 16.86 −1.74 650
weighted mixed 13.70 15.79 −2.09 3826

At or below the reference on every language — no conversion regression. Audio-weighted RTFx ≈ 3.66× on a single CPU (Intel Xeon E5-2699 v4, 4 vCPU); higher on Intel NPU / iGPU.

INT8 weight-only variant (nemotron-streaming-int8)

Weight-only INT8 compression of the encoder (per-channel symmetric, data-free — mirrors the CoreML linear_quantize_weights build). The 24 Conformer relative-position projections (self_attn.linear_pos) are kept FP16 (int8-compressing them trips an OpenVINO CPU compile bug and they hold little weight mass); decoder/joint/preprocessor stay FP16. Measured on the same FLEURS splits — directly comparable to the FP32 table:

Lang Metric FP32 INT8-wo Δ
en_us WER 11.78 11.93 +0.15
es_419 WER 6.99 7.05 +0.06
fr_fr WER 12.92 12.99 +0.07
cmn_hans_cn CER 21.05 20.94 −0.11
ja_jp CER 15.12 15.15 +0.03
weighted mixed 13.70 13.73 +0.03

Essentially lossless. Footprint: encoder .bin 732 MB (vs 1.25 GB FP16 / 2.5 GB FP32), peak RSS 2.1 GB (vs 3.9 GB FP16). It's a memory/disk win, not a CPU speed win — on x86 int8 weights decompress to float per-op (RTFx 2.96× vs 3.66×); native int8 speedup applies on Intel NPU. Use int8/ for memory-constrained / NPU deployments, fp16/ for fastest CPU inference.

Verification

  • All targets build clean (eddy, eddy_c, both CLIs, benchmark_fleurs, hf_fetch_models).
  • Nemotron C API verified end-to-end via a C program linking libeddy_c.so against the FP16 IR — transcript matches the CLI/Python reference word-for-word (detected_lang=<en-US>, prompt_id=101).
  • Parakeet regression-checked after the namespace rename: re-downloaded Parakeet-v2 via hf_fetch_models (renamed download_models) and transcribed via parakeet_cli (renamed check_models_available) — correct output, RTFx 4.5×.

Notes

  • FP32, FP16, and INT8 IR are all published in the HF repo (fp32/, fp16/, int8/ subfolders); default download is FP16.
  • openvino_utils intentionally stays under eddy::parakeet — it's coupled to the Parakeet ModelFile type and unused by Nemotron.

🤖 Generated with Claude Code

Adds a new eddy::nemotron backend for NVIDIA's
nemotron-3.5-asr-streaming-0.6b: a cache-aware streaming
FastConformer-RNNT with prompt-conditioned multilingual decoding
(40+ languages). This is distinct from the Parakeet path (stateless
overlapping-chunk encoder + TDT duration decoding) and needs its own
module: cache-aware streaming encoder loop, integer prompt_id language
conditioning, plain RNNT greedy decode, and a dedicated tokenizer
(Nemotron emits standalone U+2581 word-boundary tokens, so text is
rebuilt by concatenating raw pieces then mapping U+2581 -> space).

- include/eddy/models/nemotron/nemotron.hpp, src/models/nemotron/
  nemotron_openvino.cpp: OpenVINO backend (preprocessor/encoder/
  decoder/joint IR + vocab + metadata).
- examples/cpp/nemotron_cli.cpp: CLI (--device, --lang, --model-dir).
- model_configs.hpp: nemotron-streaming entry pointing at
  FluidInference/Nemotron-3.5-ASR-Streaming-Multilingual-0.6b-ov.
  Adds ModelConfig::repo_subdir so files download from the repo's
  fp16/ subfolder (default precision) while staying flat in cache.
- ensure_models.cpp: honor repo_subdir in the download URL.

Accuracy (FLEURS full test splits, FP32 IR, CPU; matches FP16):
en_us WER 11.78, es_419 WER 6.99, fr_fr WER 12.92,
cmn_hans_cn CER 21.05, ja_jp CER 15.12 (weighted 13.70) -- at or
below the FluidAudio CoreML reference on every language. RTFx ~3.66x
single-CPU.

Closes #8
@claude

claude Bot commented Jun 28, 2026

Copy link
Copy Markdown

Code Review — Nemotron Streaming Backend

This PR adds a well-structured cache-aware streaming RNNT backend for NVIDIA's Nemotron-3.5 model. The mel-cache management and greedy RNNT loop are correctly implemented (state persists across chunks, LSTM is not advanced on blank, cache indices are in-bounds). A few issues worth addressing before merge:


🔴 High — audio_length always passes full chunk_samples, not the actual valid sample count

src/models/nemotron/nemotron_openvino.cpp, line 276

I.preproc_req.set_tensor("audio_length", make_i32(static_cast<int>(chunk_samples)));

For the final audio chunk, end - off < chunk_samples — the buffer is zero-padded but audio_length is still the full window size. If the preprocessor gates mel computation on audio_length (the purpose of that input), it treats trailing silence as valid speech and computes spurious mel frames that the encoder decodes, producing garbage tokens at the tail of the transcript.

Fix: pass the actual valid sample count instead:

I.preproc_req.set_tensor("audio_length", make_i32(static_cast<int>(end - off)));

🔴 High — memcpy size taken from input cache, not verified against output cache

src/models/nemotron/nemotron_openvino.cpp, lines 332–333

std::memcpy(cache_channel.data<float>(), cc.data<float>(), cache_channel.get_byte_size());
std::memcpy(cache_time.data<float>(), ctt.data<float>(), cache_time.get_byte_size());

The copy length comes from cache_channel (the input tensor, sized from metadata.json). If the encoder's cache_channel_out / cache_time_out outputs have a different byte size than the input ports — which can happen when the IR is exported with differing static dimensions — this is either a buffer over-read from cc/ctt or a buffer overflow into cache_channel/cache_time, both undefined behaviour.

Fix: add a size assertion before copying:

assert(cc.get_byte_size() == cache_channel.get_byte_size());
assert(ctt.get_byte_size() == cache_time.get_byte_size());

🟡 Medium — hf_fetch_models.cpp error message lists only parakeet-v2

examples/cpp/hf_fetch_models.cpp, line 56

std::cerr << "Available models: parakeet-v2\n";

MODEL_MAP now contains parakeet-v2, parakeet-v3, and nemotron-streaming, but this error path only mentions the first. A user running hf_fetch_models --model nemotron-streaming with a typo gets misleading guidance.

Fix: iterate MODEL_MAP dynamically:

std::cerr << "Available models:";
for (const auto& [k, _] : MODEL_MAP) std::cerr << " " << k;
std::cerr << "\n";

🟡 Medium — shell injection surface widened in ensure_models.cpp

src/utils/ensure_models.cpp, download_single_file

std::string curl_cmd = "curl -L --progress-bar --fail \"" + url + "\" -o \"" + output_path.string() + "\"";
int ret = std::system(curl_cmd.c_str());

The pre-existing std::system call was already fragile; this PR widens the injection surface by adding repo_subdir as a new interpolated component. A " in repo_subdir, repo_id, a filename, or target_dir closes the shell quoting and permits arbitrary command execution. Today these are all compile-time constants ("fp16", "FluidInference/...", etc.), but the function signature accepts any caller-supplied ModelConfig.

The long-term fix is to use libcurl directly. As a short-term guard, validate repo_subdir (and the other fields) against [A-Za-z0-9._/-] before building the command string.


🔵 Low — resolve_prompt_id returns wrong value if called before warmup()

include/eddy/models/nemotron/nemotron.hpp, resolve_prompt_id

The method is public but accesses impl_->prompt_dictionary, which is only populated by ensure_compiled(). If called before transcribe() or warmup(), the dictionary is empty and every language code silently maps to the struct-default 101.

Fix: call ensure_compiled() at the top of resolve_prompt_id, or document the precondition.


Minor notes

  • #include <cmath> is included but unused in nemotron_openvino.cpp.
  • namespace eddy::parakeet in ensure_models.cpp now hosts Nemotron downloads too; consider renaming to eddy::model_utils in a follow-up.
  • blank_idx and vocab_size both default to 13087, making the explicit tok == I.blank_idx check in the token-filter loop redundant (already caught by tok >= I.vocab_size). A brief comment noting that blank intentionally sits at index vocab_size would help future readers.

🤖 Generated with Claude Code

…dening

- nemotron_openvino.cpp: assert cache_*_out byte sizes match the
  pre-allocated input caches before memcpy (mismatched IR export now
  trips here instead of silently over-/under-reading); resolve_prompt_id
  lazily calls the call_once-guarded ensure_compiled() so it is safe to
  call before transcribe()/warmup(); drop unused <cmath>, add <cctype>
  (std::tolower) + <cassert>; comment the blank_idx == vocab_size filter.
- ensure_models.cpp: reject shell-unsafe characters in the curl URL and
  output path before std::system (repo_subdir widened the interpolation
  surface; today all fields are constants but ModelConfig is caller-supplied).
- hf_fetch_models.cpp: list available models dynamically from MODEL_MAP
  instead of the stale "parakeet-v2" literal.

Not changed: the audio_length finding. The CLI passes the full padded
chunk_samples to match the WER-validated Python reference
(transcribe_ov.py pads the final chunk then passes chunk.shape[1]);
switching to the unpadded length would diverge from the validated path.
Alex-Wengg pushed a commit that referenced this pull request Jun 29, 2026
…dening

- nemotron_openvino.cpp: assert cache_*_out byte sizes match the
  pre-allocated input caches before memcpy (mismatched IR export now
  trips here instead of silently over-/under-reading); resolve_prompt_id
  lazily calls the call_once-guarded ensure_compiled() so it is safe to
  call before transcribe()/warmup(); drop unused <cmath>, add <cctype>
  (std::tolower) + <cassert>; comment the blank_idx == vocab_size filter.
- ensure_models.cpp: reject shell-unsafe characters in the curl URL and
  output path before std::system (repo_subdir widened the interpolation
  surface; today all fields are constants but ModelConfig is caller-supplied).
- hf_fetch_models.cpp: list available models dynamically from MODEL_MAP
  instead of the stale "parakeet-v2" literal.

Not changed: the audio_length finding. The CLI passes the full padded
chunk_samples to match the WER-validated Python reference
(transcribe_ov.py pads the final chunk then passes chunk.shape[1]);
switching to the unpadded length would diverge from the validated path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Alex-Wengg

Copy link
Copy Markdown
Member Author

Thanks for the review — addressed in 2420230.

Applied

  • 🔴 memcpy cache size — added assert(cc.get_byte_size() == cache_channel.get_byte_size()) (and cache_time) before the copy-back. In cache-aware streaming the *_out caches are by construction the same fixed shape as the input ring buffers, so this is defensive against a mismatched IR export rather than a live bug; the assert makes that contract explicit.
  • 🟡 hf_fetch_models stale list — now iterates MODEL_MAP dynamically.
  • 🟡 shell-injection surfacedownload_single_file now rejects any URL/output-path containing characters outside [A-Za-z0-9._/:~-] before std::system. (libcurl is the right long-term fix; this closes the repo_subdir/ModelConfig interpolation vector cheaply.)
  • 🔵 resolve_prompt_id before warmup — now calls the call_once-guarded ensure_compiled() at the top (via const_cast, idempotent/cheap), so it's safe standalone instead of silently returning the default 101.
  • Minor — dropped unused <cmath>, added <cctype> (for std::tolower) + <cassert>; commented the blank_idx == vocab_size filter.

Not applied (with reasoning)

  • 🔴 audio_length passes full chunk_samples — this is intentional and matches the WER-validated reference. The Python pipeline (transcribe_ov.py) pads the final chunk to chunk_samples (np.pad) and then passes chunk.shape[1] — i.e. the full padded length — as audio_length. The C++ is a faithful port, and the full multilingual FLEURS run (en/es/fr/zh/ja, full test splits) matched the FluidAudio reference with no tail-garbage regression. Switching to end - off would produce a variable mel-frame count on the final chunk and diverge from the validated path, so I left it as-is.
  • namespace eddy::parakeeteddy::model_utils rename — agreed, but deferring to a follow-up since it touches Parakeet call sites unrelated to this PR.

Build is clean and a sanity transcription still matches FP16/FP32 word-for-word.

@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Code Review — Add NVIDIA Nemotron-3.5-ASR streaming multilingual backend

This PR adds a well-structured, self-contained streaming ASR backend for Nemotron: cache-aware chunked encoder with prompt-conditioned language selection, plain RNNT greedy decode, and a dedicated tokenizer that handles U+2581 word-boundary tokens correctly. The accuracy numbers look solid and the architectural separation from Parakeet is well-justified.

A few issues to address before merging, ordered by severity:


1. is_shell_safe blocks Windows path separators — breaks all downloads on Windows (CONFIRMED)

src/utils/ensure_models.cpp:37

const bool ok = std::isalnum(c) || c == '.' || c == '_' || c == '-' ||
                c == '/' || c == ':' || c == '~';

Backslash (\) is not in the allowlist. On Windows, std::filesystem::path::string() returns native backslash-separated paths (e.g. C:\Users\...\eddy\models\...). So is_shell_safe(output_path.string()) returns false for every file on Windows, and download_single_file returns false before curl is ever called. This silently breaks all downloads — Parakeet and Nemotron alike — on Windows. The bug was introduced by this PR.

Fix: use output_path.generic_string() (always forward-slashes) for the safety check, or add '\\' to the allowlist, or — better — pass the path as a second quoted argument directly without the safety check (the path is already double-quoted in the curl command, so it's safe with spaces and backslashes).


2. assert() for cache tensor size check is compiled out in Release builds (CONFIRMED)

src/models/nemotron/nemotron_openvino.cpp:341

assert(cc.get_byte_size() == cache_channel.get_byte_size());  // <-- stripped in Release
assert(ctt.get_byte_size() == cache_time.get_byte_size());
std::memcpy(cache_channel.data<float>(), cc.data<float>(), cache_channel.get_byte_size());

The project's own claude.md says "Release builds only" — meaning NDEBUG is always defined and both asserts are compiled out. A mismatched model export (different cache shape, future checkpoint) silently causes memcpy to over-read the smaller cc buffer, yielding memory corruption. The comment even names this exact risk ("instead of silently over-/under-reading") but chose assert over if (...) throw.

Fix: replace with if (cc.get_byte_size() != cache_channel.get_byte_size()) throw std::runtime_error(...).

The same applies to the LSTM state copies on lines ~399–400:

std::memcpy(h.data<float>(), I.decoder_req.get_tensor("h_out").data<float>(), h.get_byte_size());
std::memcpy(c.data<float>(), I.decoder_req.get_tensor("c_out").data<float>(), c.get_byte_size());

No size guard at all before copying into fixed-size h/c tensors.


3. audio_length always set to chunk_samples even on the final partial chunk (PLAUSIBLE)

src/models/nemotron/nemotron_openvino.cpp:281

I.preproc_req.set_tensor("audio_length", make_i32(static_cast<int>(chunk_samples)));

The final chunk is zero-padded to chunk_samples, but audio_length is never set to the actual sample count (end - off). The Parakeet sibling (parakeet_preprocessor.cpp) correctly passes samples_to_copy (the real count). If the IR preprocessor uses audio_length to gate its mel output, the final chunk's silence is treated as real audio, potentially emitting spurious tokens at the transcript tail.

Whether this triggers depends on the IR implementation. Worth verifying against the reference Python transcription script with a non-chunk-aligned audio file.


4. ov::Tensor and make_i32(1) allocated inside the innermost RNNT symbol loop (CONFIRMED)

src/models/nemotron/nemotron_openvino.cpp:363, 370

for (size_t sym = 0; sym < I.config.max_symbols_per_frame; ++sym) {
    ov::Tensor token(I.token_et, ov::Shape{1, 1});  // heap alloc every iteration
    // ...
    I.decoder_req.set_tensor("token_length", make_i32(1));  // heap alloc every iteration

enc_step at line 354 is correctly hoisted before the outer t loop — the same treatment should apply here. For long audio (many encoder frames × max_symbols_per_frame), this is tens of thousands of unnecessary allocations per transcription.

Fix: allocate token and a token_length scalar tensor once before the for t < t_enc loop and overwrite their data pointers each iteration.


5. resolve_prompt_id declared const but mutates via const_cast (PLAUSIBLE)

include/eddy/models/nemotron/nemotron.hpp:69 / nemotron_openvino.cpp:133

[[nodiscard]] int resolve_prompt_id(const std::string& language) const;
// ...
int OpenVINONemotron::resolve_prompt_id(...) const {
    const_cast<OpenVINONemotron*>(this)->ensure_compiled();

All current callers use non-const objects so there's no UB today, but the public API declares the method const, allowing callers to invoke it on a const OpenVINONemotron — which would make the const_cast undefined behavior. The cleaner fix: drop const from resolve_prompt_id (it's a lazy-init accessor, not logically const), or mark compile_once and impl_ as mutable.


Minor cleanup

  • src/models/nemotron/nemotron_openvino.cpp:314: mel_cache.data.assign(bins * keep, 0.0f) zero-initialises then immediately overwrites every element in the nested loop below. Use resize or just pre-size once and write directly.
  • examples/cpp/nemotron_cli.cpp:46: The argument parser's else branch silently assigns any unrecognised token to audio_file (last-wins). nemotron_cli audio.wav --langg en-US silently uses "en-US" as the audio file. Worth adding a std::cerr warning for unrecognised -- flags.
  • EDDY_PREPROC_DEVICE env var (nemotron_openvino.cpp inside ensure_compiled): hidden configuration surface not reflected in Config. Consider surfacing as a Config field so it's testable and documented.

🤖 Generated with Claude Code

INT8 weight-only encoder (per-channel symmetric; Conformer relative-pos
projections kept FP16 to satisfy the OV CPU plugin) + FP16 decoder/joint/
preprocessor, served from the "int8" subfolder of the same HF repo.

WER matches FP16/FP32 (en_us 10.99 vs 11.78); ~half the RAM of FP16
(2.1GB vs 3.9GB peak) and ~half the disk (749MB vs 1.3GB). No CPU speed
gain (weights decompress to float on x86) — the win is memory footprint,
chiefly for Intel NPU / memory-constrained deployments.
@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Code Review — Add NVIDIA Nemotron-3.5-ASR streaming multilingual backend

This PR adds a well-structured cache-aware streaming RNNT backend for Nemotron, correctly separating it from the stateless Parakeet path. The mel-cache continuity logic, greedy RNNT decode, and U+2581 detokenization are all sound. The is_shell_safe guard in ensure_models.cpp is a good defensive addition. A few issues need attention before merging.


Bugs

src/models/nemotron/nemotron_openvino.cpp, line 281 — Last chunk sends wrong audio_length to preprocessor

I.preproc_req.set_tensor("audio_length", make_i32(static_cast<int>(chunk_samples)));

chunk_samples is a constant (the full padded buffer size). On the final partial chunk, only end - off samples are real audio; the rest are zeros from memset. Telling the preprocessor the full chunk_samples are valid means it computes mel features over trailing silence, generating extra frames that can produce phantom tokens at the end of the transcript. Fix:

I.preproc_req.set_tensor("audio_length", make_i32(static_cast<int>(end - off)));

src/models/nemotron/nemotron_openvino.cpp, lines 341–342 — Safety asserts compiled out in Release builds

assert(cc.get_byte_size() == cache_channel.get_byte_size());
assert(ctt.get_byte_size() == cache_time.get_byte_size());
std::memcpy(cache_channel.data<float>(), cc.data<float>(), cache_channel.get_byte_size());

Standard assert() is a no-op when NDEBUG is defined (i.e., in every Release build). If the encoder model's output cache shape ever diverges from the pre-allocated input shape — e.g., after a model update or a metadata.json/IR mismatch — the memcpy silently over-reads or over-writes, causing heap corruption. Replace with explicit runtime checks:

if (cc.get_byte_size() != cache_channel.get_byte_size())
    throw std::runtime_error("Cache channel shape mismatch: model IR does not match metadata.json");

examples/cpp/nemotron_cli.cpp, lines 40–47 — Named flags at end-of-args silently overwrite audio_file

} else if (a == "--device" && i + 1 < argc) {
    device = argv[++i];
} else {
    audio_file = a;   // catches --device, --lang, --model-dir when they are the last argument
}

If a named option appears without a value (e.g., nemotron_cli audio.wav --device), the i + 1 < argc guard fails and the flag string "--device" falls into the else branch, overwriting audio_file. The subsequent read_wav("--device") throws a confusing file-not-found error. Add an error branch when a known flag appears without a value:

} else if (a == "--device" || a == "--lang" || a == "--model-dir") {
    std::cerr << "Error: " << a << " requires an argument\n";
    return 1;
}

Design concern

src/models/nemotron/nemotron_openvino.cpp, line 133 — const_cast in resolve_prompt_id

int OpenVINONemotron::resolve_prompt_id(const std::string& language) const {
    const_cast<OpenVINONemotron*>(this)->ensure_compiled();

resolve_prompt_id is declared const but modifies impl_ through ensure_compiled(). The const_cast is undefined behaviour if the object was ever constructed as const. In practice it's safe here because Impl is behind a unique_ptr (const on the pointer, not the pointee), but the intent is clearer if resolve_prompt_id is simply declared non-const — it genuinely does lazy initialisation. Alternatively, mark compile_once as mutable.


Efficiency (hot path)

src/models/nemotron/nemotron_openvino.cpp, lines 363–370 — Unnecessary heap allocations inside the RNNT inner loop

make_i32(1) (line 370) and the ov::Tensor token construction (line 363) each allocate memory on every iteration of the innermost sym loop. Both values are invariant across the entire transcribe() call — token_length is always 1, and token_et is fixed after compilation. Pre-allocate them once before the outer while loop (as prompt_tensor already is) and just update the data pointer in the loop:

// Before the while loop:
ov::Tensor token_len_tensor = make_i32(1);
ov::Tensor token_tensor(I.token_et, ov::Shape{1, 1});

// Inside the sym loop, replace make_i32(1) with token_len_tensor
// and write directly: token_tensor.data<int32_t>()[0] = last_token;

Similarly, make_i32(static_cast<int>(chunk_samples)) and make_i32(static_cast<int>(total)) are called every chunk with constant values — hoist them alongside prompt_tensor.


Minor usability gap

examples/cpp/nemotron_cli.cpp — No way to select INT8 weights via the CLI

NEMOTRON_STREAMING_INT8 is correctly registered in MODEL_MAP and hf_fetch_models can download it, but nemotron_cli hardcodes "nemotron-streaming" for both the asset dir and the OpenVINO cache dir. Users who download INT8 weights must also pass --model-dir to point at the right directory. A --model <name> flag that mirrors hf_fetch_models's lookup would make the INT8 path discoverable without manual path wrangling.


Reviewed with Claude Code

@Alex-Wengg

Copy link
Copy Markdown
Member Author

INT8 weight-only variant validated ✅

Added a nemotron-streaming-int8 model config (commit 7af285c) pointing at the int8/ subfolder of the same HF repo. The INT8 IR is now uploaded and the model card carries the full comparison.

Approach: weight-only INT8 of the encoder only (per-channel symmetric, data-free — mirrors the CoreML linear_quantize_weights build). Activations stay FP16; decoder/joint/preprocessor stay FP16. The 24 Conformer relative-position projections (self_attn.linear_pos) are kept FP16 — int8-compressing them trips an OpenVINO CPU compile bug and they hold little weight mass. An earlier full-PTQ attempt (which also quantizes activations) cost +6.94 WER on English, so weight-only is the right call.

Accuracy — FLEURS full test splits, forced language, greedy (directly comparable to the FP32 table):

Lang Metric FP32 INT8-wo Δ n
en_us WER 11.78 11.93 +0.15 647
es_419 WER 6.99 7.05 +0.06 908
fr_fr WER 12.92 12.99 +0.07 676
cmn_hans_cn CER 21.05 20.94 −0.11 945
ja_jp CER 15.12 15.15 +0.03 650
weighted mixed 13.70 13.73 +0.03 3826

Essentially lossless (largest swing +0.15 WER, within noise; Chinese improves slightly).

Footprint: encoder .bin 732 MB (vs 1.25 GB FP16 / 2.5 GB FP32); peak RSS 2.1 GB (vs 3.9 GB FP16). It is a memory/disk win, not a CPU speed win — on x86 the int8 weights decompress to float per-op (RTFx 2.96× vs 3.66× FP16); native int8 speedup applies on Intel NPU. Use int8/ for memory-constrained / NPU deployments, fp16/ for fastest CPU inference.

Wire the Nemotron streaming backend through the eddy_c C API (previously
CLI-only), mirroring the Parakeet C API:
- EddyNemotronModel handle + EddyNemotronConfig (device/model_dir/language)
- EddyNemotronResult (text, detected_language, token_ids, prompt_id_used,
  latency_ms) with eddy_nemotron_free_result
- eddy_nemotron_create / _destroy / _infer_file / _infer_buffer
model_dir defaults to the "nemotron-streaming" Eddy cache; language is fixed
at creation (recreate to change). Verified end-to-end against the FP16 IR:
transcript matches the CLI/Python reference word-for-word.

Rename eddy::parakeet -> eddy::model_utils for the model-download helper
(ensure_models.{hpp,cpp}): it is model-agnostic (operates on any
eddy::ModelConfig) and already serves Nemotron downloads, so the
model-specific namespace was misleading. Updated all call sites
(parakeet_cli, benchmark_fleurs, hf_fetch_models, eddy_c). openvino_utils
stays under eddy::parakeet — it is coupled to the Parakeet ModelFile type
and unused by Nemotron. Addresses the follow-up noted in the PR #10 review.
Alex-Wengg pushed a commit that referenced this pull request Jun 29, 2026
Wire the Nemotron streaming backend through the eddy_c C API (previously
CLI-only), mirroring the Parakeet C API:
- EddyNemotronModel handle + EddyNemotronConfig (device/model_dir/language)
- EddyNemotronResult (text, detected_language, token_ids, prompt_id_used,
  latency_ms) with eddy_nemotron_free_result
- eddy_nemotron_create / _destroy / _infer_file / _infer_buffer
model_dir defaults to the "nemotron-streaming" Eddy cache; language is fixed
at creation (recreate to change). Verified end-to-end against the FP16 IR:
transcript matches the CLI/Python reference word-for-word.

Rename eddy::parakeet -> eddy::model_utils for the model-download helper
(ensure_models.{hpp,cpp}): it is model-agnostic (operates on any
eddy::ModelConfig) and already serves Nemotron downloads, so the
model-specific namespace was misleading. Updated all call sites
(parakeet_cli, benchmark_fleurs, hf_fetch_models, eddy_c). openvino_utils
stays under eddy::parakeet — it is coupled to the Parakeet ModelFile type
and unused by Nemotron. Addresses the follow-up noted in the PR #10 review.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Alex-Wengg

Copy link
Copy Markdown
Member Author

Follow-ups addressed (commit abe3a6d)

1. Nemotron C-API wiring — Nemotron is now exposed through eddy_c (was CLI-only), mirroring the Parakeet C API:

  • EddyNemotronModel handle + EddyNemotronConfig (device / model_dir / language)
  • EddyNemotronResult (text, detected_language, token_ids, prompt_id_used, latency_ms) + eddy_nemotron_free_result
  • eddy_nemotron_create / _destroy / _infer_file / _infer_buffer

model_dir defaults to the nemotron-streaming Eddy cache (NULL/"cache"); language is fixed at creation. Verified end-to-end via a small C program linking libeddy_c.so against the FP16 IR — transcript matches the CLI/Python reference word-for-word (detected_lang=<en-US>, prompt_id=101).

2. eddy::parakeeteddy::model_utils rename — done for the model-download helper (ensure_models.{hpp,cpp}), the one flagged in the review as hosting Nemotron downloads. It's model-agnostic (operates on any eddy::ModelConfig), so the model-specific namespace was misleading. All call sites updated (parakeet_cli, benchmark_fleurs, hf_fetch_models, eddy_c).

Scoping note: openvino_utils stays under eddy::parakeet — it's coupled to the Parakeet ModelFile type and isn't used by Nemotron (which goes through OpenVINOBackend), so moving it would pull a Parakeet type into the shared namespace. Happy to revisit if you'd prefer a fully neutral utils namespace.

All targets build clean (eddy, eddy_c, both CLIs, benchmark_fleurs, hf_fetch_models).

@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Code Review — Add NVIDIA Nemotron-3.5-ASR streaming multilingual backend

This PR adds a well-structured cache-aware streaming RNNT backend for Nemotron. The architecture decision to give it its own module (rather than overloading the Parakeet TDT path) is sound, and the namespace rename from eddy::parakeet to eddy::model_utils for the download utilities is a good generalization. The accuracy numbers are solid. A few issues worth addressing before merge:


Correctness

1. eddy_nemotron_infer_file returns wrong error code for non-file errorssrc/eddy_c.cpp

} catch (const std::exception& e) {
    if (err) *err = capture_exception(e);
    return EDDY_ERROR_FILE_NOT_FOUND;  // ← wrong for format/permission errors
}

read_wav throws std::runtime_error for at least 7 distinct conditions (wrong channel count, negative frame count, partial read, bad sample rate, resample failure, etc.) — none of which are "file not found". All get misreported as EDDY_ERROR_FILE_NOT_FOUND, sending callers down the wrong recovery path. The infer_buffer equivalent correctly uses EDDY_ERROR_INFERENCE_FAILED. Suggest returning EDDY_ERROR_INFERENCE_FAILED (or a new EDDY_ERROR_AUDIO_FORMAT) for non-filesystem exceptions.


2. audio_length always set to chunk_samples on the last partial chunksrc/models/nemotron/nemotron_openvino.cpp

I.preproc_req.set_tensor("audio_length", make_i32(static_cast<int>(chunk_samples)));

On the final chunk where end - off < chunk_samples (i.e., virtually all real audio files), the preprocessor receives the full-chunk size instead of the actual valid sample count. The audio buffer is correctly zero-padded to chunk_samples, but audio_length is the signal the model uses to gate valid mel frames. If the preprocessor IR uses this input to limit output, the silence-padded tail produces spurious mel frames that flow into the encoder, potentially emitting ghost tokens at the end of every transcript. The fix is make_i32(static_cast<int>(end - off)). This warrants verification against the Python export script (nemotron-ov-export/transcribe_ov.py) to confirm whether the IR uses audio_length dynamically or ignores it.


3. nemotron_fill_result leaks on OOMsrc/eddy_c.cpp

out->text = copy_string(res.text);                          // alloc 1
out->detected_language = copy_string(res.detected_language); // alloc 2
// ...
out->token_ids = new int[out->num_tokens];                  // throws → leak allocs 1 & 2

copy_string uses new[] (throws on OOM, never returns nullptr). If the token_ids allocation throws, text and detected_language are written to out but the callers' catch blocks return an error code without calling eddy_nemotron_free_result — both strings leak. The eddy_whisper_transcribe_file path in the same file uses a nested try/catch with explicit free-and-rethrow as the correct pattern. Either adopt RAII (std::unique_ptr<char[]>) inside the fill function or zero-initialize out before the call and call eddy_nemotron_free_result in the catch block.


API Consistency

4. C API null device defaults to "CPU", C++ Config defaults to "AUTO"src/eddy_c.cpp / include/eddy/models/nemotron/nemotron.hpp

// eddy_c.cpp
const std::string device = config.device ? config.device : "CPU";
// nemotron.hpp
std::string device = "AUTO";

A C caller who zero-initializes EddyNemotronConfig gets CPU-only inference. A C++ caller with a default-constructed Config gets device auto-selection. On a machine with an Intel NPU or iGPU, this is a meaningful performance difference. One of the two defaults should be changed to match the other. If CPU is the safe choice for first-time users, update the C++ struct to match.


Performance

5. ov::Tensor token allocated in the innermost RNNT symbol loopsrc/models/nemotron/nemotron_openvino.cpp

for (size_t t = 0; t < t_enc; ++t) {
    for (size_t sym = 0; sym < I.config.max_symbols_per_frame; ++sym) {
        ov::Tensor token(I.token_et, ov::Shape{1, 1});  // ← shape never changes

This is the hottest allocation in the entire pipeline — up to t_enc × max_symbols_per_frame allocations per chunk. The shape {1, 1} and element type I.token_et are invariant for the lifetime of the model. Hoist the allocation above the outer t loop and just overwrite the scalar data in-place before each decoder_req call.

Similarly, ov::Tensor audio ({1, chunk_samples}) and mel_in ({1, bins, total}) are re-allocated on every chunk even though their shapes are fixed after ensure_compiled(). Hoist both above the while loop.


Design

6. const_cast<this> in resolve_prompt_idsrc/models/nemotron/nemotron_openvino.cpp

int OpenVINONemotron::resolve_prompt_id(const std::string& language) const {
    const_cast<OpenVINONemotron*>(this)->ensure_compiled();

The method is publicly declared [[nodiscard]] … const, so any caller holding a const OpenVINONemotron& or a const-stored instance could invoke it — at which point the const_cast is undefined behavior per [dcl.type.cv]/4. The idiomatic fix is to mark compile_once and the model/metadata members in Impl as mutable and declare ensure_compiled() const. No current call site in the repo triggers the UB, but the public API makes it reachable.


Minor

  • std::string(config.model_dir) is constructed twice in eddy_nemotron_create's if condition (src/eddy_c.cpp). Hoist to a single std::string md = config.model_dir ? config.model_dir : "";.
  • EddyNemotronResult::token_ids is missing the "must be freed with eddy_nemotron_free_result" annotation that EddyParakeetResult::token_ids has (include/eddy/eddy_c.h). Binding authors reading only the struct comments won't know how it's allocated.

🤖 Generated with Claude Code

…C-API hardening

Encoder/decoder (nemotron_openvino.cpp):
- Replace assert() cache-size checks with runtime `throw` (Release builds
  define NDEBUG, so the asserts were compiled out); add the same guard for the
  decoder h_out/c_out LSTM state copies.
- Hoist the per-chunk (audio, mel_in) and inner-loop (token, token_length,
  mel_length, audio_length) tensors above the loops — they have fixed
  shape/value for the whole call; the inner token/token_length allocs were the
  hottest path. Drop now-unused <cassert>.
- Make ensure_compiled() const so resolve_prompt_id() (const) can lazily
  compile without the const_cast UB; document why it's safe (only mutates
  *impl_ through the unique_ptr).
- Keep audio_length = full padded chunk_samples (matches the WER-validated
  transcribe_ov.py), now with a comment explaining why end-off would diverge.

C API (eddy_c.cpp / eddy_c.h):
- eddy_nemotron_infer_file: only a genuinely missing file returns
  FILE_NOT_FOUND; read_wav format/decode errors now return INFERENCE_FAILED.
- eddy_nemotron_infer_buffer: zero-init *out and free on catch so a throw
  mid-fill can't leak the already-allocated strings.
- Hoist the doubly-constructed model_dir string; annotate token_ids ownership.

CLI (nemotron_cli.cpp):
- Add --model <name> to pick the cache variant (e.g. nemotron-streaming-int8)
  without --model-dir wrangling.
- Error (not silently swallow as the audio path) when a known flag is given
  without a value, and reject unknown -options.

Align nemotron::Config default device to "CPU" to match the C API and CLI.

Builds clean; FP16/INT8 transcripts match the reference word-for-word.
@Alex-Wengg
Alex-Wengg force-pushed the add-nemotron-streaming-support branch from abe3a6d to df37fa3 Compare June 29, 2026 13:23
@Alex-Wengg

Copy link
Copy Markdown
Member Author

Addressed the latest review rounds (force-pushed; final fix commit df37fa3).

Bugs

  • Release-unsafe assert on cache size — replaced with a runtime throw (Release builds define NDEBUG, so the asserts were compiled out). Added the same byte-size guard before the decoder h_out/c_out LSTM state copies, which previously had none.
  • infer_file error code — only a genuinely missing file now returns FILE_NOT_FOUND; read_wav format/decode errors return INFERENCE_FAILED.
  • nemotron_fill_result OOM leak*out is zero-initialized and freed via eddy_nemotron_free_result on catch, so a throw mid-fill can't leak the already-allocated strings.
  • CLI flags at end-of-args — a known flag without a value now errors instead of being swallowed as the audio path; unknown -options are rejected.

Design

  • const_cast in resolve_prompt_id — fixed at the root: ensure_compiled() is now const (it only mutates *impl_ through the unique_ptr, and is call_once-guarded), so the const method compiles lazily with no const_cast/UB.
  • Device default mismatchnemotron::Config::device now defaults to "CPU", matching the C API and CLI.

Performance

  • Hot-loop allocations hoistedtoken/token_length (innermost RNNT loop) plus audio, mel_in, mel_length, audio_length are now allocated once and reused. Latency on the sample dropped ~2235→2021 ms.

Usability

  • Added nemotron_cli --model <name> to select the cache variant (e.g. nemotron-streaming-int8) without manual --model-dir.

Minor — hoisted the doubly-constructed model_dir string; annotated token_ids ownership in the header; dropped the now-unused <cassert>.

Not changed (intentional): audio_length still passes the full padded chunk_samples. This is a faithful port of the WER-validated transcribe_ov.py, which np.pads the final chunk and passes chunk.shape[1] (== chunk_samples); the preprocessor IR doesn't gate mel output on it, so end-off would diverge from the validated path without changing output. Added a code comment to that effect so it stops getting re-flagged.

All targets build clean. FP16 and INT8 transcripts still match the reference word-for-word; Parakeet re-verified end-to-end after the namespace rename.

@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Code Review — Add NVIDIA Nemotron-3.5-ASR streaming multilingual backend

This PR adds a well-structured new backend for the Nemotron cache-aware streaming RNNT model: clean separation into its own eddy::nemotron namespace, a proper PIMPL implementation with std::call_once compilation guarding, a sound cache-carry loop that matches the validated Python reference, and a mirrored C API. The accuracy numbers and the namespace rename (eddy::parakeeteddy::model_utils for the download helpers) are both clean. Below are findings in severity order.


Bug — *out left uninitialized on EDDY_ERROR_FILE_NOT_FOUND return

src/eddy_c.cpp ~line 621

eddy_nemotron_infer_file validates out != nullptr but returns EDDY_ERROR_FILE_NOT_FOUND without ever touching *out:

if (!std::filesystem::exists(wav_path, ec)) {
    if (err) *err = copy_string("...");
    return EDDY_ERROR_FILE_NOT_FOUND;   // ← *out never written
}
// Only the success path (via eddy_nemotron_infer_buffer) does: *out = EddyNemotronResult{};

eddy_nemotron_free_result unconditionally delete[]s result->text, ->detected_language, and ->token_ids. A caller that passes a stack-allocated or previously-used EddyNemotronResult and then calls eddy_nemotron_free_result after receiving FILE_NOT_FOUND will invoke delete[] on garbage pointers — undefined behaviour and almost certainly a crash.

The fix is one line before the early return: *out = EddyNemotronResult{};


Usability gap — eddy_nemotron_create doesn't validate model files at creation time

src/eddy_c.cpp ~line 532

eddy_parakeet_create calls eddy::model_utils::check_models_available(model_dir, &err) immediately, so a missing model produces a structured error at the create call. eddy_nemotron_create skips this: it always returns a non-null handle, and the first failure only surfaces inside ensure_compiled() on the initial transcribe() or warmup() call as a std::runtime_error from std::ifstream. A real-time audio pipeline calling create during startup and transcribe later will get a confusing deferred failure.

The check is already available via eddy::model_utils::check_models_available; wiring it in makes the handle fail-fast and matches the Parakeet API contract.


Diagnostics gap — m.at() for cache shapes gives no context on failure

src/models/nemotron/nemotron_openvino.cpp ~line 181

Every other metadata field uses m.value(key, default) for graceful fallback. The two cache shapes are the only exceptions:

impl_->cache_channel_shape = to_shape(m.at("cache_channel_shape"));
impl_->cache_time_shape    = to_shape(m.at("cache_time_shape"));

If either key is absent from metadata.json, nlohmann::json::out_of_range propagates with only "key 'cache_channel_shape' not found" — no model name, no file path. The !f.good() guard above provides a helpful path-based message, but a truncated or hand-edited metadata.json that opens fine but is missing these keys gives a cryptic, unactionable error.

Replacing with an explicit check and a descriptive std::runtime_error (or wrapping the block in a try/catch that adds path context) costs two lines and makes model-export issues diagnosable in the field.


Efficiency — mel_cache.data.assign(bins * keep, 0.0f) initializes memory that is immediately overwritten

src/models/nemotron/nemotron_openvino.cpp ~line 330

mel_cache.data.assign(bins * keep, 0.0f);   // zero-fills all bins*keep elements
for (size_t bin = 0; bin < bins; ++bin) {
    for (size_t t = 0; t < keep; ++t) {
        mel_cache.data[bin * keep + t] = mel_src[...];  // overwrites all bins*keep elements
    }
}

The index bin * keep + t for bin ∈ [0, bins), t ∈ [0, keep) is a bijection over [0, bins*keep) — every element is unconditionally overwritten. The assign() zero-write is pure waste (writing the buffer twice per chunk). resize() avoids the redundant pass on what is the mel-cache hot buffer updated every chunk.


Cleanup — chunk_samples() computes an integer via floating-point arithmetic

src/models/nemotron/nemotron_openvino.cpp (struct Impl::chunk_samples)

size_t chunk_samples() const {
    return static_cast<size_t>(static_cast<double>(chunk_mel_frames) * 0.01 * sample_rate);
}

0.01 is not exactly representable in IEEE 754, introducing a rounding hazard. For the hardcoded defaults (chunk_mel_frames=112, sample_rate=16000) the FP result is coincidentally exact, but this is fragile against non-standard metadata values. The intent is integer division:

return static_cast<size_t>(chunk_mel_frames) * static_cast<size_t>(sample_rate) / 100;

Security hygiene — is_shell_safe allows : and ~ in a command-injection guard

src/utils/ensure_models.cpp ~line 24

The function gates a std::system() curl invocation, but its allowlist includes both : and ~. Both characters are currently harmless because : is needed for https:// and ~ is inside double-quoted shell arguments. However, admitting them makes the guard weaker than it could be — a future caller that interpolates the validated string outside double quotes could be surprised. The minimal hardening is checking url and the path component separately (allowing : only in the URL scheme position) or, better, replacing the std::system shell-out with a direct libcurl call and eliminating this class of concern.


Conventions — CLAUDE.md guidance is now stale

claude.md (repo root)

The file currently reads: "NOT building streaming yet — focus is on batch processing of complete audio files." This PR introduces a cache-aware streaming backend as a first-class feature. The guidance was written for the Parakeet-only era; it should be updated to reflect that streaming (Nemotron) and batch (Parakeet) are both supported paths.


Review generated with Claude Code

…cache hardening

- eddy_nemotron_infer_file: zero-init *out before FILE_NOT_FOUND return so a
  caller-then-free_result does not delete[] uninitialized pointers.
- eddy_nemotron_create: fail fast via check_models_available(NEMOTRON_FILES),
  matching the eddy_parakeet_create contract instead of deferring to first infer.
- nemotron metadata: require cache_channel_shape/cache_time_shape explicitly with
  a path-aware error rather than letting nlohmann's bare key-not-found propagate.
- mel_cache: resize instead of assign (every element is overwritten; drop the
  redundant per-chunk zero-fill).
- chunk_samples(): integer frames*rate/100, avoiding the 0.01 FP rounding hazard.
- ensure_models: split is_shell_safe into tight is_url_safe / is_path_safe; URL
  charset drops '~', path charset adds native '\' and space (fixes Windows paths).
- CLAUDE.md: document both Parakeet/batch and Nemotron/streaming as supported.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Code Review — Add NVIDIA Nemotron-3.5-ASR streaming multilingual backend

This is a well-structured PR: the Nemotron backend is cleanly separated from Parakeet (justified by the fundamental difference in streaming vs. stateless chunking), the C API mirrors the Parakeet pattern, and the repo_subdir extension to ModelConfig/ensure_models is minimal and non-breaking for existing callers. The accuracy numbers are solid. Three issues found, ranked by severity.


1. BUG — eddy_nemotron_infer_file: *out uninitialized when read_wav throws (src/eddy_c.cpp)

eddy_nemotron_infer_buffer correctly zero-inits *out before its try block:

*out = EddyNemotronResult{};   // ✅ safe
try { ... }

eddy_nemotron_infer_file does not. The only zero-init is inside the early FILE_NOT_FOUND return:

if (!std::filesystem::exists(wav_path, ec)) {
    *out = EddyNemotronResult{};   // only this path zero-inits
    ...
}
try {
    auto pcm = eddy::audio::read_wav(wav_path);          // can throw (corrupt WAV, wrong channels, etc.)
    return eddy_nemotron_infer_buffer(..., out, err);
} catch (const std::exception& e) {
    if (err) *err = capture_exception(e);
    return EDDY_ERROR_INFERENCE_FAILED;                  // *out is garbage here
}

If read_wav throws, both catch blocks return an error code but leave *out uninitialised. A caller that then calls eddy_nemotron_free_result(out) will delete[] garbage pointers — heap corruption or crash.

Fix: add *out = EddyNemotronResult{}; immediately before the try block (mirroring eddy_nemotron_infer_buffer). Note: the pre-existing eddy_parakeet_infer_file has the same structural gap, so the same fix should be applied there too.


2. CODE CLARITY — ensure_compiled() const mutates *impl_ without mutable (include/eddy/models/nemotron/nemotron.hpp)

// header
void ensure_compiled() const;
std::unique_ptr<Impl> impl_;   // not mutable

This compiles and runs correctly today — const unique_ptr<Impl> doesn't transitively const-qualify *impl_, so calling non-const methods on *impl_ inside a const method is legal C++. The comment in the header acknowledges this.

The concern is forward maintainability: if Pimpl is ever refactored to a value member (Impl impl_ instead of unique_ptr<Impl> impl_), the compiler would silently accept the struct but the mutation in const methods would become ill-formed — a latent correctness trap. Marking impl_ as mutable makes the intent explicit and future-proofs the design:

mutable std::unique_ptr<Impl> impl_;

3. STALE COMMENT — INT8 WER number in model_configs.hpp disagrees with the PR table (include/eddy/core/model_configs.hpp, lines 69–70)

The inline comment for NEMOTRON_STREAMING_INT8 reads:

// WER matches FP16/FP32 (en_us 10.99 vs 11.78)

The PR description's INT8 accuracy table shows en_us WER: **11.93** (vs. FP32 baseline 11.78) — a slight regression, not an improvement to 10.99. The two numbers are ~0.94 WER apart and have opposite signs relative to baseline, so they can't both be right. Looks like a copy-paste from an earlier measurement. Please update the comment to match the table (or vice versa).


Overall the implementation is solid — the cache-aware streaming loop, RNNT greedy decode, and charset validation in ensure_models are all well-reasoned. The three items above are the only actionable findings.

Preprocessor: replace the dynamic-shape nemotron_preprocessor.xml with a
native C++ log-mel featurizer (MelFeaturizer) reproducing NeMo's
AudioToMelSpectrogramPreprocessor exactly (preemph 0.97 -> center-pad ->
Hann-windowed 512-pt STFT via a self-contained radix-2 FFT -> power -> slaney
mel -> log(x+2^-24)). Reverse-engineered from the IR (validated to ~1e-5 on
window/filterbank constants and <=0.14% WER end-to-end over 1050 FLEURS files).
A startup guard verifies the featurizer's frame geometry matches the model's
chunk_mel_frames. nemotron_preprocessor.xml/.bin dropped from NEMOTRON_FILES.

NPU: the OpenVINO NPU plugin miscompiles BitwiseNot on a boolean (it does an
integer complement, so ~0=-1 and ~1=-2 are both 'true'), making the
FastConformer attention mask all-true -> uniform softmax -> encoder output
collapses to ~0 -> empty transcripts. Rewrite BitwiseNot(bool) -> LogicalNot in
the encoder IR before compiling (no-op on CPU/GPU). Nemotron now runs correctly
on NPU at WER/CER parity with CPU and ~2x the throughput (RTFx ~22x vs ~11x).

Adds benchmark_nemotron_fleurs (FLEURS WER/CER/RTFx harness, UTF-8/CJK-aware CER)
used to validate the above across en/es/fr/zh/ja on CPU and NPU.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Code Review — PR #10: Nemotron-3.5-ASR streaming multilingual backend

This PR adds a cache-aware streaming RNNT backend (eddy::nemotron) for NVIDIA's Nemotron-3.5-ASR model, including a native C++ log-mel featurizer, a C API mirroring the existing Parakeet API, two CLI/benchmark tools, and model-config entries for FP16 and INT8 variants. The architecture is well-justified, the NPU BitwiseNot→LogicalNot rewrite is a thoughtful fix for an OpenVINO plugin bug, and the featurizer geometry probe-check at compile time is a nice defensive touch.

Three issues worth addressing before merge:


1. eddy_nemotron_infer_file*out uninitialized when read_wav throws

File: src/eddy_c.cpp, eddy_nemotron_infer_file
Severity: High — undefined behaviour / crash in callers

*out is zero-initialized only in the FILE_NOT_FOUND early-return path. If read_wav throws for a non-filesystem reason (wrong channel count, unsupported codec, bad sample rate), the outer catch returns EDDY_ERROR_INFERENCE_FAILED without ever touching *out. A caller who then does:

EddyNemotronResult result;   // stack garbage
EddyError err = eddy_nemotron_infer_file(model, "stereo.wav", &result, &errmsg);
if (err != EDDY_OK) eddy_nemotron_free_result(&result);  // delete[] garbage → crash

will free garbage pointers. The comment in the FILE_NOT_FOUND branch even calls out this exact concern, which makes the gap in the try-path more surprising.

eddy_nemotron_infer_buffer handles this correctly: it does *out = EddyNemotronResult{} unconditionally before the try block and calls eddy_nemotron_free_result(out) in every catch arm.

Fix: add *out = EddyNemotronResult{}; just before the try block, and add eddy_nemotron_free_result(out); in the catch arms — mirroring infer_buffer exactly.


2. charset_ok — trailing backslash breaks cmd.exe quoting on Windows

File: src/utils/ensure_models.cpp, charset_ok / download_single_file
Severity: Medium — injection guard can be defeated on Windows

The is_path_safe allowlist includes '\\' to support Windows native paths. The download command embeds the output path inside double-quotes:

curl ... -o "C:\some\path\file.bin"

On POSIX (sh), \" inside double quotes is an escaped quote, so \ followed by " is safe. On Windows (cmd.exe), \" is not a recognized escape sequence — \" ends the quoted string at the ". A path ending with a backslash (e.g., fs::path(dir) / filename where dir itself has a trailing separator) produces "C:\eddy\models\" in the shell, which leaves the rest of the line unquoted.

All current ModelConfig values are compile-time constants with well-formed paths, so there is no current exploit. But the purpose of charset_ok is precisely to guard against future callers passing runtime configs. A trailing-backslash path slips through the check and defeats the guard.

Fix (short-term): add path.lexically_normal() (strips trailing separators) before calling output_path.string(), or explicitly reject trailing backslashes in is_path_safe. Fix (long-term): the comment itself identifies the right fix — drop std::system for a direct libcurl call.


3. n_freq_ initialized from a literal instead of n_fft_ / 2 + 1

File: src/models/nemotron/nemotron_featurizer.cpp, constructor initializer list
Severity: Low — latent maintenance hazard

n_fft_(512),
n_freq_(512 / 2 + 1),   // ← literal, not n_fft_ / 2 + 1

n_freq_ is documented in the header as n_fft_/2 + 1. Using the literal decouples the two fields. If n_fft_ is ever changed (even just the initializer integer), n_freq_ will silently disagree: the mel filterbank will be built with the wrong number of bins, the power-spectrum loop will read the wrong number of FFT outputs, and the matrix multiply will access out-of-bounds memory. The constructor-initializer ordering (n_fft_ is declared before n_freq_ in the header) guarantees n_fft_ is already initialized when n_freq_ is evaluated, so using it is both safe and idiomatic.

Fix: n_freq_(n_fft_ / 2 + 1).


Minor notes (no action required)

  • Namespace rename: eddy::parakeeteddy::model_utils in ensure_models.hpp is a breaking C++ API change. All in-tree call sites are updated. If any downstream consumers call these functions directly (they're in a public header), they'll get a compile error. Worth a note in the release changelog.

  • WER/CER utilities in benchmark_nemotron_fleurs.cpp: the UTF-8-aware normalize_text / levenshtein / wer / cer are intentionally different from the byte-wise versions in benchmark_fleurs.cpp (the multilingual CER case requires codepoint-level handling). The duplication is justified, but benchmark_fleurs.cpp's older O(m×n) Levenshtein allocation could be updated to use the same rolling two-row approach introduced here.


Overall the implementation is thorough and well-documented. Items 1 and 2 are the blocking issues.

🤖 Generated with Claude Code

Alex-Wengg and others added 2 commits June 29, 2026 16:28
… + Unicode punct strip)

Match FluidAudio's NemotronMultilingualFleursBenchmark / WERCalculator: route
CJK languages (ja/ko/zh/cmn/yue/th/lo) through character-level CER (whitespace
WER is meaningless without word segmentation; report WER=CER), and strip Unicode
Mark/Symbol/Punctuation (approx category M/S/P over the blocks that occur in
FLEURS) to a space in normalization instead of keeping non-ASCII punctuation.

Brings CJK CER onto the published reference: ja 19.0 -> 15.46 (ref 15.15),
zh 21.78 -> 20.18 (ref 20.94). en/es/fr unaffected (ASCII) or marginally
improved (European punctuation now stripped).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…EURS)

5-language FLEURS results (en/es/fr WER, zh/ja CER) for nemotron-streaming-int8 on
Intel NPU at ~22-24x RTFx, scored with the FluidAudio methodology. Matches the
OpenVINO FP32 reference and beats the FluidAudio CoreML reference on es/fr/zh/ja.
Documents the NPU BitwiseNot->LogicalNot enablement fix and the native C++ mel
preprocessor.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Code Review — Add NVIDIA Nemotron-3.5-ASR Streaming Multilingual Backend

This PR adds a well-structured, cache-aware streaming ASR backend for Nemotron. The NPU BitwiseNot→LogicalNot rewrite, the native C++ featurizer with geometry validation at init time, and the meticulous cache copy-back checks (encoder caches, LSTM state) all show careful attention to correctness. The accuracy numbers are compelling.

Three confirmed bugs and two plausible issues need attention before merge.


🔴 Confirmed bugs

1. src/eddy_c.cppeddy_nemotron_infer_file leaves *out uninitialized when read_wav throws

When the file exists but is malformed (stereo, unsupported format, wrong sample rate in the WAV header), read_wav throws. Both catch handlers set *err and return EDDY_ERROR_INFERENCE_FAILED, but never touch *out. A caller who then calls eddy_nemotron_free_result(&out) on an uninitialized EddyNemotronResult will delete[] garbage pointers — UB / crash.

eddy_nemotron_infer_buffer already gets this right: it does *out = EddyNemotronResult{} before the try block (line 618). The file path is missing that step.

// src/eddy_c.cpp — fix: add before the try block
try {
    *out = EddyNemotronResult{};          // ← add this
    auto pcm = eddy::audio::read_wav(wav_path);
    return eddy_nemotron_infer_buffer(handle, pcm.data(), pcm.size(), 16000, out, err);
} catch (const std::exception& e) {
    eddy_nemotron_free_result(out);       // ← and add this (matches infer_buffer pattern)
    if (err) *err = capture_exception(e);
    return EDDY_ERROR_INFERENCE_FAILED;
}

2. src/eddy_c.cpp line 612 — eddy_nemotron_infer_buffer zeroes *out after the sample_rate guard

The zero-init (*out = EddyNemotronResult{}) is at line 618, after the sample_rate != 16000 check at line 612. So passing, say, an 8 kHz buffer returns EDDY_ERROR_INVALID_ARGUMENT with *out still holding uninitialized garbage. Same crash path as #1.

// Current order:
if (sample_rate != 16000) { return EDDY_ERROR_INVALID_ARGUMENT; }  // *out untouched
*out = EddyNemotronResult{};  // too late

// Fix: zero-init immediately after the null-pointer guard, before any other early return:
if (!handle || !pcm || !out) { ... return EDDY_ERROR_INVALID_ARGUMENT; }
*out = EddyNemotronResult{};   // ← move here
if (sample_rate != 16000) { ... return EDDY_ERROR_INVALID_ARGUMENT; }

3. src/eddy_c.cpp*err is never set to nullptr on success in the new Nemotron functions

The header contract (eddy_c.h line 13) says:

"error_message parameters: Always call eddy_free_string() even if function succeeds"

Neither eddy_nemotron_infer_buffer nor eddy_nemotron_infer_file set *err = nullptr on the success path — nemotron_fill_result never touches err. A caller following the documented contract with an uninitialized char* err; will call eddy_free_string(err) on a garbage pointer after a successful call.

The same gap exists in the pre-existing Parakeet and Whisper functions, so this is systemic, but the new Nemotron functions perpetuate it rather than fixing it. The minimal fix is to add if (err) *err = nullptr; on the success path (or, better, at the top of both functions after the null-pointer check).


🟡 Plausible issues

4. src/models/nemotron/nemotron_openvino.cpp — No validation that the joint IR output dimension covers blank_idx

blank_idx is loaded from metadata (default 13087). The joint network is expected to emit vocab_size + 1 = 13088 logits so that index 13087 is reachable. If the joint IR was exported with vocab_size (13087) outputs instead, best will always be in [0, 13086], best == I.blank_idx (13087) can never be true, and the inner sym loop runs to max_symbols_per_frame on every encoder frame, producing runaway garbage output.

The encoder cache shapes are explicitly byte-size-validated at runtime (lines 422–427); a similar one-liner guard for the joint output could be added in ensure_compiled():

// After compiling the joint model:
auto joint_out_shape = impl_->joint.output(0).get_shape();
size_t joint_vsz = /* product of joint_out_shape */;
if (joint_vsz <= static_cast<size_t>(impl_->blank_idx)) {
    throw std::runtime_error("Nemotron joint IR output size " + std::to_string(joint_vsz) +
        " does not cover blank_idx " + std::to_string(impl_->blank_idx));
}

5. src/models/nemotron/nemotron_openvino.cpp line 469 — lg[0] read before vsz check

const float* lg = logits.data<float>();
const size_t vsz = logits.get_size();
int best = 0;
float best_score = lg[0];   // ← OOB if vsz == 0

A degenerate joint IR with a zero-size output axis would cause an out-of-bounds read here. This is low-probability (OpenVINO would likely refuse to compile such an IR) but easy to guard:

if (vsz == 0) throw std::runtime_error("Nemotron joint IR returned empty logits tensor");

This would also catch the off-by-one scenario from finding #4 more aggressively if the IR somehow produces fewer logits than expected.


Minor notes

  • eddy_download_parakeet_models in eddy_c.h works for Nemotron model names (the lookup uses the full MODEL_MAP), but the name is misleading — Nemotron C API users have no obvious way to discover it. A comment in the EddyNemotronModel section pointing to this function, or a thin eddy_download_nemotron_models alias, would help.

  • EddyNemotronConfig has no model_name field, so eddy_nemotron_create always defaults to the FP16 cache ("nemotron-streaming"). Selecting the INT8 variant via the C API requires knowing the full cache path. A model_name field (analogous to how Parakeet infers version from blank_token_id) would round out the API.

🤖 Generated with Claude Code

@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Code Review — Add NVIDIA Nemotron-3.5-ASR Streaming Multilingual Backend

This PR adds a cache-aware streaming FastConformer-RNNT backend with a clean pImpl design, a native C++ log-mel featurizer (avoiding the dynamic-shape OV preprocessor that NPU can't compile), and a C API that mirrors the existing Parakeet one. The NPU-specific BitwiseNot → LogicalNot IR rewrite is well-motivated and the cache-shape runtime guards are a good defensive touch. Findings below, ranked most-severe first.


1. eddy_nemotron_infer_file*out uninitialized when read_wav throws [BUG]

src/eddy_c.cpp — the try block that calls read_wav

try {
    auto pcm = eddy::audio::read_wav(wav_path);
    return eddy_nemotron_infer_buffer(handle, pcm.data(), pcm.size(), 16000, out, err);
} catch (const std::exception& e) {
    if (err) *err = capture_exception(e);
    return EDDY_ERROR_INFERENCE_FAILED;   // *out never touched
}

If read_wav throws (malformed WAV, wrong channel count, wrong sample rate, decode error), the catch sets *err and returns but never writes to *out. The *out = EddyNemotronResult{} guard exists on the FILE_NOT_FOUND path (with an explicit comment about why it's needed) and inside eddy_nemotron_infer_buffer—but that init is only reached if read_wav succeeds. A caller who calls eddy_nemotron_free_result(out) on any non-OK return (a reasonable defensive pattern) will delete[] garbage pointers → UB/crash.

Fix: add *out = EddyNemotronResult{}; immediately before the try {.


2. resolve_prompt_id — uppercase BCP-47 tags silently fall through to default [BUG]

src/models/nemotron/nemotron_openvino.cpp

auto it = dict.find(language);       // exact, case-sensitive — "EN-US" misses "en-US"
if (it != dict.end()) return it->second;
if (language.size() == 2) {         // hard gate — "EN-US" (len 5) skips entirely
    // case-insensitive prefix search
}
return impl_->default_prompt_id;     // silently returns "auto"=101

The dictionary holds keys like "en-US", "fr-FR". Callers (especially on Windows) commonly pass uppercase locales like "EN-US". The exact lookup misses, the 5-character length fails the == 2 guard, and the function returns default_prompt_id with no error or warning — wrong-language conditioning on every chunk.

Fix: normalize language to lowercase before the lookup, or remove the size() == 2 restriction and apply the case-insensitive prefix search for all non-matching inputs.


3. enc_step tensor allocated per chunk — missed in the hoisting pass [efficiency]

src/models/nemotron/nemotron_openvino.cpp — inside while (off < pcm.size())

// (before the loop — correctly hoisted)
ov::Tensor audio(ov::element::f32, ov::Shape{1, chunk_samples});
ov::Tensor mel_in(ov::element::f32, ov::Shape{1, bins, total});
ov::Tensor token(I.token_et, ov::Shape{1, 1});

while (off < pcm.size()) {
    // ...
    const size_t enc_d = enc_shape[1];   // fixed after compile
    ov::Tensor enc_step(ov::element::f32, ov::Shape{1, enc_d, 1});  // ← NOT hoisted

The comment block above the loop explicitly describes hoisting fixed-shape tensors to avoid per-iteration allocation—and lists audio, mel_in, token, token_length, mel_length. enc_step's shape {1, enc_d, 1} is equally fixed (encoder output dimension is constant after compile_model), but it was missed. For a 60-second audio file this is ~54 unnecessary OV tensor allocations.

Fix: declare enc_step before the while loop alongside the other hoisted tensors; on the first iteration after encoder_req.infer(), read enc_d from encoded.get_shape()[1] and lazily resize if needed (or read it once from the compiled model's output shape during ensure_compiled).


4. Benchmark normalizers diverge — Parakeet and Nemotron WER/CER are not comparable [bug]

examples/cpp/benchmark_fleurs.cpp vs. examples/cpp/benchmark_nemotron_fleurs.cpp

benchmark_fleurs.cpp normalizes byte-by-byte with std::isalnum; non-ASCII bytes (including accented letters like é, ü, CJK characters) are treated as non-alnum and replaced by spaces. benchmark_nemotron_fleurs.cpp normalizes codepoint-by-codepoint, preserving non-ASCII letters/diacritics/CJK and only stripping Unicode punctuation. For non-English FLEURS splits (French, Spanish, Mandarin, Japanese), the Parakeet benchmark scores accented letters as substitutions/deletions while the Nemotron benchmark matches them correctly. The WER/CER numbers in BENCHMARK_RESULTS.md comparing the two models are therefore measured against different denominators.

The shared utilities (Levenshtein, word tokenization, CJK detection, sample loading) should also be extracted to a header like examples/cpp/benchmark_utils.hpp to prevent this kind of drift.


5. resolve_prompt_idstd::tolower(char) is UB for non-ASCII bytes [minor]

src/models/nemotron/nemotron_openvino.cpp — line ~179

[](char a, char b) { return std::tolower(a) == std::tolower(b); }

std::tolower requires its argument to be representable as unsigned char or be EOF; passing a negative signed char is UB. Practically safe for ASCII language codes, but UB by the standard.

Fix: std::tolower(static_cast<unsigned char>(a)) == std::tolower(static_cast<unsigned char>(b))


6. eddy_nemotron_infer_file / eddy_nemotron_infer_buffer*out uninitialized on INVALID_ARGUMENT return [minor]

src/eddy_c.cpp

When !handle || !wav_path but out is non-null (a legitimate caller pattern: pass a stack-allocated result, check return code, clean up unconditionally), *out is never zero-initialized before the early EDDY_ERROR_INVALID_ARGUMENT return. Same pattern as finding #1.

Fix: add if (out) *out = EddyNemotronResult{}; before (or as part of) the null-arg guard.


7. blank_idx == vocab_size structural invariant is documented but not enforced [minor]

src/models/nemotron/nemotron_openvino.cpp — metadata load in ensure_compiled

Both values are loaded independently from metadata.json (default 13087), and a comment notes they are expected to be equal. A metadata.json from a future model variant where they differ would produce silently wrong transcripts (wrong tokens emitted or tokens dropped) rather than a clear error.

Fix: after loading both, add:

if (impl_->blank_idx != impl_->vocab_size) {
    throw std::runtime_error("Nemotron metadata: blank_idx (" +
        std::to_string(impl_->blank_idx) + ") != vocab_size (" +
        std::to_string(impl_->vocab_size) + "); expected equal.");
}

8. kWordBoundary duplicated between Nemotron and Parakeet tokenizer [cleanup]

src/models/nemotron/nemotron_openvino.cpp and src/models/parakeet-v2/tokenizer.cpp

Both define constexpr std::string_view kWordBoundary = "\xE2\x96\x81" independently. If a future model variant uses a different boundary marker, one site will be updated and the other silently left wrong.

Fix: extract to a shared header (e.g. include/eddy/models/spm_utils.hpp).


🤖 Generated with Claude Code

…ants

Auto-detect prompt conditioning from the encoder's input ports: the
multilingual encoder has a prompt_id input, the English nemotron-speech-
streaming encoder does not. When absent, skip prompt_id resolution and don't
feed the tensor. One eddy::nemotron backend now serves both models; everything
else (C++ mel featurizer, NPU BitwiseNot fix, cache-aware decode loop) is shared.

Adds nemotron-speech-streaming{,-int8} model configs (English, FluidInference/
nemotron-speech-streaming-en-0.6b-ov). Multilingual path regression-checked
(en_us NPU unchanged at WER 12.2 on the smoke set).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Code Review — Add NVIDIA Nemotron-3.5-ASR streaming multilingual backend

This PR adds a well-structured eddy::nemotron cache-aware streaming RNNT backend with a native C++ log-mel featurizer, C API bindings, and two new example CLIs. The NPU BitwiseNot → LogicalNot workaround is well-documented, the RNNT inner loop reuses tensors across frames to avoid per-frame allocations, and the namespace rename eddy::parakeet → eddy::model_utils is clean and complete across all call sites. Below are the issues worth addressing before merge.


🔴 Bug — eddy_nemotron_infer_file: *out uninitialized on read_wav exception

File: src/eddy_c.cppeddy_nemotron_infer_file

*out is only zero-initialized for the early FILE_NOT_FOUND return. If the file exists but eddy::audio::read_wav throws (bad format, wrong channel count, sample-rate mismatch, etc.), the catch block returns an error code without ever touching *out. A caller that defensively calls eddy_nemotron_free_result(out) on any non-OK return — a reasonable pattern, and exactly what eddy_nemotron_infer_buffer's catch does — will delete[] uninitialized pointers (UB).

Fix: add *out = EddyNemotronResult{}; immediately after the null-pointer guard (before filesystem::exists), and add eddy_nemotron_free_result(out); in each catch block.


🟡 Correctness risk — load_npu_safe replaces all BitwiseNot nodes regardless of element type

File: src/models/nemotron/nemotron_openvino.cppload_npu_safe

The loop replaces every ov::op::v13::BitwiseNot with ov::op::v1::LogicalNot unconditionally. LogicalNot is only semantically equivalent for boolean inputs; on integer inputs it produces !x (zero/nonzero) instead of ~x (bitwise complement). For the current Nemotron encoder the only BitwiseNot is on a bool attention mask, so this is fine today. But if any future model export includes a BitwiseNot on an integer tensor, or this helper is reused for another model, the substitution silently changes semantics. The validate_nodes_and_infer_types() call at the end may or may not catch the element-type mismatch.

Fix: guard the replacement with bn->input(0).get_element_type() == ov::element::boolean.


🟡 Latent bug — n_freq_ initialized with a hardcoded literal instead of n_fft_ / 2 + 1

File: src/models/nemotron/nemotron_featurizer.cpp, constructor initializer list

n_fft_(512),
...
n_freq_(512 / 2 + 1),   // should be n_fft_ / 2 + 1

The header documents n_freq_ as n_fft_/2 + 1 and every downstream use depends on that invariant. The literal 512 coincidentally matches n_fft_ today so there is no live bug, but if n_fft_ is ever changed both have to be updated in lockstep with nothing enforcing it. Using n_fft_ / 2 + 1 is correct by construction.


🟡 C API design — eddy_nemotron_create hardcodes "nemotron-streaming" for the OV compile-cache dir

File: src/eddy_c.cppeddy_nemotron_create

EddyNemotronConfig has no model-name field, so callers providing an explicit model_dir pointing to the int8 or speech-streaming IR always hash their OV compiled-model artifacts into the same cache directory as the default fp16 model. Switching between fp16 and int8 via model_dir can load a stale compiled model for the wrong IR variant. The monolingual speech-streaming model loaded via model_dir shares a compile cache with the multilingual one, which can trigger confusing shape or device mismatch errors.

Consider adding a const char* model_name field to EddyNemotronConfig (defaulting to "nemotron-streaming") so the OV cache key matches the actual variant in use.


🟢 Minor — JSON output: device written with no escaping

File: examples/cpp/benchmark_nemotron_fleurs.cpp

model_dir gets backslash-escaping, but device (from argv) is streamed directly into a JSON string literal with no escaping. A device string containing " or a control character produces malformed JSON. The model_dir escaping is also incomplete — it handles \ but not ". In practice device strings are always CPU/NPU/GPU, so the practical risk is very low, but a small shared JSON-string escaper would be fully correct.


🟢 Minor — benchmark silently falls back to an empty model path on Linux/macOS

File: examples/cpp/benchmark_nemotron_fleurs.cpp

const char* lad = std::getenv("LOCALAPPDATA");
if (lad) model_dir = ...;

On Linux and macOS LOCALAPPDATA is unset, so model_dir stays empty and the model paths degrade to bare filenames relative to CWD ("nemotron_encoder.xml", etc.) with no diagnostic output. The benchmark then fails deep inside OpenVINO with a cryptic file-not-found message. A cross-platform fallback to eddy::get_model_assets_dir("nemotron-streaming-int8") — the same call used in nemotron_cli.cpp — would give a sensible default on all platforms.


Summary: The core RNNT streaming loop, featurizer math, NPU workaround, and C API structure are solid. The eddy_nemotron_infer_file out-pointer bug is the most actionable item for language-binding authors who pattern-match against eddy_parakeet_infer_file; the rest are robustness and portability improvements.

Point nemotron-speech-streaming{,-int8} at the existing
FluidInference/Nemotron-3.5-ASR-Streaming-Multilingual-0.6b-ov repo under
en/fp16 and en/int8 instead of a separate HF space.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Code Review — PR #10: Nemotron Streaming Backend

This PR adds the eddy::nemotron backend: a cache-aware streaming FastConformer-RNNT with a native C++ mel featurizer, per-chunk prompt-ID language conditioning, greedy RNNT decode, and a full C API mirroring the Parakeet surface. The namespace rename of eddy::parakeet → eddy::model_utils for the download utilities is clean and all call sites are updated correctly. The NPU BitwiseNot → LogicalNot rewrite is well-explained and guarded. Overall solid work — the findings below are mostly API completeness, one crash bug, and one benchmark correctness issue.


🔴 Bug — eddy_nemotron_infer_file: *out uninitialized when read_wav throws

src/eddy_c.cpp ~line 1181

If the file exists but read_wav throws (wrong format, wrong channel count, decode error), the catch block sets *err and returns EDDY_ERROR_INFERENCE_FAILED — but *out was never zero-initialized. A caller that defensively calls eddy_nemotron_free_result(out) after any non-OK return will attempt delete[] on garbage pointer values → undefined behaviour / crash.

The file-not-found early-return path correctly does *out = EddyNemotronResult{}; (and the comment explains why), but the try block that follows does not. eddy_nemotron_infer_buffer does its own zero-init, but read_wav throws before that is ever called.

Fix: add *out = EddyNemotronResult{}; immediately before the try block (one line), mirroring eddy_nemotron_infer_buffer.


🟠 API Gap — C API hardcodes FP16; INT8 and English-only variants are inaccessible by name

src/eddy_c.cpp line 518 (kNemotronModelName = "nemotron-streaming")

eddy_nemotron_create uses kNemotronModelName for both the assets lookup and the OV compilation-cache path. EddyNemotronConfig has model_dir but no model_name field. A caller who downloaded nemotron-streaming-int8 and passes {.device="NPU", .model_dir=NULL} gets "model files not available" because the function always looks in the FP16 cache slot.

The three other registered variants (nemotron-streaming-int8, nemotron-speech-streaming, nemotron-speech-streaming-int8) are fully defined in MODEL_MAP and downloadable via hf_fetch_models, but cannot be selected through the C API without hard-coding an absolute filesystem path. The C++ CLI already supports --model nemotron-streaming-int8 via get_model_assets_dir(model_name).

Fix: add a const char* model_name field to EddyNemotronConfig (defaulting to "nemotron-streaming"), and use it to resolve both the assets dir and the OV cache path in eddy_nemotron_create.


🟠 API Completeness — No eddy_download_nemotron_models in the C API

include/eddy/eddy_c.h

eddy_download_parakeet_models is the only download entry point exposed in the C API. The underlying implementation already dispatches through the generic MODEL_MAP (which includes all four Nemotron variants), but nothing in the public header advertises this. A language-binding author cannot download Nemotron models end-to-end via the SDK without either calling eddy_download_parakeet_models with a Nemotron model name (surprising and undiscoverable) or reimplementing HuggingFace URL construction themselves.

Fix: expose eddy_download_nemotron_models (or a generic eddy_download_models(model_name, ...)) in eddy_c.h.


🟠 Performance — Benchmark recompiles the model once per language

examples/cpp/benchmark_nemotron_fleurs.cpp lines 286–299

OpenVINONemotron is constructed and warmup() called inside the for (const auto& lang : langs) loop. Each construction creates a fresh std::once_flag, so the full compile pipeline (load + rewrite encoder IR, core.compile_model for all 3 subgraphs, parse metadata + vocab) runs once per language. On NPU, a single compile is 30–90 s; benchmarking 5 languages costs 2.5–7.5 minutes of compile time that could be 30–90 s.

prompt_id is a per-chunk int32 tensor, not baked into the IR. A single compiled model can serve all languages — resolve_prompt_id is already a public method and is called per-transcribe() call, not at construction.

Fix: move model construction and warmup() outside the language loop; pass the resolved prompt_id per transcribe call, or add a set_language() / language argument to transcribe().


🟡 Benchmark Correctness — normalize_text differs between Parakeet and Nemotron benchmarks

examples/cpp/benchmark_fleurs.cpp (existing) vs benchmark_nemotron_fleurs.cpp (new)

benchmark_fleurs.cpp's normalize_text is ASCII-only: it iterates char bytes and calls std::isalnum / std::tolower, treating every multi-byte UTF-8 sequence as non-alphanumeric (all become spaces). The new benchmark_nemotron_fleurs.cpp adds a correct UTF-8-aware version using utf8_chars + is_punct_or_symbol.

The PR's comparison tables include French, Spanish, Chinese, and Japanese. For those rows, the Parakeet WER/CER was computed with mangled reference text (every accent, CJK character, and diacritic silently replaced with spaces), while the Nemotron rows used correct normalization. The numbers are not directly comparable for any non-English language.

Fix: extract the UTF-8-aware normalize_text (+ utf8_chars, levenshtein, load_samples) into a shared header and use it in both benchmarks.


🟡 Latent Bug — blank_idx and vocab_size loaded independently with no invariant check

src/models/nemotron/nemotron_openvino.cpp lines 126–127 and ~1929

Both fields default to 13087 and are loaded separately from metadata.json. The comment at line 1929 already acknowledges they should always be equal (blank intentionally sits at index vocab_size), but the code doesn't assert or derive one from the other. A malformed export where blank_idx != vocab_size would silently produce wrong transcriptions — either blank tokens leaking into output or valid tokens being dropped.

Fix: load only vocab_size; set blank_idx = vocab_size and add a single post-load assertion (or just remove blank_idx as a stored field).


🟡 API Clarity — detected_language returns the bracketed form <en-US>, not a bare locale

include/eddy/eddy_c.h line 283

The header comment says "first <xx-XX> tag emitted, or """. The returned string literally includes the angle brackets (the vocab piece is ▁<en-US>; finalize_text strips and trims whitespace but preserves < and >). Callers expecting a BCP-47 locale string for matching against system locale APIs will need to strip brackets themselves. The existing CLI demo prints detected_lang=<en-US> which shows the brackets, but the C struct field is the only thing language-binding authors have to go on.

Fix: strip the angle brackets before storing in result.detected_language, or update the header comment to explicitly say "includes surrounding <> delimiters."


🔵 Dead Code — MelBuf::bins is written every chunk but never read

src/models/nemotron/nemotron_openvino.cpp ~line 1803

mel_cache.bins = bins; is assigned on every chunk in transcribe(), but mel_cache.bins is never subsequently read — the assembly loop uses the outer bins variable directly. The field is dead state in a hot-path struct.

Fix: remove MelBuf::bins.


Review generated with Claude Code

nemotron_cli --model help lists nemotron-streaming[-int8] (multilingual) and
nemotron-speech-streaming[-int8] (English); banner/header no longer hardcode
'3.5 Multilingual'. Backend file header documents that one path serves both
variants (prompt auto-detected). Comment-only/help-text changes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Code Review — PR #10: Add NVIDIA Nemotron-3.5-ASR streaming multilingual backend

Great addition overall. The cache-aware streaming RNNT pipeline is well-structured, the NPU BitwiseNot → LogicalNot rewrite is a smart workaround, and the charset_ok injection guard is a welcome addition to the download path. Three confirmed bugs and two cleanup notes below.


Bug 1 — *out uninitialized when read_wav throws in eddy_nemotron_infer_file (UB)

File: src/eddy_c.cppeddy_nemotron_infer_file

The *out = EddyNemotronResult{} zero-init only happens on the early FILE_NOT_FOUND return path. The comment on that line explicitly calls this out:

Zero-init before the early return so a caller that calls eddy_nemotron_free_result on *out after FILE_NOT_FOUND does not delete[] uninitialized/garbage pointers.

...but the same concern is not handled for the read_wav-throws path. If the file exists but read_wav throws (wrong sample rate, wrong channel count, corrupt file, unsupported format), the catch blocks return EDDY_ERROR_INFERENCE_FAILED without ever writing *out. A caller that follows the natural C API pattern of calling eddy_nemotron_free_result(out) on any error will delete[] uninitialized pointer values — undefined behavior, almost certainly a crash.

Fix: Move the zero-init to immediately after the null-pointer guard, before the filesystem::exists check, so it covers all return paths:

if (!handle || !wav_path || !out) { ... }
*out = EddyNemotronResult{};   // covers FILE_NOT_FOUND, read_wav throws, and all other error paths
std::error_code ec;
if (!std::filesystem::exists(wav_path, ec)) { ... }
try { ... }

Bug 2 — benchmark_nemotron_fleurs crashes on Linux/macOS; silently uses wrong model on Windows

File: examples/cpp/benchmark_nemotron_fleurs.cpp — lines 252–254

Two issues:

  1. Linux/macOS: LOCALAPPDATA is unset, so model_dir stays empty. fs::path("") / "nemotron_encoder.xml" resolves to the relative path "nemotron_encoder.xml" and the model fails to open with no helpful error message.
  2. Wrong model on Windows: The path is hardcoded to nemotron-streaming-int8 regardless of which variant the user has installed; a user who downloaded nemotron-streaming (the FP16 default from hf_fetch_models) will hit a silent failure.

Both nemotron_cli.cpp and eddy_nemotron_create already use eddy::get_model_assets_dir(model_name) — the benchmark should do the same instead of open-coding the platform path.


Bug 3 — JSON output escapes only \, not " in model_dir

File: examples/cpp/benchmark_nemotron_fleurs.cpp — line 352 (JSON writer)

Only backslashes are escaped in the output loop; a double-quote character in a user-supplied --model-dir path is written verbatim, prematurely terminating the JSON string and producing a malformed results.json that downstream parsers will reject. At minimum, also escape " as \".


Cleanup — normalize_text / levenshtein duplicated with diverged semantics

Files: examples/cpp/benchmark_nemotron_fleurs.cpp vs examples/cpp/benchmark_fleurs.cpp

Seven metric utilities (utf8_chars, cp_scalar, is_punct_or_symbol, normalize_text, levenshtein, words, wer, cer) are copy-pasted between the two benchmarks. The two normalize_text implementations have already diverged: the Parakeet benchmark is ASCII-only; the Nemotron benchmark handles full UTF-8 codepoints. WER/CER for non-ASCII languages (Russian, CJK, accented Latin) are therefore computed differently, making cross-model comparisons misleading. These should be consolidated into a shared benchmark_utils.hpp.


Note — std::system() for downloads

File: src/utils/ensure_models.cpp

The code already acknowledges the root problem:

The long-term fix is to drop std::system for a direct libcurl call and eliminate this class of concern entirely.

The charset_ok allowlist is a sound mitigation for now. Worth opening a tracked issue so this does not remain as an acknowledged TODO comment indefinitely.

… guard

- eddy_c: zero-init *out on ALL early/error returns (sample_rate mismatch,
  malformed-WAV INFERENCE_FAILED), not just FILE_NOT_FOUND, so callers that
  free after any error don't delete[] uninitialized pointers.
- ensure_models: delete partial files on download failure (a truncated file
  would otherwise pass file_nonempty and be skipped, loading a corrupt model);
  reject '..' path components (charset allows '.'/'/').
- nemotron encoder: guard the BitwiseNot->LogicalNot rewrite on boolean input
  type (only valid for bool); verify cache_len_out is i32 for symmetry with the
  cache_*_out byte-size checks.
- featurizer: ptrdiff_t frame indices (long is 32-bit on Win64); clarify the
  log-guard constant (6e-8, the IR's stored value) and the length-mask vs OV.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Code Review — Add NVIDIA Nemotron-3.5-ASR streaming multilingual backend

This is a substantial, well-structured addition: a separate eddy::nemotron backend with a native C++ mel featurizer, cache-aware streaming RNNT decode, full C API mirroring the Parakeet API, and a security-improved ensure_models namespace promotion. The NPU BitwiseNot→LogicalNot workaround is well-documented, the cache/LSTM state management is clean, and the featurizer geometry probe at init time is a nice defensive touch. The verified accuracy numbers are a strong signal the core pipeline is correct.

Six issues worth addressing before merge:


1. benchmark_nemotron_fleurs.cpp:252 — Default model dir is Windows-only; silently breaks on Linux/macOS

// Default model dir: %LOCALAPPDATA%/eddy/models/nemotron-streaming-int8/files
if (model_dir.empty()) {
    const char* lad = std::getenv("LOCALAPPDATA");
    if (lad) model_dir = ...;
}
// model_dir stays "" on Linux/macOS — all ModelPaths become bare filenames

LOCALAPPDATA is Windows-only. On Linux/macOS std::getenv returns nullptr, the branch is skipped, and every ModelPaths field becomes a bare relative filename ("nemotron_encoder.xml"). OpenVINO then fails to load from the working directory. nemotron_cli.cpp already has the correct cross-platform fix:

if (model_dir.empty())
    model_dir = eddy::get_model_assets_dir("nemotron-streaming-int8").string();

2. src/eddy_c.cpp:518,545 — OV compiled-model cache dir hardcoded to "nemotron-streaming" for all variants

static constexpr const char* kNemotronModelName = "nemotron-streaming";
// ...
.cache_dir = eddy::get_model_dir(kNemotronModelName).string()

EddyNemotronConfig has no model_name field, so eddy_nemotron_create always stores compiled artifacts under the nemotron-streaming cache bucket — even when the caller's model_dir points to INT8 or nemotron-speech-streaming files. OpenVINO's content-hash keying prevents loading the wrong compiled binary, but all variants pollute the same directory, defeating per-variant cache management. Adding a model_name field to EddyNemotronConfig (defaulting to "nemotron-streaming") would mirror eddy_parakeet_create's pattern.


3. src/eddy_c.cpp (eddy_nemotron_infer_file catch) — Missing free_result in error path

try {
    auto pcm = eddy::audio::read_wav(wav_path);
    return eddy_nemotron_infer_buffer(handle, pcm.data(), pcm.size(), 16000, out, err);
} catch (const std::exception& e) {
    if (err) *err = capture_exception(e);
    return EDDY_ERROR_INFERENCE_FAILED;  // *out not freed here
}

If read_wav throws (malformed file, wrong channel count, etc.), *out has been zero-initialized but the catch does not call eddy_nemotron_free_result(out). Currently this is safe — *out is all-zeros so free_result is a no-op. But the parallel infer_buffer catch does call free_result, and the asymmetry is a maintenance trap: any future refactor that writes into *out before the transcribe() call (e.g. for streaming partial results) would silently introduce a leak. Add the defensive call.


4. examples/cpp/benchmark_nemotron_fleurs.cpp — No check_models_available preflight

Unlike nemotron_cli.cpp and eddy_nemotron_create (which both call check_models_available to give a clear "missing file: X" message), the benchmark silently lets model construction fail inside OpenVINONemotron's ensure_compiled(). The user gets an opaque std::runtime_error from deep inside OpenVINO rather than the actionable list of missing files. The fix is a single call before the per-language loop:

std::string check_err;
if (!eddy::model_utils::check_models_available(fs::path(model_dir), &check_err,
        eddy::model_configs::NEMOTRON_FILES)) {
    std::cerr << "[ERROR] " << check_err << "\n"; return 1;
}

5. WER/CER normalization divergence between benchmarks affects published numbers comparability

benchmark_fleurs.cpp (Parakeet) uses ASCII-only normalization — it strips all non-ASCII bytes, silently dropping accented Latin characters, Cyrillic, etc. from both the reference and hypothesis. benchmark_nemotron_fleurs.cpp implements the correct Unicode-aware normalizer that retains those characters. The BENCHMARK_RESULTS.md table compares Parakeet and Nemotron WER on overlapping languages (e.g. es_419, fr_fr), but the two scores are computed on different text normalization bases. benchmark_fleurs.cpp should be updated to use the Unicode-aware version (or both should share a benchmark_utils.h).


6. src/utils/ensure_models.cpp:54 — Charset rejection error is indistinguishable from curl failure

if (!is_url_safe(url) || !is_path_safe(output_path.string())) {
    if (error_msg)
        *error_msg = "Refusing to download: unsafe characters in URL or path: " + url;
    return false;
}

The return value and error message are structurally identical to a curl failure. A future model whose HuggingFace repo ID contains +, %, or @ (not unusual) would fail silently — the operator sees "Failed to download" with no indication it's a rejected URL rather than a network error. Consider a distinct return code or a more prominent prefix (e.g. "[CONFIG ERROR] ...") to distinguish the two failure modes.


The core streaming pipeline, the mel featurizer, and the RNNT decode logic are solid. Items 1 and 3–4 are straightforward fixes; items 2, 5, and 6 are worth tracking even if not blocking merge.

🤖 Generated with Claude Code

@Alex-Wengg
Alex-Wengg merged commit fea4d7e into main Jun 29, 2026
1 check passed
@Alex-Wengg
Alex-Wengg deleted the add-nemotron-streaming-support branch June 29, 2026 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: Support NVIDIA Nemotron 3.5 ASR streaming model

1 participant