Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for full and partial call stacks, improved error handling in DashR #87

Merged
merged 21 commits into from
Jun 13, 2019

Conversation

rpkyle
Copy link
Contributor

@rpkyle rpkyle commented Apr 28, 2019

This PR proposes multiple improvements to error handling and debugging in DashR:

  • Adds a debug mode to DashR apps
  • Introduces full/partial call stacks if app is served via app$run_server(debug=TRUE)
  • While in debug mode, catches errors as warnings, allowing app execution to continue
  • Introduces logic to return an HTML version of the call stack and error message for use by dash-renderer and the recently released in-browser dev tools

Here is an example of an error before the proposed changes:

Screenshot 2019-04-28 00 46 52

...and after:

Screenshot 2019-04-28 00 47 07

Eventually, it should also be possible to conditionally implement line number support for R code loaded via source().

Closes #16 and #86.

@rpkyle rpkyle requested a review from alexcjohnson April 28, 2019 04:51
@rpkyle rpkyle self-assigned this Apr 28, 2019
@alexcjohnson
Copy link
Collaborator

Nice, that's certainly an improvement!

  • Introduces logic to return an HTML version of the call stack and error message for use by dash-renderer and the recently released in-browser dev tools

Does that work now? What does it look like? Also, am I reading it right that it's returning code 200 in the new version? Can we change it back to 500 (which is also what dash-py uses for errors)?

R/dash.R Outdated Show resolved Hide resolved
@rpkyle
Copy link
Contributor Author

rpkyle commented May 4, 2019

@alexcjohnson

Nice, that's certainly an improvement!

😌

  • Introduces logic to return an HTML version of the call stack and error message for use by dash-renderer and the recently released in-browser dev tools

Does that work now? What does it look like? Also, am I reading it right that it's returning code 200 in the new version? Can we change it back to 500 (which is also what dash-py uses for errors)?

Not yet, but I think we're close.

Right, 500 would make more sense. For HTML version of the call stack, I'll need to emulate what werkzeug is doing on the Python side, no? Since fiery lacks that sort of R debugging integration, I believe this would be best accomplished by sending a response up to the renderer with the proper headers, but I'm not sure.

Any hints you can offer there would be much appreciated 🙂. I think the frontend code that handles this sort of thing is here:

https://github.com/plotly/dash-renderer/blob/master/src/components/error/FrontEnd/FrontEndError.react.js

@rpkyle
Copy link
Contributor Author

rpkyle commented May 4, 2019

Also, it looks like Carson raised a related issue with Thomas Lin Pedersen, but I don't think it went anywhere:

thomasp85/fiery#34

@alexcjohnson
Copy link
Collaborator

Right, 500 would make more sense. For HTML version of the call stack, I'll need to emulate what werkzeug is doing on the Python side, no? Since fiery lacks that sort of R debugging integration, I believe this would be best accomplished by sending a response up to the renderer with the proper headers, but I'm not sure.

I think all you need is to send back the error as response$body, and set response$status = 500L. The error should be an HTML string, but that could initially just be a super simple wrapping like '<!DOCTYPE HTML><html><body><pre>' + error_with_stack_trace + '</pre></body></html>' and we can play with styling it nicely in another PR.

@@ -255,6 +254,9 @@ Dash <- R6::R6Class(
}
}

# set the callback context associated with this invocation of the callback
private$callback_context_ <- setCallbackContext(request$body)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I think all you need to do to finish off the callback context part of this PR is set it back to NULL after the do.call below, and have callback_context throw an error if it finds NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 6fc1cac

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry - what I meant was, down here when you access callback_context_

https://github.com/plotly/dashR/pull/87/files#diff-1881af3720f8f49fd3a44e51c9fb5a0dR489

At that point, if it's NULL, that means you're not in a callback so you shouldn't be able to access it, so throw an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 96e4672

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 💃 💃

@rpkyle
Copy link
Contributor Author

rpkyle commented Jun 13, 2019

Right, 500 would make more sense. For HTML version of the call stack, I'll need to emulate what werkzeug is doing on the Python side, no? Since fiery lacks that sort of R debugging integration, I believe this would be best accomplished by sending a response up to the renderer with the proper headers, but I'm not sure.

I think all you need is to send back the error as response$body, and set response$status = 500L. The error should be an HTML string, but that could initially just be a super simple wrapping like '<!DOCTYPE HTML><html><body><pre>' + error_with_stack_trace + '</pre></body></html>' and we can play with styling it nicely in another PR.

fixed in cee5376

@rpkyle rpkyle merged commit f521fd4 into 0.0.7-dev Jun 13, 2019
@rpkyle rpkyle deleted the 0.0.7-debug branch June 13, 2019 17:52
@rpkyle rpkyle changed the title [WIP] Add support for full and partial call stacks, improved error handling in DashR Add support for full and partial call stacks, improved error handling in DashR Jan 16, 2020
@rpkyle rpkyle removed their assignment May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants