I'm marking this as a blocker so we make a decision on it in the next few days. If we decide to make a change, the implementation is easy.
Currently, if a post-execution function fails we immediately de-register it. The rationale for this was originally that if the user had a bad function, there was no point in failing every time.
But in more extensive use, I'm realizing this is a bad policy in practice. To see the issue, try in a pylab notebook just this one cell:
note that this is unicode without the proper u qualifier. That triggers a matplotlib exception (as it should) but we immediately go ahead and disable further figure display:
Disabling failed post-execution function: <function flush_figures at 0x7fe574a20a28>
ERROR: An unexpected error occurred while tokenizing input
The following traceback may be corrupted or invalid
The error message is: ('EOF in multi-line statement', (316, 0))
So even though in this case the user did pass invalid input, from this point on their experience will simply be "the notebook doesn't work anymore". They can recover functionality by calling %pylab which re-registers the figure display function, but most people will just be puzzled by the behavior and likely just do a full session restart (possibly losing costly computations).
I think what we should do instead is to store in a set all functions that have ever failed at this stage, and change the error message to something like:
Function <....> produced an error. If this problem persists, you can disable all problematic post-execution functions by calling:
The user can then make the call manually if they really need it. But in a case like this example, they would simply realize their mpl error, fix their call and be able to continue.
I think that's fine to do, though I would actually consider it a bug that we allow user errors to raise an exception in our callback. It seems to me that post-execute functions should be extremely safe, given their relatively low-level position. Especially ones that we write ourselves.
Well, but in the example above, it's not really our callback that causes an error, the traceback comes from matplotlib itself. You can see it by typing the same thing in a plain ipython --pylab session. And if we masked all exceptions in our callback, it would make it much harder for users to sort out by themselves situations like this one: in this example, the mpl traceback is actually pretty informative:
File "/home/fperez/usr/opt/lib/python2.7/site-packages/matplotlib/text.py", line 985, in is_math_text
File "/home/fperez/usr/opt/lib/python2.7/site-packages/matplotlib/cbook.py", line 1868, in is_math_text
"matplotlib display text must have all code points < 128 or use Unicode strings")
ValueError: matplotlib display text must have all code points < 128 or use Unicode strings
and seeing that would immediately tell a user what's going on. That's why I think we should actually let exceptions of this kind bubble through, and just make it very easy/obvious for the user to disable a callback if it's really a buggy one.
It's not that our callback caused an error, it's that our callback allowed a user error to raise in itself, rather than calling showtraceback(). In general, IPython does not allow user errors to raise in IPython calls, but here we are allowing exactly that.
I still think your idea is a sensible one.
Looking at the error output, I am reminded that we should put messages about side effects after tracebacks, not before. The end of a traceback is much more visible in interactive use than the beginning.
Ah yes, I misunderstood your original message then. We could certainly wrap those calls with a try/except showtraceback().
BTW, if you want to take a crack at this one, go for it; I have to clean up some materials for the p4s lecture this afternoon.
PR #1155 makes both changes - post-exec functions are not unregistered on first failure, and user-errors do not raise in flush_figures(), instead calling showtraceback().
Great, thanks! I'll review it in a bit...