✨ NEW: Add execution_fail_on_error configuration

[jakevdp]

This is a bare-bones implementation of the flag discussed in #248. This PR is incomplete; in particular a few things remain to be addressed:

• is this the right name/mechanism for such a flag?
• is the reported error message detailed enough? Should it include the path?
• docs must be updated

I'd be happy for any input on this, and I'm happy to iterate on the design given any feedback or recommendations. Thanks!

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

[codecov]

Codecov Report

Merging #296 (75b8d81) into master (6462243) will decrease coverage by 0.33%.
The diff coverage is 52.94%.

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
- Coverage   87.48%   87.14%   -0.34%     
==========================================
  Files          12       12              
  Lines        1334     1346      +12     
==========================================
+ Hits         1167     1173       +6     
- Misses        167      173       +6     

Flag	Coverage Δ
pytests	87.14% <52.94%> (-0.34%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
myst_nb/execution.py	79.73% <50.00%> (-2.66%)	⬇️
myst_nb/__init__.py	87.16% <100.00%> (+0.06%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6462243...9607206. Read the comment docs.

[choldgraf]

The flag name seems reasonable to me! One thought - do we imagine any other activity also triggering this "force a failure" mode? If not, then we could also just be more explicit and call this something like execute_stop_on_error or something 🤷‍♂️

For the implementation, is there any behavior of the Sphinx logger that can force it to fail? (e.g., I would have assumed the critical( would have made Sphinx fail by default, but maybe I am hoping to much from Sphinx 😄 . If not, then this seems fine to me.

[jakevdp]

is there any behavior of the Sphinx logger that can force it to fail? (e.g., I would have assumed the critical( would have made Sphinx fail by default)

Great thought - I gave it a try, but it doesn't look like this causes sphinx to fail without the -W flag.

Looking at the sphinx app source (https://github.com/sphinx-doc/sphinx/blob/master/sphinx/application.py) it seems that build failures result in regular Python exceptions, so I think that is sphinx's preferred mechanism for a hard build failure.

It might be worth defining a custom exception type (e.g. myst_nb.ExecutionError or something like that) and using that... what do you think?

[jakevdp]

One other thing: I know that nbsphinx does fail the build on an execution error; I'm going to see look into what mechanism they use to do that.

[jakevdp]

Here's how nbsphinx halts the build on an execution error: it catches and re-raises the CellExecutionError from nbconvert: https://github.com/spatialaudio/nbsphinx/blob/2432715fbe5bceda46835feff010596be2f291b2/src/nbsphinx.py#L1020-L1032

        try:
            rststring, resources = exporter.from_notebook_node(nb, resources)
        except nbconvert.preprocessors.execute.CellExecutionError as e:
            lines = str(e).split('\n')
            lines[0] = 'CellExecutionError in {}:'.format(
                env.doc2path(env.docname, base=None))
            lines.append("You can ignore this error by setting the following "
                         "in conf.py:\n\n    nbsphinx_allow_errors = True\n")
            raise NotebookError('\n'.join(lines))
        except Exception as e:
            raise NotebookError(type(e).__name__ + ' in ' +
                                env.doc2path(env.docname, base=None) + ':\n' +
                                str(e))

Perhaps we should follow their lead and call the flag something like mystnb_allow_errors?

[jakevdp]

In the case of myst_nb, notebook exeuction happens via jupyter_cache, which catches and returns cell execution errors: https://github.com/executablebooks/jupyter-cache/blob/b4026a25266cf9eca5121f3f9ab4d09b3beeb88a/jupyter_cache/executors/utils.py#L49-L62

    with timer:
        try:
            executenb(
                nb,
                cwd=cwd,
                timeout=timeout,
                allow_errors=allow_errors,
                record_timing=False,
            )
        except (CellExecutionError, CellTimeoutError) as err:
            error = err
            exc_string = "".join(traceback.format_exc())


    return ExecutionResult(nb, timer.last_split, error, exc_string)

So to do the equivalent of nbsphinx, we should raise a CellExecutionError when result.err is not None.

[chrisjsewell]

Hey yeh thanks @jakevdp, thats what I was just about to point out; all execution is done via jupyter-cache, which uses nbclient (the use of nbconvert is essentially deprecated)

[jakevdp]

I changed to execution_fail_on_error, which I think is more descriptive.

[jakevdp]

I've been testing this change with the JAX docs in jax-ml/jax#5537; the most recent push shows the intended behavior, i.e. the cell contents and error are reported directly in the build log, which means the problem can be easily identified from the output of a CI documentation build.

[jakevdp]

Added some docs... I think this is ready for a review.

Code coverage delta is not great, but that is primarily because I added some conditions to error handling blocks that already have no tests. Let me know if you'd like me to invest some time in trying to add coverage for those cases.

[choldgraf]

Just one quick comment, but other than that this looks great to me :-)

[choldgraf]

do we have access to the name of the file being executed here? Perhaps the error string could be something like:

f"""
Execution failed for file: {file-name}
{str(result.err))
"""

(if it is already obvious from your implementation which file the error came from, then disregard this!)

[jakevdp]

Great idea! I added the file path to the error message. The sphinx-build error now looks something like this:

Exception occurred:
  File "/Users/jakevdp/github/executablebooks/MyST-NB/myst_nb/execution.py", line 153, in generate_notebook_outputs
    raise ExecutionError(
myst_nb.execution.ExecutionError: Execution failed for file: /Users/jakevdp/github/google/jax/docs/myst_nb_test.md
An error occurred while executing the following cell:
------------------
assert x[0] == 4321
------------------

---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-3-381fab23ad2d> in <module>
----> 1 assert x[0] == 4321

AssertionError: 
AssertionError:
The full traceback has been saved in /var/folders/lr/h5hbphfs7_54btrvfk4qb7qc00h5r8/T/sphinx-err-1hoea1rl.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!

[choldgraf]

🚀

Edit: I guess that emoji has new meaning right now, maybe i should have used 💎👐

[jakevdp]

I pushed a typo fix a few days back, and need another review now. Thanks!

[choldgraf]

This still LGTM 👍. I'll wait a day or two to see if @chrisjsewell wants more changes. Is this an urgent improvement on your end @jakevdp ?

[jakevdp]

Thanks! No, it's not urgent. We're currently working around this by monkey-patching the sphinx logger that myst-nb uses to report errors.

[chrisjsewell]

thanks @jakevdp, sorry bare with me I do want to have a quick look over this

[mmcky]

thanks @jakevdp this will be super helpful once this is merged.

[jakevdp]

FWIW, JAX no longer needs this, because I managed to fix all our warnings so we can run sphinx-build with the -W flag. But I still think this would be a useful option to support in myst-nb in general.

[choldgraf]

Thanks for the update @jakevdp - sorry this one slowed down a bit.

I'm +1 on this - though I think @chrisjsewell wants to hold off on merging?

[jakevdp]

Thanks! I just rebased and force pushed to fix conflicts that had come up on master, so a new review is required.

[chrisjsewell]

Superseded by #404 cheers