Skip to content
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.

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

drake and gt: gt modifies global environment #297

Closed
gadenbuie opened this issue May 31, 2019 · 12 comments

Comments

@gadenbuie
Copy link

commented May 31, 2019

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:

---
title: "Example GT Report"
output: html_document
---

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

In a fresh session, run

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

This comment has been minimized.

Copy link

commented May 31, 2019

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

This comment has been minimized.

Copy link
Author

commented May 31, 2019

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

@wlandau

This comment has been minimized.

Copy link

commented May 31, 2019

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

This comment has been minimized.

Copy link
Author

commented May 31, 2019

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

This comment has been minimized.

Copy link

commented May 31, 2019

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

This comment has been minimized.

Copy link

commented May 31, 2019

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

@wlandau

This comment has been minimized.

Copy link

commented May 31, 2019

Yup, didn't work for me either.

@wlandau

This comment has been minimized.

Copy link

commented May 31, 2019

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? tidyverse/dtplyr#60 and tidyverse/dtplyr#68 could be relevant. I am not sure what the reason is for removing register_s3_method() from dtplyr.

wlandau-lilly added a commit to ropensci/drake that referenced this issue May 31, 2019
@wlandau

This comment has been minimized.

Copy link

commented May 31, 2019

Update: I just added #297 (comment) to drake's docs in ropensci/drake@f10ee11. 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

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@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

This comment has been minimized.

Copy link

commented Jun 7, 2019

I believe so.

@wlandau

This comment has been minimized.

Copy link

commented Oct 4, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.