Run train_parallel commands without shell=True in nnUNetV2Runner

[vishnukannaujia]

Fixes #8992.

Summary

nnUNetV2Runner.train_parallel joined each device's argument-list commands with "; " and launched the resulting string with subprocess.Popen(cmd_str, shell=True, ...). The commands are constructed internally, so this is not a confirmed injection vulnerability, but invoking a shell is unnecessary here and enlarges the attack surface if command construction ever changes.

This PR runs the commands with shell=False, preserving the original execution semantics:

• one worker thread per active device;
• commands within a device run sequentially, in their original order;
• different devices run concurrently;
• each stage still completes (all threads joined) before the next stage starts;
• empty stages/devices are skipped.

The per-device execution is extracted into a small static helper _run_device_commands, which runs each (cmd, env) pair via subprocess.run(cmd, env=env, stdout=subprocess.DEVNULL, check=False).

Behavioral notes

• No shell is spawned; each command is passed as an argument list.
• Per-command environment: each command now runs with its own env. Previously the joined shell string used only the first command's env for the whole device; since all commands for a device target the same GPU their envs are identical, so this is equivalent but more correct.
• Failure handling: check=False means a non-zero exit does not stop the remaining commands for that device, matching the previous "; "-join behavior (; continues regardless of exit status).

Tests

Added unit tests in tests/apps/nnunet/test_nnunetv2_runner_command.py (no nnunetv2 dependency required; they mock subprocess.run):

• test_runs_in_order_without_shell: commands run in order, each as an argument list (not a joined string), with shell unset/falsy and each command's own env.
• test_continues_after_nonzero_exit: check=False is used and all commands are attempted.

Full nnUNet integration training is not exercised here as it requires the nnunetv2 package and GPU data; happy to add an integration-level check if maintainers prefer a specific location.

[coderabbitai]

📝 Walkthrough

Walkthrough

train_parallel now runs one thread per GPU for each training stage, preserving sequential command order within each device while allowing devices to execute concurrently. The new _run_device_commands helper uses subprocess.run with argument lists, per-command environments, shell=False, suppressed output, and check=False. Tests cover ordering, environment propagation, shell disabling, and continuation after failures.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main change: removing shell=True from train_parallel in nnUNetV2Runner.
Description check	✅ Passed	The description covers the change, rationale, behavior, and tests, though the template checklist section is not filled out.
Linked Issues check	✅ Passed	The implementation matches #8992 by avoiding shell=True while preserving per-device order, concurrency, environments, and empty-stage handling.
Out of Scope Changes check	✅ Passed	The changes stay within the issue scope and only add the helper plus focused unit tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

tests/apps/nnunet/test_nnunetv2_runner_command.py (1)

77-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Add coverage for the threaded stage orchestration.

These tests call only _run_device_commands; they cannot detect serial GPU workers, an omitted stage barrier, or workers created for empty queues. Add a train_parallel test covering those contracts.

As per path instructions, ensure new or modified definitions are covered by existing or new unit tests.

🤖 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 `@tests/apps/nnunet/test_nnunetv2_runner_command.py` around lines 77 - 109, Add
a unit test in TestRunDeviceCommands that exercises
nnUNetV2Runner.train_parallel with multiple stages and device queues, asserting
workers run concurrently, each stage waits for all prior workers before
starting, and no workers are created for empty queues. Keep the existing
_run_device_commands tests unchanged and ensure the new orchestration behavior
is covered through the public train_parallel flow.

Source: Path instructions

monai/apps/nnunet/nnunetv2_runner.py (1)

763-774: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Complete the required Google-style docstrings.

• monai/apps/nnunet/nnunetv2_runner.py#L763-L774: document the helper’s return value and currently propagated launch exceptions.
• tests/apps/nnunet/test_nnunetv2_runner_command.py#L80-L109: add docstrings to both new test methods.

As per path instructions, every definition must document variables, return values, and raised exceptions in Google style.

🤖 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 `@monai/apps/nnunet/nnunetv2_runner.py` around lines 763 - 774, Complete the
Google-style docstrings for _run_device_commands in
monai/apps/nnunet/nnunetv2_runner.py, documenting its device_cmds argument, None
return value, and launch exceptions propagated by the helper. Add Google-style
docstrings to both new test methods in
tests/apps/nnunet/test_nnunetv2_runner_command.py, documenting their parameters,
return values, and raised exceptions as applicable.

Source: Path instructions

🤖 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.

Inline comments:
In `@monai/apps/nnunet/nnunetv2_runner.py`:
- Around line 755-760: Update the command-processing logic in
_run_device_commands to catch OSError from each subprocess.run invocation, log
the failure, and continue processing the remaining commands for that device.
Preserve the existing thread creation and join behavior so stage sequencing
remains unchanged.

---

Nitpick comments:
In `@monai/apps/nnunet/nnunetv2_runner.py`:
- Around line 763-774: Complete the Google-style docstrings for
_run_device_commands in monai/apps/nnunet/nnunetv2_runner.py, documenting its
device_cmds argument, None return value, and launch exceptions propagated by the
helper. Add Google-style docstrings to both new test methods in
tests/apps/nnunet/test_nnunetv2_runner_command.py, documenting their parameters,
return values, and raised exceptions as applicable.

In `@tests/apps/nnunet/test_nnunetv2_runner_command.py`:
- Around line 77-109: Add a unit test in TestRunDeviceCommands that exercises
nnUNetV2Runner.train_parallel with multiple stages and device queues, asserting
workers run concurrently, each stage waits for all prior workers before
starting, and no workers are created for empty queues. Keep the existing
_run_device_commands tests unchanged and ensure the new orchestration behavior
is covered through the public train_parallel flow.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c2080dfb-79ba-4971-8465-74ee8b40de64

📥 Commits

Reviewing files that changed from the base of the PR and between 5da2472 and 3cf9cdf.

📒 Files selected for processing (2)

• monai/apps/nnunet/nnunetv2_runner.py
• tests/apps/nnunet/test_nnunetv2_runner_command.py

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Continue the device queue after process-launch errors.

If subprocess.run raises FileNotFoundError or another OSError, the worker silently terminates, skips that device’s remaining commands, and join() still lets the next stage proceed. Catch and log OSError around each command to preserve continue-after-failure behavior.

Proposed fix

         for cmd, env in device_cmds:
-            subprocess.run(cmd, env=env, stdout=subprocess.DEVNULL, check=False)
+            try:
+                subprocess.run(cmd, env=env, stdout=subprocess.DEVNULL, check=False)
+            except OSError as error:
+                logger.error(f"Failed to start command {shlex.join(cmd)}: {error}")

Also applies to: 775-776

🤖 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 `@monai/apps/nnunet/nnunetv2_runner.py` around lines 755 - 760, Update the
command-processing logic in _run_device_commands to catch OSError from each
subprocess.run invocation, log the failure, and continue processing the
remaining commands for that device. Preserve the existing thread creation and
join behavior so stage sequencing remains unchanged.