[example, doc] fix: Fix RLHF script bugs and tutorial tokenization path#2364
[example, doc] fix: Fix RLHF script bugs and tutorial tokenization path#2364
Conversation
- Add missing trust_remote_code field to Args dataclass in rlhf_with_bridge.py - Add missing pg_collection argument to get_model() call - Fix MEGATRON_LM_PATH in reduced_precision_training.ipynb to use correct path Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
|
/ok to test bc5232c |
📝 WalkthroughWalkthroughPR updates a Megatron-LM submodule reference, introduces Git commit workflow documentation, extends an RLHF example with process group collection support and new configuration options, and updates a tutorial path reference. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@GIT_COMMIT_SKILLS.md`:
- Around line 35-36: Remove the hardcoded, user-specific PATH export and replace
it with a generic instruction: delete the line containing export
PATH="/Users/yuya/Library/Python/3.9/bin:$PATH" and instead state that
contributors should ensure pre-commit is installed and available on their PATH
(e.g., install via pip/pipx or add their own user bin to PATH), leaving the
pre-commit run line unchanged so examples use the generic pre-commit run
command.
🧹 Nitpick comments (3)
examples/rl/rlhf_with_bridge.py (1)
1-1: Copyright year should be updated to 2026.The copyright header says
2025but the current year is 2026. Since lines in this file are being modified in this PR, consider updating the copyright year.Proposed fix
-# Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved. +# Copyright (c) 2025-2026, NVIDIA CORPORATION. All rights reserved.GIT_COMMIT_SKILLS.md (2)
1-143: This file appears to be a personal workflow cheat-sheet rather than project documentation.This document reads like an AI-agent or personal developer instruction set (hardcoded local paths, specific branch/file references in examples, CI trigger commands). It doesn't align with the PR's stated scope of fixing RLHF script bugs and the tutorial tokenization path.
Consider whether this file should:
- Be excluded from this PR entirely (it's unrelated to the bug fixes).
- Be moved to a
CONTRIBUTING.mdsection or.github/directory if it's intended as contributor guidance.- Have the examples generalized (replace specific file/branch names with placeholders).
63-92: Example section uses overly specific references that will become stale.Line 76 references a specific test file (
tests/unit_tests/models/gemma_vl/test_gemma3_vl_bridge.py), line 83 references a specific commit message, and line 89 references a specific branch (feature/provider-bridge-refactor-3). These are real session artifacts that will confuse future readers.Replace with generic placeholders like
<your-file>,<your-branch>, etc.Proposed fix for the example section
# 3. Stage changes -git add tests/unit_tests/models/gemma_vl/test_gemma3_vl_bridge.py +git add path/to/your/changed_file.py # 4. Run pre-commit -export PATH="/Users/yuya/Library/Python/3.9/bin:$PATH" pre-commit run # 5. Commit with sign-off -git commit -s -m "[test] fix: Fix gemma3_vl bridge test for image_token_id default" +git commit -s -m "[module] type: Your descriptive commit message" # 6. Push git push # 7. Check for PR and trigger CI -PR_NUMBER=$(gh pr list --head feature/provider-bridge-refactor-3 --json number --jq '.[0].number') +PR_NUMBER=$(gh pr list --head feature/your-feature-name --json number --jq '.[0].number') COMMIT_HASH=$(git rev-parse HEAD) gh pr comment $PR_NUMBER --body "/ok to test $COMMIT_HASH"
| export PATH="/Users/yuya/Library/Python/3.9/bin:$PATH" | ||
| pre-commit run |
There was a problem hiding this comment.
Hardcoded user-specific PATH leaks a local username and won't work for other contributors.
Lines 35 and 79 both contain /Users/yuya/Library/Python/3.9/bin — this is a macOS-specific path tied to a single developer's machine. It won't work for anyone else and inadvertently exposes a username.
If pre-commit is installed properly (e.g., via uv or pip), it should already be on $PATH. Remove the export PATH=... line or replace it with a generic instruction.
Proposed fix
### 4. Run Pre-commit Hooks
```bash
-export PATH="/Users/yuya/Library/Python/3.9/bin:$PATH"
pre-commit run
And similarly in the example section (line 79):
```diff
# 4. Run pre-commit
-export PATH="/Users/yuya/Library/Python/3.9/bin:$PATH"
pre-commit run
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export PATH="/Users/yuya/Library/Python/3.9/bin:$PATH" | |
| pre-commit run | |
| pre-commit run |
🤖 Prompt for AI Agents
In `@GIT_COMMIT_SKILLS.md` around lines 35 - 36, Remove the hardcoded,
user-specific PATH export and replace it with a generic instruction: delete the
line containing export PATH="/Users/yuya/Library/Python/3.9/bin:$PATH" and
instead state that contributors should ensure pre-commit is installed and
available on their PATH (e.g., install via pip/pipx or add their own user bin to
PATH), leaving the pre-commit run line unchanged so examples use the generic
pre-commit run command.
1 participant
Summary
This PR fixes two bugs in the RLHF example script and one path issue in the tutorial notebook.
Changes
Bug 1: Missing trust_remote_code field in Args dataclass
trust_remote_code: bool = Falseto the Args dataclasstrust_remote_code=ns.trust_remote_code,to Args constructorBug 2: Missing pg_collection argument in get_model() call
from megatron.core.process_groups_config import ProcessGroupCollectionpg_collection = ProcessGroupCollection.use_mpu_process_groups()after initialize_megatron()pg_collection=pg_collection,to get_model() callBug 3: Incorrect path in tutorial notebook
MEGATRON_LM_PATH=/opt/megatron-lm/toMEGATRON_LM_PATH=/opt/Megatron-Bridge/3rdparty/Megatron-LM/Testing
Summary by CodeRabbit
New Features
Documentation
Chores