[on hold] Vendor Furo so that we can change its Python code #463

Eric-Arellano · 2023-07-06T13:27:50Z

Motivation

We need to change Furo's Python code to work around #328. Its call to _compute_navigation_tree with a maxdepth=-1 results in a 3.5 hour build for Terra because Terra has so many pages. While #451 was able to fix the HTML page sizes being huge, it did not fix the build performance because Furo's Python code would still run, and then only be overridden by our code after.

I have not been able to figure out how to disable/override Python code from an inherited Sphinx theme. I couldn't get monkeypatching to work either. This line causes Sphinx to run all of Furo's setup, including the slow _compute_navigation_tree line:

qiskit_sphinx_theme/src/qiskit_sphinx_theme/theme/qiskit-sphinx-theme/theme.conf

Line 2 in 10fff40

inherit = furo

Pre-processing the TOC Tree before it gets to Furo in #474 helped, but it still took 1.5 hours (vs. 3.5 hours), which is too slow. We're planning on restructuring Terra docs to have fewer HTML pages. But even with that, we still will want to make local docs development faster. With local docs, we want to set the navigation_depth to 1 and only use a value like -1 in production builds.

Our hope is that pradyunsg/furo#674 will be accepted, but there is no guarantee and the timeline is out of our control. So, this fork unblocks allowing us to tweak Furo's Python code to avoid the call to _compute_navigation_tree.

If pradyunsg/furo#674 is merged, we will strongly consider reverting this PR.

Fork strategy

We keep a strong emphasis on separating what is base Furo vs. our customization, along with keeping the # of customizations as low as possible.

This PR adds 58 new files, but 57 of them should never have custom changes from us. We make that clear with comments at the top of each file like // This file is vendored from Furo. It should have no changes.. For example, styling changes should never happen in the folder assets/styles/furo, only in our custom files at the root of assets/styles.

This makes it much easier to stay in sync with upstream Furo.

How to sync

We have a new file furo-sync.txt where we write the commit from Furo from the last time we synced.

As before, when we sync, we look closely at every change Furo has made and update our vendored code. This is the same before, only now we need to check more files than before.

Code tweak: only run Furo setup when relevant

We change furo_py/__init__.py to skip its setup when Pytorch is used and when the docs builder is not HTML (e.g. docstest).

Also closes #314

Closes #314 because this hooks up our CSS and JavaScript file to Furo's _add_asset_hashes that adds hashes to the file names so that they are invalidated properly on changes.

## Adds Furo license It's extra safe to do this because we vendor about 6 files from Furo. We use the prior art of Pytorch of including the license directly in `LICENSE` at the bottom. Pytorch is also an MIT license and we substantially vendored it, so it's a similar setup. ## Fixes LICENSE inclusion The switch to `pyproject.toml` accidentally changed us including the LICENSE file to instead have the text `Apache 2`. ## Adds copyright file headers We should have been doing this from the start because Qiskit projects are expected to put copyright headers on every file. This becomes especially useful if we more substantially vendor Furo code in #463 to make clear what is what. All files that are originally written by Qiskit contributors use the standard Apache 2 license we already use in this project. Meanwhile, files that are vendored from Furo now use this standard notice: > This file is vendored from Furo (created by Pradyun Gedam) and used under the MIT license. Even though the MIT license does not require file headers, and Furo itself does not use it, it is a good practice to make clear what originally comes from Furo. ## Also renames `search_sidebar.html` This file is vendored from Furo, but we had it under `custom_templates/search_sidebar.html` rather than the original `sidebar/search.html`. This PR renames the file so that we more properly attribute Furo and the MIT license.

## Adds Furo license It's extra safe to do this because we vendor about 6 files from Furo. We use the prior art of Pytorch of including the license directly in `LICENSE` at the bottom. Pytorch is also an MIT license and we substantially vendored it, so it's a similar setup. ## Fixes LICENSE inclusion The switch to `pyproject.toml` accidentally changed us including the LICENSE file to instead have the text `Apache 2`. ## Adds copyright file headers We should have been doing this from the start because Qiskit projects are expected to put copyright headers on every file. This becomes especially useful if we more substantially vendor Furo code in Qiskit#463 to make clear what is what. All files that are originally written by Qiskit contributors use the standard Apache 2 license we already use in this project. Meanwhile, files that are vendored from Furo now use this standard notice: > This file is vendored from Furo (created by Pradyun Gedam) and used under the MIT license. Even though the MIT license does not require file headers, and Furo itself does not use it, it is a good practice to make clear what originally comes from Furo. ## Also renames `search_sidebar.html` This file is vendored from Furo, but we had it under `custom_templates/search_sidebar.html` rather than the original `sidebar/search.html`. This PR renames the file so that we more properly attribute Furo and the MIT license.

