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
Set up infrastructure for qiskit-tutorials migration #10443
Conversation
Pull Request Test Coverage Report for Build 5810755307
💛 - Coveralls |
One or more of the the following people are requested to review this:
|
QISKIT_SUPPRESS_PACKAGING_WARNINGS: Y | ||
PIP_CACHE_DIR: $(Pipeline.Workspace)/.pip | ||
QISKIT_CELL_TIMEOUT: 300 |
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 couldn't find what the deleted variables were used for. Didn't show up in search.
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.
You (accidentally?) removed it in Qiskit/qiskit-tutorials#1459.
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.
For posterity: my comment above is referring to QISKIT_CELL_TIMEOUT
. QISKIT_SUPPRESS_PACKAGING_WARNINGS
disappeared in #5619.
|
||
# Docs | ||
Sphinx>=6.0 | ||
qiskit-sphinx-theme~=1.13.0 |
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.
Note that this bumps the theme version from 1.11
tox.ini
Outdated
{[testenv:docs]deps} | ||
cvxpy | ||
networkx>=2.3 | ||
tweedledum>=1.0.0,<2.0.0; python_version < '3.11' and platform_system != "Darwin" |
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.
This works properly, even though GitHub renders the ;
as a comment
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.
Yeah, we can blame Microsoft for that one. Or maybe blame Tox for using a file format with little-to-no actual specification...
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'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.
.azure/tutorials-linux.yml
Outdated
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 |
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'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.
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.
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.
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.
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.
.azure/tutorials-linux.yml
Outdated
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/ |
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.
We probably want to maintain set -e
, and likely also add shopt -s failglob
here.
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.
... unless the idea was that on a failed tox
run we'd still attempt to produce a partial build artifact?
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.
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()
.
tox.ini
Outdated
{[testenv:docs]deps} | ||
cvxpy | ||
networkx>=2.3 | ||
tweedledum>=1.0.0,<2.0.0; python_version < '3.11' and platform_system != "Darwin" |
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.
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 <jake.lishman@ibm.com>
db1ce62
to
8c4ef73
Compare
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. |
3f5f363
to
3133f20
Compare
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.
3133f20
to
5ababc0
Compare
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.
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.
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.
-r requirements-optional.txt \ | ||
-r requirements-tutorials.txt \ | ||
-e . | ||
python -m pip install -U "tox<4.4.0" |
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.
There have been ~20 releases of tox since 4.4.0 in late January of this year. Is this pin still needed?
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.
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?
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 |
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.
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.
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.
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.
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.
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 <jake.lishman@ibm.com> * 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 <jake.lishman@ibm.com> (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 <jake.lishman@ibm.com> * 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 <jake.lishman@ibm.com> (cherry picked from commit df2ddca) Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
* 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 <jake.lishman@ibm.com> * 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 <jake.lishman@ibm.com>
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 andnbsphinx
, 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 CIWe 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 ourdocs/tutorials/
folder. Then, we run thejupyter nbconvert
command explained above.Once we move the tutorials, we can skip the
git clone
step since the tutorials will already be indocs/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.