Add Jupyter Book documentation infrastructure with auto-generated API reference via sphinx-autoapi - #272
Add Jupyter Book documentation infrastructure with auto-generated API reference via sphinx-autoapi#272Mohit-Lakra wants to merge 20 commits into
Conversation
|
@joeloskarsson @leifdenby |
|
@Mohit-Lakra I just had a look at your autogenerated docs you are hosting. It looks great! This is certainly a solid technical framework to solve this issue. There is however another PR already working on this #252 with similar albeit not identical dependencies and methods. Do you think you could coordinate with @varma1221 directly from that other PR and consolidate towards one common solution? |
|
@sadamov, I've already looked at @varma1221's PR; in fact, his ReadTheDocs setup is what I'm missing. @leifdenby had enquired about per-PR preview URLs, and that configuration takes care of it |
|
@Mohit-Lakra, I will open a PR against your branch with the |
|
@varma1221 Sounds fantastic, appreciate it 👍 |
|
I tried building this branch locally and found that train_model.py's main entry point has only a one-line docstring despite having 20+ CLI arguments. Would it make sense to add docstring coverage for this file as part of this PR, or handle it separately? I also noticed the models/ inheritance chain means gaps in ar_model.py propagate to all subclasses — happy to help audit these. |
Please check #252 for docstrings |
|
Hello @Mohit-Lakra and @varma1221, I have been looking into the documentation with this PR and found a small UI bug with the collapsible sidebar when scrolling down. The sidebar when scrolling down is cut in half and not extended till the end of the page. If okay, shall I work on solving this as an issue and open a PR for the same? |
Thanks for looking, Current PR is only for API reference docs |
|
Hi @Mohit-Lakra, I tried merging your
Should be straightforward to resolve, most likely a Also confirmed locally that |
|
@sadamov,I'd appreciate your feedback on the current state of the PR. I'm happy to make any fixes, improvements, or adjustments you think are needed, and I'm also open to adding or modifying features if that would help move it forward. |
sadamov
left a comment
There was a problem hiding this comment.
Thanks @Mohit-Lakra, the autoapi engine is solid and the hosted prototype looks good. Before a line-level pass I want to settle a few direction questions:
-
Scope. The autoapi reference is the strong half and is what #61 asked for. The hand-written prose (architecture/, guides/, about/) was written against pre-refactor code and is already wrong on
main- e.g.ARModelno longer exists (it isForecasterModule+ARForecaster), andguides/configuration.mddocuments a config schema that does not exist (those keys aretrain_modelCLI flags, not config fields). You said earlier "current PR is only for API reference docs" - I would hold to that and scope this down to the autoapi infra plus minimal intro/install, dropping the prose. -
Tooling. Jupyter Book 1.x is now in maintenance mode (the team has moved to JB2 / MyST-MD), so we would be adopting a frozen wrapper, and it is the source of the astroid monkeypatch. Since no sibling mllam repo has docs yet, whatever we pick becomes the org convention. I propose to keep the engine but drop the JB1 wrapper: Sphinx +
myst-nb+sphinx-autodoc2+ a maintained theme, on ReadTheDocs. JB2/mystmd has no native API autodoc yet so it cannot close #61 alone. What do you think? -
Publish target. The PR ships both GitHub Pages and
.readthedocs.yaml. #61 proposed RTD + Sphinx from the start (modelled on the PyG docs), so I read RTD as the agreed host and the Pagesdeploy-docsjob as incidental to your fork prototype. Could we drop the Pages job and keep just RTD? -
CI cannot currently fail on a broken build (
--keep-going, no artifact check,linkcheck ... || true), so a green run is not evidence the docs built. I would fix that regardless of the above.
Once we agree on scope and tooling I will do a detailed line-level pass.
|
@sadamov thanks for the feedback, I completely agree with your thinking regarding both the scope and the tooling. I dropped all the outdated handwritten docs and strictly uses the autoapi refrence docs and a minimal intro only I replaced Jupyter Book with sphinx + sphinx-autoapi + myst-nb + sphinx-book-theme I also entirely removed the GitHub Pages deployment step from the CI workflow; we will exclusively stick to ReadTheDocs moving forward. I removed unused steps from the CI like pyreverse UML generation, since its host page was dropped and cleaned up pyproject.toml accordingly |
There was a problem hiding this comment.
Thanks @Mohit-Lakra, this settles all four points from last round.
Two blockers on our own rules, the rest are inline:
-
No CHANGELOG entry for this PR (the #252 line only mentions #272 in passing).
-
This deletes
docs/notebooks/create_reduced_meps_dataset.ipynb, the only file indocs/notebooks/, but README.md:535 still points there and the notebook documents howmeps_example_reducedwas built. I would restore it (out of scope to drop).
What do you think?
| [tool.setuptools] | ||
| py-modules = ["neural_lam"] | ||
|
|
There was a problem hiding this comment.
Backend is hatchling (line 183), so [tool.setuptools] is never read, and py-modules is the wrong concept for a package. Drop it.
| [tool.setuptools] | |
| py-modules = ["neural_lam"] |
| } | ||
|
|
||
| # OpenGraph settings | ||
| ogp_site_url = "https://mllam.github.io/neural-lam/" |
There was a problem hiding this comment.
ogp_site_url still points at the Pages prototype. Update to the RTD URL (placeholder below, set once the slug is known).
| ogp_site_url = "https://mllam.github.io/neural-lam/" | |
| ogp_site_url = "https://neural-lam.readthedocs.io/en/latest/" |
| # HTML Theme | ||
| html_theme = "sphinx_book_theme" | ||
| html_logo = "_static/logo.png" | ||
| html_favicon = "_static/favicon.ico" |
There was a problem hiding this comment.
favicon.ico is 0 bytes, so this ships a broken favicon. Commit a real icon or drop the line.
| html_favicon = "_static/favicon.ico" |
| # Run a minimal test to trigger data download | ||
| pytest tests/test_training.py -vv -s -k "test_training" --co |
There was a problem hiding this comment.
--co is collect-only, it never runs the fixture, so it won't download anything. Drop it.
| # Run a minimal test to trigger data download | |
| pytest tests/test_training.py -vv -s -k "test_training" --co | |
| # Run a minimal test to trigger the data download | |
| pytest tests/test_training.py -vv -s -k "test_training" |
| @@ -0,0 +1,133 @@ | |||
| classDiagram | |||
There was a problem hiding this comment.
Nothing references these anymore (grep -rn _static/uml docs/ is empty) and they will drift from the code. I suggest to remove docs/_static/uml/.
| @@ -0,0 +1,260 @@ | |||
| /* Neural-LAM Documentation — High-Visibility Custom Styles */ | |||
There was a problem hiding this comment.
260 lines of bespoke design-system overrides plus a remote Google Fonts @import (line 45) on a theme we agreed to track. I suggest to trim to what you need or drop it for the sphinx_book_theme defaults; remove the remote font @import regardless.
| tools: | ||
| python: "3.10" | ||
| commands: | ||
| - pip install uv | ||
| - uv pip install --system -e ".[cpu,docs]" |
There was a problem hiding this comment.
RTD can't be CI-tested yet, two things to verify: uv pip install (vs uv sync in CI) does not honour [tool.uv.sources], so RTD may pull the full CUDA torch and hit its time/disk limits; and python: "3.10" here vs 3.11 in docs.yml - align them.
- Update OpenGraph URL to ReadTheDocs and drop empty favicon - Remove --co flag from quickstart test command - Drop unused UML diagrams and custom CSS - Restore dataset notebook and add to toctree - Align RTD config with CI python version and uv sync - Add CHANGELOG entry for docs migration
|
Thanks @sadamov,
|
sadamov
left a comment
There was a problem hiding this comment.
Okay I did a full thorough review now, including inspection of the rendered docs end-to-end.
A few structural questions:
- Installation duplicates the README, and
installation.mdhas already drifted from it: its pip install block uses a.[dev]extra and a cu118 wheel that are both wrong here but correct in the README. Could we make it thin - the "build docs locally" block plus a MyST{include}of the README install section - so there is one source? - Tutorials: I would group all three example notebooks under one Tutorials section in
index.md, kept indocs/notebooks/withnb_execution_mode = "off"and committed outputs (they need data and GPU, so CI/RTD cannot run them): the existingcreate_reduced_meps_dataset.ipynb(please reset it tomainfirst, the current diff is 234/234 lines of pure metadata churn, and it also sits oddly as a top-level peer today), plus the upcoming DANRA (#577) and COSMO (#392) notebooks. Agree the skeleton now and each is a one-line toctree add?
|
@Mohit-Lakra I really like these docs! First of all the autodoc renders great, thanks to #252 🤩 |
|
Hi @sadamov, thanks for the compliment ✨. I also realised the same these changes will help make the docs faster and better to manage. |
sadamov
left a comment
There was a problem hiding this comment.
Thanks @Mohit-Lakra, this is basically ready. I rebuilt the site from the head with sphinx-build -W --keep-going (html and linkcheck) in a fresh env with no torch and both pass 👍 Two small things below, could you implement them? Then I would love for you to showcase the docs at the next dev meeting.
Co-authored-by: sadamov <45732287+sadamov@users.noreply.github.com>
Co-authored-by: sadamov <45732287+sadamov@users.noreply.github.com>
|
Thanks @sadamov I have implemented the two requested changes. Please let me know if there's anything else that needs to be adjusted. Regarding the dev meeting, I'd be happy to showcase the docs if my schedule allows, but I can't guarantee that I'll be able to attend. |
|
@Mohit-Lakra no worries if you're not there I can show the community |
|
@Mohit-Lakra for the showcase tomorrow I had to merge main, which added a new docs file about graph specs. I mvoed that one and the existing API into a new "References" Section. |
Describe your changes
Adds a Jupyter Book-based documentation site with a fully automated API reference
generated by
sphinx-autoapi. This means every module, class, and function inneural_lam/gets a structured reference page built directly from source code —with zero manually maintained
.rstfiles.Files added:
docs/_config.yml— Jupyter Book config withsphinx-autoapipointed at../neural_lam/docs/_toc.yml— site structure (Tutorials / How-To / API Reference /Background / Contributing)
docs/intro.md+docs/installation.md— landing page and install guide.github/workflows/deploy-docs.yml— builds docs on every PR, deploys toGitHub Pages on merge to
maindocs/scripts/autoapi_astroid_patch.py— patch to ensure sphinx-autoapiworks correctly with the project's dependency tree
Dependencies (docs group only, not required for model usage):
jupyter-book,sphinx-autoapi,myst-nb,sphinx-copybutton,pydata-sphinx-themeInstall with:
Build locally with:
Live prototype: https://mohit-lakra.github.io/neural-lam/autoapi/index.html (intro page: https://mohit-lakra.github.io/neural-lam)
Issue Link
closes #61
Type of change
Checklist before requesting a review
Checklist for reviewers
Author checklist after completed review
sphinx-autoapiforauto-generated API reference and GitHub Actions deploy workflow (relates
to Structured documentation for Neural-LAM #61)
Checklist for assignee