Conversation
Signed-off-by: Raghav Hrishikeshan Mukundan <rmukundan@nvidia.com>
Signed-off-by: Raghav Hrishikeshan Mukundan <rmukundan@nvidia.com>
📝 WalkthroughWalkthroughThis PR adds new LoRA configuration variants for Llama3 70B models targeting B200 and B300 GPUs. The changes span three files: base workload configurations, fine-tuning function definitions, and public module exports. All modifications are purely additive with no existing code alterations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/performance/configs/llama/__init__.py (2)
40-119:⚠️ Potential issue | 🔴 CriticalMissing imports for
LLAMA3_70B_LORA_CONFIG_B300_*base config constants.The B300 LoRA base configs (
LLAMA3_70B_LORA_CONFIG_B300_BF16_V1,LLAMA3_70B_LORA_CONFIG_B300_FP8_CS_V1,LLAMA3_70B_LORA_CONFIG_B300_FP8_MX_V1) are defined and exported inllama3_workload_base_configs.py(lines 616–621, 741–743) but are not imported here, unlike their B200 counterparts (lines 67–69).Proposed fix — add B300 LoRA imports alongside the B200 ones
LLAMA3_70B_LORA_CONFIG_B200_BF16_V1, LLAMA3_70B_LORA_CONFIG_B200_FP8_CS_V1, LLAMA3_70B_LORA_CONFIG_B200_FP8_MX_V1, + LLAMA3_70B_LORA_CONFIG_B300_BF16_V1, + LLAMA3_70B_LORA_CONFIG_B300_FP8_CS_V1, + LLAMA3_70B_LORA_CONFIG_B300_FP8_MX_V1, LLAMA3_70B_LORA_CONFIG_GB300_BF16_V1,And add the corresponding entries to
__all__:"LLAMA3_70B_LORA_CONFIG_B200_FP8_MX_V1", + "LLAMA3_70B_LORA_CONFIG_B300_BF16_V1", + "LLAMA3_70B_LORA_CONFIG_B300_FP8_CS_V1", + "LLAMA3_70B_LORA_CONFIG_B300_FP8_MX_V1", "LLAMA3_70B_LORA_CONFIG_GB300_BF16_V1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/performance/configs/llama/__init__.py` around lines 40 - 119, The file is missing imports and exports for the B300 LoRA base configs; add imports for LLAMA3_70B_LORA_CONFIG_B300_BF16_V1, LLAMA3_70B_LORA_CONFIG_B300_FP8_CS_V1, and LLAMA3_70B_LORA_CONFIG_B300_FP8_MX_V1 to the top import list (next to the existing LLAMA3_70B_LORA_CONFIG_B200_* entries) and include those three symbols in the module's __all__ export list so the B300 LoRA configs are available where expected.
9-19:⚠️ Potential issue | 🔴 CriticalMissing import for
llama3_70b_lora_config_b300.The B300 LoRA function is defined in
llama3_llm_finetune.py(line 262) but is not imported here. Onlyllama3_70b_lora_config_b200was added (line 12). Similarly, it's missing from the__all__extension block (line 286).Proposed fix
if HAVE_MEGATRON_BRIDGE: from .llama3_llm_finetune import ( llama3_8b_sft_config_gb200, llama3_8b_sft_config_h100, llama3_70b_lora_config_b200, + llama3_70b_lora_config_b300, llama3_70b_lora_config_gb200, llama3_70b_lora_config_gb300,And in the
__all__.extendblock:"llama3_70b_lora_config_b200", + "llama3_70b_lora_config_b300", "llama3_70b_lora_config_gb200",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/performance/configs/llama/__init__.py` around lines 9 - 19, Add the missing export for the B300 LoRA config: import llama3_70b_lora_config_b300 alongside the other imports from llama3_llm_finetune (so it appears with llama3_70b_lora_config_b200 etc.), and also add "llama3_70b_lora_config_b300" to the __all__.extend block so it is exported; locate the import list and the __all__.__extend block by the symbols llama3_70b_lora_config_b200 and __all__.extend respectively and insert the new name in both places.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/performance/configs/llama/__init__.py`:
- Around line 40-119: The file is missing imports and exports for the B300 LoRA
base configs; add imports for LLAMA3_70B_LORA_CONFIG_B300_BF16_V1,
LLAMA3_70B_LORA_CONFIG_B300_FP8_CS_V1, and LLAMA3_70B_LORA_CONFIG_B300_FP8_MX_V1
to the top import list (next to the existing LLAMA3_70B_LORA_CONFIG_B200_*
entries) and include those three symbols in the module's __all__ export list so
the B300 LoRA configs are available where expected.
- Around line 9-19: Add the missing export for the B300 LoRA config: import
llama3_70b_lora_config_b300 alongside the other imports from llama3_llm_finetune
(so it appears with llama3_70b_lora_config_b200 etc.), and also add
"llama3_70b_lora_config_b300" to the __all__.extend block so it is exported;
locate the import list and the __all__.__extend block by the symbols
llama3_70b_lora_config_b200 and __all__.extend respectively and insert the new
name in both places.
Signed-off-by: Raghav Hrishikeshan Mukundan <rmukundan@nvidia.com>
Summary by CodeRabbit
Release Notes