Repository navigation
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 22 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 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. |
|
Heads-up: we discussed the notebook format in the dev meeting and ran a poll in Slack afterwards, marimo won over Jupyter. Going forward notebooks in this repo are marimo For this PR that means: convert the notebook with Sorry for the churn, I know this lands mid-review. Let me know if smth is unclear or if you would rather I push the conversion to your branch. This one needs a bit more thought since jupyter-book / myst-nb render .ipynb directly. marimo can export (marimo export html / marimo export md) so I would add an export step to the docs build and point the toc at the exported files, which would also let us drop myst-nb, jupyter-client and jupyter-core from the deps. What do you think? |
|
Yeah, that makes sense to me. I’m happy to switch the notebook to marimo and drop the .ipynb. For the docs, I agree that adding a marimo export step to the docs build and pointing the TOC at the generated files is a good approach. It also makes sense to remove the Jupyter-related dependencies if they’re no longer needed. I’ll make those changes in this PR and make sure the notebook runs cleanly with python and the docs build still works. |
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