Show traceback instead of continuing

[flying-sheep]

For your consideration only, not to merge yet.

Check out cell 6 of the demo notebook.

What’s possible is displaying line numbers for functions defined in the notebook. i didn’t use that to implement showing function context, but this is easy.

Further steps would be to extend this like RStudio can do with support for package sources. (“at line 8 of foobar.R”)

fixes #218 and #220

[takluyver]

Looks good. Though it seems odd in the example traceback that it shows f2() is on line 2 of throw, when it's a one line function. Is it giving the line in the cell rather than the line in the function? We should check that, because in less trivial cases it might matter.

[flying-sheep]

Is it giving the line in the cell rather than the line in the function?

ah, interesting. if so, that’s even more useful, especially if we can somehow refer to the cell (maybe by execution count?)

[takluyver]

Do 23 and -3 have significance here?

[flying-sheep]

yes. they remove everything that ends up in the call stack

1. by us and evaluate (23 levels deep)
2. by the error handler (3 levels on the other end)

the calls between those are user and library code

[takluyver]

OK, that definitely merits an explanatory comment.

Is there any sane way to calculate the 23 more automatically? I'm worried that if anything changes in our code or evaluate, people could start losing their stack frames.

[flying-sheep]

no idea. i basically let the whole stack trace print and counted where the useful stuff began.

please play around with all that as well, it’s really complex. what i did for debugging:

calls <- NULL
withCallingHandlers(some_failing_expression(), error = function(e) { calls <<- sys.calls() })

then i inspected calls using str and found that calls in user code have a attr(call, 'srcref').

[flying-sheep]

in other news i removed all the empty skeleton doc comments for Kernel methods since they were unused by roxygen2 anyway. apparently one documents refclasses by docstrings 😆

i added docstrings to everything you intended to export, but no idea if you actually still mean to do that

[takluyver]

I think I was just following what someone else had indicated I should do. I don't really consider any part of IRkernel to be public API, so documenting the functions in it doesn't really matter. IRdisplay and repr are a different question, though.

[flying-sheep]

ok. so i’ll leave them. if you want you can go through and add or remove docstrings (or the @export of the Kernel class altogether)

[flying-sheep]

ah, there is a way to calculate it:

r-lib/evaluate#37 (comment)

[takluyver]

It looks like the constant you need to add for the evaluate frames is 22, which seems large enough that it's liable to change. I wonder about e.g. iterating through the frames to find the first one that's not in the evaluate package.

But since we don't show any tracebacks at the moment, an imperfect solution would already be an improvement, so let's not spend too long thinking about it.

[flying-sheep]

i don’t get it: i just said that we can calculate it using the method mentioned in the linked comment, but you seem to respond to me saying we can’t.

did you simply misread or do i understand you wrong?

[takluyver]

That shows how to calculate the stack depth where you're calling evaluate, but then the example adds on a constant representing the frames inside evaluate itself: frame+22.

My concern is that the constant (22) may well depend on the version of evaluate you're using, and maybe even on the options you pass to it, so it would be easy for it to be wrong. But we won't know until we try.

[flying-sheep]

ah, now i get you!

we could patch eveluate so that it attaches the stack depth as attribute to the error.

[takluyver]

Assuming you mean submitting a patch back to evaluate: go for it. I don't want to end up shipping a forked copy of evaluate ourselves, though.

If evaluate is happy to improve this for the future, I think putting a constant in there is good enough for the present.

[flying-sheep]

sure, i mean a patch

[flying-sheep]

hmm, so i investigated that srcref/srcfile stuff a bit.

here are the docs

when looking at a function compiled using --with-keep.source, we get:

• function(...) ...
  • attr(*, "srcref")=Class 'srcref' atomic [1:8] 111 17 285 1 17 1 479 653
    • attr(*, "srcfile")=Classes 'srcfilealias', 'srcfile' <environment: 0x9be4400>

ok, so srcref is a int vector with 8 elements: first_line, first_byte, last_line, last_byte, first_column, last_column, first_parsed, last_parsed

srcfile is an attribute of that vector, and it’s an environment:

> eapply(attr(attr(fn, 'srcref'), 'srcfile'), function(x) capture.output(str(x)))
$filename
[1] " chr \"/home/*/*.r\""
$original
[1] "Classes 'srcfilecopy', 'srcfile' <environment: 0x9be5690> "

and looking at the original:

> eapply(attr(attr(DiffusionMap, 'srcref'), 'srcfile')$original, function(x) capture.output(str(x)))
$wd
[1] " chr \"/home/angerer/Dev/R/destiny\""
$lines
[1] " chr [1:1591] \".packageName <- \\\"somepackage\\\"\" \"#line 1 \\\"/home/*/*.R\\\"\" ..."
$Enc
[1] " chr \"unknown\""
$isFile
[1] " logi TRUE"
$timestamp
[1] " POSIXct[1:1], format: \"2015-12-03 11:25:58\""
$filename
[1] " chr \"/home/*/R/x86_64-pc-linux-gnu-library/3.2/somepackage/R/somepackage\""
$fixedNewlines
[1] " logi TRUE"

[jankatins]

Is this also fixing the problem that when I have an error the next cell is still evaluated?

eg:

print("whatever")
# --- next cell ---
stop("Stop execution here")
# --- next cell ---
print("Should not run")

and then execute the whole notebook via "Cell -> Run All?

IMO the current behaviour is very problematic as it can damage data: e.g. I expect that when I get an error a cell below which write the data to disc does not do this... Currently it does :-(

[jankatins]

Is that a leftover from debugging? Ot why would you hardcode the code which is evaluated?

[flying-sheep]

jup, that was my original way to find out the stack depth. turns out just doing nframe <- sys.nframe() works too

[flying-sheep]

eh i’m dumb. actually it doesn’t work and we need that stuff

[flying-sheep]

Is this also fixing the problem that when I have an error the next cell is still evaluated?

no, as i have no idea how that works. @takluyver?

[jankatins]

I submitted a new issue to track the "run all cells does not stop on errors" problem: #232

[flying-sheep]

ok, so this works, has no (or better “less and smaller”) magic constants, and the filename change is merged via r-lib/evaluate#58, but not released yet so i’ll just merge this.