cp: fix: Use nargs for custom_bash_cmds (2261) into r0.3.0#2262
cp: fix: Use nargs for custom_bash_cmds (2261) into r0.3.0#2262
fix: Use nargs for custom_bash_cmds (2261) into r0.3.0#2262Conversation
Signed-off-by: oliver könig <okoenig@nvidia.com> Signed-off-by: NeMo Bot <nemo-bot@nvidia.com>
|
/ok to test 64060e6 |
📝 WalkthroughWalkthroughThe pull request modifies how custom bash commands are parsed from CLI arguments and propagated through the execution pipeline. The argument parser now uses Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 2
🤖 Fix all issues with AI agents
In `@scripts/performance/setup_experiment.py`:
- Line 209: The parameter type for custom_bash_cmds is incorrect: change its
annotation from List[List[str]] to Optional[List[List[str]]] (and add/import
Optional from typing) so the function signature reflects that
args.custom_bash_cmds can be None (argparse default) before it is forwarded to
slurm_executor; update the signature that contains the custom_bash_cmds
parameter in scripts/performance/setup_experiment.py and ensure any local usages
still handle None safely.
In `@scripts/performance/utils/executors.py`:
- Line 67: The parameter custom_bash_cmds currently uses an implicit Optional by
being annotated as List[List[str]] = None; update its type annotation to an
explicit union using the PEP 604 form (e.g., List[List[str]] | None or
list[list[str]] | None) so the signature explicitly shows None is allowed;
locate the parameter named custom_bash_cmds in the executor function signature
in scripts/performance/utils/executors.py and replace the implicit Optional
annotation with the explicit `| None` form while keeping the default = None.
| custom_env_vars: Dict[str, str], | ||
| custom_srun_args: List[str], | ||
| custom_bash_cmds: List[str], | ||
| custom_bash_cmds: List[List[str]], |
There was a problem hiding this comment.
Type hint should be nullable since default=None in argparse.
args.custom_bash_cmds can be None (the argparse default), and this None is passed straight through to slurm_executor which handles it on line 82 of executors.py. The signature here should reflect that.
Proposed fix
- custom_bash_cmds: List[List[str]],
+ custom_bash_cmds: list[list[str]] | None,📝 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.
| custom_bash_cmds: List[List[str]], | |
| custom_bash_cmds: list[list[str]] | None, |
🤖 Prompt for AI Agents
In `@scripts/performance/setup_experiment.py` at line 209, The parameter type for
custom_bash_cmds is incorrect: change its annotation from List[List[str]] to
Optional[List[List[str]]] (and add/import Optional from typing) so the function
signature reflects that args.custom_bash_cmds can be None (argparse default)
before it is forwarded to slurm_executor; update the signature that contains the
custom_bash_cmds parameter in scripts/performance/setup_experiment.py and ensure
any local usages still handle None safely.
| wandb_key: str = None, | ||
| network: str = None, | ||
| custom_bash_cmds: List[str] = None, | ||
| custom_bash_cmds: List[List[str]] = None, |
There was a problem hiding this comment.
Implicit Optional — use explicit | None annotation.
Ruff RUF013 flags this, and coding guidelines require T | None over implicit Optional.
Proposed fix
- custom_bash_cmds: List[List[str]] = None,
+ custom_bash_cmds: list[list[str]] | None = None,📝 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.
| custom_bash_cmds: List[List[str]] = None, | |
| custom_bash_cmds: list[list[str]] | None = None, |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 67-67: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
In `@scripts/performance/utils/executors.py` at line 67, The parameter
custom_bash_cmds currently uses an implicit Optional by being annotated as
List[List[str]] = None; update its type annotation to an explicit union using
the PEP 604 form (e.g., List[List[str]] | None or list[list[str]] | None) so the
signature explicitly shows None is allowed; locate the parameter named
custom_bash_cmds in the executor function signature in
scripts/performance/utils/executors.py and replace the implicit Optional
annotation with the explicit `| None` form while keeping the default = None.
1 participant
beep boop [🤖]: Hi @ko3n1g 👋,
Summary by CodeRabbit
--custom_bash_cmdsargument format in performance testing scripts to accept a flexible list-based structure, replacing the previous comma-separated string input method.