docs: update mcore optimizer docstrings to google style#2799
docs: update mcore optimizer docstrings to google style#2799Akshat8510 wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
|
Hi @Phlip79, I have submitted a PR for the optimizer and clip_grads modules as part of this docstring update. |
|
Thank you Akshat! Could you please ensure that your changes do not reduce verbosity? |
|
Thank you for the feedback, @Phlip79! I will do a thorough pass over all the changed files to ensure that all technical details and explanations from the original comments are preserved within the new Google Style format. I'll update the PR shortly. |
7199c88 to
8e80e8f
Compare
|
Hi @Phlip79, I have updated the docstrings across all 6 files to restore the original technical verbosity and detailed explanations while maintaining the Google Style structure. This includes restoring the core mapping logic in distrib_optimizer.py, the FSDP/filtering details in clip_grads.py, and the in-place modification warnings. Thank you for the feedback! |
|
Hi @Phlip79 and @chtruong814, I wanted to follow up on this PR. I have addressed the feedback regarding technical verbosity across all 6 files. |
|
I noticed the needs-follow-up label is still active, please let me know if there are any further changes required on my end to clear this and move toward a merge. Thank you! |
c97b8cd to
71a045f
Compare
|
Hi @Phlip79 @chtruong814, I've squashed the commits and restored the technical verbosity as requested. Should I update the branch whenever it is out-of-date with main, or only if merge conflicts occur? |
|
Hi @Phlip79 and @chtruong814, it's been two weeks since I restored the technical verbosity and squashed the history. The PR is ready for a final look. |
|
Could you please trigger the internal tests (approve the workflow) so we can see if any further adjustments are needed? Thank you! |
|
Hi @Phlip79 and @chtruong814, |
|
Hi @chtruong814, |
|
it's been more than 3 weeks since I restored the technical verbosity and squashed the history. The PR is ready for a final look. |
|
Hi @chtruong814, I've addressed all the requested changes, restored the technical verbosity |
|
The PR is ready for review. |
|
hi @chtruong814 , thank you for removing the follow up label. can you pls review my PR ? It was about improving standardizes the docstrings for the megatron/core/optimizer module to follow the Google Style Guide. |
|
Hi @chtruong814, I noticed the Happy to address them. Thanks! |
|
Hi @chtruong814, I noticed the Happy to address them. Thanks! |
|
Thank you @chtruong814 for removing follow up label, pls review this PR |
|
Hi @chtruong814, I noticed the Happy to address them. Thanks! |
|
Thank you @chtruong814 for removing follow up label, pls review this PR |
Phlip79
left a comment
There was a problem hiding this comment.
Thank you for taking the time to create this PR. I have a few nits.
Apologies it took so long to review, this PR went completely unnoticed because the "Expert Review" label was not added.
We are actually changing the review process moving forward to make the process more clear in #3659.
|
/ok to test b0aa307 |
Signed-off-by: Akshat Kumar <akshat230405@gmail.com>
b0aa307 to
f2c3f3b
Compare
|
Hi @Phlip79, I have squashed the branch into a single clean commit and restored the technical details in |
|
And I like the new PR review change mention in 3659 with this, PR review become more smoother |
Description
This PR updates and standardizes the docstrings for the
megatron/core/optimizermodule to follow the Google Style Guide. This is part of the documentation overhaul tracked in issue #2653.Changes
optimizer.py,distrib_optimizer.py,grad_scaler.py, andclip_grads.pywith structuredArgs,Returns, andAttributesblocks.OptimizerConfigdataclass attributes for better compatibility with auto-generated API guides.Files Updated:
Mentions: @Phlip79 @sbhavani