Repository navigation
Export omnilingual-asr to sherpa-onnx - #2770
Conversation
dosubot
Bot
added
the
size:L
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 integrates 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
|
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces complete end-to-end automation for exporting omnilingual ASR models to ONNX format, consisting of a GitHub Actions workflow, Python export script with metadata and quantization, test validation script, and documentation describing the export process. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Workflow as GitHub Actions<br/>Workflow
participant Export as export-onnx.py
participant Model as ONNX Model
participant Quantize as Quantizer
participant Test as test.py
participant HF as HuggingFace Hub
participant Release as GitHub Releases
User->>Workflow: Push to export-omnilingual-asr
Workflow->>Export: Run export script
Export->>Model: Wrap & export inference model
Model->>Export: ONNX file (model.onnx)
Export->>Quantize: Apply int8 quantization
Quantize->>Export: Quantized model (model.int8.onnx)
Note over Export: Generate tokens.txt<br/>Add metadata
Workflow->>Test: Run validation tests
Test->>Model: Load & run inference
Model-->>Test: Logits output
Test->>Test: Decode & measure RTF
Workflow->>Workflow: Package artifacts
Workflow->>HF: Push to HuggingFace<br/>(with Git LFS)
Workflow->>Release: Upload tarballs<br/>to GitHub Releases
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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 introduces scripts for exporting the omnilingual-asr model to ONNX and for testing the exported models. The implementation is mostly solid, but I've identified a few areas for improvement. A key issue is in export-onnx.py where batch processing is handled incorrectly, which would cause errors for batch sizes greater than one. In test.py, a hardcoded CTC blank ID could lead to incorrect decoding results. I've also made some suggestions to improve code clarity and maintainability in the export script and the documentation. Please see my detailed comments for suggestions on how to address these points.
| Args: | ||
| x: (N, num_samples), float32 | ||
| """ | ||
| batch_layout = BatchLayout(shape=x.shape, seq_lens=[x.shape[1]]) |
There was a problem hiding this comment.
The seq_lens argument for BatchLayout is incorrectly constructed for batch sizes greater than 1. It is currently [x.shape[1]], which means it's a list with a single element. This will fail if the batch size x.shape[0] is greater than 1. To support batching correctly, it should be a list of sequence lengths for each item in the batch.
batch_layout = BatchLayout(shape=x.shape, seq_lens=[x.shape[1]] * x.shape[0])| ids = logits[0].argmax(axis=-1) | ||
| ans = [] | ||
| prev = -1 | ||
| blank = 0 |
There was a problem hiding this comment.
The CTC blank token ID is hardcoded as 0. This is a 'magic number' and might be incorrect, leading to decoding errors. It's better to define it as a named constant. Ideally, the blank ID should be saved as part of the model metadata during export and read from there in the test script to make it more robust.
| ``` | ||
| num_frames = round(num_samples / 318 - 1.5) | ||
| num_samples = round(318 * num_frames + 477) | ||
|
|
||
| or | ||
| num_frames = round(num_samples / 320) | ||
|
|
||
| ``` | ||
|
|
||
| 20ms per frame |
There was a problem hiding this comment.
The formulas provided for num_frames and num_samples are a bit confusing. The formula num_frames = round(num_samples / 320) is consistent with a 20ms frame duration at a 16kHz sampling rate (since 16000 * 0.020 = 320). However, the other set of formulas using 318 and 477 seems to contradict this. Could you please clarify the relationship between these different formulas and explain when each should be used? Providing context on how these numbers are derived from the model architecture would be very helpful for users.
| while len(model.metadata_props): | ||
| model.metadata_props.pop() |
There was a problem hiding this comment.
This while loop to clear the metadata properties is inefficient. The RepeatedCompositeContainer returned by model.metadata_props has a clear() method that is more efficient and idiomatic for this purpose.
model.metadata_props.clear()| vocab_size = pipeline.tokenizer._model.vocabulary_size | ||
|
|
||
| with open("tokens.txt", "w") as f: | ||
| for i in range(pipeline.tokenizer._model.vocabulary_size): | ||
| f.write(f"{pipeline.tokenizer._model.index_to_token(i)} {i}\n") |
There was a problem hiding this comment.
Accessing the protected member _model of pipeline.tokenizer is fragile and can lead to issues if the omnilingual-asr library is updated. If there is a public API to get the vocabulary size and map indices to tokens, it would be much safer to use that. If not, it would be good to add a comment here acknowledging the risk.
| if len(fields) == 1: | ||
| id2token[int(fields[0])] = " " |
There was a problem hiding this comment.
The special handling for lines where len(fields) == 1 is not immediately clear. It would be helpful to add a comment explaining this case (e.g., if it's for handling the space token) to improve code readability and maintainability.
1 participant
See also
https://github.com/facebookresearch/omnilingual-asr
Download
You can find the exported models at
RTF test in GitHub actions (ubuntu-latest, 1 thread)
https://github.com/csukuangfj/sherpa-onnx/actions/runs/19296999620/job/55181385857#step:7:140
Summary by CodeRabbit
New Features
Documentation
Tests