`build_readme()` extremely slow due to `pak::local_install_deps()` installing full dependency tree in temp

[BjarkeHautop]

After the release of devtools 2.5.0, build_readme() has become very slow for packages with moderate dependency trees.

build_readme() installs the package in a temporary directory

devtools/R/install.R

Line 256 in ee2ae31

withr::local_temp_libpaths(.local_envir = env)

Because of this, pak::local_install_deps() installs the entire dependency tree in temp, even if all dependencies are already installed in the user's normal libraries. This is intended pak behavior, see
https://github.com/r-lib/pak/blob/63d646be4d2d59c29ea02cd99a68fb5b2c4164fd/R/package.R#L15-L17
(and is different from how remotes does it).

For packages with heavier dependency trees, this can add several minutes to build_readme(). E.g. for a package I maintain, on my machine it takes ~7 minutes to complete the pak::local_install_deps() call (~90 seconds if using Posit Package Manager binaries).

While this is happening, no useful progress information is printed, even with quiet = FALSE. The user sees no indication that a large dependency installation is occurring.

[jennybc]

Update: Deleting most of this reply because I was mixing this with a separate issue in my head.

confusing stuff here

But I certainly can believe there are differences due to the new approach to installing dependencies with pak instead of remotes. My guess/hope is that you will see things being snappier now that you've installed those packages via pak once (pak is pretty clever about caching).

[BjarkeHautop]

The dependencies are being installed here, not via the local_install() line you linked to:

And the whole issue is that it's done in a tempdir, which makes pak install everything again, which happens from the line I linked.

But I certainly can believe there are differences due to the new approach to installing dependencies with pak instead of remotes. My guess/hope is that you will see things being snappier now that you've installed those packages via pak once (pak is pretty clever about caching).

Sadly, no it won't. These are the timings after having run it several times (since the long times are from building some packages, this won't go away).

[BjarkeHautop]

A minimal change (which would mirror the old behavior of build_readme() which used remotes) is to just call install() with dependencies = FALSE from local_install(), so pak won't reinstall all dev dependencies in temp.

Optionally, you could ensure the user has the required packages installed first (e.g. by installing them for the user in .Library or fallback to installing the missing ones in temp).

[jennybc]

First, sorry, my first reply was a hybrid of responding to this issue while thinking about a different one (which is about install(), which certainly used by build_readme()). I'm going to delete it or redact it to head off any confusion for a future reader.

build_readme(): It has always called local_install() (an internal function). Here's what local_install() looked like in devtools 2.4.6:

local_install <- function(pkg = ".", quiet = TRUE, env = parent.frame()) {
  pkg <- as.package(pkg)

  cli::cli_inform(c(i = "Installing {.pkg {pkg$package}} in temporary library"))
  withr::local_temp_libpaths(.local_envir = env)
  install(pkg, upgrade = "never", reload = FALSE, quick = TRUE, quiet = quiet)
}

And now in devtools 2.5.0:

local_install <- function(pkg = ".", quiet = TRUE, env = parent.frame()) {
  pkg <- as.package(pkg)

  if (!quiet) {
    cli::cli_inform(c(
      i = "Installing {.pkg {pkg$package}} in temporary library"
    ))
  }
  withr::local_temp_libpaths(.local_envir = env)
  install(pkg, upgrade = FALSE, reload = FALSE, quick = TRUE, quiet = quiet)
}

So devtools has always put a temporary library in place for the duration of build_readme(). And then called devtools::install(). Major differences:

• devtools::install() now installs dependencies via pak::local_install_deps() instead of through remotes::install_deps()
• As a consequence, install(upgrade = "never") has changed to install(upgrade = FALSE).

Here are those calls juxtaposed. devtools 2.4.6:

remotes::install_deps(pkg$path,
      build = build, build_opts = build_opts,
      INSTALL_opts = opts, dependencies = dependencies, quiet = quiet,
      force = force, upgrade = upgrade, ...
    )

devtools 2.5.0

pak::local_install_deps(
          pkg$path,
          upgrade = upgrade,
          dependencies = dependencies || isTRUE(build_vignettes)
        )

So what you're describing doesn't strike me as an intended change. But perhaps there's some 2nd order change from the remotes -> pak transition. Is your source package available on GitHub? (just so I can see exactly what you're seeing)

[BjarkeHautop]

First, sorry, my first reply was a hybrid of responding to this issue while thinking about a different one (which is about install(), which certainly used by build_readme()). I'm going to delete it or redact it to head off any confusion for a future reader.

No worries!

So what you're describing doesn't strike me as an intended change. But perhaps there's some 2nd order change from the remotes -> pak transition.

Without looking too much into what remotes do and just going off observed behavior from devtools, remotes seemed to use installed packages for dependencies even when using a temp directory. pak doesn't do this, and explicitly states so, as I linked above.

