[megatron] update cross_entropy_fusion_impl default value 'te' -> 'native' (#9578)#9578
Conversation
Jintao-Huang
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request changes the default implementation of cross-entropy loss fusion (cross_entropy_fusion_impl) from dynamic resolution to a static default of 'native', updating both the code and documentation. The feedback suggests keeping the field type as Optional and adding a defensive fallback to 'native' in post_init to prevent runtime errors if None is explicitly passed or deserialized.
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.
| gradient_accumulation_fusion: bool = True | ||
| cross_entropy_loss_fusion: bool = True | ||
| cross_entropy_fusion_impl: Optional[Literal['native', 'te']] = None | ||
| cross_entropy_fusion_impl: Literal['native', 'te'] = 'native' |
There was a problem hiding this comment.
Changing the type of cross_entropy_fusion_impl to Literal['native', 'te'] (non-optional) and removing the None check in __post_init__ can lead to runtime errors if the argument is explicitly passed as None or deserialized as None from older configurations or external parsers. Keeping it as Optional and adding a fallback to 'native' in __post_init__ is safer and more robust.
| cross_entropy_fusion_impl: Literal['native', 'te'] = 'native' | |
| cross_entropy_fusion_impl: Optional[Literal['native', 'te']] = 'native' |
| if isinstance(self.report_to, str): | ||
| self.report_to = [self.report_to] |
There was a problem hiding this comment.
To prevent potential runtime issues when cross_entropy_fusion_impl is explicitly set or parsed as None, we should add a defensive check in __post_init__ to default it to 'native' if it is None.
| if isinstance(self.report_to, str): | |
| self.report_to = [self.report_to] | |
| if self.cross_entropy_fusion_impl is None: | |
| self.cross_entropy_fusion_impl = 'native' | |
| if isinstance(self.report_to, str): | |
| self.report_to = [self.report_to] |
Jintao-Huang
changed the title
2 participants
No description provided.