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

Force a render when user pkgs don't match the pkgs used in a rendered, shiny-prerendered document #1420

Merged
merged 16 commits into from Oct 8, 2018

Conversation

@schloerke
Copy link
Contributor

schloerke commented Aug 3, 2018

Address: rstudio/learnr#169

By keeping track of the package version, we can force a new render if the user does not have the same package versions as the pre-rendered document.

This solves the error where a pre-rendered resource can not be mounted (because it doesn't exist!).

  • NEWS item
  • test?
  • when in Shiny Server and RSC env set in learnr
schloerke added 3 commits Aug 3, 2018
behavior:
* return false  if there is a prerender_option = 0 or yell if not 0 or 1
* if the original file is newer than the compiled file, return true
* if any external resource is newer, return true
* NEW: if any htmldep's package version is different than the installed, return true
* return false
@jjallaire

This comment has been minimized.

Copy link
Member

jjallaire commented Aug 4, 2018

This LGTM and will be a huge improvement on what we have currently (which is obviously much too fragile).

@rich-iannone

This comment has been minimized.

Copy link
Member

rich-iannone commented Aug 7, 2018

Based on a code-pairing between me and Barret, there are some changes that incorporate R package dependencies. One thing that Barret noted was that the package dependencies (in the current form below) will incorporate packages like graphics and methods where their versions mirror the R version. It could be argued that these packages don't change much between different R versions so we shouldn't invalidate based on these specific packages.

Here are the changes per file:

util.R

add:

get_loaded_packages <- function() {

  packages <- setdiff(loadedNamespaces(), "base")
  loadedversion <- vapply(packages, getNamespaceVersion, "")
  attached <- paste0("package:", packages) %in% search()

  data.frame(packages = packages[attached], loadedversion = loadedversion[attached],
             row.names = NULL, stringsAsFactors = FALSE)
}

render.R

change (ln 362):

output_options$dependency_resolver <- function(deps) {
  shiny_prerendered_dependencies <<- list(deps = deps, packages = get_loaded_packages())
  list()
}

shiny_prerendered.R

change (ln 184):

dependencies <- lapply(shiny_prerendered_dependencies$deps, function(dependency) {

add (after ln 228):

dependencies_pkgs_json <- jsonlite::serializeJSON(shiny_prerendered_dependencies$packages, pretty = FALSE)
shiny_prerendered_append_context(con, "package_dependencies", dependencies_pkgs_json)
@yihui

This comment has been minimized.

Copy link
Member

yihui commented Aug 7, 2018

I think it is fine to ignore base R packages. The original problem was caused by packages that ship HTML dependencies. Base R packages don't have such things.

@rich-iannone

This comment has been minimized.

Copy link
Member

rich-iannone commented Aug 7, 2018

Thanks, Yihui. To ignore the base R packages, the function get_loaded_packages() in util.R would change to:

get_loaded_packages <- function() {

  base_r_packages <-
    c("base", "compiler", "datasets", "graphics", "grDevices",
      "grid", "methods", "parallel", "splines",  
      "stats", "stats4", "tcltk", "tools", "utils")
  
  packages <- sort(setdiff(loadedNamespaces(), base_r_packages))
  loadedversion <- vapply(packages, getNamespaceVersion, "")
  attached <- paste0("package:", packages) %in% search()

  data.frame(packages = packages[attached], loadedversion = loadedversion[attached],
             row.names = NULL, stringsAsFactors = FALSE)
}
@schloerke

This comment has been minimized.

Copy link
Contributor Author

schloerke commented Aug 8, 2018

Documenting decision towards using loaded packages (after execution) vs detecting library and :: calls in the code (without execution)

There were two main approaches:

  1. It was already solved in packrat and to use something similar to packrat::findDependencies. This function boils down to regular expressions and the name of the function calls. This can easily be tricked, but can not be poisoned by the user workspace.
  2. We could detect the loaded packages after executing all the R code in the document. Looking at attached packages would only give results for library or require calls. Looking at the search path would give all packages that have been loaded or used. This can not be fooled like packrat, but can be poisoned by external user packages that were not a part of the knitting of the document.

We decided to go with the loaded packages, as it can not be easily fooled and most renderings of a document are done within RStudio's clean environment and on hosted sites, which will compile in a clean environment. The only caveat are with console users, where rendering is known to not be in a clean environment by default.

The good part about being aggressive with the execution package version matching is that users will get a cohesive experience. The worst interaction of not matching the execution packages is having to re-render the shiny prerendered document. This will be the first interaction of most any pre-render documents, but after re-rendering, it wont happen until the user (or service) updates their local packages.

@yihui

This comment has been minimized.

Copy link
Member

yihui commented Aug 8, 2018

That sounds reasonable to me.

schloerke added 2 commits Aug 8, 2018
…in prerender calculation

Thanks @rich-iannone for the help!
R/util.R Outdated
"base", "compiler", "datasets", "graphics", "grDevices",
"grid", "methods", "parallel", "splines",
"stats", "stats4", "tcltk", "tools", "utils"
)

This comment has been minimized.

Copy link
@yihui

yihui Aug 8, 2018

Member

You can use rownames(installed.packages(priority = 'base')) instead of hardcoding the package names.

This comment has been minimized.

Copy link
@rich-iannone

rich-iannone Aug 8, 2018

Member

Aha! I thought there was a better way (but I couldn't find it). Thanks!

# check that the major R versions match
if (!identical(R.version$major, execution_info$rMajorVersion)) {
return(TRUE)
}

This comment has been minimized.

Copy link
@wch

wch Aug 13, 2018

Contributor

Remove -- we can use exact version matching for base packages.

R/util.R Outdated

packages <- sort(setdiff(loadedNamespaces(), base_r_packages))
version <- vapply(packages, get_package_version_string, character(1))
attached <- paste0("package:", packages) %in% search()

This comment has been minimized.

Copy link
@wch

wch Aug 13, 2018

Contributor

We need to look at all loaded packages, not just the attached ones. For example, if someone does library(shiny), then this will only record the version of shiny, but not dependencies like htmltools.

Another case is if they do htmltools::div(), it will not attach htmltools, and this code will also not record the version.

R/util.R Outdated
# find all loaded packages.
# May contain extra packages, but contains all packages used while knitting
get_loaded_packages <- function() {
base_r_packages <- rownames(installed.packages(priority = 'base'))

This comment has been minimized.

Copy link
@wch

wch Aug 13, 2018

Contributor

This can be removed -- we might as well look at the base R packages as well, since it's (marginally) safer to use them, and the code will be slightly simpler without excluding them. It also lets us not record the R major version because that comes along with these packages.

f = function(package, version) {
!identical(get_package_version_string(package), version)
}
))

This comment has been minimized.

Copy link
@wch

wch Aug 13, 2018

Contributor

Change to for loop with a short-circuit return.

Copy link
Contributor

wch left a comment

See comments.

@wch
wch approved these changes Aug 16, 2018
@@ -298,7 +293,8 @@ shiny_prerendered_append_dependencies <- function(input, # always UTF-8

# write r major version and execution package dependencies
execution_json <- jsonlite::serializeJSON(
shiny_prerendered_dependencies[c("rMajorVersion", "packages")],
# visibly display what is being stored
shiny_prerendered_dependencies[c("packages")],

This comment has been minimized.

Copy link
@wch

wch Aug 16, 2018

Contributor

Don't need c() here

@jjallaire

This comment has been minimized.

Copy link
Member

jjallaire commented Aug 24, 2018

We just need to make sure that Shiny Server and RSC run these in a way that ensures that no render occurs (on the assumption that they are in read-only directories). They can I believe do this via an environment variable (which they may or may not already be setting, worth checking on).

@schloerke

This comment has been minimized.

Copy link
Contributor Author

schloerke commented Sep 6, 2018

@jjallaire The original envvar check of RMARKDOWN_RUN_PRERENDER is still honored inside rmarkdown.

rstudio/learnr@942df45 now checks if the learnr::run_tutorial is being executed on a SS or RSC.

If learnr::run_tutorial is on a server, it will behave like it currently does (never attempt to rerender). If learnr::run_tutorial is now on a personal machine, it'll attempt to attempt to rerender aggressively.

@schloerke schloerke changed the title [WIP] Force a render when user pkgs don't match the pkgs used in a rendered, shiny-prerendered document Force a render when user pkgs don't match the pkgs used in a rendered, shiny-prerendered document Sep 6, 2018
@schloerke

This comment has been minimized.

Copy link
Contributor Author

schloerke commented Sep 6, 2018

@yihui removed [WIP], ready for merge when you're ready. Thank you!

@yihui
yihui approved these changes Oct 8, 2018
Copy link
Member

yihui left a comment

This is excellent. Thanks!

@yihui yihui added this to the v1.11 milestone Oct 8, 2018
@yihui yihui merged commit 427724f into rstudio:master Oct 8, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
yihui added a commit that referenced this pull request Oct 8, 2018
@schloerke schloerke deleted the schloerke:htmldep_version branch Oct 8, 2018
yihui added a commit to RLesur/rmarkdown that referenced this pull request Apr 1, 2019
yihui added a commit to RLesur/rmarkdown that referenced this pull request Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.