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

[do not merge yet] Add navigation_depth HTML option for new theme to limit table of contents depth #451

Closed
wants to merge 4 commits into from

Conversation

Eric-Arellano
Copy link
Collaborator

@Eric-Arellano Eric-Arellano commented Jul 1, 2023

Improves #328. It took 3.5 hours to render the qiskit-metapackage with the table of contents set to -1, compared to 0.5 hours for Pytorch. Looking at the generated HTML, about 85% of the page content is the left table of contents.

We still need to make other performance improvements, but this makes progress.

The default is still set to -1 because that is usually the optimal user experience. We want admins to experiment with what option balances UX vs. their docs build speed. They need to experiment to find that number, rather than us hardcoding it.

Removes instructions for expandable_sidebar

According to grep on the ~20 projects we know about that use qiskit_sphinx_theme, no one uses expandable_sidebar. These migration instructions were complex, so it's useful to get rid of them since they're irrelevant and made the migration look scarier than it really is.

Copy link
Member

@frankharkins frankharkins left a comment

Choose a reason for hiding this comment

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

Is it possible to set the depth differently for dev and production? E.g. through an environment variable.

@Eric-Arellano
Copy link
Collaborator Author

Is it possible to set the depth differently for dev and production? E.g. through an environment variable.

It is! For example, that is how we dynamically switch the HTML theme in our example docs:

_THEME = os.getenv("THEME", "qiskit")
html_theme = _THEME

Are you thinking that's so that you can set it to 1 in development but something like 2 or 3 in prod?

@frankharkins
Copy link
Member

Are you thinking that's so that you can set it to 1 in development but something like 2 or 3 in prod?

Exactly, or even -1 for a release?

Copy link
Collaborator

@javabster javabster left a comment

Choose a reason for hiding this comment

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

generally seems logical to me, have you tested build times for each depth level? Is there a depth number we could recommend to the terra team that is kind of a sweet spot for depth and build time?

) -> None:
"""Furo always fully expands its table of contents, which takes way too long to build docs
for most Qiskit projects."""
if "toctree" in context:
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to copy this whole chunk of code from fruo? is it possible for us to just set the maxdepth param of the toctree and leave the others to be set by furo? or do we have to overwrite the entire toctree object?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it's necessary to copy it all. I posted upstream if he'd be willing to accept a PR for adding navigation_depth since it would only be about 5 lines.

@Eric-Arellano
Copy link
Collaborator Author

generally seems logical to me, have you tested build times for each depth level? Is there a depth number we could recommend to the terra team that is kind of a sweet spot for depth and build time?

I haven't tested because we need to merge this commit to test it out. Metapackage already takes so dang long to build (30 minutes) that I was thinking we probably want to keep it to 1 for now. But I like Frank's idea that we could use 1 in PR builds and something like 2 in production builds.

@coruscating
Copy link
Collaborator

I think this is a necessary change, but I'm not sure about it closing #328 because the build is still quite slow. On a minimal version of Experiments with many experiment classes deleted for a faster build and timed with time:

  • Furo with navigation_depth=-1: 520.95s user 7.98s system, file size ~240k
  • Furo with navigation_depth=1: 435.74s user 7.88s system, file size for typical API page ~20k
  • Using this PR and setting html_theme = "qiskit_sphinx_theme": 152.07s user 7.17s system, file size ~16k
  • Using 1.12.1 and setting html_theme = "qiskit_sphinx_theme": 32.26s user 5.86s system, file size ~16k

So this PR definitely reduces output file sizes, but there's still a long way to go for build times. I'm especially surprised by the slowdown in the legacy theme on this PR—what was changed on main that would have caused this?

@Eric-Arellano Eric-Arellano changed the title Add navigation_depth HTML option for new theme to limit table of contents depth [do not merge yet] Add navigation_depth HTML option for new theme to limit table of contents depth Jul 3, 2023
@Eric-Arellano
Copy link
Collaborator Author

@coruscating's excellent benchmarking led to us realizing that the slowdown is not only from the HTML page writing taking forever. It's also from the Python code to compute the navigation tree. That explains why Pytorch has a big slowdown specifically from this PR -- I was activating the new logic for Pytorch too.

So, we need to somehow disable Furo from running its code that initially computes its navigation tree. It is not sufficient for us to simply overwrite it after the matter. That fixes file sizes, but not build times.

@Eric-Arellano
Copy link
Collaborator Author

Eric-Arellano commented Jul 5, 2023

I could not get monkeypatching of Furo to work, which would allow us to dynamically disable Furo's Python code. So, while this PR fixes HTML page size, it does not fix build performance because we can't turn off Furo's Python code.

Instead of this PR, we have three options:

  1. Add a navigation_depth option to Furo. Hope they accept it and release promptly.
  2. Fork Furo entirely so that we can get rid of its problematic Python code.
  3. Change projects to stop having a dedicated page for methods.

I'm opening a PR for Furo now (update: pradyunsg/furo#674). And I'm exploring with the relevant devs if we can pursue #3 to reorganize the site.

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

Successfully merging this pull request may close these issues.

4 participants