Is your source package available on GitHub? (just so I can see exactly what you're seeing)

Yes, it's here https://github.com/disco-coders/causalDisco/.

Maybe useful, but doing my proposed fix above fixes the issue with build_readme() being slow, which I did in my fork:
main...BjarkeHautop:devtools:main

[jennybc]

OK, yeah, I have digested this:

#' @param lib Package library to install the packages to. Note that _all_
#'   dependent packages will be installed here, even if they are
#'   already installed in another library. The only exceptions are base and recommended packages ...

just call install() with dependencies = FALSE from local_install()

On its own, that would presumably lead to some folks getting a failure to build README (some dependency not pre-installed) or, in some sense, the wrong rendered README.md (outdated version of dependency). Installing dependencies into .Library is a nonstarter, given that lots of folks (including myself) only keep base and recommended packages there.

I see what you're saying but it's also possible this is a #wontfix (or becomes a request/discussion with pak). Because build_readme() isn't something one does often (like, say, load_all()). But I can see the pain if dependencies take a long time to install 🤔

[BjarkeHautop]

On its own, that would presumably lead to some folks getting a failure to build README (some dependency not pre-installed) or, in some sense, the wrong rendered README.md (outdated version of dependency). Installing dependencies into .Library is a nonstarter, given that lots of folks (including myself) only keep base and recommended packages there.

Another possible solution could be to grab the dependency tree needed and copy the appropriate packages to the tempdir before calling pak::local_install_deps(), maybe? Then pak can see them and will only install the missing packages / update old versions.

I see what you're saying but it's also possible this is a #wontfix (or becomes a request/discussion with pak). Because build_readme() isn't something one does often (like, say, load_all()). But I can see the pain if dependencies take a long time to install 🤔

Yeah, fair.

[hadley]

@gaborcsardi any ideas what we could do here?

[gaborcsardi]

You could install the package only, without the dependencies, i.e. dependencies = FALSE. That should work if the dependencies are already installed into the regular library, even for source packages.

> pak::pkg_install("callr?source", lib = tmp <- tempfile(), dependencies = FALSE)
ℹ Creating library directory: /var/folders/h9/6ct_py4s319fxt88dxn506l00000gp/T//Rtmpsv3Ph0/file715843d6e5e2
✔ Loading metadata database ... done

→ Will install 1 package.
→ The package (104.36 kB) is cached.
+ callr   3.7.6 👷🏼
ℹ No downloads are needed, 1 pkg (104.36 kB) is cached
ℹ Building callr 3.7.6
✔ Built callr 3.7.6 (1.5s)
✔ Installed callr 3.7.6  (25ms)
✔ 1 pkg: added 1 [6.3s]

> dir(tmp)
[1] "_cache" "callr"

Not really optimal, because it fails if the dependencies are not installed into the regular library, but maybe it is ok until pak can support dependencies in a non-target library.

[BjarkeHautop]

What about copying the needed packages from .libPaths()? So something like this:

local_install <- function(pkg = ".", quiet = TRUE, env = parent.frame()) {
  pkg <- as.package(pkg)

  if (!quiet) {
    cli::cli_inform(c(
      i = "Installing {.pkg {pkg$package}} in temporary library"
    ))
  }

  orig_libs <- .libPaths()

  tmp_lib <- tempdir()
  withr::local_libpaths(tmp_lib, .local_envir = env)

  deps <- pak::pkg_deps(pkg$path)
  needed_pkgs <- setdiff(deps$package[is.na(deps$priority)], pkg$package)

  for (p in needed_pkgs) {
    pkg_path <- NULL
    for (lib in orig_libs) {
      candidate <- file.path(lib, p)
      if (dir.exists(candidate)) {
        pkg_path <- candidate
        break
      }
    }
    if (!is.null(pkg_path)) {
      file.copy(pkg_path, tmp_lib, recursive = TRUE)
    }
  }

  install(pkg, upgrade = FALSE, reload = FALSE, quick = TRUE, quiet = quiet)
}

Skipping base and recommended packages and the package itself from the copying.

Obviously, the copying is a bit expensive, but maybe it's fine?

[gaborcsardi]

Copying packages seems fragile to me, so I would not do that, especially since this is a temporary workaround. It is also worse in the common case, I think, when all required dependencies are already installed into the regular library.

[jennybc]

I think I'll take a look at checking whether the dependencies are pre-installed and alerting if not. Then I'd feel much better about doing install(dependencies = FALSE).

[gaborcsardi]

I think having a fast function to check if the required versions of all dependencies are installed, potentially with some caching, so no HTTP is required in most cases, is a great idea.

OTOH for a quick solution of the current problem, I think dependencies = FALSE is sufficient, that's how it was working before if I understand correctly.

I'll update pak asap, to potentially consider packages in non-target libraries.

But I am actually not sure if that's the ideal solution. I think ideally build_readme() would not start installing packages. Maybe it could check if all required packages are installed, and fail if not? And/or warn if they are not of the right versions?

[jennybc]

Coincidentally, when refreshing dev_sitrep() for this release as part of the pak pivot + leaning into cli, we have some recently-updated code for providing a little "ahead/behind" report re: the current source package's dependencies. I'll probably find a way to reuse that and make sure we can remember the result during the current session or something like that.

[hadley]

I think it would be fine to just skip installing dependencies for now. If you're actively working on a package, you'll presumably discover that you're missing deps some other way.