Epoxrt more zipformer ctc models to qnn - #2921
Conversation
dosubot
Bot
added
the
size:L
|
Caution Review failedThe pull request is closed. 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. WalkthroughIntroduces QNN context binary support for ZIPformer CTC models by adding a matrix-generation Python script, updating the CI/CD workflow to use generated build matrices, and extending model-selection logic across Android and C++ components to recognize context binaries as valid alternatives to model files. Changes
Sequence DiagramsequenceDiagram
participant PythonScript as Python Script<br/>(generate_zipformer.py)
participant Matrix as Build Matrix<br/>(GitHub Output)
participant Workflow as Workflow Job<br/>(export-zipformer-ctc-to-qnn)
participant BuildEnv as Build Environment<br/>(Setup & Bootstrap)
participant QNNTools as QNN Tools &<br/>Config Generation
participant Artifacts as Artifact<br/>Storage
PythonScript->>PythonScript: Iterate over SOC × input-duration<br/>combinations
PythonScript->>PythonScript: Conditionally filter<br/>per model_name
PythonScript->>Matrix: Generate & export<br/>config matrix
Matrix->>Workflow: Dispatch parallel jobs<br/>with matrix values
par Matrix-Driven Execution
Workflow->>BuildEnv: Setup Python venv,<br/>toolkit, dependencies
BuildEnv->>QNNTools: Initialize QNN<br/>environment
QNNTools->>QNNTools: Generate context binary<br/>& model configs
and Parallel Artifact Creation
QNNTools->>Artifacts: Create per-target<br/>artifacts (binary, models)
Artifacts->>Artifacts: Package into<br/>tarballs & directories
end
Artifacts->>Matrix: Store artifacts<br/>for release
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
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 integration and management of Zipformer CTC models within the Qualcomm Neural Processing SDK (QNN) framework. It introduces automation for generating QNN export configurations across diverse hardware and model variations, while simultaneously upgrading the C++ runtime to natively support and validate models provided as QNN context binaries. These changes collectively improve the flexibility, robustness, and ease of deploying QNN-optimized speech recognition models. 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 introduces a new Python script (generate_zipformer.py) responsible for generating Zipformer model configurations, including filtering specific model versions and input durations, which are then outputted as JSON. The primary C++ changes modify sherpa-onnx/csrc/offline-recognizer-impl.cc and sherpa-onnx/csrc/offline-zipformer-ctc-model-config.cc to enhance QNN model loading by allowing the OfflineRecognizerZipformerCtcQnnImpl to be initialized and validated based on the presence of a QNN context binary, even if the main model path is not provided. Review feedback suggests improving the readability of the new Python script by combining nested if statements and renaming a variable, and also recommends reformatting multi-line conditions in the C++ implementation for better code consistency.
| if model_name == "20251222": | ||
| if num_seconds not in ["5"]: | ||
| # TODO(fangjun): We only upload model-5-seconds.onnx right now | ||
| continue |
There was a problem hiding this comment.
For better readability, the nested if statements can be combined into a single if with a compound condition. This makes the logic more direct and easier to understand at a glance.
| if model_name == "20251222": | |
| if num_seconds not in ["5"]: | |
| # TODO(fangjun): We only upload model-5-seconds.onnx right now | |
| continue | |
| if model_name == "20251222" and num_seconds != "5": | |
| # TODO(fangjun): We only upload model-5-seconds.onnx right now | |
| continue | |
| ans = [asdict(c) for c in configs] | ||
|
|
||
| print(json.dumps({"include": ans})) |
There was a problem hiding this comment.
The variable name ans is not very descriptive. Renaming it to something like config_dicts would improve code clarity and make it easier for future maintainers to understand its purpose. Remember to update its usage in the print statement as well.
| ans = [asdict(c) for c in configs] | |
| print(json.dumps({"include": ans})) | |
| config_dicts = [asdict(c) for c in configs] | |
| print(json.dumps({"include": config_dicts})) | |
| } else if (!config.model_config.zipformer_ctc.model.empty() || | ||
| !config.model_config.zipformer_ctc.qnn_config.context_binary | ||
| .empty()) { |
There was a problem hiding this comment.
The formatting of this multi-line condition is inconsistent with the surrounding code (e.g., lines 174-175). To improve code consistency and readability, please format it to match the existing style.
} else if (!config.model_config.zipformer_ctc.model.empty() ||
!config.model_config.zipformer_ctc.qnn_config.context_binary.empty()) {| } else if (!config.model_config.zipformer_ctc.model.empty() || | ||
| !config.model_config.zipformer_ctc.qnn_config.context_binary | ||
| .empty()) { |
There was a problem hiding this comment.
The formatting of this multi-line condition is inconsistent with the surrounding code (e.g., lines 497-498). To improve code consistency and readability, please format it to match the existing style.
} else if (!config.model_config.zipformer_ctc.model.empty() ||
!config.model_config.zipformer_ctc.qnn_config.context_binary.empty()) {
1 participant
Please see
https://k2-fsa.github.io/sherpa/onnx/qnn/models.html
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.