nextstrain / ncov

Nextstrain build for novel coronavirus SARS-CoV-2
https://nextstrain.org/ncov
MIT License
1.35k stars 403 forks source link

docs: lists not rendering #891

Closed victorlin closed 2 years ago

victorlin commented 2 years ago

Sometime in the last few days, lists have stopped rendering properly on doc pages in this repo.

Take the Data Prep page as an example:

data-prep-bad-render

The preview generated 5 days ago for https://github.com/nextstrain/ncov/pull/878/commits/7a9d6286e1179563b8c041f08ff0d2a3ca6ce6d9 looks fine (this doc page hasn't changed since then per history):

data-prep-good-render
victorlin commented 2 years ago

Seems like this happens when docutils=0.17 is installed [ref].

RTD build for https://github.com/nextstrain/ncov/pull/878/commits/7a9d6286e1179563b8c041f08ff0d2a3ca6ce6d9 which rendered properly:

Requirement already satisfied: docutils>=0.11 in /home/docs/checkouts/readthedocs.org/user_builds/ncov/conda/878/lib/python3.9/site-packages (from recommonmark->-r /home/docs/checkouts/readthedocs.org/user_builds/ncov/checkouts/878/docs/tmpyQ53vi.requirements.txt (line 1)) (0.17.1)
...
Collecting docutils>=0.11
  Downloading docutils-0.16-py2.py3-none-any.whl (548 kB)
...
Installing collected packages: docutils, ...
  Attempting uninstall: docutils
    Found existing installation: docutils 0.17.1
    Uninstalling docutils-0.17.1:
      Successfully uninstalled docutils-0.17.1
  Attempting uninstall: recommonmark
    Found existing installation: recommonmark 0.6.0
    Uninstalling recommonmark-0.6.0:
      Successfully uninstalled recommonmark-0.6.0
Successfully installed docutils-0.16 ...

RTD build for current master which does not render properly:

Requirement already satisfied: docutils>=0.11 in /home/docs/checkouts/readthedocs.org/user_builds/ncov/conda/latest/lib/python3.9/site-packages (from recommonmark->-r /home/docs/checkouts/readthedocs.org/user_builds/ncov/checkouts/latest/docs/tmpgAMIg5.requirements.txt (line 1)) (0.17.1)
...

RTD build with docutils=0.16 in conda.yml while experimenting in #890:

Requirement already satisfied: docutils>=0.11 in /home/docs/checkouts/readthedocs.org/user_builds/ncov/conda/890/lib/python3.9/site-packages (from recommonmark->-r /home/docs/checkouts/readthedocs.org/user_builds/ncov/checkouts/890/docs/tmpq13NBh.requirements.txt (line 1)) (0.16)
...
Collecting docutils>=0.11
  Downloading docutils-0.17.1-py2.py3-none-any.whl (575 kB)
...
Installing collected packages: docutils, ...
  Attempting uninstall: docutils
    Found existing installation: docutils 0.16
    Uninstalling docutils-0.16:
      Successfully uninstalled docutils-0.16
  Attempting uninstall: recommonmark
    Found existing installation: recommonmark 0.6.0
    Uninstalling recommonmark-0.6.0:
      Successfully uninstalled recommonmark-0.6.0
Successfully installed docutils-0.17.1 ...

This makes me think that docutils=0.17 is pinned somewhere, maybe by a pip dependency (uninstall/reinstall output looks like it's from pip). Even explicitly specifying docutils=0.16 in conda.yml does not fix - it is uninstalled and 0.17 is reinstalled.

victorlin commented 2 years ago

UPDATE: it was pinned by a pip package, sphinx-tabs, in a recent release (executablebooks/sphinx-tabs#144). This also explains the timing.

Using conda to handle that dependency pulls an older version that can handle docutils!=0.17. Maybe docutils=0.18 can work, but will need to wait for sphinx-tabs to support that.

victorlin commented 2 years ago

So, turns out that installing docutils<0.17 is a fix, but not a great one.

The real reason, from a closed docutils bug:

I think this is due to <div class="section"> changing to <section>. Our theme used ".section".

This change can be observed in the examples linked in the first comment. If you add class="section" on the new <section>, the CSS which handles the list styling does its job. However, we don't want to add class="section". Instead, the CSS should be upgraded to target <section> directly.

Digging into nextstrain/sphinx-theme, there is only 1 CSS file nextstrain.css. This imports theme.css which has the list styling selector responsible for this behavior. But theme.css is not a file checked into our repo! I can tell that it is from sphinx_rtd_theme, but I'm not sure how it is getting pulled.

Take a look at theme.css used by ncov docs: https://docs.nextstrain.org/projects/ncov/en/latest/_static/css/theme.css

/* sphinx_rtd_theme version 0.4.3 | MIT license */
/* Built 20190212 16:02 */
...
.section ol,.rst-content ol.arabic,article ol{list-style:decimal

Compared to this snippet from the theme.css on sphinx-rtd-theme.readthedocs.io, which should be latest: https://sphinx-rtd-theme.readthedocs.io/en/stable/_static/css/theme.css

section ol.arabic,.wy-plain-list-decimal,article ol{list-style:decimal

A local build of the docs generates the latest theme.css. This also explains some of the slight styling differences I've noticed between local build and RTD-hosted docs.

@tsibley do you know why a version of theme.css from 20190212 is being used on our RTD sites, and how to update it?

tsibley commented 2 years ago

Good sleuthing.

But theme.css is not a file checked into our repo! I can tell that it is from sphinx_rtd_theme, but I'm not sure how it is getting pulled.

Since our theme extends sphinx_rtd_theme (and thus it's installed as a dep), Sphinx includes its static assets when it builds the docs. See the Sphinx docs on theme development.

do you know why a version of theme.css from 20190212 is being used on our RTD sites, and how to update it?

Presumably the version of sphinx-rtd-theme that's installed when our docs are built on RTD isn't 1.0.0, which is the latest release and first version of sphinx-rtd-theme to add support for docutils 0.17.

We don't force a particular minimum version of sphinx-rtd-theme anywhere AFAIK, so for example, if it's pre-installed in the RTD build env then we'll get whatever version that is.

tsibley commented 2 years ago

Indeed, looking at the RTD logs for the most recent latest build, I see:

Requirement already satisfied: sphinx-rtd-theme in … (0.4.3)

I also noticed that RTD slightly tweaks the docs/conda.yaml:

diff --git a/docs/conda.yml b/docs/conda.yml
index 950aeb4b..c0a41d15 100644
--- a/docs/conda.yml
+++ b/docs/conda.yml
@@ -1,4 +1,3 @@
-name: ncov-docs
 channels:
 - defaults
 dependencies:
@@ -8,7 +7,14 @@ dependencies:
 - sphinx
 - pip
 - pip:
+  - recommonmark
+  - readthedocs-sphinx-ext
   - nextstrain-sphinx-theme>=2020.2
   - sphinx-markdown-tables
   - sphinx-argparse
   - sphinx-tabs
+- mock
+- pillow
+- sphinx
+- sphinx_rtd_theme
+name: ncov-docs
tsibley commented 2 years ago

Notably, sphinx_rtd_theme from Conda is stuck at 0.4.3 unless you enable the conda-forge channel:

$ conda search sphinx_rtd_theme
…
sphinx_rtd_theme               0.4.3    pyhd3eb1b0_0  pkgs/main
sphinx_rtd_theme               0.5.0    pyh9f0ad1d_0  conda-forge
sphinx_rtd_theme               0.5.1    pyhd3deb0d_0  conda-forge
sphinx_rtd_theme               0.5.2    pyhd8ed1ab_0  conda-forge
sphinx_rtd_theme               0.5.2    pyhd8ed1ab_1  conda-forge
sphinx_rtd_theme               1.0.0    pyhd8ed1ab_0  conda-forge
victorlin commented 2 years ago

Ahh mystery solved! Thanks for digging further @tsibley, I'll create a PR to get sphinx_rtd_theme 1.0.0 installed.

victorlin commented 2 years ago

@tsibley the above PR in nextstrain/sphinx-theme should be sufficient once released on PyPI.

The absence of sphinx_rtd_theme from defaults shouldn't be a problem for the current conda.yml since it is handled by pip:

https://github.com/nextstrain/ncov/blob/fb985678b052809d7829f9e8e72e82250a22a142/docs/conda.yml#L10-L11

If anything, we will need to bump nextstrain-sphinx-theme to the newest version once released.

victorlin commented 2 years ago

Fixed by releasing nextstrain/sphinx-theme#19 and re-building the live docs via Read the Docs.