Set up infrastructure for qiskit-tutorials migration by Eric-Arellano · Pull Request #10443 · Qiskit/qiskit

Eric-Arellano · 2023-07-18T12:45:34Z

Implements most of Qiskit/qiskit-metapackage#1724.

We won't move the content of the actual tutorials over until we switch the docs deployment from qiskit-metapackage to qiskit-terra. That avoids the content living in two places and keeps one source of truth. Instead, this PR sets up all the infrastructure so that we will only need to copy over the files.

Executing the notebooks

Our docs build will use nbsphinx_execute='never' by default. That keeps the doc build much faster for iteration.

Similar to qiskit-metapackage, the env var QISKIT_DOCS_BUILD_TUTORIALS can be used to instead re-execute the notebooks so their output is refreshed.

We want to make sure that Terra changes don't break the tutorials. So we have a CI job that executes the notebooks. It does that using jupyter nbconvert, rather than Sphinx and nbsphinx, because it's faster to avoid rebuilding all of our docs again.

Note that it's possible for Jupyter notebooks to include Sphinx elements like cross-references. That will already be handled by our normal docs run with nbsphinx_execute='never'—Sphinx will still evaluate the RST sections.

TBD: should published docs execute the tutorials?

That is, should our docs publish step set QISKIT_DOCS_BUILD_TUTORIALS to true? That is how we do it in the metapackage.

We don't need to answer this yet, though. It will be answered in Qiskit/qiskit-metapackage#1779.

Testing `qiskit-tutorials` in CI

We previously would git clone qiskit-tutorials and run a Sphinx docs build on the whole project.

Now, we still clone the repo, but then copy over the .ipynb files into our docs/tutorials/ folder. Then, we run the jupyter nbconvert command explained above.

Once we move the tutorials, we can skip the git clone step since the tutorials will already be in docs/tutorials.

The CI job will upload the updated tutorials as notebooks. That allows us to replace whichever tutorials we want with the update. This is useful for e.g. me as an M1 user to get the result of a tutorial using Tweedledum since I can't run it locally.

coveralls · 2023-07-18T13:15:37Z

Pull Request Test Coverage Report for Build 5810755307

0 of 0 changed or added relevant lines in 0 files are covered.
30 unchanged lines in 7 files lost coverage.
Overall coverage increased (+0.02%) to 87.265%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	93.76%
crates/qasm2/src/lex.rs	1	92.15%
qiskit/transpiler/preset_passmanagers/builtin_plugins.py	4	97.04%
qiskit/transpiler/preset_passmanagers/level0.py	4	93.75%
qiskit/transpiler/preset_passmanagers/level1.py	4	96.49%
qiskit/transpiler/preset_passmanagers/level2.py	6	94.5%
qiskit/transpiler/preset_passmanagers/level3.py	10	91.74%

Totals
Change from base Build 5787606820:	0.02%
Covered Lines:	74266
Relevant Lines:	85104

💛 - Coveralls

qiskit-bot · 2023-07-18T22:46:23Z

One or more of the the following people are requested to review this:

@Eric-Arellano
@javabster
@ElePT
@Qiskit/terra-core
@woodsp-ibm

Eric-Arellano · 2023-07-18T23:34:32Z

-      QISKIT_SUPPRESS_PACKAGING_WARNINGS: Y
      PIP_CACHE_DIR: $(Pipeline.Workspace)/.pip
-      QISKIT_CELL_TIMEOUT: 300


I couldn't find what the deleted variables were used for. Didn't show up in search.

You (accidentally?) removed it in Qiskit/qiskit-tutorials#1459.

For posterity: my comment above is referring to QISKIT_CELL_TIMEOUT. QISKIT_SUPPRESS_PACKAGING_WARNINGS disappeared in #5619.

Eric-Arellano · 2023-07-18T23:35:29Z

+
+# Docs
+Sphinx>=6.0
+qiskit-sphinx-theme~=1.13.0


Note that this bumps the theme version from 1.11

Eric-Arellano · 2023-07-18T23:35:54Z

+  {[testenv:docs]deps}
+  cvxpy
+  networkx>=2.3
+  tweedledum>=1.0.0,<2.0.0; python_version < '3.11' and platform_system != "Darwin"


This works properly, even though GitHub renders the ; as a comment

Yeah, we can blame Microsoft for that one. Or maybe blame Tox for using a file format with little-to-no actual specification...

jakelishman

I've left a couple of comments on the PR in its current form for posterity, but since we don't know when Eric will be back I'm going to take over this PR and prep it to land, including making a few modifications based on the comments. If I'd made a mistake or anything, we'll have the review history to help disentangle them.

