Gt: drake and gt: gt modifies global environment

[gadenbuie]

Briefly: Using gt within an Rmd built by drake with rmarkdown::render() results in the error

target report
fail report
Error: Target `report` failed. Call `diagnose(report)` for details. Error message:
  cannot remove bindings from a locked environment. 
Please read the "Self-invalidation" section of the make() help file.

I tracked the error to gt registering the S3 methods for "knitr", "knit_print", "gt_tbl" on package load in zzz.R.

I'm not sure whether this should be addressed in gt or drake (cc @wlandau), but I opened here in case there's an easy fix in gt.

Reprex

Here's a minimal drake plan that would be saved in drake.R:

library(drake)
library(gt)

plan <- drake_plan(
  data = mtcars,
  report = rmarkdown::render(
    knitr_in("report.Rmd"),
    output_file = file_out("report.html")
  )
)

make(plan)

And in report.Rmd:

````r

title: "Example GT Report"

output: html_document

```{r}
library(drake)
library(gt)
loadd(data)
gt(data)

````

In a fresh session, run

```r
source("make.R")
## target report
## ...
## fail report
## Error: Target `report` failed. Call `diagnose(report)` for details. Error message:
##   cannot remove bindings from a locked environment. 
## Please read the "Self-invalidation" section of the make() help file.

User-level Fixes

A user can add lock_envir = FALSE to drake::make(), but this is not recommended.

A user can also use a custom environment for the drake build, for example by modifying the make() step of drake.R

drake_env <- new.env()
make(plan, envir = drake_env)

but for larger drake projects this is less desirable because the user need to source all scripts tracked by drake into drake_env.

I also noticed that the problem _does not_ occur when using knitr::knit(), so there may be an interaction here with rmarkdown::render(). The downside of this approach is that rmarkdown::render() is required for Rmd v2 documents.

Finally, I confirmed that commenting out the register_s3_method() line does also "resolve" the problem, but I'm not sure what downstream consequences that might cause.

[wlandau]

What about a special environment just for the render() call in your plan? E.g. drake_plan(..., report = rmarkdown::render(..., envir = new.env(parent = globalenv()))).

[gadenbuie]

I was hopeful that would work but rmarkdown::render(..., envir = new.env(parent = globalenv()) gives the same error :pensive:

[wlandau]

Yeah, I should have realized that would happen...

What about rendering the report in a new callr::r() process? drake should not know about the global environment of a process you create within a command.

[gadenbuie]

Yes, that does work!

plan <- drake_plan(
  data = mtcars,
  report = callr::r(
    function(...) rmarkdown::render(...),
    args = list(
      input = drake::knitr_in("report.Rmd"),
      output_file = drake::file_out("report.html")
    )
  )
)

make(plan)

[wlandau]

After more consideration, I actually recommend a different approach: load gt before calling make(). That way, those S3 methods are registered before drake locks the environment.

library(drake)
library(gt)
plan <- drake_plan(
  data = mtcars,
  report = rmarkdown::render(
    knitr_in("report.Rmd"),
    output_file = drake::file_out("report.html")
  )
)
make(plan)

If this works for you, I then will add a note in drake's documentation and vote to close this issue.

[wlandau]

Oops, that's what you started with, isn't it? Probably won't work then...

[wlandau]

Yup, didn't work for me either.

[wlandau]

Now I am curious about register_s3_method() and base::registerS3method(). Do we only need registerS3method() when the generic comes from a non-base package? https://github.com/hadley/dtplyr/issues/60 and https://github.com/hadley/dtplyr/issues/68 could be relevant. I am not sure what the reason is for removing register_s3_method() from dtplyr.

[wlandau]

Update: I just added https://github.com/rstudio/gt/issues/297#issuecomment-497778735 to drake's docs in https://github.com/ropensci/drake/commit/f10ee11a6ab6a8675131a4e939457cc7b84e79a0. I think it is a safe workaround because it does not change the global environment of the master process. That is all I am comfortable doing on drake's side. (I considered supporting lock_envir = FALSE for individual targets, but oddly enough, that would incur a bit of a performance penalty in some cases.)

[rich-iannone]

@wlandau @gadenbuie Given the workaround (and also that I can't think of a better solution than the S3 method registrations), could this issue be closed?

[wlandau]

I believe so.

[wlandau]

Short answer for onlookers: make(lock_envir = FALSE) should remove all drake-related errors regarding locked bindings and locked environments.