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

ENH: Simplify and speed up navigation bar links generation #878

Merged
merged 10 commits into from Aug 27, 2022

Conversation

choldgraf
Copy link
Collaborator

This simplifies our header navigation link code, so that we manually generate these links instead of using the Sphinx TocTree function. It does this by creating a new function to use in our HTML templates. Because we call this twice for the header, this should make documentation builds much faster.

The output shouldn't be much different, though it will remove some Sphinx-specific classes from the output nav items in the header (like toctree-l1). It also means that "numbered" top-level sections don't have numbers anymore (but the sidebars will have numbers like normal). I am not sure how to re-implement that manually but it seems like a reasonable trade-off to me if we get a big speed boost.

cc @tupui and maybe @drammock who I think both mentioned page builds had gone up.

If this makes things faster, I think it:

Copy link
Contributor

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Sorry still not working for SciPy...

python dev.py doc -j 6 4025.48s user 221.28s system 434% cpu 16:18.01 total

vs

python dev.py doc -j 6 743.65s user 61.77s system 438% cpu 3:03.52 total

Let me know if I can help with some timing or other (I don't know how and what so I would need directions, we can also make a call if you want.)

@choldgraf
Copy link
Collaborator Author

choldgraf commented Aug 17, 2022

can you provide more context what your commands are? They look to be the exact same command with different results. And can you be more specific than "still not working"? Does it not build at all?

@tupui
Copy link
Contributor

tupui commented Aug 17, 2022

Sorry yes, first is building with this PR and the other is building with 0.9. The rest is identical with Sphinx 5.1.1.

@choldgraf
Copy link
Collaborator Author

Gotcha - well then I am really not sure what's causing this slow-down. In my opinion, this PR still improves things though

@tupui
Copy link
Contributor

tupui commented Aug 17, 2022

At least on my side I didn't notice a difference with the current RC2. I have

python dev.py doc -j 6 4204.69s user 179.17s system 462% cpu 15:48.39 total

@choldgraf
Copy link
Collaborator Author

I mean improves things in terms of simplicity rather than speed, but if others disagree I can just close this PR.

@tupui
Copy link
Contributor

tupui commented Aug 17, 2022

Sure, sure this I cannot really judge 😃

@choldgraf
Copy link
Collaborator Author

maybe @jorisvandenbossche can try profiling the arrow etc docs with this version as well?

@tupui
Copy link
Contributor

tupui commented Aug 19, 2022

@drammock I've seen you pushed some changes. Let me know if I should try to build again. Or again if there is some other diagnostic I can run to help (I might need guidance here).

@drammock
Copy link
Collaborator

@drammock I've seen you pushed some changes. Let me know if I should try to build again. Or again if there is some other diagnostic I can run to help (I might need guidance here).

still working on it, will let you know

@drammock
Copy link
Collaborator

OK I tried building MNE-Python docs to check for slowdowns

on pydata-sphinx-theme 5d0c6c2 (this PR)

231.16user 2.51system 3:52.78elapsed 100%CPU (0avgtext+0avgdata 1010004maxresident)k
427384inputs+522559outputs (884major+316128minor)pagefaults 0swaps

on pydata-sphinx-theme 83e39b9 (current main)

256.93user 2.61system 4:18.82elapsed 100%CPU (0avgtext+0avgdata 1015004maxresident)k
468098inputs+521394outputs (0major+316763minor)pagefaults 0swaps

on pydata-sphinx-theme b97d332 (v0.9.0)

257.40user 2.77system 4:19.37elapsed 100%CPU (0avgtext+0avgdata 1020516maxresident)k
442204inputs+515520outputs (0major+319835minor)pagefaults 0swaps

environment

$ mamba list sphinx
# packages in environment at /opt/miniforge3/envs/mnedev:
#
# Name                    Version                   Build  Channel
jupyter-sphinx            0.4.0                    pypi_0    pypi
jupyter_sphinx            0.4.0           py310hff52083_0    conda-forge
nbsphinx                  0.8.9              pyhd8ed1ab_0    conda-forge
pydata-sphinx-theme       0.10.0rc2                pypi_0    pypi
sphinx                    5.1.1              pyhd8ed1ab_1    conda-forge
sphinx-autobuild          2021.3.14          pyhd8ed1ab_0    conda-forge
sphinx-copybutton         0.5.0                    pypi_0    pypi
sphinx-design             0.2.0              pyhd8ed1ab_1    conda-forge
sphinx-gallery            0.11.0.dev0               dev_0    <develop>
sphinx-sitemap            2.2.0              pyhd8ed1ab_0    conda-forge
sphinx-theme-builder      0.2.0b1            pyhd8ed1ab_0    conda-forge
sphinx-togglebutton       0.3.2                    pypi_0    pypi
sphinxcontrib-applehelp   1.0.2                      py_0    conda-forge
sphinxcontrib-bibtex      2.4.2              pyhd8ed1ab_0    conda-forge
sphinxcontrib-devhelp     1.0.2                      py_0    conda-forge
sphinxcontrib-htmlhelp    2.0.0              pyhd8ed1ab_0    conda-forge
sphinxcontrib-jsmath      1.0.1                      py_0    conda-forge
sphinxcontrib-qthelp      1.0.3                      py_0    conda-forge
sphinxcontrib-serializinghtml 1.1.5              pyhd8ed1ab_2    conda-forge
sphinxcontrib-youtube     1.2.0                    pypi_0    pypi
sphinxext-rediraffe       0.2.7              pyhd8ed1ab_0    conda-forge

so for me, things are in fact faster on this PR than on main or v0.9.0

I'll see what I can do WRT reproducing the scipy build, but I've had difficulty in the past getting scipy dev environment set up on my machine, so if that continues I may need to rely on @tupui to help debug

@tupui
Copy link
Contributor

tupui commented Aug 19, 2022

@drammock nice I can try to build with that now.

I am more than happy to help you setup SciPy if you want. We can make a quick call on discord or anything else if that helps (feel free to send me an invite too).

TL;DR we have updated our quick start instructions to build (use conda/mamba and avoid windows): https://scipy.github.io/devdocs/dev/dev_quickstart.html

@tupui
Copy link
Contributor

tupui commented Aug 19, 2022

Just tested again and I still have slow builds I am afraid.

@drammock
Copy link
Collaborator

@tupui OK I think I have a working scipy dev env under the new meson build system. python dev.py test yields this:

FAILED scipy/linalg/tests/test_solvers.py::test_solve_discrete_are - AssertionError: 
= 1 failed, 37153 passed, 2172 skipped, 12121 deselected, 133 xfailed, 13 xpassed in 374.61s (0:06:14) ==

Docs say all test should pass on Linux but I'm seeing one failure, will assume it's a fluke and not something wrong with my install / dependencies. LMK if that's a failure you've seen before and know how to fix it. Meanwhile I think I can now at least try to replicate your slow builds.

@tupui
Copy link
Contributor

tupui commented Aug 19, 2022

Super thanks! It's fine for the tests especially if it's just one.

For the doc you can build using:

python dev.py doc -j N_PROCS with N_PROCS the number of cores you want to use to have it parallel. Or you do it manually with make going to the doc folder and it should work as well.

(I assume here you used the conda env file so all deps are there)

@tupui
Copy link
Contributor

tupui commented Aug 19, 2022

You can use my branch here which has updates for the new version scipy/scipy#16660

@drammock
Copy link
Collaborator

Hmm, after removing v0.9 of the theme from the conda env and reinstalling it with pip install -e ., I can't even get the scipy docs to build:

Traceback (most recent call last):
  File "/opt/miniforge3/envs/scipy-dev/lib/python3.10/site-packages/sphinx/builders/html/__init__.py", line 1048, in handle_page
    output = self.templates.render(templatename, ctx)
  File "/opt/miniforge3/envs/scipy-dev/lib/python3.10/site-packages/sphinx/jinja2glue.py", line 188, in render
    return self.environment.get_template(template).render(context)
  File "/opt/miniforge3/envs/scipy-dev/lib/python3.10/site-packages/jinja2/environment.py", line 1301, in render
    self.environment.handle_exception()
  File "/opt/miniforge3/envs/scipy-dev/lib/python3.10/site-packages/jinja2/environment.py", line 936, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "/opt/miniforge3/envs/scipy-dev/lib/python3.10/site-packages/sphinx/themes/basic/page.html", line 10, in top-level template code
    {%- extends "layout.html" %}
  File "/opt/scipy/doc/source/_templates/layout.html", line 12, in top-level template code
    {% extends "!layout.html" %}
  File "/opt/pydata-sphinx-theme/src/pydata_sphinx_theme/theme/pydata_sphinx_theme/layout.html", line 25, in top-level template code
    {% set remove_sidebar_secondary = (meta is defined and meta is not none
  File "/opt/miniforge3/envs/scipy-dev/lib/python3.10/site-packages/sphinx/themes/basic/../basic/layout.html", line 169, in top-level template code
    {%- block content %}
  File "/opt/pydata-sphinx-theme/src/pydata_sphinx_theme/theme/pydata_sphinx_theme/layout.html", line 85, in block 'content'
    {% include "sections/sidebar-primary.html" %}
  File "/opt/pydata-sphinx-theme/src/pydata_sphinx_theme/theme/pydata_sphinx_theme/sections/sidebar-primary.html", line 1, in top-level template code
    {% block docs_sidebar %}
  File "/opt/pydata-sphinx-theme/src/pydata_sphinx_theme/theme/pydata_sphinx_theme/sections/sidebar-primary.html", line 34, in block 'docs_sidebar'
    {%- include sidebartemplate %}
  File "/opt/scipy/doc/source/_templates/sidebar-nav-bs.html", line 6, in top-level template code
    {{ generate_nav_html("sidebar", maxdepth=2, collapse=True, includehidden=True, titles_only=True) }}
  File "/opt/miniforge3/envs/scipy-dev/lib/python3.10/site-packages/jinja2/sandbox.py", line 391, in call
    if not __self.is_safe_callable(__obj):
  File "/opt/miniforge3/envs/scipy-dev/lib/python3.10/site-packages/jinja2/sandbox.py", line 275, in is_safe_callable
    getattr(obj, "unsafe_callable", False) or getattr(obj, "alters_data", False)
jinja2.exceptions.UndefinedError: 'generate_nav_html' is undefined

It looks like the error is in your template of sidebar-nav-bs:

  File "/opt/scipy/doc/source/_templates/sidebar-nav-bs.html", line 6, in top-level template code
    {{ generate_nav_html("sidebar", maxdepth=2, collapse=True, includehidden=True, titles_only=True) }}

it's calling generate_nav_html and not finding it. Are you not seeing that?

@drammock
Copy link
Collaborator

ah OK, I see, I should be using your open PR branch from scipy/scipy#16660

@drammock
Copy link
Collaborator

drammock commented Aug 19, 2022

running with -j 12, here are the build times I get for scipy

on pydata-sphinx-theme 5d0c6c2 (this PR)

5913.52user 147.10system 9:49.39elapsed 1028%CPU (0avgtext+0avgdata 1794204maxresident)k
66180587inputs+1258600outputs (4067major+17010014minor)pagefaults 0swaps

on pydata-sphinx-theme 83e39b9 (main)

Cannot build docs, UndefinedError("'generate_toctree_html' is undefined")

on pydata-sphinx-theme (v0.9.0)

Didn't try because assuming same error as on main

@tupui I need more details about how exactly you are testing / timing. Given what is going on in scipy's doc/source/_templates/sidebar-nav-bs.html I can't seem to actually do a fair comparison (?) since it calls a function that only exists on this PR branch.

@tupui
Copy link
Contributor

tupui commented Aug 19, 2022

@drammock To build on main, you just need to remove my last commit. This is just a change in the template name introduced in this PR (on the theme) that I needed to fix in my PR.

But the timing you get is similar to what I had on 6 cores (factoring the core difference, I needed almost double the time).

@choldgraf
Copy link
Collaborator Author

If it simplifies things to undo the function renaming, that is fine with me. I just renamed it because I thought it would make things easier to remember and understand.

It is a bit strange that @drammock is reporting a boost in performance while @tupui is not 🤔

@drammock
Copy link
Collaborator

actually, just thinking about it, it appears that scipy does not override the theme's layout.html (which means the theme's layout.html file will call generate_toctree_html), but in scipy's doc/source/_templates/sidebar-nav-bs.html that function is being called again. If that's correct, I think in effect you are manually doing twice what we were previously doing twice automatically (and which this PR fixed to only run once by default). Can you investigate/confirm?

@tupui
Copy link
Contributor

tupui commented Aug 19, 2022

Oh then you mean that this should just be moved to layout.html? I am not sure I see how to do this properly.

@drammock
Copy link
Collaborator

Oh then you mean that this should just be moved to layout.html?

well, I haven't looked in detail about exactly what you're doing in the sidebar-nav-bs template. I just followed the traceback and looked at a couple lines of context. Let me look...

@choldgraf It looks like what scipy is doing is adjusting maxdepth depending on which page is being rendered (1 for "reference" pages and 2 for other pages). You understand sphinx better than I do, is there a way to achieve that without having to build the toctree twice? Can the toctree be built once, and then simply "pruned" before being rendered on certain pages?

@tupui is it really necessary to limit the maxdepth in this way, or would it work to instead control the level of "collapse" (https://pydata-sphinx-theme.readthedocs.io/en/latest/user_guide/navigation.html?highlight=collapse#control-how-many-navigation-levels-are-shown-by-default) ?

@tupui
Copy link
Contributor

tupui commented Aug 19, 2022

We did this because the tree is huge for SciPy so for all API we just want a single level otherwise the HTML is huge just for the tree (and since we have thousands of functions, then the overall size of the doc explode just for that). For the other pages we wanted to have a second level, but if that's too complicated then a single level could do. Ideally, this tree should not be repeated on all HTML pages, but I understood from previous discussions that it was not possible or difficult to do.

Right now I am trying to set "navigation_depth": 1 and remove the template, to see if it improves things.

@choldgraf
Copy link
Collaborator Author

Maybe this is something we could also add a feature to control in a per page basis?

@choldgraf
Copy link
Collaborator Author

I've added a GitHub action to this repository that will generate a py-spy results SVG and upload it as an artifact in a GitHub action. If somebody wants to use that to compare the call stack between PRs, we can use that now.

For example:

I'm way over my "nights and weekends" budget on this theme so I will be able to make minimal improvements on this one. However I am more than happy to review PRs or suggestions from others for how we can improve the performance.

@tupui
Copy link
Contributor

tupui commented Aug 22, 2022

To be clear, I am not blaming you for anything and really appreciate all the work you all do on the theme 😃 I am also spending ludicrous time on OSS (not counting my offset from work).

@choldgraf
Copy link
Collaborator Author

choldgraf commented Aug 23, 2022

I looked into this a little bit more, and something tells me that this is an issue with SciPy's implementation, and not necessarily going to be experienced by others. I did the following analysis:

  • For 0.9, 0.10rc2, and this PR
    • For n_pages in [10, 20, 40, 80, 160]:
      • Add n_pages to the demo site
      • Build the demo site 3 times and take the average time to build

This is what resulted:

image

and another run with larger starting values:

image

This is confusing to me, because it's significantly faster on main compared with the last major release.

@tupui
Copy link
Contributor

tupui commented Aug 23, 2022

Thank you for doing this! I think you are right @choldgraf. I tried to slim down the doc and just compile one or two submodules and I have similar timings (between the release and this PR). I suppose I must then try to add them one by one until I see diverging timings... I don't know how to debug this better.

@drammock
Copy link
Collaborator

you could also try removing scipy's custom _template overrides, then adding back elements of the templates one at a time.

@tupui
Copy link
Contributor

tupui commented Aug 23, 2022

Aside from the autosummary part, the last version I am testing is without anything else (as above).

@choldgraf
Copy link
Collaborator Author

What do people think we should do with this PR? According to the tests in #878 (comment), this PR is a significant performance improvement over 0.9. However, it seems like #855 will still not be resolved, but may also no be related to the pydata theme directly?

Can we merge this in as an iterative improvement (happy to have a review of the code though) and figure out what to do next in follow-up issues?

Copy link
Contributor

@tupui tupui left a comment

Choose a reason for hiding this comment

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

I am personally fine with iterating elsewhere if you want to get this one in. It does bring an improvement over main.

src/pydata_sphinx_theme/__init__.py Outdated Show resolved Hide resolved
Co-authored-by: Pamphile Roy <roy.pamphile@gmail.com>
@tupui
Copy link
Contributor

tupui commented Aug 23, 2022

I did a few experiments and I am really puzzled. Individual modules seems to compile at a similar speed between the versions. But when grouped together, some slowdown appears. e.g. the sparse module is being built fast when it's alone, but when there are other modules it seems like it's building slower for some reason.

Maybe there are some deadlocks somewhere. I am not sure what I can try next. Looks like the last resort test worked... see next comment.

@tupui
Copy link
Contributor

tupui commented Aug 23, 2022

Oh @drammock I actually just tested to remove all autosummary templates and this is it! But seems like it's really a problem only when building multiple modules.

I am not sure if we can remove these or if you have some workaround for us especially for the template of classes. The other ones don't seem to do much (but I don't really know much about Sphinx templating):

{{ fullname }}
{{ underline }}

.. currentmodule:: {{ module }}

.. autoclass:: {{ objname }}
   :no-members:
   :no-inherited-members:
   :no-special-members:

  {% block methods %}
   .. HACK -- the point here is that we don't want this to appear in the output, but the autosummary should still generate the pages.
      .. autosummary::
         :toctree:
      {% for item in all_methods %}
         {%- if not item.startswith('_') or item in ['__call__', '__mul__', '__getitem__', '__len__'] %}
         {{ name }}.{{ item }}
         {%- endif -%}
      {%- endfor %}
      {% for item in inherited_members %}
         {%- if item in ['__call__', '__mul__', '__getitem__', '__len__'] %}
         {{ name }}.{{ item }}
         {%- endif -%}
      {%- endfor %}
  {% endblock %}

  {% block attributes %}
  {% if attributes %}
   .. HACK -- the point here is that we don't want this to appear in the output, but the autosummary should still generate the pages.
      .. autosummary::
         :toctree:
      {% for item in all_attributes %}
         {%- if not item.startswith('_') %}
         {{ name }}.{{ item }}
         {%- endif -%}
      {%- endfor %}
  {% endif %}
  {% endblock %}

@drammock
Copy link
Collaborator

I actually just tested to remove all autosummary templates and this is it! But seems like it's really a problem only when building multiple modules.

great that we know where the problem is now! I can try to look again next tues/weds when I'm back at my office (overseas at the moment, and my laptop doesn't have a scipy dev environment set up)

@tupui
Copy link
Contributor

tupui commented Aug 23, 2022

Thank you 🙇 FYI to build the doc faster, I just trimmed down the file API.rst.txt to only build modules I wanted (and removed all corresponding files in source/reference), and I removed in index.rst the tutorial section which is also a large chunk as it runs a lot of code examples.

@choldgraf
Copy link
Collaborator Author

choldgraf commented Aug 24, 2022

Could you clarify whether you think that the speed issue is related to this PR or this theme? If not then I think it would be better to open a dedicated issue to track down and discuss the autosummary issue.

Sorry for being picky I am just trying to figure out if / what I need to do in this PR specifically 😅

@tupui
Copy link
Contributor

tupui commented Aug 24, 2022

Sure, the issue is not linked to this PR. On main I have the same behaviour, when I have some autosummary template, it's slow. And if I remove them, build is almost twice as fast.

I can create a new issue if you want and we merge this?

@choldgraf
Copy link
Collaborator Author

That sounds good - I mostly just think that it will be more effective if we have a dedicate place to track conversation etc around the topic of the scipy docs / autosummary, because it sounds like the fixes that would be needed are not directly related to this PR and we don't want to lose track of it.

Would anybody else prefer that we hold off on merging this one?

@drammock
Copy link
Collaborator

+1 for merge here.

@choldgraf
Copy link
Collaborator Author

OK I haven't seen any objections and this one does come with a nice speed-up, so I'm gonna merge this :-)

@drammock
Copy link
Collaborator

drammock commented Oct 11, 2022 via email

@jarrodmillman jarrodmillman added this to the 0.11 milestone Oct 17, 2022
@jayaddison
Copy link
Contributor

Connecting a reference that GitHub doesn't seem to have linked/displayed here, to make sure that doesn't get dropped - this could be related to a JSON encoding issue reported at sphinx-doc/sphinx#10990.

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