Skip to content

189 - Add Pattern Matching Callbacks for Dash R #228

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 65 commits into from
Oct 21, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
65 commits
Select commit Hold shift + click to select a range
508f8f5
Dash for R v0.7.1 (#221)
rpkyle Jul 31, 2020
9be5ef9
Testing initial implementation
HammadTheOne Aug 27, 2020
7cb20ef
More testing
HammadTheOne Aug 28, 2020
8c39812
Callback Context Updates
HammadTheOne Sep 1, 2020
0f47a6c
Updating callback context logic
HammadTheOne Sep 3, 2020
c4b6896
Fixing callback returns
HammadTheOne Sep 5, 2020
e860e0d
Adding callback args conditional
HammadTheOne Sep 8, 2020
26372a3
Cleanup and additional changes to callback value conditionals
HammadTheOne Sep 9, 2020
aa8a9fc
Comment cleanup
HammadTheOne Sep 10, 2020
a4a196a
Added PMC callback validation, removed unnecessary code
HammadTheOne Sep 17, 2020
d19ed4e
Update R/dependencies.R
HammadTheOne Sep 27, 2020
fd09358
Update R/dependencies.R
HammadTheOne Sep 27, 2020
4219296
Update R/dependencies.R
HammadTheOne Sep 27, 2020
6f2539b
Update R/dependencies.R
HammadTheOne Sep 27, 2020
be2e509
Added build to gitignore
HammadTheOne Sep 28, 2020
7ea4f56
Updated dependencies.R
HammadTheOne Sep 28, 2020
18192cd
Update boilerplate docs and add wildcard symbols
HammadTheOne Sep 28, 2020
504233a
Drying up validation code and applying symbol logic
HammadTheOne Sep 29, 2020
1944f6f
Merge branch 'master' into 189-wildcards
HammadTheOne Sep 29, 2020
ba8f58c
Update test to use symbols
HammadTheOne Sep 29, 2020
fc58486
Cleaned up code and added allsmaller test example
HammadTheOne Sep 30, 2020
c94455f
Cleaning up redundant code
HammadTheOne Oct 1, 2020
f7723e0
Merge branch '189-wildcards' of https://github.com/plotly/dashr into …
HammadTheOne Oct 1, 2020
939dc24
Update FUNDING.yml
nicolaskruchten Oct 1, 2020
3c30a78
Updated callback_args logic and example
HammadTheOne Oct 1, 2020
bcec380
Adding basic unittests, updated validation
HammadTheOne Oct 2, 2020
28e0bf4
Fixed response for MATCH callbacks
HammadTheOne Oct 6, 2020
4af91cb
Added integration test and updated examples for docs
HammadTheOne Oct 6, 2020
85e3052
Added additional integration test
HammadTheOne Oct 7, 2020
77981ce
Formatting and cleanup
HammadTheOne Oct 7, 2020
2d84a88
Merge branch 'master' into 189-wildcards
HammadTheOne Oct 7, 2020
e8831dd
update docs
Oct 9, 2020
bf63f8a
Update to-do app
HammadTheOne Oct 10, 2020
fb34b11
Merge branch '189-wildcards' of https://github.com/plotly/dashr into …
HammadTheOne Oct 10, 2020
84f7cdf
Add comments to examples
HammadTheOne Oct 11, 2020
297c0e8
Change empy vector to character type.
HammadTheOne Oct 11, 2020
1ace13d
Update boilerplate text.
HammadTheOne Oct 11, 2020
67166fb
Update tests/integration/callbacks/test_pattern_matching.py
HammadTheOne Oct 11, 2020
aa89f81
Update tests/integration/callbacks/test_pattern_matching.py
HammadTheOne Oct 11, 2020
5d8c12a
Update tests/integration/callbacks/test_pattern_matching.py
HammadTheOne Oct 11, 2020
b2d9f30
Update tests/integration/callbacks/test_pattern_matching.py
HammadTheOne Oct 11, 2020
6c01c78
Update tests/integration/callbacks/test_pattern_matching.py
HammadTheOne Oct 11, 2020
7118828
Update tests/testthat/test-wildcards.R
HammadTheOne Oct 11, 2020
ab327c4
Update wildcards_test.R
HammadTheOne Oct 11, 2020
dc5e2e3
Update wildcards_test.R
HammadTheOne Oct 11, 2020
e06dd57
Update wildcards_test.R
HammadTheOne Oct 11, 2020
d43a59b
Update wildcards_test.R
HammadTheOne Oct 11, 2020
1bc1cfc
Update wildcards_test.R
HammadTheOne Oct 11, 2020
e8a4fb1
Update wildcards_test.R
HammadTheOne Oct 11, 2020
c7fc099
Update wildcards_test.R
HammadTheOne Oct 11, 2020
28560cd
Update wildcards_test.R
HammadTheOne Oct 11, 2020
56f491a
Removed triple colon syntax
HammadTheOne Oct 11, 2020
577deb4
Use seq_along and remove unnecessary unittest
HammadTheOne Oct 11, 2020
617e873
Merge branch 'dev' into 189-wildcards
rpkyle Oct 12, 2020
0250dbc
Update CHANGELOG.md
rpkyle Oct 12, 2020
e745e2f
Update CHANGELOG.md
rpkyle Oct 12, 2020
b9218b7
Add support for arbitrary and sorted keys
HammadTheOne Oct 15, 2020
06b37d8
Whitespace deleted
HammadTheOne Oct 15, 2020
fc45ee7
Added integration tests
HammadTheOne Oct 19, 2020
0b6d543
Fixing test output
HammadTheOne Oct 19, 2020
1bec3c5
Fixing flakiness
HammadTheOne Oct 19, 2020
d777ae1
Update test_pattern_matching.py
HammadTheOne Oct 20, 2020
330eb9d
Update test_pattern_matching.py
HammadTheOne Oct 20, 2020
5969add
Updating boilerplate text and test with generalized keys
HammadTheOne Oct 21, 2020
210d3c1
Minor test fixes
HammadTheOne Oct 21, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions R/dash.R
Original file line number Diff line number Diff line change
Expand Up @@ -211,23 +211,37 @@ Dash <- R6::R6Class(
#
# https://cran.r-project.org/doc/FAQ/R-FAQ.html#Others:
callback_args <- list()

for (input_element in request$body$inputs) {
if(is.null(input_element$value))
if ("id.index" %in% names(unlist(input_element))) {
unlisted_input <- unlist(input_element)
Copy link
Contributor

Choose a reason for hiding this comment

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

We could 🌴 this up a little by moving the unlist(input_element) outside of the if.

If what we're mostly interested in is names(unlisted_input), we could actually assign that instead, and use it where needed below.

Just an observation, but I think this may be a circumstance where using a for loop and a conditional causes a lot of housekeeping that could avoided by retaining the original list structure. For example, here:

values <- unname(unlisted_input[names(unlisted_input) == "value" | names(unlisted_input) == "value.index"])

we have to call names twice, even though we already extracted the names above ☺️

On the other hand, had we left it as a list, we could instead try

values <- unlist(input_list[c("value", "value.index")], use.names=FALSE)

which I think would give us what we want in a slightly easier-to-read/easier-to-follow way. I haven't tried it (and input_list is so compact it hardly matters), but I suspect this might be a bit more efficient for large lists as well.

Copy link
Contributor Author

@HammadTheOne HammadTheOne Sep 29, 2020

Choose a reason for hiding this comment

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

I'm a bit hesitant to change this part for a couple of reasons. For a normal callback, request$body$inputs is already a named list, and unlisting that would make handling regular callbacks more complicated as we then have to append a list of lists to callback_args for default callbacks.

In the case of pattern matching callbacks, each input_element is a list of lists. By unlisting it we can access each of the wildcard inputs. I tried to use a vector to subset as you suggested, but while it works for some scenarios, on the initial call of the PMC we run into a subscript out of bounds error, and it might overcomplicate it to account for another condition. We'll also have to deal with NA values that can come up if the input doesn't have a value.index.

I'll try to figure out a way to simplify it and improve readability, but I think unlisting with a for loop in this case would have to be a necessary evil.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, trust your judgment (since I do as well). Though one remark I'd make is that the out of bounds error may be related to the current default values for callback inputs, which is list(NULL). In that case, we could probably just ensure these values are handled properly, but don't worry about this for now.

We can always revisit the implementation later if we have an epiphany about making it a little more concise, for the moment it looks OK as-is.

values <- unname(unlisted_input[names(unlisted_input) == "value" | names(unlisted_input) == "value.index"])
callback_args <- c(callback_args, ifelse(length(values), list(values), list(NULL)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Here it looks like we're checking if values has a non-zero length. I'd guess that if values = NULL, then the length is zero. If this is correct, could we rewrite this as follows?

callback_args <- c(callback_args, list(values))

}
else if(is.null(input_element$value)) {
callback_args <- c(callback_args, list(list(NULL)))
else
}
else {
callback_args <- c(callback_args, list(input_element$value))
}
}

if (length(request$body$state)) {
for (state_element in request$body$state) {
if(is.null(state_element$value))
if ("id.index" %in% names(unlist(state_element))) {
unlisted_state <- unlist(state_element)
values <- unname(unlisted_state[names(unlisted_state) == "value" | names(unlisted_state) == "value.index"])
callback_args <- c(callback_args, ifelse(length(values), list(values), list(NULL)))
}
else if(is.null(state_element$value)) {
callback_args <- c(callback_args, list(list(NULL)))
else
}
else {
callback_args <- c(callback_args, list(state_element$value))
}
}
}

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

Expand Down Expand Up @@ -639,10 +653,8 @@ Dash <- R6::R6Class(
#' JavaScript function.
callback = function(output, params, func) {
assert_valid_callbacks(output, params, func)

inputs <- params[vapply(params, function(x) 'input' %in% attr(x, "class"), FUN.VALUE=logical(1))]
state <- params[vapply(params, function(x) 'state' %in% attr(x, "class"), FUN.VALUE=logical(1))]

if (is.function(func)) {
clientside_function <- NULL
} else if (is.character(func)) {
Expand Down
62 changes: 62 additions & 0 deletions R/dependencies.R
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
#' @rdname dependencies
#' @export
output <- function(id, property) {
if (is(id, 'list')) {
id = sprintf('{"index":["%s"],"type":"%s"}', id[['index']], id[['type']])
}
structure(
dependency(id, property),
class = c("dash_dependency", "output")
Expand All @@ -24,6 +27,9 @@ output <- function(id, property) {
#' @rdname dependencies
#' @export
input <- function(id, property) {
if (is(id, 'list')) {
id = sprintf('{"index":["%s"],"type":"%s"}', id[['index']], id[['type']])
}
structure(
dependency(id, property),
class = c("dash_dependency", "input")
Expand All @@ -33,6 +39,9 @@ input <- function(id, property) {
#' @rdname dependencies
#' @export
state <- function(id, property) {
if (is(id, 'list')) {
id = sprintf('{"index":["%s"],"type":"%s"}', id[['index']], id[['type']])
}
structure(
dependency(id, property),
class = c("dash_dependency", "state")
Expand All @@ -41,6 +50,9 @@ state <- function(id, property) {

dependency <- function(id = NULL, property = NULL) {
if (is.null(id)) stop("Must specify an id", call. = FALSE)
if (is(id, 'list')) {
id = sprintf('{"index":["%s"],"type":"%s"}', id[['index']], id[['type']])
}
list(
id = id,
property = property
Expand All @@ -54,3 +66,53 @@ dashNoUpdate <- function() {
class(x) <- "no_update"
return(x)
}

# Wildcards Updates
#' @rdname dependencies
#' @export
id_matches <- function(id, other) {
my_id = id
other_id = other
self_list = is(my_id, "list") && length(names(my_id)) != 0
other_list = is(my_id, "list") && length(names(my_id)) != 0

if (self_list != other_list){
return(FALSE)
}
if (self_list){
if (sort(names(my_id)) != sort(names(other_id))){
return(FALSE)
}

for (k in names(my_id)){
other_v = other_id[[k]]
if (my_id[[k]] == other_v){
next
}

v_wild = is(my_id[[k]], "Wildcard")
other_wild = is(other_v, "Wildcard")

if (v_wild || other_wild) {
if (!(v_wild && other_wild)){
next # one wild, one not
}
if (my_id[[k]] == ALL || other_id[[k]] == ALL){
next # either ALL
}
if (my_id[[k]] == MATCH || other_id[[k]] == MATCH){
return(FALSE) # one MATCH, one ALLSMALLER
}
}
else {
return(FALSE)
}
# Here we have to decide how we're using classes or if we're using a different flag.
# The idea is that we need a way to see if the callback is a wildcard or not, and if it is,
# what kind of wildcard it corresponds to. We could use `class(n) <- "Wildcard"` to assign
# the attribute to a component id.
}
}

return(my_id == other_id)
}
87 changes: 72 additions & 15 deletions R/utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -890,29 +890,86 @@ removeHandlers <- function(fnList) {
}

setCallbackContext <- function(callback_elements) {
states <- lapply(callback_elements$states, function(x) {
setNames(x$value, paste(x$id, x$property, sep="."))
})
# Set state elements for this callback

if (length(callback_elements$state[[1]]) == 0) {
states <- sapply(callback_elements$state, function(x) {
setNames(list(x$value), paste(x$id, x$property, sep="."))
})
} else if (is.character(callback_elements$state[[1]][[1]])) {
states <- sapply(callback_elements$state, function(x) {
setNames(list(x$value), paste(x$id, x$property, sep="."))
})
} else {
states <- sapply(callback_elements$state, function(x) {
states_vector <- unlist(x)
setNames(list(states_vector[names(states_vector) == "value" | names(states_vector) == "value.index"]),
paste(as.character(jsonlite::toJSON(x[[1]])), x$property, sep="."))
})
}

splitIdProp <- function(x) unlist(strsplit(x, split = "[.]"))

triggered <- lapply(callback_elements$changedPropIds,
function(x) {
input_id <- splitIdProp(x)[1]
prop <- splitIdProp(x)[2]

id_match <- vapply(callback_elements$inputs, function(x) x$id %in% input_id, logical(1))
prop_match <- vapply(callback_elements$inputs, function(x) x$property %in% prop, logical(1))

value <- sapply(callback_elements$inputs[id_match & prop_match], `[[`, "value")

list(`prop_id` = x, `value` = value)

# The following conditionals check whether the callback is a pattern-matching callback and if it has been triggered.
if (startsWith(input_id, "{")){
id_match <- vapply(callback_elements$inputs, function(x) {
x <- unlist(x)
any(x[names(x) == "id.index"] %in% jsonlite::fromJSON(input_id)[[1]])
}, logical(1))[[1]]
} else {
id_match <- vapply(callback_elements$inputs, function(x) x$id %in% input_id, logical(1))
}

if (startsWith(input_id, "{")){
prop_match <- vapply(callback_elements$inputs, function(x) {
x <- unlist(x)
any(x[names(x) == "property"] %in% prop)
}, logical(1))[[1]]
} else {
prop_match <- vapply(callback_elements$inputs, function(x) x$property %in% prop, logical(1))
}

if (startsWith(input_id, "{")){
if (length(callback_elements$inputs) == 1) {
value <- sapply(callback_elements$inputs[id_match & prop_match], `[[`, "value")
} else {
value <- sapply(callback_elements$inputs[id_match & prop_match][[1]], `[[`, "value")
}
} else {
value <- sapply(callback_elements$inputs[id_match & prop_match], `[[`, "value")
}

if (startsWith(input_id, "{")){
return(list(`prop_id` = x, `value` = value))
} else {
return(list(`prop_id` = x, `value` = value))
}
}
)

inputs <- sapply(callback_elements$inputs, function(x) {
setNames(list(x$value), paste(x$id, x$property, sep="."))
})
)
if (length(callback_elements$inputs[[1]]) == 0) {
inputs <- sapply(callback_elements$inputs, function(x) {
setNames(list(x$value), paste(x$id, x$property, sep="."))
})
} else if (is.character(callback_elements$inputs[[1]][[1]])) {
inputs <- sapply(callback_elements$inputs, function(x) {
setNames(list(x$value), paste(x$id, x$property, sep="."))
})
} else if (length(callback_elements$inputs[[1]]) > 1) {
inputs <- sapply(callback_elements$inputs, function(x) {
inputs_vector <- unlist(x)
setNames(list(inputs_vector[names(inputs_vector) == "value" | names(inputs_vector) == "value.index"]), paste(as.character(jsonlite::toJSON(x$id)), x$property, sep="."))
})
} else {
inputs <- sapply(callback_elements$inputs, function(x) {
inputs_vector <- unlist(x)
setNames(list(inputs_vector[names(inputs_vector) == "value" | names(inputs_vector) == "value.index"]), paste(as.character(jsonlite::toJSON(x[[1]]$id)), x[[1]]$property, sep="."))
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect we could DRY this function up a bit as well. I was hoping to ask you about this block:

if (startsWith(input_id, "{")){
  return(list(`prop_id` = x, `value` = value))
} else {
  return(list(`prop_id` = x, `value` = value))
}

Is it just me or did we end up with some 🍝 in one of those two branches? I kept staring at it thinking I must have missed something. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect we could DRY this function up a bit as well. I was hoping to ask you about this block:

As discussed offline, we're going to see if it's possible to DRY up the first part of the function, and see if there's a cleaner way to handle the pattern matching callbacks case within setCallbackContext:

  } else if (length(callback_elements$inputs[[1]]) > 1) {
    inputs <- sapply(callback_elements$inputs, function(x) {
      inputs_vector <- unlist(x)
      setNames(list(inputs_vector[names(inputs_vector) == "value" | names(inputs_vector) == "value.index"]), paste(as.character(jsonlite::toJSON(x$id)), x$property, sep="."))
    })
  } else {
    inputs <- sapply(callback_elements$inputs, function(x) {
      inputs_vector <- unlist(x)
      setNames(list(inputs_vector[names(inputs_vector) == "value" | names(inputs_vector) == "value.index"]), paste(as.character(jsonlite::toJSON(x[[1]]$id)), x[[1]]$property, sep="."))
    })
  }


return(list(states=states,
triggered=unlist(triggered, recursive=FALSE),
Expand Down
148 changes: 148 additions & 0 deletions wildcards_test.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
# Simple Example with ALL
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice sample app! As far as tests go, I imagine we might want to consider

  • unit tests in tests/testthat (for throwing errors when callbacks are misspecified, after app$callback is invoked, but before app$run_server() is used)
  • a simple integration test to check functionality at adding/removing elements (like with Alex's to-do list app)
  • at least some of the checks we implemented in https://github.com/plotly/dash/blob/master/tests/integration/devtools/test_callback_validation.py, we should probably ensure we error out on the same items in R as we do in Python (and maybe provide the same messages when possible)


library(dash)
library(dashCoreComponents)
library(dashHtmlComponents)

app <- Dash$new()

app$layout(htmlDiv(list(
htmlButton("Add Filter", id="add-filter", n_clicks=0),
htmlDiv(id="dropdown-container", children=list()),
htmlDiv(id="dropdown-container-output"),
dccInput(id='my-id', value='initial value', type='text'),
htmlDiv(id='my-div')
)))


app$callback(
output(id="dropdown-container", property = "children"),
params = list(
input(id = "add-filter", property = "n_clicks"),
state(id = "dropdown-container", property = "children")
),
display_dropdowns <- function(n_clicks, children){
new_dropdown = dccDropdown(
id=list(
"index" = n_clicks,
"type" = "filter-dropdown"
),
options = lapply(1:4, function(x){
list("label" = x, "value" = x)
})
)
children[[n_clicks + 1]] <- new_dropdown
return(children)
}
)


app$callback(
output(id="dropdown-container-output", property="children"),
params = list(
input(id=list("index" = "ALL", "type" = "filter-dropdown"), property= "value")
),
display_output <- function(test){
print(test)
return(htmlDiv(
lapply(1:length(test), function(x){
return(htmlDiv(sprintf("Dropdown %s = %s", x, test[[x]])))
})
))
}
)


app$callback(
output=list(id='my-div', property='children'),
params=list(input(id='my-id', property='value')),
function(input_value) {
sprintf("You've entered \"%s\"", input_value)
})


app$run_server(debug=F, showcase = TRUE)

# Standard Callback Example

library(dash)
library(dashCoreComponents)
library(dashHtmlComponents)

app <- Dash$new()

app$layout(
htmlDiv(
list(
dccInput(id='my-id', value='initial value', type='text'),
htmlDiv(id='my-div')
)
)
)

app$callback(
output=list(id='my-div', property='children'),
params=list(input(id='my-id', property='value')),
function(input_value) {
sprintf("You've entered \"%s\"", input_value)
})

app$run_server()


# Simple Example with MATCH

library(dash)
library(dashCoreComponents)
library(dashHtmlComponents)


app <- Dash$new()

app$layout(htmlDiv(list(
htmlButton("Add Filter", id="dynamic-add-filter", n_clicks=0),
htmlDiv(id="dynamic-dropdown-container", children=list())
)))


app$callback(
output(id="dynamic-dropdown-container", "children"),
params = list(
input("dynamic-add-filter", "n_clicks"),
state("dynamic-dropdown-container", "children")
),
display_dropdown <- function(n_clicks, children){
new_element = htmlDiv(list(
dccDropdown(
id = list("type" = "dynamic-dropdown", "index" = sprintf("%s", n_clicks)),
options = lapply(c("NYC", "MTL", "LA", "TOKYO"), function(x){
list("label" = x, "value" = x)
})
),
htmlDiv(
id = list("type" = "dynamic-output", "index" = sprintf("%s", n_clicks))
)
))
children <- c(children, list(new_element))
return(children)
}
)


app$callback(
output(list("type" = "dynamic-output", "index" = "MATCH"), "children"),
params = list(
input(list("type" = "dynamic-dropdown", "index" = "MATCH"), "value"),
state(list("type" = "dynamic-dropdown", "index" = "MATCH"), "id")
),
display_output <- function(value, id){
print(id)
print(value)
return(htmlDiv(list(
id, value
)))
}
)

app$run_server(showcase = T, debug = F)