Skip to content

Propagate errors from check_installed() #4845

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.

Sign up for GitHub

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

Closed
wants to merge 9 commits into from
Closed

Propagate errors from check_installed() #4845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
996a59c
Propagate errors from check_installed().
yutannihilation May 14, 2022
cb894a9
Use abort()
yutannihilation May 14, 2022
56778bf
Add tests
yutannihilation May 14, 2022
3115900
Skip examples if Hmisc is not installed
yutannihilation May 14, 2022
4d59326
Skip examples if hexbin is not installed
yutannihilation May 14, 2022
bc7e725
Merge remote-tracking branch 'upstream/main' into feature/gracefully-…
yutannihilation May 18, 2022
8d7d97b
Use cli_abort()
yutannihilation May 18, 2022
b016039
Fix
yutannihilation May 18, 2022
2948069
Empty commit to try invoking the CI
yutannihilation May 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions R/geom-hex.r
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#' @inheritParams geom_point
#' @export
#' @examples
#' if (!requireNamespace("hexbin", quietly = TRUE)) {
#' d <- ggplot(diamonds, aes(carat, price))
#' d + geom_hex()
#'
Expand All @@ -27,6 +28,7 @@
#' d + geom_hex(binwidth = c(1, 1000))
#' d + geom_hex(binwidth = c(.1, 500))
#' }
#' }
geom_hex <- function(mapping = NULL, data = NULL,
stat = "binhex", position = "identity",
...,
Expand Down
16 changes: 12 additions & 4 deletions R/stat-.r
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,18 @@ Stat <- ggproto("Stat",
args <- c(list(data = quote(data), scales = quote(scales)), params)
dapply(data, "PANEL", function(data) {
scales <- layout$get_scales(data$PANEL[1])
try_fetch(do.call(self$compute_panel, args), error = function(cnd) {
cli::cli_warn("Computation failed in {.fn {snake_class(self)}}", parent = cnd)
new_data_frame()
})
try_fetch(do.call(self$compute_panel, args),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, I think this should be more like:

try_fetch(do.call(self$compute_panel, args),
  rlib_error_package_not_found = function(cnd) {
    cli::cli_abort("Aborted computation in {.fn {snake_class(self)}}", parent = cnd)
  },
  error = function(cnd) {
    cli::cli_warn("Computation failed in {.fn {snake_class(self)}}", parent = cnd)
    new_data_frame()
  }
)

Copy link
Member

@hadley hadley May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that the in {.fn {snake_class(self)}} is temporary until we replace it with a proper call. Not sure if we want to do that now or later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that the in {.fn {snake_class(self)}} is temporary unless we replace it with a proper call. Not sure if we want to do that now or later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks not bad to me, but let me think. I'm a noob at cli...

> ggplot(diamonds, aes(carat, price)) + geom_hex()
Error in `fun()` at ggplot2/R/compat-plyr.R:375:4:
! Aborted computation in `stat_binhex()`
Caused by error in `f()` at ggplot2/R/ggproto.r:178:11:
! The package `hexxbin` is required for `stat_binhex()`
Run `rlang::last_error()` to see where the error occurred.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd structure the errors in this way, just to avoid multiple formulations of the same idea (an error occurred):

Error in `fun()` at ggplot2/R/compat-plyr.R:375:4:
Caused by error in `stat_binhex()`:
Caused by error in `f()` at ggplot2/R/ggproto.r:178:11:
! The package `hexxbin` is required for `stat_binhex()`

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caused by error in stat_binhex():
Caused by error in f() at ggplot2/R/ggproto.r:178:11:

I couldn't figure out how to structure like this. Can we add the additional "Caused by" like without providing the error message (the line starting with !)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, I think this should be more like:

try_fetch(do.call(self$compute_panel, args),
  rlib_error_package_not_found = function(cnd) {
    cli::cli_abort("Aborted computation in {.fn {snake_class(self)}}", parent = cnd)
  },
  error = function(cnd) {
    cli::cli_warn("Computation failed in {.fn {snake_class(self)}}", parent = cnd)
    new_data_frame()
  }
)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought so at first, but the rethrown error gets caught in the error case, so I had to do it all in error case. Are there any better way?

ignore_all_but_custom_error <- function(expr) {
  result <- rlang::try_fetch(
    expr,
    custom_error = function(cnd) {
      rlang::inform("custom_error detected")
      rlang::abort("rethrowing the error", parent = cnd)
    },
    error = function(cnd) {
      rlang::inform("ignoring the error")
    },
  )
  
  invisible(result)
}

# works as expected
ignore_all_but_custom_error(stop("foo"))
#> ignoring the error

# hmm...
ignore_all_but_custom_error(rlang::abort("foo", class = "custom_error"))
#> custom_error detected
#> ignoring the error

Created on 2022-05-20 by the reprex package (v2.0.1)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I suspect this is because of the way that tryCatch() adds the calling handlers behind the scenes. Hopefully @lionel- can suggest a fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm wondering if check_installed() should signal non-error conditions. This way it'd work in tryCatch().

There are two steps currently:

  1. Signal a classed condition (that currently inherits from "error") with a restart.
  2. Call invokeRestart("abort") if user selected no, bypassing everything on the stack including error handlers.

The second step is tryCatch()-proof but not the first step. If it were, I think no complications would be needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But doesn't the same problem arise with any classed condition? i.e. you can't use tryCatch() to handle a classed error and all other errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have to keep the error class as errors, otherwise they wouldn't get caught by a generic error argument?

Isn't the whole point of the design of tryCatch() to allow you to deal with error subclasses individually or as a whole?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering whether check-installed really follows the error model or instead is more like an interrupt.

  • Like interrupts, the current computation is aborted after user interaction, when the user selects "No". By contrast errors are triggered by the program rather than the user.

  • Like interrupts, this is accomplished using invokeRestart("abort"), at which point nothing can prevent the jump (except an intercepting "abort" restart).

Originally I restarted to "abort" instead of throwing an error to improve the UI. When the user selects no, they are taken back to a prompt instead of seeing an error.

Also note that currently tryCatch(error = ) is causing check_installed() to exit too soon, I think it's a bug. We added a restart mechanism in r-lib/rlang#1199 in which we signal the error condition instead of a bare condition. I think this is not correct since condition communication should always be done with bare conditions (or more precisely classed conditions that don't inherit from error, warning, or message) to avoid catch-all handlers.

We can fix the restart signal protocol but then we still need to think about what UI do we want to implement:

  1. Should check_installed() prompt the user at all within tryCatch(error = )?

  2. If we do prompt the user, and they select "No", should they consistently be taken back to the prompt even when within tryCatch()? In that case, this means we implement the interrupt model in interactive sessions. Otherwise, we should resignal the condition, this time inheriting from "error".

Regarding (1), I think we should prompt the user in interactive sessions, since that is the documented purpose of check_installed(). Though I'm a bit concerned about users setting up a loop e.g. with safely() to run a long computation and then come back to their computer the next day to find the check_installed() prompt. Maybe we should detect that we are called within source() and in that case behave as if the session was non-interactive?

Regarding (2), I think the interrupt model makes the most sense to me. I'd be a bit surprised to select "No" and then see the computation continue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that makes sense — that way the initial call is an interrupt and it only becomes an error if you select "No". I'm not too concerned with your safely() scenario, because I think it will be relatively unlikely that a computation half-way through the loop will require package installation, and generally you will have prototyped at least once before doing the loop.

I agree that selecting "No" should take you to the top level, as otherwise in the code below you'd need to select "No" 10 times in order to exit the map():

f <- function(i) {
  # make a plot that uses hexbin
  check_installed("hexbin")
}
purrr::map(1:10, safely(f))

Copy link
Member

@lionel- lionel- May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that selecting "No" should take you to the top level,

hmm but then we agree that check_installed() is either a no-op (after a restart if user selects yes) or an interrupt (if no is selected), never an error, right?

Just making sure because you also say:

the initial call is an interrupt and it only becomes an error if you select "No".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point regarding repeated prompts in loops by the way. This clearly shows why check_installed() should implement the interrupt model rather than the error model.

error = function(cnd) {
# if the error comes from check_installed(), propagate it immediately.
if (inherits(cnd, "rlib_error_package_not_found")) {
cli::cli_abort("Aborted computation in {.fn {snake_class(self)}}", parent = cnd)
}

# for other errors, ignore them with warnings
cli::cli_warn("Computation failed in {.fn {snake_class(self)}}", parent = cnd)
new_data_frame()
}
)
})
},

Expand Down
2 changes: 2 additions & 0 deletions R/stat-summary.r
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
#' @param fun.args Optional additional arguments passed on to the functions.
#' @export
#' @examples
#' if (requireNamespace("Hmisc", quietly = TRUE)) {
#' d <- ggplot(mtcars, aes(cyl, mpg)) + geom_point()
#' d + stat_summary(fun.data = "mean_cl_boot", colour = "red", size = 2)
#'
Expand Down Expand Up @@ -126,6 +127,7 @@
#' m2 + coord_trans(y="log10")
#' }
#' }
#' }
stat_summary <- function(mapping = NULL, data = NULL,
geom = "pointrange", position = "identity",
...,
Expand Down
2 changes: 2 additions & 0 deletions man/geom_hex.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions man/stat_summary.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions tests/testthat/test-stat-summary.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,18 @@ test_that("stat_summary_(2d|hex) work with lambda expressions", {
)

})

test_that("stat_summary() errors when check_installed() fails (#4736)", {
fun1 <- function(...) {
stop("usual error")
}

fun2 <- function(...) {
check_installed("nosuchpackage")
}

p <- ggplot(data.frame(x = 1), aes(x, x))

expect_warning(expect_error(ggplot_build(p + stat_summary(fun.data = fun1)), NA))
expect_error(ggplot_build(p + stat_summary(fun.data = fun2)), class = "rlib_error_package_not_found")
})