Skip to content
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

Docs fix #1081

Merged
merged 6 commits into from
May 5, 2023
Merged

Docs fix #1081

merged 6 commits into from
May 5, 2023

Conversation

damonge
Copy link
Collaborator

@damonge damonge commented May 5, 2023

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.

@damonge damonge requested a review from nikfilippas May 5, 2023 17:21
@damonge damonge marked this pull request as ready for review May 5, 2023 17:21
@coveralls
Copy link

Pull Request Test Coverage Report for Build 4895928411

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.004%) to 97.306%

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

💛 - Coveralls

@damonge
Copy link
Collaborator Author

damonge commented May 5, 2023

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
Copy link
Contributor

nikfilippas commented May 5, 2023

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
Copy link
Contributor

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

Copy link
Contributor

@nikfilippas nikfilippas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

@damonge
Copy link
Collaborator Author

damonge commented May 5, 2023

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
Copy link
Collaborator Author

damonge commented May 5, 2023

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
Copy link
Contributor

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
Copy link
Contributor

nikfilippas commented May 5, 2023

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
Copy link
Collaborator Author

damonge commented May 5, 2023

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
Copy link
Collaborator Author

damonge commented May 5, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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

@nikfilippas nikfilippas merged commit fea3e58 into master May 5, 2023
@nikfilippas nikfilippas deleted the doc_fix_5523 branch May 5, 2023 23:39
@nikfilippas
Copy link
Contributor

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

@damonge
Copy link
Collaborator Author

damonge commented May 6, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants