-
Notifications
You must be signed in to change notification settings - Fork 128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use sphinx-autoapi to generate API documentation #3646
base: main
Are you sure you want to change the base?
Conversation
@bouweandela have you actually run the API docs builds with
|
also any clue how to tell ReadTheDocs we don't want a local build of the package? It complains it's missing the pkg metadata since the darn thing was not pip installed, but we don't want that, that's exactly what we're trying to avoid with autoapi https://readthedocs.org/projects/esmvaltool/builds/24658734/ |
We probably need a different way of getting the version than importing the whole package (which fails because it is not installed): ESMValTool/doc/sphinx/source/conf.py Line 26 in ae00a53
|
No
🍻
How slow is incredibly slow? The current build on readthedocs takes 15 minutes.
Are those for modules that are part of the public API? |
let's see how slow it is on RtD - when it runs, it took me some 10min and I killed it; indeed, all manners of modules that should have private API, think that needs configuring not to go through those, need to see their configuration settings. Ah cheers for the |
well good news! It ran and it ran in 6min 🥳 The warnings are annoying though - it's looking for esmvaltool modules in diags - we need to configure it so it doesn't do that; incidentally, RtD's new dashboard is looking nice but it adds a million empty lines to an already very stuffy output https://app.readthedocs.org/projects/esmvaltool/builds/24659446/ |
OK 396 warnings from intial 470 - I am now excluding
|
doc/sphinx/source/conf.py
Outdated
] | ||
|
||
# Autoapi configuration | ||
autoapi_dirs = ['../../../esmvaltool'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible change this list so only these are documented? https://github.com/ESMValGroup/ESMValTool/tree/main/doc/sphinx/source/api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeh lemme add all those to the list that way then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it works nicely! Less than two minutes to run, but it throws 100+ warnings, see https://readthedocs.org/projects/esmvaltool/builds/24660091/ - I think I know what's going on in here - there are two types of warnings:
- a lot coming from
autodoc
which I should turn it off completely - another lot coming from
autoapi
docs dir sitting inside oursource
so it's kicking its own butt since theautoapi
docs are not included in any docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey Bouwe, any ideas how to fix these warnings? https://readthedocs.org/projects/esmvaltool/builds/24661254/ - I've done everything I could think of, and a tap dance on top of it too - autodoc keeps wanting to import modules, even though I added ESMValTool and esmvaltool to the Python path (that's just not how things should be done, but that's what the Sphinx folks say we should do), and the blithering autoapi keeps creating that index.rst
even if I told it no with autoapi_generate_api_docs = False
(see https://sphinx-autoapi.readthedocs.io/en/latest/how_to.html). I am stuck, and am gonna leave it for now 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I fixed the build in f07c760, but it is not pretty. Somehow I cannot get autoapi
to respect what is in __all__
(see here). At least in part that seems to be because it also searches for executable scripts and documents those and those appear to be scattered throughout the code base. Maybe if we clean that up things will work better?
We would still need to move the documentation from the now deleted doc/sphinx/source/api
directory to the Python modules and do some further polishing before this is ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a serious bit of progress - from 126 to 7 warnings, bud 🍺 But what do you mean moving the docs to the Python modules, as in create the docs in the same place where the source code lives? Lemme see about restricting Autoapi to not look for executables, that should have a configuration solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not executable scripts after all, it is missing __init__.py
files that generate all the extra packages. Let me try to add some.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahaa! That's me completely unawares - it's great you figured that out otherwise it'd have taken me until 2027 to 😁 So I figured out the tests
issue and now I think I just need to add Katja's testkw
to the ignored list too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what do you mean moving the docs to the Python modules, as in create the docs in the same place where the source code lives?
Copy the text from the deleted .rst files to the module docstrings
This is not quite ready for review yet: the text from the removed .rst files still needs to be moved to the docstrings of the relevant python modules. |
well, you are the only reviewer, and major contributor, so this is ready only when you say so, bud - plop a changes needed blocker and we is good 🍺 |
@bouweandela am gonna fix the myriad of conflicts now, then maybe you have a second or two give it a bit more polish, and merge? It don't have to be perfect 😁 |
@bouweandela you reckon we can get this in? Or, you'd still want to prettify the docs layouts? |
Merging this without restoring the text I deleted to get things working quickly (#3646 (comment)) would make the documentation rather unreadable. |
Description
Before you get started
Checklist
It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.