Conversation
github-actions
Bot
added
the
Documentation
lbliii
force-pushed
the
llane/get-started-section
branch
from
3df12ee to
16da7b7
Compare
jgerh
left a comment
jgerh
left a comment
There was a problem hiding this comment.
Completed the review of get-started articles. The articles are clear, accessible, and engaging. Consistent style makes them easy to navigate, the teaching tone feels welcoming, and putting the "Steps" section upfront gives a clear path. The “What’s happening” sections also add helpful context, the index is a strong entry point into RL, and the Sphinx cards and tabs keep the presentation compact and visually interesting.
📝 WalkthroughWalkthroughDocumentation reorganization and relocation effort that moves setup, installation, and quickstart guides from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (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
🧹 Nitpick comments (4)
docs/get-started/cluster.md (1)
70-71: Remove "re-" from "re-queueing".Line 71 uses "re-queueing" which is flagged by analysis tools. The standard spelling in this context is "requeuing" (without hyphen) or "re-submitting."
-Interactive mode launches the cluster and gives you a shell on the **Head Node**. This is perfect for debugging because you can run scripts, check files, and kill/restart jobs without re-queueing. +Interactive mode launches the cluster and gives you a shell on the **Head Node**. This is perfect for debugging because you can run scripts, check files, and kill/restart jobs without resubmitting.docs/get-started/index.md (3)
123-131: Specify language for code block.The code block showing expected output should have a language specified for consistency with other examples in the documentation.
- **Example output**: - ``` + **Example output**: + ```text Initializing Ray cluster... Ray dashboard available at http://127.0.0.1:8265 Loading model: Qwen/Qwen2.5-1.5B-Instruct Training started... Step 1: reward=0.25, policy_kl_error=0.001 Step 2: reward=0.31, policy_kl_error=0.002 ... - ``` + ```
137-197: Convert dropdown headers from emphasis to proper Markdown headings.Lines 137, 148, 168, and 172 use bold text as pseudo-headers within dropdown sections. Per Markdown best practices (MD036), these should be converted to proper heading levels (e.g.,
### How to Control GPU Usage & Run Concurrent Jobs) for semantic correctness and consistency.-:::{dropdown} 💡 How to Control GPU Usage & Run Concurrent Jobs +:::{dropdown} 💡 How to Control GPU Usage & Run Concurrent Jobs -**Controlling GPU Usage** +#### Controlling GPU UsageApply the same pattern to the other emphasis-as-heading instances at lines 148, 168, and 172.
24-26: Add periods for consistency with other steps sections.Per previous feedback, steps sections should have periods for consistency. Lines 24-26 lack terminal punctuation.
**Steps**: -1. Install `uv` and system prerequisites -2. Clone the repository and initialize the environment -3. Run a sample GRPO training job to verify installation +1. Install `uv` and system prerequisites. +2. Clone the repository and initialize the environment. +3. Run a sample GRPO training job to verify installation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/about/clusters.md(0 hunks)docs/about/installation.md(0 hunks)docs/about/quick-start.md(0 hunks)docs/cluster.md(0 hunks)docs/debugging.md(1 hunks)docs/get-started/cluster.md(1 hunks)docs/get-started/dpo.md(1 hunks)docs/get-started/grpo.md(1 hunks)docs/get-started/index.md(1 hunks)docs/get-started/installation.md(1 hunks)docs/get-started/sft.md(1 hunks)docs/guides/dpo.md(1 hunks)docs/guides/grpo.md(1 hunks)docs/guides/rm.md(1 hunks)docs/guides/sft.md(1 hunks)docs/index.md(1 hunks)docs/local-workstation.md(0 hunks)
💤 Files with no reviewable changes (5)
- docs/local-workstation.md
- docs/about/installation.md
- docs/about/quick-start.md
- docs/about/clusters.md
- docs/cluster.md
🧰 Additional context used
📓 Path-based instructions (2)
docs/**/*.md
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Update docs/index.md when a new markdown doc is added under docs/**/*.md or a markdown file is renamed, ensuring the document appears in the most appropriate section
Files:
docs/guides/dpo.mddocs/guides/grpo.mddocs/debugging.mddocs/get-started/installation.mddocs/get-started/cluster.mddocs/get-started/dpo.mddocs/get-started/grpo.mddocs/guides/rm.mddocs/guides/sft.mddocs/get-started/sft.mddocs/index.mddocs/get-started/index.md
!(**/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:
docs/guides/dpo.mddocs/guides/grpo.mddocs/debugging.mddocs/get-started/installation.mddocs/get-started/cluster.mddocs/get-started/dpo.mddocs/get-started/grpo.mddocs/guides/rm.mddocs/guides/sft.mddocs/get-started/sft.mddocs/index.mddocs/get-started/index.md
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA-NeMo/RL PR: 0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-11-24T17:24:41.976Z
Learning: Applies to docs/**/*.md : Update docs/index.md when a new markdown doc is added under docs/**/*.md or a markdown file is renamed, ensuring the document appears in the most appropriate section
📚 Learning: 2025-09-18T14:57:31.003Z
Learnt from: zpqiu
Repo: NVIDIA-NeMo/RL PR: 1006
File: nemo_rl/algorithms/distillation.py:312-354
Timestamp: 2025-09-18T14:57:31.003Z
Learning: The distillation algorithm's cluster setup logic is designed to follow the same patterns used in GRPO for handling distributed training clusters and resource allocation.
Applied to files:
docs/get-started/grpo.mddocs/get-started/index.md
🪛 LanguageTool
docs/get-started/cluster.md
[grammar] ~71-~71: Ensure spelling is correct
Context: ...files, and kill/restart jobs without re-queueing. 1. Submit the Request Ask for the...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~183-~183: Consider a different adjective to strengthen your wording.
Context: ...ation The following variables allow for deeper customization of the Ray cluster. Most ...
(DEEP_PROFOUND)
🪛 markdownlint-cli2 (0.18.1)
docs/get-started/cluster.md
203-203: Table pipe style
Expected: leading_and_trailing; Actual: no_leading_or_trailing; Missing leading pipe
(MD055, table-pipe-style)
203-203: Table pipe style
Expected: leading_and_trailing; Actual: no_leading_or_trailing; Missing trailing pipe
(MD055, table-pipe-style)
203-203: Table column count
Expected: 2; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
docs/get-started/index.md
123-123: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
139-139: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
148-148: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
168-168: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
172-172: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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). (3)
- GitHub Check: Docs_Tests
- GitHub Check: Post submodule check comment / Comment on PR
- GitHub Check: Post automodel integration comment / Comment on PR
🔇 Additional comments (14)
docs/guides/dpo.md (1)
9-9: Consistency check on cluster documentation reference.This file now points to
../get-started/cluster.md, consistent with the restructuring. Ensure this path is correct and the target file exists.docs/guides/rm.md (1)
7-7: Consistency check on cluster documentation reference.This file now points to
../get-started/cluster.md, consistent with the documentation restructuring across other guides. Verify the target file exists at the new location.docs/guides/grpo.md (1)
7-7: Consistency check on cluster documentation reference.This file now points to
../get-started/cluster.md, consistent with the documentation restructuring. Verify the target file exists.docs/get-started/dpo.md (3)
153-153: Relative path to cluster.md is correct.The file
docs/get-started/cluster.mdexists and the reference[Cluster Setup](cluster.md)on line 153 is a valid relative link to the sibling file.
10-10: Verify that downstream documentation files reference this cross-reference target.The MyST syntax
(gs-dpo)=is correctly formatted to create a referenceable target that downstream files can link to using{ref}\gs-dpo``. Confirm that files referencing this target exist and resolve correctly in documentation builds.
1-8: Verify this file is registered in docs/index.md.As per the coding guideline, all new markdown files under
docs/**/*.mdmust be added to the documentation index. Confirm that this file appears in the appropriate section ofdocs/index.md.docs/get-started/sft.md (1)
153-153: The relative path reference is correct. The filedocs/get-started/cluster.mdexists and the link[Cluster Setup](cluster.md)on line 153 resolves properly to the sibling file.docs/debugging.md (1)
12-12: The target sectionget-started/cluster.md#2-submit-a-jobexists at line 53 with the heading "## 2. Submit a Job". The link is correctly configured and points to a valid section describing the job submission workflow.docs/get-started/installation.md (1)
1-8: Ensure this file is registered in docs/index.md.Per the coding guidelines, new markdown files under
docs/**/*.mdmust be added to the documentation index. This file should appear in the appropriate section ofdocs/index.md(likely under "Getting Started" or similar).docs/guides/sft.md (1)
7-7: The link update is correct and the target file exists.The relative path
../get-started/cluster.mdfromdocs/guides/sft.mdcorrectly resolves todocs/get-started/cluster.md, which exists and contains the relevant cluster setup information. The documentation index (docs/index.md) already includes proper references toguides/sft.md, so no additional updates are needed.docs/get-started/grpo.md (1)
1-146: Well-structured GRPO quickstart guide.The content is well-organized with clear progression from prerequisites through data preparation, configuration, execution, monitoring, and scaling. Code examples are properly formatted with appropriate language syntax highlighting. Cross-references to related documentation (cluster setup and advanced GRPO guide) are correct.
docs/index.md (3)
202-213: Get Started section properly registered in documentation index.The new get-started documents are correctly added to the main
docs/index.mdtoctree with appropriate maxdepth and organization. The structure aligns with the coding guideline requirement to update the index when new markdown docs are added underdocs/**/*.md. All six new get-started pages are registered and linked in the Quickstarts grid section.Per coding guidelines, this satisfies the requirement: "Update docs/index.md when a new markdown doc is added under docs/**/*.md".
59-98: Documentation navigation structure is well-organized.The restructured Quickstarts section (lines 59-98) with grid cards clearly guides users to the new get-started pages. The section properly links to:
- Installation & Quickstart (get-started/index.md)
- GRPO Quickstart (get-started/grpo.md)
- SFT Quickstart (get-started/sft.md)
- DPO Quickstart (get-started/dpo.md)
This aligns well with the PR objective to create a dedicated Get Started section with scoped articles.
184-287: All referenced files in the toctree sections exist and are correctly located. The cross-references are valid with no stale or missing file references detected.Likely an incorrect or invalid review comment.
terrykong
left a comment
terrykong
left a comment
There was a problem hiding this comment.
@lbliii can you git mv the source files so that github can show us the diff? it's hard to tell what changes were made
ex:
git mv docs/cluster.md docs/get-started/cluster.md
can you do that to all the markdown files that were moved and then modified? it will make reviewing much easier
lbliii
force-pushed
the
llane/get-started-section
branch
from
abca904 to
a0043f6
Compare
Not sure if I did this correctly, but:
if i need to regenerate the branch/PR in a different way, let me know. |
Signed-off-by: Lawrence Lane <llane@nvidia.com>
Signed-off-by: Lawrence Lane <llane@nvidia.com>
Signed-off-by: Lawrence Lane <llane@nvidia.com>
|
do you think it makes sense to also link the in-depth installation from the index page? |
|
Good to add docker commands as another installation option. |
Signed-off-by: L.B. <llane@nvidia.com>
Signed-off-by: Lawrence Lane <llane@nvidia.com>
|
thanks @lbliii for trying to get github to recognize the move. still looks hard to read. let me try to review offline |
|
@lbliii could you also use github admonitions everywhere? |
Signed-off-by: Lawrence Lane <llane@nvidia.com>
Signed-off-by: Lawrence Lane <llane@nvidia.com>
Signed-off-by: Lawrence Lane <llane@nvidia.com>
Signed-off-by: Lawrence Lane <llane@nvidia.com>
4 participants
This PR creates a dedicated Get Started section that provides scoped articles for:
This is currently an exploration for what your Get Started could look like. I'm looking for feedback.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.