MBridge: fix vp parameter handling

[podkidyshev]

Summary

• vp=1 wasn't handled correctly

Test Plan

• Automated CI
• Manual run on EOS

Additional Notes

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR modifies the -vp flag argument generation in the Megatron Bridge SLURM command strategy, changing the assigned value from Python None to the string "None" when vp equals 1 or [1]. A corresponding test case verifies this behavior is correctly emitted in the generated wrapper script.

Changes

Cohort / File(s)	Summary
Implementation src/cloudai/workloads/megatron_bridge/slurm_command_gen_strategy.py	Modified _build_launcher_parts to set vp_for_launcher to string "None" instead of Python None when vp == 1 or [1], altering the emitted -vp flag argument.
Test Coverage tests/workloads/megatron_bridge/test_command_gen_strategy_slurm.py	Added test_vp test case that verifies the generated SLURM wrapper script contains the -vp None argument when vp is set to 1.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 The -vp flag now speaks with grace,
"None" as a string takes its place!
From ghostly Python to words so clear,
Tests verify what we hold dear! ✨

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: fixing the vp parameter handling in the MBridge code, which aligns with the changeset modifications.
Description check	✅ Passed	The description is related to the changeset, explaining that vp=1 handling was incorrect and needs fixing, which corresponds to the code changes made.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch ipod/megatron-vp

---

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

[coderabbitai]

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)

src/cloudai/workloads/megatron_bridge/slurm_command_gen_strategy.py (1)

391-397: ⚠️ Potential issue | 🟡 Minor

Update the stale vp behavior comment to match actual emission.

Line 391 says -vp is not emitted, but Lines 396–397 now intentionally emit -vp None by passing the string "None". Please align the comment with the current behavior to avoid future regressions.

Suggested fix

-        # When vp is 1 (or [1]), pass None so -vp is not emitted in sbatch
+        # When vp is 1 (or [1]), pass the literal string "None" to the launcher
+        # so the generated command includes `-vp None`.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cloudai/workloads/megatron_bridge/slurm_command_gen_strategy.py` around
lines 391 - 397, The comment above the vp handling is stale: it says "-vp is not
emitted" but the code sets vp_for_launcher to the string "None" so
add_field("vp", "-vp", vp_for_launcher) will emit "-vp None"; update the comment
to reflect that when args.vp equals 1 (or [1]) we intentionally pass the string
"None" so the sbatch argument "-vp None" is emitted, referencing the
vp_for_launcher variable and add_field call to locate the logic and adjust the
comment accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/workloads/megatron_bridge/test_command_gen_strategy_slurm.py`:
- Around line 457-465: Update the test_vp to cover both normalized vp inputs by
parameterizing the test over the scalar 1 and the single-item list [1] (and/or
tuple (1,)) returned via make_test_run(cmd_args_overrides={"vp": ...}); for each
input create MegatronBridgeSlurmCommandGenStrategy(configured_slurm_system, tr),
call self._wrapper_content(cmd_gen), and assert that "-vp None" is present in
wrapper_content so both the scalar and list/tuple normalization branches in
MegatronBridgeSlurmCommandGenStrategy are exercised.

---

Outside diff comments:
In `@src/cloudai/workloads/megatron_bridge/slurm_command_gen_strategy.py`:
- Around line 391-397: The comment above the vp handling is stale: it says "-vp
is not emitted" but the code sets vp_for_launcher to the string "None" so
add_field("vp", "-vp", vp_for_launcher) will emit "-vp None"; update the comment
to reflect that when args.vp equals 1 (or [1]) we intentionally pass the string
"None" so the sbatch argument "-vp None" is emitted, referencing the
vp_for_launcher variable and add_field call to locate the logic and adjust the
comment accordingly.

🪄 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: ASSERTIVE

Plan: Pro

Run ID: 18f96f2f-9921-4b18-935b-d10449d398b2

📥 Commits

Reviewing files that changed from the base of the PR and between 4ea5899 and 9150b85.

📒 Files selected for processing (2)

• src/cloudai/workloads/megatron_bridge/slurm_command_gen_strategy.py
• tests/workloads/megatron_bridge/test_command_gen_strategy_slurm.py