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

Bundle and load renv at startup #157

Merged
merged 17 commits into from
Nov 9, 2022
Merged

Bundle and load renv at startup #157

merged 17 commits into from
Nov 9, 2022

Conversation

juliasilge
Copy link
Member

@juliasilge juliasilge commented Nov 4, 2022

Related to #154, #4, and #127

Once I get this right and merged in, I'll work on extending how we use renv (not only writing Dockerfiles, but also when writing/reading the model to storage).

R/zzz.R Outdated Show resolved Hide resolved
R/renv.R Outdated Show resolved Hide resolved
@juliasilge
Copy link
Member Author

In the inst/resources/renv.R file that is generated/embedded, I had to do a find-and-replace to change all the instances of tail() to utils::tail(). Maybe it makes more sense to use @importFrom utils tail instead, although I think that will require one of my PRECIOUS slots for imports going to utils.

@juliasilge
Copy link
Member Author

Clearly this is not working yet:

  Loading this package had a fatal error status code 1
  Loading log:
  Error: package or namespace load failed for ‘vetiver’:
   .onLoad failed in loadNamespace() for 'vetiver', details:
    call: NULL
    error: "" does not exist

I can build and check the package locally in the current state.

@juliasilge
Copy link
Member Author

I took out the stuff in .onLoad that was causing the problems for loading the package. 👍

However, now I am having the problems with actually running any code from the renv environment. Trying to call renv$snapshot results in this traceback:

Error in get(scope, envir = `_renv_filebacked`) : 
object 'settings' not found
14. get(scope, envir = `_renv_filebacked`)
13. renv_filebacked_envir(scope)
12. renv_filebacked_get(scope, path)
11. filebacked("settings", path, renv_settings_read_impl)
10. renv_settings_read(path)
9. renv_settings_get(project, name)
8. settings$r.version(project = project)
7. settings$r.version(project = project) %||% getRversion()
6. renv_lockfile_init_r_version(project)
5. renv_lockfile_init_r(project)
4. renv_lockfile_init(project)
3. renv_lockfile_create(project, libpaths, type, packages)
2. renv$snapshot(project = path, lockfile = lockfile, packages = pkgs, 
prompt = FALSE, force = TRUE) at write-docker.R#63
1. vetiver_write_docker(v, tmp_plumber, tempdir())

@kevinushey do you have any ideas on this?

  • How does packrat handle these kinds of settings? I looked but couldn't figure it out.
  • After taking a look, does this (vendoring the renv code) strike you as the best option, vs. new exported functionality in renv itself?

@kevinushey
Copy link
Collaborator

Ah, I think this is a small bug in renv in terms of being embeddable. Normally, some stuff needs to happen in its .onLoad() hook; however, that's not getting called here. I think I can fix this in renv; let me take a look.

@kevinushey
Copy link
Collaborator

I made a change in renv that seems to fix the issue for me locally:

rstudio/renv@f672288

Can you let me know if you have better luck with 0.16.0-23?

@kevinushey
Copy link
Collaborator

Maybe it makes more sense to use @importFrom utils tail instead, although I think that will require one of my PRECIOUS slots for imports going to utils.

renv uses quite a few things from utils:

https://github.com/rstudio/renv/blob/f672288ff7a261928d69d454aa483dd280fb788b/NAMESPACE#L44-L72

I should probably avoid doing this to help the embedding scenario, or perhaps provide a helper script that ensures renv can always "see" utils.

@kevinushey
Copy link
Collaborator

I've added some tooling to the development version of renv to make this easier. Now, you should be able to just run:

renv:::vendor()

inside the package directory, and renv will set up the necessary infrastructure for you. For example:

kevin@MBP-P2MQ:~/r/pkg/vetiver-r [main]
$ R -s -e "renv:::vendor()"
# Cloning renv 0.16.0-26 from https://github.com/rstudio/renv ...Done!
#
# A vendored copy of `renv` was created at: "~/r/pkg/vetiver-r/inst/vendor/renv.R"
# The `renv` auto-loader was generated at:  "~/r/pkg/vetiver-r/R/renv.R"
#
# Please add `renv$initialize()` to your package's `.onLoad()`
# to ensure that `renv` is initialized on package load.
#

@juliasilge
Copy link
Member Author

I think something is still up with loading the package in this way @kevinushey. 😕 I am getting these errors (the same as when I tried to put things like renv$renv_platform_init() in .onLoad):

  Loading this package had a fatal error status code 1
  Loading log:
  Error: package or namespace load failed for ‘vetiver’:
   .onLoad failed in loadNamespace() for 'vetiver', details:
    call: NULL
    error: "" does not exist

I only see these errors on CI, not locally. It reminds me of this kind of problem that we saw in glue and stringr, where one package is being loaded while another namespace is "sort of" loaded. Maybe related?

@kevinushey
Copy link
Collaborator

Doh; I can reproduce locally if I run the tests if renv is not installed, so there's likely something in renv code that expects to be able to find itself as an installed package. I should be able to fix this; sorry for the trouble!

@kevinushey
Copy link
Collaborator

Sorry for the false starts! I think this is finally ready to go; I've added some proper tests to renv as well and tests are passing there, so I'm fairly confident this should work in vetiver now. You can update to the latest with:

remotes::install_github("rstudio/renv")
renv:::vendor()

@kevinushey
Copy link
Collaborator

Progress! Although now it looks like the failures are related to the repository URLs. This might be related to some internal code that renv tries to use when running tests, which tries to make sure it always runs without any CRAN repositories visible. I think I need to turn this off for the embedded mode.

@juliasilge
Copy link
Member Author

Thank you so much for all your help @kevinushey! 🙌

  • Do you have any more thoughts or feedback before I merge this in?
  • I don't need to do anything special about licensing, do I? RStudio/Posit is the copyright holder on both renv and vetiver, and they are both MIT licensed.

@kevinushey
Copy link
Collaborator

Should be good to go! Indeed, there shouldn't be any licensing issues.

The only thing I'd add is that if you plan to use internal renv functions, please add a few unit tests in case I change something in a backwards-incompatible way (although I'll do my best not to!)

@juliasilge
Copy link
Member Author

if you plan to use internal renv functions, please add a few unit tests in case I change something in a backwards-incompatible way (although I'll do my best not to!)

Definitely will! The current use in Dockerfiles is pretty well tested via snapshots, etc, and I'll be sure to test additional use. Just to make this explicit here, that means I will find out if a change in renv breaks vetiver functionality, not you, which is backwards compared to the normal reverse dependency checks for R packages. 👍

@juliasilge juliasilge merged commit 43a7285 into main Nov 9, 2022
@juliasilge juliasilge deleted the embed-renv branch November 9, 2022 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants