(bug)Zero: Only support training with ms2.7.0 when using ZeRO #1383
Conversation
wtomin
requested review from
CaitinZhao,
SamitHuang,
vigo999 and
zhanghuiyao
as code owners
Summary of ChangesHello @wtomin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical bug in the ZeRO optimization implementation by reverting a problematic change in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 fixes a bug in get_optimizer_param_tuples by reverting a previous change, which simplifies the logic for finding optimizer parameter tuples. This change, along with the README.md update, restricts ZeRO support to MindSpore 2.7.0. While the fix is correct, I've added a comment regarding the implementation's reliance on internal optimizer details, which poses a maintainability risk.
| for attr in self.optimizer.__dict__: | ||
| if isinstance(getattr(self.optimizer, attr), ms.ParameterTuple): | ||
| if attr in ["_parameters", "parameters"]: | ||
| continue | ||
| _logger.debug(f"Add optimizer param_tuples {name}") | ||
| param_tuples.append(getattr(self.optimizer, name)) | ||
| else: | ||
| for attr in self.optimizer.__dict__: | ||
| if isinstance(getattr(self.optimizer, attr), ms.ParameterTuple): | ||
| if attr in ["_parameters", "parameters"]: | ||
| continue | ||
| _logger.debug(f"Add optimizer param_tuples {attr}") | ||
| param_tuples.append(getattr(self.optimizer, attr)) | ||
| _logger.debug(f"Add optimizer param_tuples {attr}") | ||
| param_tuples.append(getattr(self.optimizer, attr)) |
There was a problem hiding this comment.
Iterating over self.optimizer.__dict__ to find optimizer state parameter tuples is fragile as it depends on the internal implementation details of MindSpore's optimizer classes. This could lead to unexpected behavior or breakages if the internal structure of these optimizers changes in a future MindSpore release. It would be more robust to rely on a public API if one is available for accessing optimizer states. If not, consider adding a comment to explain why this approach is necessary and to highlight its potential fragility for future maintainers.
2 participants
What does this PR do?
Fixes # (issue)
Due to the bug in
get_optimizer_param_tuplesinzero.py, now revert the change introduced in #1377 , and then only support training with ms2.7.0 when using ZeROBefore submitting
What's New. Here are thedocumentation guidelines
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@SamitHuang @zhtmike @CaitinZhao