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

DOC: update pydata-sphinx theme #16660

Merged
merged 42 commits into from
Jan 24, 2024
Merged

DOC: update pydata-sphinx theme #16660

merged 42 commits into from
Jan 24, 2024

Conversation

tupui
Copy link
Member

@tupui tupui commented Jul 21, 2022

This is a provision for the next release of the PyData Sphinx Theme which should happen in the following days (RC2 already up).

This will be needed if the doc is build after 0.10 is released otherwise there will be no search bar. The new search bar is a way better experience. Overall the new version provides a lot of improvements.

@tupui tupui added Website Items related to the website; please also check https://github.com/scipy/scipy.org Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org backport-candidate This fix should be ported by a maintainer to previous SciPy versions. labels Jul 21, 2022
@tupui tupui added this to the 1.9.0 milestone Jul 21, 2022
@tupui tupui requested a review from tylerjereddy July 21, 2022 06:50
@tupui tupui requested a review from rgommers as a code owner July 21, 2022 06:50
@mdhaber
Copy link
Contributor

mdhaber commented Jul 21, 2022

@tupui tried to resolve merge conflicts. Hope I did it correctly. (Oops, probably shouldn't have skipped CI. )

doc/source/conf.py Outdated Show resolved Hide resolved
Co-authored-by: Matt Haberland <mhaberla@calpoly.edu>
@tupui
Copy link
Member Author

tupui commented Jul 21, 2022

Thanks Matt for having a look and catching the lint issue 👍 (for the rest we need to wait for the release of the theme.)

@tupui tupui modified the milestones: 1.9.0, 1.9.1 Jul 29, 2022
@tupui tupui removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Aug 10, 2022
@melissawm
Copy link
Contributor

melissawm commented Jan 19, 2024

13 mins! And it looks like the warnings are safe. We probably need to add a filter to silence the "theme is not parallel safe" warning, but that seems to be the only issue 🙏

@tupui
Copy link
Member Author

tupui commented Jan 19, 2024

Just for fun, I clicked on the "Explain this error" with AI 🤣 Looks like Sphinx is still too hard for a LLM

Screenshot 2024-01-19 at 01 12 36

@tupui
Copy link
Member Author

tupui commented Jan 19, 2024

@drammock I will have to test locally but online, the sidebar is still off I am afraid. The build time is good though 😃 Was there another PR you wanted to make on this branch?

Screenshot 2024-01-19 at 01 23 17

@h-vetinari
Copy link
Member

I will have to test locally but online, the sidebar is still off I am afraid.

It's sidebar and top bar, it seems. I.e. clicking on the "API reference" tab on the top still shows "Development" tab being highlighted (matching the content of the left sidebar).

@tupui
Copy link
Member Author

tupui commented Jan 19, 2024

Yes, same as current deployment.

@drammock
Copy link
Contributor

Argh, ok. I'll try to look again tomorrow.

@drammock
Copy link
Contributor

drammock commented Jan 19, 2024

  • make sure that pydata-sphinx-theme 0.15.2 is actually getting installed into CircleCI environment (no "show installed versions" step in CircleCI, but the rendered doc says 0.15.2 in the footer, so we can be fairly sure)
  • check if sphinx version matters. CircleCI logs say it was built with Sphinx 5.3.0; my local tests from before were using Sphinx 7.2.6.
    • Local tests can't replicate problem in CIs. Building docs in an env based on the repo's environment.yml (which uses Sphinx 5.3.0) but with pydata theme updated to 0.15.2, using python dev.py --no-build doc --no-cache -jN, I see the correct sidebar on API pages regardless of building with -j1, -j2, or -j8. Not sure what to try next 😞

@tupui
Copy link
Member Author

tupui commented Jan 19, 2024

@drammock if building locally leads to all sidebars etc being ok. Then I think we are good. We can only know for sure once we deploy for real but it might just be again the same issue we have right now with our current dev doc.

@drammock
Copy link
Contributor

@drammock if building locally leads to all sidebars etc being ok. Then I think we are good. We can only know for sure once we deploy for real but it might just be again the same issue we have right now with our current dev doc.

in that case I guess to get the CircleCI build to "pass" you need to somehow make it OK/expected to see

WARNING: the pydata_sphinx_theme extension is not safe for parallel writing
WARNING: doing serial write

... or else stop running CircleCI with -j2

@tupui tupui requested a review from larsoner as a code owner January 19, 2024 23:36
@tupui tupui removed the request for review from larsoner January 19, 2024 23:38
@tupui
Copy link
Member Author

tupui commented Jan 19, 2024

@drammock thank you. For now let's try with removing -j2 because IIUC the warning is a log and would need to write a filter on an event handler.

Otherwise I had the chance to run this locally and it seems all good.

Screenshot 2024-01-20 at 00 50 08

@h-vetinari it would be helpful if you had time to confirm this too. And if everything is fine (and CI comes back green), I would suggest to move this forward.

@h-vetinari
Copy link
Member

@h-vetinari it would be helpful if you had time to confirm this too.

So far everything I've done was check the docs as rendered by CI (though IMO this is also the closest we're going to get in advance to what the actual deployment will look like), and with the latest change, the rendered docs look fine AFAICT.

Given the behaviour observed by @drammock, it seems pretty clear that the -j2 is the culprit, given the lack of thread-safety of PST?

And if everything is fine (and CI comes back green), I would suggest to move this forward.

Fine by me to move forward with this and merge, though given the current 42 commits1, I think we should either squash, or clean up the git history.

Footnotes

  1. the answer to life, the universe and everything!

@tupui
Copy link
Member Author

tupui commented Jan 20, 2024

@h-vetinari I am fine with a squash 👍

(In general I am a partisan of squash merge either way 😉)

@drammock
Copy link
Contributor

Squash is fine with me, FWIW. I'll keep looking into why it wasn't enough to just mark the theme as parallel write unsafe... But that shouldn't hold up things here.

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Thanks for sticking through this slog @tupui, and for the help @drammock!

@tupui
Copy link
Member Author

tupui commented Jan 21, 2024

Thank you all for your help and dedication! This is great that we can finally move forward and be in sync with the theme.

@tylerjereddy would it be possible for you to re-publish the doc for 1.12 with this?

@h-vetinari
Copy link
Member

h-vetinari commented Jan 23, 2024

@tylerjereddy would it be possible for you to re-publish the doc for 1.12 with this?

Let's just merge this and backport it, then it'll automatically show up for 1.12.1 at the latest anyway.

(FWIW, I only didn't push the button yet because I don't want to write the commit message resp. filter/condense the one proposed by github - I feel that's better handled by the author in this case)

@tupui
Copy link
Member Author

tupui commented Jan 23, 2024

@h-vetinari Please feel free to merge. For the message I don't think we need much more than saying we updated the theme. No need to talk about all the commits in the squash. The only important thing to me is to keep the co-author mentions.

Copy link
Contributor

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

make dist seems to succeed on this branch, so that's a good sign for the release process

If 1.13.0 comes out before 1.12.1 I don't know how much effort backporting is worth, but keeping the label for now seems "ok."

@h-vetinari h-vetinari merged commit e8b043d into scipy:main Jan 24, 2024
25 checks passed
@h-vetinari h-vetinari modified the milestones: 1.12.0, 1.13.0 Jan 24, 2024
@tupui tupui deleted the search_theme branch January 26, 2024 16:10
@tylerjereddy tylerjereddy removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org Website Items related to the website; please also check https://github.com/scipy/scipy.org
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOC: Dev branch docs render Dev TOC while viewing API Reference BUG: scrollbar not visible in docs