Minimally invasive result field fix

[nbenn]

For my two example cases, this works. Have not checked more "exotic" scenarios such as plot blocks etc.

[nbenn]

@DivadNojnarg many thanks for the pointer

if you remove req(is_valid$block) and replace by if (!is_valid$block) return(data.frame()).

[DivadNojnarg]

@nbenn you probably want to remove the expect_error to fix the broken test: https://github.com/BristolMyersSquibb/blockr/pull/435/files#diff-e3fcd79bc10ab1ead4020ff2d87bb6d914b30e88828a04babc7b51abf8ccea5a as we don't use req anymore.

[DivadNojnarg]

@nbenn We need some safe gards whenever the data passed from one block to another is NULL and the block isn't a data block.
For instance, at the moment, if you empty the data selection in the first block, the app crashes since we try to select on an empty data.frame :

serve_workspace(
  stack1 = new_stack(new_dataset_block("BOD"), new_select_block("Time")),
  stack2 = new_stack(new_result_block("stack1"))
)

Something that also accounts for plot_block (since plot block are not transformed block and if you pass NULL data, you can enter the else if statement which would crash with a layer):

reactive({
          if (!is_valid$block) {
            NULL
          } else if (is.null(in_dat()) && !inherits(x, "transform_block") && !inherits(x, "plot_block")) {
            browser()
            evaluate_block(blk())
          } else {
           # Safe guard
            if (is.null(in_dat())) return(NULL)
            evaluate_block(blk(), data = in_dat())
          }
        })

You can test this by emptying data in the first block below:

library(palmerpenguins)
library(ggplot2)

new_ggplot_block <- function(col_x = character(), col_y = character(), ...) {

  data_cols <- function(data) colnames(data)

  new_block(
    fields = list(
      x = new_select_field(col_x, data_cols, type = "name"),
      y = new_select_field(col_y, data_cols, type = "name")
    ),
    expr = quote(
      ggplot(mapping = aes(x = .(x), y = .(y)))
    ),
    class = c("ggplot_block", "plot_block"),
    ...
  )
}

new_geompoint_block <- function(color = character(), shape = character(), ...) {

  data_cols <- function(data) colnames(data$data)

  new_block(
    fields = list(
      color = new_select_field(color, data_cols, type = "name"),
      shape = new_select_field(shape, data_cols, type = "name")
    ),
    expr = quote(
      geom_point(aes(color = .(color), shape = .(shape)), size = 2)
    ),
    class = c("plot_layer_block", "plot_block"),
    ...
  )
}

serve_workspace(
  stack1 = new_stack(new_dataset_block("penguins", "palmerpenguins"), new_ggplot_block("body_mass_g", "flipper_length_mm"), new_geompoint_block("species", "species"))
)

[nbenn]

Good points @DivadNojnarg, thanks! I was suspecting that there might be a bit of fallout with this NULL result value. But I do feel that moving in such a direction is probably good (to have a generic NULL instead of more "block-specific" values such as data.frame() etc.). Do you agree with this sentiment?

(Maybe the "minimally-invasive" part of the title was a bit misleading..)

[DivadNojnarg]

Do you agree with this sentiment?

Yes I prefer NULL over having to specify different empty constructors.

[nbenn]

@DivadNojnarg do you know why we do

blockr/R/server.R

Line 227 in 061ac78

if (is.null(in_dat()) && !inherits(x, "transform_block")) {

in the first place? Wouldn't this better be something like if (inherits(x, "data_block")?

(Side remark: the logic in

blockr/R/server.R

Line 214 in 061ac78

if (is.null(in_dat())) {

should be exactly the same, no?)

For the other issue you point out:

For instance, at the moment, if you empty the data selection in the first block, the app crashes since we try to select on an empty data.frame

High-level, I feel the problem is somewhere else: if you empty the dataset selection, the datase block should turn "invalid" and the select block should then not evaluate. So we'd need to factor in is_prev_valid here too. Does that sound reasonable?

[nbenn]

@DivadNojnarg

We need some safe gards whenever the data passed from one block to another is NULL and the block isn't a data block.

Does this happen, i.e. that we have NULL as a result of a "valid" block? I was hoping that this could be avoided.

[DivadNojnarg]

The first point is likely an accidental removal. They should have the same conditions.

Second point: something like:

reactive({
          if (!is_valid$block || (!is.null(is_prev_valid()) && !isTRUE(is_prev_valid()))) {
            NULL
          } else if (is.null(in_dat()) && !inherits(x, "transform_block") && !inherits(x, "plot_block")) {
            evaluate_block(blk())
          } else {
            evaluate_block(blk(), data = in_dat())
          }
        })

[DivadNojnarg]

In the update block observer, we also may want to prevent to update the block if the previous one is invalid:

obs$update_blk <- observeEvent(c(r_values(), in_dat(), is_prev_valid()),
        {
          if (!is.null(is_prev_valid()) && !isTRUE(is_prev_valid())) return(NULL)
         ...
})

An even better alternative would be to avoid triggering the event at all. However, with our current logic, you can't just do req(is_prev_valid()) since for data block is_prev_valid() is always NULL, and this would block the whole chain. That said, it could be rewritten a bit like so:

# I must admit this thing should be done with different S3 server methods, but as we don't have them anymore ...
update_blk_trigger <- reactive({
   if (inherits(x, "data_block")) {
     r_values()
   } else {
     c(r_values(), in_dat(), req(is_prev_valid()))
   }
})

obs$update_blk <- observeEvent(update_blk_trigger(), { ...})