From 6474d069be8af7f330d83a10a2eb71a4aaddcb31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kirill=20M=C3=BCller?= Date: Tue, 20 Oct 2020 09:00:38 +0200 Subject: [PATCH 1/3] Parallel styling --- DESCRIPTION | 2 ++ NAMESPACE | 1 + R/transform-files.R | 21 ++++++++++++++++++--- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 7df9745b5..f00a5b511 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -33,6 +33,8 @@ Suggests: data.tree (>= 0.1.6), digest, dplyr, + furrr, + future, here, knitr, prettycode, diff --git a/NAMESPACE b/NAMESPACE index 67f3fcd43..88f12cd8d 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -39,6 +39,7 @@ importFrom(purrr,when) importFrom(rlang,abort) importFrom(rlang,is_empty) importFrom(rlang,is_installed) +importFrom(rlang,local_options) importFrom(rlang,seq2) importFrom(rlang,warn) importFrom(rlang,with_handlers) diff --git a/R/transform-files.R b/R/transform-files.R index 14121bbbb..5e2f202fa 100644 --- a/R/transform-files.R +++ b/R/transform-files.R @@ -11,6 +11,7 @@ #' styling whether or not it was actually changed (or would be changed when #' `dry` is not "off"). #' @keywords internal +#' @importFrom rlang local_options transform_files <- function(files, transformers, include_roxygen_examples, base_indention, dry) { transformer <- make_transformer(transformers, include_roxygen_examples, base_indention) max_char <- min(max(nchar(files), 0), getOption("width")) @@ -19,9 +20,23 @@ transform_files <- function(files, transformers, include_roxygen_examples, base_ cat("Styling ", len_files, " files:\n") } - changed <- map_lgl(files, transform_file, - fun = transformer, max_char_path = max_char, dry = dry - ) + if (rlang::is_installed("furrr")) { + if (inherits(future::plan(), "uniprocess")) { + local_options(future.supportsMulticore.unstable = "quiet") + oplan <- future::plan("multiprocess") + on.exit(future::plan(oplan), add = TRUE) + } + + changed <- furrr::future_map_lgl(files, transform_file, + fun = transformer, max_char_path = max_char, dry = dry, + .progress = TRUE + ) + } else { + changed <- map_lgl(files, transform_file, + fun = transformer, max_char_path = max_char, dry = dry + ) + } + communicate_summary(changed, max_char) communicate_warning(changed, transformers) new_tibble(list(file = files, changed = changed), nrow = len_files) From 810f2c724a9808363774343e7d7a725115bf0b52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kirill=20M=C3=BCller?= Date: Wed, 21 Oct 2020 10:21:47 +0200 Subject: [PATCH 2/3] No parallelization in tests --- R/transform-files.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/transform-files.R b/R/transform-files.R index 5e2f202fa..3ad1d9269 100644 --- a/R/transform-files.R +++ b/R/transform-files.R @@ -20,7 +20,7 @@ transform_files <- function(files, transformers, include_roxygen_examples, base_ cat("Styling ", len_files, " files:\n") } - if (rlang::is_installed("furrr")) { + if (!identical(Sys.getenv("TESTTHAT"), "true") && rlang::is_installed("furrr")) { if (inherits(future::plan(), "uniprocess")) { local_options(future.supportsMulticore.unstable = "quiet") oplan <- future::plan("multiprocess") From b79ff02479141cca65d437cc549035d547819ae8 Mon Sep 17 00:00:00 2001 From: Lorenz Walthert Date: Mon, 30 Nov 2020 00:08:01 +0100 Subject: [PATCH 3/3] allow escaping via env variable, document and test --- .github/workflows/R-CMD-check.yaml | 1 + .github/workflows/benchmarking.yaml | 1 + .github/workflows/test-coverage.yaml | 1 + DESCRIPTION | 1 + NEWS.md | 4 +++ R/future.R | 20 +++++++++++ R/transform-files.R | 24 +++++++++---- inst/WORDLIST | 5 +++ man/styler_future.Rd | 25 +++++++++++++ tests/testthat/test-future.R | 53 ++++++++++++++++++++++++++++ 10 files changed, 129 insertions(+), 6 deletions(-) create mode 100644 R/future.R create mode 100644 man/styler_future.Rd create mode 100644 tests/testthat/test-future.R diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index 0f5ea8220..82c08c3e9 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.github/workflows/R-CMD-check.yaml @@ -30,6 +30,7 @@ jobs: env: R_REMOTES_NO_ERRORS_FROM_WARNINGS: true + R_STYLER_FUTURE_NO_OVERRIDE: true RSPM: ${{ matrix.config.rspm }} GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }} _R_CHECK_FORCE_SUGGESTS_: false diff --git a/.github/workflows/benchmarking.yaml b/.github/workflows/benchmarking.yaml index 909a3213d..9623f2d03 100644 --- a/.github/workflows/benchmarking.yaml +++ b/.github/workflows/benchmarking.yaml @@ -18,6 +18,7 @@ jobs: env: R_REMOTES_NO_ERRORS_FROM_WARNINGS: true + R_STYLER_FUTURE_NO_OVERRIDE: true RSPM: ${{ matrix.config.rspm }} GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }} steps: diff --git a/.github/workflows/test-coverage.yaml b/.github/workflows/test-coverage.yaml index 3058d037b..3397bffb6 100644 --- a/.github/workflows/test-coverage.yaml +++ b/.github/workflows/test-coverage.yaml @@ -13,6 +13,7 @@ jobs: runs-on: macOS-latest env: GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }} + R_STYLER_FUTURE_NO_OVERRIDE: true steps: - uses: actions/checkout@v2 diff --git a/DESCRIPTION b/DESCRIPTION index f00a5b511..6f4baee47 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -57,6 +57,7 @@ Collate: 'detect-alignment.R' 'environments.R' 'expr-is.R' + 'future.R' 'indent.R' 'initialize.R' 'io.R' diff --git a/NEWS.md b/NEWS.md index 181387cb3..dd2effb85 100644 --- a/NEWS.md +++ b/NEWS.md @@ -16,6 +16,10 @@ ## Major changes +- `style_file()`, `style_dir()` and `style_pkg()` now process input files in + parallel and display process bars if `{furrr}` is installed. This feature is + experimental, please see `help(styler_future, package = "styler")` for + details. - blank lines in function calls and headers are now removed, for the former only when there are no comments before or after the blank line (#629, #630, #635). - speed improvements: (~10%) when cache is activated because transformers are not diff --git a/R/future.R b/R/future.R new file mode 100644 index 000000000..218dd269a --- /dev/null +++ b/R/future.R @@ -0,0 +1,20 @@ +#' Parallelization of styling +#' +#' [style_file()], [style_pkg()] and [style_dir()] leverage the `{future}` +#' framework for parallelization. To make use of parallel processing, you need +#' to have the package `{furrr}` installed. In that case, the strategy +#' [future::multiprocess()] is set temporarily during styling (only if there is +#' more than two files to style, because of start-up costs of parallelization). +#' You can prevent this temporary change in the future strategy by setting the +#' environment variable `R_STYLER_FUTURE_NO_OVERRIDE` to `TRUE` (case ignored). In +#' that case, the future strategy set before calling styler will be used, if +#' you have not set any, the future `{framework}` falls back to +#' [future::sequential()]. +#' +#' @section Life-cycle: +#' The parallelization feature is experimental, in particular how its governed +#' with `R_STYLER_FUTURE_NO_OVERRIDE` and how progress bars are displayed with +#' `{furrr}`. In the future, most likely the `{progressr}` package will be used +#' to handle progress bars. +#' @name styler_future +NULL diff --git a/R/transform-files.R b/R/transform-files.R index 3ad1d9269..55b6b34a3 100644 --- a/R/transform-files.R +++ b/R/transform-files.R @@ -17,21 +17,33 @@ transform_files <- function(files, transformers, include_roxygen_examples, base_ max_char <- min(max(nchar(files), 0), getOption("width")) len_files <- length(files) if (len_files > 0L && !getOption("styler.quiet", FALSE)) { - cat("Styling ", len_files, " files:\n") + cat("Styling ", len_files, " files:\n") # TODO should only have one space between words } - if (!identical(Sys.getenv("TESTTHAT"), "true") && rlang::is_installed("furrr")) { - if (inherits(future::plan(), "uniprocess")) { + future_strategy_no_override <- tolower(Sys.getenv("R_STYLER_FUTURE_NO_OVERRIDE", "FALSE")) == "true" + if (rlang::is_installed("furrr")) { + # only parallelise when furrr is installed + if (!future_strategy_no_override && length(files) > 2) { + # only override when env not set and more than two files. local_options(future.supportsMulticore.unstable = "quiet") - oplan <- future::plan("multiprocess") - on.exit(future::plan(oplan), add = TRUE) - } + oplan <- future::plan("multisession") # not sure we should use + # withr::defer() gives C stack overflow on macOS, on.exit not. + on.exit(future::plan(oplan), add = TRUE, after = FALSE) + } changed <- furrr::future_map_lgl(files, transform_file, fun = transformer, max_char_path = max_char, dry = dry, .progress = TRUE ) } else { + if (future_strategy_no_override) { + rlang::warn(paste( + "Environment variable `R_STYLER_FUTURE_NO_OVERRIDE` is not respected", + "because package {furrr} is not installed. Falling back to ", + "`purrr::map_lgl()` to iterate over files to style instead of", + "`furrr::future_map_lgl()`." + )) + } changed <- map_lgl(files, transform_file, fun = transformer, max_char_path = max_char, dry = dry ) diff --git a/inst/WORDLIST b/inst/WORDLIST index e51e192ba..0955d25c8 100644 --- a/inst/WORDLIST +++ b/inst/WORDLIST @@ -54,6 +54,7 @@ fileext filetype forcond funct +furrr gcc getChecksum getOption @@ -91,6 +92,7 @@ macOS magrittr md Müller +multiprocess mutli navbar netlify @@ -101,6 +103,8 @@ ourself packagemanager packrat pandoc +parallelization +Parallelization params parsable parsesum @@ -113,6 +117,7 @@ pre precommit prefill prettycode +progressr PRs purrr rcmdcheck diff --git a/man/styler_future.Rd b/man/styler_future.Rd new file mode 100644 index 000000000..f9bbaaf2c --- /dev/null +++ b/man/styler_future.Rd @@ -0,0 +1,25 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/future.R +\name{styler_future} +\alias{styler_future} +\title{Parallelization of styling} +\description{ +\code{\link[=style_file]{style_file()}}, \code{\link[=style_pkg]{style_pkg()}} and \code{\link[=style_dir]{style_dir()}} leverage the \code{{future}} +framework for parallelization. To make use of parallel processing, you need +to have the package \code{{furrr}} installed. In that case, the strategy +\code{\link[future:multiprocess]{future::multiprocess()}} is set temporarily during styling (only if there is +more than two files to style, because of start-up costs of parallelization). +You can prevent this temporary change in the future strategy by setting the +environment variable \code{R_STYLER_FUTURE_NO_OVERRIDE} to \code{TRUE} (case ignored). In +that case, the future strategy set before calling styler will be used, if +you have not set any, the future \code{{framework}} falls back to +\code{\link[future:sequential]{future::sequential()}}. +} +\section{Life-cycle}{ + +The parallelization feature is experimental, in particular how its governed +with \code{R_STYLER_FUTURE_NO_OVERRIDE} and how progress bars are displayed with +\code{{furrr}}. In the future, most likely the \code{{progressr}} package will be used +to handle progress bars. +} + diff --git a/tests/testthat/test-future.R b/tests/testthat/test-future.R new file mode 100644 index 000000000..db70307b6 --- /dev/null +++ b/tests/testthat/test-future.R @@ -0,0 +1,53 @@ +path_rel <- c( + "dirty-sample-with-scope-tokens.R", + "dirty-sample-with-scope-spaces.R", + "clean-sample-with-scope-tokens.R" +) + +path_dir <- file.path( + "public-api", "xyzdir-dirty", path_rel +) + +test_that("can style in parallel when no strategy is set and overriding is deactivated", { + withr::local_envvar("R_STYLER_FUTURE_NO_OVERRIDE" = "TRUE") + expect_match( + catch_style_file_output(path_dir), + 'Styling +3', + all = FALSE + ) +}) + +test_that("can style in parallel when no strategy is set and overriding is not deactivated", { + withr::local_envvar("R_STYLER_FUTURE_NO_OVERRIDE" = "FALSE") + expect_match( + catch_style_file_output(path_dir), + 'Styling +3', + all = FALSE + ) +}) + +test_that("can style in parallel when no strategy is set", { + + expect_match( + catch_style_file_output(path_dir), + 'Styling +3', + all = FALSE + ) +}) + +test_that("can style in parallel when strategy is set", { + future::plan('sequential') + expect_match( + catch_style_file_output(path_dir), + 'Styling +3', + all = FALSE + ) +}) + +test_that("with fewer than two files, the plan is not modified", { + expect_match( + catch_style_file_output(path_dir[1]), + 'Styling +1', + all = FALSE + ) +})