Skip to content

Conversation

@terrykong
Copy link
Contributor

@terrykong terrykong commented Sep 29, 2025

After #1070 , the admonition rewrite from github to the myst-parser format wasn't working correctly. This PR fixes.

  • The converter was only hooked to Sphinx’s include-read event, which fires for files pulled in via .. include::. Top-level .md pages like docs/testing.md don’t trigger that event, so the GitHub-style admonitions (> [!IMPORTANT], > [!TIP]) were never rewritten to MyST blocks.
  • By also connecting to the source-read event, Sphinx now hands us the raw source for every document (including top-level .md). Our callback mutates the text in-place to {note}/{tip} etc., so MyST can render them correctly.
  • The EOF-close tweak ensures we add a closing fence if an admonition runs to the end of a file, preventing malformed blocks.

Currently

image image

After PR

image image

Summary by CodeRabbit

  • Documentation

    • GitHub-style admonitions are now automatically converted to MyST across all documentation, including top‑level pages and included files, ensuring consistent rendering and styling.
    • No authoring changes required—existing GitHub admonitions will render correctly.
  • Bug Fixes

    • Fixed edge cases where code blocks could remain unclosed after admonition conversion, preventing broken formatting in rendered pages.
    • Improved robustness of the conversion process to cover more documents reliably.

@terrykong terrykong requested a review from a team as a code owner September 29, 2025 05:17
@terrykong terrykong requested review from wangshangsam and removed request for a team September 29, 2025 05:17
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Sep 29, 2025
Signed-off-by: Terry Kong <[email protected]>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 29, 2025

📝 Walkthrough

Walkthrough

Adds an in-place Markdown transformer for GitHub-style admonitions to MyST, introduces wrapper functions for Sphinx event hooks, and wires the transformation into both include-read and source-read events. Adjusts control flow to properly close code fences when replacements span multiple lines.

Changes

Cohort / File(s) Summary of edits
Sphinx admonition conversion
docs/conf.py
- Added _convert_gh_admonitions_inplace(contents: list[str]) to mutate Markdown lines in place
- Replaced prior _convert_gh_admonitions(...) with a delegating wrapper matching include-read signature
- Added _convert_gh_admonitions_source(_app, _docname, source) for source-read
- Connected both include-read and source-read events to run conversion
- Tweaked control flow to ensure closing code fences at segment end

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant S 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 T as _convert_gh_admonitions_inplace(...)

  rect rgb(245,245,255)
    Note over S: New flow
    S->>IR: include-read(app, relative_path, parent_docname, contents)
    IR->>W1: call
    W1->>T: transform(contents)
    T-->>W1: contents mutated
    W1-->>IR: return
  end

  rect rgb(245,255,245)
    S->>SR: source-read(app, docname, source)
    SR->>W2: call
    W2->>T: transform(source)
    T-->>W2: source mutated
    W2-->>SR: return
  end
Loading
sequenceDiagram
  autonumber
  participant S as Sphinx
  participant IR as include-read event
  participant Old as _convert_gh_admonitions(...)

  Note over S,Old: Prior flow (before change)
  S->>IR: include-read(app, relative_path, parent_docname, contents)
  IR->>Old: convert(contents)
  Old-->>IR: contents mutated
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
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 (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly states that the pull request fixes the conversion of GitHub-style admonitions to MyST parser format, directly reflecting the main change in the changeset. It is concise, specific, and free of unrelated details. The phrasing accurately communicates the PR’s purpose to anyone scanning the project history.
Test Results For Major Changes ✅ Passed The PR only adjusts documentation preprocessing logic in docs/conf.py by adding in-place admonition conversion helpers and connecting them to Sphinx events, which is a localized documentation tooling fix rather than a major feature, behavioral, numerical, or performance change, so test evidence in the description is not required for this check.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch tk/fix-admonition

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: 1

📜 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 5166d74 and b06511e.

📒 Files selected for processing (1)
  • docs/conf.py (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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). (4)
  • GitHub Check: Docs_Tests
  • GitHub Check: Lint check
  • GitHub Check: Post submodule check comment / Comment on PR
  • GitHub Check: Post automodel integration comment / Comment on PR

wangshangsam
wangshangsam previously approved these changes Sep 29, 2025
Copy link
Contributor

@wangshangsam wangshangsam left a comment

Choose a reason for hiding this comment

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

Other than the comment CodeRabbit gave, LGTM

Signed-off-by: Terry Kong <[email protected]>
@terrykong
Copy link
Contributor Author

FYI @aschilling-nv

Copy link
Contributor

@wangshangsam wangshangsam left a comment

Choose a reason for hiding this comment

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

LGTM

@terrykong terrykong added r0.4.0 and removed CI:docs Run doctest labels Oct 10, 2025
@terrykong terrykong added the CI:docs Run doctest label Oct 10, 2025
@terrykong terrykong enabled auto-merge (squash) October 10, 2025 05:17
@terrykong terrykong merged commit 7c574d0 into main Oct 10, 2025
54 checks passed
@terrykong terrykong deleted the tk/fix-admonition branch October 10, 2025 05:24
chtruong814 pushed a commit that referenced this pull request Oct 10, 2025
PrinsYin pushed a commit to PrinsYin/RL that referenced this pull request Nov 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:docs Run doctest documentation Improvements or additions to documentation r0.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants