Conversation
📝 WalkthroughWalkthroughThese changes simplify and refactor DeepSeek V3 pretraining configurations for GB300 hardware profiles, consolidating redundant configurations into aliases and inheriting from base settings. Additionally, MoE-specific logic is introduced to preserve cuDNN LayerNorm for MXFP8 and BF16 compute types. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🤖 Fix all issues with AI agents
In `@scripts/performance/configs/deepseek/deepseek_workload_base_configs.py`:
- Around line 55-58: The four variant constants
DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_BF16_V1,
DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_FP8_CS_V1,
DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_FP8_MX_V1 and
DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_NVFP4_V1 should be created as copies of
DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_V1 using dataclasses.replace() rather than
direct assignment to avoid aliasing the same WorkloadBaseConfig instance; update
these assignments (and any similar variant assignments elsewhere, e.g., the ones
you noted around line 137) to call replace(DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_V1,
...) so each variant is its own dataclass instance (or alternatively make
WorkloadBaseConfig frozen=True if you prefer enforced immutability).
| DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_BF16_V1 = DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_V1 | ||
| DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_FP8_CS_V1 = DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_V1 | ||
| DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_FP8_MX_V1 = DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_V1 | ||
| DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_NVFP4_V1 = DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_V1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect WorkloadBaseConfig definition and cloning behavior
rg -n "class WorkloadBaseConfig" -C 3
rg -n "def get_workload_base_config" -C 3
rg -n "pp_layout|WorkloadBaseConfig\(" -C 2Repository: NVIDIA-NeMo/Megatron-Bridge
Length of output: 9797
🏁 Script executed:
#!/bin/bash
# Get full WorkloadBaseConfig class definition
sed -n '29,78p' scripts/performance/utils/utils.py
# Check line 137 in deepseek file
sed -n '130,145p' scripts/performance/configs/deepseek/deepseek_workload_base_configs.pyRepository: NVIDIA-NeMo/Megatron-Bridge
Length of output: 2661
🏁 Script executed:
#!/bin/bash
# Check imports in deepseek_workload_base_configs.py
head -20 scripts/performance/configs/deepseek/deepseek_workload_base_configs.py
# Search for any mutations/assignments to these config variables
rg "DEEPSEEK_V3_PRETRAIN_CONFIG.*=" scripts/performance/configs/deepseek/deepseek_workload_base_configs.py -n
# Check if any code mutates config objects after assignment
rg "base_cfg\." scripts/performance/configs/deepseek/ -n | head -20Repository: NVIDIA-NeMo/Megatron-Bridge
Length of output: 6886
🏁 Script executed:
#!/bin/bash
# Check imports for 'replace'
head -30 scripts/performance/configs/deepseek/deepseek_workload_base_configs.py | grep -i "import\|from"Repository: NVIDIA-NeMo/Megatron-Bridge
Length of output: 145
Use replace() for all variant configs to avoid aliasing mutable dataclass instances.
These aliases (lines 55–58, and throughout the file including line 137) reference the same WorkloadBaseConfig instance. While the current usage is read-only, this pattern creates fragile coupling. The inconsistency—where V2 base configs use replace() but variants use direct assignment—should be unified. Consider applying replace() consistently for all variants, or mark WorkloadBaseConfig with frozen=True to enforce immutability.
🤖 Prompt for AI Agents
In `@scripts/performance/configs/deepseek/deepseek_workload_base_configs.py`
around lines 55 - 58, The four variant constants
DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_BF16_V1,
DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_FP8_CS_V1,
DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_FP8_MX_V1 and
DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_NVFP4_V1 should be created as copies of
DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_V1 using dataclasses.replace() rather than
direct assignment to avoid aliasing the same WorkloadBaseConfig instance; update
these assignments (and any similar variant assignments elsewhere, e.g., the ones
you noted around line 137) to call replace(DEEPSEEK_V3_PRETRAIN_CONFIG_GB300_V1,
...) so each variant is its own dataclass instance (or alternatively make
WorkloadBaseConfig frozen=True if you prefer enforced immutability).
Signed-off-by: Dingqing Yang <dingqingy@nvidia.com>
f4fb433 to
3b799fc
Compare
Signed-off-by: Dingqing Yang <dingqingy@nvidia.com> Signed-off-by: NeMo Bot <nemo-bot@nvidia.com>
Signed-off-by: Dingqing Yang <dingqingy@nvidia.com>
What does this PR do ?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.