Conversation
Signed-off-by: Terry Kong <terryk@nvidia.com>
Signed-off-by: Terry Kong <terryk@nvidia.com>
WalkthroughAdds a repository CODEOWNERS file with detailed path-based reviewer mappings; removes license header comments from two vLLM converter files (one left empty); deletes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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). (4)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
🧹 Nitpick comments (4)
.github/CODEOWNERS (4)
49-49: fp8.py currently drops generation owners due to last‑match wins.If you want both perf and generation notified, list both on the specific line.
-/nemo_rl/models/generation/fp8.py @nvidia-nemo/rl_reviewers_perf +/nemo_rl/models/generation/fp8.py @nvidia-nemo/rl_reviewers_generation @nvidia-nemo/rl_reviewers_perf
51-52: dtensor/megatron won’t match directories; cover dirs and files explicitly.**Add directory forms so nested content is owned too.
-/nemo_rl/models/policy/dtensor* @nvidia-nemo/rl_reviewers_automodel -/nemo_rl/models/policy/megatron* @nvidia-nemo/rl_reviewers_mcore +/nemo_rl/models/policy/dtensor*/ @nvidia-nemo/rl_reviewers_automodel +/nemo_rl/models/policy/dtensor*.py @nvidia-nemo/rl_reviewers_automodel +/nemo_rl/models/policy/megatron*/ @nvidia-nemo/rl_reviewers_mcore +/nemo_rl/models/policy/megatron*.py @nvidia-nemo/rl_reviewers_mcore
32-34: Narrow sft/dpo/rm globs to code files to avoid accidental matches (e.g., READMEs).Current patterns match any path containing “rm”, “sft”, “dpo” substrings. Tighten to .py/.ipynb; optionally route example READMEs to docs.
-/examples/**/*sft* @nvidia-nemo/rl_reviewers_supervised -/examples/**/*dpo* @nvidia-nemo/rl_reviewers_supervised -/examples/**/*rm* @nvidia-nemo/rl_reviewers_supervised +/examples/**/*sft*.py @nvidia-nemo/rl_reviewers_supervised +/examples/**/*sft*.ipynb @nvidia-nemo/rl_reviewers_supervised +/examples/**/*dpo*.py @nvidia-nemo/rl_reviewers_supervised +/examples/**/*dpo*.ipynb @nvidia-nemo/rl_reviewers_supervised +/examples/**/*rm*.py @nvidia-nemo/rl_reviewers_supervised +/examples/**/*rm*.ipynb @nvidia-nemo/rl_reviewers_supervised +# (optional) Example READMEs to docs team +/examples/**/README* @nvidia-nemo/rl_reviewers_doc
19-21: Enable enforcement.Ensure branch protection enables “Require review from Code Owners” on main so these rules are enforced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/CODEOWNERS(1 hunks)nemo_rl/converters/huggingface/vllm_export.py(0 hunks)nemo_rl/converters/megatron/vllm_export.py(0 hunks)nemo_rl/metrics/metrics_utils.py(0 hunks)
💤 Files with no reviewable changes (3)
- nemo_rl/converters/huggingface/vllm_export.py
- nemo_rl/converters/megatron/vllm_export.py
- nemo_rl/metrics/metrics_utils.py
🔇 Additional comments (3)
.github/CODEOWNERS (3)
60-60: Confirm intended scope for ray job scripts.Is the target a single root file “/ray.sub” or any “.sub” files under a ray folder? Adjust if needed.
Option if you meant any Ray submit scripts under a folder:
-/ray.sub @nvidia-nemo/rl_reviewers_infra +/ray/**/*.sub @nvidia-nemo/rl_reviewers_infra
1-1: Precedence note is correct.“Later matches have higher precedence” aligns with GitHub’s CODEOWNERS behavior.
57-59: Cannot verify — verification returned no case-sensitive matchesVerification run produced no matches; confirm these exact directories exist or update .github/CODEOWNERS (lines 57–59) to match repository casing: 3rdparty/Automodel-workspace, 3rdparty/Megatron-LM-workspace, 3rdparty/Megatron-Bridge-workspace.
Signed-off-by: Terry Kong <terryk@nvidia.com>
Signed-off-by: Terry Kong <terrycurtiskong@gmail.com>
ℹ️ File Consistency CheckCheck based on commit: 2234bd8 (PR #1133 from This is a test comment This check ensures that related file implementations remain synchronized across the codebase. If you believe this warning is incorrect or the files should intentionally differ, please add a comment explaining the reasoning. |
|
@coderabbitai configuration |
CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: false
enable_free_tier: true
reviews:
profile: chill
request_changes_workflow: false
high_level_summary: true
high_level_summary_placeholder: '@coderabbitai summary'
high_level_summary_in_walkthrough: false
auto_title_placeholder: '@coderabbitai'
auto_title_instructions: ''
review_status: true
commit_status: true
fail_commit_status: false
collapse_walkthrough: false
changed_files_summary: true
sequence_diagrams: true
estimate_code_review_effort: true
assess_linked_issues: true
related_issues: true
related_prs: true
suggested_labels: true
auto_apply_labels: false
suggested_reviewers: true
auto_assign_reviewers: false
poem: true
labeling_instructions: []
path_filters: []
path_instructions: []
abort_on_close: true
disable_cache: false
auto_review:
enabled: true
auto_incremental_review: true
ignore_title_keywords: []
labels: []
drafts: false
base_branches: []
ignore_usernames: []
finishing_touches:
docstrings:
enabled: true
unit_tests:
enabled: true
pre_merge_checks:
docstrings:
mode: warning
threshold: 80
title:
mode: warning
requirements: ''
description:
mode: warning
issue_assessment:
mode: warning
custom_checks: []
tools:
ast-grep:
rule_dirs: []
util_dirs: []
essential_rules: true
packages: []
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_rules: []
disabled_rules: []
enabled_categories: []
disabled_categories: []
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
phpmd:
enabled: true
phpcs:
enabled: true
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
checkov:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
flake8:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
cppcheck:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
clippy:
enabled: true
sqlfluff:
enabled: true
prismaLint:
enabled: true
pylint:
enabled: true
oxc:
enabled: true
shopifyThemeCheck:
enabled: true
luacheck:
enabled: true
brakeman:
enabled: true
dotenvLint:
enabled: true
htmlhint:
enabled: true
checkmake:
enabled: true
osvScanner:
enabled: true
chat:
art: true
auto_reply: true
integrations:
jira:
usage: auto
linear:
usage: auto
knowledge_base:
opt_out: false
web_search:
enabled: true
code_guidelines:
enabled: true
filePatterns: []
learnings:
scope: auto
issues:
scope: auto
jira:
usage: auto
project_keys: []
linear:
usage: auto
team_keys: []
pull_requests:
scope: auto
mcp:
usage: auto
disabled_servers: []
code_generation:
docstrings:
language: en-US
path_instructions: []
unit_tests:
path_instructions: []
|
chtruong814
removed this pull request from the merge queue due to the queue being cleared
Signed-off-by: Terry Kong <terryk@nvidia.com> Signed-off-by: Terry Kong <terrycurtiskong@gmail.com>
Signed-off-by: Terry Kong <terryk@nvidia.com> Signed-off-by: Terry Kong <terrycurtiskong@gmail.com>
3 participants
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
# Add a code snippet demonstrating how to use thisBefore your PR is "Ready for review"
Pre checks:
Additional Information
Summary by CodeRabbit
No changes to features or runtime behavior.