Enable JupyterKernel to print traceback on error

[ssiccha]

This is a bit dirty. Setting BreakOnError := false suppresses
both the break loop and the traceback. So to make the traceback
available under jupyter ErrorInner checks whether a JupyterKernel
is currently running.

See new commit message below.

[ssiccha]

This builds on #2529.

[ssiccha]

This seemed to me to be the easiest way to get this done for now.

Another idea would be to have ErrorInner always print a traceback, except for when a Test is running. Test could e.g. define a local variable IsTesting which is checked. This would maybe also benefit libGAP?
It is not documented that BreakOnError := false implies that the traceback is suppressed, to the contrary:

-T
     suppresses  the  usual  break  loop  behaviour of GAP. With this option GAP behaves as if the user quit
     immediately  from  every  break loop. This is intended for automated testing of GAP. This option may be
     repeated to toggle this behavior on and off.

I can't imagine a situation where a traceback would not be helpful. Maybe I'm suffering a spontaneous lack of imagination though. :)

[markuspf]

I think its a bad idea to have a variable for specific IsJupyterKernelRunning or IsTesting.

What's wrong with a variable that enables or disables printing of backtraces? I struggle imagining a case in which one'd want to suppress backtraces, but then at least one could.

I also agree that BreakOnError := false should not suppress printing backtraces unless this breaks some bit of testing in a major way.

[fingolfin]

Uhm, I think Test actually relies on BreakOnError disabling back traces. So at the very least, there still must be a mechanism to disable the backtrace, because we really don't want them in tests (else they'd become super brittle; even if we suppressed line numbers, they'd still depend on implementation details and call orders, etc.).

And I also I have quite some custom code which relies on the current behavior of BreakOnError. E.g. my UnitTests package (although that's rarely used these days, and I hope I can replace it by e.g. the QuickCheck code one of your students is working on).

Don't get me wrong, I'd survive if this changed, but I am not convinced by the argument given here that this is necessary or even that desirable, nor do I agree that -T implies that what we do right now is wrong -- and even if, this has been the behaviour of GAP for decades, and so I'd argue that the documentation for -T should be changed, if at all.

Can't you, instead of changing this code, instead install a custom OnBreak handler, which basically replicates the code from the BreakOnError = false code path? I.e. something like this (untested):

OnBreak := function()
        ErrorLevel := ErrorLevel-1;
        ErrorLVars := errorLVars;
        if ErrorLevel = 0 then LEAVE_ALL_NAMESPACES(); fi;
        JUMP_TO_CATCH(0);
end;

That said, I'd generally agree that ErrorInner could/should be refactored further (and also be merged with its HPC-GAP twin in hpcgap/lib/error.g. But I am not sure this is the way...

[fingolfin]

This causes the test suite to fail, as GAP_ERROR_STREAM is not defined. Perhaps you meant "*errout*"?

[markuspf]

he did mean GAP_ERROR_STREAM, because I used that global variable in my prototype code for error printing in the JupyterKernel package.

Whether it's useful to use GAP_ERROR_STREAM over "*errout*" is debatable, as long as it's search-and-replaceable, either is fine for me, with preference for "*errout*" because its consistent with the remainder of the codebase.

[fingolfin]

Then this pull request is incomplete, as GAP_ERROR_STREAM is not defined. It cannot be merged until this is resolved, either by defining GAP_ERROR_STREAM, or by not using it.

[ssiccha]

Sorry for being absent. I neglected to mention that this needs #2529 to be merged and that printing tracebacks while testing would lead to "false errors" when e.g. changing filenames.

wrt this PR:
@markuspf I will get rid of the local variable IsRunningJupyterKernel and try what @fingolfin suggested.

@fingolfin You mean when running a JupyterKernel we don't use -T but instead overwrite OnBreak() with the function you suggested? That should work. I'll test it.

More general (I made separate issues for these points):
I think the documentation of -T should be changed to say that it disables the break loop and the traceback. Do we want a command line option (like --nobreakloop ) that disables the break loop but still prints tracebacks? Then JupyterKernel wouldn't need to overwrite OnBreak() anymore.

I think with a global variable TracebackOnError (or any better name) one should be able to refactor ErrorInner in such a way that the behaviour of ErrorInner for BreakOnError := false could afterwards be achieved with BreakOnError := false; TracebackOnError := false;. Would that make it easy to adapt your UnitTests package @fingolfin?

[codecov]

Codecov Report

Merging #2530 into master will decrease coverage by <.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #2530      +/-   ##
==========================================
- Coverage   74.46%   74.46%   -0.01%     
==========================================
  Files         481      481              
  Lines      242743   242743              
==========================================
- Hits       180770   180769       -1     
- Misses      61973    61974       +1

Impacted Files	Coverage Δ
lib/error.g	35.79% <66.66%> (ø)	⬆️
hpcgap/lib/hpc/stdtasks.g	64.19% <0%> (-0.26%)	⬇️

[ssiccha]

@fingolfin I made it work with your suggestion.

On the GAP site now I did the following (new commit's message):
This makes it possible for the JupyterKernel package to
override the function OnBreak. Namely errorLvars needs to
be global so that the alternative OnBreak function can also
read from and write to it.

[ssiccha]

I'm not sure whether making errorLVars global has any unintended side effects. It basically is a buffer for the original ErrorLVars variable. The original value ofErrorLVars is always restored before exiting the function via a return or a JUMP_TO_CATCH. I think this is problematic if an error occurs inside ErrorInner. This could be alleviated by making errorLVars into a stack. Should I do this or ignore this for now?
This should only be a temporary hack, so I don't mind if this is not merged in at all. I would rather like to introduce TracebackOnError and then there would be no need for the JupyterKernel package to overwrite OnBreak() anymore and this could be closed / reverted.

[ssiccha]

I renamed errorLVars to ErrorLVarsOld to please the test suite.

New commit message:

Enable JupyterKernel to print traceback on error
    
This makes it possible for the JupyterKernel package to
override the function OnBreak. Namely `errorLVars` becomes
the global variable `ErrorLVarsOld` so that the alternative
OnBreak function can also read from and write to it.

[fingolfin]

With this change, nested errors won't work -- i.e. nested break loops.

All you need is the correct ErrorLVars in your OnBreak handler, right? So you simply leave errorLVars as it is, but insert ErrorLVars := errorLVars; right before the call to OnBreak. That should never cause harms, but would remove the need for your OnBreak handler to do anything.

I.e. this PR would essentially insert a single line.

Does that work?

[ssiccha]

Since the nested break loops don't work like this this can't be merged.

I think the problem with setting ErrorLVars := errorLVars; right before calling OnBreak() is that currently, when entering OnBreak, ErrorLVars is set to context. Where uses ErrorLVars to access context to print the traceback. (if only OnBreak could take context as an argument..)

Or did I misunderstand something there?

[fingolfin]

OK, I just re-read the whole of ErrorInner (as I should have done before my comments sigh), and yes, you are right.

But what should work is adding a new global ErrorLVarsOld as you did, but retaining errorLVars; then set ErrorLVarsOld only just before calling OnBreak. That's not very nice, because we are essentially passing an argument through a global variable, but it'd work.

But yeah, at this point, we might as well introduce AlwaysShowTracebackOnError as discussed in issue #2547.

And refactor this code later on again ;-).

[ssiccha]

I will close this in favor of a global variable AlwaysPrintTracebackOnError as discussed in #2547.