189 - Add Pattern Matching Callbacks for Dash R

[HammadTheOne]

This PR adds front-end support in Dash for R for parsing and passing through Pattern Matching ID's and values into callback arguments, similar to plotly/dash#1103. It involves changes to the route handler for callbacks in Dash for R, callback context updates to correctly identify triggered component ID's and values, and dependency changes to allow input, output, and state id's to be passed as lists.

An example of PMC for Dash R in action:

This PR is still a work in progress, standard unit tests and callback validation have to be added before it is merged.

• Before merging, remove wildcards_test.R - Placeholder for easier local testing.

Closes #189

[rpkyle]

Nice work! This feature required some serious work to get right, definitely not one of the simpler improvements to the framework in R, but definitely one of the most valuable.

The majority of my comments relate to DRYing up the code a bit, but there are some spots where the code is a little hard to follow without comments. In particular, we should probably review setCallbackContext since this function has gotten a lot more complex and I'm not completely sure what's happening in a few spots.

On the testing side of things, we'll definitely want to add a couple unit tests to ensure errors are thrown, and at least a basic integration test to confirm that a simple app using a pattern matching callback (like the to-do list example) works as expected. Really excited to see the arrival of this feature in R, one of the cooler improvements to Dash in quite a while.

👏 🏆 👏

[rpkyle]

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.

[HammadTheOne]

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.

[rpkyle]

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.

[rpkyle]

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. 🤔

[rpkyle]

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="."))
    })
  }

[rpkyle]

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)

[rpkyle]

@HammadTheOne In addition to Alex's comments, do we currently handle/store outputs_list, inputs_list, and states_list in Dash for R now? I only ask because of what I see here.

In Python, it looks like these are stored via Flask's application context; I believe in Fiery the analog is assigning these values via self$server$set_data and retrieving them via self$server$get_data as in #224.

(callback_context was initially added in #87, in 140bf51. 🙈 It's a little hard to tell because this one probably should've had its own PR, but I added it to something in progress since it was needed for an application in development and was blocking.)

[alexcjohnson]

💃 Fantastic, that's all from me!

[rpkyle]

💃 🚀