Skip to content

Conversation

@chtruong814
Copy link
Contributor

@chtruong814 chtruong814 commented Oct 10, 2025

beep boop [🤖]: Hi @terrykong 👋,

we've cherry picked #1224 into  for you! 🚀

Please review and approve this cherry pick by your convenience!

Summary by CodeRabbit

  • Documentation

    • Standardized TIP/NOTE/IMPORTANT admonitions across docs using blockquote-style Markdown for consistent presentation.
    • Minor spacing and formatting adjustments; no changes to instructional content or commands.
    • Updates applied to cluster, testing, design, and profiling guides.
  • Refactor

    • Improved documentation build to automatically convert admonitions in both included and top-level sources, ensuring consistent rendering.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 10, 2025

📝 Walkthrough

Walkthrough

Documentation files replace custom/old admonition syntax with blockquote-style Markdown admonitions. Sphinx config refactors GitHub-style admonition conversion into an in-place converter with wrapper hooks registered for both include-read and source-read events, adjusting fence handling and spacing. No content semantics or executable commands changed.

Changes

Cohort / File(s) Summary
Sphinx admonition converter refactor
docs/conf.py
Replaced previous conversion function with _convert_gh_admonitions_inplace(contents) and thin wrappers for include-read and source-read events; updated fence-based replacement logic and spacing between sequential admonitions; wired both Sphinx events to run conversion.
Docs admonition formatting updates
docs/cluster.md, docs/design-docs/logger.md, docs/nsys-profiling.md, docs/testing.md
Converted :::tip/:::note/:::important and similar blocks to blockquote Markdown admonitions (> [!TIP]/[!NOTE]/[!IMPORTANT]); minor whitespace/reflow; no content changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Dev as Sphinx
  participant IR as include-read event
  participant SR as source-read event
  participant W1 as _convert_gh_admonitions(...)
  participant W2 as _convert_gh_admonitions_source(...)
  participant C as _convert_gh_admonitions_inplace(contents)

  rect rgba(230,240,255,0.6)
    note over Dev: Build starts
    Dev->>IR: Emit include-read(app, relative_path, parent_docname, contents)
    IR->>W1: Call wrapper with contents
    W1->>C: Convert in-place
    C-->>W1: contents mutated
    W1-->>IR: return
  end

  rect rgba(230,255,230,0.6)
    Dev->>SR: Emit source-read(app, docname, source)
    SR->>W2: Call wrapper with source
    W2->>C: Convert in-place
    C-->>W2: source mutated
    W2-->>SR: return
  end

  note over C: Replace GitHub admonitions with MyST<br/>using fenced blocks and spacing rules
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested labels

documentation, r0.4.0

Suggested reviewers

  • terrykong

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title primarily describes the cherry-pick action and includes backticks, a commit number, and a branch name rather than succinctly summarizing the substantive fix to GitHub-to-MyST parser admonition conversion. This makes it noisy and less immediately clear to teammates scanning pull request history. It does not follow best practices for concise, descriptive titles. Rename the title to a clear, single sentence focused on the actual change, for example “Fix GitHub-to-MyST parser admonition conversion” (optionally noting “cherry-pick into r0.4.0” after the main description).
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Test Results For Major Changes ✅ Passed The cherry-picked PR only updates documentation formatting and adjusts an internal helper in docs/conf.py that converts admonition syntax for Sphinx, with no product behavior, numerics, or performance impact. These are minor documentation-oriented changes, and the PR description, while lacking explicit test results, is acceptable under the check’s rule that minor changes do not require testing evidence.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch cherry-pick-1224-r0.4.0

📜 Recent 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 2019c53 and d658fa5.

📒 Files selected for processing (5)
  • docs/cluster.md (4 hunks)
  • docs/conf.py (3 hunks)
  • docs/design-docs/logger.md (1 hunks)
  • docs/nsys-profiling.md (1 hunks)
  • docs/testing.md (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
docs/**/*.md

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

When a markdown doc under docs/**/*.md is added or renamed, update docs/index.md to include it in the appropriate section

Files:

  • docs/design-docs/logger.md
  • docs/cluster.md
  • docs/nsys-profiling.md
  • docs/testing.md
**/*.py

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

**/*.py: Follow the Google Python Style Guide for all Python code
Target Python 3.12+ for all Python code in NeMo-RL
Indent Python code with 4 spaces; do not use tabs
Python filenames should be snake_case (e.g., some_file.py)
Class names should be PascalCase
Function and method names should be snake_case
Local variable names should be snake_case; if starting with a number, prefix with k (e.g., k_99th_percentile)
Global variables should be UPPER_SNAKE_CASE and prefixed with G_ (e.g., G_MY_GLOBAL)
Constants should be UPPER_SNAKE_CASE
Avoid shadowing variables declared in an outer scope
Initialize all externally visible members of a class in the constructor
For public interfaces used outside a file, prefer docstrings over comments
Use comments mainly for code within a function or interfaces local to a file
Commented-out code must include a nearby comment explaining usage and why it is commented out; otherwise remove before merging
Use Google-style docstrings for classes and functions (Sphinx-parseable)
Avoid using reflection when functionality can be easily achieved without it
Limit except clauses to the smallest specific set of exceptions possible
For duck-typing via try/except, keep the try body minimal and use else for main logic
Add the NVIDIA copyright header (with current year) at the top of all Python files, excluding tests/ and test-only scripts

Files:

  • docs/conf.py
⏰ 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). (7)
  • GitHub Check: Docs_Tests
  • GitHub Check: sphinx-build / Build docs
  • GitHub Check: Lint check
  • GitHub Check: Lint check
  • GitHub Check: Lint check
  • GitHub Check: Post automodel integration comment / Comment on PR
  • GitHub Check: Post submodule check comment / Comment on PR
🔇 Additional comments (8)
docs/conf.py (3)

119-165: LGTM! Admonition conversion logic is well-structured.

The in-place converter correctly handles:

  • 8-backtick fences to nest code blocks with 3 or 6 backticks
  • Multiple adjacent admonitions with proper blank line separation
  • Blockquote prefix removal from content lines
  • Final fence closing if still in replacement mode

The for...else construct at line 151 is correct Python (executes when the inner loop completes without breaking), though it may initially appear unusual to reviewers unfamiliar with this pattern.


168-178: LGTM! Wrapper functions correctly delegate to the in-place converter.

The separate wrappers for include-read and source-read events properly handle different parameter signatures while delegating the core conversion logic to _convert_gh_admonitions_inplace. The underscore-prefixed parameters appropriately signal unused event handler arguments.


254-256: LGTM! Event hooks correctly wired for both included files and top-level sources.

The dual event registration ensures GitHub-style admonitions are converted consistently across both included files (via include-read) and primary source documents (via source-read).

docs/design-docs/logger.md (1)

174-182: LGTM! Admonition formatting updated to blockquote style.

The conversion from :::note to > [!NOTE] aligns with the standardized admonition format defined in docs/conf.py. Content remains unchanged.

docs/nsys-profiling.md (1)

66-67: LGTM! Admonition formatting updated to blockquote style.

The conversion to > [!IMPORTANT] follows the standardized format. Content is preserved.

docs/cluster.md (1)

28-29: LGTM! All admonition formatting updated to blockquote style.

Multiple admonitions converted from :::tip/:::note to > [!TIP]/> [!NOTE] format. All changes are consistent with the documentation-wide standardization effort and preserve the original content.

Also applies to: 42-43, 112-118, 170-172

docs/testing.md (2)

132-144: LGTM! Tip admonition formatting updated to blockquote style.

The conversion from :::{tip} to > [!TIP] aligns with the standardized format. Content is unchanged.


148-149: LGTM! Important admonition formatting updated to blockquote style.

The conversion from :::{important} to > [!IMPORTANT] follows the standardized format. Content is preserved.


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.

@terrykong terrykong added the CI:docs Run doctest label Oct 10, 2025
@terrykong terrykong enabled auto-merge (squash) October 10, 2025 06:55
@terrykong terrykong merged commit 9689bca into r0.4.0 Oct 10, 2025
70 checks passed
@terrykong terrykong deleted the cherry-pick-1224-r0.4.0 branch October 10, 2025 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-pick CI:docs Run doctest documentation Improvements or additions to documentation Run CICD

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants