Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds initial Sphinx/Read the Docs scaffolding for hipFile documentation, including pinned Python doc dependencies, a Sphinx conf.py, a JupyterBook-style TOC, and a Doxygen configuration intended to feed API reference pages.
Changes:
- Add Read the Docs config and Sphinx configuration (
.readthedocs.yaml,docs/conf.py). - Add doc build inputs: Doxygen template, external TOC, and pinned Sphinx dependency set (
docs/doxygen/Doxyfile.in,docs/sphinx/*). - Add initial RST content for the docs landing page and API sections (
docs/index.rst,docs/api/*.rst, etc.).
Reviewed changes
Copilot reviewed 8 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/sphinx/requirements.txt | Pinned, compiled Python dependency set for building docs. |
| docs/sphinx/requirements.in | Source input for pip-compile (rocm-docs-core pin). |
| docs/sphinx/_toc.yml.in | External TOC definition for Sphinx (currently committed as .in). |
| docs/index.rst | New docs landing page and navigation grid. |
| docs/hipify.rst | Stub page for hipify docs section. |
| docs/doxygen/Doxyfile.in | Doxygen configuration template intended for API XML generation. |
| docs/conf.py | Sphinx configuration using rocm_docs helpers and Doxygen invocation. |
| docs/api/async.rst | Async API page (currently notes unsupported). |
| docs/api/batch.rst | Batch API page (currently notes unsupported). |
| docs/api/core.rst | Core API and versioning documentation. |
| docs/api/driver.rst | Driver API documentation. |
| docs/api/errors.rst | Error handling documentation. |
| docs/api/file.rst | File handle/buffer/IO operations documentation. |
| docs/api/misc_api.rst | Placeholder “Misc Notes” page. |
| docs/api/rdma.rst | RDMA page (currently notes unsupported). |
| cmake/AISDocumentation.cmake | Update Doxygen template path for CMake-driven doc builds. |
| .readthedocs.yaml | Read the Docs build configuration pointing at Sphinx config + requirements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
30bc4d3 to
00e586d
Compare
Also adds a blank hipify.rst file
Avoids dirtying the source directory
0212ad6 to
c6cbb4b
Compare
There was a problem hiding this comment.
Pull request overview
Adds an initial Sphinx-based documentation build (integrated with ROCm’s rocm-docs-core) and wires it into CI/container images, alongside a new set of .rst pages for the hipFile docs.
Changes:
- Install Python + Sphinx toolchain (Sphinx/Breathe/rocm-docs-core) into the CI Docker images and activate the venv during the GitHub Actions build.
- Add Sphinx configuration + TOC and introduce/expand multiple documentation pages (index, API reference sections, etc.).
- Update the docs CMake flow to run Doxygen then Sphinx, and add a Read the Docs config.
Reviewed changes
Copilot reviewed 13 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| util/docker/DOCKERFILE.ais_ci_ubuntu | Installs Python + creates venv with Sphinx-related packages for CI builds. |
| util/docker/DOCKERFILE.ais_ci_suse | Same as above for SUSE; uses Python 3.12 and upgrades pip. |
| util/docker/DOCKERFILE.ais_ci_rocky | Same as above for Rocky Linux. |
| .github/workflows/build-ais.yml | Activates the venv so CMake can find Python deps while building docs in CI. |
| .github/workflows/pylint.yml | Installs rocm-docs-core so pylint can analyze Sphinx config importing it. |
| cmake/AISDocumentation.cmake | Adds Sphinx build steps (Doxygen XML + Sphinx HTML) and checks for rocm_docs. |
| docs/doxygen/Doxyfile.in | Redirects Doxygen output under the build docs directory. |
| docs/sphinx/conf.py | Adds Sphinx config using rocm_docs + Doxygen environment wiring. |
| docs/sphinx/_toc.yml.in | Adds ROCm docs-style TOC definition for the Sphinx site structure. |
| .readthedocs.yaml | Introduces RTD config intending to build the docs on Read the Docs. |
| docs/index.rst | Adds a new documentation landing page using sphinx-design grids. |
| docs/stats_collection.rst | Adjusts formatting/content for the stats collection tool page. |
| docs/hipify.rst | Adds a stub page for hipify docs. |
| docs/api/errors.rst | Adds error-handling documentation. |
| docs/api/core.rst | Adds core/API versioning documentation. |
| docs/api/driver.rst | Adds driver API documentation. |
| docs/api/file.rst | Adds file handle/buffer/I/O documentation. |
| docs/api/async.rst | Adds async API status page (currently unsupported). |
| docs/api/batch.rst | Adds batch API status page (currently unsupported). |
| docs/api/rdma.rst | Adds RDMA API status page (currently unsupported). |
| docs/api/misc_api.rst | Adds miscellaneous notes page. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| version: 2 | ||
|
|
||
| sphinx: | ||
| configuration: docs/conf.py |
There was a problem hiding this comment.
Read the Docs is configured to use docs/conf.py, but this repo only contains docs/sphinx/conf.py (and the CMake build copies that into the build tree). As-is, RTD builds will fail to start because the configured Sphinx conf.py path does not exist. Update the RTD config to point at the actual Sphinx config file (or add a docs/conf.py shim that delegates to docs/sphinx/conf.py).
| configuration: docs/conf.py | |
| configuration: docs/sphinx/conf.py |
|
|
||
| python: | ||
| install: | ||
| - requirements: docs/sphinx/requirements.txt |
There was a problem hiding this comment.
.readthedocs.yaml installs from docs/sphinx/requirements.txt, but that file isn't present in the repository. RTD builds will fail during dependency installation unless you add this requirements file or change the python.install section to install the needed packages another way.
| - requirements: docs/sphinx/requirements.txt | |
| - method: pip | |
| path: . |
| release = version_number | ||
| external_projects_current_project = "rocshmem" | ||
| extensions = ["sphinx_design"] | ||
| external_toc_path = "./sphinx/_toc.yml" |
There was a problem hiding this comment.
external_toc_path points to ./sphinx/_toc.yml, but the repo only adds _toc.yml.in (and the CMake build also copies _toc.yml.in). Unless something generates _toc.yml at build time in the expected location, Sphinx/rocm_docs won't be able to find the TOC. Make external_toc_path reference the file that actually exists (or ensure _toc.yml is generated/copied before Sphinx runs).
| external_toc_path = "./sphinx/_toc.yml" | |
| external_toc_path = "./sphinx/_toc.yml.in" |
| left_nav_title = f"hipFile {version_number} documentation" | ||
|
|
||
| # for PDF output on Read the Docs | ||
| project = "rocSHMEM" |
There was a problem hiding this comment.
Maybe add a comment why hipFile is masquerading as rocSHMEM.
riley-dixon
left a comment
There was a problem hiding this comment.
Feel free to auto-resolve these comments.
| python -m pip install --upgrade pip | ||
| python -m pip install pylint | ||
| python -m pip install black | ||
| python -m pip install rocm-docs-core |
There was a problem hiding this comment.
Does the pylint workflow still need this package?
| sphinx \ | ||
| breathe \ | ||
| sphinx-rtd-theme \ | ||
| rocm-docs-core |
There was a problem hiding this comment.
You may wish to consider listing these in a requirements.txt file if they are going to be shared across CI images.
That file could exist in the docker directory as rocm-docs-requirements.txt and then used via pip3 install -r rocm-docs-requirements.txt. You would just need to COPY the file into the container first.
| :keywords: hipFile, ROCm, storage, library, API, DMA | ||
|
|
||
| ********************* | ||
| hipFile documentation |
There was a problem hiding this comment.
Capitalize Documentation?
| @@ -0,0 +1,43 @@ | |||
| .. meta:: | |||
| :description: hipFile does hipFile stuff | |||
There was a problem hiding this comment.
Was this going to be replaced later?
| # pylint: enable=redefined-builtin | ||
| version = version_number | ||
| release = version_number | ||
| external_projects_current_project = "rocshmem" |
There was a problem hiding this comment.
Same comment as https://github.com/ROCm/hipFile/pull/224/changes#r2996045281 in case this was not intentional. Feel free to auto-resolve.
WIP
AIHIPFILE-41