Move methods into class pages for docs #1290
Conversation
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
| @@ -11,59 +11,27 @@ | |||
| :no-members: | |||
| :no-inherited-members: | |||
| :no-special-members: | |||
| :show-inheritance: | |||
There was a problem hiding this comment.
Why this change? With my original PR, I was too eager in copying over exactly what Qiskit did. You did a better job in your PRs for other repos with making the minimum change necessary to get methods on class pages. It might be possible to get this diff more focused.
There was a problem hiding this comment.
Thanks @Eric-Arellano! You are right, I was using another template and I let the show-inheritance by error. It's fixed now. This file changes a lot because I removed the counter logic following your PR. Do you think I should add it again? As you can see, in the other templates I made now the minimum changes possible =)
|
Thanks for working on this! You'll have to modify some of the other template files under Another issue is that when you click into an actual class page, the sidebar doesn't highlight where you are in the tree: |
|
Hi @coruscating! Thanks for comment! I have added the changes in the other templates as well. With these changes, it seems like we achieve a speed up building the documentation. Right now, in the main branch it takes 10min 53s to build the documentation vs. the 6min 49s in this last commit. As for the highlight in the sidebar, when scrolling further than the class title it should highlight where we are (see the screenshot), maybe you mean something different, just let me know and I'll check it and fix it :) 
|
|
@arnaucasau thanks, I meant the left sidebar. When you navigate the class pages in the Qiskit docs, the left sidebar expands and shows where you are in the navigation. But this PR currently has the left sidebar fully collapsed when you navigate to a class as shown the screenshots above. |
|
I just tried it with the most recent build in the main branch and it has the same behaviour. This is because we are currently removing the stubs from the toctrees for non-production build to achieve a speed up. This change was introduced in #1237. Due to the fact that in this PR we are improving the time as well, I'll open another pull request to study if we can remove that change in the left sidebar without increasing the time a lot. |
|
Oh right, I forgot I made that change 😅. I think it's fine to turn off the production vs. test build distinction in this PR since it was made to deal with the slow build problem, but you can also make a separate PR if you'd like. I tested it locally and a full production build is now ~250 seconds, versus ~200 seconds for the old test build and over an hour for the old production build, so I think the new production build is acceptable to run in all CI. One issue is that individual pages are still rather large at ~1 MB, I don't know if there's an easy fix for this. |
|
I have been checking the sizes of the pages and I didn't see any page bigger than 100 KB, but if you give me an example of page with that issue I can investigate if its possible to reduce its size in another PR as well 👍. Right now, comparing the size of the whole documentation built in this PR vs the one in the main branch, we achieve a reduction of 1.20 MB + the speedup in time. |
|
Hmm you're right, the files themselves are not large. I was saving them as complete webpages in Chrome during testing, which for some reason balloons them to 1 MB+. Are you still planning on turning off the test build settings in a separate PR? If so I think this PR can be merged. |
|
One minor issue is that headings with no members are still rendered, like Attributes below: |
|
@coruscating you're right. The problem was that in reality we had attributes but starting with "__". With the logic in the original templates, those attributes were avoided. So, I've recovered that logic and made the minimum changes possible like in the other template files. I mark the PR as a draft for the moment until I solve the increasing in time using the original template logic or something equivalent.
Yes, I will explore that option in another PR 👍 |
…periments into no-method-pages
…t-experiments into no-method-pages
|
Hi @coruscating! I have changed the templates to not show the title Attributes or Methods if we don't have elements to show. The first approach was to use the same logic that we have in the main branch, but we don't have any improvement in the time building the documentation using the count. Instead, in this PR, we store beforehand all the elements we want to show, and then check if there are elements or not. |
|
Thanks @arnaucasau, the pages look good but it's unfortunate that the docs CI is still 20+ minutes. That doesn't seem faster than the original logic, or am I missing something? |
There was a problem hiding this comment.
Looking at the build times again, this seems to be a slight improvement over the current build, which is closer to 30 minutes, so I'm fine with it for now. I think it would be good to merge this ASAP so we can stop running very big jobs for building the preview site. This can merge after #1312 makes the docs build compatible with the new Qiskit release.
|
Great! Thanks a lot @coruscating, and I apologize for not answering you before, I was looking at the increasing in time as well. When I uploaded the current version of this PR, the building was 9 minutes, but after merging the main branch into this branch, the time increased a lot. I think it can be helpful to know when this happened and I found out that in the main branch it happened something similar after the commit #1306. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Thanks for pointing this out, I've temporarily pinned the Aer version in #1312 and now the docs CI is back to 5 minutes. The docs deploy job for this PR was only 18 minutes compared to 1-2 hours before and the new docs are live 🎉 |
### Summary Moved all the methods and attributes into class pages to speed up the building process of the documentation with the new ecosystem sphinx theme. This PR continues the work done by @Eric-Arellano in qiskit-community#1231. See Qiskit/qiskit#10455 for more information. --------- Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Summary
Moved all the methods and attributes into class pages to speed up the building process of the documentation with the new ecosystem sphinx theme.
This PR continues the work done by @Eric-Arellano in #1231.
See Qiskit/qiskit#10455 for more information.