…467) ## Adds Furo license It's extra safe to do this because we vendor about 6 files from Furo. We use the prior art of Pytorch of including the license directly in `LICENSE` at the bottom. Pytorch is also an MIT license and we substantially vendored it, so it's a similar setup. ## Fixes LICENSE inclusion The switch to `pyproject.toml` accidentally changed us including the LICENSE file to instead have the text `Apache 2`. ## Adds copyright file headers We should have been doing this from the start because Qiskit projects are expected to put copyright headers on every file. This becomes especially useful if we more substantially vendor Furo code in #463 to make clear what is what. All files that are originally written by Qiskit contributors use the standard Apache 2 license we already use in this project. Meanwhile, files that are vendored from Furo now use this standard notice: > This file is vendored from Furo (created by Pradyun Gedam) and used under the MIT license. Even though the MIT license does not require file headers, and Furo itself does not use it, it is a good practice to make clear what originally comes from Furo. ## Also renames `search_sidebar.html` This file is vendored from Furo, but we had it under `custom_templates/search_sidebar.html` rather than the original `sidebar/search.html`. This PR renames the file so that we more properly attribute Furo and the MIT license.

Eric-Arellano · 2023-07-07T18:37:50Z

src/qiskit_sphinx_theme/__init__.py

@@ -68,26 +68,13 @@ def remove_thebe_if_not_needed(


 def activate_themes(app: sphinx.application.Sphinx, config: sphinx.config.Config) -> None:
-    if config.html_theme == "qiskit":
-        # We set a low priority so that our Qiskit CSS file overrides Furo.
-        app.add_css_file("styles/furo.css", 100)


We no longer need to add furo.css because it gets included in qiskit-sphinx-theme.css thanks to Webpack.

Eric-Arellano · 2023-07-07T18:38:20Z

src/qiskit_sphinx_theme/__init__.py


-
-def remove_furo_js(


No need to do this anymore.

Eric-Arellano · 2023-07-07T18:38:37Z

src/qiskit_sphinx_theme/assets/styles/_layout.scss

-// Remove once https://github.com/pradyunsg/furo/commit/c8b51d09af3dcaac3046f7e761119e9d1b7c9e37
-// is released.
-article {
-  overflow-wrap: break-word;
-}


We are using a commit from Furo that already includes this change.

Eric-Arellano · 2023-07-07T18:38:54Z

src/qiskit_sphinx_theme/assets/styles/furo/_index.sass

+/* This file is vendored from Furo (created by Pradyun Gedam) and used under the MIT license.
+ *
+ * It's only changes are:
+ *
+ *   1. Renaming the file from `furo.sass` to `_index.sass`.
+ *   2. Including furo-extensions.sass directly in it.
+ */


This comment is worth reading.

But every other file in assets/styles/furo is 100% identical to Furo outside of the copyright comment at the top.

Eric-Arellano · 2023-07-07T18:40:13Z

src/qiskit_sphinx_theme/assets/styles/qiskit-sphinx-theme.scss

+// Import the base Furo style
+@import "furo";


This is how we import Furo's style now.

The next file in this PR, furo_py/__init__.py is really important to review btw.

Eric-Arellano · 2023-07-07T18:41:11Z

src/qiskit_sphinx_theme/furo_py/__init__.py

+# This file is vendored from Furo (created by Pradyun Gedam) and used under the MIT license.
+#
+# When making changes, surround it with `QISKIT CHANGE: start` and `QISKIT CHANGE: end` comments.
+# We also add MyPy type ignores.


This file is copied exactly, other than the parts where I put QISKIT CHANGE: start.

Eric-Arellano · 2023-07-07T18:42:30Z

src/qiskit_sphinx_theme/furo_py/navigation.py

+# This file is vendored from Furo (created by Pradyun Gedam) and used under the MIT license.
+# It should have no changes.


Nothing to review here.

Eric-Arellano · 2023-07-07T18:42:46Z

src/qiskit_sphinx_theme/theme/qiskit-sphinx-theme/components/edit-this-page.html

+{#-
+  This file is vendored from Furo (created by Pradyun Gedam) and used under the MIT license.
+  It should have no changes.
+-#}


Nothing to review in these new HTML files.

Eric-Arellano · 2023-07-07T18:43:15Z

src/qiskit_sphinx_theme/theme/qiskit-sphinx-theme/theme.conf

@@ -11,8 +11,9 @@
 # that they have been altered from the originals.


This file is basically copy-and-paste from Furo.

Eric-Arellano · 2023-07-07T18:43:33Z

webpack.config.js

-        test: /\.scss$/,
+        test: /\.s[ac]ss$/i,


Furo uses Sass files, so we need to compile both Sass and SCSS.

Eric-Arellano · 2023-07-10T13:30:35Z

Going to close for now. We only want to set navigation_depth for dev builds. Prod should still use -1. Qiskit devs have signed off on the plan to speed things up by reducing the # of HTML pages for API docs.

Hopefully pradyunsg/furo#674 is merged. If not, we can speed up dev builds by using sphinx-remove-toctrees. Fully vendoring Furo is not worth the cost solely to speed up dev builds. While navigation_depth is nicer than sphinx-remove-toctrees, the latter is fine enough for dev builds.

github-actions bot had a problem deploying to Preview July 6, 2023 13:28 Failure

github-actions bot deployed to Preview July 6, 2023 13:37 View deployment

Eric-Arellano added the needs cherrypick label Jul 6, 2023

Eric-Arellano mentioned this pull request Jul 6, 2023

Add copyright headers along with Furo license #464

Merged

Eric-Arellano force-pushed the EA/fully-vendor-furo branch from a2ce73d to 0abe5f3 Compare July 6, 2023 20:13

github-actions bot deployed to Preview July 6, 2023 20:13 View deployment

Eric-Arellano mentioned this pull request Jul 6, 2023

Add copyright headers along with Furo license (Cherry-pick of #464) #467

Merged

Eric-Arellano force-pushed the EA/fully-vendor-furo branch from 0abe5f3 to fbdb341 Compare July 6, 2023 20:35

github-actions bot deployed to Preview July 6, 2023 20:35 View deployment

Eric-Arellano mentioned this pull request Jul 7, 2023

Add navigation_depth to html_theme_options pradyunsg/furo#674

Open

Eric-Arellano added 2 commits July 7, 2023 12:05

Vendor Furo

ba25464

Move Furo init code to normal top level, not in config-inited

85c185e

Eric-Arellano force-pushed the EA/fully-vendor-furo branch from fbdb341 to 85c185e Compare July 7, 2023 18:16

github-actions bot deployed to Preview July 7, 2023 18:16 View deployment

Another approach to skipping setup

d97f4a0

github-actions bot had a problem deploying to Preview July 7, 2023 18:24 Failure

Eric-Arellano changed the title ~~[DO NOT MERGE] Experiment with fully vendoring Furo~~ Vendor Furo so that we can change its Python code Jul 7, 2023

Thou shalt not use "not not"

37fdd07

github-actions bot deployed to Preview July 7, 2023 18:31 View deployment

Eric-Arellano marked this pull request as ready for review July 7, 2023 18:34

Eric-Arellano commented Jul 7, 2023

View reviewed changes

Fix docs

660df65

github-actions bot had a problem deploying to Preview July 7, 2023 18:44 Failure

github-actions bot had a problem deploying to Preview July 7, 2023 18:50 Failure

Eric-Arellano changed the title ~~Vendor Furo so that we can change its Python code~~ [on hold] Vendor Furo so that we can change its Python code Jul 7, 2023

Eric-Arellano added the on hold On hold for now label Jul 7, 2023

Eric-Arellano closed this Jul 10, 2023

Eric-Arellano deleted the EA/fully-vendor-furo branch July 10, 2023 13:30

Eric-Arellano removed the needs cherrypick label Jul 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[on hold] Vendor Furo so that we can change its Python code #463

[on hold] Vendor Furo so that we can change its Python code #463

Eric-Arellano commented Jul 6, 2023 •

edited

Loading

Eric-Arellano Jul 7, 2023

Eric-Arellano Jul 7, 2023

Eric-Arellano Jul 7, 2023

Eric-Arellano Jul 7, 2023

Eric-Arellano Jul 7, 2023

Eric-Arellano Jul 7, 2023

Eric-Arellano Jul 7, 2023

Eric-Arellano Jul 7, 2023

Eric-Arellano Jul 7, 2023

Eric-Arellano Jul 7, 2023

Eric-Arellano commented Jul 10, 2023

		# This file is vendored from Furo (created by Pradyun Gedam) and used under the MIT license.
		# It should have no changes.

		@@ -11,8 +11,9 @@
		# that they have been altered from the originals.

[on hold] Vendor Furo so that we can change its Python code #463

[on hold] Vendor Furo so that we can change its Python code #463

Conversation

Eric-Arellano commented Jul 6, 2023 • edited Loading

Motivation

Fork strategy

How to sync

Code tweak: only run Furo setup when relevant

Also closes #314

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano commented Jul 10, 2023

Eric-Arellano commented Jul 6, 2023 •

edited

Loading