Repository navigation
Refactor sense voice impl - #2873
Conversation
dosubot
Bot
added
the
size:XL
|
Warning Rate limit exceeded@csukuangfj has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
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. WalkthroughThe PR consolidates provider-specific offline Sense Voice recognizer implementations (ASCEND, AXCL, AXERA, RKNN, QNN) into a unified templated implementation. Provider-specific header files are removed and replaced with a generic template wrapper, with the factory updated to instantiate the template with provider-specific model types. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 @csukuangfj, 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 introduces a significant refactoring of the Sense Voice offline recognizer. By consolidating platform-specific implementations into a generic templated class, the change aims to streamline the integration of Sense Voice models across different hardware backends like RKNN, Axera, AXCL, Ascend, and QNN. This approach enhances code reusability and simplifies future development and maintenance efforts for these components. Additionally, a minor adjustment was made to a build script to ensure correct environment variable usage and optimize build performance. 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 refactors the sense-voice implementations for different providers (rknn, axera, axcl, ascend, qnn) into a single templated class. This is an excellent change that significantly reduces code duplication and improves maintainability. I've found one minor issue in a build script comment, which I've pointed out. Otherwise, the refactoring looks solid.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the SenseVoice offline recognizer implementations to eliminate code duplication across multiple hardware backends. Previously, each hardware provider (RKNN, Axera, AXCL, Ascend, QNN) had its own nearly identical implementation file. The refactoring consolidates all these implementations into a single template class OfflineRecognizerSenseVoiceTplImpl parameterized by the model type.
Key Changes:
- Introduced a template-based implementation that accepts different SenseVoice model types as template parameters
- Removed 5 duplicate implementation files (one for each hardware backend)
- Updated factory methods to instantiate the template class with appropriate model types
- Fixed inconsistent empty logits checking (now all backends handle empty results)
- Fixed minor issues in build script comments and parameters
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sherpa-onnx/csrc/offline-recognizer-sense-voice-tpl-impl.h | New template-based implementation that replaces all hardware-specific implementations |
| sherpa-onnx/csrc/offline-recognizer-impl.cc | Updated factory methods to use template instantiations; changed includes from hardware-specific impl headers to model headers |
| sherpa-onnx/csrc/rknn/offline-recognizer-sense-voice-rknn-impl.h | Removed duplicate RKNN-specific implementation |
| sherpa-onnx/csrc/axera/offline-recognizer-sense-voice-axera-impl.h | Removed duplicate Axera-specific implementation |
| sherpa-onnx/csrc/axcl/offline-recognizer-sense-voice-axcl-impl.h | Removed duplicate AXCL-specific implementation |
| sherpa-onnx/csrc/ascend/offline-recognizer-sense-voice-ascend-impl.h | Removed duplicate Ascend-specific implementation |
| build-axcl-linux-aarch64.sh | Fixed typo in environment variable comment and corrected make parallel jobs parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Before you run this file, make sure you have first cloned | ||
| # https://github.com/Abandon-ht/axcl_bsp_sdk | ||
| # and set the environment variable SHERPA_ONNX_AXERA_PATH | ||
| # and set the environment variable SHERPA_ONNX_AXCL_SDK_ROOT |
There was a problem hiding this comment.
Typo in comment: should be SHERPA_ONNX_AXCL_SDK_ROOT instead of SHERPA_ONNX_ASCL_SDK_ROOT (missing 'X'). This matches the actual environment variable name used in the script.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
build-axcl-linux-aarch64.sh (1)
24-31: Comment mentions different env var name than the script actually usesThe comment says to set
SHERPA_ONNX_AXCL_SDK_ROOT, but the script checks and usesAXCL_SDK_ROOTeverywhere (lines 26–31, 33–39, 43–45). That mismatch is likely to confuse users.I’d recommend aligning them one way or the other; e.g. either:
- Keep
AXCL_SDK_ROOTas the canonical variable and update the comment:-# and set the environment variable SHERPA_ONNX_AXCL_SDK_ROOT +# and set the environment variable AXCL_SDK_ROOTor
- Rename the script’s variable to
SHERPA_ONNX_AXCL_SDK_ROOTconsistently.
🧹 Nitpick comments (3)
build-axcl-linux-aarch64.sh (1)
117-118: Consider making the parallel job count configurable instead of hard‑coded-j2Using
-j2is safe but can be unnecessarily slow on larger machines. If you want slightly more flexibility without losing control, consider something like:-make VERBOSE=1 -j2 +MAKE_JOBS="${MAKE_JOBS:-2}" +make VERBOSE=1 -j"${MAKE_JOBS}"So users can override
MAKE_JOBSwhen they know their hardware limits.sherpa-onnx/csrc/offline-recognizer-sense-voice-tpl-impl.h (2)
90-115: Early‑return on empty logits: confirm intended stream result semantics
DecodeOneStreamnow returns immediately iflogitsis empty:std::vector<float> logits = model_->Run(std::move(f), language, text_norm); if (logits.empty()) { return; }This avoids computing
num_out_framesand calling the decoder with zero frames, which is sensible. However, it also means theOfflineStream’s result is left as-is (likely default/previous) andSetResultis never called.If
DecodeStreamsis only ever invoked once per stream and an empty logits output semantically means “no speech / no hypothesis”, this is fine. If you expect an explicit empty result to be set in that case, you might instead construct a defaultOfflineRecognitionResultand calls->SetResult(...).
81-88: Use of a localconfig_copy is consistent with prior pattern but can diverge from base config
InitFeatConfig()updatesconfig_.feat_config, andCreateStream()uses this localconfig_rather than the base class’s config. This mirrors the prior SenseVoice impl pattern, but note that calls toOfflineRecognizerImpl::SetConfig()only update the base class’s config, not this local copy.If dynamic reconfiguration via
SetConfig()is not used for SenseVoice, this is fine. Otherwise, consider delegating feature access entirely to the base class config to avoid divergence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build-axcl-linux-aarch64.sh(2 hunks)sherpa-onnx/csrc/ascend/offline-recognizer-sense-voice-ascend-impl.h(0 hunks)sherpa-onnx/csrc/axcl/offline-recognizer-sense-voice-axcl-impl.h(0 hunks)sherpa-onnx/csrc/axera/offline-recognizer-sense-voice-axera-impl.h(0 hunks)sherpa-onnx/csrc/offline-recognizer-impl.cc(12 hunks)sherpa-onnx/csrc/offline-recognizer-sense-voice-tpl-impl.h(5 hunks)sherpa-onnx/csrc/rknn/offline-recognizer-sense-voice-rknn-impl.h(0 hunks)
💤 Files with no reviewable changes (4)
- sherpa-onnx/csrc/rknn/offline-recognizer-sense-voice-rknn-impl.h
- sherpa-onnx/csrc/axcl/offline-recognizer-sense-voice-axcl-impl.h
- sherpa-onnx/csrc/axera/offline-recognizer-sense-voice-axera-impl.h
- sherpa-onnx/csrc/ascend/offline-recognizer-sense-voice-ascend-impl.h
🧰 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/offline-recognizer-impl.ccsherpa-onnx/csrc/offline-recognizer-sense-voice-tpl-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:
build-axcl-linux-aarch64.sh
🧬 Code graph analysis (1)
sherpa-onnx/csrc/offline-recognizer-sense-voice-tpl-impl.h (3)
sherpa-onnx/csrc/offline-recognizer-impl.cc (4)
OfflineRecognizerImpl(696-747)OfflineRecognizerImpl(750-800)OfflineRecognizerImpl(829-830)OfflineRecognizerImpl(837-838)sherpa-onnx/csrc/offline-recognizer-impl.h (1)
std(36-40)sherpa-onnx/csrc/rknn/offline-ctc-greedy-search-decoder-rknn.h (1)
OfflineCtcGreedySearchDecoderRknn(14-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Release static tts-OFF
- GitHub Check: ubuntu-24.04 3.12
- GitHub Check: ubuntu-24.04 3.9
- GitHub Check: ubuntu-24.04 3.11
- GitHub Check: ubuntu-24.04 3.13
- GitHub Check: ubuntu-24.04 3.8
- GitHub Check: ubuntu-24.04 3.10
- GitHub Check: rknn shared OFF
- GitHub Check: rknn shared ON
- GitHub Check: Debug shared-ON tts-ON
- GitHub Check: Debug shared-ON tts-OFF
- GitHub Check: Debug shared-OFF tts-OFF
- GitHub Check: Release shared-ON tts-ON
- GitHub Check: Debug shared-OFF tts-ON
- GitHub Check: Release shared-OFF tts-OFF
- GitHub Check: Release shared-OFF tts-ON
- GitHub Check: Release shared-ON tts-OFF
- GitHub Check: swift (macos-13)
- GitHub Check: swift (macos-latest)
🔇 Additional comments (3)
sherpa-onnx/csrc/offline-recognizer-impl.cc (2)
34-62: SenseVoice templated wiring across providers looks consistentThe refactor to route SenseVoice through
OfflineRecognizerSenseVoiceTplImpl<OfflineSenseVoiceModelX>for all providers (rknn,axera,axcl,ascend,qnn) appears mechanically consistent:
- Each provider block includes its corresponding
offline-sense-voice-model-*.h.- Both
Create(const OfflineRecognizerConfig &)andCreate(Manager *mgr, ...)use the matchingOfflineSenseVoiceModelXtemplate parameter.- Existing non-SenseVoice branches (Paraformer, Zipformer CTC, etc.) are preserved as before.
Assuming each
OfflineSenseVoiceModel*exposes the same metadata andRunAPI as the original QNN model, this change should be behavior-preserving and removes the duplication nicely.Please double-check that all
OfflineSenseVoiceModel*classes share the same constructor signatures used here ((const OfflineModelConfig&)and(Manager*, const OfflineModelConfig&)) and compatibleGetModelMetadata()/Run()APIs so the template truly remains backend-agnostic.Also applies to: 71-74, 96-99, 119-121, 143-145, 170-172, 386-389, 411-413, 434-436, 458-460, 487-489
792-793: Minor formatting tweak to closing comments is fineThe updated closing comments on the
rule_farsloop andifblock keep style consistent with the rest of the file; no behavioral impact.sherpa-onnx/csrc/offline-recognizer-sense-voice-tpl-impl.h (1)
26-35: Template generalization of SenseVoice impl looks cleanThe new
OfflineRecognizerSenseVoiceTplImpl<SenseVoiceModel>nicely factors out backend differences:
SenseVoiceModelis constructed uniformly fromconfig.model_config(andmgrvariant), stored asstd::unique_ptr<SenseVoiceModel>.- Symbol table handling mirrors the non-manager and manager paths.
- Decoder setup still uses
OfflineCtcGreedySearchDecoderRknnbased only onblank_id, which is backend-agnostic in practice.InitFeatConfig()derives feature settings from the model metadata, similar to the previous QNN-specific flow.This should make it straightforward to plug in additional SenseVoice backends without duplicating recognizer glue code.
Also applies to: 48-55, 131-136
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.