Export X-ASR models to sherpa-onnx - #3662
Conversation
dosubot
Bot
added
the
size:L
![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 export and test scripts for streaming and non-streaming Zipformer transducer models, and refactors C++ source files to use a shared RemoveSpaceBetweenCjk utility. Feedback from the reviewer highlights potential runtime crashes in the test scripts due to hardcoded input data types (np.int32 and np.int64), recommending dynamic detection of these types from the ONNX models. Additionally, the reviewer suggests a more robust implementation for the load_tokens function in both test scripts to prevent ValueError crashes when parsing empty lines or space tokens.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| model_type = encoder_meta["model_type"] | ||
| assert model_type == "zipformer2", model_type | ||
|
|
||
| self.context_size = self.decoder.get_inputs()[0].shape[1] |
There was a problem hiding this comment.
The input data types for the encoder length and decoder inputs are currently hardcoded to np.int32. However, depending on whether --use-int32-inputs is set to 1 or 0 during export, these inputs can be either int32 or int64. Hardcoding them to np.int32 will cause a runtime type mismatch crash in ONNX Runtime when testing models exported with int64 inputs (such as the no-punct model in this PR). We should dynamically detect the expected input types from the ONNX model.
| self.context_size = self.decoder.get_inputs()[0].shape[1] | |
| self.context_size = self.decoder.get_inputs()[0].shape[1] | |
| encoder_len_input = self.encoder.get_inputs()[1] | |
| self.encoder_len_dtype = np.int64 if "int64" in encoder_len_input.type else np.int32 | |
| decoder_input = self.decoder.get_inputs()[0] | |
| self.decoder_dtype = np.int64 if "int64" in decoder_input.type else np.int32 |
| """ | ||
| Args: x: (1, T, C] | ||
| """ | ||
| x_len = np.array([x.shape[1]], dtype=np.int32) |
There was a problem hiding this comment.
Use the dynamically detected encoder length data type instead of hardcoded np.int32 to prevent runtime crashes.
| x_len = np.array([x.shape[1]], dtype=np.int32) | |
| x_len = np.array([x.shape[1]], dtype=self.encoder_len_dtype) |
| return out[0] | ||
|
|
||
| def run_decoder(self, hyp): | ||
| hyp = np.array([hyp], dtype=np.int32) |
There was a problem hiding this comment.
Use the dynamically detected decoder input data type instead of hardcoded np.int32 to prevent runtime crashes.
| hyp = np.array([hyp], dtype=np.int32) | |
| hyp = np.array([hyp], dtype=self.decoder_dtype) |
|
|
||
| decoder_meta = self.decoder.get_modelmeta().custom_metadata_map | ||
| self.context_size = int(decoder_meta["context_size"]) | ||
| self.vocab_size = int(decoder_meta["vocab_size"]) |
There was a problem hiding this comment.
The input data type for the decoder is currently hardcoded to np.int64. To make the script robust to models exported with either int32 or int64 inputs, we should dynamically detect the expected input type from the ONNX model.
| self.vocab_size = int(decoder_meta["vocab_size"]) | |
| self.vocab_size = int(decoder_meta["vocab_size"]) | |
| decoder_input = self.decoder.get_inputs()[0] | |
| self.decoder_dtype = np.int64 if "int64" in decoder_input.type else np.int32 |
| return out[0], out[1:] | ||
|
|
||
| def run_decoder(self, hyp): | ||
| hyp = np.array([hyp], dtype=np.int64) |
There was a problem hiding this comment.
Use the dynamically detected decoder input data type instead of hardcoded np.int64 to prevent runtime crashes.
| hyp = np.array([hyp], dtype=np.int64) | |
| hyp = np.array([hyp], dtype=self.decoder_dtype) |
| def load_tokens(filename): | ||
| ans = dict() | ||
| with open(filename, encoding="utf-8") as f: | ||
| for line in f: | ||
| t, i = line.strip().split() | ||
| ans[int(i)] = t | ||
| pass | ||
|
|
||
| return ans |
There was a problem hiding this comment.
The current implementation of load_tokens can fail with a ValueError if a line is empty or if the token is a space character (which is common in BPE/character vocabularies, where a space token might be represented as <id>). Stripping and splitting such lines results in a single-element list, causing unpacking to fail. We should handle these cases robustly.
def load_tokens(filename):
ans = dict()
with open(filename, encoding="utf-8") as f:
for line in f:
line = line.rstrip("\r\n")
parts = line.split()
if len(parts) == 1:
t = " "
i = parts[0]
elif len(parts) == 2:
t, i = parts
else:
continue
ans[int(i)] = t
return ans| def load_tokens(filename): | ||
| ans = dict() | ||
| with open(filename, encoding="utf-8") as f: | ||
| for line in f: | ||
| t, i = line.strip().split() | ||
| ans[int(i)] = t | ||
| pass | ||
|
|
||
| return ans |
There was a problem hiding this comment.
The current implementation of load_tokens can fail with a ValueError if a line is empty or if the token is a space character. We should handle these cases robustly.
def load_tokens(filename):
ans = dict()
with open(filename, encoding="utf-8") as f:
for line in f:
line = line.rstrip("\r\n")
parts = line.split()
if len(parts) == 1:
t = " "
i = parts[0]
elif len(parts) == 2:
t, i = parts
else:
continue
ans[int(i)] = t
return ans
📝 WalkthroughWalkthroughThis PR adds a complete CI/CD pipeline to automatically export X-ASR Zipformer transducer models in both non-streaming and streaming modes, creates Python test scripts for ONNX inference validation, disables conflicting legacy upload steps, and refactors CJK text spacing normalization into a shared utility module. ChangesX-ASR Model Export & Test Infrastructure
CJK Text Post-Processing Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
.github/workflows/export-x-asr.yaml (4)
1-11: ⚖️ Poor tradeoffConsider workflow security hardening.
The workflow has several security posture gaps flagged by static analysis:
- No
permissions:block (defaults to broad GITHUB_TOKEN permissions)- Actions not pinned to commit SHAs (tags can be moved)
actions/checkoutdoes not setpersist-credentials: false(credentials remain accessible)While not actively exploitable, these reduce defense-in-depth. Consider:
- Adding
permissions: {}at workflow level and granting minimal permissions per job- Pinning actions to commit SHAs for supply-chain integrity
- Setting
persist-credentials: falseon checkout steps🤖 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 @.github/workflows/export-x-asr.yaml around lines 1 - 11, The workflow "export-x-asr" lacks explicit permissions and has unpinned actions and an insecure checkout; add a top-level permissions: {} to deny everything by default, then grant minimal required permissions per job (e.g., permissions: contents: read only where needed), replace action references (e.g., actions/checkout) with commit SHAs instead of tags to pin them, and set persist-credentials: false on the actions/checkout step to avoid leaking GITHUB_TOKEN to subsequent steps.
280-280: 💤 Low valueRemove redundant
if: truecondition.Same as the non-streaming release step.
🧹 Proposed fix
- name: Release - if: true uses: svenstaro/upload-release-action@v2🤖 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 @.github/workflows/export-x-asr.yaml at line 280, Remove the redundant "if: true" condition from the workflow step so it behaves like the non-streaming release step; locate the literal line "if: true" in the export-x-asr GitHub Actions YAML and delete that key from the step definition so the step runs by default without the unnecessary conditional.
63-134: ⚖️ Poor tradeoffHardcoded release dates in directory names.
The directory names include
2026-06-03(non-streaming) and2026-06-05(streaming), which will require manual updates for future releases. Consider using a variable like${{ env.RELEASE_DATE }}set at the workflow level for easier maintenance.🤖 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 @.github/workflows/export-x-asr.yaml around lines 63 - 134, Replace the hardcoded dates in the directory-name assignments (the lines that set d=... like in the "Collect model files (punct fp32)", "Collect model files (punct int8)", "Collect model files (no-punct fp32)", and "Collect model files (no-punct int8)" steps) with a workflow-level variable (e.g. use env.RELEASE_DATE) and construct d using that variable instead of literal "2026-06-03"/"2026-06-05"; also add RELEASE_DATE to the workflow env section so all steps reference the same date variable for future releases.
137-137: 💤 Low valueRemove redundant
if: truecondition.The step will run by default without an explicit
if: true. Per actionlint, constant conditions should be removed.🧹 Proposed fix
- name: Release - if: true uses: svenstaro/upload-release-action@v2🤖 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 @.github/workflows/export-x-asr.yaml at line 137, Remove the redundant constant condition "if: true" from the workflow step (the literal "if: true" entry) so the step runs by default; locate the step block containing that "if: true" and delete that line entirely to satisfy actionlint and simplify the workflow.scripts/zipformer-transducer/x-asr/export-non-streaming.sh (1)
43-84: ⚡ Quick winQuote variable expansions to prevent word splitting.
Shellcheck flags unquoted
$dirvariables (lines 52-53, 63, 74-75, 85). While unlikely to cause issues in the controlled CI environment, quoting prevents potential word-splitting if the path ever contains spaces.🛡️ Proposed fix
--exp-dir $dir/punct \ --tokens $dir/punct/tokens.txt \ + --exp-dir "$dir/punct" \ + --tokens "$dir/punct/tokens.txt" \Apply the same fix to all instances of
$dirin lines 52-53, 63, 74-75, and 85.🤖 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 `@scripts/zipformer-transducer/x-asr/export-non-streaming.sh` around lines 43 - 84, The $dir variable expansions in the shell invocations and the ls command (e.g., the two calls to ./zipformer/export-streaming-as-non-streaming-onnx.py and ls -lh $dir/punct) are unquoted and can suffer word-splitting; update every occurrence of $dir (including $dir/punct, $dir/no-punct, and any uses in --exp-dir and --tokens flags and the ls command) to be quoted (e.g., "$dir", "$dir/punct", "$dir/no-punct") so paths with spaces are handled safely while leaving the rest of the arguments unchanged.scripts/zipformer-transducer/x-asr/export-streaming.sh (1)
48-112: ⚡ Quick winQuote variable expansions to prevent word splitting.
Shellcheck flags unquoted
$dirvariables throughout the export commands and path references. While unlikely to cause issues in CI, quoting prevents potential word-splitting if paths contain spaces.🛡️ Proposed fix
- --exp-dir $dir/punct \ - --tokens $dir/punct/tokens.txt \ + --exp-dir "$dir/punct" \ + --tokens "$dir/punct/tokens.txt" \Apply the same fix to all instances of
$dirin lines 55-56, 66, 68, 78, 89-90, 100, 102, and 112.🤖 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 `@scripts/zipformer-transducer/x-asr/export-streaming.sh` around lines 48 - 112, The script uses unquoted variable expansions of $dir (e.g. the --exp-dir and --tokens args in the export-onnx-streaming.py calls, and commands like ls -lh $dir/punct, pushd $dir/punct, mv ... $dir/punct, rm ... $dir/no-punct, popd) which can cause word-splitting; update every occurrence to use quoted expansions (\"$dir\", \"$dir/punct\", \"$dir/no-punct\", and \"$dir/punct/tokens.txt\") so paths with spaces are handled safely, ensuring you quote the --exp-dir and --tokens arguments and all ls/pushd/mv/rm uses referencing $dir.
🤖 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 @.github/workflows/export-x-asr.yaml:
- Around line 1-11: The workflow "export-x-asr" lacks explicit permissions and
has unpinned actions and an insecure checkout; add a top-level permissions: {}
to deny everything by default, then grant minimal required permissions per job
(e.g., permissions: contents: read only where needed), replace action references
(e.g., actions/checkout) with commit SHAs instead of tags to pin them, and set
persist-credentials: false on the actions/checkout step to avoid leaking
GITHUB_TOKEN to subsequent steps.
- Line 280: Remove the redundant "if: true" condition from the workflow step so
it behaves like the non-streaming release step; locate the literal line "if:
true" in the export-x-asr GitHub Actions YAML and delete that key from the step
definition so the step runs by default without the unnecessary conditional.
- Around line 63-134: Replace the hardcoded dates in the directory-name
assignments (the lines that set d=... like in the "Collect model files (punct
fp32)", "Collect model files (punct int8)", "Collect model files (no-punct
fp32)", and "Collect model files (no-punct int8)" steps) with a workflow-level
variable (e.g. use env.RELEASE_DATE) and construct d using that variable instead
of literal "2026-06-03"/"2026-06-05"; also add RELEASE_DATE to the workflow env
section so all steps reference the same date variable for future releases.
- Line 137: Remove the redundant constant condition "if: true" from the workflow
step (the literal "if: true" entry) so the step runs by default; locate the step
block containing that "if: true" and delete that line entirely to satisfy
actionlint and simplify the workflow.
In `@scripts/zipformer-transducer/x-asr/export-non-streaming.sh`:
- Around line 43-84: The $dir variable expansions in the shell invocations and
the ls command (e.g., the two calls to
./zipformer/export-streaming-as-non-streaming-onnx.py and ls -lh $dir/punct) are
unquoted and can suffer word-splitting; update every occurrence of $dir
(including $dir/punct, $dir/no-punct, and any uses in --exp-dir and --tokens
flags and the ls command) to be quoted (e.g., "$dir", "$dir/punct",
"$dir/no-punct") so paths with spaces are handled safely while leaving the rest
of the arguments unchanged.
In `@scripts/zipformer-transducer/x-asr/export-streaming.sh`:
- Around line 48-112: The script uses unquoted variable expansions of $dir (e.g.
the --exp-dir and --tokens args in the export-onnx-streaming.py calls, and
commands like ls -lh $dir/punct, pushd $dir/punct, mv ... $dir/punct, rm ...
$dir/no-punct, popd) which can cause word-splitting; update every occurrence to
use quoted expansions (\"$dir\", \"$dir/punct\", \"$dir/no-punct\", and
\"$dir/punct/tokens.txt\") so paths with spaces are handled safely, ensuring you
quote the --exp-dir and --tokens arguments and all ls/pushd/mv/rm uses
referencing $dir.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d20d041e-a459-4704-8742-e42cb53e6373
📒 Files selected for processing (8)
.github/workflows/export-x-asr.yaml.github/workflows/upload-models.yamlscripts/zipformer-transducer/x-asr/export-non-streaming.shscripts/zipformer-transducer/x-asr/export-streaming.shscripts/zipformer-transducer/x-asr/test_onnx_non_streaming.pyscripts/zipformer-transducer/x-asr/test_onnx_streaming.pysherpa-onnx/csrc/online-recognizer-transducer-impl.hsherpa-onnx/csrc/qnn/offline-recognizer-transducer-qnn-impl.h
💤 Files with no reviewable changes (1)
- sherpa-onnx/csrc/qnn/offline-recognizer-transducer-qnn-impl.h
1 participant
See also https://github.com/Gilgamesh-J/X-ASR
https://github.com/k2-fsa/sherpa-onnx/releases/tag/asr-models
Usage
Non-streaming
Streaming
Summary by CodeRabbit
New Features
Improvements