Add C++ runtime and Python API for Google MedASR models - #2935
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. 📝 WalkthroughWalkthroughAdds first-class support for Google MedASR CTC: new C++ model implementation, config, factory integration, Python bindings and API constructor, example decoding script, CI/test updates, and exporter metadata for subsampling_factor. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Py as Python client
participant OR as OfflineRecognizer (C++)
participant RC as OfflineRecognizerCtcImpl
participant MM as OfflineMedAsrCtcModel
participant ORT as ONNXRuntime
Note over Py,OR: Instantiate recognizer
Py->>OR: OfflineRecognizer.from_medasr_ctc(model, tokens...)
OR->>RC: create CTC impl with medasr config
RC->>MM: construct OfflineMedAsrCtcModel (load model, read metadata)
MM->>ORT: create Session, prepare inputs
Note over Py,MM: Decode request flow
Py->>OR: decode(audio)
OR->>RC: forward to CTC impl
RC->>MM: MM.Forward(features, features_length)
MM->>ORT: Run session (features, mask) -> outputs
ORT-->>MM: raw logits / tokens
MM-->>RC: outputs
RC-->>OR: decoded tokens/text (apply MedASR post-processing)
OR-->>Py: final transcription
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 |
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 enhances Highlights
Ignored Files
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
python-api-examples/offline-medasr-ctc-decode-files.py (1)
99-130: Consider adding strict=True to zip() for robustness.The batch decoding logic is correct. For added safety, consider using
strict=Truewith zip() (available in Python 3.10+) to ensure filenames and streams have matching lengths.Optional enhancement
- for name, stream in zip(filenames, streams): + for name, stream in zip(filenames, streams, strict=True):sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.cc (1)
8-8: Unused include.
<vector>is included but not used in this file.🔎 Proposed fix
#include <string> -#include <vector> #include "sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.h"sherpa-onnx/csrc/offline-medasr-ctc-model.cc (1)
49-54: Potential signed/unsigned or width mismatch in loop.
iis declared asint32_twhilebatch_sizeisint64_t. For very large batch sizes (unlikely but possible), this could cause issues. Consider using a consistent type.🔎 Proposed fix
- for (int32_t i = 0; i < batch_size; ++i) { + for (int64_t i = 0; i < batch_size; ++i) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/scripts/test-python.sh.github/workflows/export-medasr-ctc-to-onnx.yamlpython-api-examples/offline-medasr-ctc-decode-files.pyscripts/medasr/export_onnx.pysherpa-onnx/csrc/CMakeLists.txtsherpa-onnx/csrc/offline-ctc-model.ccsherpa-onnx/csrc/offline-medasr-ctc-model-config.ccsherpa-onnx/csrc/offline-medasr-ctc-model-config.hsherpa-onnx/csrc/offline-medasr-ctc-model.ccsherpa-onnx/csrc/offline-medasr-ctc-model.hsherpa-onnx/csrc/offline-model-config.ccsherpa-onnx/csrc/offline-model-config.hsherpa-onnx/csrc/offline-recognizer-ctc-impl.hsherpa-onnx/csrc/offline-recognizer-impl.ccsherpa-onnx/python/csrc/CMakeLists.txtsherpa-onnx/python/csrc/offline-medasr-ctc-model-config.ccsherpa-onnx/python/csrc/offline-medasr-ctc-model-config.hsherpa-onnx/python/csrc/offline-model-config.ccsherpa-onnx/python/csrc/offline-wenet-ctc-model-config.ccsherpa-onnx/python/csrc/offline-wenet-ctc-model-config.hsherpa-onnx/python/sherpa_onnx/offline_recognizer.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-06T04:23:50.237Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:23:50.237Z
Learning: The sherpa-onnx JNI library files are stored in Hugging Face repository at https://huggingface.co/csukuangfj/sherpa-onnx-libs under versioned directories like jni/1.12.7/, and the actual Windows JNI library filename is "sherpa-onnx-jni.dll" as defined in Core.java constants.
Applied to files:
sherpa-onnx/csrc/offline-ctc-model.cc
📚 Learning: 2025-08-06T04:18:47.981Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:18:47.981Z
Learning: In sherpa-onnx Java API, the native library names in Core.java (WIN_NATIVE_LIBRARY_NAME = "sherpa-onnx-jni.dll", UNIX_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.so", MACOS_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.dylib") are copied directly from the compiled binary filenames and should not be changed to match other libraries' naming conventions.
Applied to files:
sherpa-onnx/csrc/offline-ctc-model.cc
🧬 Code graph analysis (8)
sherpa-onnx/python/csrc/offline-model-config.cc (1)
sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.cc (2)
PybindOfflineMedAsrCtcModelConfig(14-20)PybindOfflineMedAsrCtcModelConfig(14-14)
sherpa-onnx/csrc/offline-medasr-ctc-model-config.h (3)
sherpa-onnx/csrc/offline-model-config.h (1)
sherpa_onnx(24-111)sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.h (1)
sherpa_onnx(10-14)sherpa-onnx/csrc/offline-medasr-ctc-model-config.cc (2)
Register(14-19)Register(14-14)
sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.h (2)
sherpa-onnx/csrc/offline-medasr-ctc-model-config.h (1)
sherpa_onnx(12-27)sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.cc (2)
PybindOfflineMedAsrCtcModelConfig(14-20)PybindOfflineMedAsrCtcModelConfig(14-14)
.github/scripts/test-python.sh (12)
.github/scripts/test-offline-ctc.sh (1)
log(5-9).github/scripts/test-offline-tts.sh (1)
log(5-9)scripts/apk/build-apk-kws.sh (1)
log(11-15).github/scripts/test-offline-transducer.sh (1)
log(5-9).github/scripts/test-online-transducer.sh (1)
log(5-9).github/scripts/test-online-ctc.sh (1)
log(5-9).github/scripts/test-offline-moonshine.sh (1)
log(5-9).github/scripts/test-online-punctuation.sh (1)
log(8-12).github/scripts/test-offline-whisper.sh (1)
log(5-9).github/scripts/test-online-paraformer.sh (1)
log(5-9).github/scripts/test-spoken-language-identification.sh (1)
log(5-9).github/scripts/test-offline-punctuation.sh (1)
log(5-9)
sherpa-onnx/csrc/offline-medasr-ctc-model.h (3)
sherpa-onnx/csrc/offline-medasr-ctc-model-config.h (1)
sherpa_onnx(12-27)sherpa-onnx/csrc/offline-model-config.h (1)
sherpa_onnx(24-111)sherpa-onnx/csrc/offline-ctc-model.h (1)
OfflineCtcModel(16-59)
sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.cc (2)
sherpa-onnx/csrc/offline-medasr-ctc-model-config.cc (2)
ToString(30-37)ToString(30-30)sherpa-onnx/csrc/offline-model-config.cc (2)
ToString(177-205)ToString(177-177)
python-api-examples/offline-medasr-ctc-decode-files.py (1)
sherpa-onnx/python/sherpa_onnx/offline_recognizer.py (1)
from_medasr_ctc(541-588)
sherpa-onnx/csrc/offline-medasr-ctc-model.cc (1)
sherpa-onnx/csrc/offline-medasr-ctc-model.h (1)
OfflineMedAsrCtcModel(25-61)
🪛 Ruff (0.14.10)
python-api-examples/offline-medasr-ctc-decode-files.py
49-53: Avoid specifying long messages outside the exception class
(TRY003)
120-120: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
⏰ 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: 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: Release shared-OFF tts-OFF
- GitHub Check: Release shared-OFF tts-ON
- GitHub Check: Debug shared-ON tts-OFF
- GitHub Check: Release shared-ON tts-OFF
- GitHub Check: Debug shared tts-OFF
- GitHub Check: Release shared tts-OFF
- GitHub Check: Release shared tts-ON
- GitHub Check: Release static tts-OFF
- GitHub Check: Release static tts-ON
- GitHub Check: rknn shared ON
- 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.9
- GitHub Check: ubuntu-24.04 3.13
- GitHub Check: ubuntu-24.04 3.12
🔇 Additional comments (35)
sherpa-onnx/python/csrc/offline-wenet-ctc-model-config.h (1)
1-1: LGTM—Comment updated to match filename.The header comment now correctly references the actual filename.
scripts/medasr/export_onnx.py (1)
110-110: LGTM—Subsampling factor metadata added.The subsampling factor metadata is correctly added to support the MedASR CTC model implementation.
sherpa-onnx/python/csrc/offline-wenet-ctc-model-config.cc (1)
1-1: LGTM—Comment updated to match filename.The header comment now correctly references the actual filename.
python-api-examples/offline-medasr-ctc-decode-files.py (4)
26-66: LGTM—Clear validation and recognizer construction.The file validation and recognizer creation logic is well-structured with helpful error messages.
69-73: LGTM—Audio loading is correct.The audio loading with librosa at 16kHz sample rate is appropriate and includes a validation assertion.
76-96: LGTM—Single-file decoding with proper timing.The timing and real-time factor calculation is correctly implemented.
133-138: LGTM—Main function orchestrates decoding correctly.The example demonstrates both single-file and batch decoding workflows.
sherpa-onnx/csrc/offline-model-config.h (3)
12-12: LGTM—MedASR header included.
40-40: LGTM—MedASR config member added.The new member follows the existing pattern for other model configurations.
76-76: LGTM—Constructor updated consistently.The constructor parameter and initializer list are properly extended to include the MedASR configuration.
Also applies to: 95-95
sherpa-onnx/python/csrc/CMakeLists.txt (1)
17-17: LGTM—MedASR Python binding source added to build.The source file is correctly added to the CMake build configuration.
sherpa-onnx/csrc/offline-recognizer-impl.cc (2)
215-223: LGTM—MedASR integrated into CTC model routing.The condition correctly routes MedASR models to the CTC implementation, following the pattern of other CTC models.
546-554: LGTM—Managed Create overload updated consistently.The template Manager version of Create is correctly updated to include MedASR routing.
sherpa-onnx/csrc/offline-ctc-model.cc (3)
24-24: LGTM—MedASR model header included.
130-131: LGTM—MedASR model instantiation added to factory.The factory method correctly creates an OfflineMedAsrCtcModel when the MedASR configuration is present.
198-199: LGTM—Managed factory overload updated consistently.The template Manager version of the factory is correctly extended to support MedASR models.
.github/scripts/test-python.sh (1)
11-20: LGTM! MedASR test workflow follows established patterns.The test block correctly downloads, extracts, validates, and cleans up the MedASR model following the same structure as other model tests in this script.
sherpa-onnx/python/csrc/offline-model-config.cc (3)
14-14: LGTM! MedASR binding integration follows conventions.The include and binding registration follow the same pattern as other model configurations.
Also applies to: 42-42
59-62: LGTM! Constructor binding correctly extended for MedASR.The medasr parameter is properly added to the constructor signature with an appropriate default value.
Also applies to: 76-76
94-94: LGTM! Read/write binding correctly exposed.The medasr field binding follows the established pattern.
.github/workflows/export-medasr-ctc-to-onnx.yaml (2)
6-6: Verify the branch trigger is correct.The workflow triggers on branch
cpp-medasr-2, which appears to be a development/PR branch. Ensure this is intentional or update it to trigger on the appropriate permanent branch (e.g.,masteror a feature branch) before merging.
45-47: LGTM! Extended test coverage with multiple audio files.The loop expansion from a single test file to 6 files (0-5.wav) improves test coverage and follows a consistent pattern across download and test steps.
Also applies to: 58-60, 69-71
sherpa-onnx/csrc/CMakeLists.txt (1)
43-44: LGTM! MedASR source files added to build.The two source files are correctly added to the CMake sources list with proper placement in the alphabetical order.
sherpa-onnx/csrc/offline-model-config.cc (1)
28-28: LGTM! MedASR configuration properly integrated.The medasr configuration is correctly integrated into Register, Validate, and ToString methods, following the same pattern as other model configurations.
Also applies to: 160-162, 194-194
sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.h (1)
1-16: LGTM! Standard Python binding header.The header correctly declares the Python binding function with appropriate include guards and namespace.
sherpa-onnx/csrc/offline-recognizer-ctc-impl.h (3)
42-45: LGTM! End-of-sentence token properly filtered.The token skipping follows the same pattern as the existing SIL token handling and will only apply when is present in the symbol table.
66-68: Verify if leading space trimming should apply to all models.This code unconditionally trims a leading space from the decoded text for all CTC models. If this behavior is only needed for MedASR, consider adding a conditional check (e.g.,
if (!config_.model_config.medasr.model.empty())). If it's intentionally applied to all models, please add a comment explaining why.
156-165: Most MedASR feature extraction parameters are confirmed; frequency range lacks explicit documentation.The feature configuration has been verified against test_onnx.py:
- ✓ Confirmed:
remove_dc_offset=false,dither=0,preemph_coeff=0,window_type="hanning",feature_dim=128,snip_edges=true- Not explicitly documented:
low_freq=125andhigh_freq=7500(test script useskaldi_native_fbankwithout setting frequency bounds, suggesting they may be defaults or handled implicitly)The parameters that are documented match the test implementation exactly.
sherpa-onnx/csrc/offline-medasr-ctc-model-config.h (1)
1-29: LGTM! Well-structured configuration header.The OfflineMedAsrCtcModelConfig struct follows the established pattern for model configuration with appropriate constructors and public methods (Register, Validate, ToString).
sherpa-onnx/python/sherpa_onnx/offline_recognizer.py (1)
540-588: LGTM! The newfrom_medasr_ctcfactory method is well-implemented.The implementation correctly follows the existing pattern established by
from_omnilingual_asr_ctc(lines 590-638), which also omitsfeat_configfor models that handle feature extraction internally. The method signature, documentation, and configuration setup are consistent with the codebase conventions.sherpa-onnx/csrc/offline-medasr-ctc-model-config.cc (1)
14-37: Implementation looks correct.The
Register(),Validate(), andToString()methods follow the established pattern used by other model config classes in the codebase.sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.cc (1)
14-20: Python binding implementation is correct.The binding correctly exposes the constructor,
modelattribute, and__str__method, following the established pattern for other model config bindings.sherpa-onnx/csrc/offline-medasr-ctc-model.h (1)
1-65: Well-structured header file.The header correctly implements the PIMPL pattern with
std::unique_ptr<Impl>, properly inherits fromOfflineCtcModel, and provides comprehensive documentation for theForwardmethod. The include guard and namespace usage are correct.sherpa-onnx/csrc/offline-medasr-ctc-model.cc (2)
61-101: Model initialization and forward pass implementation look correct.The
Implclass properly initializes the ONNX Runtime session, extracts input/output names, validates model metadata, and implements the forward pass with mask tensor creation. The memory management and tensor creation follow the established patterns in the codebase.
163-196: Public API implementation and template instantiations are correct.The forwarding methods properly delegate to the
Implclass, and the template instantiations for Android and OHOS platforms follow the established pattern for cross-platform support.
| int32_t VocabSize() const { return vocab_size_; } | ||
|
|
||
| int32_t SubsamplingFactor() const { return 4; } | ||
|
|
||
| OrtAllocator *Allocator() { return allocator_; } |
There was a problem hiding this comment.
SubsamplingFactor() returns hardcoded value instead of using the metadata field.
The method returns a hardcoded 4 at line 105, but subsampling_factor_ is read from model metadata at lines 141-142 and stored in the member variable. This appears to be a bug or oversight.
Additionally, Allocator() in Impl is non-const (line 107), but the public OfflineMedAsrCtcModel::Allocator() is declared const in the header. This works because impl_ is a unique_ptr, but for consistency the Impl method should also be const.
🔎 Proposed fix
- int32_t SubsamplingFactor() const { return 4; }
+ int32_t SubsamplingFactor() const { return subsampling_factor_; }
- OrtAllocator *Allocator() { return allocator_; }
+ OrtAllocator *Allocator() const { return allocator_; }📝 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.
| int32_t VocabSize() const { return vocab_size_; } | |
| int32_t SubsamplingFactor() const { return 4; } | |
| OrtAllocator *Allocator() { return allocator_; } | |
| int32_t VocabSize() const { return vocab_size_; } | |
| int32_t SubsamplingFactor() const { return subsampling_factor_; } | |
| OrtAllocator *Allocator() const { return allocator_; } |
🤖 Prompt for AI Agents
In sherpa-onnx/csrc/offline-medasr-ctc-model.cc around lines 103-107,
SubsamplingFactor() currently returns a hardcoded 4 instead of the member
subsampling_factor_ (which is read from metadata at ~lines 141-142); change
SubsamplingFactor() to return subsampling_factor_. Also make Impl::Allocator() a
const method to match the public const OfflineMedAsrCtcModel::Allocator()
declaration (i.e., change the signature to be const and return allocator_),
preserving behavior but fixing const-correctness.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for the Google MedASR CTC model to sherpa-onnx. The changes are comprehensive, covering the C++ implementation, Python bindings, a new example script, and updates to the CI test scripts. The code is well-structured and follows the existing patterns in the repository. I have a couple of suggestions to improve code quality and maintainability, mainly regarding code duplication in the Python example and an inconsistency in the C++ model implementation. Overall, this is a solid contribution.
| def create_recognizer(): | ||
| model = "./sherpa-onnx-medasr-ctc-en-int8-2025-12-25/model.int8.onnx" | ||
| tokens = "./sherpa-onnx-medasr-ctc-en-int8-2025-12-25/tokens.txt" | ||
| test_wav_0 = "./sherpa-onnx-medasr-ctc-en-int8-2025-12-25/test_wavs/0.wav" | ||
| test_wav_1 = "./sherpa-onnx-medasr-ctc-en-int8-2025-12-25/test_wavs/1.wav" | ||
| test_wav_2 = "./sherpa-onnx-medasr-ctc-en-int8-2025-12-25/test_wavs/2.wav" | ||
| test_wav_3 = "./sherpa-onnx-medasr-ctc-en-int8-2025-12-25/test_wavs/3.wav" | ||
| test_wav_4 = "./sherpa-onnx-medasr-ctc-en-int8-2025-12-25/test_wavs/4.wav" | ||
| test_wav_5 = "./sherpa-onnx-medasr-ctc-en-int8-2025-12-25/test_wavs/5.wav" | ||
|
|
||
| for f in [ | ||
| model, | ||
| tokens, | ||
| test_wav_0, | ||
| test_wav_1, | ||
| test_wav_2, | ||
| test_wav_3, | ||
| test_wav_4, | ||
| test_wav_5, | ||
| ]: | ||
| if not Path(f).is_file(): | ||
| print(f"{f} does not exist") | ||
|
|
||
| raise ValueError( | ||
| """Please download model files from | ||
| https://github.com/k2-fsa/sherpa-onnx/releases/tag/asr-models | ||
| """ | ||
| ) | ||
| return ( | ||
| sherpa_onnx.OfflineRecognizer.from_medasr_ctc( | ||
| model=model, | ||
| tokens=tokens, | ||
| num_threads=2, | ||
| ), | ||
| test_wav_0, | ||
| test_wav_1, | ||
| test_wav_2, | ||
| test_wav_3, | ||
| test_wav_4, | ||
| test_wav_5, | ||
| ) |
There was a problem hiding this comment.
The create_recognizer function contains a lot of repeated strings for file paths, which makes it harder to read and maintain. You can refactor it by defining a base path and constructing the file paths programmatically. This will make the code cleaner and less error-prone.
def create_recognizer():
model_dir = Path("./sherpa-onnx-medasr-ctc-en-int8-2025-12-25")
model = model_dir / "model.int8.onnx"
tokens = model_dir / "tokens.txt"
test_wavs = [model_dir / "test_wavs" / f"{i}.wav" for i in range(6)]
all_files = [model, tokens] + test_wavs
for f in all_files:
if not f.is_file():
print(f"{f} does not exist")
raise ValueError(
"""Please download model files from
https://github.com/k2-fsa/sherpa-onnx/releases/tag/asr-models
"""
)
recognizer = sherpa_onnx.OfflineRecognizer.from_medasr_ctc(
model=str(model),
tokens=str(tokens),
num_threads=2,
)
return (recognizer,) + tuple(str(w) for w in test_wavs)|
|
||
| int32_t VocabSize() const { return vocab_size_; } | ||
|
|
||
| int32_t SubsamplingFactor() const { return 4; } |
There was a problem hiding this comment.
The SubsamplingFactor() method is hardcoded to return 4, while the Init() function reads the subsampling_factor from the model's metadata into the subsampling_factor_ member variable, which is then unused. This is inconsistent. To ensure the value from the model metadata is used, SubsamplingFactor() should return the subsampling_factor_ member variable.
int32_t SubsamplingFactor() const { return subsampling_factor_; }
csukuangfj
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
sherpa-onnx/csrc/offline-medasr-ctc-model.cc (1)
103-107: SubsamplingFactor fixed; Allocator const-correctness still needs attention.Good work fixing
SubsamplingFactor()to returnsubsampling_factor_instead of the hardcoded value!However,
Allocator()at line 107 should be markedconstto match the public interface declaration at line 184. The method only returns a member pointer and doesn't modify state.🔎 Proposed fix
- OrtAllocator *Allocator() { return allocator_; } + OrtAllocator *Allocator() const { return allocator_; }
🧹 Nitpick comments (1)
sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.cc (1)
7-8: Remove unused<vector>include.The
<vector>header is included but not used in this file. Only<string>is needed for thestd::stringparameter in the constructor binding.🔎 Proposed fix
#include <string> -#include <vector> #include "sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.h"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sherpa-onnx/csrc/offline-medasr-ctc-model-config.ccsherpa-onnx/csrc/offline-medasr-ctc-model.ccsherpa-onnx/python/csrc/offline-medasr-ctc-model-config.cc
🧰 Additional context used
🧬 Code graph analysis (3)
sherpa-onnx/csrc/offline-medasr-ctc-model-config.cc (1)
sherpa-onnx/csrc/offline-model-config.cc (6)
Register(14-61)Register(14-14)Validate(63-175)Validate(63-63)ToString(177-205)ToString(177-177)
sherpa-onnx/csrc/offline-medasr-ctc-model.cc (1)
sherpa-onnx/csrc/offline-medasr-ctc-model.h (1)
OfflineMedAsrCtcModel(25-61)
sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.cc (1)
sherpa-onnx/csrc/offline-medasr-ctc-model-config.cc (2)
ToString(31-38)ToString(31-31)
🔇 Additional comments (7)
sherpa-onnx/csrc/offline-medasr-ctc-model-config.cc (1)
7-38: LGTM! Previous issue resolved.The missing
<sstream>include from the previous review has been added. The implementation correctly follows the established pattern for model configurations in this codebase.Note: Line 19 references PR #2934, while this is PR #2935. Verify if the reference should point to #2935 or if #2934 is a related/prerequisite PR.
sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.cc (1)
14-20: LGTM! Previous copyright issue resolved.The copyright year has been updated to 2025 as suggested in the previous review. The Python binding implementation correctly exposes the constructor,
modelattribute, and string representation, following established patterns in the codebase.sherpa-onnx/csrc/offline-medasr-ctc-model.cc (5)
32-57: LGTM!The
GetMaskhelper correctly constructs a padding mask from the features_length tensor, following the standard pattern of creating a flat (batch_size × max_len) mask with 1s for valid positions and 0s for padding.
63-80: LGTM!The constructors correctly initialize the model from either the filesystem or a platform-specific asset manager, following the established pattern used by other offline models in the codebase.
82-101: LGTM!The
Forwardmethod correctly constructs a mask tensor from the features_length input and invokes the ONNX session with both features and mask inputs. The mask shape is properly derived from the features tensor dimensions.
110-143: LGTM!The
Initmethod properly initializes the ONNX session, validates the model type as "medasr_ctc", and reads metadata includingvocab_sizeandsubsampling_factor(with a sensible default of 4). Error handling and debug logging are appropriate.
163-196: LGTM!The public interface correctly delegates to the
Implclass, and platform-specific template instantiations for Android and OHOS are properly provided. This follows the established pattern used throughout the codebase.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive support for Google MedASR (Medical Automatic Speech Recognition) CTC models to sherpa-onnx, enabling offline speech recognition for medical domain applications. The implementation includes both C++ runtime support and Python API bindings.
Key Changes
- Added C++ implementation for MedASR CTC model with custom feature extraction configuration (128-dim features, 125-7500 Hz frequency range, Hanning window)
- Introduced Python API with
from_medasr_ctc()factory method for creating MedASR recognizers - Enhanced text post-processing to skip
</s>tokens and strip leading spaces specific to MedASR models - Fixed file header comments in offline-wenet-ctc-model-config files (corrected from "wenet-model" to "wenet-ctc-model")
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sherpa-onnx/python/sherpa_onnx/offline_recognizer.py | Added from_medasr_ctc() factory method for creating MedASR recognizers |
| sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.{h,cc} | Python bindings for MedASR model configuration |
| sherpa-onnx/python/csrc/offline-model-config.cc | Integrated MedASR config into Python offline model bindings |
| sherpa-onnx/csrc/offline-medasr-ctc-model.{h,cc} | Core C++ implementation of MedASR CTC model with mask generation |
| sherpa-onnx/csrc/offline-medasr-ctc-model-config.{h,cc} | Configuration structure for MedASR models |
| sherpa-onnx/csrc/offline-recognizer-ctc-impl.h | Added MedASR-specific feature config and text post-processing logic |
| sherpa-onnx/csrc/offline-recognizer-impl.cc | Integrated MedASR into CTC recognizer creation paths |
| sherpa-onnx/csrc/offline-model-config.{h,cc} | Added MedASR config to main model configuration |
| sherpa-onnx/csrc/offline-ctc-model.cc | Added MedASR model instantiation in CTC model factory |
| sherpa-onnx/csrc/CMakeLists.txt | Added MedASR source files to build |
| sherpa-onnx/python/csrc/CMakeLists.txt | Added MedASR Python binding sources to build |
| sherpa-onnx/python/csrc/offline-wenet-ctc-model-config.{h,cc} | Fixed header comment from "wenet-model" to "wenet-ctc-model" |
| scripts/medasr/export_onnx.py | Added subsampling_factor metadata to exported ONNX model |
| python-api-examples/offline-medasr-ctc-decode-files.py | Example demonstrating single and batch file decoding with MedASR |
| .github/workflows/export-medasr-ctc-to-onnx.yaml | Workflow to export and test MedASR models with 6 test audio files |
| .github/scripts/test-python.sh | Added MedASR Python API test to CI pipeline |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,22 @@ | |||
| // sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.cc | |||
| // | |||
| // Copyright (c) 2025 Xiaomi Corporation | |||
There was a problem hiding this comment.
The copyright year is 2023, but this is a new file created in 2025. The copyright year should be updated to 2025 to match the actual creation date of this file, consistent with the corresponding header file and other new files in this PR.
| @@ -0,0 +1,16 @@ | |||
| // sherpa-onnx/python/csrc/offline-medasr-ctc-model-config.h | |||
| // | |||
| // Copyright (c) 2023 Xiaomi Corporation | |||
There was a problem hiding this comment.
The copyright year is 2023, but this is a new file created in 2025. The copyright year should be updated to 2025 to match the actual creation date of this file.
| // Copyright (c) 2023 Xiaomi Corporation | |
| // Copyright (c) 2025 Xiaomi Corporation |
| #include "sherpa-onnx/csrc/offline-medasr-ctc-model-config.h" | ||
|
|
||
| #include <string> | ||
| #include <vector> |
There was a problem hiding this comment.
The include directive for vector is not used in this file. Consider removing it to keep the includes clean and minimal.
| #include <vector> |
|
|
||
| int32_t VocabSize() const { return vocab_size_; } | ||
|
|
||
| int32_t SubsamplingFactor() const { return subsampling_factor_; } |
There was a problem hiding this comment.
The SubsamplingFactor method returns a hardcoded value of 4 instead of using the subsampling_factor_ member variable that is read from the model metadata at line 141-142. This should return subsampling_factor_ to properly respect the value from the model metadata.
|
is it possible to add support for ngram LM? (NOTE: I am asking if I can do it on my own, is so, how? not asking you to add it) |
|
@stqc Already supported. You need to build HLG by yourself. HINT: Search for |
if I am not mistaken it isn't available for C api (i am currently deploying it on device iOS) |
|
We support 12 programming languages, including c api. As said before, we suggest you first have a look at how we use hlg for ctc models. |
Usage
Try it with our Huggingface space
https://huggingface.co/spaces/k2-fsa/automatic-speech-recognition
Build sherpa-onnx
Please refer to our doc: https://k2-fsa.github.io/sherpa/onnx/install/index.html
Download a model
Please download them from
https://github.com/k2-fsa/sherpa-onnx/releases/tag/asr-models
Run it
The output is
Test audio files are
0.mov
1.mov
2.mov
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.