Propagate errors from check_installed() #4845

yutannihilation · 2022-05-14T07:34:02Z

Ideally, it's nice if check_install() would interactively ask users to install the package, but it seems it doesn't work inside tryCatch(), which is used in Stat$compute_layer().

> tryCatch(rlang::check_installed("ggfortify"), error = function(e) print(e))
<error/rlib_error_package_not_found>
Error:
! The package `ggfortify` is required.

As a workaround, this pull request propagates the error to the user instead of gracefully drawing a blank plot with a mysterious warning "Computation failed".

hadley · 2022-05-17T21:26:44Z

I think it's probably worth waiting on @thomasp85's revamp of wrapped errors before doing anything here. We now have a much better mechanism for rethrowing errors while maintaining context (https://rlang.r-lib.org/reference/topic-error-chaining.html), but this should be applied globally to all existing tryCatch()s before attempting to resolve the specific package installation problem.

yutannihilation · 2022-05-18T04:37:27Z

Sure, I'll wait (and learn about the feature in rlang).

thomasp85 · 2022-05-18T08:45:29Z

The cli update has now been merged into main so if you merge this into here we can finish it off

…raise-error-when-check_installed

yutannihilation · 2022-05-18T14:50:02Z

No idea why this fails...

> devtools::test(filter = "stat-summary")
ℹ Loading ggplot2
ℹ Testing ggplot2
✔ | F W S  OK | Context
✖ | 2       5 | stat-summary [0.8s]                                                                
───────────────────────────────────────────────────────────────────────────────────────────────────
Failure (test-stat-summary.R:95:3): stat_summary() errors when check_installed() fails (#4736)
`ggplot_build(p + stat_summary(fun.data = fun1))` threw an unexpected error.
Message: Computation failed in `stat_summary()`
Caused by error:
! usual error
Class:   rlang_warning/warning/condition
Backtrace:
  1. testthat::expect_warning(...)
       at test-stat-summary.R:95:2
 46. base::.handleSimpleError(`<fn>`, "usual error", base::quote(`<fn>`(df$y)))
 47. rlang h(simpleError(msg, call))
 48. handlers[[1L]](cnd)
 49. cli::cli_warn(...)
       at ggplot2/R/stat-.r:108:10

Failure (test-stat-summary.R:95:3): stat_summary() errors when check_installed() fails (#4736)
`expect_error(...)` did not throw the expected warning.
───────────────────────────────────────────────────────────────────────────────────────────────────

══ Results ════════════════════════════════════════════════════════════════════════════════════════
Duration: 0.8 s

[ FAIL 2 | WARN 0 | SKIP 0 | PASS 5 ]

hadley · 2022-05-19T22:05:25Z

R/stat-.r

-        cli::cli_warn("Computation failed in {.fn {snake_class(self)}}", parent = cnd)
-        new_data_frame()
-      })
+      try_fetch(do.call(self$compute_panel, args),


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() } )

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.

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.

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.

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()`

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 !)?

hadley · 2022-05-19T22:05:25Z

R/stat-.r

-        cli::cli_warn("Computation failed in {.fn {snake_class(self)}}", parent = cnd)
-        new_data_frame()
-      })
+      try_fetch(do.call(self$compute_panel, args),


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() } )

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)}

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

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

There are two steps currently:

Signal a classed condition (that currently inherits from "error") with a restart.

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.

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.

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?

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:

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

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.

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))

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".

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.

yutannihilation · 2022-05-21T12:06:44Z

As the things discussed here are beyond my knowledge, I'm getting to feel I'm not the right person to address this... But, I'd like to include at least a temporary fix in the next release because it seems the current error message confuses users (#4671 (comment), #4819, and #4843. I think we've never seen this kind of questions this frequently before the last release). Let me think what I can do here.

hadley · 2022-05-21T13:53:19Z

We can easily ensure that the error message is correctly forwarded into the warning; Lionel and I just need to figure out the right way to handle the package not installed error. We should definitely get a fix into 3.4.

yutannihilation · 2022-05-21T14:33:23Z

Thanks. Lionel commented as follows on my question on rlang's repo, so I was wondering if "the right way to handle the package not installed error" would require a new feature on check_installed()'s side.

We could add a fallback argument to complement action.
r-lib/rlang#1402 (comment)

hadley · 2022-05-21T19:09:17Z

I don't think so; we need to get to the root cause of why tryCatch() doesn't work here, and resolve that.

yutannihilation · 2022-05-22T01:25:32Z

I see.

hadley · 2022-05-23T14:19:05Z

I think #4856 is probably all that's necessary from the ggplot2 side. With it and hexbin uninstalled, I see:

#> ggplot(mtcars, aes(cyl, disp)) + geom_hex()
Warning message:
Computation failed in `stat_binhex()`
Caused by error in `compute_group()` at ggplot2/R/ggproto.r:182:16:
! The package `hexbin` is required for `stat_binhex()`

I agree that an error would be better than a warning, but this at least shows the underlying problem, is consistent with how we handle other errors in stats, and doesn't require any other changes to ggplot2 once we improve the behaviour in rlang.

If I understand Lionel correctly, the improved rlang behaviour will yield:

#> ggplot(mtcars, aes(cyl, disp)) + geom_hex()

ℹ The package `hexbin` is required.
✖ Would you like to install it?

1: Yes
2: No

If you select "Yes", the hexbin will be installed and the plot will proceed as usual; if you select "No", you'll be dropped back to the interactive terminal just like if you'd pushed Ctrl + C during plotting.

thomasp85 · 2022-05-25T07:08:22Z

@hadley do we elect to close this and let the "real fix" happen in rlang?

Closes #1402 Part of tidyverse/ggplot2#4845

lionel- · 2022-05-25T09:48:51Z

With dev rlang we now have the behaviour described in @hadley's last message.

Thanks for looking into this issue @yutannihilation!

thomasp85 · 2022-05-25T09:56:05Z

Ok - closing for now

yutannihilation · 2022-05-25T12:10:26Z

Oh, cool. Glad to see the issue I had no idea how to fix was resolved while I did nothing :) Thanks so much!

@KryeKuzhinieri

# rlang 1.0.6 * `as_closure(seq.int)` now works (#1468). * rlang no longer stores errors and backtraces in a `org:r-lib` environment on the search path. * The low-level function `error_call()` is now exported (#1474). * Fixed an issue that caused a failure about a missing `is_character` function when rlang is installed alongside an old version of vctrs (#1482). * Fixed an issue that caused multiline calls in backtraces. * The C API function `r_lgl_which()` now propagates the names of the input (#1471). * The `pkg_version_info()` function now allows `==` for package version comparison (#1469, @KryeKuzhinieri). # rlang 1.0.5 * Fixed backtrace display with calls containing long lists of arguments (#1456). * New `r_obj_type_friendly()` function in the C library (#1463). It interfaces with `obj_type_friendly()` from `compat-obj-type.R` via a C callable. # rlang 1.0.4 * `is_installed()` no longer throws an error with irregular package names. * `is_installed()` and `check_installed()` now properly detect that the base package is installed on older versions of R (#1434). # rlang 1.0.3 * Child errors may now have empty messages to enable this pattern: ``` Error in `my_function()`: Caused by error in `their_function()`: ! Message. ``` * The `rlib_bytes` class now uses prettyunits to format bytes. The bytes are now represented with decimal prefixes instead of binary prefixes. * Supplying a frame environment to the `call` argument of `abort()` now causes the corresponding function call in the backtrace to be highlighted. In addition, if you store the argument name of a failing input in the `arg` error field, the argument is also highlighted in the backtrace. Instead of: ``` cli::cli_abort("{.arg {arg}} must be a foobar.", call = call) ``` You can now write this to benefit from arg highlighting: ``` cli::cli_abort("{.arg {arg}} must be a foobar.", arg = arg, call = call) ``` * `abort(message = )` can now be a function. In this case, it is stored in the `header` field and acts as a `cnd_header()` method invoked when the message is displayed. * New `obj_type_oo()` function in `compat-obj-type.R` (#1426). * `friendly_type_of()` from `compat-obj-type.R` (formerly `compat-friendly-type.R`) is now `obj_type_friendly()`. * `options(backtrace_on_error = "collapse")` and `print(trace, simplify = "collapse")` are deprecated. They fall back to `"none"` with a warning. * `call_match()` now better handles `...` when `dots_expand = FALSE`. * `list2(!!!x)` is now faster when `x` is a list. It is now returned as is instead of being duplicated into a new list. * `abort()` gains a `.trace_bottom` argument to disambiguate from other `.frame`. This allows `cli::cli_abort()` to wrap `abort()` in such a way that `.internal` mentions the correct package to report the error in (#1386). * The `transpose()` compat is now more consistent with purrr when inner names are not congruent (#1346). * New `reset_warning_verbosity()` and `reset_message_verbosity()` functions. These reset the verbosity of messages signalled with `warn()` and `inform()` with the `.frequency` argument. This is useful for testing verbosity in your package (#1414). * `check_dots_empty()` now allows trailing missing arguments (#1390). * Calls to local functions that are not accessible through `::` or `:::` are now marked with `(local)` in backtraces (#1399). * Error messages now mention indexed calls like `foo$bar()`. * New `env_coalesce()` function to copy bindings from one environment to another. Unlike approaches based on looping with `[[<-`, `env_coalesce()` preserves active and lazy bindings. * Chaining errors at top-level (directly in the console instead of in a function) no longer fails (#1405). * Warning style is propagated across parent errors in chained error messages (#1387). * `check_installed()` now works within catch-all `tryCatch(error = )` expressions (#1402, tidyverse/ggplot2#4845). * `arg_match()` and `arg_match0()` now mention the correct call in case of type error (#1388). * `abort()` and `inform()` now print messages to `stdout` in RStudio panes (#1393). * `is_installed()` now detects unsealed namespaces (#1378). This fixes inconsistent behaviour when run within user onLoad hooks. * Source references in backtraces and `last_error()`/`last_trace()` instructions are now clickable in IDEs that support links (#1396). * `compat-cli.R` now supports `style_hyperlink()`. * `abort(.homonyms = "error")` now throws the expected error (#1394). * `env_binding_are_active()` no longer accidentally triggers active bindings (#1376). * Fixed bug in `quo_squash()` with nested quosures containing the missing argument.

yutannihilation added 5 commits May 14, 2022 16:33

Propagate errors from check_installed().

996a59c

Use abort()

cb894a9

Add tests

56778bf

Skip examples if Hmisc is not installed

3115900

Skip examples if hexbin is not installed

4d59326

yutannihilation changed the title ~~Propagate errors from check_installed().~~ Propagate errors from check_installed() May 14, 2022

yutannihilation mentioned this pull request May 17, 2022

Can check_installed() be used inside tryCatch()? r-lib/rlang#1402

Closed

yutannihilation added 4 commits May 18, 2022 23:09

Merge remote-tracking branch 'upstream/main' into feature/gracefully-…

bc7e725

…raise-error-when-check_installed

Use cli_abort()

8d7d97b

Fix

b016039

Empty commit to try invoking the CI

2948069

hadley reviewed May 19, 2022

View reviewed changes

lionel- added a commit to r-lib/rlang that referenced this pull request May 25, 2022

Fix check_installed() within catch-all tryCatch(error = )

4cbe53f

Closes #1402 Part of tidyverse/ggplot2#4845

thomasp85 closed this May 25, 2022

yutannihilation deleted the feature/gracefully-raise-error-when-check_installed branch May 25, 2022 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate errors from check_installed() #4845

Propagate errors from check_installed() #4845

yutannihilation commented May 14, 2022 •

edited

Loading

hadley commented May 17, 2022

yutannihilation commented May 18, 2022

thomasp85 commented May 18, 2022

yutannihilation commented May 18, 2022

hadley May 19, 2022

hadley May 19, 2022 •

edited

Loading

hadley May 19, 2022

yutannihilation May 20, 2022

lionel- May 20, 2022

yutannihilation May 21, 2022

hadley May 19, 2022

yutannihilation May 19, 2022

hadley May 20, 2022

lionel- May 20, 2022

hadley May 20, 2022

hadley May 20, 2022

lionel- May 22, 2022

hadley May 23, 2022

lionel- May 23, 2022 •

edited

Loading

lionel- May 23, 2022

yutannihilation commented May 21, 2022

hadley commented May 21, 2022

yutannihilation commented May 21, 2022

hadley commented May 21, 2022

yutannihilation commented May 22, 2022

hadley commented May 23, 2022

thomasp85 commented May 25, 2022

lionel- commented May 25, 2022

thomasp85 commented May 25, 2022

yutannihilation commented May 25, 2022

Propagate errors from check_installed() #4845

Propagate errors from check_installed() #4845

Conversation

yutannihilation commented May 14, 2022 • edited Loading

hadley commented May 17, 2022

yutannihilation commented May 18, 2022

thomasp85 commented May 18, 2022

yutannihilation commented May 18, 2022

Choose a reason for hiding this comment

hadley May 19, 2022 • 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

lionel- May 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yutannihilation commented May 21, 2022

hadley commented May 21, 2022

yutannihilation commented May 21, 2022

hadley commented May 21, 2022

yutannihilation commented May 22, 2022

hadley commented May 23, 2022

thomasp85 commented May 25, 2022

lionel- commented May 25, 2022

thomasp85 commented May 25, 2022

yutannihilation commented May 25, 2022

yutannihilation commented May 14, 2022 •

edited

Loading

hadley May 19, 2022 •

edited

Loading

lionel- May 23, 2022 •

edited

Loading