Repository navigation
Export KittenTTS models - #3604
Conversation
dosubot
Bot
added
the
size:M
📝 WalkthroughWalkthroughThis PR adds KittenTTS model support to the Node.js WASM runtime by introducing a new model type case for configuration, creating a build synchronization script to convert ES modules to CommonJS, and updating the CI/CD workflow to trigger on a different branch and publish to a new Hugging Face repository path. ChangesKittenTTS WASM Integration and Export
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 KittenTTS in the sherpa-onnx-tts.node.js file and introduces a Python script, sync-tts-node.py, to automate the conversion of ES modules to CommonJS. The review feedback suggests improving the Python script by explicitly specifying UTF-8 encoding for file operations and refactoring the regex replacement logic to enhance readability and ensure PEP 8 compliance.
| src = os.path.join(dir_path, "sherpa-onnx-tts.js") | ||
| dst = os.path.join(dir_path, "sherpa-onnx-tts.node.js") | ||
|
|
||
| with open(src) as f: |
There was a problem hiding this comment.
It is recommended to specify the encoding (e.g., encoding="utf-8") when opening files to ensure consistent behavior across different platforms and locales.
| with open(src) as f: | |
| with open(src, encoding="utf-8") as f: |
| content, | ||
| ) | ||
|
|
||
| with open(dst, "w") as f: |
There was a problem hiding this comment.
It is recommended to specify the encoding when opening the destination file for writing to maintain consistency.
| with open(dst, "w") as f: | |
| with open(dst, "w", encoding="utf-8") as f: |
| content = re.sub( | ||
| r"export \{[^}]*\};", | ||
| """if (typeof process == 'object' && typeof process.versions == 'object' &&\\n typeof process.versions.node == 'string') {\\n module.exports = {\\n createOfflineTts,\\n getDefaultOfflineTtsModelType,\\n };\\n}""", | ||
| content, | ||
| ) |
There was a problem hiding this comment.
The re.sub call uses an excessively long line with escaped newlines, which violates PEP 8 line length guidelines and reduces readability. Refactoring the replacement string into a separate variable using concatenated strings or a multi-line string is more idiomatic. Additionally, the regex can be made more robust by allowing for variable whitespace between the export keyword and the opening brace.
replacement = (\n "if (typeof process == 'object' && typeof process.versions == 'object' &&\n"\n " typeof process.versions.node == 'string') {\n"\n " module.exports = {\n"\n " createOfflineTts,\n"\n " getDefaultOfflineTtsModelType,\n"\n " };\n"\n "}"\n)\ncontent = re.sub(r"export\\s+\\{[^}]*\\};", replacement, content)![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wasm/tts/sync-tts-node.py (1)
17-22: 💤 Low valueConsider validating the regex substitution.
The script performs a regex substitution but doesn't verify that it succeeded. If the export pattern changes in the source file, the script will silently produce incorrect output.
♻️ Optional: Add validation for the substitution
# Replace ES module export with CommonJS module.exports -content = re.sub( +new_content = re.sub( r"export \{[^}]*\};", """if (typeof process == 'object' && typeof process.versions == 'object' &&\\n typeof process.versions.node == 'string') {\\n module.exports = {\\n createOfflineTts,\\n getDefaultOfflineTtsModelType,\\n };\\n}""", content, ) + +if new_content == content: + print("Warning: No export statement was replaced") + +content = new_content🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wasm/tts/sync-tts-node.py` around lines 17 - 22, The regex substitution replacing the ES export with a CommonJS module (using re.sub on variable content) isn't validated and may silently fail; update the code to use re.subn (or compare content before/after) when substituting the export pattern and check the returned substitution count, and if it is zero raise/log an explicit error (or exit) so you surface failures related to the export pattern not being found; reference the existing regex, the re.sub/re.subn call, and the exported symbols createOfflineTts and getDefaultOfflineTtsModelType when implementing the validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@wasm/tts/sync-tts-node.py`:
- Around line 17-22: The regex substitution replacing the ES export with a
CommonJS module (using re.sub on variable content) isn't validated and may
silently fail; update the code to use re.subn (or compare content before/after)
when substituting the export pattern and check the returned substitution count,
and if it is zero raise/log an explicit error (or exit) so you surface failures
related to the export pattern not being found; reference the existing regex, the
re.sub/re.subn call, and the exported symbols createOfflineTts and
getDefaultOfflineTtsModelType when implementing the validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4803c2d4-b338-4030-9baa-4576d9b11b62
📒 Files selected for processing (3)
.github/workflows/export-kitten.yamlwasm/tts/sherpa-onnx-tts.node.jswasm/tts/sync-tts-node.py
1 participant
See also #3591
cc @dewana-sl
You can find the exported models at https://github.com/k2-fsa/sherpa-onnx/releases/tag/tts-models
Usage
Logs are:
Generated audio:
kitten-micro-0.mov
Audio of different speakers (0-7)
sid 0
kitten-micro-0.mov
sid 1
kitten-micro-1.mov
sid 2
kitten-micro-2.mov
sid 3
kitten-micro-3.mov
sid 4
kitten-micro-4.mov
sid 5
kitten-micro-5.mov
sid 6
kitten-micro-6.mov
sid 7
kitten-micro-7.mov
Summary by CodeRabbit