Add C++ API with ACL C API for SenseVoice ASR on Ascend NPU - #2728
Conversation
dosubot
Bot
added
the
size:XL
|
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. WalkthroughThis PR migrates many GitHub Actions Windows runners to windows-2022, bumps several release tags and artifact upload steps, and adds Ascend NPU support: new CMake option, Ascend ACL RAII wrappers, Ascend SenseVoice model and recognizer implementations, and runtime integration for provider="ascend". Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Factory as OfflineRecognizerImpl
participant AscendRec as OfflineRecognizerSenseVoiceAscendImpl
participant Model as OfflineSenseVoiceModelAscend
participant ACL as Ascend ACL
App->>Factory: Create(config provider="ascend")
alt SHERPA_ONNX_ENABLE_ASCEND_NPU
Factory->>AscendRec: instantiate with config
AscendRec->>Model: Load / Init model
Model->>ACL: aclInit / create context / load model
Model-->>AscendRec: model ready (metadata)
App->>AscendRec: DecodeStreams(streams)
AscendRec->>AscendRec: DecodeOneStream(stream)
AscendRec->>Model: Run(features, language, text_norm)
Model->>ACL: allocate device buffers / aclmdlExecute
Model-->>AscendRec: logits
AscendRec->>AscendRec: decode logits -> text
AscendRec-->>App: recognition result
else not enabled
Factory-->>App: error + rebuild guidance
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds C++ API support for SenseVoice ASR on Ascend NPU using the ACL (Ascend Computing Language) C API. The implementation follows a similar pattern to the existing RKNN NPU support.
Key changes:
- Introduces new Ascend NPU backend with ACL C API wrappers for model loading, inference, and memory management
- Shares the CTC greedy search decoder implementation between RKNN and Ascend NPU backends
- Provides default values for SenseVoice model metadata to support both NPU backends
- Updates CI workflows to use windows-2022 runner instead of windows-latest
Reviewed Changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sherpa-onnx/csrc/ascend/*.{h,cc} | New Ascend NPU implementation with ACL API wrappers and SenseVoice model support |
| sherpa-onnx/csrc/offline-recognizer-impl.cc | Integration of Ascend provider into recognizer factory |
| sherpa-onnx/csrc/offline-sense-voice-model-meta-data.h | Default values for model metadata supporting NPU backends |
| CMakeLists.txt | Build configuration for Ascend NPU support |
| sherpa-onnx/csrc/CMakeLists.txt | Source file organization for shared decoder and Ascend-specific files |
| .github/workflows/*.yaml | CI workflow updates to windows-2022 and version tag updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SHERPA_ONNX_LOGE( | ||
| "Only SenseVoice models are currently supported " | ||
| "by Ascend NPU for non-streaming ASR. Fallback to CPU"); | ||
| } else if (!config.model_config.sense_voice.model.empty()) { |
There was a problem hiding this comment.
The second condition is redundant. If line 70 checks that model is empty, line 74 checking that model is not empty is unnecessary since it's already guaranteed by the else branch. Remove the explicit condition in the else if and use a simple else.
| } else if (!config.model_config.sense_voice.model.empty()) { | |
| } else { |
| if (config.model_config.sense_voice.model.empty()) { | ||
| SHERPA_ONNX_LOGE( | ||
| "Only SenseVoice models are currently supported " | ||
| "by Ascend NPU for non-streaming ASR. Fallback to CPU"); | ||
| } else if (!config.model_config.sense_voice.model.empty()) { |
There was a problem hiding this comment.
The second condition is redundant. If line 297 checks that model is empty, line 301 checking that model is not empty is unnecessary since it's already guaranteed by the else branch. Remove the explicit condition in the else if and use a simple else.
| aclFormat format = aclmdlGetOutputFormat(desc_->Get(), i); | ||
|
|
||
| os << " format: " << AclFormatToString(format) << "\n"; | ||
| aclDataType type = aclmdlGetInputDataType(desc_->Get(), i); |
There was a problem hiding this comment.
Using aclmdlGetInputDataType for output information. Should use aclmdlGetOutputDataType when retrieving data type for outputs.
| aclDataType type = aclmdlGetInputDataType(desc_->Get(), i); | |
| aclDataType type = aclmdlGetOutputDataType(desc_->Get(), i); |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| AclHostPtr(const AclDevicePtr &) = delete; | ||
| const AclHostPtr &operator=(const AclDevicePtr &) = delete; |
There was a problem hiding this comment.
The deleted copy constructor and assignment operator parameters should be AclHostPtr instead of AclDevicePtr. This appears to be a copy-paste error from the AclDevicePtr class.
| AclHostPtr(const AclDevicePtr &) = delete; | |
| const AclHostPtr &operator=(const AclDevicePtr &) = delete; | |
| AclHostPtr(const AclHostPtr &) = delete; | |
| const AclHostPtr &operator=(const AclHostPtr &) = delete; |
| repo_name: k2-fsa/sherpa-onnx | ||
| repo_token: ${{ secrets.UPLOAD_GH_SHERPA_ONNX_TOKEN }} | ||
| tag: v1.12.15 |
There was a problem hiding this comment.
Previously commented-out configuration lines have been uncommented. Ensure that the UPLOAD_GH_SHERPA_ONNX_TOKEN secret is properly configured in the repository settings.
| repo_name: k2-fsa/sherpa-onnx | ||
| repo_token: ${{ secrets.UPLOAD_GH_SHERPA_ONNX_TOKEN }} | ||
| tag: v1.12.15 |
There was a problem hiding this comment.
New release configuration added. Ensure that the UPLOAD_GH_SHERPA_ONNX_TOKEN secret is properly configured in the repository settings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-wheels-win64.yaml (1)
268-268: Update build_wheels_win64 job to use windows-2022 for consistency.The
build_wheels_win64job at line 268 useswindows-latestwhile thecoreandtestjobs in the same workflow usewindows-2022. This inconsistency can lead to build issues when different OS versions run related jobs in the same workflow.- os: [windows-latest] + os: [windows-2022]
🧹 Nitpick comments (10)
sherpa-onnx/csrc/keyword-spotter-impl.cc (1)
8-8: Use SHERPA_ONNX_EXIT consistently and add an unreachable return.You already include macros.h. Prefer the project macro over raw exit for consistency and to satisfy static analyzers.
Apply outside this hunk near Line 71:
- SHERPA_ONNX_LOGE("Please specify a model"); - SHERPA_ONNX_EXIT(-1); + SHERPA_ONNX_LOGE("Please specify a model"); + SHERPA_ONNX_EXIT(-1); + return nullptr; // unreachable, silences compilerssherpa-onnx/csrc/rknn/offline-recognizer-sense-voice-rknn-impl.h (1)
1-1: Guard logits shape and verify metadata assumptions.Add a check before dividing to avoid silent truncation; also confirm frame_shift and subsampling source.
Apply outside this hunk near Lines 113-121:
- std::vector<float> logits = model_->Run(std::move(f), language, text_norm); - int32_t num_out_frames = logits.size() / meta_data.vocab_size; + std::vector<float> logits = model_->Run(std::move(f), language, text_norm); + SHERPA_ONNX_CHECK(logits.size() % static_cast<size_t>(meta_data.vocab_size) == 0, + "logits.size() (%zu) is not a multiple of vocab_size (%d)", + logits.size(), meta_data.vocab_size); + int32_t num_out_frames = static_cast<int32_t>(logits.size() / meta_data.vocab_size);Please verify:
- frame_shift_ms hard-coded to 10 (Line 119): is this guaranteed by SenseVoice metadata?
- subsampling_factor from meta_data.window_shift (Line 120): confirm this indeed encodes subsampling factor for RKNN SenseVoice.
sherpa-onnx/csrc/ascend/macros.h (1)
10-19: Include ACL header and null-check error message to avoid UB.The macro references ACL_ERROR_NONE and aclGetRecentErrMsg. Without including acl/acl.h here, include-order matters. Also, aclGetRecentErrMsg may return nullptr.
-#include "sherpa-onnx/csrc/macros.h" +#include "sherpa-onnx/csrc/macros.h" +#include <acl/acl.h> -#define SHERPA_ONNX_ASCEND_CHECK(ret, msg, ...) \ +#define SHERPA_ONNX_ASCEND_CHECK(ret, msg, ...) \ do { \ if (ret != ACL_ERROR_NONE) { \ - const char *_msg = aclGetRecentErrMsg(); \ + const char *_msg = aclGetRecentErrMsg(); \ + const char *_safe = _msg ? _msg : "unknown"; \ SHERPA_ONNX_LOGE("Return code is: %d", ret); \ - SHERPA_ONNX_LOGE("Error message: %s", _msg); \ + SHERPA_ONNX_LOGE("Error message: %s", _safe);\ SHERPA_ONNX_LOGE(msg, ##__VA_ARGS__); \ SHERPA_ONNX_EXIT(-1); \ } \ } while (0)sherpa-onnx/csrc/ascend/utils.h (3)
21-23: Minor: assignment operator delete signature.Prefer returning non-const reference for deleted operator= to match convention.
- const Acl &operator=(const Acl &) = delete; + Acl &operator=(const Acl &) = delete;
38-40: Make conversion operator const.Safe and conventional; mirrors Get() const.
- operator aclrtContext() { return context_; } + operator aclrtContext() const { return context_; }
91-99: AclModelDesc::Size() returns unused, never-initialized size_.As written, Size() always returns 0. Either populate size_ (e.g., sum of input sizes or model size if available) or remove the method to avoid misleading APIs.
Is Size() needed by callers? If not, drop it; otherwise, implement using ACL query APIs.
sherpa-onnx/csrc/CMakeLists.txt (1)
307-313: Optional: prefer imported targets/find_library and add rpath for ascend libs.Using raw -L/-l works but is brittle. Consider find_library(ASCENDCL ...) and target_link_libraries(sherpa-onnx-core PRIVATE ${ASCENDCL}). Also add an rpath entry for ${ASCEND_TOOLKIT_HOME}/lib64 for installed binaries when needed.
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.h (1)
19-20: Ensure ACL is initialized in Impl before any aclrt calls.*Please confirm Impl holds an RAII Acl member (e.g., sherpa_onnx::Acl acl_;) constructed before PreInit(). Without aclInit/aclFinalize, aclrtSetDevice/LoadModel may fail.
I can draft the minimal RAII wiring in Impl if helpful.
sherpa-onnx/csrc/ascend/offline-recognizer-sense-voice-ascend-impl.h (2)
119-123: Optional: clarify time-scaling parameter.You’re using window_shift as subsampling_factor for timestamps; add a brief comment or use meta_data.subsampling_factor if that’s the intended knob.
37-44: Optional: decoder naming mismatch.The greedy decoder lives under rknn/ but is backend-agnostic. Consider moving/renaming to a neutral path to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
-
.github/workflows/build-wheels-win64.yaml(1 hunks) -
.github/workflows/export-paraformer-to-ascend-npu.yaml(2 hunks) -
.github/workflows/flutter-windows-x64.yaml(2 hunks) -
.github/workflows/jar.yaml(6 hunks) -
.github/workflows/lazarus.yaml(4 hunks) -
.github/workflows/mfc.yaml(4 hunks) -
.github/workflows/pascal.yaml(4 hunks) -
.github/workflows/test-build-wheel.yaml(1 hunks) -
.github/workflows/test-dart-package.yaml(1 hunks) -
.github/workflows/test-dart.yaml(2 hunks) -
.github/workflows/test-dot-net-nuget.yaml(1 hunks) -
.github/workflows/test-go-package.yaml(15 hunks) -
.github/workflows/test-go.yaml(3 hunks) -
.github/workflows/test-nodejs-addon-api.yaml(2 hunks) -
.github/workflows/test-nodejs-addon-npm-win-x86.yaml(1 hunks) -
.github/workflows/test-nodejs-addon-npm.yaml(1 hunks) -
.github/workflows/test-piper-phonemize.yaml(3 hunks) -
.github/workflows/test-python-offline-websocket-server.yaml(3 hunks) -
.github/workflows/test-python-online-websocket-server.yaml(1 hunks) -
.github/workflows/windows-arm64.yaml(1 hunks) -
.github/workflows/windows-x64-cuda.yaml(3 hunks) -
.github/workflows/windows-x64-debug.yaml(1 hunks) -
.github/workflows/windows-x64.yaml(2 hunks) -
.github/workflows/windows-x86-debug.yaml(1 hunks) -
.github/workflows/windows-x86.yaml(2 hunks) -
CMakeLists.txt(3 hunks) -
sherpa-onnx/csrc/CMakeLists.txt(2 hunks) -
sherpa-onnx/csrc/ascend/macros.h(1 hunks) -
sherpa-onnx/csrc/ascend/offline-recognizer-sense-voice-ascend-impl.h(1 hunks) -
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc(1 hunks) -
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.h(1 hunks) -
sherpa-onnx/csrc/ascend/utils.cc(1 hunks) -
sherpa-onnx/csrc/ascend/utils.h(1 hunks) -
sherpa-onnx/csrc/keyword-spotter-impl.cc(1 hunks) -
sherpa-onnx/csrc/offline-recognizer-impl.cc(4 hunks) -
sherpa-onnx/csrc/offline-sense-voice-model-meta-data.h(2 hunks) -
sherpa-onnx/csrc/rknn/macros.h(1 hunks) -
sherpa-onnx/csrc/rknn/offline-recognizer-sense-voice-rknn-impl.h(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-06T04:18:47.981Z
Learnt from: litongjava
PR: k2-fsa/sherpa-onnx#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:
.github/workflows/jar.yaml
📚 Learning: 2025-08-06T04:23:50.237Z
Learnt from: litongjava
PR: k2-fsa/sherpa-onnx#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:
.github/workflows/jar.yaml
🧬 Code graph analysis (6)
sherpa-onnx/csrc/offline-recognizer-impl.cc (2)
nodejs-addon-examples/test_asr_streaming_transducer_itn.js (1)
config(6-28)nodejs-addon-examples/test_asr_streaming_transducer_microphone_itn.js (1)
config(9-37)
sherpa-onnx/csrc/ascend/offline-recognizer-sense-voice-ascend-impl.h (4)
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.h (2)
sherpa_onnx(13-41)OfflineSenseVoiceModelAscend(15-39)sherpa-onnx/csrc/offline-sense-voice-model-meta-data.h (1)
sherpa_onnx(11-48)sherpa-onnx/csrc/offline-recognizer-sense-voice-impl.h (1)
ConvertSenseVoiceResult(25-59)sherpa-onnx/csrc/rknn/offline-ctc-greedy-search-decoder-rknn.h (1)
OfflineCtcGreedySearchDecoderRknn(14-24)
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.h (3)
sherpa-onnx/csrc/ascend/offline-recognizer-sense-voice-ascend-impl.h (1)
sherpa_onnx(20-134)sherpa-onnx/csrc/offline-sense-voice-model-meta-data.h (1)
sherpa_onnx(11-48)sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc (13)
OfflineSenseVoiceModelAscend(199-201)OfflineSenseVoiceModelAscend(203-203)OfflineSenseVoiceModelAscend(216-217)OfflineSenseVoiceModelAscend(221-222)Run(205-208)Run(205-206)features(43-95)features(43-44)GetModelMetadata(210-213)GetModelMetadata(211-211)Impl(23-27)Impl(23-23)Impl(30-37)
sherpa-onnx/csrc/ascend/utils.cc (2)
sherpa-onnx/csrc/ascend/utils.h (16)
Acl(16-26)AclContext(28-43)AclContext(31-31)Get(55-55)Get(74-74)Get(91-91)Get(159-159)Get(174-174)Get(190-190)AclDevicePtr(45-63)AclHostPtr(65-80)AclModelDesc(82-99)AclModel(101-146)AclMdlDataset(148-164)AclDataBuffer(166-179)AclTensorDesc(181-195)sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc (3)
device_id(114-124)data(106-112)data(106-106)
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc (1)
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.h (1)
OfflineSenseVoiceModelAscend(15-39)
sherpa-onnx/csrc/ascend/utils.h (2)
sherpa-onnx/csrc/ascend/utils.cc (37)
Acl(97-101)Acl(103-108)AclContext(110-113)AclContext(115-120)Get(122-122)Get(122-122)AclDevicePtr(124-133)AclDevicePtr(135-140)AclHostPtr(142-150)AclHostPtr(152-157)AclModelDesc(159-168)AclModelDesc(170-175)AclModel(177-184)AclModel(186-191)AclModel(193-198)GetInfo(265-325)GetInfo(265-265)Init(200-208)Init(200-200)InitInputNames(210-218)InitInputNames(210-210)InitInputShapes(220-236)InitInputShapes(220-220)InitOutputNames(238-245)InitOutputNames(238-238)InitOutputShapes(247-263)InitOutputShapes(247-247)AclMdlDataset(327-333)AclMdlDataset(335-340)AddBuffer(342-345)AddBuffer(342-342)SetTensorDesc(347-353)SetTensorDesc(347-348)AclDataBuffer(355-362)AclDataBuffer(364-369)AclTensorDesc(371-378)AclTensorDesc(380-384)sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc (3)
device_id(114-124)data(106-112)data(106-106)
⏰ 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). (18)
- GitHub Check: ubuntu-latest Debug static tts-OFF
- GitHub Check: Debug shared-ON tts-ON
- GitHub Check: Release shared-OFF tts-ON
- GitHub Check: ubuntu-latest Debug shared tts-ON
- GitHub Check: Release shared tts-OFF
- GitHub Check: Debug shared-ON tts-OFF
- GitHub Check: Release shared-ON tts-OFF
- GitHub Check: Debug shared-OFF tts-OFF
- GitHub Check: Release shared-OFF tts-OFF
- GitHub Check: Debug shared-OFF tts-ON
- GitHub Check: Debug shared tts-OFF
- GitHub Check: ubuntu-24.04 3.9
- GitHub Check: ubuntu-24.04 3.11
- GitHub Check: ubuntu-24.04 3.10
- GitHub Check: ubuntu-24.04 3.12
- GitHub Check: ubuntu-24.04 3.13
- GitHub Check: rknn shared OFF
- GitHub Check: rknn shared ON
🔇 Additional comments (46)
.github/workflows/test-piper-phonemize.yaml (1)
24-24: LGTM! CI infrastructure upgrade with consistent pinning.The update to explicitly use
windows-2022across all references (matrix definition, exclude rules, and conditional checks) improves CI stability by pinning to a known Windows runner version rather than relying on the latest. All references are consistent and the test logic is preserved correctly—non-Windows tests run conditionally on non-Windows runners, and Windows tests run onwindows-2022.Also applies to: 28-28, 70-70, 82-82
.github/workflows/flutter-windows-x64.yaml (1)
23-23: LGTM: Windows runner standardization.Both jobs consistently updated from
windows-latesttowindows-2022. No conditional logic changes needed in this workflow.Also applies to: 128-128
.github/workflows/test-python-online-websocket-server.yaml (1)
29-29: LGTM: Windows runner update.Matrix OS correctly updated to
windows-2022. Maintains multi-platform test coverage..github/workflows/windows-x86.yaml (2)
39-39: LGTM: Windows runner update.Matrix OS correctly updated to
windows-2022.
153-153: LGTM: Release tag update.Tag updated to
v1.12.15for GitHub release uploads, consistent with PR objectives to bump release tags to v1.12.15..github/workflows/test-build-wheel.yaml (1)
83-84: LGTM: Python 3.13 test matrix for Windows.Added
windows-2022+ Python 3.13 to the test matrix, consistent with existing Windows entries and the broader Windows runner standardization..github/workflows/windows-x86-debug.yaml (1)
33-33: LGTM: Windows runner update for debug builds.Matrix OS correctly updated to
windows-2022. Maintains debug build type variants..github/workflows/test-nodejs-addon-npm.yaml (1)
32-32: LGTM: Windows runner standardized in Node.js addon tests.Matrix OS updated to include
windows-2022. Maintains comprehensive multi-platform coverage..github/workflows/test-go.yaml (1)
27-27: LGTM: Consistent windows-2022 standardization across matrix and conditionals.All three OS references updated consistently: matrix definition (line 27) and conditional checks (lines 81, 95). Platform-specific build logic is properly preserved.
Also applies to: 81-81, 95-95
.github/workflows/windows-x64-debug.yaml (1)
33-33: LGTM: Windows runner update for x64 debug builds.Matrix OS correctly updated to
windows-2022. Mirrors the x86 debug workflow changes..github/workflows/test-nodejs-addon-npm-win-x86.yaml (1)
33-33: LGTM: Align Windows runner to windows-2022.This infrastructure update aligns with the broader CI migration across the repository and carries no impact on test logic or behavior.
.github/workflows/windows-x64.yaml (2)
39-39: LGTM: Align Windows runner to windows-2022.Infrastructure update consistent with broader CI migration.
148-150: Verify that hardcoded release tag v1.12.15 is intentional.This section hardcodes the release tag as
v1.12.15instead of computing it from CMakeLists.txt like other sections in the same file (e.g., lines 84, 118). Confirm this reflects the intended release version and is not stale..github/workflows/windows-arm64.yaml (1)
27-27: LGTM: Align Windows runner to windows-2022.Routine infrastructure update.
.github/workflows/test-dot-net-nuget.yaml (1)
28-28: LGTM: Include windows-2022 in multi-OS test matrix.Aligns with cross-repository CI standardization.
.github/workflows/export-paraformer-to-ascend-npu.yaml (1)
170-170: LGTM: Parameterize directory naming for multi-SOC support.Good improvement—the parameterized
${soc_version}variable allows the script to handle both 910B and 310P3 SOC versions dynamically, eliminating the hardcoded 910B reference and aligning with the matrix configuration..github/workflows/test-dart-package.yaml (1)
26-26: LGTM: Update commented OS suggestion to windows-2022.Documentation update that keeps the commented example consistent with current CI practices. No functional impact since the line remains disabled.
.github/workflows/lazarus.yaml (4)
33-33: LGTM: Update Windows runner to windows-2022.Infrastructure update aligned with other workflows.
100-100: LGTM: Update conditional to match new OS label.Correctly updates the conditional check to reference the new
windows-2022matrix OS value.
176-176: LGTM: Update artifact upload conditional for Windows x64 collection.Properly aligns the conditional to match the updated Windows matrix OS label.
237-237: LGTM: Update artifact upload conditional for Windows x64 artifacts.Correctly updates the upload gate to use the new
windows-2022label..github/workflows/build-wheels-win64.yaml (1)
29-29: LGTM: Update core job Windows runner to windows-2022.Aligns with cross-repository CI standardization.
.github/workflows/test-nodejs-addon-api.yaml (1)
81-113: Consistent Windows runner update applied correctly.The conditionals on lines 81 and 113 properly branch on
windows-2022vs. other OSes, splitting the build to use Visual Studio's MSBuild (Windows-specific) vs. Ninja (cross-platform). No functional changes to the workflow logic..github/workflows/test-dart.yaml (1)
27-27: Windows Dart testing intentionally disabled.Line 27 comments out
windows-2022from the test matrix, which makes the Windows-specific logic on line 92 a harmless dead code path. This is likely intentional due to Dart/Flutter environment constraints on Windows CI runners.Also applies to: 92-92
.github/workflows/test-python-offline-websocket-server.yaml (1)
29-29: Windows runner added; paraformer tests appropriately skipped on Windows.Line 29 adds
windows-2022to the test matrix; lines 101 and 116 then skip paraformer model tests specifically on Windows (likely due to library/model availability constraints). The conditional logic is consistent and intentional.Also applies to: 101-101, 116-116
.github/workflows/windows-x64-cuda.yaml (1)
100-109: Release artifact upload logic correctly differentiates between repository owners.Lines 100–109 upload artifacts for
csukuangfjowner tok2-fsa/sherpa-onnxwith explicit tagv1.12.15; lines 111–117 handlek2-fsaowner releases to the current repository. This dual-upload pattern allows personal mirror releases while maintaining org releases. No issues detected.Also applies to: 111-117
.github/workflows/jar.yaml (1)
38-38: Windows runner and release tag updates applied consistently across JAR build workflow.All references to
windows-latestreplaced withwindows-2022(lines 38, 129, 184, 239, 248). Release tag updated tov1.12.15on line 221. Path separator logic on line 248 correctly uses Windows semicolon. No issues detected.Also applies to: 129-129, 184-184, 221-221, 239-239, 248-248
.github/workflows/pascal.yaml (1)
30-30: Windows runner update applied consistently with appropriate test exclusions.Lines 30, 63, 109 correctly reference
windows-2022for compiler install, DLL copy, and streaming tests. Line 150 appropriately excludes Windows from paraformer/CTC HLG tests using!=. Pattern is consistent across the workflow.Also applies to: 63-63, 109-109, 150-150
.github/workflows/mfc.yaml (1)
29-29: MFC release uploads configured for all three binary types with consistent versioning.Matrix set to Windows-only (line 29). Three release upload steps (lines 128–152) handle streaming, non-streaming, and TTS binaries, each with matching
repo_name: k2-fsa/sherpa-onnx, token, andtag: v1.12.15. Gating condition on line 122 ensures uploads only occur for authorized owners on tag pushes. Configuration is correct.Also applies to: 128-152
.github/workflows/test-go-package.yaml (1)
35-38: Go test workflow comprehensively updated for Windows 2022 with architecture-specific handling.Matrix lines 35–38 include both
windows-2022x64 and x86 entries. MinGW setup (lines 62, 68, 75) gates on architecture. Linux/macOS-only tests (lines 80, 88, 95, 102, 109, 153, 160, 374, 487, 500) use!= 'windows-2022'to exclude Windows. Windows-specific tests (lines 116, 167, 188, 224, 274, 320, 405, 442, 519, 544) properly check both OS and architecture. Pattern is comprehensive and consistent.Also applies to: 62-62, 68-68, 75-75, 80-80, 88-88, 95-95, 102-102, 109-109, 153-153, 160-160, 167-167, 188-188, 224-224, 274-274, 320-320, 374-374, 405-405, 442-442, 487-487, 500-500, 519-519, 544-544
CMakeLists.txt (1)
63-64: ****The Ascend toolkit wiring is already properly configured in
sherpa-onnx/csrc/CMakeLists.txt(lines 307–313). The implementation includes:
- Include directories:
target_include_directories(sherpa-onnx-core PRIVATE ${ASCEND_TOOLKIT_HOME}/include)(line 308)- Link directories and libraries:
-L${ASCEND_TOOLKIT_HOME}/lib64and-lascendcl(lines 310–311)The code already uses target-scoped configuration via
target_include_directoriesandtarget_link_librariesas recommended. No additional wiring or refactoring is needed.Likely an incorrect or invalid review comment.
sherpa-onnx/csrc/offline-recognizer-impl.cc (1)
42-44: Ascend provider integration verified—all plumbing correct.
- Class
OfflineRecognizerSenseVoiceAscendImplpresent and properly included.- Provider string checks uniform across both
Create()overloads (lines 68 and 295).- CMake build system correctly wires Ascend libraries:
target_link_librariesincludes-L${ASCEND_TOOLKIT_HOME}/lib64 -lascendcl.- Mirror of RKNN path is consistent and well-structured.
- Minor: the "Fallback to CPU" message only logs; consider clarifying it if no CPU SenseVoice model is provided.
sherpa-onnx/csrc/rknn/macros.h (1)
10-17: Consider defensive include-guard but current code is safe due to transitive includes.Your concern about include-order fragility is valid in principle—
macros.hrelies onRKNN_SUCC(from externalrknn_api.h) without explicitly including it. However, all current uses work because model header files (e.g.,offline-sense-voice-model-rknn.h,utils.h) includerknn_api.hbeforemacros.his included, satisfying the dependency transitively.The risk emerges only if:
- A new file includes
macros.hdirectly without first including a header that hasrknn_api.h- Include order changes
Recommendation: Adding the guard is good defensive practice with no cost:
#ifndef RKNN_SUCC #error "Include <rknn_api.h> before sherpa-onnx/csrc/rknn/macros.h" #endifThis is preferable to including the vendor header in
macros.hbecause it avoids coupling and still ensures safety. Verify with your team whether this defensive pattern aligns with your project standards.sherpa-onnx/csrc/CMakeLists.txt (1)
195-201: Validation for ASCEND_TOOLKIT_HOME already exists in parent CMakeLists.txt.The root CMakeLists.txt (lines 305-334) already implements all the validations suggested in this review:
- Checks whether ASCEND_TOOLKIT_HOME is set or defaults to
/usr/local/Ascend/ascend-toolkit/latest- Validates
acl.hexists withFATAL_ERRORif missing (line 328-329)- Validates
libascendcl.soexists withFATAL_ERRORif missing (line 332-333)Since this validation occurs at the root level before
sherpa-onnx/csrc/CMakeLists.txtis processed, the variable is guaranteed to be valid when used in lines 307-310 of that file. No additional validation is needed in the csrc subdirectory.Likely an incorrect or invalid review comment.
sherpa-onnx/csrc/ascend/utils.cc (6)
13-95: LGTM!The enum-to-string conversion functions are comprehensive and provide good diagnostic support for ACL data types and formats.
97-122: LGTM!The RAII wrappers for ACL initialization and context management follow best practices with proper error checking and cleanup.
124-157: LGTM!The device and host memory wrappers correctly handle allocation, deallocation, and zero-size edge cases with appropriate RAII semantics.
159-175: LGTM!The model descriptor wrapper properly checks for allocation failures and manages the lifecycle correctly.
177-263: LGTM!The model loading and metadata initialization logic is well-structured with proper resource management and efficient use of move semantics.
327-384: LGTM!The dataset, buffer, and tensor descriptor wrappers correctly implement RAII patterns with appropriate error handling for ACL operations.
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc (6)
21-41: LGTM!The constructor initialization sequence (PreInit → InitModel → PostInit) provides a clean and maintainable structure for setting up the model on Ascend NPU.
43-95: LGTM!The inference pipeline correctly handles feature preprocessing, dynamic shape configuration, model execution, and result retrieval with appropriate error checking at each step.
114-124: Device selection is hardcoded.Line 115 hardcodes
device_id = 0, which is reasonable for the initial implementation. The setup and context management logic is correct.
143-179: LGTM!The low frame rate (LFR) processing implementation correctly handles frame windowing with appropriate truncation warnings for overly long inputs.
182-197: LGTM!The private member organization is clean with appropriate use of
unique_ptrfor RAII-managed ACL resources.
199-223: LGTM!The public interface correctly implements the pimpl pattern with proper delegation, and includes necessary template instantiations for Android and OHOS platforms.
| push: | ||
| branches: | ||
| - ascend-npu-310 | ||
| - ascend-npu-acl-api- |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Fix branch trigger typo—trailing dash will prevent workflow from running.
Line 6 specifies ascend-npu-acl-api- (with trailing dash), but the PR source branch is ascend-npu-acl-api (without dash). The workflow will never trigger on the correct branch.
branches:
- - ascend-npu-acl-api-
+ - ascend-npu-acl-api🏁 Script executed:
#!/bin/bash
# Inspect the workflow file to verify branch trigger on line 6
echo "=== Checking workflow file content ==="
cat .github/workflows/export-paraformer-to-ascend-npu.yaml | head -20
echo ""
echo "=== Checking line 6 specifically ==="
sed -n '6p' .github/workflows/export-paraformer-to-ascend-npu.yaml | od -c
echo ""
echo "=== Checking git branch information ==="
git branch -a
git rev-parse --abbrev-ref HEADLength of output: 727
🏁 Script executed:
#!/bin/bash
# Check git log and other workflow files for branch name patterns
echo "=== Searching for branch name references in workflows ==="
grep -r "ascend-npu-acl-api" .github/workflows/ --include="*.yaml" --include="*.yml"
echo ""
echo "=== Checking recent commits for context ==="
git log --oneline -10
echo ""
echo "=== Checking if branch ascend-npu-acl-api exists ==="
git branch -a | grep ascend-npuLength of output: 465
Critical: Fix branch trigger typo—trailing dash will prevent workflow from running.
Line 6 specifies ascend-npu-acl-api- (with trailing dash), but this branch does not exist in the repository. The workflow will never trigger. Remove the trailing dash to match the intended branch name ascend-npu-acl-api.
branches:
- - ascend-npu-acl-api-
+ - ascend-npu-acl-api📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - ascend-npu-acl-api- | |
| branches: | |
| - ascend-npu-acl-api |
🤖 Prompt for AI Agents
In .github/workflows/export-paraformer-to-ascend-npu.yaml around line 6, the
branch name in the workflow trigger includes an extraneous trailing dash
("ascend-npu-acl-api-") which doesn't match any branch and prevents the workflow
from running; remove the trailing dash so the branch reads "ascend-npu-acl-api"
(ensure there are no surrounding whitespace or YAML syntax changes).
| void InitModel(const std::string &filename) { | ||
| model_ = std::make_unique<AclModel>(filename); | ||
| if (config_.debug) { | ||
| auto s = model_->GetInfo(); | ||
| SHERPA_ONNX_LOGE("%s", s.c_str()); | ||
| } | ||
| } | ||
|
|
||
| void InitModel(void *data, size_t size) { | ||
| model_ = std::make_unique<AclModel>(data, size); | ||
| if (config_.debug) { | ||
| auto s = model_->GetInfo(); | ||
| SHERPA_ONNX_LOGE("%s", s.c_str()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider using appropriate log level for debug output.
Lines 102 and 110 use SHERPA_ONNX_LOGE (error level) to output model information when config_.debug is enabled. Debug or informational content should use a more appropriate log level (e.g., SHERPA_ONNX_LOGD or SHERPA_ONNX_LOGI if available) to avoid polluting error logs.
🤖 Prompt for AI Agents
In sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc around lines 98
to 112, the model info print uses the error-level macro SHERPA_ONNX_LOGE when
config_.debug is true; change these debug prints to the appropriate debug/info
log macro (e.g., SHERPA_ONNX_LOGD or SHERPA_ONNX_LOGI if LOGD is unavailable) so
model information is not logged as an error, keeping the message text and
variable usage unchanged.
| logits_ptr_ = std::make_unique<AclDevicePtr>((max_num_frames_ + 4) * | ||
| vocab_size_ * sizeof(float)); |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify the +4 buffer allocation for logits.
Line 139 allocates (max_num_frames_ + 4) * vocab_size_ for the logits buffer. The reason for the additional 4 frames is not immediately clear from the code. Please add a comment explaining whether this is a safety buffer, related to prompt processing, or required by the model architecture.
🏁 Script executed:
#!/bin/bash
# Search for documentation or comments explaining the logits buffer sizing
rg -n -C3 'max_num_frames_.*\+.*4|logits.*allocation|output.*buffer.*size' --type=cc --type=hLength of output: 26
🏁 Script executed:
#!/bin/bash
# Search for documentation about the +4 buffer allocation, using correct ripgrep types
rg -n -C3 'max_num_frames_.*\+.*4|logits.*allocation|output.*buffer' --type=cpp --type=hLength of output: 15683
🏁 Script executed:
#!/bin/bash
# Look for the specific file and examine the Preallocate function
fd 'offline-sense-voice-model-ascend' --type fLength of output: 118
🏁 Script executed:
#!/bin/bash
# Search for comments or documentation about buffer sizing in the offline sense voice model
rg -n 'buffer|allocation|max_num_frames' sherpa-onnx/csrc/ascend/ -C2 | head -50Length of output: 3575
🏁 Script executed:
#!/bin/bash
# Examine the header file for class definition and any documentation
cat sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.hLength of output: 1276
🏁 Script executed:
#!/bin/bash
# Search for sense-voice references to understand the model architecture
rg -n 'sense.voice|SenseVoice' --type=cpp --type=h -C2 | head -80Length of output: 6864
🏁 Script executed:
#!/bin/bash
# Look for similar +4 or buffer offset patterns in other model implementations
rg -n 'make_unique.*\+.*4|make_unique.*\+.*sizeof' sherpa-onnx/csrc/ascend/ --type=cpp --type=h -C1Length of output: 344
🏁 Script executed:
#!/bin/bash
# Look for non-Ascend SenseVoice implementations
fd 'offline.*sense.*voice' --type f | grep -v ascendLength of output: 1018
🏁 Script executed:
#!/bin/bash
# Search for documentation files that might explain model architecture
find . -name '*.md' -o -name '*.txt' -o -name 'README*' | xargs grep -l 'sense.voice\|SenseVoice' 2>/dev/null | head -10Length of output: 339
🏁 Script executed:
#!/bin/bash
# Search the Ascend implementation file more thoroughly for any comments near buffer allocations
cat sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc | sed -n '125,165p'Length of output: 1329
🏁 Script executed:
#!/bin/bash
# Check the standard CPU SenseVoice implementation
cat sherpa-onnx/csrc/offline-sense-voice-model.cc | grep -n -A20 -B5 'logits\|buffer\|Preallocate' | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for RKNN version as another reference implementation
cat sherpa-onnx/csrc/rknn/offline-sense-voice-model-rknn.cc | grep -n -A20 -B5 'logits\|buffer\|Preallocate' | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any comments about the +4 or buffer sizing near the logits allocation
rg -n 'logits|output.*buffer' sherpa-onnx/csrc/offline-sense-voice-model.cc sherpa-onnx/csrc/rknn/offline-sense-voice-model-rknn.cc -C3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Examine the full Ascend implementation file to understand logits usage
cat sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc | sed -n '140,220p'Length of output: 2336
🏁 Script executed:
#!/bin/bash
# Search for how logits_ptr_ is used after allocation
rg -n 'logits_ptr_' sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc -C2Length of output: 961
🏁 Script executed:
#!/bin/bash
# Check if there are any git history clues or annotations about this allocation
cd sherpa-onnx/csrc/ascend && git log -p --all -S 'max_num_frames_ + 4' -- offline-sense-voice-model-ascend.cc 2>/dev/null | head -50Length of output: 59
Add clarifying comment for the +4 buffer allocation on line 139.
The logits buffer is intentionally allocated with 4 extra frames beyond max_num_frames_ as an overflow safety margin. While the buffer is used with only num_frames * vocab_size_ bytes (lines 81–82 and 90), the allocation reserves extra capacity. Add an inline comment explaining this is a safety buffer to prevent accidental overflow.
logits_ptr_ = std::make_unique<AclDevicePtr>((max_num_frames_ + 4) *
vocab_size_ * sizeof(float));
Consider: // +4 frames as overflow safety margin
🤖 Prompt for AI Agents
In sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc around lines
139-140, the allocation for logits_ptr_ adds +4 to max_num_frames_ but has no
explanation; add an inline comment after the allocation expression clarifying
that the extra 4 frames are an intentional overflow safety margin to prevent
accidental buffer overruns when indexing (e.g., "// +4 frames as overflow safety
margin"), so future readers understand why capacity exceeds used num_frames *
vocab_size_.
| template <typename Manager> | ||
| OfflineSenseVoiceModelAscend(Manager *mgr, const OfflineModelConfig &config); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Templated constructor is declared but not defined; cc provides non-template overloads.
This will instantiate with no definition at call sites, causing link/instantiation errors.
Apply:
+#if defined(__ANDROID__)
+struct AAssetManager;
+#endif
+class NativeResourceManager;
@@
- template <typename Manager>
- OfflineSenseVoiceModelAscend(Manager *mgr, const OfflineModelConfig &config);
+ // Explicit overloads; keep in header to match .cc definitions.
+#if defined(__ANDROID__)
+ explicit OfflineSenseVoiceModelAscend(AAssetManager *mgr,
+ const OfflineModelConfig &config);
+#endif
+ explicit OfflineSenseVoiceModelAscend(NativeResourceManager *mgr,
+ const OfflineModelConfig &config);
+ // Prevent accidental template instantiation.
+ template <typename Manager>
+ OfflineSenseVoiceModelAscend(Manager *, const OfflineModelConfig &) = delete;If you prefer the template, move its definition into this header or add explicit template instantiations in a TU.
---
🏁 Script executed:
```shell
#!/bin/bash
# Locate the header and corresponding .cc file
fd -t f "offline-sense-voice-model-ascend" --maxdepth 3
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Examine the header file around lines 21-23
cat sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.h | head -40Length of output: 1176
🏁 Script executed:
#!/bin/bash
# Find the corresponding .cc file
find . -name "*offline-sense-voice-model-ascend.cc" -type fLength of output: 61
🏁 Script executed:
#!/bin/bash
# Check for any template definitions in header or implementation
rg -n "template.*OfflineSenseVoiceModelAscend" --type=h --type=cc -A 10Length of output: 26
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the .cc file to see constructor implementations
cat ./sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc
Length of output: 7126
🏁 Script executed:
#!/bin/bash
# Search for template definitions using correct syntax
rg -n "template" ./sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc -A 5Length of output: 607
🏁 Script executed:
#!/bin/bash
# Search for Manager-related types (AAssetManager, NativeResourceManager)
rg -n "AAssetManager|NativeResourceManager" ./sherpa-onnx/csrc/ascend/ -A 2 -B 2Length of output: 977
🏁 Script executed:
#!/bin/bash
# Search for forward declarations in header
rg -n "class.*Manager|struct.*Manager" ./sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.hLength of output: 0
Templated constructor is declared but not defined; explicit template instantiations in .cc have no corresponding definition.
The header declares the template constructor (lines 21-23) but provides no definition. The .cc file contains only explicit template instantiations (lines 216-222) without the template definition itself, causing linker/instantiation errors.
Replace with explicit overloads:
+#if defined(__ANDROID__)
+struct AAssetManager;
+#endif
+class NativeResourceManager;
+
explicit OfflineSenseVoiceModelAscend(const OfflineModelConfig &config);
- template <typename Manager>
- OfflineSenseVoiceModelAscend(Manager *mgr, const OfflineModelConfig &config);
+#if defined(__ANDROID__)
+ explicit OfflineSenseVoiceModelAscend(AAssetManager *mgr,
+ const OfflineModelConfig &config);
+#endif
+#if defined(__OHOS__)
+ explicit OfflineSenseVoiceModelAscend(NativeResourceManager *mgr,
+ const OfflineModelConfig &config);
+#endifRemove the explicit template instantiations from the .cc file (lines 215–223) and define these overloads there instead.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.h around lines
21-23, the templated constructor OfflineSenseVoiceModelAscend(Manager *mgr,
const OfflineModelConfig &config) is declared but not defined; the .cc contains
explicit template instantiations at lines ~215-223 with no template body.
Replace the template by declaring explicit non-template overloads for each
concrete Manager type used (e.g., OfflineSenseVoiceModelAscend(SpecificManagerA*
mgr, const OfflineModelConfig& config) and
OfflineSenseVoiceModelAscend(SpecificManagerB* mgr, const OfflineModelConfig&
config)) in the header, then remove the explicit template instantiation lines
from the .cc (lines ~215-223) and add corresponding definitions for those
overloads in the .cc file.
| AclHostPtr(const AclDevicePtr &) = delete; | ||
| const AclHostPtr &operator=(const AclDevicePtr &) = delete; | ||
|
|
There was a problem hiding this comment.
Wrong deleted copy signatures leave AclHostPtr copyable (double-free risk).
You’re deleting ctors/assignments taking AclDevicePtr, not AclHostPtr. The real copy ctor/assignment remain, risking double free.
Apply:
- AclHostPtr(const AclDevicePtr &) = delete;
- const AclHostPtr &operator=(const AclDevicePtr &) = delete;
+ AclHostPtr(const AclHostPtr &) = delete;
+ AclHostPtr &operator=(const AclHostPtr &) = delete;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AclHostPtr(const AclDevicePtr &) = delete; | |
| const AclHostPtr &operator=(const AclDevicePtr &) = delete; | |
| AclHostPtr(const AclHostPtr &) = delete; | |
| AclHostPtr &operator=(const AclHostPtr &) = delete; | |
🤖 Prompt for AI Agents
sherpa-onnx/csrc/ascend/utils.h around lines 71 to 73: the deleted copy ctor and
assignment currently take AclDevicePtr, leaving the real AclHostPtr copy
ctor/assignment intact and risking double-free; replace those deletions with the
correct signatures that take AclHostPtr (i.e., declare AclHostPtr(const
AclHostPtr&) = delete; and const AclHostPtr& operator=(const AclHostPtr&) =
delete;) so copies of AclHostPtr are disabled.
| std::string GetInfo() const; | ||
|
|
There was a problem hiding this comment.
GetInfo() implementation bugs (utils.cc): wrong APIs and fields, missing checks.
- For outputs, use aclmdlGetOutputDataType(), not GetInputDataType().
- After aclmdlGet(Input|Output)Dims, check return and don’t read uninitialized dims.
- aclmdlIODims has dims/dimCount; printing dims.name is invalid.
Fix in utils.cc:
@@ void AclModel::GetInfo() const {
- aclmdlIODims dims;
- aclError ret = aclmdlGetInputDims(desc_->Get(), i, &dims);
- os << " dim: " << dims.dimCount << "\n";
- for (size_t d = 0; d < dims.dimCount; ++d) {
- os << " " << d << " -> " << dims.name << ", " << dims.dims[d] << "\n";
- }
+ aclmdlIODims dims;
+ aclError ret = aclmdlGetInputDims(desc_->Get(), i, &dims);
+ SHERPA_ONNX_ASCEND_CHECK(ret, "Failed to call aclmdlGetInputDims");
+ os << " dim: " << dims.dimCount << "\n";
+ for (size_t d = 0; d < dims.dimCount; ++d) {
+ os << " dim[" << d << "] = " << dims.dims[d] << "\n";
+ }
@@
- aclDataType type = aclmdlGetInputDataType(desc_->Get(), i);
+ aclDataType type = aclmdlGetOutputDataType(desc_->Get(), i);
@@
- aclmdlIODims dims;
- aclError ret = aclmdlGetOutputDims(desc_->Get(), i, &dims);
- os << " dim: " << dims.dimCount << "\n";
- for (size_t d = 0; d < dims.dimCount; ++d) {
- os << " " << d << " -> " << dims.name << ", " << dims.dims[d] << "\n";
- }
+ aclmdlIODims dims;
+ aclError ret = aclmdlGetOutputDims(desc_->Get(), i, &dims);
+ SHERPA_ONNX_ASCEND_CHECK(ret, "Failed to call aclmdlGetOutputDims");
+ os << " dim: " << dims.dimCount << "\n";
+ for (size_t d = 0; d < dims.dimCount; ++d) {
+ os << " dim[" << d << "] = " << dims.dims[d] << "\n";
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In sherpa-onnx/csrc/ascend/utils.h around lines 113-114, the GetInfo()
implementation in utils.cc uses wrong APIs and reads invalid fields; update it
to call aclmdlGetOutputDataType() for outputs (keep aclmdlGetInputDataType() for
inputs), check the return value of aclmdlGetInputDims/aclmdlGetOutputDims before
using dims, and never access dims.name (use dims.dims array with dims.dimCount).
Add guards to avoid reading uninitialized dims on error, log or handle acl call
failures, and iterate only up to dimCount when printing dimensions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
sherpa-onnx/csrc/ascend/utils.cc (1)
313-313: Use correct function for output data type.Line 313 incorrectly calls
aclmdlGetInputDataTypewhile iterating over model outputs. This should beaclmdlGetOutputDataTypeto retrieve the correct data type for output tensors.Apply this diff:
- aclDataType type = aclmdlGetInputDataType(desc_->Get(), i); + aclDataType type = aclmdlGetOutputDataType(desc_->Get(), i);sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc (2)
99-113: Use appropriate log level for debug output.Lines 102-103 and 110-111 use
SHERPA_ONNX_LOGE(error level) for model information whenconfig_.debugis enabled. Debug output should use a more appropriate log level such asSHERPA_ONNX_LOGDorSHERPA_ONNX_LOGIto avoid polluting error logs.Apply this diff:
void InitModel(const std::string &filename) { model_ = std::make_unique<AclModel>(filename); if (config_.debug) { auto s = model_->GetInfo(); - SHERPA_ONNX_LOGE("%s", s.c_str()); + SHERPA_ONNX_LOGI("%s", s.c_str()); } } void InitModel(void *data, size_t size) { model_ = std::make_unique<AclModel>(data, size); if (config_.debug) { auto s = model_->GetInfo(); - SHERPA_ONNX_LOGE("%s", s.c_str()); + SHERPA_ONNX_LOGI("%s", s.c_str()); } }
140-141: Add comment explaining +4 buffer safety margin.The logits buffer allocation adds 4 extra frames beyond
max_num_frames_. While the actual usage (lines 82-83, 90) respects the truenum_frames * vocab_size_size, a comment explaining this is an overflow safety margin would clarify the intent.Apply this diff:
- logits_ptr_ = std::make_unique<AclDevicePtr>((max_num_frames_ + 4) * - vocab_size_ * sizeof(float)); + // +4 frames as overflow safety margin + logits_ptr_ = std::make_unique<AclDevicePtr>((max_num_frames_ + 4) * + vocab_size_ * sizeof(float));
🧹 Nitpick comments (2)
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc (2)
46-48: Consider validating frame size divisibility.Line 48 assumes
features.size()is perfectly divisible by 560. WhileApplyLFRshould guarantee this, an assertion or check would make the assumption explicit and catch potential upstream bugs.Consider adding a validation:
features = ApplyLFR(std::move(features)); +assert(features.size() % 560 == 0 && "Features must be divisible by 560"); int32_t num_frames = features.size() / 560;
134-134: Add comment explaining max_num_frames calculation.Line 134 uses a magic formula
(10 * 100 - 7) / 6 + 1to calculatemax_num_frames_. A brief comment explaining this formula (e.g., "10 seconds at 100 frames/sec with LFR params") would improve readability.Consider adding a comment:
- max_num_frames_ = (10 * 100 - 7) / 6 + 1; + max_num_frames_ = (10 * 100 - 7) / 6 + 1; // 10s audio: LFR with window=7, shift=6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc(1 hunks)sherpa-onnx/csrc/ascend/utils.cc(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
sherpa-onnx/csrc/ascend/utils.cc (2)
sherpa-onnx/csrc/ascend/utils.h (16)
Acl(16-26)AclContext(28-43)AclContext(31-31)Get(55-55)Get(74-74)Get(91-91)Get(159-159)Get(174-174)Get(190-190)AclDevicePtr(45-63)AclHostPtr(65-80)AclModelDesc(82-99)AclModel(101-146)AclMdlDataset(148-164)AclDataBuffer(166-179)AclTensorDesc(181-195)sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc (3)
device_id(115-125)data(107-113)data(107-107)
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc (1)
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.h (1)
OfflineSenseVoiceModelAscend(15-39)
⏰ 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). (20)
- GitHub Check: ubuntu-24.04 3.10
- GitHub Check: ubuntu-latest Debug shared tts-ON
- GitHub Check: macos-latest Debug shared tts-ON
- GitHub Check: macos-latest Debug static tts-OFF
- GitHub Check: ubuntu-latest Debug static tts-ON
- GitHub Check: macos-latest Debug static tts-ON
- GitHub Check: ubuntu-latest Release shared tts-ON
- GitHub Check: ubuntu-latest Release shared tts-OFF
- GitHub Check: ubuntu-latest Debug static tts-OFF
- GitHub Check: Release shared-OFF tts-OFF
- GitHub Check: Release shared-OFF tts-ON
- GitHub Check: Debug shared-OFF tts-ON
- GitHub Check: Debug shared-OFF tts-OFF
- GitHub Check: Debug shared-ON tts-ON
- GitHub Check: Release shared-ON tts-ON
- GitHub Check: Debug shared-ON tts-OFF
- GitHub Check: rknn shared OFF
- GitHub Check: rknn shared ON
- GitHub Check: swift (macos-13)
- GitHub Check: swift (macos-latest)
🔇 Additional comments (8)
sherpa-onnx/csrc/ascend/utils.cc (4)
13-95: LGTM! Comprehensive enum-to-string converters.The helper functions provide complete coverage of ACL data types and formats with appropriate fallback handling.
97-157: LGTM! Solid RAII wrappers for ACL resources.The resource management classes correctly implement initialization, cleanup, and error handling. The conditional checks prevent double-free, and zero-size allocations are handled safely.
200-263: LGTM! Model metadata initialization is correct.The initialization methods properly extract and store input/output names and shapes with appropriate error handling.
327-384: LGTM! Dataset and buffer wrappers are correctly implemented.The RAII wrappers for datasets, data buffers, and tensor descriptors follow the same robust pattern with proper error handling and resource cleanup.
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc (4)
19-39: LGTM! Impl initialization follows a clear sequence.The constructors properly orchestrate the initialization flow: device setup, model loading, and post-initialization. The template constructor correctly handles in-memory model data.
50-96: LGTM! Inference pipeline correctly orchestrates ACL operations.The memory copies, dataset construction, model execution, and result retrieval all use proper ACL APIs with consistent error handling. Buffer sizes are correctly aligned between allocation and usage.
144-180: LGTM! LFR processing correctly handles frame windowing and truncation.The
ApplyLFRmethod properly implements low-frame-rate processing with sliding windows. The truncation logic (lines 153-163) appropriately warns users when input exceedsmax_num_frames_and clips the output.
182-227: LGTM! Clean pimpl pattern and platform-specific instantiations.The private members are appropriately declared, the public interface correctly delegates to the impl, and the template instantiations properly support Android and OHOS platforms.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai please review |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc (2)
110-113: Use info/debug log level for model introspection, not error.Avoid polluting error logs when config_.debug is true.
- SHERPA_ONNX_LOGE("%s", s.c_str()); + SHERPA_ONNX_LOGI("%s", s.c_str());Also applies to: 118-121
142-151: Preallocate from metadata; document +4 extra frames.Keep parameters consistent with meta and clarify extra capacity.
- max_num_frames_ = (10 * 100 - 7) / 6 + 1; + // 10 s @ 100 fps after LFR; derive with metadata for consistency + max_num_frames_ = + (10 * 100 - meta_data_.window_size) / meta_data_.window_shift + 1; @@ - logits_ptr_ = std::make_unique<AclDevicePtr>((max_num_frames_ + 4) * - vocab_size_ * sizeof(float)); + // +4 frames as safety headroom for dynamic shape rounding + logits_ptr_ = std::make_unique<AclDevicePtr>((max_num_frames_ + 4) * + vocab_size_ * sizeof(float));sherpa-onnx/csrc/ascend/utils.cc (1)
274-279: Fix GetInfo(): wrong field access and missing error checks for dims.Must check aclmdlGet(Input|Output)Dims and don’t read dims.name (not valid). Use dims.dims[] with dimCount. Also print error via existing macro.
- aclmdlIODims dims; - aclError ret = aclmdlGetInputDims(desc_->Get(), i, &dims); - os << " dim: " << dims.dimCount << "\n"; - for (size_t d = 0; d < dims.dimCount; ++d) { - os << " " << d << " -> " << dims.name << ", " << dims.dims[d] << "\n"; - } + aclmdlIODims dims; + aclError ret = aclmdlGetInputDims(desc_->Get(), i, &dims); + SHERPA_ONNX_ASCEND_CHECK(ret, "Failed to call aclmdlGetInputDims"); + os << " dim: " << dims.dimCount << "\n"; + for (size_t d = 0; d < dims.dimCount; ++d) { + os << " dim[" << d << "] = " << dims.dims[d] << "\n"; + } @@ - aclmdlIODims dims; - aclError ret = aclmdlGetOutputDims(desc_->Get(), i, &dims); - os << " dim: " << dims.dimCount << "\n"; - for (size_t d = 0; d < dims.dimCount; ++d) { - os << " " << d << " -> " << dims.name << ", " << dims.dims[d] << "\n"; - } + aclmdlIODims dims; + aclError ret = aclmdlGetOutputDims(desc_->Get(), i, &dims); + SHERPA_ONNX_ASCEND_CHECK(ret, "Failed to call aclmdlGetOutputDims"); + os << " dim: " << dims.dimCount << "\n"; + for (size_t d = 0; d < dims.dimCount; ++d) { + os << " dim[" << d << "] = " << dims.dims[d] << "\n"; + }Also applies to: 299-304
🧹 Nitpick comments (4)
sherpa-onnx/csrc/ascend/utils.h (3)
21-22: Use canonical operator= signatures (drop const return type).Assignment operators should return T&, not const T&. While deleted, keeping canonical signatures avoids surprises and matches normal lookup rules.
Apply:
- const Acl &operator=(const Acl &) = delete; + Acl &operator=(const Acl &) = delete; @@ - const AclContext &operator=(const AclContext &) = delete; + AclContext &operator=(const AclContext &) = delete; @@ - const AclModelDesc &operator=(const AclModelDesc &) = delete; + AclModelDesc &operator=(const AclModelDesc &) = delete;Also applies to: 34-35, 70-71
37-39: Make conversion operator const.Allows use from const contexts and matches Get() const.
- operator aclrtContext() { return context_; } + operator aclrtContext() const { return context_; }
76-81: Remove unused size_/Size() or populate it.Currently always 0; dead API surface.
Options:
- Drop Size()/size_, or
- Set size_ to something meaningful (e.g., total IO count) and document it.
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc (1)
59-67: Minor: pass destination capacity via x_ptr_->Size() for consistency.Semantically identical but clearer and consistent with prompt/logits paths.
- aclError ret = - aclrtMemcpy(*x_ptr_, features.size() * sizeof(float), features.data(), - features.size() * sizeof(float), ACL_MEMCPY_HOST_TO_DEVICE); + aclError ret = aclrtMemcpy(*x_ptr_, x_ptr_->Size(), features.data(), + features.size() * sizeof(float), + ACL_MEMCPY_HOST_TO_DEVICE);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc(1 hunks)sherpa-onnx/csrc/ascend/utils.cc(1 hunks)sherpa-onnx/csrc/ascend/utils.h(1 hunks)sherpa-onnx/csrc/offline-sense-voice-model-meta-data.h(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
sherpa-onnx/csrc/ascend/utils.cc (2)
sherpa-onnx/csrc/ascend/utils.h (13)
Acl(16-26)AclContext(28-42)Get(54-54)Get(73-73)Get(141-141)Get(156-156)Get(172-172)AclDevicePtr(44-62)AclModelDesc(64-81)AclModel(83-128)AclMdlDataset(130-146)AclDataBuffer(148-161)AclTensorDesc(163-177)sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc (3)
device_id(124-134)data(116-122)data(116-116)
sherpa-onnx/csrc/ascend/utils.h (1)
sherpa-onnx/csrc/ascend/utils.cc (35)
Acl(97-101)Acl(103-108)AclContext(110-113)AclContext(115-120)Get(122-122)Get(122-122)AclDevicePtr(124-133)AclDevicePtr(135-140)AclModelDesc(142-151)AclModelDesc(153-158)AclModel(160-167)AclModel(169-174)AclModel(176-181)GetInfo(248-308)GetInfo(248-248)Init(183-191)Init(183-183)InitInputNames(193-201)InitInputNames(193-193)InitInputShapes(203-219)InitInputShapes(203-203)InitOutputNames(221-228)InitOutputNames(221-221)InitOutputShapes(230-246)InitOutputShapes(230-230)AclMdlDataset(310-316)AclMdlDataset(318-323)AddBuffer(325-328)AddBuffer(325-325)SetTensorDesc(330-336)SetTensorDesc(330-331)AclDataBuffer(338-345)AclDataBuffer(347-352)AclTensorDesc(354-361)AclTensorDesc(363-367)
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc (1)
sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.h (1)
OfflineSenseVoiceModelAscend(15-39)
⏰ 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 shared tts-OFF
- GitHub Check: Release static tts-OFF
- GitHub Check: Debug static tts-OFF
- GitHub Check: Debug shared tts-ON
- GitHub Check: ubuntu-24.04 3.8
- GitHub Check: ubuntu-latest Release static tts-ON
- GitHub Check: ubuntu-latest Debug shared tts-ON
- GitHub Check: ubuntu-latest Debug static tts-ON
- GitHub Check: ubuntu-latest Release shared tts-OFF
- GitHub Check: Debug shared-ON tts-ON
- GitHub Check: Release shared-ON tts-ON
- GitHub Check: Release shared-ON tts-OFF
- GitHub Check: Debug shared-ON tts-OFF
- GitHub Check: rknn shared OFF
- GitHub Check: Release shared-OFF tts-OFF
- GitHub Check: Debug shared-OFF tts-OFF
- GitHub Check: Debug shared-OFF tts-ON
- GitHub Check: Release shared-OFF tts-ON
- GitHub Check: rknn shared ON
🔇 Additional comments (1)
sherpa-onnx/csrc/offline-sense-voice-model-meta-data.h (1)
15-23: LGTM: sensible defaults and lang2id fix.Defaults align with SenseVoice; “ja” key looks correct. No issues spotted.
Also applies to: 42-44, 46-47
| void PreInit() { | ||
| int32_t device_id = 0; | ||
| aclError ret = aclrtSetDevice(device_id); | ||
| SHERPA_ONNX_ASCEND_CHECK( | ||
| ret, "Failed to call aclrtSetDevice with device id: %d", device_id); | ||
|
|
||
| context_ = std::make_unique<AclContext>(device_id); | ||
|
|
||
| ret = aclrtSetCurrentContext(*context_); | ||
| SHERPA_ONNX_ASCEND_CHECK(ret, "Failed to call aclrtSetCurrentContext"); | ||
| } |
There was a problem hiding this comment.
Reset device on teardown; match aclrtSetDevice in PreInit.
Add device_id_ member, store it in PreInit, and call aclrtResetDevice in Impl dtor after destroying context. Prevents leaks on Ascend.
class OfflineSenseVoiceModelAscend::Impl {
public:
+ ~Impl() {
+ // Destroy context before resetting device.
+ context_.reset();
+ aclError ret = aclrtResetDevice(device_id_);
+ SHERPA_ONNX_ASCEND_CHECK(ret, "Failed to call aclrtResetDevice");
+ }
@@
- void PreInit() {
- int32_t device_id = 0;
- aclError ret = aclrtSetDevice(device_id);
+ void PreInit() {
+ device_id_ = 0;
+ aclError ret = aclrtSetDevice(device_id_);
SHERPA_ONNX_ASCEND_CHECK(
- ret, "Failed to call aclrtSetDevice with device id: %d", device_id);
+ ret, "Failed to call aclrtSetDevice with device id: %d", device_id_);
- context_ = std::make_unique<AclContext>(device_id);
+ context_ = std::make_unique<AclContext>(device_id_);
@@
private:
+ int32_t device_id_ = 0;
std::mutex mutex_;
Acl acl_;Also applies to: 191-208
🤖 Prompt for AI Agents
In sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc around lines 124
to 134 (and also apply change at ~191-208), PreInit currently calls
aclrtSetDevice(0) but does not store the device id and the Impl destructor does
not call aclrtResetDevice, which can leak device resources; add a member int32_t
device_id_ to the Impl class, set device_id_ = device_id in PreInit after
determining the device, and in the Impl destructor, after destroying/freeing
context_ and before returning, call aclrtResetDevice(device_id_) and check its
return value (use existing SHERPA_ONNX_ASCEND_CHECK macro) to match the
aclrtSetDevice call and ensure the device is reset.
| int32_t in_num_frames = in.size() / in_feat_dim; | ||
| int32_t out_num_frames = | ||
| (in_num_frames - lfr_window_size) / lfr_window_shift + 1; | ||
|
|
There was a problem hiding this comment.
Fix LFR underflow: out_num_frames becomes 1 when input < window.
Current formula yields 1 for small inputs, causing OOB reads when copying LFR windows. Guard explicitly.
- int32_t out_num_frames =
- (in_num_frames - lfr_window_size) / lfr_window_shift + 1;
+ int32_t out_num_frames = 0;
+ if (in_num_frames >= lfr_window_size) {
+ out_num_frames =
+ (in_num_frames - lfr_window_size) / lfr_window_shift + 1;
+ }🤖 Prompt for AI Agents
In sherpa-onnx/csrc/ascend/offline-sense-voice-model-ascend.cc around lines 158
to 161, the LFR output frame calculation underflows: when in_num_frames <
lfr_window_size the current formula yields 1 and later causes out-of-bounds
reads when building windows; fix by guarding the computation — if in_num_frames
< lfr_window_size set out_num_frames to 0 (or otherwise handle the empty-output
case) else compute out_num_frames = (in_num_frames - lfr_window_size) /
lfr_window_shift + 1; ensure this branch is taken before any LFR window indexing
so no OOB accesses occur.
|
Tip For best results, initiate chat on the files or code changes. @csukuangfj I'll provide a comprehensive review of this PR. OverviewThis PR successfully adds Ascend NPU support for SenseVoice ASR using the ACL C API. The implementation follows the existing RKNN pattern and introduces clean RAII wrappers. However, I notice this PR also includes many Windows CI workflow changes that seem unrelated to the main objective. ✅ Strengths
🔍 Key Review Points1. CMake Configuration (
|
Usage
Install CANN
Build sherpa-onnx
git clone https://github.com/k2-fsa/sherpa-onnx cd sherpa-onnx mkdir build cmake -DSHERPA_ONNX_ENABLE_ASCEND_NPU=ON .. make sherpa-onnx-offlineTest with Ascend 910B NPU
cd /path/to/sherpa-onnx wget https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/sherpa-onnx-ascend-910B-sense-voice-zh-en-ja-ko-yue-2024-07-17.tar.bz2 tar xvf sherpa-onnx-ascend-910B-sense-voice-zh-en-ja-ko-yue-2024-07-17.tar.bz2 ./build/bin/sherpa-onnx-offline \ --debug=1 \ --provider=ascend \ --sense-voice-model=./sherpa-onnx-ascend-910B-sense-voice-zh-en-ja-ko-yue-2024-07-17/model.om \ --tokens=./sherpa-onnx-ascend-910B-sense-voice-zh-en-ja-ko-yue-2024-07-17/tokens.txt \ ./sherpa-onnx-ascend-910B-sense-voice-zh-en-ja-ko-yue-2024-07-17/test_wavs/zh.wavSummary by CodeRabbit
New Features
Chores
Bug Fixes / Improvements