thomasp85 / fiery

A flexible and lightweight web server
https://fiery.data-imaginist.com
Other
240 stars 12 forks source link

future::multiprocess is deprecated since long and soon to become defunct #50

Closed HenrikBengtsson closed 1 year ago

HenrikBengtsson commented 1 year ago

Hi. The 'multiprocess' backend has been deprecated since future 1.20.0 (Oct 2020). The fiery package uses it in the code and mentions it in the docs, e.g.

https://github.com/thomasp85/fiery/blob/da9c27fc301c0b922a598785108da02ac58fe657/R/FutureStack.R#L96-L104

Using it produces a deprecation warning:

Warning message:
Detected creation of a 'multiprocess' future. Strategy 'multiprocess' is deprecated in future (>= 1.20.0)
[2020-10-30]. Instead, explicitly specify either 'multisession' (recommended) or 'multicore'. Starting 
with future 1.31.0 [2023-01-31], 'multiprocess' is the same as 'sequential'.

But this soon become a defunct error. When that happens, R CMD check on the fiery package will break:

* checking tests ... ERROR
  Running ‘testthat.R’
Running the tests in ‘tests/testthat.R’ failed.
Complete output:
  > library(testthat)
  > library(fiery)
  > 
  > test_check("fiery")

  [ FAIL 1 | WARN 0 | SKIP 1 | PASS 253 ]

  ══ Skipped tests ═══════════════════════════════════════════════════════════════
  • On CRAN (1)

  ══ Failed tests ════════════════════════════════════════════════════════════════
  ── Error ('test-Fire.R:368'): futures can be added and called ──────────────────
  <defunctError/error/condition>
  Error: Detected creation of a 'multiprocess' future. Strategy 'multiprocess' is defunct in future (>= 1.20.0) [2020-10-30
]. Instead, explicitly specify either 'multisession' (recommended) or 'multicore'.
  Backtrace:
      ▆
   1. └─app$async(...) at test-Fire.R:368:4
   2.   └─private$ASYNC$add(substitute(expr), then, substituted = TRUE)
   3.     └─private$make_future(expr, then, ...)
   4.       ├─base::do.call(private$catcher, list(expr = expr, lazy = private$lazy))
   5.       └─future::multiprocess(...)
   6.         └─future (local) dfcn(msg = msg, package = .packageName)
   7.           └─base::.Defunct(...)

  [ FAIL 1 | WARN 0 | SKIP 1 | PASS 253 ]
  Error: Test failures
  Execution halted

Can you please fix?

PS. This one went under my revdepcheck radar, because it called multiprocess() directly. Please avoid that and use the recommended plan(...) approach instead. (It might be that we one day will replace all those backend functions with backend configuration objects with the same names)

HenrikBengtsson commented 1 year ago

Regarding plan(), you might want to do something like (non-verified):

fiery_bg_process <- function(..., envir = parent.frame()) {
  oplan <- plan(multisession)
  on.exit(plan(oplan), add = TRUE)
  future(..., envir = envir)
}

and use it as:

 AsyncStack <- R6Class('AsyncStack', 
   inherit = FutureStack, 
   private = list( 
     catcher = 'fiery_bg_process' 
   )
 ) 

Source: https://future.futureverse.org/reference/plan.html

HenrikBengtsson commented 1 year ago

Just a friendly reminder about this one

HenrikBengtsson commented 1 year ago

FYI, future 1.32.0 is now on CRAN making multiprocess is defunct. Your package will begin to produce check errors on CRAN any time soon, and you might get a request to update your package from the CRAN Team.

thomasp85 commented 1 year ago

Hi @HenrikBengtsson , sorry for the late reply and thanks for your persistence on this. I'll see if I can work out a solution for fiery

HenrikBengtsson commented 1 year ago

I see fiery 1.2.0 fixing this is now on CRAN. Thx