Skip to content

Conversation

@yaoyu-33
Copy link
Contributor

@yaoyu-33 yaoyu-33 commented Dec 17, 2025

What does this PR do ?

Add a one line overview of what this PR aims to accomplish.

Issues

List issues that this PR closes (syntax):

Usage

  • You can potentially add a usage example below
# Add a code snippet demonstrating how to use this

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you run the unit tests and functional tests locally? Visit our Testing Guide for how to run tests
  • Did you add or update any necessary documentation? Visit our Document Development Guide for how to write, build and test the docs.

Additional Information

  • ...

Summary by CodeRabbit

  • Chores
    • Updated workspace dependencies to latest versions.

✏️ Tip: You can customize this high-level summary in your review settings.

terrykong and others added 5 commits December 10, 2025 00:21
Signed-off-by: Terry Kong <terryk@nvidia.com>
Signed-off-by: Terry Kong <terryk@nvidia.com>
Signed-off-by: Terry Kong <terryk@nvidia.com>
Signed-off-by: Terry Kong <terryk@nvidia.com>
@yaoyu-33 yaoyu-33 requested a review from a team as a code owner December 17, 2025 22:59
@github-actions
Copy link

❌ Submodule Fast-Forward Check Failed

Check based on commit: 95984f5 (PR #1653 from chore/update-megatron-submodule-ed804b4)

❌ Submodules that need attention:

Megatron-LM: ❌ Commits have DIVERGED from a common ancestor
TARGET (main branch): https://github.com/terrykong/Megatron-LM/commits/25a62edf77b5130f888328ca8119d6a76117cf23/
CURRENT (PR #1653 from chore/update-megatron-submodule-ed804b4): https://github.com/terrykong/Megatron-LM/commits/2f28198c37d5e7d4616f43f13e614c61671f5866/

Please ensure all submodule commits are fast-forwards of the main branch before merging.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 17, 2025

📝 Walkthrough

Walkthrough

The PR updates the Megatron-LM submodule pointer from commit 25a62edf77b5130f888328ca8119d6a76117cf23 to 2f28198c37d5e7d4616f43f13e614c61671f5866. This is a subproject commit bump with no functional code changes to the repository itself.

Changes

Cohort / File(s) Change Summary
Submodule Pointer Update
3rdparty/Megatron-LM-workspace/Megatron-LM
Updated submodule commit reference

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

  • Submodule pointer update requires verification that the target commit is valid and intentional
  • No functional changes in the main repository to review

Possibly related PRs

  • RL#1568 — Also updates the 3rdparty/Megatron-LM-workspace/Megatron-LM submodule pointer

Suggested labels

CI:L1

Suggested reviewers

  • terrykong
  • yfw

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Test Results For Major Changes ⚠️ Warning PR updates Megatron-LM submodule without test results, validation, performance benchmarks, or documentation of changes and compatibility requirements. Add test results, before-and-after performance benchmarks, summary of Megatron-LM changes, and resolve commit hash mismatch between PR title and actual submodule pointer.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title correctly describes the main change: updating the Megatron-LM submodule. It is concise, specific, and clearly communicates the primary objective of the PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/update-megatron-submodule-ed804b4

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 441f745 and 95984f5.

📒 Files selected for processing (1)
  • 3rdparty/Megatron-LM-workspace/Megatron-LM (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
!(**/tests/**|**/test_*.py|**/test_*.sh)

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

Add the NVIDIA copyright header to all Python files and shell scripts (excluding tests). The header should include the current year

Files:

  • 3rdparty/Megatron-LM-workspace/Megatron-LM
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Post automodel integration comment / Comment on PR

@@ -1 +1 @@
Subproject commit 25a62edf77b5130f888328ca8119d6a76117cf23
Subproject commit 2f28198c37d5e7d4616f43f13e614c61671f5866
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Find and check the git submodule file
ls -la 3rdparty/Megatron-LM-workspace/ 2>/dev/null || echo "Directory not found at expected location"
find . -name "Megatron-LM" -type f 2>/dev/null | head -20
# Check git configuration
if [ -f .gitmodules ]; then
  cat .gitmodules | grep -A 3 "Megatron"
fi
# Look at git history
git log --oneline -20 -- "*Megatron-LM*" 2>/dev/null | head -10

Repository: NVIDIA-NeMo/RL

Length of output: 906


🏁 Script executed:

# Check the actual content of the submodule pointer
cat 3rdparty/Megatron-LM-workspace/Megatron-LM

# Also check git ls-tree to see what commit is currently recorded
git ls-tree HEAD 3rdparty/Megatron-LM-workspace/Megatron-LM

# Check the previous commit to see what changed
git show HEAD:3rdparty/Megatron-LM-workspace/Megatron-LM 2>/dev/null || echo "No previous version"
git log -p --follow -1 -- 3rdparty/Megatron-LM-workspace/Megatron-LM 2>/dev/null | head -20

Repository: NVIDIA-NeMo/RL

Length of output: 256


🏁 Script executed:

# Check if ed804b4 could be related to the full hash
# First 8 chars of 2f28198c37d5e7d4616f43f13e614c61671f5866
echo "Full hash starts with: $(echo '2f28198c37d5e7d4616f43f13e614c61671f5866' | cut -c1-8)"
echo "PR title hash: ed804b4"

# Try to verify what the PR author intended
git log --oneline --all | grep -i "ed804b4\|2f28198c" | head -5
git show ed804b4 2>/dev/null | head -5 || echo "ed804b4 not found in repository"

Repository: NVIDIA-NeMo/RL

Length of output: 167


Commit hash mismatch: PR title references ed804b4 but submodule points to 2f28198c37d5e7d4616f43f13e614c61671f5866.

The git submodule file shows a completely different commit than advertised in the PR title and commit message. Verify that the intended commit is 2f28198c37d5e7d4616f43f13e614c61671f5866 and update the PR title/message accordingly, or confirm whether ed804b4 was the intended target and correct the submodule pointer.

🤖 Prompt for AI Agents
In 3rdparty/Megatron-LM-workspace/Megatron-LM around lines 1 to 1, the submodule
commit recorded (2f28198c37d5e7d4616f43f13e614c61671f5866) does not match the
commit referenced in the PR title/message (ed804b4); update the inconsistency by
either (A) changing the .gitmodules/submodule pointer and the submodule's HEAD
to ed804b4 if that is the intended commit (run git submodule set-branch / update
and commit the new SHA), or (B) modify the PR title and commit message to
reference 2f28198c37d5e7d4616f43f13e614c61671f5866 if that is the correct
commit; ensure you run git submodule sync/update and include the resulting
submodule SHA in the commit message so the repo and PR metadata match.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "*.gitmodules" -o -name ".gitmodules" | head -5

Repository: NVIDIA-NeMo/RL

Length of output: 71


🏁 Script executed:

cat ./.gitmodules

Repository: NVIDIA-NeMo/RL

Length of output: 778


🏁 Script executed:

git log --oneline -20 3rdparty/Megatron-LM-workspace/Megatron-LM 2>/dev/null | head -10

Repository: NVIDIA-NeMo/RL

Length of output: 112


🏁 Script executed:

git log -1 --format=fuller 95984f5

Repository: NVIDIA-NeMo/RL

Length of output: 329


🏁 Script executed:

git log --all --grep="Megatron" --oneline | head -20

Repository: NVIDIA-NeMo/RL

Length of output: 1050


🏁 Script executed:

git show 95984f5 --stat

Repository: NVIDIA-NeMo/RL

Length of output: 38228


🏁 Script executed:

git show 95984f5:3rdparty/Megatron-LM-workspace/Megatron-LM

Repository: NVIDIA-NeMo/RL

Length of output: 231


🏁 Script executed:

git show 95984f5 --format=fuller

Repository: NVIDIA-NeMo/RL

Length of output: 50374


🏁 Script executed:

git log --all --oneline | head -5

Repository: NVIDIA-NeMo/RL

Length of output: 370


🏁 Script executed:

cd 3rdparty/Megatron-LM-workspace/Megatron-LM 2>/dev/null && git log --oneline -5 2>/dev/null || echo "Submodule not initialized"

Repository: NVIDIA-NeMo/RL

Length of output: 112


🏁 Script executed:

git log 95984f5 -1 --format="%B"

Repository: NVIDIA-NeMo/RL

Length of output: 105


🏁 Script executed:

head -50 CODING_GUIDELINES.md

Repository: NVIDIA-NeMo/RL

Length of output: 1103


Provide comprehensive PR description for Megatron-LM submodule update.

The commit message lacks essential context for a major infrastructure change. This submodule update affects training performance and numerical convergence. Include in the PR description:

  1. Summary of changes in this Megatron-LM commit (ed804b4) relative to the previous version
  2. Any breaking changes or API modifications affecting downstream code
  3. Testing/validation results performed before this update
  4. Required NeMo RL compatibility updates, if any

@github-actions
Copy link

❌ Submodule Fast-Forward Check Failed

Check based on commit: 6bdb470 (PR #1653 from chore/update-megatron-submodule-ed804b4)

✅ Submodules that are properly updated:

Megatron-Bridge: ✅ PR branch is ahead of main branch (fast-forward)

❌ Submodules that need attention:

Megatron-LM: ❌ Commits have DIVERGED from a common ancestor
TARGET (main branch): https://github.com/terrykong/Megatron-LM/commits/25a62edf77b5130f888328ca8119d6a76117cf23/
CURRENT (PR #1653 from chore/update-megatron-submodule-ed804b4): https://github.com/terrykong/Megatron-LM/commits/2f28198c37d5e7d4616f43f13e614c61671f5866/

Please ensure all submodule commits are fast-forwards of the main branch before merging.

@github-actions
Copy link

❌ Submodule Fast-Forward Check Failed

Check based on commit: 2ef25ef (PR #1653 from chore/update-megatron-submodule-ed804b4)

✅ Submodules that are properly updated:

Megatron-Bridge: ✅ PR branch is ahead of main branch (fast-forward)

❌ Submodules that need attention:

Megatron-LM: ❌ Commits have DIVERGED from a common ancestor
TARGET (main branch): https://github.com/terrykong/Megatron-LM/commits/25a62edf77b5130f888328ca8119d6a76117cf23/
CURRENT (PR #1653 from chore/update-megatron-submodule-ed804b4): https://github.com/terrykong/Megatron-LM/commits/2f28198c37d5e7d4616f43f13e614c61671f5866/

Please ensure all submodule commits are fast-forwards of the main branch before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants