Nbconvert: "allow errors" vs. "raises exception"

[mgeier]

Support for the raises-exception tag was implemented in #684.

However, if a cell tagged with raises-exception does not raise an exception, nothing bad seems to happen.

I would expect an exception to be raised in this case, probably a CellExecutionError.

If this behavior is not wanted, the tag should be renamed to something like allow-exception.

Alternatively, there could be a configuration setting to switch between the two behaviors, but the default should nevertheless be changed.

Of course, the global "allow errors" setting should still allow the error of not raising an exception.

UPDATE:

Related issues:

• https://github.com/jupyter/notebook/issues/4198
• https://github.com/computationalmodelling/nbval/issues/108
• https://github.com/jupyterlab/jupyterlab/issues/2412

[takluyver]

Renaming the tag is a no-go, as far as I'm concerned: it's already supported across a number of projects. I'm open to at least having a warning if a cell tagged raises-exception doesn't, and for nbval it probably makes sense to have an error in that case.

[mgeier]

Sure, for nbval that makes sense, too, as I assume for all other projects. What are those other projects?

I thought about it some more, and a configuration setting to switch between the behaviors doesn't really make sense, since this can only be decided on a case-by-case basis.
It still makes sense to silence all errors with a global "allow errors" setting.

I also thought about introducing two different tags: raises-exception and allow-exception.
But I couldn't come up with a situation where allow-exception would even make sense, do you know one?
Whenever there could happen an exception but isn't guaranteed to, I would use a try/except statement anyway.

[mgeier]

I forgot: raising a warning has only limited usefulness, namely when running nbconvert manually in a console. When running it in a script or when using it programmatically, the warning will often not be seen.

[mgeier]

Ping.

Are there any further opinions?

This misnomer could still be fixed for version 5.5. The tag was introduced in 5.4.

[takluyver]

The tag is also used in nbval and in the notebook interface itself - cells queued for execution aren't cancelled on an error from a cell with the raises-exception tag. It's been effective in the notebook for several releases starting over a year ago: https://github.com/jupyter/notebook/pull/2549

So I still think we're well past the time when we could practically reconsider the name. It's hardly the worst anomaly to be putting up with for historical reasons.

I'd still be open to tweaking the behaviour to do something when a cell tagged raises-exception doesn't, or to introducing a tag like should-raise-exception or error-expected.

[mgeier]

@takluyver Thanks for your answer!

It's been effective in the notebook for several releases starting over a year ago

For the record, I've created this issue 10 months ago, less than 4 months after the feature was released in notebook version 5.1.0.

But anyway, I agree that we shouldn't change the perfectly fine name raises-exception, but we should change the behavior.

I'd still be open to tweaking the behaviour

Yes please, that would be great!

A cell tagged with raises-exception should generate an error (most likely some exception mentioning the raises-exception tag) if the code in the cell doesn't actually raise an exception.

[takluyver]

I have some concerns that raising an exception is going too far, if people have already got used to the current behaviour. But that's up for discussion.

I don't think I'll have time to work on this any time soon, so PRs welcome. I'd suggest starting with nbval; it makes most sense to be strict in a testing tool.

[mgeier]

I've created an issue for nbval: https://github.com/computationalmodelling/nbval/issues/108.

I have some concerns that raising an exception is going too far

Au contraire, I think that not raising an exception is actually harmful, because it can be a source for hard-to-find bugs: Since the tags are not always visible in the UI, a change in a previously exception-throwing notebook cell might stop the exception from happening without anybody noticing the now useless raises-exception tag that's still there. This doesn't sound too bad yet, but further down the line, after some more edits, someone might unintentionally create a bug in that cell that throws an exception. But since it still has the raises-exception tag (which still nobody noticed), the bug goes unnoticed. Of course the exception can be seen in the cell output, but if you execute a very long notebook, you probably don't check every single cell output, you just scroll down to see if it executed successfully, and you'll miss the exception int this case because it looks like the execution was successful..

With the behavior suggested by me here, the first change (that disabled the exception) would immediately be noticed and the tag would be removed. Everything is good.

Please also note that copying a cell also copies its tags. This way, the raises-exception tag could be copied many times, without anybody noticing. Which later could hide bugs that would be otherwise noticed immediately.

[MSeal]

I think, given that notebook, jupyterlab, and nbconvert all don't respect the behavior implied by this issue (unless I mistook the other threads) and haven't for a long while now it would be damaging to change behavior at this time. I think this would be a better discussion for nbformat 4.5 or 5.0 discussions. I'm going to remove the 5.5 milestone flag, and unless there's consensus to keep the discussion here -- close the issue later this week.

[mgeier]

it would be damaging to change behavior at this time.

I think the long-term damage caused by the unexpected behavior of raises-exception will be greater than the (probably non-existent) short-time damage of changing said behavior.

If anything breaks, it will break loudly, which is a good thing!

If we keep the current behavior, the problems will be much harder to locate and much more subtle and annoying (and therefore damaging).

