Skip to content

cp: fix: Use nargs for custom_bash_cmds (2261) into r0.3.0 #2262

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ko3n1g merged 1 commit into from
Feb 6, 2026
Merged

cp: fix: Use nargs for custom_bash_cmds (2261) into r0.3.0 #2262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions scripts/performance/argument_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,9 +422,10 @@ def parse_cli_args():
slurm_args.add_argument(
"-cb",
"--custom_bash_cmds",
type=list_of_strings,
help="Comma separated string of bash commands",
default=[],
nargs="*",
action="append",
help="List of bash commands to execute before the main command",
default=None,
)
slurm_args.add_argument(
"--gres",
Expand Down
2 changes: 1 addition & 1 deletion scripts/performance/setup_experiment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ def main(
custom_mounts: List[str],
custom_env_vars: Dict[str, str],
custom_srun_args: List[str],
custom_bash_cmds: List[str],
custom_bash_cmds: List[List[str]],
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

nccl_ub: bool,
pretrained_checkpoint: Optional[str],
num_gpus: int,
Expand Down
4 changes: 2 additions & 2 deletions scripts/performance/utils/executors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def slurm_executor(
nemo_home: str = DEFAULT_NEMO_HOME,
wandb_key: str = None,
network: str = None,
custom_bash_cmds: List[str] = None,
custom_bash_cmds: List[List[str]] = None,
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

additional_slurm_params: Dict[str, Any] = None,
gres: Optional[str] = None,
) -> run.SlurmExecutor:
Expand All @@ -79,7 +79,7 @@ def slurm_executor(
#SBATCH --nodelist=node001,node002
#SBATCH --constraint=gpu
"""
custom_bash_cmds = [] if custom_bash_cmds is None else custom_bash_cmds
custom_bash_cmds = [] if custom_bash_cmds is None else [" ".join(cmd) for cmd in custom_bash_cmds]
mounts = []
# Explicitly request GPU resources to ensure proper allocation
# Without --gres=gpu:N, some clusters only allocate 1 GPU regardless of ntasks_per_node
Expand Down
Loading