Attempt to fix Result block don't get updated properly #432 #435

DivadNojnarg · 2024-10-14T08:53:20Z

@nbenn. I believe this PR fixes the result block update issues, as well as the join block problem. I left comments next to each change to give further context.

I tested this PR with this codes:

Stack with join block

serve_workspace(
  stack1 = new_stack(
    blockr.pharmaverseadam::new_adam_block("adae")
  ),
  stack2 = new_stack(blockr.pharmaverseadam::new_adam_block("adsl"), new_join_block(y = "stack1"))
)

Stack with result block:

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

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

Again, for the long term, I still have concerns with passing reactive expressions between modules within attributes as it may be very confusing for someone to read and debug the code. That said, in the sort term, as the workshop is coming very soon, we can go ahead with this and try to rework/improve it in the #427 stack validation PR. Happy to chat about it.

… string.

DivadNojnarg · 2024-10-14T13:50:46Z

R/server.R

@@ -21,6 +21,8 @@ generate_server.result_field <- function(x, ...) {
        res <- get_stack_result(
          get_workspace_stack(inp)
        )
+        # Handle the join block


The join block was broken since we do this:

by_choices <- function(data, y) { intersect(colnames(data), colnames(y)) }

and y is a reactive expression coming from the result field, which means it needs to be evaluated to get the correct result. I believe the best place to do this is here, in the server logic rather than in the code block code.

This shouldn't cause problems: y from "within the block" should not behave as reactive, but as regular R value. All of this should happen in value():

blockr/R/field-core.R

Lines 184 to 209 in 061ac78

value.result_field <- function(x, name = "value") {

stopifnot(identical(name, "value"))

field <- get_field_value(x, "value")

if (length(field) && field %in% list_workspace_stacks()) {

res <- get_stack_result(get_workspace_stack(field))

if (inherits(res, "reactive")) {

if (is.null(getDefaultReactiveDomain())) {

list()

} else {

res()

}

} else {

res

}

} else {

list()

}

}

So if this is still an open issue (is it?), the question is why the "reactive representation" slips through here.

Sorry. missed this part of the code. Debugging with a new_join_block shows we never enter the first if statement as list_workspace_stacks() always return character(0).
To reproduce:

serve_workspace( stack1 = new_stack( blockr.pharmaverseadam::new_adam_block("adae") ), stack2 = new_stack(blockr.pharmaverseadam::new_adam_block("adsl"), new_join_block(y = "stack1")) )

DivadNojnarg · 2024-10-14T13:52:23Z

R/server.R

@@ -702,9 +714,17 @@ generate_server.workspace <- function(x, id, ...) {
      })

      attr(x, "reactive_stack_directory") <- reactive({
-        names(vals$stacks)
+        vals$stacks


We need to pass the stacks to do further check within the result field server logic.

Just for my understanding: why do we need to pass through the entire stacks here? Initially the intention for the reactive_stack_directory was just to give a reactive object for populating result field drop-downs. Where do we need the reactive stack? Sorry, I didn't spot anything relevant in your changes.

I see, it is the validity check for the dependent stack that this is used for. If this is the chase, the result field validator should take care of this, no? Can we extend

blockr/R/fields.R

Lines 298 to 312 in 061ac78

validate_field.result_field <- function(x) {

field <- get_field_value(x, "value")

validate_string(field)

if (!field %in% list_workspace_stacks()) {

validation_failure(

"result fields have to refer to existing stack names",

class = "result_failure"

)

}

NextMethod()

}

to not only check for existence of a stack, but also it's "validity"?

I played around this idea in 5822c89. However, while it works well for a "static" case, that is, with predefined stacks, it fails in the dynamic case when block are added on the fly:

# works serve_workspace( stack1 = new_stack(new_dataset_block("BOD"), new_select_block()), stack2 = new_stack(new_result_block("stack1")) ) # fails: to replicate, add a new_select_block on the fly and nothing happens. serve_workspace( stack1 = new_stack(new_dataset_block("BOD")), stack2 = new_stack(new_result_block("stack1")) )

Actually this just does not work. My workspace was already contaminated by other existing stacks so I though it worked but no ...

DivadNojnarg · 2024-10-14T13:53:12Z

R/server.R

-        chr_ply(lapply(vals$stacks, `[[`, "stack"), attr, "title")
+        c(
+          chr_ply(lapply(vals$stacks, `[[`, "stack"), attr, "title"),
+          lapply(vals$stack, \(stack) {


added an extra trigger each time a block changes and it's validity change, otherwise the result field does not update consistently.

DivadNojnarg · 2024-10-14T13:54:00Z

R/server.R

-        ),
+        {
+          lapply(vals$blocks, \(block) {
+            req(block)


a little extra check to prevent this event from triggering when vals$block is a list of NULL elements.

DivadNojnarg · 2024-10-14T13:54:55Z

R/server.R

@@ -44,13 +44,18 @@ generate_server.result_field <- function(x, ...) {
        updateSelectInput(
          session,
          "select-stack",
-          choices = result_field_stack_opts(session$ns, workspace_stacks()),
+          choices = result_field_stack_opts(session$ns, names(workspace_stacks())),


Since we pass the entire stack object in the reactive_stack_directory, we just extract the names to display them in the select choices.

DivadNojnarg · 2024-10-14T13:56:27Z

R/server.R

@@ -50,6 +50,7 @@ generate_server.result_field <- function(x, ...) {
      )

      reactive({
+        req(input[["select-stack"]])


This is needed now as this expression takes dependencies on other reactive element. Avoids to subset when input$select-stack is NULL.

DivadNojnarg · 2024-10-14T13:57:48Z

R/server.R

@@ -210,7 +219,7 @@ generate_server_block <- function(
      out_dat <- if (attr(x, "submit") > -1) {
        eventReactive(input$submit,
          {
-            req(is_valid$block)


req was blocking the reactive chain. We just return an empty data.frame if invalid.

DivadNojnarg · 2024-10-14T13:58:37Z

R/server.R

          if (is.null(in_dat()) && !inherits(x, "transform_block")) {
            evaluate_block(blk())
          } else {
+            if (nrow(in_dat()) == 0 && !inherits(x, "parser_block")) return(data.frame())


parser_block are an exception as their input isn't rectangular data but a file path.

I actually replaced this by:

if (inherits(in_dat(), "data.frame") && nrow(in_dat()) == 0) { return(data.frame()) }

as it broke the plots.

DivadNojnarg · 2024-10-14T14:11:09Z

R/server.R

+        all_valid <- lgl_ply(blocks, \(block) {
+          block$is_valid()
+        })
+        if (all(all_valid) == TRUE) get_result(input[["select-stack"]]) else data.frame()


Overall, we check that the previous stack is valid (otherwise the result block has no way to know about the previous blocks from the linked stack). Note: this will be superseded in the stack validation PR #427.

DivadNojnarg · 2024-10-14T14:33:41Z

R/server.R

@@ -236,7 +236,9 @@ generate_server_block <- function(
          if (is.null(in_dat()) && !inherits(x, "transform_block")) {
            evaluate_block(blk())
          } else {
-            if (nrow(in_dat()) == 0 && !inherits(x, "parser_block")) return(data.frame())


sorry this thing was a dumb idea from me ...

Attempt to fix Result block don't get updated properly #432

a9330aa

DivadNojnarg mentioned this pull request Oct 14, 2024

Result block don't get updated properly #432

Open

DivadNojnarg added 3 commits October 14, 2024 14:48

fix issue with parser block since input data isn't a data.frame but a…

6f67cb5

… string.

wait for input[["select-stack"]]

38a2f60

fix join_block by issue (empty intersection)

b4d605a

DivadNojnarg commented Oct 14, 2024

View reviewed changes

fix issue with plot blocks, bump version + fix tests

8260e59

DivadNojnarg commented Oct 14, 2024

View reviewed changes

DivadNojnarg marked this pull request as ready for review October 14, 2024 14:50

DivadNojnarg requested a review from nbenn as a code owner October 14, 2024 14:50

fix Min version dependency for shinyWidgets #434

c03f1d5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt to fix Result block don't get updated properly #432 #435

Attempt to fix Result block don't get updated properly #432 #435

DivadNojnarg commented Oct 14, 2024 •

edited

Loading

DivadNojnarg Oct 14, 2024

nbenn Oct 15, 2024

DivadNojnarg Oct 15, 2024 •

edited

Loading

DivadNojnarg Oct 14, 2024

nbenn Oct 15, 2024

nbenn Oct 15, 2024

DivadNojnarg Oct 15, 2024

DivadNojnarg Oct 15, 2024

DivadNojnarg Oct 14, 2024

DivadNojnarg Oct 14, 2024

DivadNojnarg Oct 14, 2024

DivadNojnarg Oct 14, 2024

DivadNojnarg Oct 14, 2024

DivadNojnarg Oct 14, 2024

DivadNojnarg Oct 14, 2024

DivadNojnarg Oct 14, 2024

DivadNojnarg Oct 14, 2024 •

edited

Loading

	value.result_field <- function(x, name = "value") {

	stopifnot(identical(name, "value"))

	field <- get_field_value(x, "value")

	if (length(field) && field %in% list_workspace_stacks()) {

	res <- get_stack_result(get_workspace_stack(field))

	if (inherits(res, "reactive")) {

	if (is.null(getDefaultReactiveDomain())) {
	list()
	} else {
	res()
	}

	} else {
	res
	}

	} else {
	list()
	}
	}

	validate_field.result_field <- function(x) {

	field <- get_field_value(x, "value")

	validate_string(field)

	if (!field %in% list_workspace_stacks()) {
	validation_failure(
	"result fields have to refer to existing stack names",
	class = "result_failure"
	)
	}

	NextMethod()
	}

Attempt to fix Result block don't get updated properly #432 #435

Are you sure you want to change the base?

Attempt to fix Result block don't get updated properly #432 #435

Conversation

DivadNojnarg commented Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DivadNojnarg Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DivadNojnarg Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

DivadNojnarg commented Oct 14, 2024 •

edited

Loading

DivadNojnarg Oct 15, 2024 •

edited

Loading

DivadNojnarg Oct 14, 2024 •

edited

Loading