tidyverse / purrr

A functional programming toolkit for R
https://purrr.tidyverse.org/
Other
1.28k stars 272 forks source link

Bug: progress bars can only access global environment #1078

Open DanChaltiel opened 1 year ago

DanChaltiel commented 1 year ago

Hi,

Progress bars documentation says:

Format strings may contain R expressions to evaluate in braces.

However, while one would expect these expressions to evaluate in the current environment, it seems that they can only access variables declared in the global environment.

Here is a reprex. As you can see, the progress bar doesn't know about y (parent environment) but knows about x (global environment). It doesn't know about z (current environment) either, you can test it.

library(purrr)
f = function(){
  z="z"
  pb = list(format="Computing {x} {y} {zz} {pb_bar} {pb_percent}")

  walk(1:5, ~{
    Sys.sleep(1)
  }, .progress=pb)
}

g=function(){
  y="y"
  f()
}

x="x"
g()
#> Error in `map()`:
#> i In index: 2.
#> Caused by error:
#> ! Could not evaluate cli `{}` expression: `y`.
#> Caused by error:
#> ! object 'y' not found
#> Backtrace:
#>      x
#>   1. +-global g()
#>   2. | \-global f()
#>   3. |   \-purrr::walk(...)
#>   4. |     \-purrr::map(.x, .f, ..., .progress = .progress)
#>   5. |       \-purrr:::map_("list", .x, .f, ..., .progress = .progress)
#>   6. |         +-purrr:::with_indexed_errors(...)
#>   7. |         | \-base::withCallingHandlers(...)
#>   8. |         \-purrr:::call_with_cleanup(...)
#>   9. +-cli:::progress_c_update(`<env>`)
#>  10. | \-h$add(pb, .envir = caller)
#>  11. |   \-cli::cli_status(...)
#>  12. |     +-cli:::cli__message(...)
#>  13. |     | \-"id" %in% names(args)
#>  14. |     \-cli:::glue_cmd(msg, .envir = .envir)
#>  15. |       \-cli:::glue(...)
#>  16. +-cli (local) `<fn>`("y")
#>  17. | +-.transformer(expr, .envir) %||% character()
#>  18. | \-cli (local) .transformer(expr, .envir)
#>  19. |   +-eval(expr, envir = envir) %??% ...
#>  20. |   | \-chain_error(expr, err, srcref = utils::getSrcref(sys.call()))
#>  21. |   |   \-base::withCallingHandlers(...)
#>  22. |   \-base::eval(expr, envir = envir)
#>  23. |     \-base::eval(expr, envir = envir)
#>  24. +-base::.handleSimpleError(...)
#>  25. | \-h(simpleError(msg, call))
#>  26. |   \-throw_error(err, parent = e)
#>  27. |     \-base::signalCondition(cond)
#>  28. \-purrr (local) `<fn>`(`<rlb__3_0>`)
#>  29.   \-cli::cli_abort(...)
#>  30.     \-rlang::abort(...)

Created on 2023-05-02 with reprex v2.0.2

DanChaltiel commented 1 year ago

Actually, if it could also access .x or even its name, it would be tremendously useful 👍

bairdj commented 1 year ago

I also think being able to access .x would be a really helpful feature. For long running operations, it would be very useful to know which element is currently being processed. Sometimes I find that one element may take much longer than others, but it can be hard to determine which one it is (there are other ways to identify this of course, but this would be a quick and simple way)

gaborcsardi commented 1 year ago

Actually, it is already possible, but maybe purrr could have a better default, and better docs would be nice:

f <- function() {
  b <- "bar"
  purrr::map(
    1:10, 
    function(x) Sys.sleep(1), 
    .progress = list(caller = environment(), format = "{b} {cli::pb_current}")
  )
}
invisible(f())

I.e. caller is the environment where the format string(s) are evaluated.

hadley commented 4 months ago

I think this should be reasonably straightforward to implement — in map_, map2_, and pmap_, check isTRUE(.progress). If it is replace with the call suggested by @gaborcsardi using the .purrr_user_env as the environment. Then just needs a test and a news bullet. (Not sure any documentation changes are needed since I think this is the behaviour that folks would expect).