futureverse / future

:rocket: R package: future: Unified Parallel and Distributed Processing in R for Everyone
https://future.futureverse.org
955 stars 85 forks source link

BUG: signal on "error" - not "simpleError" #200

Closed HenrikBengtsson closed 6 years ago

HenrikBengtsson commented 6 years ago

From https://github.com/rstudio/shiny/issues/1949: value() for MulticoreFuture test whether an error has occurred or not by checking for inheritance of simpleError. This should be for error.

Look for other related mistakes on simpleError and possibly simpleWarning (which is not signalled).

HenrikBengtsson commented 6 years ago

@jcheng5, this has now been fixed in the develop branch; I only fixed multicore - there might be other cases.

jcheng5 commented 6 years ago

It almost works... now the error is propagated, but the custom error classes are lost. You can see this in the example below (you'll need to devtools::install_github("rstudio/shiny") to get the future/async-compatible version of Shiny). Both outputs should show the same (gray) text, but instead the future one is red:

library(shiny)
library(promises)
library(future)
library(ggplot2)
plan(multiprocess)

ui <- fluidPage(
  plotOutput("plot1", height = 200),
  plotOutput("plot2", height = 200)
)

server <- function(input, output, session) {
  output$plot1 <- renderPlot({
    # This looks good--gray validation message
    validate(need(FALSE, "Validation error"))
    ggplot(NULL)
  })

  output$plot2 <- renderPlot({
    # This looks wrong--red error
    future({ validate(need(FALSE, "Validation error")) }) %...>%
      { ggplot(.) }
  })
}

shinyApp(ui, server)
HenrikBengtsson commented 6 years ago

ok. I'll try to look into this tomorrow or so.

jcheng5 commented 6 years ago

Thanks, really appreciate it!

HenrikBengtsson commented 6 years ago

Give the develop branch a try; it should now re-signal error objects "as-is" without messing with the class attribute.

EDIT 201-03-06: *now (was 'not')

HenrikBengtsson commented 6 years ago

Important typo in my previous comment: it should say 'now' (not 'not')

HenrikBengtsson commented 6 years ago

I'm closing this one; feel free to re-open if you find that develop does not work as expected.

jcheng5 commented 6 years ago

Sorry, will try on Monday

jcheng5 commented 6 years ago

It seems to work fine! Thanks!

HenrikBengtsson commented 6 years ago

Thanks for confirming.

It looks like it also works with the current future.callr::callr backend (although I'll tweak that a bit too), but other backends such as future.batchtools will have be updated - that's in the to-do pipeline.