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

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

Closed
HenrikBengtsson opened this Issue Feb 20, 2018 · 10 comments

Comments

Projects
None yet
2 participants
@HenrikBengtsson
Owner

HenrikBengtsson commented Feb 20, 2018

From rstudio/shiny#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 HenrikBengtsson added the bug label Feb 20, 2018

@HenrikBengtsson HenrikBengtsson added this to the Next release milestone Feb 20, 2018

HenrikBengtsson added a commit that referenced this issue Feb 20, 2018

@HenrikBengtsson

This comment has been minimized.

Owner

HenrikBengtsson commented Feb 20, 2018

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

@jcheng5

This comment has been minimized.

jcheng5 commented Feb 20, 2018

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

This comment has been minimized.

Owner

HenrikBengtsson commented Feb 20, 2018

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

@jcheng5

This comment has been minimized.

jcheng5 commented Feb 20, 2018

Thanks, really appreciate it!

HenrikBengtsson added a commit that referenced this issue Feb 22, 2018

BUG FIX: value() for ClusterFuture would not signal errors unless the…
…y inherited from 'simpleError' [#200]

BUG FIX: value() for ClusterFuture no longer produces a FutureEvaluationError - only FutureError

HenrikBengtsson added a commit that referenced this issue Feb 22, 2018

HenrikBengtsson added a commit that referenced this issue Feb 22, 2018

HenrikBengtsson added a commit that referenced this issue Feb 23, 2018

Introducing FutureResult for returning richer sets of information fro…
…m resolved futures [#25, #59, #67, #154, #188 #199, #200]. This also fixes #199 and #200.

HenrikBengtsson added a commit that referenced this issue Feb 24, 2018

@HenrikBengtsson

This comment has been minimized.

Owner

HenrikBengtsson commented Mar 6, 2018

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

This comment has been minimized.

Owner

HenrikBengtsson commented Mar 6, 2018

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

@HenrikBengtsson

This comment has been minimized.

Owner

HenrikBengtsson commented Mar 11, 2018

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

@jcheng5

This comment has been minimized.

jcheng5 commented Mar 11, 2018

Sorry, will try on Monday

@jcheng5

This comment has been minimized.

jcheng5 commented Mar 12, 2018

It seems to work fine! Thanks!

@HenrikBengtsson

This comment has been minimized.

Owner

HenrikBengtsson commented Mar 13, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment