Allow profiling ranks in string format with comma as separator#855
Allow profiling ranks in string format with comma as separator#855podkidyshev merged 2 commits intoNVIDIA:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdated the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cloudai/workloads/megatron_bridge/megatron_bridge.py`:
- Line 116: The type annotation for the profiling_ranks Field should use modern
PEP 604 syntax: replace Optional[Union[int, str, List[int]]] with int | str |
list[int] | None for clarity and consistency; remove any now-unused typing
imports (Optional, Union, List) or update imports accordingly and ensure the
annotation is applied on the profiling_ranks Field definition in
megatron_bridge.py.
- Around line 116-119: Add a Pydantic field validator for profiling_ranks to
validate and normalize comma-separated string input: when profiling_ranks is a
str, ensure it matches only digits separated by commas (e.g. "0,4,8"), split and
convert to a list[int] (or raise a ValidationError on invalid format); keep
existing support for int and list[int]. Attach the validator to the
profiling_ranks field (use the model's validator/@field_validator for
"profiling_ranks") so downstream command generation always receives a validated
list[int] or a single int.
🪄 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: 21a82a8b-87a4-4cbf-bc66-1a1d6a2de461
📒 Files selected for processing (1)
src/cloudai/workloads/megatron_bridge/megatron_bridge.py
|
@juntaowww let me know when to merge |
|
good to merge now, thanks! |
Summary
Allow profiling ranks in string format with comma as separator. The list format in
cmd_argswould make it DSE target.Test Plan
manual run