adding --use-adapter and --adapter-dim to zipformer_adapter decode.py#2079
adding --use-adapter and --adapter-dim to zipformer_adapter decode.py#2079Elquintas wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe zipformer_adapter decode script now accepts two new CLI arguments— Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 updates the decode.py script for the Zipformer adapter by modifying docstring examples and adding --use-adapters and --adapter-dim arguments to the parser. The review feedback identifies a naming inconsistency between the examples and the parser, suggests using str2bool for boolean arguments to handle string inputs correctly, and recommends updating the default adapter dimension for consistency with training scripts.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50be6cb778
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
egs/librispeech/ASR/zipformer_adapter/decode.py (1)
23-27:⚠️ Potential issue | 🔴 Critical
--use-adapteris completely unusable as documented.Documentation (lines 23, 26, 35, 46, 57, 70, 85, 100, 113) shows
--use-adapter Trueand--use-adapter False, but line 396 defines--use-adapters(plural) withtype=int. Parsing fails with:error: argument --use-adapters: invalid int value: 'True'.Fix by making the flag name singular and the type boolean-compatible:
Suggested fix
parser.add_argument( - "--use-adapters", - type=int, - default=0, - help="Whether to use adapters", + "--use-adapter", + type=str2bool, + default=False, + help="Whether to use adapters", )Also applies to: 35–36, 395–407
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@egs/librispeech/ASR/zipformer_adapter/decode.py` around lines 23 - 27, The flag is defined as --use-adapters (plural) with type=int but docs and examples use --use-adapter True/False; update the argument registration (the parser.add_argument call that defines "--use-adapters") to use the singular name "--use-adapter" and make it boolean-compatible (use action="store_true" / action="store_false" or type=bool handling) so CLI accepts True/False as documented and the help text matches lines referencing --use-adapter; ensure any downstream references to the variable (e.g., use_adapters or args.use_adapters) are renamed/aliased to args.use_adapter to stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@egs/librispeech/ASR/zipformer_adapter/decode.py`:
- Around line 402-407: The CLI argument for adapter bottleneck uses default=0
which conflicts with Zipformer2's adapter_dim default of 16; change the
parser.add_argument for "--adapter-dim" to default=16 to align with the model,
and add a parse-time guard after argument parsing that checks if adapters are
enabled (e.g., the flag controlling adapter usage in this script) and ensures
adapter_dim > 0—if not, either set it to 16 or raise a clear error message so
adapters are never instantiated with a zero bottleneck.
---
Outside diff comments:
In `@egs/librispeech/ASR/zipformer_adapter/decode.py`:
- Around line 23-27: The flag is defined as --use-adapters (plural) with
type=int but docs and examples use --use-adapter True/False; update the argument
registration (the parser.add_argument call that defines "--use-adapters") to use
the singular name "--use-adapter" and make it boolean-compatible (use
action="store_true" / action="store_false" or type=bool handling) so CLI accepts
True/False as documented and the help text matches lines referencing
--use-adapter; ensure any downstream references to the variable (e.g.,
use_adapters or args.use_adapters) are renamed/aliased to args.use_adapter to
stay consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35038d16-bc7f-4e0c-a3f3-ea46ee9018ec
📒 Files selected for processing (1)
egs/librispeech/ASR/zipformer_adapter/decode.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
egs/librispeech/ASR/zipformer_adapter/decode.py (1)
395-407:⚠️ Potential issue | 🟠 MajorGuard adapter configuration and avoid zero adapter dimension by default.
Line 405 still sets
--adapter-dimto0. Combined with--use-adapters 1, this permits an invalid adapter setup unless users always override it manually. This was already raised in prior review and remains unresolved.Suggested fix
parser.add_argument( "--adapter-dim", type=int, - default=0, + default=16, help="Adapter dimension", )def main(): parser = get_parser() LibriSpeechAsrDataModule.add_arguments(parser) LmScorer.add_arguments(parser) args = parser.parse_args() + if args.use_adapters and args.adapter_dim <= 0: + parser.error("--adapter-dim must be > 0 when --use-adapters=1")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@egs/librispeech/ASR/zipformer_adapter/decode.py` around lines 395 - 407, The CLI currently allows enabling adapters via the --use-adapters flag while --adapter-dim defaults to 0, which permits an invalid configuration; update the parser/validation so that parser.add_argument("--adapter-dim", ...) uses a sensible non-zero default (e.g., 32/64) or add a runtime check after parsing that raises an error if args.use_adapters is true and args.adapter_dim <= 0; reference the parser.add_argument calls for "--use-adapters" and "--adapter-dim" and enforce the validation immediately after args = parser.parse_args() to prevent starting with a zero adapter dimension.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@egs/librispeech/ASR/zipformer_adapter/decode.py`:
- Around line 395-407: The CLI currently allows enabling adapters via the
--use-adapters flag while --adapter-dim defaults to 0, which permits an invalid
configuration; update the parser/validation so that
parser.add_argument("--adapter-dim", ...) uses a sensible non-zero default
(e.g., 32/64) or add a runtime check after parsing that raises an error if
args.use_adapters is true and args.adapter_dim <= 0; reference the
parser.add_argument calls for "--use-adapters" and "--adapter-dim" and enforce
the validation immediately after args = parser.parse_args() to prevent starting
with a zero adapter dimension.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1a99599-9cdd-4a77-b306-c7e9a58660cc
📒 Files selected for processing (1)
egs/librispeech/ASR/zipformer_adapter/decode.py
1 participant
egs/librispeech/ASR/zipformer_adapter/decode.py expects
--use-adapterand--adapter-dimarguments. I've added both arguments in the parser and updated the docstring.Summary by CodeRabbit
New Features
Documentation