update mcore requirements#120
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the minimum required version of megatron-core to 0.16 (up to <0.19), allowing the removal of legacy compatibility workarounds and conditional checks for older versions. However, a critical issue was identified in lora.py where removing the tp_group propagation block entirely will cause non-grouped parallel layers to default to the global tensor parallel group, potentially leading to silent correctness issues or runtime crashes. It is recommended to restore tp_group propagation for non-grouped layers as suggested.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| 'config': self.config, | ||
| 'is_expert': self.is_expert, | ||
| } |
There was a problem hiding this comment.
By removing the tp_group assignment block entirely, non-grouped parallel layers (such as TEColumnParallelLinear and TERowParallelLinear) will no longer receive the correct tp_group from the base layer. This will cause them to default to the global tensor parallel group, which can lead to silent correctness issues or runtime crashes when custom or non-default tensor parallel groups are used.
Please restore the tp_group propagation for non-grouped layers.
| 'config': self.config, | |
| 'is_expert': self.is_expert, | |
| } | |
| 'config': self.config, | |
| 'is_expert': self.is_expert, | |
| } | |
| if not self.is_grouped: | |
| tp_group = getattr(self.base_layer, 'tp_group', getattr(self.base_layer, 'parallel_group', None)) | |
| if tp_group is not None: | |
| kwargs['tp_group'] = tp_group |
3f7f787 to
2c03259
Compare
No description provided.