CI: sphinx-build doesn't fail on notebook execution error.

[rossbar]

It turns out that execution errors from the myst-nb extension only raise sphinx warnings, not errors, so the sphinx-build process is treated as completed successfully even if a notebook was not executed entirely successfully!

There are several ways we could handle this:

1. Have sphinx-build treat warnings as errors with SPHINXOPTS=-W
2. Push for a config option in myst-nb to treat execution failures as sphinx errors.
3. Re-activate nbval (or similar) in CI

The only problem with option 1 is that it may prove to be too stringent, as the build will fail on any warning. Option 2 has already been raised upstream: executablebooks/MyST-NB#248. Reading a bit more about nbval, it seems like a nice option with more testing features than just the lax mode (which is essentially what we're trying to achieve with the sphinx build, without needing to build twice).

[melissawm]

+1 on nbval - seems to be the best option in terms of features and flexibility.

[rossbar]

+1 on nbval - seems to be the best option in terms of features and flexibility.

I agree, it seems like it would give us an avenue to really beef up testing. I was initially concerned with build times (executing all the notebooks multiple times) but with parallel CI jobs this is less of a concern. It will probably require an additional .md->.ipynb step for a CI workflow with nbval, but that's no big deal. I'm also pursuing option 1 in #52, as I think having a service that makes all the sphinx warnings very visible would be beneficial, but I'm +1 on adding nbval as well.

Thanks @melissawm for the original suggestion and also @MridulS for the intro to nbval!

[bsipocz]

I get make error locally for a CellExecutionError, so this issue is likely resolved now.

[rossbar]

I agree that this is was mostly resolved, though the "elevate-all-sphinx-warnings-to-errors" approach certainly has it's drawbacks, especially since the full tracebacks for cell execution failures are not always immediately available (as discussed elsewhere).

We had discussed adding nbval to the CI above, which I've taken a stab at in #132. That should be sufficient to fully close this issue for good!