Repository navigation
Add Go API for Google MedAsr model - #2952
Conversation
dosubot
Bot
added
the
size:L
|
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 a non-streaming MedASR CTC Go example and internal wrapper, updates Go bindings to support MedASR model config, and integrates build/run steps for this example into the test-go CI workflow (including Windows DLL distribution and cleanup). Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(240,248,255)
participant User as Developer/CI
participant GoApp as Go example (main)
participant GoBindings as scripts/go/sherpa_onnx.go
participant CLib as SherpaONNX C library
participant ModelFiles as MedASR model & tokens
Note over User,GoApp: Run build & execute (local or CI)
end
User->>GoApp: execute binary / run.sh
GoApp->>ModelFiles: ensure tokens and model present (download/unpack)
GoApp->>GoBindings: create OfflineRecognizerConfig (includes MedAsr model path)
GoBindings->>CLib: marshal config -> C struct (set model_config.medasr.model)
GoApp->>CLib: create stream, send waveform samples
CLib-->>GoApp: decoding events/results
GoApp->>User: print recognized text
Note right of CLib: cleanup frees C strings (medasr.model)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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 the 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
|
![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 model to the Go API. This includes changes to the Go bindings to incorporate the new model configuration and a new example demonstrating its usage. The changes to the Go bindings are correct and follow the existing conventions. The new example code has a few issues, mainly related to error handling and performance, which should be addressed. I've provided specific comments and suggestions for these improvements.
| } | ||
|
|
||
| func readWave(filename string) (samples []float32, sampleRate int) { | ||
| file, _ := os.Open(filename) |
There was a problem hiding this comment.
The error returned by os.Open() is ignored. If the file does not exist or cannot be opened, this will lead to a panic later in the code. You should handle this error to make the program more robust.
| file, _ := os.Open(filename) | |
| file, err := os.Open(filename) | |
| if err != nil { | |
| log.Fatalf("Failed to open %s: %v", filename, err) | |
| } |
| n, err := reader.Read(buf) | ||
| if n != int(reader.Size) { | ||
| log.Fatalf("Failed to read %v bytes. Returned %v bytes\n", reader.Size, n) | ||
| } |
There was a problem hiding this comment.
The error returned by reader.Read() is not checked. An error during reading could lead to processing incomplete or corrupt data. It's important to handle this error. Also, log.Fatalf automatically adds a newline, so the \n at the end of the format string is redundant.
| n, err := reader.Read(buf) | |
| if n != int(reader.Size) { | |
| log.Fatalf("Failed to read %v bytes. Returned %v bytes\n", reader.Size, n) | |
| } | |
| n, err := reader.Read(buf) | |
| if err != nil { | |
| log.Fatalf("Failed to read wave data: %v", err) | |
| } | |
| if n != int(reader.Size) { | |
| log.Fatalf("Failed to read %v bytes. Got %v bytes", reader.Size, n) | |
| } |
| config.ModelConfig.MedAsr.Model = "./sherpa-onnx-medasr-ctc-en-int8-2025-12-25/model.int8.onnx" | ||
| config.ModelConfig.Tokens = "./sherpa-onnx-medasr-ctc-en-int8-2025-12-25/tokens.txt" | ||
|
|
||
| waveFilename := "./sherpa-onnx-medasr-ctc-en-int8-2025-12-25/test_wavs/0.wav" |
There was a problem hiding this comment.
The file paths for the model, tokens, and wave file are hardcoded. While this is an example, making them configurable via command-line flags would make the example more flexible and easier to use with different files. Consider using the flag package for this.
| format, err := reader.Format() | ||
| if err != nil { | ||
| log.Fatalf("Failed to read wave format") | ||
| } |
There was a problem hiding this comment.
When logging a fatal error, it's helpful to include the underlying error message for better debugging.
| format, err := reader.Format() | |
| if err != nil { | |
| log.Fatalf("Failed to read wave format") | |
| } | |
| format, err := reader.Format() | |
| if err != nil { | |
| log.Fatalf("Failed to read wave format: %v", err) | |
| } |
| for i := 0; i != numSamples; i++ { | ||
| s := inSamples[i*2 : (i+1)*2] | ||
|
|
||
| var s16 int16 | ||
| buf := bytes.NewReader(s) | ||
| err := binary.Read(buf, binary.LittleEndian, &s16) | ||
| if err != nil { | ||
| log.Fatal("Failed to parse 16-bit sample") | ||
| } | ||
| outSamples[i] = float32(s16) / 32768 | ||
| } |
There was a problem hiding this comment.
The current implementation of samplesInt16ToFloat creates a new bytes.Reader for every sample inside the loop, which is inefficient. You can achieve better performance by directly reading from the byte slice using binary.LittleEndian.Uint16.
for i := 0; i < numSamples; i++ {
s16 := int16(binary.LittleEndian.Uint16(inSamples[i*2:]))
outSamples[i] = float32(s16) / 32768.0
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/go/_internal/non-streaming-medasr-ctc-decode-files/.gitignore (1)
1-1: Consider adding Windows executable pattern for cross-platform support.The
.gitignoreentry currently ignores only the Unix binary name. For cross-platform projects, add a pattern to also ignore Windows executables (.exeextension).🔎 Proposed improvement
-non-streaming-medasr-ctc-decode-files +non-streaming-medasr-ctc-decode-files +*.exeAlternatively, use a glob pattern to match both:
-non-streaming-medasr-ctc-decode-files +non-streaming-medasr-ctc-decode-files*
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/test-go.yamlgo-api-examples/non-streaming-medasr-ctc-decode-files/go.modgo-api-examples/non-streaming-medasr-ctc-decode-files/main.gogo-api-examples/non-streaming-medasr-ctc-decode-files/run.shscripts/go/_internal/non-streaming-medasr-ctc-decode-files/.gitignorescripts/go/_internal/non-streaming-medasr-ctc-decode-files/go.modscripts/go/_internal/non-streaming-medasr-ctc-decode-files/main.goscripts/go/_internal/non-streaming-medasr-ctc-decode-files/run.shscripts/go/sherpa_onnx.go
🧰 Additional context used
🧬 Code graph analysis (2)
scripts/go/sherpa_onnx.go (1)
sherpa-onnx/c-api/cxx-api.h (1)
OfflineMedAsrCtcModelConfig(275-277)
go-api-examples/non-streaming-medasr-ctc-decode-files/main.go (1)
scripts/go/sherpa_onnx.go (5)
OfflineRecognizerConfig(513-529)NewOfflineRecognizer(830-842)DeleteOfflineRecognizer(823-826)NewOfflineStream(860-864)DeleteOfflineStream(853-856)
🔇 Additional comments (15)
scripts/go/_internal/non-streaming-medasr-ctc-decode-files/main.go (1)
1-1: LGTM - Consistent reference pattern.This file follows the project's pattern of referencing the actual implementation from the internal test directory.
scripts/go/_internal/non-streaming-medasr-ctc-decode-files/go.mod (1)
1-15: LGTM - Complete module configuration.The module manifest properly declares all dependencies with the correct replace directive for local development.
scripts/go/sherpa_onnx.go (4)
426-428: LGTM - Consistent struct definition.The
OfflineMedAsrCtcModelConfigstruct follows the established pattern for CTC model configurations and matches the C++ API structure.
492-492: LGTM - Proper integration into model config.The MedAsr field is correctly integrated into
OfflineModelConfigalongside other model types.
597-597: LGTM - Correct C string allocation.The C string is properly allocated and will be freed by
freeCOfflineRecognizerConfig.
751-754: LGTM - Proper memory cleanup.The MedASR model string is correctly freed with appropriate nil check, following the established pattern.
go-api-examples/non-streaming-medasr-ctc-decode-files/main.go (3)
14-42: LGTM - Clear example implementation.The main function demonstrates proper usage of the MedASR model with appropriate resource cleanup using defer statements.
66-72: LGTM - Proper wave data reading.The code correctly initializes the reader size and validates the number of bytes read.
80-97: LGTM - Correct PCM to float conversion.The function properly converts 16-bit PCM samples to normalized float32 values with appropriate endianness handling.
scripts/go/_internal/non-streaming-medasr-ctc-decode-files/run.sh (1)
1-1: LGTM - Consistent reference pattern.This file follows the project's pattern of referencing the actual implementation script from the internal test directory.
go-api-examples/non-streaming-medasr-ctc-decode-files/run.sh (3)
1-5: LGTM - Proper shell script setup.The script correctly enables error handling and sets CGO_ENABLED for C bindings.
7-11: LGTM - Efficient model download.The script efficiently checks for existing files before downloading and properly cleans up the archive.
13-15: LGTM - Standard Go build workflow.The build and execution sequence is correct.
.github/workflows/test-go.yaml (2)
105-105: LGTM - Consistent Windows DLL distribution.The DLL copy follows the established pattern for other test directories.
145-157: LGTM - Well-structured test step.The MedASR test step follows the established pattern for other model tests with appropriate cleanup.
| module non-streaming-medasr-ctc-decode-files | ||
|
|
||
| go 1.17 |
There was a problem hiding this comment.
Add missing dependencies to the module manifest.
The main.go file imports github.com/k2-fsa/sherpa-onnx-go/sherpa_onnx and github.com/youpy/go-wav, but this go.mod does not declare them. Users attempting to build this example directly will encounter unresolved dependency errors.
🔎 Add the missing dependencies
module non-streaming-medasr-ctc-decode-files
go 1.17
+
+replace github.com/k2-fsa/sherpa-onnx-go/sherpa_onnx => ../../scripts/go
+
+require (
+ github.com/k2-fsa/sherpa-onnx-go/sherpa_onnx v0.0.0-00010101000000-000000000000
+ github.com/youpy/go-wav v0.3.2
+)📝 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.
| module non-streaming-medasr-ctc-decode-files | |
| go 1.17 | |
| module non-streaming-medasr-ctc-decode-files | |
| go 1.17 | |
| replace github.com/k2-fsa/sherpa-onnx-go/sherpa_onnx => ../../scripts/go | |
| require ( | |
| github.com/k2-fsa/sherpa-onnx-go/sherpa_onnx v0.0.0-00010101000000-000000000000 | |
| github.com/youpy/go-wav v0.3.2 | |
| ) |
🤖 Prompt for AI Agents
In go-api-examples/non-streaming-medasr-ctc-decode-files/go.mod lines 1-3, the
module manifest is missing required dependencies used in main.go; add require
directives for github.com/k2-fsa/sherpa-onnx-go/sherpa_onnx and
github.com/youpy/go-wav with appropriate semantic versions (or run the module
add command to resolve and populate go.mod and go.sum) so the example builds
without unresolved dependency errors.
| func readWave(filename string) (samples []float32, sampleRate int) { | ||
| file, _ := os.Open(filename) | ||
| defer file.Close() |
There was a problem hiding this comment.
Fix unhandled file open error.
The error from os.Open is ignored, which will cause a nil pointer dereference panic when defer file.Close() is executed if the file cannot be opened.
🔎 Handle the error properly
func readWave(filename string) (samples []float32, sampleRate int) {
- file, _ := os.Open(filename)
+ file, err := os.Open(filename)
+ if err != nil {
+ log.Fatalf("Failed to open file: %v", err)
+ }
defer file.Close()📝 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.
| func readWave(filename string) (samples []float32, sampleRate int) { | |
| file, _ := os.Open(filename) | |
| defer file.Close() | |
| func readWave(filename string) (samples []float32, sampleRate int) { | |
| file, err := os.Open(filename) | |
| if err != nil { | |
| log.Fatalf("Failed to open file: %v", err) | |
| } | |
| defer file.Close() |
🤖 Prompt for AI Agents
In go-api-examples/non-streaming-medasr-ctc-decode-files/main.go around lines
44-46, the call to os.Open ignores the error and defers file.Close()
immediately, which can panic if the open fails; change the code to capture the
error (file, err := os.Open(filename)), check if err != nil and handle it
(return nil, 0 or propagate/log the error as appropriate for this package), and
only call defer file.Close() after confirming file is non-nil; ensure callers
can distinguish the failure by returning zero values or returning the error up
the call chain.
1 participant
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.