docs: update mcore optimizer docstrings to google style - #2799
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 @jaredcasper, I sincerely apologize for these unintended logical changes. During a rebase and squash to clean up my Git history, I must have incorrectly resolved some conflicts or applied auto-formatting that altered the code logic. I will reset the logic in these files to match |
Signed-off-by: Akshat Kumar <akshat230405@gmail.com>
Updated docstrings to improve clarity and detail regarding grad buffer shard ranges and ownership.
|
Hi @jaredcasper , I have restored the full explanation regarding conceptual partitioning, DP ownership, and the specific range mappings. Thank you for catching this! |
Removed detailed attribute documentation from OptimizerConfig.
|
Hi @jaredcasper, that makes perfect sense. I have removed the redundant |
|
Hi @chtruong814, could you pls also review my PR ?. The PR is ready for a final look. |
|
/ok to test dd73cfe |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/23955018970 |
Signed-off-by: Akshat Kumar <akshat230405@gmail.com>
Signed-off-by: Akshat Kumar <akshat230405@gmail.com> Signed-off-by: yhgalaxy <yhgalaxy@outlook.com>
Signed-off-by: Akshat Kumar <akshat230405@gmail.com> Signed-off-by: Jon Barker <jbarker@aws-cmh-slurm-1-vscode-02.cm.cluster>
# Description NVIDIA#4024 NVIDIA#3746 NVIDIA#2799 NVIDIA#3676 NVIDIA#4133 NVIDIA#3295 NVIDIA#3831 NVIDIA#3754 ## Type of change - [ ] New feature (non-breaking change which adds functionality) - [ ] Infra/Build change (changes to CI/CD workflows or build scripts) - [ ] Code refactoring - [ ] Documentation change - [x] Bug fix - [ ] Breaking change ## Changes - Content 1 - Content 2 - Content 3 - Content 4 ## Checklist - [ ] I have read and followed the contributing guidelines - [ ] The functionality is complete - [ ] I have commented my code, particularly in coverage report uploading steps - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added/updated tests that prove my feature works - [ ] New and existing unit tests pass locally --------- Signed-off-by: Cory Ye <cye@nvidia.com> Signed-off-by: yexiaochuan <yexiaochuan@baidu.com> Signed-off-by: Akshat Kumar <akshat230405@gmail.com> Signed-off-by: Youngeun Kwon <youngeunk@nvidia.com> Signed-off-by: jinliangl <jinliangl@nvidia.com> Co-authored-by: Ritesh Patel <ripatel@nvidia.com> Co-authored-by: Cory Ye <cye@nvidia.com> Co-authored-by: Cory Ye <44509866+cspades@users.noreply.github.com> Co-authored-by: Xiaochuan Ye <tap91624@gmail.com> Co-authored-by: yexiaochuan <yexiaochuan@baidu.com> Co-authored-by: Akshat8510 <akshat230405@gmail.com> Co-authored-by: Kunlun Li <94586211+kunlunl@users.noreply.github.com> Co-authored-by: eternally-z <zzywzj@gmail.com> Co-authored-by: Philip Petrakian <ppetrakian@nvidia.com> Co-authored-by: Youngeun Kwon <youngeunk@nvidia.com> Co-authored-by: xuwchen <xuwenc@nvidia.com> Co-authored-by: Li Jinliang <jinliangl@nvidia.com>
Signed-off-by: Akshat Kumar <akshat230405@gmail.com>
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