fix(profiler): make parallel label injective across (tp, pp, dp, moe_tp, moe_ep) - #10086
Conversation
…tp, moe_ep)
'PickedParallelConfig.label()' in 'parallelization.py' (and its mirror 'make_parallel_label()' in 'aic_dataframe.py') used a 3-way bucket — 'dep{moe_ep}' / 'tep{moe_tp}' / 'tp{tp}' — that collapsed distinct '(tp, pp, dp, moe_tp, moe_ep)' 5-tuples to the same string. Two unrelated topologies could share a label, which silently corrupted 'thorough.py' sweep 'work_dir' naming and 'aiconfigurator.sdk.picking' 'groupby(parallel)' dedup downstream of the picker.
Signed-off-by: jooe0824 <jooe0824@sk.com>
|
👋 Hi jooe0824! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR fixes label collisions in parallel configuration encoding by rewriting ChangesInjective Parallel Label Encoding
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@components/src/dynamo/planner/tests/unit/test_parallelization_label.py`:
- Around line 14-25: Add a module-level pytestmark list in
test_parallelization_label.py by defining the variable pytestmark and assigning
it a list containing the markers pytest.mark.gpu_0, pytest.mark.pre_merge (with
a TODO comment about reverting to post_merge), pytest.mark.unit, and
pytest.mark.planner so the test module is properly tagged for CI scheduling and
component categorization.
In `@components/src/dynamo/profiler/tests/unit/test_aic_dataframe.py`:
- Around line 12-21: Add a module-level pytestmark list to
components/src/dynamo/profiler/tests/unit/test_aic_dataframe.py to ensure the
module is picked up by pre-merge CI: create a Python list named pytestmark at
top-level that includes pytest.mark.pre_merge (and any existing project markers
you follow in sibling tests), and attach a TODO comment indicating this
promotion is temporary and should be reverted; you can place this near the
existing imports that reference make_parallel_label and _NUM_GPUS so the whole
module is marked without adding per-test decorators.
🪄 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: 2c4a7e3d-9c82-432e-9436-d86b0daffc56
📒 Files selected for processing (4)
components/src/dynamo/planner/config/parallelization.pycomponents/src/dynamo/planner/tests/unit/test_parallelization_label.pycomponents/src/dynamo/profiler/tests/unit/test_aic_dataframe.pycomponents/src/dynamo/profiler/utils/aic_dataframe.py
Signed-off-by: jooe0824 <jooe0824@sk.com>
✅ Actions performedReview triggered.
|
|
Actionable comments posted: 0 |
|
/ok to test 69e3775 |
Overview:
'PickedParallelConfig.label()' in 'parallelization.py' (and its mirror 'make_parallel_label()' in 'aic_dataframe.py') used a 3-way bucket — 'dep{moe_ep}' / 'tep{moe_tp}' / 'tp{tp}' — that collapsed distinct '(tp, pp, dp, moe_tp, moe_ep)' 5-tuples to the same string.
Two unrelated topologies could share a label, which silently corrupted 'thorough.py' sweep 'work_dir' naming and 'aiconfigurator.sdk.picking' 'groupby(parallel)' dedup downstream of the picker.
Details:
The legacy encoding
Each branch drops every dimension it doesn't name, producing three independent collision classes — not all MoE-related:
dep{moe_ep}(MoE-EP)(tp=2, moe_ep=2)vs(tp=1, dp=2, moe_ep=2)→ both"dep2"tep{moe_tp}(MoE-TP)(tp=2, moe_tp=2)vs(tp=1, pp=2, moe_tp=2)→ both"tep2"tp{tp}(dense, non-MoE)(tp=2)vs(tp=2, pp=2)vs(tp=2, dp=2)→ all"tp2"The dense class is reachable any time the picker emits a non-MoE config with pp > 1 (multi-stage large dense) or dp > 1 (attention-DP without MoE), so the bug is not MoE-specific.
How the bug manifests downstream:
Both failures are silent: no exception, no log warning. The pick proceeds with corrupted data, and the downstream planner uses a perf model bootstrapped from the wrong row.
The fix:
label() now emits every dimension whose value is > 1.
make_parallel_label() mirrors the same encoding so the profiler and the planner agree.
(tp=2, dp=1, moe_tp=2, moe_ep=1) -> "tp2-moetp2"
(tp=2, dp=1, moe_tp=1, moe_ep=2) -> "tp2-moeep2"
(tp=1, dp=2, moe_tp=1, moe_ep=2) -> "tp1-dp2-moeep2" # was "dep2"
(tp=2, pp=2, dp=1, moe_tp=1, moe_ep=1) -> "tp2-pp2" # was "tp2" (dense+PP)
(tp=2, pp=1, dp=2, moe_tp=1, moe_ep=1) -> "tp2-dp2" # was "tp2" (dense+DP)
Test:
Added two unit-test files.
components/src/dynamo/planner/tests/unit/test_parallelization_label.pytest_label_unique_per_tuple(parametrized cross-producer agreement) ·test_label_distinct_across_all_enumerated_tuples(function-wide injectivity) ·test_regression_dep2_collision(explicit pin of the historical MoE-EP collision pair) ·test_label_format_pin(parametrized format pins — docstring examples, default-1 omission, dense+pp/dpcases)components/src/dynamo/profiler/tests/unit/test_aic_dataframe.pytest_label_distinct_across_all_enumerated_tuples(function-level injectivity) ·test_groupby_does_not_merge_distinct_topologies(consumer-shape regression againstdf.groupby("parallel")) ·test_make_parallel_label_format_pin(parametrized format pins)Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
The actual production-code change is small — about a dozen lines across two functions (
PickedParallelConfig.label()andmake_parallel_label()). For that reason no separate issue was filed — the bug manifestation is documented inline below.Summary by CodeRabbit
Bug Fixes
Tests
Documentation