[MSeal]

As a programmer I actually expect the behavior as-is from raises-exception. This implies to me that there is a catch-all for exceptions, not that that it's applying a test-style with assertRaises... pattern. I actually see making a pattern available for exceptions MUST be raised as a clear anti-pattern to modern programming paradigms. The only time exception raising as a requirement to control-flow has found a foothold in good programming paradigms that I can recall off-hand is loop iteration exit patterns, which is not what we have here.

The risk here I am specifying in towards adopted systems that respect < 4.4 nbformat notebooks and that the convention is well cemented on how this behaves across the major actors. Changing that within 4.4 is not realistic. If you want to change it, it should be targeted at future schemas and not by convention of individual app versions at this point. I don't think you'll get any traction trying to change this after more than a year of behavior, nor should it be pushed on each project individually to try and get one to change behavior without some consensus of the standards.

If users want more sophisticated exception handling they should capture it in code, not in the system. The only reason I think raises-exception is handy is to help when a call out to a magic / code path doesn't have a nice place to write your own try/catch pattern. If this weren't the case I'd be even more opinionated that notebooks should concern themselves at all on the variance of exception control flows. Even with indicating an exception is possible, expecting to always fail should be controlled by the user (e.g. !my_failing_bash.sh vs !(my_failing_bash.sh && exit(1)) || exit(0)) not by the higher level tooling imo.

[mgeier]

I actually see making a pattern available for exceptions MUST be raised as a clear anti-pattern to modern programming paradigms.

I don't understand what you want to say with this.

We are not discussing whether Python (or any language) should have exceptions or whether exceptions should be used only for error reporting or for control flow as well. Right?

If you have a code cell that raises an exception in your Jupyter notebook and you actually want that to happen (for whatever reason, whether you use modern programming paradigms or not), you also expect an exception to happen. And if it does not happen, you want to be made aware of it, right?

Can you please tell me a single situation where you would want to mark a cell to raise an exception, but that exception doesn't happen every single time?

Sorry for repeating myself, I've already asked this above (https://github.com/jupyter/nbconvert/issues/730#issuecomment-356867251), but I didn't get an answer. There was also some discussion over there: https://github.com/jupyter/notebook/issues/4198#issuecomment-438630006, where I didn't get a satisfactory answer, either.

I believe there is not a single reasonable case where an exception would be expected only some of the times.

Therefore, if nbconvert would hypothetically change its behavior, the only cases where an expected exception would not happen (i.e. the only situation where anyone would even realize that something has changed), would be cases where the raises-exception tag was applied erroneously. And that's good, because then the tag can be removed to avoid the problems I've described above (https://github.com/jupyter/nbconvert/issues/730#issuecomment-438223813).

[t-makaro]

I tend to lean towards thinking this behaviour should be changed, but I've never actually used this feature.

If we do wish to change the behaviour, it may be best to add allow-exception, and then add a warning to raises-exception for a few releases before changing the behaviour in 6.0. I also think such a PR would need to be done in tandem with Jupyter notebook.

[MSeal]

There's plenty of cases where an exception is sometimes raised. Even within nbconvert the bibtex command call fails if there's an error or if no bibtex references are found without a way to differentiate the two, we ignore the failure today until we get around to writing log parsers to correctly classify the failure. Any optimistic update path also follows a maybe-raise pattern (e.g. fetch latest cached results against an API that has a lot of downtime, but for which you have a stale cached version). However I'm more inclined to not have the tag at all over expanding it's impact. Given it's an accepted standard I won't push that for 4.x schema.

I believe there is not a single reasonable case where an exception would be expected only some of the times.

is simply not true in the many production systems and environments I've worked in. If that's not been your experience take it as a data point that there's others who have seen this often and chose the pattern used.

My main argument for why this issue should be closed is that there's a field already in place, that maybe could be named a little better, with an established convention. We have a system for updating such conventions through standards versioning. So I really think the discussion should be there and not in the implementation points of that standard after they're established.

My additional argument is that in all the threads you've started there's been a consistent "we're not changing this" response or no response at all. This is a hint that pushing the topic harder is not going to be received well in the way you're doing now. It's ok to ask if something should be changed or push back on an idea. But continuing to push and push after multiple people disagree can start to fringe on unprofessional.

[mscuthbert]

How about leaving the current raises-exception tag alone and adding a requires-exception tag for what @mgeier wants?

[MSeal]

Possibly, though a rename would likely be less confusing. It would need to be adopted in notebook / nbformat regardless for us to add it in nbconvert for this change. Going to close, we can reopen if the core adopts a different convention.

[obestwalter]

If this is ever reconsidered, I would also propose that cells marked with raises-exception which don't raise one should crash with something like a DidntRaise exception. pytest does the same when using the pytest.raises context manager in a test and I find that very helpful. I would also like this behaviour in notebooks.

Maybe it would help to clearly separate the different expectations about the behaviour by having two flags:

• raises-exception => must raise an exception
• might-raise-exception => might or might not raise an exception