Bump pydata-sphinx-theme #27658
Bump pydata-sphinx-theme #27658
Conversation
8632235 to
b19af75
Compare
5219372 to
149374a
Compare
c66acba to
9ec03b7
Compare
9ec03b7 to
908f3c8
Compare
4c030e9 to
d369a06
Compare
| @@ -147,7 +147,7 @@ commands: | |||
| export RELEASE_TAG='-t release' | |||
| fi | |||
| mkdir -p logs | |||
| make html O="-T $RELEASE_TAG -j4 -w /tmp/sphinxerrorswarnings.log" | |||
| make html O="-T $RELEASE_TAG -j1 -w /tmp/sphinxerrorswarnings.log" | |||
There was a problem hiding this comment.
This change is because pydata-sphinx-theme doesn't support parallel writes: pydata/pydata-sphinx-theme#1643. It doesn't seem to impact build time significantly though, 18m19s before and 18m57s after.
There was a problem hiding this comment.
Not tested with the new version, but IIRC, I got significant speedups on my local machine with -j4. If we can't have that anymore, we need to update https://matplotlib.org/devdocs/devel/document.html#build-the-docs. Also, > 10min doc build times makes working on docs very unattractive. As somebody else said
In practice, that goes from "let me quickly build the docs to see the effect of this change" to "let's just hope it renders correctly".
Are the other improvements so significant that we are willing to take this on?
Edit: or is the above comparison old version -j4 vs. new version -j1. There were other performance improvements in the pydata Sphinx theme that might just compensate the losses of non-parallel builds.
There was a problem hiding this comment.
The comparison is on circleCI between 591cd65 and f178f5c, the only difference being changing -j4 to -j1. Perhaps even with -j4 circleCI wasn't using any parallelism to build the docs?
Are the other improvements so significant that we are willing to take this on?
They include at least some accesibility improvements which I would advocate for.
There was a problem hiding this comment.
Note that this is changing the parallel option only for the circleCI build, not any local or other builds, so the comparison using only times measured on circleCI is fair.
There was a problem hiding this comment.
And you can still use parallel options, it will just raise a warning at the end of the doc build that pydata-sphinx-theme isn't parallel write safe.
There was a problem hiding this comment.
I don't see any need to change our call here. The theme marks it self as parallel-unsafe, so requesting parallel build will be turned off anyway. And then when they fix themselves, we'd need to remember to turn it back on again.
There was a problem hiding this comment.
Without using -j1, shpinx emits a warning that the theme isn't parallel write safe, which fails the build. Ideally there would be a way to ignore just this warning and use -j4, but I'm not aware if that's possible with sphinx?
There was a problem hiding this comment.
You could try to add a warnings filter to conf.py
There was a problem hiding this comment.
Do you mean the suppress_warnings configuration? It seems to be the only relevant option to do this, but warning about a non-parallel write safe theme doesn't seem to be convered by the list of warning types that can be suppressed.
| @@ -14,29 +14,6 @@ | |||
| margin: 0; | |||
| } | |||
|
|
|||
| /* Make announcement an error colour for unreleased documentation, and sticky. */ | |||
There was a problem hiding this comment.
This CSS interacts badly with dark mode, and removing it (and defaulting to what pydata-sphinx-theme uses) looks fine.
|
After much force pushing I think this is finally ready for review! |
|
TL;DR I’m just stating my discomfort, not suggesting to block or do anything about it. On a general note, I'm a bit annoyed that every link is now underlined. While this is required for accessibility pydata/pydata-sphinx-theme#1353 and makes sense for regular links, it's rather distracting in gallery code:
|
|
I'm not thrilled with the new pydata theme either. I suppose we can customize somewhat and maybe there is a css entry for the gallery entries? |
|
FWIW I quite like the link underlining in the gallery code - there's lots I didn't realise were there before they were underlined! I wouldn't block a PR to change that though. @jklymak are there any specific bits you don't like? |
|
In particular, not a fan of underlining anywhere except in very rare circumstances. But I appreciate that many web sites use this. |
|
Ping @dstansby the doc build now fails with the parallel warnings: e.g. https://app.circleci.com/pipelines/github/matplotlib/matplotlib/30177/workflows/c42c2b78-af5a-4640-a051-efcd152bf95c/jobs/82144/parallel-runs/0/steps/0-113 |
PR summary
The goal of this PR is to bump
pydata-shpinx-theme, so we can take advantage of improvements/bug fixes that have happened since version 0.13.Things that are
currently brokennow fixed:mpl-sphinx-theme, so there must be something funny going on inmatplotlibitself that is stopping this from working... update 1: it'sversion-switcher.htmlthat's breaking this somehow. update 2: I think Search button/field broken on local development build pydata/pydata-sphinx-theme#1654 is the upstream issue hereFixes #27652
Fixes matplotlib/mpl-sphinx-theme#80
PR checklist