LSSTDESC / CCL

DESC Core Cosmology Library: cosmology routines with validated numerical accuracy
BSD 3-Clause "New" or "Revised" License
145 stars 68 forks source link

Docs fix #1081

Closed damonge closed 1 year ago

damonge commented 1 year ago

Fixes the current docs on rtd (you can check that it is actually working here).

@nikfilippas please review, but remember we want to keep any changes here as minimal as possible. Polishing the docs or including any API improvements is for later.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4895928411


Totals Coverage Status
Change from base Build 4869602522: -0.004%
Covered Lines: 5815
Relevant Lines: 5976

💛 - Coveralls
damonge commented 1 year ago

Note that we also want to have docs for v2.7 (before official API switch), which were also broken. I've fixed them now in https://github.com/LSSTDESC/CCL/tree/v2.7.1, and pointed rtd's latest to it, so this is what it shows as latest until we tag the version in the current master. At that point we'll switch it back to master.

nikfilippas commented 1 year ago

Okay, assuming this is a hotfix branch which really only exists for people to see some docs, overall it's fine. I browsed through them and there are still issues (formulas not showing correctly etc) but let's live with that for 2.7.1 for now, it's okay I guess.

One thing I'd like to point out is the following: because we restructured CCL there are so many new *.rst files. At the same time, these can all be auto-built using make html both locally, and on RTD. So, instead of polluting the repo storage & history with these files, why don't we remove them, add readthedocs/api to .gitignore, and then, simply call make html on RTD after the requirements have been installed.

There are some other issues with the RTD build as well, but we'll go through them on the Docs v3 PR. For example, these two lines, install stuff that clash with one another, in 2 different ways. It's okay though, since the docs build, let's leave it like this for now, and we'll get back to it for v3. Screenshot from 2023-05-05 22-55-08 The current build commands on RTD are convoluted and the docs will soon break once again if these are not addressed.

So, overall, what I propose is to just remove the *.rst files (with make clean), update .gitignore, and replace this command

 python -m sphinx -T -E -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html 

with make html, which is why the makefile is there in the first place.

nikfilippas commented 1 year ago

Also, help me understand your Slack announcement, what is the difference between the docs here and the docs on v2.7.1?

damonge commented 1 year ago

I don't know what you mean by

Replace this command

 python -m sphinx -T -E -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html 

with make html, which is why the makefile is there in the first place.

damonge commented 1 year ago

Also, help me understand your Slack announcement, what is the difference between the docs here and the docs on v2.7.1?

The docs on master show the new API. The docs on v2.7.1 show the docs of the currently released v2.7, which uses the v2 API. The docs from master are not useful, as most people install the latest release from pip/conda-forge.

nikfilippas commented 1 year ago

The docs on v2.7.1 show the docs of the currently released v2.7, which uses the v2 API.

OK, but shouldn't there be a separate PR for those? Am I missing something?

nikfilippas commented 1 year ago

I don't know what you mean by ...

There is a makefile in readthedocs/Makefile which automatically writes the *.rst files using sphinx-apidoc, and then builds the documentation using sphinx-build.

This allows us to remove the *.rst files from the repo completely, so that we don't rely on the developer manually rebuilding the *.rst files in every new PR we merge. This can all happen automatically on RTD.

Until now, we had to create these files locally, then push them to master, where RTD would read them from. This way the docs were almost always out of date, until someone created a docs-fix PR.

damonge commented 1 year ago

I know all that, but I cannot touch the build commands on rtd. And as I said at the beginning, we don't need this. Repos store rst files routinely.

damonge commented 1 year ago

2.7.1: i can't make a PR to rewrite the commit history. This is just there for now so we have useful docs until we make the next release

nikfilippas commented 1 year ago

I cannot touch the build commands on rtd

Right, I checked and we can in fact customize the commands, but I agree - let's not go this far.

This is just there for now so we have useful docs

I see! That's fine.

nikfilippas commented 1 year ago

I'm merging it and I'll check if it pulls the ones from master now.

nikfilippas commented 1 year ago

yes! finally they're back. Now we can point latest to master.

damonge commented 1 year ago

Ok, great. But no, I'll do that when we've released the last v2 version.