Repository navigation
Refactor axcl examples. - #2867
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. WalkthroughReplaces the legacy AXCL runner with a new AXCL integration: adds AxclModel, AxclManager, RAII guards (engine, IO, IOInfo), device pointer utilities, CMake entries, and updates offline-sense-voice model to use AxclModel; removes old ax_model_runner_axcl and related headers/impls; tightens move semantics in ascend utils. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Mgr as AxclManager
participant Model as AxclModel
participant EngGuard as AxclEngineGuard
participant IOInfo as AxclEngineIOInfoGuard
participant IOGuard as AxclEngineIOGuard
participant AXCL as AXCL Runtime
App->>Mgr: AxclManager(config)
activate Mgr
Mgr->>AXCL: axclInit(config)
AXCL-->>Mgr: OK
deactivate Mgr
App->>Model: AxclModel(filename, device_id)
activate Model
Model->>EngGuard: AxclEngineGuard(npuKind)
EngGuard->>AXCL: axclrtEngineInit()
AXCL-->>EngGuard: OK
Model->>IOInfo: AxclEngineIOInfoGuard(model_id)
IOInfo->>AXCL: axclrtEngineGetIOInfo(model_id)
AXCL-->>IOInfo: io_info
Model->>IOGuard: AxclEngineIOGuard(io_info)
IOGuard->>AXCL: axclrtEngineCreateIO(io_info)
AXCL-->>IOGuard: io_handle
Note over Model: prepare tensors and device buffers
App->>Model: SetInputTensorData(name,data)
Model->>AXCL: copy to device
App->>Model: Run()
Model->>AXCL: axclrtEngineRun(io_handle)
AXCL-->>Model: inference done
App->>Model: GetOutputTensorData(name)
Model->>AXCL: copy from device
deactivate Model
App->>Mgr: ~AxclManager()
activate Mgr
Mgr->>AXCL: axclFinalize() (when last instance)
AXCL-->>Mgr: OK
deactivate Mgr
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 @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 significantly refactors the AXCL (Ascend Computing Language) integration within the 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 is a significant and well-executed refactoring of the AXCL integration. It replaces a monolithic runner class with a set of smaller, more focused RAII wrapper classes and a high-level AxclModel class. This greatly improves the code's structure, safety, and maintainability, as evidenced by the much cleaner implementation in offline-sense-voice-model-axcl.cc. While the overall direction is excellent, I've identified a few critical bugs and minor issues that should be addressed to ensure correctness and robustness. My comments focus on fixing these issues.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
sherpa-onnx/csrc/axcl/utils.h (1)
40-40: Consider making the implicit conversion operatorconst.The
operator void*()is non-const whileGet()is const. For consistency and to allow usage on const objects, consider:- operator void *() { return p_; } + operator void *() const { return p_; }sherpa-onnx/csrc/axcl/axcl-engine-io-info-guard.h (1)
23-23: Consider making the conversion operatorconst.For consistency and to allow usage on const objects:
- operator axclrtEngineIOInfo() { return io_info_; } + operator axclrtEngineIOInfo() const { return io_info_; }sherpa-onnx/csrc/axcl/axcl-engine-io-guard.h (1)
21-21: Consider making the conversion operatorconst.For consistency with the other guard classes and to allow usage on const objects:
- operator axclrtEngineIO() { return io_; } + operator axclrtEngineIO() const { return io_; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
sherpa-onnx/csrc/CMakeLists.txt(1 hunks)sherpa-onnx/csrc/ascend/utils.h(8 hunks)sherpa-onnx/csrc/axcl/ax_model_runner.hpp(0 hunks)sherpa-onnx/csrc/axcl/ax_model_runner_axcl.cc(0 hunks)sherpa-onnx/csrc/axcl/ax_model_runner_axcl.hpp(0 hunks)sherpa-onnx/csrc/axcl/axcl-engine-guard.cc(1 hunks)sherpa-onnx/csrc/axcl/axcl-engine-guard.h(1 hunks)sherpa-onnx/csrc/axcl/axcl-engine-io-guard.cc(1 hunks)sherpa-onnx/csrc/axcl/axcl-engine-io-guard.h(1 hunks)sherpa-onnx/csrc/axcl/axcl-engine-io-info-guard.cc(1 hunks)sherpa-onnx/csrc/axcl/axcl-engine-io-info-guard.h(1 hunks)sherpa-onnx/csrc/axcl/axcl-manager.cc(1 hunks)sherpa-onnx/csrc/axcl/axcl-manager.h(1 hunks)sherpa-onnx/csrc/axcl/axcl-model.cc(1 hunks)sherpa-onnx/csrc/axcl/axcl-model.h(1 hunks)sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.cc(4 hunks)sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.h(0 hunks)sherpa-onnx/csrc/axcl/utils.cc(1 hunks)sherpa-onnx/csrc/axcl/utils.h(1 hunks)
💤 Files with no reviewable changes (4)
- sherpa-onnx/csrc/axcl/ax_model_runner_axcl.hpp
- sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.h
- sherpa-onnx/csrc/axcl/ax_model_runner_axcl.cc
- sherpa-onnx/csrc/axcl/ax_model_runner.hpp
🧰 Additional context used
🧠 Learnings (2)
📚 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/axcl/offline-sense-voice-model-axcl.ccsherpa-onnx/csrc/CMakeLists.txt
📚 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/axcl/offline-sense-voice-model-axcl.ccsherpa-onnx/csrc/CMakeLists.txt
🧬 Code graph analysis (13)
sherpa-onnx/csrc/axcl/axcl-manager.h (1)
sherpa-onnx/csrc/axcl/axcl-manager.cc (2)
AxclManager(18-29)AxclManager(31-42)
sherpa-onnx/csrc/axcl/utils.cc (1)
sherpa-onnx/csrc/axcl/utils.h (1)
AxclDevicePtr(12-47)
sherpa-onnx/csrc/axcl/axcl-engine-guard.h (3)
sherpa-onnx/csrc/axcl/axcl-model.h (1)
sherpa_onnx(13-47)sherpa-onnx/csrc/axcl/utils.h (1)
sherpa_onnx(10-49)sherpa-onnx/csrc/axcl/axcl-engine-guard.cc (2)
AxclEngineGuard(14-24)AxclEngineGuard(26-37)
sherpa-onnx/csrc/axcl/axcl-engine-io-info-guard.cc (2)
sherpa-onnx/csrc/axcl/axcl-engine-io-info-guard.h (1)
AxclEngineIOInfoGuard(13-28)sherpa-onnx/csrc/axcl/axcl-model.cc (1)
ret(284-293)
sherpa-onnx/csrc/axcl/axcl-engine-guard.cc (2)
sherpa-onnx/csrc/axcl/axcl-model.cc (1)
ret(284-293)sherpa-onnx/csrc/axera/ax-engine-guard.h (1)
sherpa_onnx(9-26)
sherpa-onnx/csrc/axcl/utils.h (2)
sherpa-onnx/csrc/axcl/axcl-engine-guard.h (1)
sherpa_onnx(9-25)sherpa-onnx/csrc/axcl/utils.cc (4)
AxclDevicePtr(11-22)AxclDevicePtr(38-38)Release(24-36)Release(24-24)
sherpa-onnx/csrc/axcl/axcl-manager.cc (1)
sherpa-onnx/csrc/axcl/axcl-manager.h (1)
AxclManager(13-27)
sherpa-onnx/csrc/axcl/axcl-engine-io-guard.cc (1)
sherpa-onnx/csrc/axcl/axcl-engine-io-guard.h (1)
AxclEngineIOGuard(11-26)
sherpa-onnx/csrc/axcl/axcl-engine-io-guard.h (1)
sherpa-onnx/csrc/axcl/axcl-engine-io-guard.cc (2)
AxclEngineIOGuard(14-24)AxclEngineIOGuard(26-37)
sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.cc (1)
sherpa-onnx/csrc/axera/offline-sense-voice-model-axera.cc (2)
features(56-102)features(56-57)
sherpa-onnx/csrc/axcl/axcl-model.cc (1)
sherpa-onnx/csrc/axcl/axcl-model.h (1)
AxclModel(15-45)
sherpa-onnx/csrc/ascend/utils.h (1)
sherpa-onnx/csrc/ascend/utils.cc (17)
Acl(103-107)Acl(109-114)AclContext(116-119)AclContext(121-126)AclDevicePtr(130-139)AclDevicePtr(141-146)AclModelDesc(148-157)AclModelDesc(159-164)AclModel(166-173)AclModel(175-180)AclModel(182-187)AclMdlDataset(316-322)AclMdlDataset(324-329)AclDataBuffer(344-351)AclDataBuffer(353-358)AclTensorDesc(360-367)AclTensorDesc(369-373)
sherpa-onnx/csrc/axcl/axcl-engine-io-info-guard.h (1)
sherpa-onnx/csrc/axcl/axcl-engine-io-info-guard.cc (2)
AxclEngineIOInfoGuard(14-24)AxclEngineIOInfoGuard(26-37)
⏰ 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: Debug static tts-OFF
- GitHub Check: Release shared-OFF tts-ON
- GitHub Check: Debug shared-OFF tts-OFF
- GitHub Check: Release shared-ON tts-OFF
- GitHub Check: Release shared-OFF tts-OFF
- GitHub Check: Debug shared-ON tts-OFF
- GitHub Check: Debug shared-ON tts-ON
- GitHub Check: Release shared-ON tts-ON
- GitHub Check: ubuntu-24.04 3.10
- GitHub Check: ubuntu-24.04 3.12
- GitHub Check: rknn shared OFF
- GitHub Check: Debug shared-OFF tts-ON
- GitHub Check: rknn shared ON
- GitHub Check: swift (macos-latest)
- GitHub Check: ubuntu-24.04 3.13
- GitHub Check: swift (macos-13)
- GitHub Check: ubuntu-24.04 3.11
- GitHub Check: ubuntu-24.04 3.8
- GitHub Check: ubuntu-24.04 3.9
🔇 Additional comments (16)
sherpa-onnx/csrc/axcl/axcl-engine-guard.h (1)
11-23: LGTM! Clean RAII guard design.The class correctly implements a non-copyable, non-movable guard pattern with proper state tracking via
initialized_. This ensures safe cleanup in the destructor.sherpa-onnx/csrc/axcl/axcl-engine-io-guard.cc (1)
14-37: LGTM! Proper RAII lifecycle management.The constructor and destructor correctly manage the
axclrtEngineIOlifecycle with appropriate error handling. Theinitialized_flag ensures the destructor only attempts cleanup when initialization succeeded.sherpa-onnx/csrc/axcl/axcl-engine-io-info-guard.cc (1)
14-37: LGTM! Consistent with other guard implementations.The implementation follows the established pattern for AXCL resource guards with proper initialization tracking and cleanup.
sherpa-onnx/csrc/axcl/utils.cc (1)
24-38: LGTM! Proper cleanup logic.The
Release()method correctly handles the null check, frees memory, and nulls the pointer. The destructor delegates toRelease()appropriately.sherpa-onnx/csrc/axcl/axcl-engine-guard.cc (1)
14-37: LGTM! Proper AXCL engine lifecycle management.The constructor and destructor correctly manage the AXCL engine initialization and finalization with appropriate error handling and state tracking.
sherpa-onnx/csrc/CMakeLists.txt (1)
207-217: LGTM!The new AXCL source files are correctly added within the
SHERPA_ONNX_ENABLE_AXCLconditional block, following the established pattern used for other backends (RKNN, AXERA). The file organization is clear and consistent.sherpa-onnx/csrc/axcl/axcl-manager.h (1)
13-27: LGTM!The
AxclManagerclass provides proper thread-safe reference-counted lifecycle management for the AXCL library. The design correctly uses a mutex with an atomic counter to ensureaxclInitis called once on first use andaxclFinalizeon last destruction. Copy and move semantics are appropriately deleted for this singleton-like RAII pattern.sherpa-onnx/csrc/axcl/axcl-engine-io-info-guard.h (1)
13-28: LGTM!The
AxclEngineIOInfoGuardclass correctly implements RAII foraxclrtEngineIOInforesources. Theinitialized_flag properly guards cleanup, and copy/move semantics are appropriately deleted.sherpa-onnx/csrc/axcl/axcl-engine-io-guard.h (1)
11-26: LGTM!The
AxclEngineIOGuardclass correctly implements RAII foraxclrtEngineIOresources. The design is consistent with the other guard classes in this PR.sherpa-onnx/csrc/axcl/axcl-manager.cc (1)
14-29: LGTM! Thread-safe reference-counted AXCL lifecycle management.The implementation correctly handles initialization on first use with proper cleanup on failure. Minor observation: since
instanceCount_is always accessed undermutex_protection,std::atomic<int>could be simplified toint. However, the current approach is safe and works correctly.sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.cc (2)
38-47: Verify thread safety for concurrentRun()calls.The Axera implementation in
sherpa-onnx/csrc/axera/offline-sense-voice-model-axera.cc(lines 55-59) uses a mutex lock inRun()with the comment "TODO(fangjun): Support multi clients". This AXCL version lacks similar protection. If concurrent inference is expected, consider adding a mutex here as well.
50-62: LGTM!The
PostInit()method properly validates model initialization and extracts the required tensor shape metadata.sherpa-onnx/csrc/axcl/axcl-model.cc (2)
85-96: LGTM!Destructor correctly guards unload with
model_loaded_check and handles errors appropriately.
295-370: LGTM!Input and output tensor initialization is thorough, with proper error handling and buffer binding.
sherpa-onnx/csrc/axcl/axcl-model.h (1)
15-45: LGTM! Clean API design with proper encapsulation.The pimpl idiom effectively hides implementation details. The API provides a complete interface for model lifecycle, tensor metadata access, data transfer, and execution.
sherpa-onnx/csrc/ascend/utils.h (1)
21-25: Explicitly deleted move operations and normalized operator= signatures look goodAcross
Acl,AclContext,AclDevicePtr,AclModelDesc,AclModel,AclMdlDataset,AclDataBuffer, andAclTensorDesc, making the move constructor/assignment explicitly= deleteand changing deleted copy-assignments to returnT&instead ofconst T&is consistent with RAII best practices and clarifies intent. Given these types already had user-defined destructors (and in most cases deleted copy operations), they were effectively non-movable before; this change mainly improves readability and makes the non-movable property explicit without altering behavior. No issues from my side.Also applies to: 37-41, 57-61, 79-83, 104-108, 153-154, 174-175, 193-194
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
sherpa-onnx/csrc/axcl/utils.h (2)
22-41: Past move semantics issue is now resolved.The move constructor and move assignment operator now correctly transfer both
p_andsize_members, addressing the critical bug flagged in previous reviews.For consistency with standard library best practices, consider marking both move operations
noexceptto enable optimizations in standard containers likestd::vector.Apply this diff to mark move operations as
noexcept:- AxclDevicePtr(AxclDevicePtr &&other) { + AxclDevicePtr(AxclDevicePtr &&other) noexcept { p_ = other.p_; size_ = other.size_; other.p_ = nullptr; other.size_ = 0; } - AxclDevicePtr &operator=(AxclDevicePtr &&other) { + AxclDevicePtr &operator=(AxclDevicePtr &&other) noexcept { if (this == &other) { return *this; }
46-46: Mark conversion operator asconst.For consistency with
Get()(line 45), theoperator void *()should be markedconstsince it doesn't modify object state.Apply this diff:
- operator void *() { return p_; } + operator void *() const { return p_; }sherpa-onnx/csrc/axcl/axcl-model.cc (1)
243-243: Minor typo in log message."Validate range" should likely be "Valid range" for grammatical correctness.
Apply this diff:
- SHERPA_ONNX_LOGE("Invalid device_id: %d. Validate range: 0-%d", device_id, + SHERPA_ONNX_LOGE("Invalid device_id: %d. Valid range: 0-%d", device_id, lst.num - 1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
sherpa-onnx/csrc/axcl/axcl-engine-guard.cc(1 hunks)sherpa-onnx/csrc/axcl/axcl-model.cc(1 hunks)sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.cc(4 hunks)sherpa-onnx/csrc/axcl/utils.cc(1 hunks)sherpa-onnx/csrc/axcl/utils.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sherpa-onnx/csrc/axcl/axcl-engine-guard.cc
🧰 Additional context used
🧠 Learnings (2)
📚 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/axcl/offline-sense-voice-model-axcl.cc
📚 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/axcl/offline-sense-voice-model-axcl.cc
🧬 Code graph analysis (4)
sherpa-onnx/csrc/axcl/utils.cc (1)
sherpa-onnx/csrc/axcl/utils.h (1)
AxclDevicePtr(12-53)
sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.cc (1)
sherpa-onnx/csrc/axera/offline-sense-voice-model-axera.cc (2)
features(56-102)features(56-57)
sherpa-onnx/csrc/axcl/utils.h (1)
sherpa-onnx/csrc/axcl/utils.cc (4)
AxclDevicePtr(11-22)AxclDevicePtr(39-39)Release(24-37)Release(24-24)
sherpa-onnx/csrc/axcl/axcl-model.cc (2)
sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.cc (6)
Impl(21-25)Impl(21-21)Impl(28-33)out(84-84)Run(113-117)Run(113-115)sherpa-onnx/csrc/axcl/axcl-model.h (1)
AxclModel(15-45)
🔇 Additional comments (8)
sherpa-onnx/csrc/axcl/utils.cc (1)
11-39: LGTM!The RAII implementation is correct and follows best practices:
- Constructor allocates device memory and stores size
- Release properly frees memory and resets state
- Destructor ensures cleanup via Release
- Comprehensive error handling throughout
sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.cc (3)
21-33: LGTM!The refactoring to use
AxclModelis clean and well-structured. Both constructors properly initialize the model (from file or buffer) and callPostInit()for validation.
39-48: LGTM!The
Runmethod is cleanly refactored to use theAxclModelAPI. Input data is set viaSetInputTensorData, execution happens viaRun(), and output is retrieved viaGetOutputTensorData.
51-63: LGTM!The new
PostInitmethod provides clear initialization validation and properly derivesnum_input_frames_from the model's tensor shape.sherpa-onnx/csrc/axcl/axcl-model.cc (4)
33-52: LGTM!The file-based constructor properly initializes the AXCL engine, loads the model, and calls
PostInit(). Error handling is comprehensive, and the previous issue with logging0instead ofretat line 45 has been resolved.
54-83: LGTM!The memory-based constructor correctly copies the model buffer to device memory using
AxclDevicePtrRAII wrapper, loads from device memory, and proceeds withPostInit().
155-184: LGTM!The
SetInputTensorDatatemplate correctly validates tensor size, performs the memory copy, and now properly returnstrueon success (line 177), fixing the critical bug flagged in previous reviews.
226-256: LGTM!Device selection logic is correct with proper validation of device count and device ID range.
There was a problem hiding this comment.
Pull request overview
This PR refactors the AXCL (AX compute library) implementation by replacing legacy runner code with a new, cleaner model abstraction API. The refactoring introduces RAII-based resource management similar to the existing Ascend implementation pattern, improving lifecycle safety and reducing header dependencies.
Key Changes:
- Introduced a new high-level
AxclModelAPI for loading, running, and querying AXCL models - Replaced legacy
ax_runner_axclwith RAII guard classes for managing engine, IO, and device memory resources - Refactored
OfflineSenseVoiceModelAxclto use the new cleaner API
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| sherpa-onnx/csrc/axcl/utils.h | New device memory pointer RAII wrapper with move semantics |
| sherpa-onnx/csrc/axcl/utils.cc | Implementation of AxclDevicePtr memory allocation/deallocation |
| sherpa-onnx/csrc/axcl/axcl-model.h | High-level AXCL model API interface with tensor operations |
| sherpa-onnx/csrc/axcl/axcl-model.cc | Implementation of AxclModel with device setup, model loading, and execution |
| sherpa-onnx/csrc/axcl/axcl-manager.h | Singleton manager for AXCL initialization/finalization with reference counting |
| sherpa-onnx/csrc/axcl/axcl-manager.cc | Thread-safe AXCL lifecycle management using mutex and atomic counter |
| sherpa-onnx/csrc/axcl/axcl-engine-guard.h | RAII guard for engine initialization/finalization |
| sherpa-onnx/csrc/axcl/axcl-engine-guard.cc | Engine guard implementation |
| sherpa-onnx/csrc/axcl/axcl-engine-io-info-guard.h | RAII guard for engine IO info lifecycle |
| sherpa-onnx/csrc/axcl/axcl-engine-io-info-guard.cc | IO info guard implementation |
| sherpa-onnx/csrc/axcl/axcl-engine-io-guard.h | RAII guard for engine IO lifecycle |
| sherpa-onnx/csrc/axcl/axcl-engine-io-guard.cc | IO guard implementation |
| sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.h | Removed axcl.h and runner header dependencies |
| sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.cc | Refactored to use new AxclModel API, simplified initialization and inference |
| sherpa-onnx/csrc/axcl/ax_model_runner_axcl.hpp | Deleted legacy runner header |
| sherpa-onnx/csrc/axcl/ax_model_runner_axcl.cc | Deleted legacy runner implementation (470 lines) |
| sherpa-onnx/csrc/axcl/ax_model_runner.hpp | Deleted legacy runner base class (148 lines) |
| sherpa-onnx/csrc/ascend/utils.h | Added move constructor/operator deletions for consistency |
| sherpa-onnx/csrc/CMakeLists.txt | Updated source list to include new files and remove legacy runner |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (ret != 0) { | ||
| SHERPA_ONNX_LOGE("Failed to call axclrtMemcpy(). Return code is: %d", | ||
| static_cast<int32_t>(ret)); | ||
| return; |
There was a problem hiding this comment.
If axclrtMemcpy fails (line 63-68), the function returns early without calling SHERPA_ONNX_EXIT(-1), unlike other initialization failures. This leaves the AxclModel object in a partially initialized state with model_loaded_ = false, but an initialized engine_guard_. For consistency with other fatal initialization errors in this file, consider calling SHERPA_ONNX_EXIT(-1) instead of just returning.
| return; | |
| SHERPA_ONNX_EXIT(-1); |
| SHERPA_ONNX_LOGE( | ||
| "Failed to call axclrtEngineLoadFromMem(). Return code is: %d", | ||
| static_cast<int32_t>(ret)); | ||
| return; |
There was a problem hiding this comment.
If axclrtEngineLoadFromMem fails (line 71-76), the function returns early without calling SHERPA_ONNX_EXIT(-1), unlike axclrtEngineLoadFromFile failure on line 46. This inconsistency leaves the object in a partially initialized state. For consistency with other fatal initialization errors in this file, consider calling SHERPA_ONNX_EXIT(-1) instead of just returning.
| return; | |
| SHERPA_ONNX_EXIT(-1); |
| if (instanceCount_++ == 0) { | ||
| auto ret = axclInit(config); | ||
| if (ret != 0) { | ||
| instanceCount_--; | ||
| SHERPA_ONNX_LOGE("Failed to call axclInit(). Return code: %d", | ||
| static_cast<int32_t>(ret)); | ||
| SHERPA_ONNX_EXIT(-1); | ||
| } |
There was a problem hiding this comment.
The post-increment instanceCount_++ followed by decrement on error path creates a race condition window. If another thread checks the count between the increment and the error decrement, it will see an incorrect count. Consider using compare-and-swap or only incrementing after successful initialization.
| AxclDevicePtr(AxclDevicePtr &&other) { | ||
| p_ = other.p_; | ||
| size_ = other.size_; | ||
|
|
||
| other.p_ = nullptr; | ||
| other.size_ = 0; | ||
| } | ||
| AxclDevicePtr &operator=(AxclDevicePtr &&other) { | ||
| if (this == &other) { | ||
| return *this; | ||
| } | ||
| Release(); | ||
| p_ = other.p_; | ||
| size_ = other.size_; | ||
|
|
||
| other.p_ = nullptr; | ||
| other.size_ = 0; | ||
|
|
||
| return *this; | ||
| } | ||
|
|
There was a problem hiding this comment.
AxclDevicePtr implements move semantics, but the similar AclDevicePtr class in sherpa-onnx/csrc/ascend/utils.h (lines 60-61) deletes move operations to enforce strict single ownership. Consider deleting move operations here as well for consistency with the Ascend implementation pattern, unless there's a specific reason to allow moving device pointers.
| AxclDevicePtr(AxclDevicePtr &&other) { | |
| p_ = other.p_; | |
| size_ = other.size_; | |
| other.p_ = nullptr; | |
| other.size_ = 0; | |
| } | |
| AxclDevicePtr &operator=(AxclDevicePtr &&other) { | |
| if (this == &other) { | |
| return *this; | |
| } | |
| Release(); | |
| p_ = other.p_; | |
| size_ = other.size_; | |
| other.p_ = nullptr; | |
| other.size_ = 0; | |
| return *this; | |
| } | |
| AxclDevicePtr(AxclDevicePtr &&) = delete; | |
| AxclDevicePtr &operator=(AxclDevicePtr &&) = delete; |
|
|
||
| // device_id counts from 0 | ||
| if (device_id < 0 || device_id >= lst.num) { | ||
| SHERPA_ONNX_LOGE("Invalid device_id: %d. Validate range: 0-%d", device_id, |
There was a problem hiding this comment.
Typo in error message: "Validate" should be "Valid".
| SHERPA_ONNX_LOGE("Invalid device_id: %d. Validate range: 0-%d", device_id, | |
| SHERPA_ONNX_LOGE("Invalid device_id: %d. Valid range: 0-%d", device_id, |
|
|
||
| Impl(const void *cpu_buf, size_t buf_len_in_bytes, int32_t device_id) { | ||
| if (!SetDevice(device_id)) { | ||
| return; |
There was a problem hiding this comment.
When SetDevice fails (line 55-56), the constructor returns early without setting model_loaded_ or cleaning up engine_guard_. This leaves the object in an inconsistent state. The destructor will skip unloading, but the AxclEngineGuard destructor will still call axclrtEngineFinalize, potentially interfering with other instances. Consider calling SHERPA_ONNX_EXIT(-1) on SetDevice failure for consistency with other fatal initialization errors.
| return; | |
| SHERPA_ONNX_LOGE("Failed to set device (id=%d) in AxclModel::Impl constructor.", device_id); | |
| SHERPA_ONNX_EXIT(-1); |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.cc (1)
40-49: Handle SetInputTensorData/Run failures instead of ignoring them
SetInputTensorDataandRunreturnboolbut the results are ignored. If the feature length or prompt size mismatches the expected tensor size, or if AXCL copy/execute fails, this path will quietly continue and likely reuse stale device buffers, diverging from the axera backend which exits on such mismatches.Consider checking these return values and treating failures as fatal (or at least bailing out) before reading outputs. For example:
- model_->SetInputTensorData("x", features.data(), features.size()); - model_->SetInputTensorData("prompt", prompt.data(), prompt.size()); - model_->Run(); - return model_->GetOutputTensorData("logits"); + if (!model_->SetInputTensorData("x", features.data(), + static_cast<int32_t>(features.size()))) { + SHERPA_ONNX_LOGE("Failed to set input tensor 'x' for model '%s'", + config_.sense_voice.model.c_str()); + SHERPA_ONNX_EXIT(-1); + } + + if (!model_->SetInputTensorData("prompt", prompt.data(), + static_cast<int32_t>(prompt.size()))) { + SHERPA_ONNX_LOGE("Failed to set input tensor 'prompt' for model '%s'", + config_.sense_voice.model.c_str()); + SHERPA_ONNX_EXIT(-1); + } + + if (!model_->Run()) { + SHERPA_ONNX_LOGE("Failed to run inference for model '%s'", + config_.sense_voice.model.c_str()); + SHERPA_ONNX_EXIT(-1); + } + + return model_->GetOutputTensorData("logits");If this class is ever used from multiple threads, consider also adding a mutex (similar to the axera backend) around this block to avoid concurrent mutation of shared AXCL IO buffers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sherpa-onnx/csrc/axcl/axcl-manager.cc(1 hunks)sherpa-onnx/csrc/axcl/axcl-manager.h(1 hunks)sherpa-onnx/csrc/axcl/axcl-model.cc(1 hunks)sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.cc(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sherpa-onnx/csrc/axcl/axcl-manager.h
🧰 Additional context used
🧠 Learnings (2)
📚 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/axcl/offline-sense-voice-model-axcl.cc
📚 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/axcl/offline-sense-voice-model-axcl.cc
🧬 Code graph analysis (3)
sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.cc (1)
sherpa-onnx/csrc/axera/offline-sense-voice-model-axera.cc (2)
features(56-102)features(56-57)
sherpa-onnx/csrc/axcl/axcl-manager.cc (2)
sherpa-onnx/csrc/axcl/axcl-manager.h (1)
AxclManager(13-27)sherpa-onnx/csrc/axcl/axcl-model.cc (1)
ret(286-295)
sherpa-onnx/csrc/axcl/axcl-model.cc (2)
sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.cc (5)
Impl(22-26)Impl(22-22)Impl(29-34)Run(114-118)Run(114-116)sherpa-onnx/csrc/axcl/axcl-model.h (1)
AxclModel(15-45)
⏰ 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: Release shared tts-OFF
- GitHub Check: Debug static tts-ON
- GitHub Check: Debug shared-ON tts-ON
- GitHub Check: Release shared-OFF tts-ON
- GitHub Check: Debug shared-OFF tts-OFF
- GitHub Check: Debug shared-ON tts-OFF
- GitHub Check: Debug shared-OFF tts-ON
- GitHub Check: Release shared-ON tts-OFF
- GitHub Check: Release shared-OFF tts-OFF
- GitHub Check: Release shared-ON tts-ON
- GitHub Check: ubuntu-24.04 3.9
- GitHub Check: ubuntu-24.04 3.10
- GitHub Check: ubuntu-24.04 3.11
- GitHub Check: ubuntu-24.04 3.8
- GitHub Check: ubuntu-24.04 3.13
- GitHub Check: ubuntu-24.04 3.12
- GitHub Check: swift (macos-13)
- GitHub Check: swift (macos-latest)
- GitHub Check: rknn shared OFF
- GitHub Check: rknn shared ON
🔇 Additional comments (3)
sherpa-onnx/csrc/axcl/axcl-manager.cc (3)
18-30: Clarify semantics when multipleAxclManagerinstances use differentconfigvaluesOnly the first construction's
configis actually used foraxclInit(); later instances silently ignore theirconfigargument if AXCL is already initialized. This could be surprising if callers pass different configs, making bugs hard to diagnose.Consider one of:
- Documenting that only the first non‑null
configis respected and all subsequent ones must benullptr; or- Storing the first
config(e.g., as a canonical string) and asserting/logging if later calls pass a different, incompatible config.
32-43: Add defensive checks aroundcount_to catch misuse and verify static lifetime safety
--count_assumes every destructor call is balanced with a prior constructor; ifcount_ever hits negative (e.g., via manual destructor calls or odd static‑lifetime interactions), it will silently underflow and never finalize correctly. Consider adding a defensive guard to catch this misuse:AxclManager::~AxclManager() { std::lock_guard<std::mutex> lock(mutex_); + if (count_ <= 0) { + SHERPA_ONNX_LOGE("AxclManager destroyed more times than constructed"); + SHERPA_ONNX_EXIT(-1); + } + if (--count_ == 0) { auto ret = axclFinalize();Additionally, verify that all
AxclManagerinstances are non-static (stack- or member-owned). If any instances have static storage duration in other translation units, their destructors might run aftermutex_/count_teardown, causing undefined behavior.
14-30: Thread-safe refcounted init pattern looks solidUsing a single static
mutex_to guard bothcount_and theaxclInitcall, and incrementingcount_only after successful init, gives you a clean, race-free first-use initialization across threads.However, verification of the complete implementation (destructor, usage patterns, and edge cases) could not be completed due to repository access limitations. To strengthen confidence in this review, consider also examining:
- The destructor implementation to ensure
count_is properly decremented- Whether defensive checks guard against
count_underflow- Static storage duration and destruction order if AxclManager instances exist in other translation units
- How the
configparameter is handled across multiple AxclManager instances
| bool Run() const { | ||
| uint32_t group = 0; | ||
| auto ret = | ||
| axclrtEngineExecute(model_id_, context_id_, group, *engine_io_guard_); | ||
| if (ret != 0) { | ||
| SHERPA_ONNX_LOGE("Failed to call axclrtEngineExecute(), return code: %d", | ||
| static_cast<int32_t>(ret)); | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Avoid dereferencing engine_io_guard_ when the model isn’t initialized
Run() unconditionally dereferences *engine_io_guard_. If SetDevice or model loading failed in the constructor, PostInit() is never called, engine_io_guard_ stays nullptr, and model_loaded_ remains false. A client that (incorrectly) calls Run() anyway will hit undefined behavior.
It’s cheap to harden this and make the failure mode explicit:
- bool Run() const {
- uint32_t group = 0;
- auto ret =
- axclrtEngineExecute(model_id_, context_id_, group, *engine_io_guard_);
- if (ret != 0) {
- SHERPA_ONNX_LOGE("Failed to call axclrtEngineExecute(), return code: %d",
- static_cast<int32_t>(ret));
- return false;
- }
- return true;
- }
+ bool Run() const {
+ if (!model_loaded_ || !engine_io_guard_) {
+ SHERPA_ONNX_LOGE(
+ "AxclModel::Run() called on an uninitialized model or without IO "
+ "buffers.");
+ return false;
+ }
+
+ uint32_t group = 0;
+ auto ret =
+ axclrtEngineExecute(model_id_, context_id_, group, *engine_io_guard_);
+ if (ret != 0) {
+ SHERPA_ONNX_LOGE("Failed to call axclrtEngineExecute(), return code: %d",
+ static_cast<int32_t>(ret));
+ return false;
+ }
+ return true;
+ }This preserves the current behavior on successful initialization while preventing a null dereference in error cases.
🤖 Prompt for AI Agents
In sherpa-onnx/csrc/axcl/axcl-model.cc around lines 211 to 221, Run() currently
dereferences *engine_io_guard_ unconditionally which can be null if PostInit()
never ran; before calling axclrtEngineExecute check that model_loaded_ is true
and engine_io_guard_ is non-null, and if not, log an error (e.g.
SHERPA_ONNX_LOGE with a clear message about uninitialized
model/engine_io_guard_) and return false; otherwise proceed to call
axclrtEngineExecute as before.
| void PostInit() { | ||
| if (!model_->IsInitialized()) { | ||
| SHERPA_ONNX_LOGE("Failed to initialize the model with '%s'", | ||
| config_.sense_voice.model.c_str()); | ||
| SHERPA_ONNX_EXIT(-1); | ||
| } | ||
|
|
||
| auto &in0 = runner_.get_input(0); | ||
| if (in0.vShape.size() < 2) { | ||
| SHERPA_ONNX_LOGE( | ||
| "Input tensor rank is too small (rank = %zu). Shape vector is empty " | ||
| "or has only 1 dim.", | ||
| in0.vShape.size()); | ||
| SHERPA_ONNX_EXIT(-1); | ||
| } | ||
| num_input_frames_ = in0.vShape[1]; | ||
| num_input_frames_ = model_->TensorShape("x")[1]; | ||
|
|
There was a problem hiding this comment.
Guard against missing or malformed input tensor "x" in PostInit()
PostInit() assumes that TensorShape("x") exists and has at least two dimensions, then directly reads [1]. If the model doesn’t expose an input named "x" or has rank < 2, TensorShape logs an error and returns {}, and this indexing becomes undefined behavior.
Adding a shape-size check will make this failure mode explicit:
- void PostInit() {
- if (!model_->IsInitialized()) {
- SHERPA_ONNX_LOGE("Failed to initialize the model with '%s'",
- config_.sense_voice.model.c_str());
- SHERPA_ONNX_EXIT(-1);
- }
-
- num_input_frames_ = model_->TensorShape("x")[1];
+ void PostInit() {
+ if (!model_->IsInitialized()) {
+ SHERPA_ONNX_LOGE("Failed to initialize the model with '%s'",
+ config_.sense_voice.model.c_str());
+ SHERPA_ONNX_EXIT(-1);
+ }
+
+ auto shape = model_->TensorShape("x");
+ if (shape.size() <= 1) {
+ SHERPA_ONNX_LOGE(
+ "Input tensor 'x' has unexpected rank %zu for model '%s'",
+ shape.size(), config_.sense_voice.model.c_str());
+ SHERPA_ONNX_EXIT(-1);
+ }
+
+ num_input_frames_ = shape[1];This keeps the existing fatal-on-bad-model behavior but avoids UB if the model doesn’t match expectations.
📝 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.
| void PostInit() { | |
| if (!model_->IsInitialized()) { | |
| SHERPA_ONNX_LOGE("Failed to initialize the model with '%s'", | |
| config_.sense_voice.model.c_str()); | |
| SHERPA_ONNX_EXIT(-1); | |
| } | |
| auto &in0 = runner_.get_input(0); | |
| if (in0.vShape.size() < 2) { | |
| SHERPA_ONNX_LOGE( | |
| "Input tensor rank is too small (rank = %zu). Shape vector is empty " | |
| "or has only 1 dim.", | |
| in0.vShape.size()); | |
| SHERPA_ONNX_EXIT(-1); | |
| } | |
| num_input_frames_ = in0.vShape[1]; | |
| num_input_frames_ = model_->TensorShape("x")[1]; | |
| void PostInit() { | |
| if (!model_->IsInitialized()) { | |
| SHERPA_ONNX_LOGE("Failed to initialize the model with '%s'", | |
| config_.sense_voice.model.c_str()); | |
| SHERPA_ONNX_EXIT(-1); | |
| } | |
| auto shape = model_->TensorShape("x"); | |
| if (shape.size() <= 1) { | |
| SHERPA_ONNX_LOGE( | |
| "Input tensor 'x' has unexpected rank %zu for model '%s'", | |
| shape.size(), config_.sense_voice.model.c_str()); | |
| SHERPA_ONNX_EXIT(-1); | |
| } | |
| num_input_frames_ = shape[1]; |
🤖 Prompt for AI Agents
In sherpa-onnx/csrc/axcl/offline-sense-voice-model-axcl.cc around lines 52 to
60, PostInit() currently calls model_->TensorShape("x")[1] without validating
the returned shape; capture the shape into a local variable, verify it's
non-empty and has at least two dimensions (shape.size() > 1), and if the check
fails log a clear error (including config_.sense_voice.model) and call
SHERPA_ONNX_EXIT(-1); otherwise assign num_input_frames_ = shape[1]. This
preserves the fatal-on-bad-model behavior while avoiding undefined behavior when
"x" is missing or malformed.
@Abandon-ht
see also #2849
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.