Add funASR-Nano support - #2936
Conversation
dosubot
Bot
added
the
size:XXL
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds end-to-end FunASR‑nano offline ASR support: new FunASR‑nano tokenizer, model, recognizer implementation, config types and validation, C/C++/Python bindings and examples, ONNX half<->float helpers, build targets, and factory integration into OfflineRecognizer creation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Factory as OfflineRecognizerImpl::Create
participant Recognizer as OfflineRecognizerFunASRNanoImpl
participant Feat as FeatureExtractor
participant Encoder as OfflineFunASRNanoModel::EncoderAdaptor
participant LLM as OfflineFunASRNanoModel::LLM
participant Tokenizer as FunASRNanoTokenizer
Client->>Factory: Create(config with funasr_nano.encoder_adaptor)
Factory-->>Client: OfflineRecognizerFunASRNanoImpl
Client->>Recognizer: DecodeStreams(streams)
Recognizer->>Feat: Extract features (FBANK)
Feat-->>Recognizer: frames
Recognizer->>Recognizer: ApplyLFR(frames)
Recognizer->>Encoder: ForwardEncoderAdaptor(lfr_features)
Encoder-->>Recognizer: encoder_out
Recognizer->>Tokenizer: Encode(prompts)
Tokenizer-->>Recognizer: token_ids
loop autoregressive
alt prefill
Recognizer->>LLM: ForwardLLMPrefill(inputs_embeds, mask)
LLM-->>Recognizer: logits + kv_cache
else decode
Recognizer->>LLM: ForwardLLMDecode(cur_emb, mask, past_kv)
LLM-->>Recognizer: logits + updated_kv
end
Recognizer->>Recognizer: SampleTokenFromLogits(...)
end
Recognizer-->>Client: OfflineRecognitionResult(text, timestamps)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @Wasser1462, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the capabilities of sherpa-onnx by adding full support for FunASR-Nano offline ASR models. The primary goal is to enable users to leverage these advanced models for speech recognition tasks, complete with robust tokenizer handling and optimized inference paths for both CPU and GPU. The changes ensure that FunASR-Nano can be seamlessly integrated and utilized through both programmatic APIs and command-line tools, offering flexibility and high performance for various deployment scenarios. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for FunASR-Nano offline ASR models. The changes are comprehensive, including new model and recognizer implementations, tokenizer integration, C++ and Python examples, and updates to the build system and configuration handling. The implementation is well-structured and follows the project's existing patterns. I've identified a few areas for improvement, mainly related to code duplication, logic simplification, and enhancing the C++ example's functionality. Overall, this is a great contribution.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (8)
cxx-api-examples/funasr-nano-cxx-api.cc (1)
100-108: Consider wrapping numeric parsing in try-catch.
std::stoiandstd::stofthrowstd::invalid_argumentorstd::out_of_rangeon invalid input. If a user provides a non-numeric value (e.g.,--funasr-nano-max-new-tokens=abc), the program will terminate with an uncaught exception instead of a helpful error message.🔎 Suggested approach
} else if (arg.find("--funasr-nano-max-new-tokens=") == 0) { - config.model_config.funasr_nano.max_new_tokens = - std::stoi(arg.substr(strlen("--funasr-nano-max-new-tokens="))); + try { + config.model_config.funasr_nano.max_new_tokens = + std::stoi(arg.substr(strlen("--funasr-nano-max-new-tokens="))); + } catch (const std::exception &e) { + std::cerr << "Invalid value for --funasr-nano-max-new-tokens\n"; + return -1; + }Apply similar handling for
--funasr-nano-temperature,--funasr-nano-top-p, and--num-threads.sherpa-onnx/csrc/offline-funasr-nano-model-config.cc (1)
57-74: Simplify redundant conditional logic.The
use_kv_cachecheck on line 65 is always true at that point because line 60-63 already returns false when!use_kv_cache. Theif (use_kv_cache)block can be simplified.🔎 Suggested simplification
// KV cache mode (prefill + decode) is required - bool use_kv_cache = !llm_prefill.empty() && !llm_decode.empty(); - - if (!use_kv_cache) { + if (llm_prefill.empty() || llm_decode.empty()) { SHERPA_ONNX_LOGE("Both --funasr-nano-llm-prefill and --funasr-nano-llm-decode are required"); return false; } - if (use_kv_cache) { - if (!FileExists(llm_prefill)) { - SHERPA_ONNX_LOGE("--funasr-nano-llm-prefill: '%s' does not exist", llm_prefill.c_str()); - return false; - } - if (!FileExists(llm_decode)) { - SHERPA_ONNX_LOGE("--funasr-nano-llm-decode: '%s' does not exist", llm_decode.c_str()); - return false; - } + if (!FileExists(llm_prefill)) { + SHERPA_ONNX_LOGE("--funasr-nano-llm-prefill: '%s' does not exist", llm_prefill.c_str()); + return false; + } + if (!FileExists(llm_decode)) { + SHERPA_ONNX_LOGE("--funasr-nano-llm-decode: '%s' does not exist", llm_decode.c_str()); + return false; }sherpa-onnx/csrc/funasr-nano-tokenizer.cc (2)
418-426: Nested anonymous namespace is redundant.There's already an enclosing anonymous namespace starting at line 20. The inner
namespace { ... }at line 418 is unnecessary.🔎 Suggested fix
-namespace { static inline int64_t TokenToIdOrDefault( const std::unordered_map<std::string, int32_t> &vocab, const std::string &tok, int64_t def_val) { auto it = vocab.find(tok); if (it == vocab.end()) return def_val; return static_cast<int64_t>(it->second); } -} // namespace
788-846: Consider moving internal helper functions to anonymous namespace.
ParseAddedTokensFromTokenizerJson,BuildAddedTokensTrie,MergeVocabAndAddedTokens, andBuildIdToToken(lines 788-942) are only used internally but are not in the anonymous namespace, exposing them as external linkage symbols. This increases binary size and risks ODR violations if similar symbols exist elsewhere.If these are intentionally public (for testing or other modules), this can be ignored. Otherwise, wrap them in the anonymous namespace or mark them
static.sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.cc (1)
23-76: Consider extracting FP16/FP32 conversion utilities to a shared header.These
Fp16ToFp32andFp32ToFp16functions are duplicated withHalfBitsToFloatandFloatToHalfBitsinoffline-funasr-nano-model.cc. Extracting them to a common utility header (e.g.,float16-utils.h) would reduce duplication and simplify maintenance.sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.h (1)
18-18: Unused include.The
pad-sequence.hheader appears to be unused in this file. Consider removing it to reduce compilation dependencies.sherpa-onnx/csrc/offline-funasr-nano-model.cc (2)
38-111: Duplicate FP16/FP32 conversion code.These conversion functions (
HalfBitsToFloat,FloatToHalfBits) duplicate the logic inoffline-recognizer-funasr-nano-impl.cc(Fp16ToFp32,Fp32ToFp16). As noted earlier, consolidating these into a shared utility header would reduce maintenance burden.
161-186: Thread-safety concern with static CUDA handle initialization.The one-time initialization of CUDA function pointers using static variables and a boolean flag (
cuda_checked) is not thread-safe. If multiple threads callCopyRawToCpusimultaneously before initialization completes, this could lead to a race condition.For offline recognition where concurrency may be limited, this is likely acceptable, but consider using
std::call_oncefor thread-safe initialization if multi-threaded usage is expected.🔎 Thread-safe initialization with std::call_once
+#include <mutex> // ... - static void *cuda_handle = nullptr; - static cudaMemcpyFunc cudaMemcpy_dyn = nullptr; - static cudaDeviceSynchronizeFunc cudaDeviceSynchronize_dyn = nullptr; - static cudaGetLastErrorFunc cudaGetLastError_dyn = nullptr; - static bool cuda_checked = false; - - if (!cuda_checked) { - cuda_checked = true; + static std::once_flag cuda_init_flag; + static void *cuda_handle = nullptr; + static cudaMemcpyFunc cudaMemcpy_dyn = nullptr; + static cudaDeviceSynchronizeFunc cudaDeviceSynchronize_dyn = nullptr; + static cudaGetLastErrorFunc cudaGetLastError_dyn = nullptr; + + std::call_once(cuda_init_flag, []() { cuda_handle = dlopen("libcudart.so", RTLD_LAZY); // ... rest of initialization - } + });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
cxx-api-examples/CMakeLists.txtcxx-api-examples/funasr-nano-cxx-api.ccpython-api-examples/offline-funasr-nano-decode-files.pysherpa-onnx/c-api/c-api.ccsherpa-onnx/c-api/c-api.hsherpa-onnx/c-api/cxx-api.ccsherpa-onnx/c-api/cxx-api.hsherpa-onnx/csrc/CMakeLists.txtsherpa-onnx/csrc/funasr-nano-tokenizer.ccsherpa-onnx/csrc/funasr-nano-tokenizer.hsherpa-onnx/csrc/offline-funasr-nano-model-config.ccsherpa-onnx/csrc/offline-funasr-nano-model-config.hsherpa-onnx/csrc/offline-funasr-nano-model.ccsherpa-onnx/csrc/offline-funasr-nano-model.hsherpa-onnx/csrc/offline-model-config.ccsherpa-onnx/csrc/offline-model-config.hsherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.ccsherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.hsherpa-onnx/csrc/offline-recognizer-impl.ccsherpa-onnx/csrc/sherpa-onnx-offline.ccsherpa-onnx/csrc/sherpa-onnx-vad-with-offline-asr.cc
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:23:50.237Z
Learning: The sherpa-onnx JNI library files are stored in Hugging Face repository at https://huggingface.co/csukuangfj/sherpa-onnx-libs under versioned directories like jni/1.12.7/, and the actual Windows JNI library filename is "sherpa-onnx-jni.dll" as defined in Core.java constants.
📚 Learning: 2025-08-06T04:23:50.237Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:23:50.237Z
Learning: The sherpa-onnx JNI library files are stored in Hugging Face repository at https://huggingface.co/csukuangfj/sherpa-onnx-libs under versioned directories like jni/1.12.7/, and the actual Windows JNI library filename is "sherpa-onnx-jni.dll" as defined in Core.java constants.
Applied to files:
sherpa-onnx/csrc/offline-model-config.hcxx-api-examples/CMakeLists.txtsherpa-onnx/csrc/offline-recognizer-impl.ccsherpa-onnx/csrc/CMakeLists.txtsherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.h
📚 Learning: 2025-08-06T04:18:47.981Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:18:47.981Z
Learning: In sherpa-onnx Java API, the native library names in Core.java (WIN_NATIVE_LIBRARY_NAME = "sherpa-onnx-jni.dll", UNIX_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.so", MACOS_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.dylib") are copied directly from the compiled binary filenames and should not be changed to match other libraries' naming conventions.
Applied to files:
cxx-api-examples/CMakeLists.txtsherpa-onnx/csrc/CMakeLists.txt
🧬 Code graph analysis (9)
sherpa-onnx/csrc/offline-funasr-nano-model-config.h (2)
sherpa-onnx/c-api/cxx-api.h (1)
OfflineFunASRNanoModelConfig(282-294)sherpa-onnx/csrc/offline-funasr-nano-model-config.cc (6)
Register(10-43)Register(10-10)Validate(45-112)Validate(45-45)ToString(114-131)ToString(114-114)
sherpa-onnx/c-api/cxx-api.cc (1)
sherpa-onnx/csrc/funasr-nano-tokenizer.cc (4)
c(203-210)c(203-203)c(212-225)c(212-212)
sherpa-onnx/csrc/offline-model-config.cc (1)
sherpa-onnx/c-api/cxx-api.cc (2)
FileExists(829-831)FileExists(829-829)
sherpa-onnx/csrc/funasr-nano-tokenizer.h (1)
sherpa-onnx/csrc/funasr-nano-tokenizer.cc (8)
Init(989-1041)Init(989-989)Init(1044-1081)Init(1044-1045)Init(1085-1122)Init(1085-1086)FinalizeSpecialIds(1125-1140)FinalizeSpecialIds(1125-1125)
sherpa-onnx/csrc/offline-funasr-nano-model-config.cc (2)
sherpa-onnx/csrc/offline-model-config.cc (6)
Register(14-61)Register(14-14)Validate(63-179)Validate(63-63)ToString(181-209)ToString(181-181)sherpa-onnx/c-api/cxx-api.cc (2)
FileExists(829-831)FileExists(829-829)
sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.cc (2)
sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.h (1)
OfflineRecognizerFunASRNanoImpl(22-56)sherpa-onnx/csrc/offline-recognizer-impl.cc (4)
ApplyInverseTextNormalization(834-845)ApplyInverseTextNormalization(834-835)ApplyHomophoneReplacer(847-854)ApplyHomophoneReplacer(847-848)
python-api-examples/offline-funasr-nano-decode-files.py (1)
sherpa-onnx/c-api/cxx-api.h (4)
sherpa_onnx(15-366)OfflineRecognizerConfig(327-342)OfflineModelConfig(296-320)OfflineFunASRNanoModelConfig(282-294)
cxx-api-examples/funasr-nano-cxx-api.cc (1)
sherpa-onnx/c-api/cxx-api.cc (16)
Create(52-114)Create(52-53)Create(315-321)Create(315-316)Create(403-459)Create(403-403)Create(508-550)Create(508-508)Create(622-636)Create(622-623)Create(663-666)Create(663-663)Create(702-728)Create(702-703)Create(785-792)Create(785-788)
sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.h (2)
sherpa-onnx/csrc/funasr-nano-tokenizer.h (1)
FunASRNanoTokenizer(29-104)sherpa-onnx/csrc/offline-funasr-nano-model.h (1)
OfflineFunASRNanoModel(13-104)
🪛 Flake8 (7.3.0)
python-api-examples/offline-funasr-nano-decode-files.py
[error] 22-22: 'typing.List' imported but unused
(F401)
🔇 Additional comments (37)
python-api-examples/offline-funasr-nano-decode-files.py (4)
31-144: LGTM!The argument parser is well-structured with appropriate defaults and clear help text. The required model paths are correctly marked, and the sound files positional argument ensures at least one input file is provided.
147-176: LGTM!The recognizer configuration correctly maps all CLI arguments to the
OfflineFunASRNanoModelConfigstructure, matching the C++ API. The hardcoded feature extraction parameters (16kHz, 80 mel bins) and greedy search decoding are appropriate defaults for this model.
179-189: LGTM!The decode workflow correctly follows the sherpa-onnx pattern: read wave → create stream → accept waveform → decode → return result.
192-218: LGTM!Good error handling that prints missing files to stderr and continues processing remaining files. The conditional output for tokens and timestamps handles cases where these may be empty.
sherpa-onnx/csrc/sherpa-onnx-offline.cc (1)
97-112: Documentation block looks good overall.The FunASR-nano usage example is clear and follows the established pattern for other model types. The inconsistency with
--funasr-nano-embeddingbeing optional here vs required insherpa-onnx-vad-with-offline-asr.ccwas noted in that file's review.sherpa-onnx/c-api/cxx-api.h (1)
319-319: LGTM!The
funasr_nanomember is correctly added toOfflineModelConfig, consistent with the pattern used for other model types.sherpa-onnx/csrc/offline-funasr-nano-model.h (1)
1-108: Well-structured header following established patterns.The PIMPL pattern, templated constructor for platform-specific resource managers, and ONNX Runtime integration are consistent with other model classes in the codebase. The documentation comments clearly describe the tensor shapes and expected inputs/outputs.
sherpa-onnx/csrc/offline-model-config.h (4)
12-12: LGTM!Include placement follows alphabetical ordering convention.
40-40: LGTM!Member declaration placement is consistent with other model configs.
76-76: LGTM!Constructor parameter added in correct position, matching member declaration order.
95-95: LGTM!Initializer list entry is correctly placed and follows the established pattern.
sherpa-onnx/csrc/CMakeLists.txt (1)
246-252: LGTM - source files correctly added to build.The FunASR-nano sources are added unconditionally, matching the pattern of other core model types. The placement after QNN sources is acceptable.
sherpa-onnx/csrc/offline-recognizer-impl.cc (1)
31-31: LGTM!The include for the new FunASR Nano implementation header is properly placed in alphabetical order with other offline recognizer headers.
cxx-api-examples/CMakeLists.txt (1)
145-147: LGTM!The new
funasr-nano-cxx-apiexecutable target follows the established pattern in this file for adding C++ API examples.cxx-api-examples/funasr-nano-cxx-api.cc (1)
114-186: Approve the example structure and timing instrumentation.The example demonstrates proper usage of the sherpa-onnx C++ API for FunASR-nano, including model loading, stream creation, decoding, and RTF calculation. The timing metrics provide useful performance feedback.
sherpa-onnx/c-api/c-api.h (1)
527-527: LGTM!The
funasr_nanomember is correctly added toSherpaOnnxOfflineModelConfig, following the same pattern as other model configurations in this struct.sherpa-onnx/csrc/offline-model-config.cc (3)
28-28: LGTM!The
funasr_nano.Register(po)call is correctly added following the pattern of other model configs.
97-104: LGTM!Correctly skips the tokens file requirement for FunASR-nano since it uses a tokenizer directory instead.
164-166: LGTM!The FunASR Nano validation branch is correctly placed and delegates to
funasr_nano.Validate().sherpa-onnx/csrc/offline-funasr-nano-model-config.h (1)
12-52: LGTM!The
OfflineFunASRNanoModelConfigstruct is well-designed with sensible defaults, clear documentation comments, and proper method declarations. The defaults match those in the C++ API header (sherpa-onnx/c-api/cxx-api.h).sherpa-onnx/csrc/funasr-nano-tokenizer.cc (2)
1229-1300: LGTM on the Encode implementation.The encoding logic correctly handles:
- Added token matching with trie-based longest match
- Single-word boundary checking
- ByteLevel encoding followed by BPE with caching
- Pattern-based text splitting for Qwen3 compatibility
1302-1364: LGTM on the Decode implementation.The decoding correctly:
- Filters out special tokens
- Reverses ByteLevel encoding via unicode_to_byte mapping
- Strips remaining special token strings as a safety measure
- Trims whitespace from the final output
sherpa-onnx/csrc/funasr-nano-tokenizer.h (1)
1-108: LGTM!The header file is well-structured with:
- Proper include guards
- Platform-specific constructor overloads for Android and OHOS are correctly guarded
- Clean public API with
Encode/Decodemethods and special token accessors- PIMPL-like organization with public structures and private implementation details
sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.cc (8)
80-101: LGTM!Constructors properly initialize all members, delegate to the base class, and call
InitFeatConfig()for feature configuration setup.
118-139: LGTM!The LFR (Low Frame Rate) processing correctly handles edge cases (empty output when insufficient frames) and efficiently copies concatenated frames using pointer arithmetic.
145-167: LGTM!The chat template construction is well-organized with proper token ID fallback logic when pad token is unavailable.
169-201: LGTM!Greedy decoding correctly handles both FP16 and FP32 logits with proper NaN/Inf filtering. The fallback to token 0 is a reasonable safety measure.
333-338: Verify tensor data lifetime with local vector.The
one_idvector is local to this block, and its data pointer is passed toCreateTensor. While this appears safe becauseone_tensoris immediately consumed byForwardEmbedding, the pattern could be fragile if the code is refactored.
466-494: LGTM!The timestamp estimation using linear interpolation is clearly documented as an approximation. The calculation correctly reverses LFR to estimate original frames and distributes tokens evenly over the audio duration.
498-528: LGTM!Sequential stream processing is appropriate for offline recognition. The use of
const_caston line 519 is a necessary pattern for ONNX Runtime tensor creation when the underlying data is logically const.
530-538: LGTM!Platform-specific template instantiations are correctly guarded with preprocessor conditionals.
sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.h (1)
22-56: LGTM overall!The class design follows the existing patterns in sherpa-onnx with proper inheritance from
OfflineRecognizerImpl, manager-based constructor template, and appropriate override methods.sherpa-onnx/csrc/offline-funasr-nano-model.cc (5)
231-336: LGTM!Tensor type conversion utilities are well-implemented with proper handling for FP16, FP32, and int32/int64 types. Error logging is appropriate for unsupported types.
338-357: LGTM!The function correctly calculates tensor byte sizes for FP16 and FP32 types, with appropriate error handling for unsupported types.
363-531: LGTM!The
Implclass constructors and initialization methods are well-organized with:
- Proper session options configuration
- Metadata extraction from model files
- Debug logging when enabled
- Platform-specific loading via template constructor
631-656: Non-owning tensor views created from const references.The
const_castusage to create non-owningOrt::Valueviews from thepast_key_valuesconst reference is necessary for ONNX Runtime's API but creates a subtle contract: the caller must ensure the original tensors remain valid during theRuncall. The comment at line 632 documents this intent, which is good.
891-971: LGTM!The public API correctly delegates to the
Implclass following the PIMPL pattern, with properstd::movesemantics and platform-specific template instantiations.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sherpa-onnx/csrc/CMakeLists.txtsherpa-onnx/csrc/offline-model-config.ccsherpa-onnx/csrc/offline-model-config.hsherpa-onnx/csrc/offline-recognizer-impl.cc
🚧 Files skipped from review as they are similar to previous changes (3)
- sherpa-onnx/csrc/offline-recognizer-impl.cc
- sherpa-onnx/csrc/offline-model-config.h
- sherpa-onnx/csrc/CMakeLists.txt
🧰 Additional context used
🧬 Code graph analysis (1)
sherpa-onnx/csrc/offline-model-config.cc (1)
sherpa-onnx/c-api/cxx-api.cc (2)
FileExists(829-831)FileExists(829-829)
🔇 Additional comments (3)
sherpa-onnx/csrc/offline-model-config.cc (3)
28-28: LGTM!The registration follows the established pattern for other model configurations.
202-202: LGTM!The ToString() addition follows the established pattern for including model configurations in the debug output.
98-105: The special-case handling for FunASR-nano is intentional and properly implemented. FunASR-nano uses a dedicated tokenizer directory (funasr-nano-tokenizerparameter) containing an LLM-style tokenizer with embeddings, which is fundamentally different from the token vocabulary files (tokensparameter) used by other ASR models. The conditional skip at lines 100-105 correctly prevents redundant validation, and FunASR-nano delegates to its ownValidate()method at line 165 to validate the tokenizer directory. The tokenizer directory loading mechanism is complete and actively used in offline-recognizer-funasr-nano-impl.cc. No changes needed.
There is no Can you update the modelscope or the commandline? |
|
I see. It is in Please ignore the previous comment. |
|
Can you add sherpa-onnx/scripts/medasr/export_onnx.py Line 100 in 1f5f963 when exporting models to ONNX? It simplifies the deployment for Android using assetsif no external data is used. |
|
Yes, I can do that. I’ll re-export the ONNX models with |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
sherpa-onnx/csrc/offline-funasr-nano-model.cc (3)
39-110: Consider using ONNX Runtime's built-in cast operators.The manual IEEE 754 FP16↔FP32 conversion implementations (HalfBitsToFloat, FloatToHalfBits) are comprehensive and appear correct, handling special cases like subnormals, infinities, and NaN. However, they add ~70 lines of complex bit manipulation. ONNX Runtime may provide built-in Cast operators that could simplify this code and reduce maintenance burden.
If ONNX Runtime's Cast op is not suitable here (e.g., due to performance or API constraints), the current implementation is acceptable. Consider adding a comment explaining why manual conversion is preferred.
356-357: CUDA device 0 is hardcoded for memory allocation.Line 356 hardcodes CUDA device 0 for the
cuda_mem_info_allocator. The comment notes thatSessionOptionsconfigures the execution provider's device, but the binding allocator here independently specifies device 0. If users configure a different CUDA device via session options, there may be a mismatch.Consider extracting the device ID from session options or making it configurable via
OfflineModelConfigto ensure consistency. If device 0 is intentional for all deployments, document this constraint in the config or readme.
648-673: Document lifetime and const-correctness requirements for non-owning tensor views.Lines 667-672 use
const_cast<void*>to create non-owningOrt::Valueviews of thepast_key_valuestensors. This is necessary becauseOrt::Value::CreateTensorrequires a non-const pointer even for read-only views, but it creates a subtle safety contract:
- The
inputsvector must not outlive thepast_key_valuesreference.- The ONNX Runtime
Run()call must not modify the input tensors.- The tensor memory must remain valid during the entire
Run()execution.Currently, this is safe because
inputsis local toForwardLLMDecodeand used immediately at line 704-706. However, future refactoring could inadvertently violate these assumptions.🔎 Suggested improvement
Add an inline comment documenting the lifetime requirements:
for (const auto &kv : past_key_values) { auto k_info = kv.first.GetTensorTypeAndShapeInfo(); // ... extract shape, type, bytes ... + // SAFETY: CreateTensor creates a non-owning view. The underlying + // past_key_values tensors must remain valid until after Run() completes. + // const_cast is safe because ONNX Runtime does not modify inputs. inputs.push_back(Ort::Value::CreateTensor( k_mem, const_cast<void *>(kv.first.GetTensorData<void>()), k_bytes, k_shape.data(), k_shape.size(), k_type));Alternatively, investigate whether ONNX Runtime provides a const-safe API for creating read-only tensor views.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sherpa-onnx/csrc/offline-funasr-nano-model.cc
🧰 Additional context used
🧬 Code graph analysis (1)
sherpa-onnx/csrc/offline-funasr-nano-model.cc (1)
sherpa-onnx/csrc/offline-funasr-nano-model.h (1)
OfflineFunASRNanoModel(13-104)
🔇 Additional comments (2)
sherpa-onnx/csrc/offline-funasr-nano-model.cc (2)
955-1036: LGTM: Clean pimpl pattern implementation.The public API cleanly delegates to the
Implclass, maintaining encapsulation and binary compatibility. The platform-specific template instantiations for Android and OHOS asset managers are correctly guarded with preprocessor directives.
525-546: Excellent CUDA memory optimization strategy.The selective IO binding approach (logits→CPU for sampling, KV cache→GPU to avoid large transfers) is a well-considered optimization for the FunASR-Nano architecture. The inline comments clearly explain the rationale, and the implementation is consistent across encoder, prefill, and decode forward passes.
Also applies to: 580-602, 687-707
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
sherpa-onnx/csrc/offline-funasr-nano-model.cc (1)
418-429: Metadata mismatches should be fatal errors.The code logs vocab_size mismatches between decode and prefill models but continues execution. This is inconsistent with other error handling in the codebase and can lead to runtime failures or silent correctness issues.
Based on past review feedback, these checks should call
SHERPA_ONNX_EXIT(-1)after logging to enforce strict metadata consistency.🔎 Proposed fix
if (vocab_size_ > 0 && decode_vocab_size != vocab_size_) { SHERPA_ONNX_LOGE( "Decode model vocab_size (%d) != prefill vocab_size (%d)", decode_vocab_size, vocab_size_); + SHERPA_ONNX_EXIT(-1); }sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.cc (1)
23-77: Consolidate duplicate FP16/FP32 conversion functions.These conversion functions are duplicated from
offline-funasr-nano-model.cc(asHalfBitsToFloatandFloatToHalfBits), but this implementation is less robust—it truncates rather than rounds, leading to precision loss.As noted in past reviews, consolidate these functions into a shared utility file (e.g.,
sherpa-onnx/csrc/onnx-utils.h) and use the more accurate implementation with proper rounding fromoffline-funasr-nano-model.cceverywhere.
🧹 Nitpick comments (2)
sherpa-onnx/csrc/offline-funasr-nano-model.cc (1)
477-494: Apply consistent scoping to all model buffers.The prefill buffer uses scoped blocks (lines 480-483) to reduce peak memory usage, but the encoder, decode, and embedding buffers don't. For consistency and optimal memory usage, wrap all buffer reads in scoped blocks.
🔎 Proposed refactor for consistent buffer scoping
- auto buf_encoder = ReadFile(mgr, c.encoder_adaptor); - InitEncoderAdaptorFromMemory(buf_encoder.data(), buf_encoder.size()); + { + auto buf_encoder = ReadFile(mgr, c.encoder_adaptor); + InitEncoderAdaptorFromMemory(buf_encoder.data(), buf_encoder.size()); + } { auto buf_prefill = ReadFile(mgr, c.llm_prefill); InitLLMPrefillFromMemory(buf_prefill.data(), buf_prefill.size()); } - auto buf_decode = ReadFile(mgr, c.llm_decode); - InitLLMDecodeFromMemory(buf_decode.data(), buf_decode.size()); + { + auto buf_decode = ReadFile(mgr, c.llm_decode); + InitLLMDecodeFromMemory(buf_decode.data(), buf_decode.size()); + } if (!c.embedding.empty()) { - auto buf_embedding = ReadFile(mgr, c.embedding); - InitEmbeddingFromMemory(buf_embedding.data(), buf_embedding.size()); + { + auto buf_embedding = ReadFile(mgr, c.embedding); + InitEmbeddingFromMemory(buf_embedding.data(), buf_embedding.size()); + } has_embedding_model_ = true;sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.cc (1)
206-448: Text generation logic is correct but could benefit from refactoring.The autoregressive generation correctly implements:
- Prefill/decode mode switching with KV cache
- Embedding preparation (text + audio)
- Greedy token sampling with EOS handling
- Post-processing with ITN and homophone replacement
- Timestamp estimation via linear interpolation
The function is quite long (240+ lines). Consider extracting helper methods for:
- Embedding preparation (lines 233-295)
- Decoding loop (lines 307-405)
- Post-processing (lines 406-448)
This would improve readability and testability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
sherpa-onnx/c-api/c-api.ccsherpa-onnx/c-api/c-api.hsherpa-onnx/c-api/cxx-api.ccsherpa-onnx/csrc/offline-funasr-nano-model.ccsherpa-onnx/csrc/offline-funasr-nano-model.hsherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.ccsherpa-onnx/csrc/onnx-utils.cc
🚧 Files skipped from review as they are similar to previous changes (2)
- sherpa-onnx/c-api/c-api.h
- sherpa-onnx/c-api/cxx-api.cc
🧰 Additional context used
🧬 Code graph analysis (2)
sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.cc (2)
sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.h (1)
OfflineRecognizerFunASRNanoImpl(22-56)sherpa-onnx/csrc/offline-funasr-nano-model.cc (4)
input_ids(708-737)input_ids(708-708)features(505-534)features(505-505)
sherpa-onnx/csrc/offline-funasr-nano-model.cc (1)
sherpa-onnx/csrc/onnx-utils.cc (8)
View(188-226)View(188-188)GetInputNames(52-62)GetInputNames(52-53)GetOutputNames(64-74)GetOutputNames(64-65)PrintModelMetadata(105-125)PrintModelMetadata(105-105)
🔇 Additional comments (13)
sherpa-onnx/c-api/c-api.cc (1)
514-537: Thesystem_promptfield is present in theSherpaOnnxOfflineFunASRNanoModelConfigstruct definition inc-api.h. All field mappings in lines 514-537 ofc-api.cccorrespond to existing struct members, so the code will compile correctly.sherpa-onnx/csrc/onnx-utils.cc (1)
192-192: Correct implementation for GPU tensor view support in FunASR-Nano.The
Viewfunction correctly usesGetTensorMemoryInfo()to preserve the original tensor's memory location (CPU or GPU) when creating a tensor view. This is the appropriate approach for proper GPU tensor handling in the FunASR-Nano model integration.sherpa-onnx/csrc/offline-funasr-nano-model.h (1)
1-101: LGTM!The header is well-structured with comprehensive documentation, proper include guards, and correct PIMPL pattern implementation. The API surface is clean and the method signatures are appropriate for the FunASR-nano model integration.
sherpa-onnx/csrc/offline-funasr-nano-model.cc (4)
37-108: Well-implemented FP16/FP32 conversion helpers.The conversion functions correctly handle special cases (zero, subnormal, infinity, NaN) and include proper rounding. This implementation is more robust than simpler versions that truncate.
302-345: LGTM!The constructor properly validates configuration, initializes ONNX sessions, and conditionally enables CUDA IO binding for performance optimization. The error handling is appropriate.
505-704: Well-implemented forward methods with CUDA optimization.The forward methods properly handle type conversions, implement smart IO binding (logits on CPU for sampling, KV cache on GPU to avoid large copies), and use non-owning tensor views to avoid unnecessary copies of KV cache data. The error handling is comprehensive.
915-989: LGTM!The public API correctly forwards all calls to the PIMPL implementation, and platform-specific template instantiations are properly guarded for Android and OHOS. The implementation follows the standard PIMPL pattern correctly.
sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.cc (6)
80-101: LGTM!Both constructors properly initialize the model, tokenizer, and RNG with appropriate configuration. The feature config initialization ensures correct preprocessing for FunASR-nano.
120-139: LGTM!The Low Frame Rate (LFR) processing correctly concatenates multiple consecutive frames and includes appropriate edge case handling for empty results.
145-167: LGTM!The chat template construction correctly formats system/user prompts and inserts audio token placeholders. The fallback from pad_id to eos_id (line 159) is a reasonable default.
172-201: LGTM!The token sampling correctly handles both FP16 and FP32 logits, skips NaN/Infinity values, and provides a safe fallback (token ID 0) if all logits are invalid. The greedy decoding strategy is appropriate for this model.
452-480: LGTM!The batch decoding correctly processes each stream independently, applies LFR preprocessing, validates input, and sets results. The per-stream isolation is appropriate for offline ASR.
482-490: LGTM!Platform-specific template instantiations are correctly guarded for Android and OHOS, following the project's standard pattern for platform-specific code.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
sherpa-onnx/csrc/offline-funasr-nano-model-config.cc (1)
1-6: LGTM: Includes are now complete.The missing
<sstream>include noted in the previous review has been added.sherpa-onnx/csrc/offline-funasr-nano-model.cc (2)
336-343: Metadata mismatch logs but continues execution - could cause runtime failures.When
decode_vocab_size != vocab_size_, the code logs an error but continues. This inconsistency between prefill and decode models could lead to incorrect tensor shapes or silent correctness issues during decode steps.🔎 Suggested fix: Make the mismatch fatal
if (vocab_size_ > 0 && decode_vocab_size != vocab_size_) { SHERPA_ONNX_LOGE( "Decode model vocab_size (%d) != prefill vocab_size (%d)", decode_vocab_size, vocab_size_); + SHERPA_ONNX_EXIT(-1); }
734-741: Same metadata mismatch issue in file-based initialization path.The
InitLLMDecodemethod has the same issue asInitLLMDecodeFromMemory- it logs mismatches but doesn't exit.🔎 Suggested fix
if (vocab_size_ > 0 && decode_vocab_size != vocab_size_) { SHERPA_ONNX_LOGE( "Decode model vocab_size (%d) != prefill vocab_size (%d)", decode_vocab_size, vocab_size_); + SHERPA_ONNX_EXIT(-1); }
🧹 Nitpick comments (4)
python-api-examples/offline-funasr-nano-decode-files.py (2)
146-175: Document the hardcoded sampling rate behavior.The function hardcodes
sampling_rate=16000(line 150), but the usage documentation (lines 139-140) states: "Its sample rate can be arbitrary and does not need to be 16kHz." This might confuse users about whether the script will resample audio or requires 16kHz input.Consider adding a comment explaining that sherpa-onnx automatically resamples input audio to match the configured feature extraction rate, or clarify the documentation if 16kHz input is actually required.
178-188: Consider adding error handling for audio file operations.The function lacks try-except blocks around
read_wave()anddecode()operations. If an audio file is corrupted, in an unsupported format, or causes decoding errors, the script will crash rather than gracefully handling the error and continuing with remaining files.🔎 Suggested error handling pattern
def decode_file( recognizer: sherpa_onnx.OfflineRecognizer, filename: str, ) -> sherpa_onnx.OfflineRecognitionResult: """Decode a single audio file.""" - wave = sherpa_onnx.read_wave(filename) - stream = recognizer.create_stream() - stream.accept_waveform(wave.sample_rate, wave.samples) - recognizer.decode(stream) - result = stream.result - return result + try: + wave = sherpa_onnx.read_wave(filename) + stream = recognizer.create_stream() + stream.accept_waveform(wave.sample_rate, wave.samples) + recognizer.decode(stream) + result = stream.result + return result + except Exception as e: + print(f"Error decoding {filename}: {e}", file=sys.stderr) + return NoneThen in
main(), check for None before printing results.sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.cc (2)
53-58: InitFeatConfig overwrites user-provided feature configuration.This method unconditionally sets
normalize_samples,window_type,snip_edges, anddither, potentially overriding user preferences. If these are strict model requirements, consider documenting this behavior or logging when overrides occur.🔎 Optional: Add debug logging for overrides
void OfflineRecognizerFunASRNanoImpl::InitFeatConfig() { + // FunASR-nano requires specific feature settings; override user config + if (config_.model_config.debug) { + SHERPA_ONNX_LOGE("FunASR-nano: Setting required feature config overrides"); + } config_.feat_config.normalize_samples = true; config_.feat_config.window_type = "hamming"; config_.feat_config.snip_edges = false; config_.feat_config.dither = 0.0f; }
164-168: Silent truncation when context exceeds max_seq_len.When
context_len > 2048, the source_ids are silently truncated. For long audio, this could cause unexpected behavior. Consider logging a warning.🔎 Optional: Add warning for truncation
const int32_t max_seq_len = 2048; if (context_len > max_seq_len) { + SHERPA_ONNX_LOGE( + "Context length %d exceeds max_seq_len %d, truncating", + context_len, max_seq_len); source_ids.resize(max_seq_len); context_len = max_seq_len; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cxx-api-examples/funasr-nano-cxx-api.ccpython-api-examples/offline-funasr-nano-decode-files.pysherpa-onnx/csrc/offline-funasr-nano-model-config.ccsherpa-onnx/csrc/offline-funasr-nano-model-config.hsherpa-onnx/csrc/offline-funasr-nano-model.ccsherpa-onnx/csrc/offline-funasr-nano-model.hsherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.ccsherpa-onnx/csrc/offline-recognizer-impl.ccsherpa-onnx/csrc/onnx-utils.ccsherpa-onnx/csrc/onnx-utils.hsherpa-onnx/csrc/sherpa-onnx-offline.cc
🚧 Files skipped from review as they are similar to previous changes (3)
- sherpa-onnx/csrc/sherpa-onnx-offline.cc
- sherpa-onnx/csrc/offline-funasr-nano-model.h
- cxx-api-examples/funasr-nano-cxx-api.cc
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-06T04:23:50.237Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:23:50.237Z
Learning: The sherpa-onnx JNI library files are stored in Hugging Face repository at https://huggingface.co/csukuangfj/sherpa-onnx-libs under versioned directories like jni/1.12.7/, and the actual Windows JNI library filename is "sherpa-onnx-jni.dll" as defined in Core.java constants.
Applied to files:
sherpa-onnx/csrc/onnx-utils.ccsherpa-onnx/csrc/offline-recognizer-impl.cc
📚 Learning: 2025-08-06T04:18:47.981Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:18:47.981Z
Learning: In sherpa-onnx Java API, the native library names in Core.java (WIN_NATIVE_LIBRARY_NAME = "sherpa-onnx-jni.dll", UNIX_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.so", MACOS_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.dylib") are copied directly from the compiled binary filenames and should not be changed to match other libraries' naming conventions.
Applied to files:
sherpa-onnx/csrc/onnx-utils.cc
🧬 Code graph analysis (4)
sherpa-onnx/csrc/onnx-utils.h (1)
sherpa-onnx/csrc/onnx-utils.cc (4)
HalfBitsToFloat(430-457)HalfBitsToFloat(430-430)FloatToHalfBits(462-501)FloatToHalfBits(462-462)
sherpa-onnx/csrc/offline-funasr-nano-model-config.cc (3)
sherpa-onnx/csrc/offline-model-config.cc (6)
Register(14-62)Register(14-14)Validate(64-184)Validate(64-64)ToString(186-215)ToString(186-186)sherpa-onnx/c-api/cxx-api.cc (2)
FileExists(831-833)FileExists(831-831)sherpa-onnx/csrc/offline-tts-matcha-model-config.cc (1)
Register(13-125)
sherpa-onnx/csrc/offline-funasr-nano-model-config.h (1)
sherpa-onnx/csrc/offline-model-config.h (1)
sherpa_onnx(25-115)
sherpa-onnx/csrc/offline-funasr-nano-model.cc (1)
sherpa-onnx/csrc/onnx-utils.cc (12)
HalfBitsToFloat(430-457)HalfBitsToFloat(430-430)FloatToHalfBits(462-501)FloatToHalfBits(462-462)View(190-229)View(190-190)GetInputNames(54-64)GetInputNames(54-55)GetOutputNames(66-76)GetOutputNames(66-67)PrintModelMetadata(107-127)PrintModelMetadata(107-107)
🔇 Additional comments (18)
python-api-examples/offline-funasr-nano-decode-files.py (4)
19-27: LGTM!The import handling is well-structured with clear error messaging for missing dependencies.
30-143: LGTM!The argument parser is comprehensive with clear help text and appropriate defaults for FunASR-nano configuration parameters.
191-213: LGTM!The main orchestration is well-structured with appropriate file existence checks and conditional printing of optional result fields. The error handling for missing files is appropriate, allowing the script to continue processing remaining files.
216-217: LGTM!Standard Python entry point implementation.
sherpa-onnx/csrc/onnx-utils.h (1)
121-123: LGTM! Clear and appropriate API additions.The function declarations are well-named and follow the existing code style. They provide a clean API for IEEE 754 half-precision ↔ single-precision float conversions.
sherpa-onnx/csrc/onnx-utils.cc (3)
8-9: LGTM! Necessary includes for the new conversion functions.Both
<cstdint>(for fixed-width integer types) and<cstring>(forstd::memcpy) are required for the IEEE 754 conversion implementations below.
428-457: LGTM! Correct IEEE 754 half-to-float conversion.The implementation correctly handles all special cases (zero, subnormal, normal, infinity, NaN) and uses
std::memcpyfor standards-compliant type punning. The subnormal normalization logic and exponent bias adjustments are accurate.
459-501: LGTM! Correct IEEE 754 float-to-half conversion with proper rounding.The implementation correctly handles overflow, underflow, infinity, and NaN cases. The round-to-nearest-even logic (lines 482-484, 489-497) is accurate, including proper handling of rounding overflow. The NaN payload simplification at line 470 is a valid trade-off for a basic conversion utility.
sherpa-onnx/csrc/offline-recognizer-impl.cc (2)
31-31: LGTM: FunASR Nano integration follows established patterns.The include and the new branch in the non-template
Createfunction are correctly placed and follow the existing model selection pattern (checking a config field, then returning the appropriate implementation).Also applies to: 212-215
547-550: LGTM: Template Create function now includes FunASR Nano branch.The template version for Android/OHOS platforms correctly mirrors the non-template branch, addressing the mobile platform support.
sherpa-onnx/csrc/offline-funasr-nano-model-config.cc (1)
45-113: LGTM: Comprehensive validation logic.The
Validate()method properly checks:
- Required fields are non-empty
- All model files exist on disk
- Parameter ranges (max_new_tokens > 0, temperature ≥ 0, top_p ∈ [0,1])
This aligns with the validation patterns used by other model configs in the codebase.
sherpa-onnx/csrc/offline-funasr-nano-model-config.h (1)
12-52: LGTM: Well-structured configuration with appropriate defaults.The struct correctly declares all necessary model paths and generation parameters. The defaults (temperature=0.3, top_p=0.8) are sensible for deterministic ASR output. The structure aligns with the CXX API definition.
sherpa-onnx/csrc/offline-funasr-nano-model.cc (3)
400-403: Good: Scoped buffer to reduce peak memory usage.The
{}scope ensuresbuf_prefillis freed immediately after initialization, as suggested in the previous review.
64-69: LGTM: CUDA provider detection usesfindas suggested.The implementation correctly uses
p.find("cuda") == 0for readability.
217-219: Good: ReusesView()from onnx-utils.cc.The
ViewConsthelper leverages the shared utility function as suggested in the previous review.sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.cc (3)
114-143: LGTM: Uses shared FP16 conversion utilities.The sampling function correctly uses
HalfBitsToFloatfromonnx-utils.h, addressing the previous review comment about code duplication.
389-417: DecodeStreams processes streams sequentially, not in batch.The loop at line 394 processes each stream one at a time. For multiple streams, this misses potential batching opportunities in the encoder. However, given the KV-cache autoregressive nature, true batching would be complex. The current approach is straightforward and correct.
22-43: LGTM: Constructor implementations are well-structured.Both the regular and templated constructors properly initialize the model, tokenizer, and RNG, then call
InitFeatConfig()to set required feature parameters.
|
i am still reading the code. Thank you for your contribution! |
|
The code looks great to me—thanks again! Would it be possible to also open-source the model export code? The repository below does not seem to include code for exporting LLMs with KV cache support or for exporting fp16 models: |
| // - Loads vocab.json + merges.txt + tokenizer.json(added_tokens) | ||
| // - Supports AddedTokens via Trie longest-match | ||
| // - ByteLevel bytes_to_unicode encode/decode | ||
| // Author: zengyongwang |
There was a problem hiding this comment.
Was this file authored by you? Additionally, could you add copyright information to the other files included in this pull request?
|
Thanks for the kind words!
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
cxx-api-examples/funasr-nano-cxx-api.cc (1)
128-198: Critical: Only processes first audio file.The audio file processing logic (lines 128-198) is embedded inside the argument parsing loop and exits immediately after processing one file (
return 0;at line 197). This prevents processing multiple audio files passed on the command line, inconsistent with other examples in the project likesherpa-onnx-offline.🔎 Recommended fix
Move the audio processing outside the argument parsing loop:
+ std::vector<std::string> audio_files; + for (int32_t i = 1; i < argc; ++i) { std::string arg = argv[i]; // ... option parsing ... } else if (arg[0] != '-') { - // This should be the audio file - std::string wave_filename = arg; - // ... all the processing code ... - return 0; + audio_files.push_back(arg); } } + if (audio_files.empty()) { + std::cerr << "Error: Please provide at least one audio file.\n"; + std::cerr << kUsageMessage << "\n"; + return -1; + } + + // Validate required arguments here + + std::cout << "Loading model...\n"; + // ... print config ... + + const auto begin_init = std::chrono::steady_clock::now(); + OfflineRecognizer recognizer = OfflineRecognizer::Create(config); + // ... check recognizer ... + // ... print init time ... + + for (const auto& wave_filename : audio_files) { + Wave wave = ReadWave(wave_filename); + // ... rest of processing per file ... + } + + return 0; - - std::cerr << "Error: Please provide an audio file.\n"; - std::cerr << kUsageMessage << "\n"; - return -1;This allows batch processing of multiple files and initializes the recognizer only once.
sherpa-onnx/csrc/funasr-nano-tokenizer.cc (3)
659-679: Fix indentation inconsistency.Lines 665-667 have incorrect indentation that breaks code readability. The closing brace and subsequent while loop appear misaligned.
790-848: Make function static or move to anonymous namespace.
ParseAddedTokensFromTokenizerJsonis used only within this file and should be markedstaticor moved into the anonymous namespace to limit its visibility.
876-901: Make function static or move to anonymous namespace.
MergeVocabAndAddedTokensis used only within this file and should be markedstaticor moved into the anonymous namespace to limit its visibility.
🧹 Nitpick comments (2)
cxx-api-examples/funasr-nano-cxx-api.cc (1)
71-74: Strengthen required argument validation.The check
argc < 6is a rough heuristic and doesn't validate that all five required model paths are actually provided. A user could provide 5--num-threadsarguments and no model paths, and this check would pass.🔎 Suggested improvement
After the argument parsing loop (before Line 128), explicitly check that required fields are non-empty:
// After the argument parsing loop (before processing audio) if (config.model_config.funasr_nano.encoder_adaptor.empty() || config.model_config.funasr_nano.llm_prefill.empty() || config.model_config.funasr_nano.llm_decode.empty() || config.model_config.funasr_nano.embedding.empty() || config.model_config.funasr_nano.tokenizer.empty()) { std::cerr << "Error: All required model paths must be provided.\n"; std::cerr << kUsageMessage << "\n"; return -1; }sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.cc (1)
150-392: Consider breaking down the GenerateText method.The
GenerateTextmethod is 242 lines long and handles multiple responsibilities: building source IDs, preparing embeddings, running autoregressive generation, and post-processing. Consider extracting helper methods for:
- Building and preparing the input embeddings buffer (lines 178-240)
- The autoregressive generation loop (lines 250-349)
- Post-processing and timestamp calculation (lines 351-391)
This would improve readability and make the code easier to maintain and test.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
cxx-api-examples/funasr-nano-cxx-api.ccpython-api-examples/offline-funasr-nano-decode-files.pysherpa-onnx/csrc/funasr-nano-tokenizer.ccsherpa-onnx/csrc/funasr-nano-tokenizer.hsherpa-onnx/csrc/offline-funasr-nano-model-config.ccsherpa-onnx/csrc/offline-funasr-nano-model-config.hsherpa-onnx/csrc/offline-funasr-nano-model.ccsherpa-onnx/csrc/offline-funasr-nano-model.hsherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.ccsherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.hsherpa-onnx/python/csrc/CMakeLists.txtsherpa-onnx/python/csrc/offline-funasr-nano-model-config.ccsherpa-onnx/python/csrc/offline-funasr-nano-model-config.hsherpa-onnx/python/csrc/offline-model-config.ccsherpa-onnx/python/sherpa_onnx/__init__.pysherpa-onnx/python/sherpa_onnx/offline_recognizer.py
🚧 Files skipped from review as they are similar to previous changes (2)
- sherpa-onnx/csrc/offline-funasr-nano-model-config.cc
- sherpa-onnx/csrc/offline-recognizer-funasr-nano-impl.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T04:23:50.237Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:23:50.237Z
Learning: The sherpa-onnx JNI library files are stored in Hugging Face repository at https://huggingface.co/csukuangfj/sherpa-onnx-libs under versioned directories like jni/1.12.7/, and the actual Windows JNI library filename is "sherpa-onnx-jni.dll" as defined in Core.java constants.
Applied to files:
sherpa-onnx/python/csrc/CMakeLists.txtsherpa-onnx/python/csrc/offline-funasr-nano-model-config.hsherpa-onnx/python/csrc/offline-model-config.ccpython-api-examples/offline-funasr-nano-decode-files.py
🧬 Code graph analysis (9)
sherpa-onnx/python/csrc/offline-funasr-nano-model-config.h (2)
sherpa-onnx/csrc/offline-funasr-nano-model-config.h (1)
sherpa_onnx(12-54)sherpa-onnx/python/csrc/offline-funasr-nano-model-config.cc (2)
PybindOfflineFunASRNanoModelConfig(13-29)PybindOfflineFunASRNanoModelConfig(13-13)
sherpa-onnx/python/sherpa_onnx/offline_recognizer.py (1)
sherpa-onnx/c-api/cxx-api.h (3)
OfflineFunASRNanoModelConfig(286-298)OfflineModelConfig(300-325)OfflineRecognizerConfig(332-347)
cxx-api-examples/funasr-nano-cxx-api.cc (1)
sherpa-onnx/csrc/offline-recognizer-impl.cc (6)
Create(69-399)Create(69-70)Create(402-732)Create(402-403)Create(870-871)Create(877-878)
sherpa-onnx/python/csrc/offline-funasr-nano-model-config.cc (1)
sherpa-onnx/csrc/offline-funasr-nano-model-config.cc (2)
ToString(119-136)ToString(119-119)
sherpa-onnx/python/csrc/offline-model-config.cc (2)
sherpa-onnx/python/csrc/offline-funasr-nano-model-config.cc (2)
PybindOfflineFunASRNanoModelConfig(13-29)PybindOfflineFunASRNanoModelConfig(13-13)sherpa-onnx/c-api/cxx-api.h (1)
OfflineFunASRNanoModelConfig(286-298)
sherpa-onnx/csrc/funasr-nano-tokenizer.cc (1)
sherpa-onnx/csrc/funasr-nano-tokenizer.h (1)
FunASRNanoTokenizer(30-105)
sherpa-onnx/csrc/offline-funasr-nano-model.cc (2)
sherpa-onnx/csrc/onnx-utils.cc (6)
HalfBitsToFloat(430-457)HalfBitsToFloat(430-430)FloatToHalfBits(462-501)FloatToHalfBits(462-462)View(190-229)View(190-190)sherpa-onnx/csrc/offline-funasr-nano-model.h (1)
OfflineFunASRNanoModel(17-100)
sherpa-onnx/python/sherpa_onnx/__init__.py (1)
sherpa-onnx/c-api/cxx-api.h (1)
OfflineFunASRNanoModelConfig(286-298)
sherpa-onnx/csrc/funasr-nano-tokenizer.h (1)
sherpa-onnx/csrc/funasr-nano-tokenizer.cc (15)
FunASRNanoTokenizer(972-974)FunASRNanoTokenizer(977-980)FunASRNanoTokenizer(984-987)Encode(1231-1302)Encode(1231-1231)Decode(1304-1366)Decode(1304-1304)Init(991-1043)Init(991-991)Init(1046-1083)Init(1046-1047)Init(1087-1124)Init(1087-1088)FinalizeSpecialIds(1127-1142)FinalizeSpecialIds(1127-1127)
🔇 Additional comments (12)
sherpa-onnx/python/csrc/CMakeLists.txt (1)
16-16: LGTM! FunASR-nano Python binding properly integrated.The addition of
offline-funasr-nano-model-config.ccfollows the established pattern for offline model configurations, maintains alphabetical order, and correctly integrates the Python bindings for the new FunASR-nano model type. The corresponding Python binding file exists, and all core FunASR-nano implementation files are properly compiled into the core library.sherpa-onnx/python/csrc/offline-funasr-nano-model-config.h (1)
1-17: LGTM!The header file follows the project's standard pattern for Python binding declarations. The include guards, namespace, and function declaration are properly structured.
sherpa-onnx/python/sherpa_onnx/__init__.py (1)
17-17: LGTM!The import is correctly placed in alphabetical order and follows the existing pattern for exposing model configuration classes.
sherpa-onnx/python/sherpa_onnx/offline_recognizer.py (1)
10-10: LGTM!The import is correctly placed and necessary for the new
from_funasr_nanoclassmethod.sherpa-onnx/python/csrc/offline-funasr-nano-model-config.cc (1)
1-32: LGTM!The Python binding implementation correctly exposes all fields of the
OfflineFunASRNanoModelConfigstruct to Python. The binding pattern follows established conventions in the codebase and properly maps the C++ToString()method to Python's__str__.sherpa-onnx/python/csrc/offline-model-config.cc (3)
14-14: LGTM!The include is correctly added and follows the pattern of other model config includes.
43-43: LGTM!The binding initializer is called in the appropriate location, maintaining consistency with other model config bindings.
61-61: LGTM!The FunASR-nano model config is properly integrated into the
OfflineModelConfigPython binding:
- Constructor parameter added at Line 61
- Default value provided at Line 79
- Read/write property exposed at Line 98
This follows the established pattern for other model configurations in the codebase.
Also applies to: 79-79, 98-98
sherpa-onnx/csrc/offline-funasr-nano-model-config.h (1)
1-59: LGTM!The header file is well-structured with:
- Proper include guards
- Clear comments documenting each field
- Reasonable default values for LLM generation parameters
- Standard method declarations (Register, Validate, ToString)
The struct follows established patterns in the codebase for model configuration.
python-api-examples/offline-funasr-nano-decode-files.py (3)
151-167: LGTM!The
create_recognizerfunction correctly uses the newfrom_funasr_nanoclassmethod and passes all necessary parameters. The implementation is clean and straightforward.
170-182: LGTM!The
decode_filefunction properly handles audio file reading and stream processing. Taking only the first channel (Line 176) is appropriate for mono ASR processing.
185-208: LGTM!The
mainfunction orchestrates the workflow correctly:
- Creates the recognizer once
- Validates file existence before processing
- Processes all provided files
- Handles and displays results appropriately
Hope you can release the export script soon. I want to try it with Qualcomm NPU. |
|
The export script has been pushed here for reference: Please note it’s not fully cleaned up yet — I originally exported with opset 18, and later had to add a number of compatibility fixes (opset/IR downgrades, Reduce* axes fixes, etc.) to match the ONNX Runtime version used by sherpa-onnx. I’m currently tied up with year-end work tasks, so I haven’t had time to refactor and document it properly, but it should still be useful as a starting point for trying Qualcomm NPU. |
| float temperature = 0.3f; | ||
|
|
||
| // Top-p (nucleus) sampling threshold | ||
| float top_p = 0.8f; |
There was a problem hiding this comment.
Looks like temperature and top_p are never used.
Do you plan to use them in a separate pull request?
|
Thanks again for your great contribution! I will merge it first. Please address any issues, if any, in a separate pull request. |
|
... |
|
Heads up: this PR was built on the old main export code (the prefill + decode inference flow). Since main has been refactored and the export pipeline was restructured, the original working version is preserved here: https://github.com/Wasser1462/FunASR-nano-onnx/tree/backup/main-2026-01-06 If you need the legacy prefill + decode export/inference code, please use that backup branch. I’ll open a new PR soon for the refactored version. |
3 participants
FunASR-Nano Offline ASR Integration
Summary
This PR adds/updates FunASR-Nano offline ASR integration in sherpa-onnx, including tokenizer integration, model config wiring, and example commands.
The goal is to make FunASR-Nano usable via
sherpa-onnx-offlineandsherpa-onnx-vad-with-offline-asrwith clear guidance on which precision is recommended on GPU/CPU.Key Changes
Model Download (ModelScope)
Pre-exported models are uploaded to ModelScope and can be downloaded with:
Recommended Runtime / Performance Notes
Based on local benchmarking:
GPU (CUDA): Recommend FP32
CPU: Recommend INT8
FP16
Example Commands
1) FP32 on GPU (Recommended for CUDA)
2) INT8 (Recommended for CPU)
3) VAD + INT8 on CPU (Recommended)
4) VAD + FP32 on GPU (Example)
Testing
Ran basic CLI sanity tests with:
./bin/sherpa-onnx-offline./bin/sherpa-onnx-vad-with-offline-asrVerified output correctness on CPU and CUDA environments as described above.
Notes / Disclaimer
This work was completed with assistance from AI tools. If you spot any issues or have improvement suggestions, please let me know — I will follow up quickly.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.