Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughA comprehensive documentation infrastructure is established for the RAPIDS CLI project, including Sphinx configuration updates, API reference documentation for core modules, user guides, developer plugin documentation, troubleshooting guides, and documentation build dependencies. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/source/api/debug.rst`:
- Line 10: Replace the bare reference to run_debug with a fully qualified Sphinx
cross-reference using the module path used in the automodule
(rapids_cli.debug.debug); update the text to use the form
~rapids_cli.debug.debug.run_debug so Sphinx resolves with the full path but
displays only the short name `run_debug`, ensuring consistency with ..
automodule:: rapids_cli.debug.debug.
In `@docs/source/api/doctor.rst`:
- Line 12: The short-form cross-reference :class:`CheckResult` may not resolve —
replace it with the fully-qualified Sphinx reference that includes the module
path (e.g. :class:`your_package.module.CheckResult`) or use the same resolution
pattern applied for :func:`run_debug` in debug.rst; ensure the module path
matches the actual import location of CheckResult or add a suitable ..
currentmodule:: directive so the class reference resolves correctly.
In `@rapids_cli/doctor/doctor.py`:
- Around line 42-43: Remove the incorrect "Raises: ValueError" section from the
docstring of the doctor_check function; doctor_check catches all exceptions (via
the except Exception as e: path) and records them in the CheckResult object
instead of propagating, so update the docstring in doctor_check to reflect that
it returns a bool/CheckResult rather than raising, and do not add any new raise
statements—leave exception handling to the individual check modules (e.g.,
gpu.py, nvlink.py) which may raise internally.
🧹 Nitpick comments (1)
docs/source/conf.py (1)
74-81: External resource loading looks intentional for RAPIDS branding; consider pinning Font Awesome with SRI.The
setup()hook loads CSS/JS fromdocs.rapids.ai(first-party) and Font Awesome 5.9.0 fromcdnjs. This is standard for org-wide docs branding.Two optional notes:
- Font Awesome 5.9.0 is quite dated (FA 6.x has been available for years). If the RAPIDS custom CSS depends on FA 5.x class names, this is fine — otherwise consider updating.
- For the cdnjs resource, you could add a Subresource Integrity hash if Sphinx's
add_css_filesupports anintegrityattribute, to guard against CDN tampering.Neither is blocking.
jacobtomlinson
left a comment
There was a problem hiding this comment.
Thanks for this!
I left some comments. I tried to write them in a way that should be actionable for an agent.
… page - Fix Sphinx cross-references for run_debug and CheckResult to use fully qualified module paths - Add sphinx-llm extension to generate llms.txt for LLM consumption - Add sphinx-autobuild with `make serve` target for live-reload dev - Rewrite doctor.rst with user-facing content explaining cross-layer compatibility checks and library-bundled health checks - Move check execution flow details to plugin_development.rst - Fix incorrect Raises section in doctor_check docstring Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Mike McCarty <mmccarty@nvidia.com>
|
Thanks @jacobtomlinson Just a note, CodeRabbit give Agent prompts that start with "@rapids_cli/" around lines x - y" to help the Agent fine the specific location of the feedback. Even less typing for the humans! |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Mike McCarty <mmccarty@nvidia.com>
jameslamb
left a comment
There was a problem hiding this comment.
Left some minor comments, giving this a packaging-codeowners approval so you can do what you want with them and not need another review from me.
Will defer to Jacob and Naty on the actual content of the docs.
| The function discovers and loads check functions defined in entry points | ||
| under the 'rapids_doctor_check' group. It also checks specific | ||
| configurations related to a corresponding subcommand if given. | ||
| Args: |
There was a problem hiding this comment.
For my education... what's the purpose of changing this from numpydoc format to this format? Does it change how the docs are rendered in the API reference?
There was a problem hiding this comment.
Claude says...
● The project's ruff configuration enforces Google-style docstrings via the pydocstyle convention. Let me show the relevant config:
● Searched for 1 pattern (ctrl+o to expand)
● The ruff linter is configured with convention = "google" at pyproject.toml:131. The D rule set (pydocstyle) enforces this, so any NumPy-style
docstrings (which use section headers like Parameters\n----------) would fail ruff checks. Google-style uses Args:, Returns:, Raises: with indented
descriptions instead.
The doctor_check() docstring was the one that still had a NumPy-style Raises section that we corrected (and replaced with Returns since the function
doesn't actually raise).
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Verify that cuml_check is not called when filtering for cudf checks only. Also wrap long docstring line in doctor_check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Mike McCarty <mmccarty@nvidia.com>
Summary
doctor.rst,cuda_driver.rst) and restructureindex.rstwith proper toctree sectionsdoctor_check()docstring from NumPy-style to Google-style (matches ruff pydocstyle convention)dependencies.yamlrun_debugandCheckResultto use fully qualified module pathssphinx-llmextension to generatellms.txtfor LLM consumptionsphinx-autobuildwithmake servetarget for live-reload docs developmentRaisessection indoctor_checkdocstring (replace withReturns)Note: This branch also includes the commits from #135 (unit tests and coverage). The docs-specific changes are in commits after
2137644.Test plan
make -C docs clean htmlbuilds with 0 warningspre-commit run --all-filespassesllms.txtandllms-full.txtgenerated in build outputmake servestarts live-reload server🤖 Generated with Claude Code