Unexpected behavior on cells tagged with "raises-exception"

[mgeier]

I didn't try it, but according to @takluyver (see jupyter/nbconvert#730 (comment)), nbval does not raise an error if a cell tagged with raises-exception does not raise an exception.

I think that not raising an exception in a raises-exception cell should generally (in every Jupyter-related tool) be an error and @takluyver said (jupyter/nbconvert#730 (comment)):

for nbval it probably makes sense to have an error in that case

How about raising an error in that case?

Related issue on nbconvert: jupyter/nbconvert#730

Related issue on notebook: jupyter/notebook#4198

Related issue on jupyterlab: jupyterlab/jupyterlab#2412

[vidartf]

Off the top of my head, if you ensure that nbval checks the output of the cell, then the exception ename and evalue will be compared (the traceback is ignored due to file paths etc). As such, a missing exception should trigger a failure as well! If using nbval-lax, you can force the output check of a specific cell with the cell-tag "nbval-check-output" or a leading cell comment # NBVAL_CHECK_OUTPUT.

[mgeier]

@vidartf Thanks for the quick response!

I've now installed nbval and tried it myself:

If the notebook is un-executed (i.e. no outputs are stored), nbval doesn't report an error on a raises-exception cell that doesn't raise. I guess this isn't the typical use of nbval, normally all cell outputs are stored in the notebook, right?

However, if the outputs are stored in the notebook file, nbval still doesn't report an error. Which IMHO is a bug, but it may be a bug in the tool that initially executed the notebook and not a bug in nbval itself.

If I use # NBVAL_RAISES_EXCEPTION instead of the raises-exception tag, nbval still doesn't report an error if no exception is raised, which IMHO also is a bug, this time in nbval.

Note, however, that if notebook and jupyterlab (and whatever else that can execute notebooks and store the results) would correctly handle the raises-exception tag, the whole situation would change, but nbval would still have to be careful to do the right thing: In a hypothetical future where notebook and jupyterlab are fixed, they would raise a CellExecutionError (or similar) in the case a raises-exception cell doesn't raise. This new exception would be stored in the notebook, and nbval would have to make sure that the same CellExecutionError is raised when it executes the cell.

[vidartf]

Thanks for the feedback. I'll look a bit further into the current behavior, but for now I just want to clear up that # NBVAL_RAISES_EXCEPTION and the raises-exception/nbval-raises-exception tags are all equivalent. Having a way to assert that a cell raises an error even if there is no outputs saved seems like a good idea, but I'll have to try and see what the cons are, if any.

[vidartf]

In a hypothetical future where notebook and jupyterlab are fixed, they would raise a CellExecutionError (or similar) in the case a raises-exception cell doesn't raise.

Raising an error or not seems like the domain of the kernel, and should not be done by the front-end (it might report an error to the user e.g. with a dialog, but again, not as an exception in the output). It also does not strike me as something the notebook server should get itself involved with.

[vidartf]

However, if the outputs are stored in the notebook file, nbval still doesn't report an error. Which IMHO is a bug, but it may be a bug in the tool that initially executed the notebook and not a bug in nbval itself.

I cannot reproduce this behavior. If I edit the exceptions.ipynb notebook in the tests folder such that the first cell does not raise an error, I get the following test failure:

PS C:\dev\nbval> py.test .\tests\exceptions.ipynb --nbval
============================= test session starts =============================
platform win32 -- Python 3.6.5, pytest-3.8.1, py-1.5.3, pluggy-0.7.1
rootdir: C:\dev\nbval, inifile:
plugins: tornado5-1.0.0, timeout-1.3.2, cov-2.5.1, nbval-0.7, check-links-0.2.0
collected 6 items

tests\exceptions.ipynb F.....                                            [100%]

================================== FAILURES ===================================
_______________________ tests\exceptions.ipynb::Cell 0 ________________________
Notebook cell execution failed
Cell 0: Cell outputs differ

Input:
# NBVAL_RAISES_EXCEPTION
# raise RuntimeError("foo")

Traceback:
Missing output fields from running code: {'evalue', 'ename'}

============================== warnings summary ===============================
C:\Miniconda3\lib\site-packages\jupyter_client\manager.py:62: DeprecationWarning: KernelManager._kernel_spec_manager_change
d is deprecated in traitlets 4.1: use @observe and @unobserve instead.
  def _kernel_spec_manager_changed(self):

-- Docs: https://docs.pytest.org/en/latest/warnings.html
=============== 1 failed, 5 passed, 1 warnings in 2.25 seconds ================
PS C:\dev\nbval>

[mgeier]

If I edit the exceptions.ipynb notebook in the tests folder such that the first cell does not raise an error

But you kept the old cell output?

Then nbval (correctly) reports an error.

But if I re-execute the cell, generating a new (possibly empty, but definitely non-exception) output and save the notebook, then all 6 tests pass again (which is a bug).

BTW I'm using the latest nbval release from PyPI.

Raising an error or not seems like the domain of the kernel, and should not be done by the front-end

You are right, the front-end should show some kind of error and stop further execution, but raising an exception in the notebook context doesn't make sense at all. I should have thought about this before posting my comment above.

I was thinking about CellExecutionError because I initially encountered the problem when running the nbconvert.preprocessors.ExecutePreprocessor which raises exactly that exception. But it is of course not raised in the context of the notebook, but in the script/program that calls the preprocessor.

[vidartf]

But you kept the old cell output?

Yes, this is for example what you would expect if the code initially produced an error, but now no longer does (nbval will then catch that behavior).

But if I re-execute the cell, generating a new (possibly empty, but definitely non-exception) output and save the notebook, then all 6 tests pass again (which is a bug).

If during your normal operation, you do not generate an error, and you save such a state to the repo, then I would not expect nbval to see this as a fault. Can you expand upon a use-case where this would be the case? Note that #109 now fixes the case for cells that are stored in an un-executed state (i.e. the execution_count of the cell is None).

[mgeier]

If during your normal operation, you do not generate an error, and you save such a state to the repo, then I would not expect nbval to see this as a fault.

This is the case about which I said above (#108 (comment)):

"Which IMHO is a bug, but it may be a bug in the tool that initially executed the notebook and not a bug in nbval itself."

So yes, it may be fine if nbval doesn't detect this as error (but it doesn't hurt if it does).
But if there is no execution_count, nbval should report an error (which #109 already does, as you mentioned above).

Can you expand upon a use-case

I don't have a use case other than using a buggy tool to generate the cell outputs. It may need some more convincing from my side, but I hope notebook, jupyterlab and nbconvert will at some time get fixed.