Upload supertonic tts models#3263
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 integrates new Supertonic Text-to-Speech models, significantly expanding the system's multilingual TTS capabilities. It also refines the model acquisition process by switching to a direct Highlights
Changelog
Ignored Files
Activity
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 failedPull request was closed or merged during review 📝 WalkthroughWalkthroughUpdates CI/CD: change trigger branch and runner to macOS, add brew-based tooling, implement multi-item publishing loops for HuggingFace and ModelScope (with LFS support), adjust artifact naming and retrieval, and refactor Stage 0 asset download from ModelScope to a git clone move. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Runner (macOS)
participant Up as Upstream Repo (git)
participant HF as HuggingFace Repo
participant MS as ModelScope Repo
participant Art as Artifacts (tar.bz2, wav, LICENSE, README)
CI->>Up: git clone supertonic repo (stage0 assets)
Up-->>CI: repository contents (assets/)
CI->>Art: prepare artifacts, rename destination
CI->>HF: clone/push per-item (track wav, README, LICENSE)
HF-->>CI: confirm upload
CI->>MS: clone modelscope repo per artifact, copy files, git lfs track tar.bz2, push
MS-->>CI: confirm push
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/export-supertonic.yaml (2)
145-163: Consider moving loop-invariant environment variables outside the loop.
GIT_LFS_SKIP_SMUDGEandGIT_CLONE_PROTECTION_ACTIVEare set on every iteration but their values don't change. Moving them before the loop improves clarity.Proposed refactor
git config --global user.email "csukuangfj@gmail.com" git config --global user.name "Fangjun Kuang" + + export GIT_LFS_SKIP_SMUDGE=1 + export GIT_CLONE_PROTECTION_ACTIVE=false + for m in *.tar.bz2; do - export GIT_LFS_SKIP_SMUDGE=1 - export GIT_CLONE_PROTECTION_ACTIVE=false - rm -rf ms🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/export-supertonic.yaml around lines 145 - 163, Move the loop-invariant environment exports out of the loop: set GIT_LFS_SKIP_SMUDGE and GIT_CLONE_PROTECTION_ACTIVE once before the for m in *.tar.bz2; do loop instead of exporting them inside the loop; update the script so the exports occur prior to the git clone/copy/push sequence (refer to the variables GIT_LFS_SKIP_SMUDGE and GIT_CLONE_PROTECTION_ACTIVE and the for m in *.tar.bz2; do loop) to improve clarity and avoid redundant commands.
133-134: Remove redundantif: truecondition.The static analysis tool correctly identified that
if: trueis always true and serves no purpose. This should be removed.Proposed fix
- name: Publish to modelscope - if: true env: MS_TOKEN: ${{ secrets.MODEL_SCOPE_GIT_TOKEN }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/export-supertonic.yaml around lines 133 - 134, Remove the redundant always-true conditional from the GitHub Actions step named "Publish to modelscope": delete the line "if: true" from that step block so the step relies on normal job-level or step-level conditions (or no condition) instead of an unnecessary tautology.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/supertonic/run.sh`:
- Around line 64-68: The current sequence clones into supertonic-2 and then runs
"mv supertonic-2 assets", which will create assets/supertonic-2 if an assets/
directory already exists; change the flow in run.sh so the clone targets the
intended assets/onnx path directly or explicitly place the cloned repo into
assets/onnx (e.g., clone into a temporary dir then move/rename to assets/onnx or
clone with the target directory argument), and ensure you create assets/ or
remove/rename any partial existing directories before moving to guarantee the
final structure contains assets/onnx as expected.
---
Nitpick comments:
In @.github/workflows/export-supertonic.yaml:
- Around line 145-163: Move the loop-invariant environment exports out of the
loop: set GIT_LFS_SKIP_SMUDGE and GIT_CLONE_PROTECTION_ACTIVE once before the
for m in *.tar.bz2; do loop instead of exporting them inside the loop; update
the script so the exports occur prior to the git clone/copy/push sequence (refer
to the variables GIT_LFS_SKIP_SMUDGE and GIT_CLONE_PROTECTION_ACTIVE and the for
m in *.tar.bz2; do loop) to improve clarity and avoid redundant commands.
- Around line 133-134: Remove the redundant always-true conditional from the
GitHub Actions step named "Publish to modelscope": delete the line "if: true"
from that step block so the step relies on normal job-level or step-level
conditions (or no condition) instead of an unnecessary tautology.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f60d8ca-5019-43f6-8c26-7fc36edf0a81
📒 Files selected for processing (2)
.github/workflows/export-supertonic.yamlscripts/supertonic/run.sh
| if [ ! -d assets/onnx/ ]; then | ||
| git clone https://huggingface.co/Supertone/supertonic-2 | ||
| mv supertonic-2 assets | ||
|
|
||
| ls -lh assets/onnx/ |
There was a problem hiding this comment.
Potential issue: mv behavior depends on pre-existing assets/ directory.
If assets/ exists but assets/onnx/ doesn't (e.g., from a partial/failed run), mv supertonic-2 assets will move the cloned directory into assets/ as assets/supertonic-2/, rather than renaming it to assets. This results in an incorrect structure.
Proposed fix to ensure consistent behavior
if [ ! -d assets/onnx/ ]; then
+ rm -rf assets supertonic-2
git clone https://huggingface.co/Supertone/supertonic-2
mv supertonic-2 assetsAlternatively, clone directly to the target:
if [ ! -d assets/onnx/ ]; then
- git clone https://huggingface.co/Supertone/supertonic-2
- mv supertonic-2 assets
+ rm -rf assets
+ git clone https://huggingface.co/Supertone/supertonic-2 assets
ls -lh assets/onnx/📝 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.
| if [ ! -d assets/onnx/ ]; then | |
| git clone https://huggingface.co/Supertone/supertonic-2 | |
| mv supertonic-2 assets | |
| ls -lh assets/onnx/ | |
| if [ ! -d assets/onnx/ ]; then | |
| rm -rf assets supertonic-2 | |
| git clone https://huggingface.co/Supertone/supertonic-2 | |
| mv supertonic-2 assets | |
| ls -lh assets/onnx/ |
| if [ ! -d assets/onnx/ ]; then | |
| git clone https://huggingface.co/Supertone/supertonic-2 | |
| mv supertonic-2 assets | |
| ls -lh assets/onnx/ | |
| if [ ! -d assets/onnx/ ]; then | |
| rm -rf assets | |
| git clone https://huggingface.co/Supertone/supertonic-2 assets | |
| ls -lh assets/onnx/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/supertonic/run.sh` around lines 64 - 68, The current sequence clones
into supertonic-2 and then runs "mv supertonic-2 assets", which will create
assets/supertonic-2 if an assets/ directory already exists; change the flow in
run.sh so the clone targets the intended assets/onnx path directly or explicitly
place the cloned repo into assets/onnx (e.g., clone into a temporary dir then
move/rename to assets/onnx or clone with the target directory argument), and
ensure you create assets/ or remove/rename any partial existing directories
before moving to guarantee the final structure contains assets/onnx as expected.
![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 updates the model download mechanism in run.sh to use git clone from Hugging Face. However, the current implementation has several critical issues that will cause the script to fail, including a less robust check for model existence, missing directory creation, and incorrect file paths. I've provided a suggestion to fix these issues.
| if [ ! -d assets/onnx/ ]; then | ||
| git clone https://huggingface.co/Supertone/supertonic-2 | ||
| mv supertonic-2 assets | ||
|
|
||
| ls -lh assets/onnx/ | ||
|
|
||
| echo "Download completed" |
There was a problem hiding this comment.
The new logic for downloading models has a few critical issues:
- Less robust check: The condition
if [ ! -d assets/onnx/ ]does not handle the case where the directory exists but is empty, which would cause the download to be skipped incorrectly. The original check was more robust. - Missing directory creation: The
assetsdirectory is not created before attempting to move files into it. If the directory doesn't exist, themvcommand will fail. - Incorrect destination path: The command
mv supertonic-2 assetsresults inassets/supertonic-2, but the script expects models to be inassets/onnx. This will cause subsequent commands to fail.
To ensure the script runs correctly, I recommend revising this block to address these points.
| if [ ! -d assets/onnx/ ]; then | |
| git clone https://huggingface.co/Supertone/supertonic-2 | |
| mv supertonic-2 assets | |
| ls -lh assets/onnx/ | |
| echo "Download completed" | |
| if [ ! -d assets/onnx/ ] || [ -z "$(ls -A assets/onnx/ 2>/dev/null)" ]; then | |
| echo "ONNX models not found, downloading..." | |
| git clone https://huggingface.co/Supertone/supertonic-2 | |
| mkdir -p assets | |
| mv supertonic-2 assets/onnx | |
| ls -lh assets/onnx/ | |
| echo "Download completed" |
1 participant
cc @Wasser1462 @rodrigomatta
Usage
You would get
And then build sherpa-onnx from master.
After that, run
English
You can see it is very fast.
The generated audio files are given below:
en-0.mov
en-1.mov
en-2.mov
en-3.mov
en-4.mov
en-5.mov
en-6.mov
en-7.mov
en-8.mov
en-9.mov
Summary by CodeRabbit
New Features
Chores