Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
5.14k stars 2.35k forks source link

Run doctests to validate code examples in documentation and limit jupyter-execute to visualization examples #7661

Open mtreinish opened 2 years ago

mtreinish commented 2 years ago

What is the expected enhancement?

Right now in qiskit-terra's api documentation we tend to rely heavily on jupyter execute for including inline code examples. Even when there is not output visualization to include with the code, for example:

https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/pulse/builder.py#L383-L393

We should typically be avoiding jupyter-execute in the general case because it adds complexity to the build process as a new jupyter kernel is launched for each document that has the directive to run the code which can significantly slow down the build process. It also has weird interactions with parallel sphinx builds which means we can't rely on parallel sphinx builds. Ideally we should only be using jupyter-execute where we have an output visualization we want to display with the code example as this is what the directive provides over a normal code block.

However, while we should decrease our reliance on jupyter-execute we should still be executing our code examples in CI to verify everything in our documentation is valid. Instead we should investigate using something like sphinx's built in doctest builder:

https://www.sphinx-doc.org/en/master/usage/extensions/doctest.html

(or a similar framework) to verify code blocks function as expected but without adding complexity or runtime to the main sphinx html builds.

HuangJunye commented 2 years ago

This is a large issues that probably should be borken down into many and fix gradually. I just found https://github.com/Qiskit/qiskit-terra/blob/b2f0964f2c43b7f6104db9e28191210cc6a71bd4/qiskit/pulse/builder.py has many usage of jupyter-execute that do not have visualization. Do we have the doctest setup already to make the change?

HuangJunye commented 2 years ago

I was trying to use doctest in https://github.com/Qiskit/qiskit-terra/pull/8121 but I couldn't get it to actually test the code. Probably I should investigate it further but I see a lot of advantages of using jupyter-execute. It's really simple to use and the behavior of executing is exactly the same as jupyter notebook which is the major usage environment for our users. And it can display rich output.

I wonder whether there is something we can do to contribute to https://github.com/jupyter/jupyter-sphinx to reduce the building time.

While reducing the overall sphinx build would be the ultimate goal, I think it would be a great improvement already we can configure it to only execute codes that have been modified. This will greatly accelerate documentation development time. Currently wherever I modified code in a jupyter-execute block it triggers a complete rebuild of the documentation which take ages.

jakelishman commented 2 years ago

Sphinx should only be executing code blocks that have been modified already, that's a big part of its job. It should only trigger a complete re-build if it doesn't have a successful base build to go off, such as if the previous build errorred out.

The jupyter-sphinx extension also provides a .. jupyter-kernel:: command, which in theory we can use to cause code blocks to share the same kernel (somewhat reducing the build time), but that's a stop-gap measure at best. It's just begging for huge problems with parallel builds, but more fundamentally, we shouldn't be using Jupyter for simple, text-only comparisons. jupyter-execute is for including Jupyter-enhanced visualisations in the build output. Text-only things, where we actually want to have the output be validated as well should be doctest (though I will say that doctest can be annoying with floating-point mathematics and custom-repr objects).

There's no need to update #8121 to use doctest. We don't need it there, and it'll complicate an already large PR. If we do add it, it should be as a separate PR. If you just put doctest directives in directly, it probably didn't work because it needs a different extension loading in the Sphinx conf.py file as well.

HuangJunye commented 2 years ago

I did modify conf.py to include sphinx.ext.doctest extention but it didn't work for some reasons. But you are right, I should not try to use doctest in #8121 to complicate things. I use jupyter-execute instead.

HuangJunye commented 1 year ago

9346 has removed the usage of jupyter-sphinx extension and replaced the jupyter-execute directives with plot directive from matplotlib.sphinxext.plot_directive for visualization or simply code-block for other code examples. We should use doctest to cover tests for code examples that were changed to code-block.

HuangJunye commented 1 year ago

We need to add doctest to CI as we are starting to use doctest and testcode directives in documentation. See https://github.com/Qiskit/qiskit-terra/pull/9549/ and https://github.com/Qiskit/qiskit-terra/pull/9716