Repository navigation
Export models for Ascend 910B4 - #2878
Conversation
dosubot
Bot
added
the
size:M
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 enhances the model export process by integrating support for the new Ascend 910B4 chip. It achieves this by refactoring how supported CANN and SOC versions are managed, centralizing these configurations into reusable functions. This update ensures that all relevant scripts consistently use the same version definitions, streamlining future maintenance and hardware compatibility updates. 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
|
WalkthroughRefactors hardcoded CANN and SOC version lists into dynamic retrieval functions. Introduces Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
![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 Ascend 910B4 by updating the list of supported SoC versions. It also refactors the scripts to centralize the lists of supported CANN and SoC versions into functions, which improves maintainability by reducing code duplication. My review includes a suggestion to further improve this refactoring by using module-level constants instead of functions for these static lists, which is a more idiomatic approach in Python.
| def get_soc_version(): | ||
| soc_version = ["910B", "910B2", "910B3", "910B4", "310P3"] | ||
| return soc_version | ||
|
|
||
|
|
||
| def get_cann_version(): | ||
| cann_version = ["7.0", "8.0", "8.2"] | ||
| return cann_version |
There was a problem hiding this comment.
For lists of values that are fixed and don't change at runtime, it's more idiomatic and clearer to define them as module-level constants rather than wrapping them in functions. This signals that these are static configuration values.
I suggest replacing these functions with constants. You would then need to update the call sites in this file, generate_paraformer.py, and generate_sense_voice.py to import and use these constants directly.
For example, in generate_paraformer.py:
from generate_zipformer_ctc_20250703 import get_image, SUPPORTED_CANN_VERSIONS, SUPPORTED_SOC_VERSIONS
def main():
cann_version = SUPPORTED_CANN_VERSIONS
soc_version = SUPPORTED_SOC_VERSIONS
# ...SUPPORTED_SOC_VERSIONS = ["910B", "910B2", "910B3", "910B4", "310P3"]
SUPPORTED_CANN_VERSIONS = ["7.0", "8.0", "8.2"]
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/scripts/export-ascend/generate_sense_voice.py (1)
8-31: Good reuse of shared CANN/SOC helpers; consider updating the comment for 910B4Importing
get_image,get_soc_version, andget_cann_versionfromgenerate_zipformer_ctc_20250703keeps the SenseVoice matrix generation in sync with zipformer/paraformer and avoids hard‑coding the version lists in multiple places. The import style will work when this script is invoked as done in the workflow (python3 .github/scripts/export-ascend/generate_sense_voice.py).Minor nit: now that
get_soc_version()includes"910B4", theConfig.soc_versioncomment still listing910B, 910B2, 910B3, 310P3is slightly stale. You may want to extend the comment to include910B4for accuracy..github/workflows/export-paraformer-to-ascend-npu.yaml (1)
98-199: Optional: job id/name still mentions RKNNThe job id
export-paraformer-to-rknnno longer matches the job name (export-paraformer-to-ascend-npu) or what the steps actually do. This is purely cosmetic, but renaming the job id (and fixing the smallnputtypo in the concurrency group) in a follow‑up would make the workflow easier to read..github/scripts/export-ascend/generate_zipformer_ctc_20250703.py (1)
35-43: Centralized CANN/SOC version helpers are straightforward and reusableDefining
get_soc_version()andget_cann_version()here is a clean way to keep the supported versions in one place for all export scripts. The values match the keys used inget_image, and adding"910B4"while still routing it through the"910"image mapping is consistent with the existing"910B*"handling.Minor nit: the
Config.soc_versioncomment below still lists910B, 910B2, 910B3, 310P3. Consider updating it to mention910B4so the docs stay in sync withget_soc_version()..github/scripts/export-ascend/generate_paraformer.py (1)
8-37: Paraformer generator correctly reuses shared version/image helpersImporting
get_cann_version,get_image, andget_soc_versionfromgenerate_zipformer_ctc_20250703and using them inmain()ensures the paraformer matrix stays aligned with the zipformer one. The generated configs (withframework,soc_version,cann,image) match what the workflow expects, and the framework names (FunASR,WSChuan-ASR) are consistent with theif: matrix.framework == ...checks.As with the other scripts,
get_soc_version()now includes"910B4", so you may want to update theConfig.soc_versioncomment that currently lists only910B, 910B2, 910B3, 310P3to include910B4for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/scripts/export-ascend/generate_paraformer.py(2 hunks).github/scripts/export-ascend/generate_sense_voice.py(2 hunks).github/scripts/export-ascend/generate_zipformer_ctc_20250703.py(2 hunks).github/workflows/export-paraformer-to-ascend-npu.yaml(5 hunks).github/workflows/export-sense-voice-to-ascend-npu.yaml(1 hunks).github/workflows/export-zipformer-ctc-to-ascend-20250703.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
.github/scripts/export-ascend/generate_sense_voice.py (1)
.github/scripts/export-ascend/generate_zipformer_ctc_20250703.py (3)
get_image(15-32)get_soc_version(35-37)get_cann_version(40-42)
🔇 Additional comments (6)
.github/workflows/export-sense-voice-to-ascend-npu.yaml (1)
3-7: Trigger branch update looks good; just confirm the target branch nameThe push trigger now listens on
ascend-910b4-2, matching the other updated workflows in this PR. Please double‑check thatascend-910b4-2is the long‑lived branch you intend to use for these Ascend exports before merging..github/workflows/export-zipformer-ctc-to-ascend-20250703.yaml (1)
3-7: Zipformer export workflow trigger aligned with new branchSwitching the push trigger to
ascend-910b4-2keeps this workflow consistent with the other Ascend export workflows. As with the others, just ensure this branch is the canonical one you’ll be using for 910B4 exports..github/workflows/export-paraformer-to-ascend-npu.yaml (3)
3-7: Paraformer workflow now tied to the new Ascend branchThe push trigger now targets
ascend-910b4-2, keeping this workflow consistent with the other Ascend export jobs. Confirm that this is the branch you intend to use for ongoing paraformer exports.
25-31: Switch togenerate_paraformer.pyfor the build matrix is consistent with the new helpersUsing
.github/scripts/export-ascend/generate_paraformer.pyfor the matrix keeps paraformer’s CANN/SOC/framework combinations in sync with the shared helpers ingenerate_zipformer_ctc_20250703.py. That will reduce drift versus the zipformer/sense‑voice matrices and looks correct given the matrix keys referenced in this workflow.
68-80: Extra LD_LIBRARY_PATH entry for CANN 7.0.0 is reasonableAdding the second
LD_LIBRARY_PATHentry fordevlib/x86_64(with the# for cann 7.0.0comment) mirrors the pattern in your other Ascend workflows and should help with 7.0.0 layout differences while remaining harmless for other CANN versions. No functional issues here..github/scripts/export-ascend/generate_zipformer_ctc_20250703.py (1)
61-69: main() now reuses the helpers; matrix generation stays compatibleUsing
cann_version = get_cann_version()andsoc_version = get_soc_version()keeps this script aligned with the other generators that import these helpers. The resulting config objects still provide thecann,soc_version,num_seconds, andimagefields expected by the GitHub Actions matrix, so behavior remains the same except for the new 910B4 coverage.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.