Skip to content

Refactor DashR callbacks for increased parity with Dash for Python API #51

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

Merged
merged 31 commits into from
Feb 20, 2019

Conversation

rpkyle
Copy link
Contributor

@rpkyle rpkyle commented Feb 12, 2019

The proposed changes include

  • the callback method for Dash now uses the output, inputs, state, func ordering for parameters, while previously output was last; the change reflects the current Dash for Python callback signature
  • the callback method for DashR now has a function signature that is closer to the Dash for Python API, with the caveat that inputs and state are unified into a single params argument
  • the addition of a func argument, to pass the callback handler; R does not natively offer a Python-like decorator pattern, which is the current approach for specifying this function in Dash for Python
  • basic validation of callbacks using assert_valid_callbacks
  • updating dash-renderer to 18.0

The current batch of examples will have to be updated to reflect the changes. I'm continuing to work on the code, but callbacks appear to function properly now.

R/dash.R Outdated
output=callback_signature$output,
inputs=callback_signature$inputs,
state=callback_signature$state,
key=paste(callback_signature$output$id, callback_signature$output$property, sep='.')
Copy link
Member

Choose a reason for hiding this comment

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

Is key a special keyword in R's lists? I'm assuming so, since I don't see this referenced anywhere else in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was present in the original code, but I can remove it. The key is also referenced here:

key = paste0(id, ".", property)

Copy link
Collaborator

Choose a reason for hiding this comment

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

These key= lines are still present 🔪

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 7e51f91

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 313dff1

Copy link
Member

@chriddyp chriddyp left a comment

Choose a reason for hiding this comment

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

Looking good! Just a few small comments & questions

@rpkyle
Copy link
Contributor Author

rpkyle commented Feb 13, 2019

Looking good! Just a few small comments & questions

Thanks for taking the time to review. I'll make these initial edits and await Nadia and Alex's suggestions.

@rpkyle
Copy link
Contributor Author

rpkyle commented Feb 19, 2019

@alexcjohnson @chriddyp @TahiriNadia

I've updated the examples in README.md, in case you're able to take a quick look. Hopefully we're nearing a point where we can 💃 this one.

@rpkyle rpkyle self-assigned this Feb 19, 2019
@rpkyle rpkyle added this to the DashR Launch milestone Feb 19, 2019
Added a note explaining `showcase=TRUE`.
Modified README.md to simplify arguments, remove id and property declarations.
Simplified arguments to first example.
Modify apps to use state, instead of specifying with input/type as before.
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.

Just one small item got left behind #51 (comment) but otherwise this is ready to go! 💃

R/dash.R Outdated
@@ -328,6 +301,7 @@ Dash <- R6::R6Class(
},
layout_set = function(...) {
private$layout <- if (is.function(..1)) ..1 else list(...)
lay <- private$layout_render()
Copy link
Member

Choose a reason for hiding this comment

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

This line confuses me a bit, although it might just be my misunderstanding of R. Where is lay being used?

  • Is this setting it to the class?
  • Does this also implicitly return lay?

Copy link
Member

Choose a reason for hiding this comment

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

Also, why are we calling layout_render here? Could we leave a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, if you don't assign layout_render() to an object, it returns the layout, which will display in the console.

There are a couple ways I could handle this -- I can wrap the return in invisible, so that it silently returns the layout, which can be assigned to a new object. Or, I can add a silent option which is TRUE by default and let the app developer decide.

Copy link
Contributor Author

@rpkyle rpkyle Feb 20, 2019

Choose a reason for hiding this comment

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

Also, why are we calling layout_render here? Could we leave a comment?

I mentioned this last week, but I think it got lost in the shuffle. If we don't render the layout when it's set, I don't believe it will get rendered until a GET request occurs and is handled by routr unless layout_get(render=TRUE) is called.

Copy link
Member

Choose a reason for hiding this comment

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

it returns the layout, which will display in the console.

Because if you didn't return the object but still needed to call private$layout_render(), the code would look like:

layout_set = function(...) {
       private$layout <- if (is.function(..1)) ..1 else list(...)
       private$layout_render()
}

which would actually return private$layout_render() (since it's the last line of the function).

But if we didn't need to call private$layout_render(), then the function would just be

layout_set = function(...) {
       private$layout <- if (is.function(..1)) ..1 else list(...)
}

which wouldn't return anything and be OK.

Am I understanding this correctly?

In Python, there is a convention to call "unused / do not care" arguments _ (https://hackernoon.com/understanding-the-underscore-of-python-309d1a029edc). Not sure if you can assign variables to _ in R or if R has that convention, but this codeblock might be easier for other folks to understand if it was written:

layout_set = function(...) {
       private$layout <- if (is.function(..1)) ..1 else list(...)
       # call layout_render() in order to  [...] but don't return its response
       _ <- private$layout_render()
}

Copy link
Member

Choose a reason for hiding this comment

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

anyway, this isn't blocking, just trying to understand things a bit better!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, no worries -- I appreciate your questions because they often lead me to a better understanding of the way Dash works.

This line confuses me a bit, although it might just be my misunderstanding of R. Where is lay being used?

  • Is this setting it to the class?
  • Does this also implicitly return lay?

The code (which I've since modified) does not set class attributes or implicitly return lay since this line isn't the last one in the function. I've since wrapped layout_render() inside a call to invisible, which I think has the same effect as assignment to _ in Python. (I actually did not know about that, but it's a cool feature of which I should be aware.)

I've added a brief comment to explain what's happening here also.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, would be nice to understand at some point why this call is necessary (for that matter, I don't quite get why we have a nearly identical call on the Python side 😏) but thanks for making the intention clearer with invisible.

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 f9c9174

Copy link
Contributor Author

@rpkyle rpkyle Feb 20, 2019

Choose a reason for hiding this comment

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

Missed a question or two here:

which would actually return private$layout_render() (since it's the last line of the function).

Yes, that's right.

But if we didn't need to call private$layout_render(), then the function would just be

Yep, got it -- this line basically says if the first argument of ... is a function, assign it to layout, otherwise wrap all the arguments into a list and assign that into layout.

Copy link
Member

@chriddyp chriddyp left a comment

Choose a reason for hiding this comment

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

nice work! 💃 once you consider my comments

@rpkyle rpkyle merged commit 8732270 into master Feb 20, 2019
@rpkyle rpkyle deleted the 0.0.2-issue11984 branch February 20, 2019 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants