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
Conversation
There was a problem hiding this 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.)
tests/sites/sidebars/_templates_single_sidebar/components/sidebar-nav-bs.html
Show resolved
Hide resolved
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? |
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. |
Gotcha - well then I am really not sure what's causing this slow-down. In my opinion, this PR still improves things though |
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 |
I mean improves things in terms of simplicity rather than speed, but if others disagree I can just close this PR. |
Sure, sure this I cannot really judge 😃 |
maybe @jorisvandenbossche can try profiling the arrow etc docs with this version as well? |
@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 |
OK I tried building MNE-Python docs to check for slowdowns on pydata-sphinx-theme 5d0c6c2 (this PR)
on pydata-sphinx-theme 83e39b9 (current main)
on pydata-sphinx-theme b97d332 (v0.9.0)
environment
so for me, things are in fact faster on this PR than on 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 |
@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 |
Just tested again and I still have slow builds I am afraid. |
@tupui OK I think I have a working scipy dev env under the new meson build system.
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. |
Super thanks! It's fine for the tests especially if it's just one. For the doc you can build using:
(I assume here you used the conda env file so all deps are there) |
You can use my branch here which has updates for the new version scipy/scipy#16660 |
Hmm, after removing v0.9 of the theme from the conda env and reinstalling it with
It looks like the error is in your template of
it's calling |
ah OK, I see, I should be using your open PR branch from scipy/scipy#16660 |
running with on pydata-sphinx-theme 5d0c6c2 (this PR)
on pydata-sphinx-theme 83e39b9 (main)Cannot build docs, on pydata-sphinx-theme (v0.9.0)Didn't try because assuming same error as on @tupui I need more details about how exactly you are testing / timing. Given what is going on in scipy's |
@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). |
actually, just thinking about it, it appears that scipy does not override the theme's |
Oh then you mean that this should just be moved to |
well, I haven't looked in detail about exactly what you're doing in the @choldgraf It looks like what scipy is doing is adjusting @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) ? |
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 |
Maybe this is something we could also add a feature to control in a per page basis? |
I've added a GitHub action to this repository that will generate a 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. |
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). |
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:
This is what resulted: and another run with larger starting values: This is confusing to me, because it's significantly faster on |
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. |
you could also try removing scipy's custom |
Aside from the |
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 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? |
There was a problem hiding this 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
.
Co-authored-by: Pamphile Roy <roy.pamphile@gmail.com>
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.
|
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):
|
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) |
Thank you 🙇 FYI to build the doc faster, I just trimmed down the file |
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 😅 |
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? |
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? |
+1 for merge here. |
OK I haven't seen any objections and this one does come with a nice speed-up, so I'm gonna merge this :-) |
I cannot tell yet (haven't looked closely at their templates yet) but it's almost surely not related to the changes in *this PR*. Fine by me to open a new issue
-------- Original Message --------
…On Aug 24, 2022, 05:30, Chris Holdgraf wrote:
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.
—
Reply to this email directly, [view it on GitHub](#878 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAN2AUZYGVBDDYMT5EM5N7TV2WQMXANCNFSM56YJ4DMQ).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
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. |
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: