From 37d4e12f87875532e43980349cc73185fd5ee542 Mon Sep 17 00:00:00 2001 From: wlandau-lilly Date: Fri, 1 Mar 2019 14:01:48 -0500 Subject: [PATCH 1/3] Add a menu for interactive make()s re #761 --- NAMESPACE | 1 + R/api-make.R | 15 ++++++++--- R/api-package.R | 2 +- R/exec-backend.R | 1 - R/exec-session.R | 23 +++++++++++++++++ R/preprocess-config.R | 7 +++--- R/vis-color.R | 1 + R/vis-console.R | 18 ++++++++++++++ man/drake_config.Rd | 2 +- man/make.Rd | 5 ++-- tests/testthat/test-always-skipped.R | 37 ++++++++++++++++++++++++++++ 11 files changed, 100 insertions(+), 12 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index 55b0ea8e4..5ad042418 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -205,6 +205,7 @@ importFrom(storr,storr_environment) importFrom(storr,storr_rds) importFrom(utils,compareVersion) importFrom(utils,head) +importFrom(utils,menu) importFrom(utils,packageVersion) importFrom(utils,read.csv) importFrom(utils,sessionInfo) diff --git a/R/api-make.R b/R/api-make.R index 71a151ff9..fd1d15054 100644 --- a/R/api-make.R +++ b/R/api-make.R @@ -9,8 +9,7 @@ #' [drake_plan()], #' [drake_config()], #' [vis_drake_graph()], -#' [outdated()], -#' [triggers()] +#' [outdated()] #' @export #' @return nothing #' @inheritParams drake_config @@ -69,7 +68,7 @@ make <- function( cpu = Inf, elapsed = Inf, retries = 0, - force = FALSE, + force = NULL, graph = NULL, trigger = drake::trigger(), skip_imports = FALSE, @@ -151,6 +150,16 @@ make <- function( if (!config$skip_imports) { process_imports(config) } + if (is.character(config$parallelism)) { + config$schedule <- pretrim_schedule(config) + } + abort <- FALSE + if (check_intv_make(config)) { + abort <- abort_intv_make(config) # nocov + } + if (abort) { + return(invisible()) # nocov + } if (!config$skip_targets) { process_targets(config) } diff --git a/R/api-package.R b/R/api-package.R index 7b6589921..f354850f7 100644 --- a/R/api-package.R +++ b/R/api-package.R @@ -49,6 +49,6 @@ #' @importFrom methods new setRefClass #' @importFrom rlang enquo eval_tidy expr quo_squash quos #' @importFrom storr storr_environment storr_rds -#' @importFrom utils compareVersion head packageVersion +#' @importFrom utils compareVersion head menu packageVersion #' read.csv sessionInfo stack type.convert unzip write.table NULL diff --git a/R/exec-backend.R b/R/exec-backend.R index 09f2dfd4f..b8b62de9c 100644 --- a/R/exec-backend.R +++ b/R/exec-backend.R @@ -11,7 +11,6 @@ run_native_backend <- function(config) { config$parallelism, c("loop", "clustermq", "future") ) - config$schedule <- pretrim_schedule(config) if (igraph::gorder(config$schedule)) { get( paste0("backend_", parallelism), diff --git a/R/exec-session.R b/R/exec-session.R index 809e7b2f9..4ed96378b 100644 --- a/R/exec-session.R +++ b/R/exec-session.R @@ -83,3 +83,26 @@ conclude_session <- function(config) { console_final_notes(config) invisible() } + +check_intv_make <- function(config) { + force_intv <- config$force %||% + getOption("drake_force_interactive") %||% + FALSE + interactive() && + igraph::gorder(config$schedule) && + !identical(force_intv, TRUE) +} + +abort_intv_make <- function(config) { + # nocov start + title <- paste( + paste(igraph::gorder(config$schedule), "outdated targets:"), + multiline_message(igraph::V(config$schedule)$name), + "\nReally run make() in interactive mode?", + "Considerations: https://github.com/ropensci/drake/issues/761", + sep = "\n" + ) + out <- utils::menu(choices = c("yes", "no"), title = title) + identical(as.integer(out), 2L) + # nocov end +} diff --git a/R/preprocess-config.R b/R/preprocess-config.R index e8ddd8ff9..a4e8933f6 100644 --- a/R/preprocess-config.R +++ b/R/preprocess-config.R @@ -432,7 +432,7 @@ drake_config <- function( cpu = Inf, elapsed = Inf, retries = 0, - force = FALSE, + force = NULL, log_progress = FALSE, graph = NULL, trigger = drake::trigger(), @@ -553,7 +553,7 @@ drake_config <- function( console_log_file = console_log_file ) } - if (force) { + if (force %||% FALSE) { drake_set_session_info(cache = cache, full = session_info) } seed <- choose_seed(supplied = seed, cache = cache) @@ -632,7 +632,8 @@ drake_config <- function( template = template, sleep = sleep, hasty_build = hasty_build, - lock_envir = lock_envir + lock_envir = lock_envir, + force = force ) out <- enforce_compatible_config(out) config_checks(out) diff --git a/R/vis-color.R b/R/vis-color.R index e53317234..32c9883f2 100644 --- a/R/vis-color.R +++ b/R/vis-color.R @@ -14,6 +14,7 @@ colors <- c( launch = "#ff9933", load = "#ff9933", unload = "#ff7221", + interactive = "#ff7221", trigger = "maroon", skip = "skyblue1", store = "skyblue1", diff --git a/R/vis-console.R b/R/vis-console.R index 20f9cc616..3319618a0 100644 --- a/R/vis-console.R +++ b/R/vis-console.R @@ -123,7 +123,25 @@ console_up_to_date <- function(config) { } } +console_interactive <- function(config) { + if (!interactive()) { + return() + } + # nocov start + msg <- paste0( + "make() in interactive mode requires extra care:\n", + "https://github.com/ropensci/drake/issues/761" + ) + out <- color(msg, colors["interactive"]) + drake_message(out, config = config) + # nocov end +} + console_final_notes <- function(config) { + if (config$verbose < 1L) { + return() + } + console_interactive(config) if (config$verbose < 2L) { return() } diff --git a/man/drake_config.Rd b/man/drake_config.Rd index aa7f670d7..f41aca6ed 100644 --- a/man/drake_config.Rd +++ b/man/drake_config.Rd @@ -12,7 +12,7 @@ drake_config(plan, targets = NULL, envir = parent.frame(), packages = rev(.packages()), lib_loc = NULL, prework = character(0), prepend = NULL, command = NULL, args = NULL, recipe_command = NULL, timeout = NULL, cpu = Inf, - elapsed = Inf, retries = 0, force = FALSE, log_progress = FALSE, + elapsed = Inf, retries = 0, force = NULL, log_progress = FALSE, graph = NULL, trigger = drake::trigger(), skip_targets = FALSE, skip_imports = FALSE, skip_safety_checks = FALSE, lazy_load = "eager", session_info = TRUE, cache_log_file = NULL, diff --git a/man/make.Rd b/man/make.Rd index 39224759a..1f611b16c 100644 --- a/man/make.Rd +++ b/man/make.Rd @@ -12,7 +12,7 @@ make(plan, targets = NULL, envir = parent.frame(), verbose = 1L, prework = character(0), prepend = NULL, command = NULL, args = NULL, recipe_command = NULL, log_progress = TRUE, skip_targets = FALSE, timeout = NULL, cpu = Inf, elapsed = Inf, - retries = 0, force = FALSE, graph = NULL, + retries = 0, force = NULL, graph = NULL, trigger = drake::trigger(), skip_imports = FALSE, skip_safety_checks = FALSE, config = NULL, lazy_load = "eager", session_info = TRUE, cache_log_file = NULL, seed = NULL, @@ -455,6 +455,5 @@ clean() # Start from scratch next time around. \code{\link[=drake_plan]{drake_plan()}}, \code{\link[=drake_config]{drake_config()}}, \code{\link[=vis_drake_graph]{vis_drake_graph()}}, -\code{\link[=outdated]{outdated()}}, -\code{\link[=triggers]{triggers()}} +\code{\link[=outdated]{outdated()}} } diff --git a/tests/testthat/test-always-skipped.R b/tests/testthat/test-always-skipped.R index 20660f0e2..f1e6b4f70 100644 --- a/tests/testthat/test-always-skipped.R +++ b/tests/testthat/test-always-skipped.R @@ -81,4 +81,41 @@ test_with_dir("forks + lock_envir = informative error msg", { ) }) +test_with_dir("make() in interactive mode", { + # Must run this test in an interactive session. + # Cannot be fully automated like the other tests. + load_mtcars_example() + config <- drake_config(my_plan) + make(my_plan) # Select 2. + expect_equal(cached(), character(0)) + expect_equal(sort(outdated(config)), sort(my_plan$target)) + expect_equal(sort(justbuilt(config)), character(0)) + make(my_plan, console_log_file = "log.txt") # Select 1. + expect_equal(cached(), sort(my_plan$target)) + expect_equal(sort(outdated(config)), character(0)) + expect_equal(sort(justbuilt(config)), sort(my_plan$target)) + lines <- readLines("log.txt") + expect_true(any(grepl("interactive", lines))) + expect_false(any(grepl("up to date", lines))) + make(my_plan, console_log_file = "log.txt") # No menu. + expect_equal(cached(), sort(my_plan$target)) + expect_equal(sort(outdated(config)), character(0)) + expect_equal(sort(justbuilt(config)), character(0)) + lines <- readLines("log.txt") + expect_true(any(grepl("interactive", lines))) + expect_true(any(grepl("up to date", lines))) + clean() + make(my_plan, force = TRUE) # No menu. + expect_equal(sort(outdated(config)), character(0)) + expect_equal(sort(justbuilt(config)), sort(my_plan$target)) + clean() + options(drake_force_interactive = TRUE) + make(my_plan, force = FALSE) # Select 2. + expect_equal(sort(outdated(config)), sort(my_plan$target)) + expect_equal(sort(justbuilt(config)), character(0)) + make(my_plan) # No menu. + expect_equal(sort(outdated(config)), character(0)) + expect_equal(sort(justbuilt(config)), sort(my_plan$target)) +}) + } From 2ed82b4b2d105f308f6b5a4d0d343dcefbb6d182 Mon Sep 17 00:00:00 2001 From: wlandau-lilly Date: Fri, 1 Mar 2019 14:19:42 -0500 Subject: [PATCH 2/3] Document the menu proposed in #761 --- R/preprocess-config.R | 17 +++++++++++------ man/drake_config.Rd | 19 +++++++++++++------ man/make.Rd | 19 +++++++++++++------ 3 files changed, 37 insertions(+), 18 deletions(-) diff --git a/R/preprocess-config.R b/R/preprocess-config.R index a4e8933f6..d7ddaf7c2 100644 --- a/R/preprocess-config.R +++ b/R/preprocess-config.R @@ -142,14 +142,19 @@ #' Assign target-level retries with an optional `retries` #' column in `plan`. #' -#' @param force Logical. If `FALSE` (default) then `drake` will stop you -#' if the cache was created with an old -#' and incompatible version of drake. -#' This gives you an opportunity to +#' @param force Logical. If `FALSE` (default) then `drake` +#' imposes the following safeguards +#' to keep `make()` from mangling your project. +#' 1. If you are running an interactive session +#' (i.e. if `interactive()` is `TRUE`) +#' and some targets are outdated, +#' then `make()` pauses with a menu to check if you really want to +#' proceed. Ref: . +#' 2. If the cache was created with an old +#' and incompatible version of drake, `make()` stops to +#' give you an opportunity to #' downgrade `drake` to a compatible version #' rather than rerun all your targets from scratch. -#' If `force` is `TRUE`, then `make()` executes your workflow -#' regardless of the version of `drake` that last ran `make()` on the cache. #' #' @param graph An `igraph` object from the previous `make()`. #' Supplying a pre-built graph could save time. diff --git a/man/drake_config.Rd b/man/drake_config.Rd index f41aca6ed..cfd21ae1b 100644 --- a/man/drake_config.Rd +++ b/man/drake_config.Rd @@ -146,14 +146,21 @@ column in \code{plan}.} Assign target-level retries with an optional \code{retries} column in \code{plan}.} -\item{force}{Logical. If \code{FALSE} (default) then \code{drake} will stop you -if the cache was created with an old -and incompatible version of drake. -This gives you an opportunity to +\item{force}{Logical. If \code{FALSE} (default) then \code{drake} +imposes the following safeguards +to keep \code{make()} from mangling your project. +\enumerate{ +\item If you are running an interactive session +(i.e. if \code{interactive()} is \code{TRUE}) +and some targets are outdated, +then \code{make()} pauses with a menu to check if you really want to +proceed. Ref: \url{https://github.com/ropensci/drake/issues/761}. +\item If the cache was created with an old +and incompatible version of drake, \code{make()} stops to +give you an opportunity to downgrade \code{drake} to a compatible version rather than rerun all your targets from scratch. -If \code{force} is \code{TRUE}, then \code{make()} executes your workflow -regardless of the version of \code{drake} that last ran \code{make()} on the cache.} +}} \item{log_progress}{Logical, whether to log the progress of individual targets as they are being built. Progress logging diff --git a/man/make.Rd b/man/make.Rd index 1f611b16c..3446e5602 100644 --- a/man/make.Rd +++ b/man/make.Rd @@ -156,14 +156,21 @@ column in \code{plan}.} Assign target-level retries with an optional \code{retries} column in \code{plan}.} -\item{force}{Logical. If \code{FALSE} (default) then \code{drake} will stop you -if the cache was created with an old -and incompatible version of drake. -This gives you an opportunity to +\item{force}{Logical. If \code{FALSE} (default) then \code{drake} +imposes the following safeguards +to keep \code{make()} from mangling your project. +\enumerate{ +\item If you are running an interactive session +(i.e. if \code{interactive()} is \code{TRUE}) +and some targets are outdated, +then \code{make()} pauses with a menu to check if you really want to +proceed. Ref: \url{https://github.com/ropensci/drake/issues/761}. +\item If the cache was created with an old +and incompatible version of drake, \code{make()} stops to +give you an opportunity to downgrade \code{drake} to a compatible version rather than rerun all your targets from scratch. -If \code{force} is \code{TRUE}, then \code{make()} executes your workflow -regardless of the version of \code{drake} that last ran \code{make()} on the cache.} +}} \item{graph}{An \code{igraph} object from the previous \code{make()}. Supplying a pre-built graph could save time.} From 66a1fdcd529990ca88f6b275a0915ed8335e367f Mon Sep 17 00:00:00 2001 From: wlandau-lilly Date: Fri, 1 Mar 2019 14:31:51 -0500 Subject: [PATCH 3/3] Update news and elaborate on options --- NEWS.md | 1 + R/preprocess-config.R | 4 ++++ man/drake_config.Rd | 4 ++++ man/make.Rd | 4 ++++ 4 files changed, 13 insertions(+) diff --git a/NEWS.md b/NEWS.md index b77c1939c..e87cc4705 100644 --- a/NEWS.md +++ b/NEWS.md @@ -29,6 +29,7 @@ - Introduce a new experimental domain-specific language for generating large plans (#233). Details [here](file:///home/landau/projects/drake-manual/_book/plans.html#large-plans). - Implement a `lock_envir` argument to safeguard reproducibility. See [this thread](https://github.com/ropensci/drake/issues/615#issuecomment-447585359) for a demonstration of the problem solved by `make(lock_envir = TRUE)`. More discussion: #619, #620. - The new `from_plan()` function allows the users to reference custom plan columns from within commands. Changes to values in these columns columns do not invalidate targets. +- Add a menu prompt (https://github.com/ropensci/drake/pull/762) to safeguard against `make()` pitfalls in interactive mode (https://github.com/ropensci/drake/issues/761). Disable with `make(force = TRUE)` or `options(drake_force_interactive = TRUE)`. ## Enhancements diff --git a/R/preprocess-config.R b/R/preprocess-config.R index d7ddaf7c2..0862b7cff 100644 --- a/R/preprocess-config.R +++ b/R/preprocess-config.R @@ -150,6 +150,10 @@ #' and some targets are outdated, #' then `make()` pauses with a menu to check if you really want to #' proceed. Ref: . +#' You can also disable this menu with +#' `options(drake_force_interactive = TRUE)`. +#' Save `options(drake_force_interactive = TRUE)` in your +#' `~/.Rprofile` file to never see the menu. #' 2. If the cache was created with an old #' and incompatible version of drake, `make()` stops to #' give you an opportunity to diff --git a/man/drake_config.Rd b/man/drake_config.Rd index cfd21ae1b..f9816c56b 100644 --- a/man/drake_config.Rd +++ b/man/drake_config.Rd @@ -155,6 +155,10 @@ to keep \code{make()} from mangling your project. and some targets are outdated, then \code{make()} pauses with a menu to check if you really want to proceed. Ref: \url{https://github.com/ropensci/drake/issues/761}. +You can also disable this menu with +\code{options(drake_force_interactive = TRUE)}. +Save \code{options(drake_force_interactive = TRUE)} in your +\code{~/.Rprofile} file to never see the menu. \item If the cache was created with an old and incompatible version of drake, \code{make()} stops to give you an opportunity to diff --git a/man/make.Rd b/man/make.Rd index 3446e5602..a4c04ce06 100644 --- a/man/make.Rd +++ b/man/make.Rd @@ -165,6 +165,10 @@ to keep \code{make()} from mangling your project. and some targets are outdated, then \code{make()} pauses with a menu to check if you really want to proceed. Ref: \url{https://github.com/ropensci/drake/issues/761}. +You can also disable this menu with +\code{options(drake_force_interactive = TRUE)}. +Save \code{options(drake_force_interactive = TRUE)} in your +\code{~/.Rprofile} file to never see the menu. \item If the cache was created with an old and incompatible version of drake, \code{make()} stops to give you an opportunity to