jakelishman · 2023-08-07T11:59:34Z

+          mv qiskit-tutorials/tutorials/algorithms/** docs/tutorials/algorithms
+          mv qiskit-tutorials/tutorials/circuits/** docs/tutorials/circuits
+          mv qiskit-tutorials/tutorials/circuits_advanced/** docs/tutorials/circuits_advanced
+          mv qiskit-tutorials/tutorials/operators/** docs/tutorials/operators


I'm not sure why we're using ** here which can resolve at arbitrary depth (and consequently potentially fail if there's nested directories that aren't created in the outdir yet), in place of mv qiskit-tutorials/tutorials/{algorithms,circuits,circuits_advanced,operators} docs/tutorials. The latter matches more with what I expected this script to be doing, but I'm not sure if there was a particular intent with the other form, especially since later down, we're using brace expansion.

If there was a special reason for **, we probably need a comment and potentially to shopt -s failglob globstar at the top to make it behave more safely.

For posterity: the reason was because this PR currently sets up those directories to have place-holder tutorials in them, so they're not empty and we can't use mv directly.

5ababc0 changes this to explicitly test that the directories have the form that we expect, and rejects the attempt to copy files in if they're wrong, so CI will defensively fail if we try and change the structure without updating the scripts.

jakelishman · 2023-08-07T12:01:52Z

-          set -e
-          cd qiskit-tutorials
-          sphinx-build -b html . _build/html
-        env:
-          QISKIT_PARALLEL: False
+          tox -e tutorials
+          mkdir -p updated-tutorials/{algorithms,circuits,circuits_advanced,operators}
+          mv docs/tutorials/algorithms/*.nbconvert.ipynb updated-tutorials/algorithms/
+          mv docs/tutorials/circuits/*.nbconvert.ipynb updated-tutorials/circuits/
+          mv docs/tutorials/circuits_advanced/*.nbconvert.ipynb updated-tutorials/circuits_advanced/
+          mv docs/tutorials/operators/*.nbconvert.ipynb updated-tutorials/operators/


We probably want to maintain set -e, and likely also add shopt -s failglob here.

... unless the idea was that on a failed tox run we'd still attempt to produce a partial build artifact?

In the big refactor (5ababc0) I did of all this to make it more resilient, the upshot is that a partial success will now upload the partial artifacts, whereas before the upload job couldn't have triggered because of the implicit condition: succeeded().

jakelishman · 2023-08-07T12:04:59Z

+  {[testenv:docs]deps}
+  cvxpy
+  networkx>=2.3
+  tweedledum>=1.0.0,<2.0.0; python_version < '3.11' and platform_system != "Darwin"


Yeah, we can blame Microsoft for that one. Or maybe blame Tox for using a file format with little-to-no actual specification...

This first commit is a rebase of Eric's initial PR as of db1ce62 onto `main`, fixing up some changes caused by the CI infrastructure changing a bit since the PR was first opened. Co-authored-by: Jake Lishman <[email protected]>

jakelishman · 2023-08-07T12:47:29Z

I'll just wait and see if I broke anything in that rebase of the prior PR state, then make the changes I'd flagged in review.

This moves much of the fetch- and process-related code into separate scripts that assert far more about the directory structure, and fail if they do not match the assumptions. We don't want to accidentally get out-of-sync while we're changing things and end up with a tutorials job that isn't really doing its job without us noticing. The tutorials-fetching script can now also be re-used in a separate GitHub Actions workflow that will handle the full tutorials-included documentation build and deployment in the future. The notebook-convertion step is moved into Python space, using `nbconvert` as a library in order to parallelise the build process for the tutorials, and to allow CI and developers calling `tox` directly to specify the output directories for the built tutorials.

This reorganises the tutorial "conversion" utility to make it clearer that what it's actually doing is just executing the tutorials. The script itself is changed to default to editing the files inplace, while the `tox` job is updated to write the files into a special directory, making it easier to clean up a dirty build directory and making it so subsequent local executions will not pick up the converted files.

mtreinish

This LGTM, this is a good starting point. A couple of questions inline, but nothing blocking feel free to enqueue this for merging if there's nothing to change.

mtreinish · 2023-08-09T10:49:25Z

-            -r requirements-optional.txt \
-            -r requirements-tutorials.txt \
-            -e .
+          python -m pip install -U "tox<4.4.0"


There have been ~20 releases of tox since 4.4.0 in late January of this year. Is this pin still needed?

Probably not, but the pin is just inherited because we haven't reverted #9460 yet. Maybe best do that in one go in a follow-up?

mtreinish · 2023-08-09T10:57:57Z

+for component in "$@"; do
+    echo "Getting tutorials from '${component}'"
+
+    if [[ ! -d "${indir}/${component}" ]]; then
+        echo "Component '${component}' not in tutorials repository." >&2
+        exit 3
+    fi
+    if [[ -d "${outdir}/${component}" && -f "${outdir}/${component}/placeholder.ipynb" ]]; then
+        rm "${outdir}/${component}/placeholder.ipynb"
+        if [[ -z "$(ls -A "${outdir}/${component}")" ]]; then
+            rm -r "${outdir}/${component}"
+        else
+            echo "Directory '${outdir}/${component}' contains files other than the placeholder. This script needs updating." >&2
+            exit 4
+        fi
+    else
+        echo "Directory '${outdir}/${component}' does not exist, or has no placeholder. This script needs updating." >&2
+        exit 5
+    fi
+    mv "${indir}/${component}" "${outdir}/${component}"
+done


Could this just be done as something like: rm -rf $outdir && mv $indir $outdir ? Like I guess it's possible that we'd remove a component from the tutorials before we move them into terra, but it seems pretty unlikely.

We could, but I'm more concerned with us accidentally leaving the placeholder tutorial files in place during the actual migration, so we'll be building nonsense into our docs. Having rm -rf made me nervous that it'll mask mistakes. The components subset of this is mostly just inherited from the previous version of this PR, which separated them out like this as well.

There was a worry that not being able to configure these would make it more unpleasant to use `tox` for the jobs locally.

mtreinish

LGTM, thanks for the updates.

* Add infrastructure for building tutorials This first commit is a rebase of Eric's initial PR as of db1ce62 onto `main`, fixing up some changes caused by the CI infrastructure changing a bit since the PR was first opened. Co-authored-by: Jake Lishman <[email protected]> * Harden tutorials Azure job This moves much of the fetch- and process-related code into separate scripts that assert far more about the directory structure, and fail if they do not match the assumptions. We don't want to accidentally get out-of-sync while we're changing things and end up with a tutorials job that isn't really doing its job without us noticing. The tutorials-fetching script can now also be re-used in a separate GitHub Actions workflow that will handle the full tutorials-included documentation build and deployment in the future. The notebook-convertion step is moved into Python space, using `nbconvert` as a library in order to parallelise the build process for the tutorials, and to allow CI and developers calling `tox` directly to specify the output directories for the built tutorials. * Retarget tutorial-conversion utility as executor This reorganises the tutorial "conversion" utility to make it clearer that what it's actually doing is just executing the tutorials. The script itself is changed to default to editing the files inplace, while the `tox` job is updated to write the files into a special directory, making it easier to clean up a dirty build directory and making it so subsequent local executions will not pick up the converted files. * Allow configuration of tutorials execution There was a worry that not being able to configure these would make it more unpleasant to use `tox` for the jobs locally. --------- Co-authored-by: Jake Lishman <[email protected]> (cherry picked from commit df2ddca)

* Add infrastructure for building tutorials This first commit is a rebase of Eric's initial PR as of db1ce62 onto `main`, fixing up some changes caused by the CI infrastructure changing a bit since the PR was first opened. Co-authored-by: Jake Lishman <[email protected]> * Harden tutorials Azure job This moves much of the fetch- and process-related code into separate scripts that assert far more about the directory structure, and fail if they do not match the assumptions. We don't want to accidentally get out-of-sync while we're changing things and end up with a tutorials job that isn't really doing its job without us noticing. The tutorials-fetching script can now also be re-used in a separate GitHub Actions workflow that will handle the full tutorials-included documentation build and deployment in the future. The notebook-convertion step is moved into Python space, using `nbconvert` as a library in order to parallelise the build process for the tutorials, and to allow CI and developers calling `tox` directly to specify the output directories for the built tutorials. * Retarget tutorial-conversion utility as executor This reorganises the tutorial "conversion" utility to make it clearer that what it's actually doing is just executing the tutorials. The script itself is changed to default to editing the files inplace, while the `tox` job is updated to write the files into a special directory, making it easier to clean up a dirty build directory and making it so subsequent local executions will not pick up the converted files. * Allow configuration of tutorials execution There was a worry that not being able to configure these would make it more unpleasant to use `tox` for the jobs locally. --------- Co-authored-by: Jake Lishman <[email protected]> (cherry picked from commit df2ddca) Co-authored-by: Eric Arellano <[email protected]>

* Add infrastructure for building tutorials This first commit is a rebase of Eric's initial PR as of db1ce62 onto `main`, fixing up some changes caused by the CI infrastructure changing a bit since the PR was first opened. Co-authored-by: Jake Lishman <[email protected]> * Harden tutorials Azure job This moves much of the fetch- and process-related code into separate scripts that assert far more about the directory structure, and fail if they do not match the assumptions. We don't want to accidentally get out-of-sync while we're changing things and end up with a tutorials job that isn't really doing its job without us noticing. The tutorials-fetching script can now also be re-used in a separate GitHub Actions workflow that will handle the full tutorials-included documentation build and deployment in the future. The notebook-convertion step is moved into Python space, using `nbconvert` as a library in order to parallelise the build process for the tutorials, and to allow CI and developers calling `tox` directly to specify the output directories for the built tutorials. * Retarget tutorial-conversion utility as executor This reorganises the tutorial "conversion" utility to make it clearer that what it's actually doing is just executing the tutorials. The script itself is changed to default to editing the files inplace, while the `tox` job is updated to write the files into a special directory, making it easier to clean up a dirty build directory and making it so subsequent local executions will not pick up the converted files. * Allow configuration of tutorials execution There was a worry that not being able to configure these would make it more unpleasant to use `tox` for the jobs locally. --------- Co-authored-by: Jake Lishman <[email protected]>

Eric-Arellano changed the title ~~[wip] Set up infrastructure for qiskit-tutorials migration~~ Set up infrastructure for qiskit-tutorials migration Jul 18, 2023

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

Eric-Arellano requested review from a team, ElePT and woodsp-ibm as code owners July 18, 2023 22:46

Eric-Arellano commented Jul 18, 2023

View reviewed changes

This was referenced Jul 19, 2023

Add no-optionals and full-optionals test runs #8967

Merged

Add docs infrastructure qiskit-community/qiskit-algorithms#13

Merged

jakelishman reviewed Aug 7, 2023

View reviewed changes

Add infrastructure for building tutorials

8c4ef73

This first commit is a rebase of Eric's initial PR as of db1ce62 onto `main`, fixing up some changes caused by the CI infrastructure changing a bit since the PR was first opened. Co-authored-by: Jake Lishman <[email protected]>

jakelishman force-pushed the add-tutorials branch from db1ce62 to 8c4ef73 Compare August 7, 2023 12:37

jakelishman force-pushed the add-tutorials branch 2 times, most recently from 3f5f363 to 3133f20 Compare August 7, 2023 20:58

jakelishman force-pushed the add-tutorials branch from 3133f20 to 5ababc0 Compare August 7, 2023 21:06

mtreinish previously approved these changes Aug 9, 2023

View reviewed changes

Allow configuration of tutorials execution

6e3f4ff

There was a worry that not being able to configure these would make it more unpleasant to use `tox` for the jobs locally.

jakelishman dismissed mtreinish’s stale review via 6e3f4ff August 9, 2023 15:02

mtreinish approved these changes Aug 9, 2023

View reviewed changes

mtreinish added this pull request to the merge queue Aug 9, 2023

Merged via the queue into Qiskit:main with commit df2ddca Aug 9, 2023

Eric-Arellano deleted the add-tutorials branch August 9, 2023 19:41

jakelishman mentioned this pull request Aug 11, 2023

Epic - Implementation of RFC 0011: Plan to rename Qiskit/qiskit-terra repo to Qiskit/qiskit Qiskit/RFCs#41

Closed

21 tasks

mtreinish added stable backport potential Make Mergify open a backport PR to the most recent stable branch on merge. Changelog: None Do not include in the GitHub Release changelog. labels Aug 14, 2023

mergify Bot mentioned this pull request Aug 14, 2023

Set up infrastructure for qiskit-tutorials migration (backport #10443) #10625

Merged

Conversation

Eric-Arellano commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Executing the notebooks

TBD: should published docs execute the tutorials?

Testing qiskit-tutorials in CI

Uh oh!

coveralls commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 5810755307

💛 - Coveralls

Uh oh!

qiskit-bot commented Jul 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelishman Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelishman Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelishman commented Aug 7, 2023

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Eric-Arellano commented Jul 18, 2023 •

edited

Loading

Testing `qiskit-tutorials` in CI

coveralls commented Jul 18, 2023 •

edited

Loading

jakelishman Aug 7, 2023 •

edited

Loading

jakelishman Aug 7, 2023 •

edited

Loading