Repository navigation
Add Pascal API for Moonshine v2 - #3248
Conversation
dosubot
Bot
added
the
size:L
Summary of ChangesHello, 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 expands the Pascal API within Highlights
Changelog
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
|
📝 WalkthroughWalkthroughThis PR extends Sherpa ONNX's Pascal binding to support Moonshine v2 ONNX models. Changes include adding a Changes
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 Moonshine v2 model to the Pascal API, including a new example program and a run script. The changes to the sherpa_onnx.pas file correctly add the MergedDecoder field for the new model. My feedback focuses on improving the maintainability of the new example script and Pascal program by reducing hardcoded strings.
| Initialize(Config); | ||
|
|
||
| Config.ModelConfig.Moonshine.Encoder := './sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27/encoder_model.ort'; | ||
| Config.ModelConfig.Moonshine.MergedDecoder := './sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27/decoder_model_merged.ort'; | ||
|
|
||
| Config.ModelConfig.Tokens := './sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27/tokens.txt'; | ||
| Config.ModelConfig.Provider := 'cpu'; | ||
| Config.ModelConfig.NumThreads := 1; | ||
| Config.ModelConfig.Debug := False; | ||
|
|
||
| WaveFilename := './sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27/test_wavs/0.wav'; |
There was a problem hiding this comment.
The model directory path is repeated multiple times. To improve maintainability, you can define it as a local variable. This makes it easier to update the path in one place if needed.
var
ModelDir: AnsiString;
Initialize(Config);
ModelDir := './sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27';
Config.ModelConfig.Moonshine.Encoder := ModelDir + '/encoder_model.ort';
Config.ModelConfig.Moonshine.MergedDecoder := ModelDir + '/decoder_model_merged.ort';
Config.ModelConfig.Tokens := ModelDir + '/tokens.txt';
Config.ModelConfig.Provider := 'cpu';
Config.ModelConfig.NumThreads := 1;
Config.ModelConfig.Debug := False;
WaveFilename := ModelDir + '/test_wavs/0.wav';
| if [ ! -f ./sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27/encoder_model.ort ]; then | ||
| curl -SL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27.tar.bz2 | ||
| tar xvf sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27.tar.bz2 | ||
| rm sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27.tar.bz2 | ||
| fi |
There was a problem hiding this comment.
The model name sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27 is hardcoded multiple times. To improve maintainability, it's better to define it as a variable. This makes it easier to update the model version in the future.
| if [ ! -f ./sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27/encoder_model.ort ]; then | |
| curl -SL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27.tar.bz2 | |
| tar xvf sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27.tar.bz2 | |
| rm sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27.tar.bz2 | |
| fi | |
| model_name="sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27" | |
| if [ ! -f ./${model_name}/encoder_model.ort ]; then | |
| curl -SL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/${model_name}.tar.bz2 | |
| tar xvf ${model_name}.tar.bz2 | |
| rm ${model_name}.tar.bz2 | |
| fi |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pascal-api-examples/non-streaming-asr/run-moonshine-v2.sh (2)
22-24: Minor:lsshows build directory, not install directory.The
ls -lh libcommand lists the build output directory, but the library check at line 10 looks for files in../../build/install/lib. For consistency, consider listing the install directory instead:🔧 Proposed fix
cmake --build . --target install --config Release - ls -lh lib + ls -lh install/lib popd🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pascal-api-examples/non-streaming-asr/run-moonshine-v2.sh` around lines 22 - 24, The script currently runs the build (cmake --build . --target install --config Release) but then executes ls -lh lib which lists the build output directory; change that ls invocation to list the actual install directory used by the earlier check (use the ../../build/install/lib path referenced earlier) so the directory inspected matches the install check—update the ls command (replace ls -lh lib) to ls -lh ../../build/install/lib (or equivalent install dir variable) to keep outputs consistent.
5-6: Quote variable in path to handle spaces.The
$SCRIPT_DIRvariable should be quoted when used in thecdcommand to properly handle paths containing spaces.🔧 Proposed fix
SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd ) -SHERPA_ONNX_DIR=$(cd $SCRIPT_DIR/../.. && pwd) +SHERPA_ONNX_DIR=$(cd "$SCRIPT_DIR/../.." && pwd)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pascal-api-examples/non-streaming-asr/run-moonshine-v2.sh` around lines 5 - 6, The SHERPA_ONNX_DIR assignment uses cd $SCRIPT_DIR/../.. which will break on paths with spaces; update the cd invocation to quote the SCRIPT_DIR variable (use "$SCRIPT_DIR" when composing the path) so the shell treats the directory with spaces as a single argument and SHERPA_ONNX_DIR is computed correctly; reference: variables SCRIPT_DIR and SHERPA_ONNX_DIR and the use of BASH_SOURCE in the SCRIPT_DIR assignment.pascal-api-examples/non-streaming-asr/moonshine_v2.pas (1)
46-48: Consider adding error handling for wave file loading.If
SherpaOnnxReadWavefails to load the file (file not found, invalid format),Wave.Sampleswill be empty andWave.SampleRatewill be 0, causing a division by zero at line 62 when calculatingDuration.While this example script is straightforward and the run script ensures the file exists, adding a simple check would make the example more robust for users adapting it:
💡 Proposed defensive check
WaveFilename := './sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27/test_wavs/0.wav'; Wave := SherpaOnnxReadWave(WaveFilename); + + if (Wave.Samples = nil) or (Wave.SampleRate = 0) then + begin + WriteLn('Failed to read wave file: ', WaveFilename); + Exit; + end; Recognizer := TSherpaOnnxOfflineRecognizer.Create(Config);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pascal-api-examples/non-streaming-asr/moonshine_v2.pas` around lines 46 - 48, Check the result of SherpaOnnxReadWave for a valid waveform before using Wave.Samples/ Wave.SampleRate: after calling SherpaOnnxReadWave(WaveFilename) validate that (Wave.SampleRate > 0) and (Length(Wave.Samples) > 0), and if the check fails emit a clear message including WaveFilename (e.g., Writeln or process logger) and abort (Halt(1) or raise an exception) so the subsequent Duration calculation cannot divide by zero; update references around WaveFilename, SherpaOnnxReadWave, Wave.Samples and Wave.SampleRate accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pascal-api-examples/non-streaming-asr/moonshine_v2.pas`:
- Around line 46-48: Check the result of SherpaOnnxReadWave for a valid waveform
before using Wave.Samples/ Wave.SampleRate: after calling
SherpaOnnxReadWave(WaveFilename) validate that (Wave.SampleRate > 0) and
(Length(Wave.Samples) > 0), and if the check fails emit a clear message
including WaveFilename (e.g., Writeln or process logger) and abort (Halt(1) or
raise an exception) so the subsequent Duration calculation cannot divide by
zero; update references around WaveFilename, SherpaOnnxReadWave, Wave.Samples
and Wave.SampleRate accordingly.
In `@pascal-api-examples/non-streaming-asr/run-moonshine-v2.sh`:
- Around line 22-24: The script currently runs the build (cmake --build .
--target install --config Release) but then executes ls -lh lib which lists the
build output directory; change that ls invocation to list the actual install
directory used by the earlier check (use the ../../build/install/lib path
referenced earlier) so the directory inspected matches the install check—update
the ls command (replace ls -lh lib) to ls -lh ../../build/install/lib (or
equivalent install dir variable) to keep outputs consistent.
- Around line 5-6: The SHERPA_ONNX_DIR assignment uses cd $SCRIPT_DIR/../..
which will break on paths with spaces; update the cd invocation to quote the
SCRIPT_DIR variable (use "$SCRIPT_DIR" when composing the path) so the shell
treats the directory with spaces as a single argument and SHERPA_ONNX_DIR is
computed correctly; reference: variables SCRIPT_DIR and SHERPA_ONNX_DIR and the
use of BASH_SOURCE in the SCRIPT_DIR assignment.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/pascal.yamlpascal-api-examples/non-streaming-asr/.gitignorepascal-api-examples/non-streaming-asr/moonshine_v2.paspascal-api-examples/non-streaming-asr/run-moonshine-v2.shsherpa-onnx/pascal-api/sherpa_onnx.pas
There was a problem hiding this comment.
Pull request overview
This PR adds Pascal API support for Moonshine v2 speech recognition models, which use a different architecture than Moonshine v1 (a single MergedDecoder instead of separate UncachedDecoder/CachedDecoder + Preprocessor). It extends the existing Moonshine model configuration in the Pascal bindings and adds a full working example.
Changes:
- Adds
MergedDecoderfield to theTSherpaOnnxOfflineMoonshineModelConfigPascal record (and corresponding C-interop struct), matching the field already defined in the C header. - Introduces a new example program
moonshine_v2.pasand build/run scriptrun-moonshine-v2.shdemonstrating usage with the v2 model. - Registers the new example in the CI workflow and
.gitignore.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
sherpa-onnx/pascal-api/sherpa_onnx.pas |
Adds MergedDecoder field to both the high-level Pascal record and C-interop struct; updates ToString and ConvertOfflineRecognizerConfig accordingly |
pascal-api-examples/non-streaming-asr/moonshine_v2.pas |
New example program using Moonshine v2 with encoder + merged decoder |
pascal-api-examples/non-streaming-asr/run-moonshine-v2.sh |
New build and run script for the moonshine v2 example |
pascal-api-examples/non-streaming-asr/.gitignore |
Adds moonshine_v2 binary to the ignore list |
.github/workflows/pascal.yaml |
Adds CI step to run the new moonshine v2 example |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rm -rf sherpa-onnx-* | ||
| echo "---" | ||
|
|
||
| ./run-moonshine-v2.sh |
There was a problem hiding this comment.
The ./run-moonshine-v2.sh step is missing the rm -rf sherpa-onnx-* and echo "---" lines that every other step in the workflow uses. While the moonshine-v2 directory (sherpa-onnx-moonshine-tiny-en-quantized-2026-02-27) will eventually be cleaned up by the rm -rf sherpa-onnx-* that follows run-moonshine.sh, the inconsistency could leave artifacts during the run-moonshine.sh run. The pattern used throughout the workflow is that each script is immediately followed by its own cleanup and separator. This should be corrected to match the established pattern.
| ./run-moonshine-v2.sh | |
| ./run-moonshine-v2.sh | |
| rm -rf sherpa-onnx-* | |
| echo "---" |
Summary by CodeRabbit
New Features
Tests