From 8de614e973f8d9f09cb8c5d85998933ef26aface Mon Sep 17 00:00:00 2001 From: wlandau Date: Fri, 18 Sep 2020 08:22:17 -0400 Subject: [PATCH] Fix #161 --- R/class_clustermq.R | 10 +--------- R/class_cross.R | 4 +--- R/class_map.R | 4 +--- R/class_settings.R | 5 ----- R/class_stem.R | 4 +--- R/class_target.R | 2 -- R/tar_make_clustermq.R | 8 ++------ R/tar_option_get.R | 1 - R/tar_option_set.R | 8 -------- R/tar_target.R | 18 ++++++------------ R/tar_target_raw.R | 3 --- R/utils_assert.R | 12 ------------ man/tar_make_clustermq.Rd | 6 ++---- man/tar_option_set.Rd | 17 ++++++----------- man/tar_target.Rd | 17 ++++++----------- man/tar_target_raw.Rd | 17 ++++++----------- tests/testthat/test-class_clustermq.R | 5 ----- tests/testthat/test-class_settings.R | 4 ++-- tests/testthat/test-class_stem.R | 11 ++++------- tests/testthat/test-utils_assert.R | 7 ------- 20 files changed, 38 insertions(+), 125 deletions(-) diff --git a/R/class_clustermq.R b/R/class_clustermq.R index c5d7b6d4d..e8f4a39da 100644 --- a/R/class_clustermq.R +++ b/R/class_clustermq.R @@ -6,7 +6,6 @@ clustermq_init <- function( reporter = "verbose", garbage_collection = FALSE, workers = 1L, - template = list(), log_worker = FALSE ) { clustermq_new( @@ -17,7 +16,6 @@ clustermq_init <- function( reporter = reporter, garbage_collection = garbage_collection, workers = as.integer(workers), - template = as.list(template), log_worker = log_worker ) } @@ -31,7 +29,6 @@ clustermq_new <- function( garbage_collection = NULL, workers = NULL, crew = NULL, - template = NULL, log_worker = NULL ) { clustermq_class$new( @@ -43,7 +40,6 @@ clustermq_new <- function( garbage_collection = garbage_collection, workers = workers, crew = crew, - template = template, log_worker = log_worker ) } @@ -57,7 +53,6 @@ clustermq_class <- R6::R6Class( public = list( workers = NULL, crew = NULL, - template = NULL, log_worker = NULL, initialize = function( pipeline = NULL, @@ -68,7 +63,6 @@ clustermq_class <- R6::R6Class( garbage_collection = NULL, workers = NULL, crew = NULL, - template = NULL, log_worker = NULL ) { super$initialize( @@ -81,7 +75,6 @@ clustermq_class <- R6::R6Class( ) self$workers <- workers self$crew <- crew - self$template <- template self$log_worker <- log_worker }, set_common_data = function(envir) { @@ -98,7 +91,7 @@ clustermq_class <- R6::R6Class( create_crew = function() { crew <- clustermq::workers( n_jobs = self$workers, - template = self$template, + template = tar_option_get("resources"), log_worker = self$log_worker ) self$crew <- crew @@ -228,7 +221,6 @@ clustermq_class <- R6::R6Class( super$validate() assert_lgl(self$garbage_collection) assert_int(self$workers) - assert_list(self$template) } ) ) diff --git a/R/class_cross.R b/R/class_cross.R index 9270a1c9c..065ab3a62 100644 --- a/R/class_cross.R +++ b/R/class_cross.R @@ -52,9 +52,7 @@ print.tar_cross <- function(x, ...) { "\n storage mode:", x$settings$storage, "\n retrieval mode:", x$settings$retrieval, "\n deploy to:", x$settings$deployment, - "\n template (clustermq):\n ", - produce_lines(paste_list(x$settings$template)), - "\n resources (future):\n ", + "\n resources:\n ", produce_lines(paste_list(x$settings$resources)), "\n cue:\n ", produce_lines(paste_list(as.list(x$cue))), diff --git a/R/class_map.R b/R/class_map.R index b7661d9f7..473694cdc 100644 --- a/R/class_map.R +++ b/R/class_map.R @@ -57,9 +57,7 @@ print.tar_map <- function(x, ...) { "\n storage mode:", x$settings$storage, "\n retrieval mode:", x$settings$retrieval, "\n deploy to:", x$settings$deployment, - "\n template (clustermq):\n ", - produce_lines(paste_list(x$settings$template)), - "\n resources (future):\n ", + "\n resources:\n ", produce_lines(paste_list(x$settings$resources)), "\n cue:\n ", produce_lines(paste_list(as.list(x$cue))), diff --git a/R/class_settings.R b/R/class_settings.R index a33d04ad5..101748199 100644 --- a/R/class_settings.R +++ b/R/class_settings.R @@ -8,7 +8,6 @@ settings_init <- function( deployment = "remote", priority = 0, resources = list(), - template = NULL, storage = "local", retrieval = storage ) { @@ -25,7 +24,6 @@ settings_init <- function( deployment = deployment, priority = priority, resources = resources, - template = template, storage = storage, retrieval = retrieval ) @@ -42,7 +40,6 @@ settings_new <- function( deployment = NULL, priority = NULL, resources = NULL, - template = NULL, storage = NULL, retrieval = NULL ) { @@ -56,7 +53,6 @@ settings_new <- function( force(deployment) force(priority) force(resources) - force(template) force(storage) force(retrieval) environment() @@ -78,7 +74,6 @@ settings_clone <- function(settings) { deployment = settings$deployment, priority = settings$priority, resources = settings$resources, - template = settings$template, storage = settings$storage, retrieval = settings$retrieval ) diff --git a/R/class_stem.R b/R/class_stem.R index 482fd82f8..eba914ce7 100644 --- a/R/class_stem.R +++ b/R/class_stem.R @@ -173,9 +173,7 @@ print.tar_stem <- function(x, ...) { "\n storage mode:", x$settings$storage, "\n retrieval mode:", x$settings$retrieval, "\n deploy to:", x$settings$deployment, - "\n template (clustermq):\n ", - produce_lines(paste_list(x$settings$template)), - "\n resources (future):\n ", + "\n resources:\n ", produce_lines(paste_list(x$settings$resources)), "\n cue:\n ", produce_lines(paste_list(as.list(x$cue))), diff --git a/R/class_target.R b/R/class_target.R index 020169e1e..1cea51213 100644 --- a/R/class_target.R +++ b/R/class_target.R @@ -13,7 +13,6 @@ target_init <- function( memory = "persistent", deployment = "remote", priority = 0, - template = NULL, resources = list(), storage = "local", retrieval = storage, @@ -34,7 +33,6 @@ target_init <- function( memory = memory, deployment = deployment, priority = priority, - template = template, resources = resources, storage = storage, retrieval = retrieval diff --git a/R/tar_make_clustermq.R b/R/tar_make_clustermq.R index b34db98bf..94549d99a 100644 --- a/R/tar_make_clustermq.R +++ b/R/tar_make_clustermq.R @@ -12,12 +12,12 @@ #' To read more about configuring `clustermq` for your scheduler, visit #' # nolint #' and navigate to the appropriate link under "Setting up the scheduler". +#' Wildcards in the template file are filled in with elements from +#' `tar_option_get("resources")`. #' @return `NULL` except if `callr_function = callr::r_bg()`, in which case #' a handle to the `callr` background process is returned. Either way, #' the value is invisibly returned. #' @inheritParams tar_make_future -#' @param template Named list of values to insert as fields -#' in the `clustermq` template file, such as computing resource requirements. #' @param log_worker Logical, whether to write a log file for each worker. #' Same as the `log_worker` argument of `clustermq::Q()` #' and `clustermq::workers()`. @@ -37,7 +37,6 @@ tar_make_clustermq <- function( reporter = "verbose", garbage_collection = FALSE, workers = 1L, - template = list(), log_worker = FALSE, callr_function = callr::r, callr_arguments = list() @@ -52,7 +51,6 @@ tar_make_clustermq <- function( reporter = reporter, garbage_collection = garbage_collection, workers = workers, - template = template, log_worker = log_worker ) out <- callr_outer( @@ -70,7 +68,6 @@ tar_make_clustermq_inner <- function( reporter, garbage_collection, workers, - template, log_worker ) { pipeline_validate_lite(pipeline) @@ -82,7 +79,6 @@ tar_make_clustermq_inner <- function( reporter = reporter, garbage_collection = garbage_collection, workers = workers, - template = template, log_worker = log_worker )$run() invisible() diff --git a/R/tar_option_get.R b/R/tar_option_get.R index a84db2940..f8adee47e 100644 --- a/R/tar_option_get.R +++ b/R/tar_option_get.R @@ -38,7 +38,6 @@ tar_option_default <- function(option) { deployment = "remote", priority = 0, resources = list(), - template = NULL, storage = "local", retrieval = "local", cue = targets::tar_cue(), diff --git a/R/tar_option_set.R b/R/tar_option_set.R index 30402ea79..1be93fe27 100644 --- a/R/tar_option_set.R +++ b/R/tar_option_set.R @@ -39,7 +39,6 @@ tar_option_set <- function( deployment = NULL, priority = NULL, resources = NULL, - template = NULL, storage = NULL, retrieval = NULL, cue = NULL, @@ -57,7 +56,6 @@ tar_option_set <- function( tar_option_set_deployment(deployment) tar_option_set_priority(priority) tar_option_set_resources(resources) - tar_option_set_template(template) tar_option_set_storage(storage) tar_option_set_retrieval(retrieval) tar_option_set_cue(cue) @@ -138,12 +136,6 @@ tar_option_set_resources <- function(resources) { assign("resources", resources, envir = tar_envir_options) } -tar_option_set_template <- function(template) { - template <- template %||% tar_option_get("template") - warn_template(template) - assign("template", template, envir = tar_envir_options) -} - tar_option_set_storage <- function(storage) { storage <- storage %||% tar_option_get("storage") storage <- match.arg(storage, c("local", "remote")) diff --git a/R/tar_target.R b/R/tar_target.R index 572cbb9cc..7f374302a 100644 --- a/R/tar_target.R +++ b/R/tar_target.R @@ -88,15 +88,12 @@ #' @param priority Numeric of length 1 between 0 and 1. Controls which #' targets get deployed first when multiple competing targets are ready #' simultaneously. Targets with priorities closer to 1 get built earlier. -#' @param template Relevant to [tar_make_clustermq()] only. -#' Named list of values to fill in the `clustermq` template file. -#' Unsupported for now. May be supported in the future if -#' `clustermq` ever supports heterogeneous workers with varying -#' resource requirements. In the meantime, use the `template` -#' argument of [tar_make_clustermq()]. -#' @param resources Relevant to [tar_make_future()] only. -#' A named list of resources passed to `future::future()` when -#' defining a new worker. +#' @param resources A named list of computing resources +#' for high-performance computing. Elements in this list are passed +#' to `future::future()` in [tar_make_future()] and +#' to `clustermq::workers()` in [tar_make_clustermq()] +#' to fill in the patterns in cluster computing +#' template files with user-defined values. #' @param storage Character of length 1, only relevant to #' [tar_make_clustermq()] and [tar_make_future()]. #' If `"local"`, the target's return value is sent back to the @@ -137,7 +134,6 @@ tar_target <- function( memory = targets::tar_option_get("memory"), deployment = targets::tar_option_get("deployment"), priority = targets::tar_option_get("priority"), - template = targets::tar_option_get("template"), resources = targets::tar_option_get("resources"), storage = targets::tar_option_get("storage"), retrieval = targets::tar_option_get("retrieval"), @@ -160,7 +156,6 @@ tar_target <- function( assert_scalar(priority) assert_ge(priority, 0) assert_le(priority, 1) - warn_template(template) assert_list(resources, "resources in tar_target() must be a named list.") storage <- match.arg(storage, c("local", "remote")) retrieval <- match.arg(retrieval, c("local", "remote")) @@ -183,7 +178,6 @@ tar_target <- function( memory = memory, deployment = deployment, priority = priority, - template = template, resources = resources, storage = storage, retrieval = retrieval, diff --git a/R/tar_target_raw.R b/R/tar_target_raw.R index 44ac286e6..34422844d 100644 --- a/R/tar_target_raw.R +++ b/R/tar_target_raw.R @@ -55,7 +55,6 @@ tar_target_raw <- function( memory = targets::tar_option_get("memory"), deployment = targets::tar_option_get("deployment"), priority = targets::tar_option_get("priority"), - template = targets::tar_option_get("template"), resources = targets::tar_option_get("resources"), storage = targets::tar_option_get("storage"), retrieval = targets::tar_option_get("retrieval"), @@ -76,7 +75,6 @@ tar_target_raw <- function( assert_scalar(priority) assert_ge(priority, 0) assert_le(priority, 1) - warn_template(template) assert_list( resources, "resources in tar_target_raw() must be a named list." @@ -99,7 +97,6 @@ tar_target_raw <- function( memory = memory, deployment = deployment, priority = priority, - template = template, resources = resources, storage = storage, retrieval = retrieval, diff --git a/R/utils_assert.R b/R/utils_assert.R index 610457ab8..62f9e271d 100644 --- a/R/utils_assert.R +++ b/R/utils_assert.R @@ -241,15 +241,3 @@ warn_output <- function(name, path) { ) } } - -warn_template <- function(template) { - if (!is.null(template)) { - warn_validate( - "Functions tar_option_set(), tar_target(), and tar_target_raw() ", - "currently ignore the template argument. It will only be supported if ", - "clustermq ever supports heterogeneous workers with varying resource ", - "configurations. In the meantime, use the template argument of ", - "tar_make_clustermq()." - ) - } -} diff --git a/man/tar_make_clustermq.Rd b/man/tar_make_clustermq.Rd index 8a778ca03..8eed17ced 100644 --- a/man/tar_make_clustermq.Rd +++ b/man/tar_make_clustermq.Rd @@ -10,7 +10,6 @@ tar_make_clustermq( reporter = "verbose", garbage_collection = FALSE, workers = 1L, - template = list(), log_worker = FALSE, callr_function = callr::r, callr_arguments = list() @@ -38,9 +37,6 @@ between targets. The pipeline will run slower but consume less memory.} \item{workers}{Positive integer, maximum number of transient \code{future} workers allowed to run at any given time.} -\item{template}{Named list of values to insert as fields -in the \code{clustermq} template file, such as computing resource requirements.} - \item{log_worker}{Logical, whether to write a log file for each worker. Same as the \code{log_worker} argument of \code{clustermq::Q()} and \code{clustermq::workers()}.} @@ -75,6 +71,8 @@ To use with a cluster, you will need to set the global options To read more about configuring \code{clustermq} for your scheduler, visit \url{https://mschubert.github.io/clustermq/articles/userguide.html#configuration} # nolint and navigate to the appropriate link under "Setting up the scheduler". +Wildcards in the template file are filled in with elements from +\code{tar_option_get("resources")}. } \examples{ \dontrun{ diff --git a/man/tar_option_set.Rd b/man/tar_option_set.Rd index 5f5f22c53..4892198c7 100644 --- a/man/tar_option_set.Rd +++ b/man/tar_option_set.Rd @@ -16,7 +16,6 @@ tar_option_set( deployment = NULL, priority = NULL, resources = NULL, - template = NULL, storage = NULL, retrieval = NULL, cue = NULL, @@ -113,16 +112,12 @@ the target builds on the host machine / process managing the pipeline.} targets get deployed first when multiple competing targets are ready simultaneously. Targets with priorities closer to 1 get built earlier.} -\item{resources}{Relevant to \code{\link[=tar_make_future]{tar_make_future()}} only. -A named list of resources passed to \code{future::future()} when -defining a new worker.} - -\item{template}{Relevant to \code{\link[=tar_make_clustermq]{tar_make_clustermq()}} only. -Named list of values to fill in the \code{clustermq} template file. -Unsupported for now. May be supported in the future if -\code{clustermq} ever supports heterogeneous workers with varying -resource requirements. In the meantime, use the \code{template} -argument of \code{\link[=tar_make_clustermq]{tar_make_clustermq()}}.} +\item{resources}{A named list of computing resources +for high-performance computing. Elements in this list are passed +to \code{future::future()} in \code{\link[=tar_make_future]{tar_make_future()}} and +to \code{clustermq::workers()} in \code{\link[=tar_make_clustermq]{tar_make_clustermq()}} +to fill in the patterns in cluster computing +template files with user-defined values.} \item{storage}{Character of length 1, only relevant to \code{\link[=tar_make_clustermq]{tar_make_clustermq()}} and \code{\link[=tar_make_future]{tar_make_future()}}. diff --git a/man/tar_target.Rd b/man/tar_target.Rd index fb7308fce..33c911339 100644 --- a/man/tar_target.Rd +++ b/man/tar_target.Rd @@ -17,7 +17,6 @@ tar_target( memory = targets::tar_option_get("memory"), deployment = targets::tar_option_get("deployment"), priority = targets::tar_option_get("priority"), - template = targets::tar_option_get("template"), resources = targets::tar_option_get("resources"), storage = targets::tar_option_get("storage"), retrieval = targets::tar_option_get("retrieval"), @@ -121,16 +120,12 @@ the target builds on the host machine / process managing the pipeline.} targets get deployed first when multiple competing targets are ready simultaneously. Targets with priorities closer to 1 get built earlier.} -\item{template}{Relevant to \code{\link[=tar_make_clustermq]{tar_make_clustermq()}} only. -Named list of values to fill in the \code{clustermq} template file. -Unsupported for now. May be supported in the future if -\code{clustermq} ever supports heterogeneous workers with varying -resource requirements. In the meantime, use the \code{template} -argument of \code{\link[=tar_make_clustermq]{tar_make_clustermq()}}.} - -\item{resources}{Relevant to \code{\link[=tar_make_future]{tar_make_future()}} only. -A named list of resources passed to \code{future::future()} when -defining a new worker.} +\item{resources}{A named list of computing resources +for high-performance computing. Elements in this list are passed +to \code{future::future()} in \code{\link[=tar_make_future]{tar_make_future()}} and +to \code{clustermq::workers()} in \code{\link[=tar_make_clustermq]{tar_make_clustermq()}} +to fill in the patterns in cluster computing +template files with user-defined values.} \item{storage}{Character of length 1, only relevant to \code{\link[=tar_make_clustermq]{tar_make_clustermq()}} and \code{\link[=tar_make_future]{tar_make_future()}}. diff --git a/man/tar_target_raw.Rd b/man/tar_target_raw.Rd index 191863016..61222c54c 100644 --- a/man/tar_target_raw.Rd +++ b/man/tar_target_raw.Rd @@ -18,7 +18,6 @@ tar_target_raw( memory = targets::tar_option_get("memory"), deployment = targets::tar_option_get("deployment"), priority = targets::tar_option_get("priority"), - template = targets::tar_option_get("template"), resources = targets::tar_option_get("resources"), storage = targets::tar_option_get("storage"), retrieval = targets::tar_option_get("retrieval"), @@ -130,16 +129,12 @@ the target builds on the host machine / process managing the pipeline.} targets get deployed first when multiple competing targets are ready simultaneously. Targets with priorities closer to 1 get built earlier.} -\item{template}{Relevant to \code{\link[=tar_make_clustermq]{tar_make_clustermq()}} only. -Named list of values to fill in the \code{clustermq} template file. -Unsupported for now. May be supported in the future if -\code{clustermq} ever supports heterogeneous workers with varying -resource requirements. In the meantime, use the \code{template} -argument of \code{\link[=tar_make_clustermq]{tar_make_clustermq()}}.} - -\item{resources}{Relevant to \code{\link[=tar_make_future]{tar_make_future()}} only. -A named list of resources passed to \code{future::future()} when -defining a new worker.} +\item{resources}{A named list of computing resources +for high-performance computing. Elements in this list are passed +to \code{future::future()} in \code{\link[=tar_make_future]{tar_make_future()}} and +to \code{clustermq::workers()} in \code{\link[=tar_make_clustermq]{tar_make_clustermq()}} +to fill in the patterns in cluster computing +template files with user-defined values.} \item{storage}{Character of length 1, only relevant to \code{\link[=tar_make_clustermq]{tar_make_clustermq()}} and \code{\link[=tar_make_future]{tar_make_future()}}. diff --git a/tests/testthat/test-class_clustermq.R b/tests/testthat/test-class_clustermq.R index 5e81fd0f9..aaad22fa8 100644 --- a/tests/testthat/test-class_clustermq.R +++ b/tests/testthat/test-class_clustermq.R @@ -3,11 +3,6 @@ tar_test("clustermq$workers", { expect_equal(out$workers, 3L) }) -tar_test("clustermq$template", { - out <- clustermq_init(tar_pipeline(), template = list(cores = 3L)) - expect_equal(out$template, list(cores = 3L)) -}) - tar_test("all local deployment works", { skip_on_os("windows") skip_if_not_installed("clustermq") diff --git a/tests/testthat/test-class_settings.R b/tests/testthat/test-class_settings.R index 058e3f5ef..ce86aaa7d 100644 --- a/tests/testthat/test-class_settings.R +++ b/tests/testthat/test-class_settings.R @@ -16,8 +16,8 @@ tar_test("settings_validate()", { expect_silent(settings_validate(x)) }) -tar_test("settings_validate() with template", { - x <- settings_init(name = "abc", template = "123") +tar_test("settings_validate() with resources", { + x <- settings_init(name = "abc", resources = list(ncores = 2)) expect_silent(settings_validate(x)) }) diff --git a/tests/testthat/test-class_stem.R b/tests/testthat/test-class_stem.R index 409867c1b..82d93160f 100644 --- a/tests/testthat/test-class_stem.R +++ b/tests/testthat/test-class_stem.R @@ -203,13 +203,10 @@ tar_test("stem validate with junction", { }) tar_test("stem print", { - expect_warning( - x <- tar_target(x, { - a <- 1 - b - }, template = list(cpu = 1, mem = 2)), - class = "condition_validate" - ) + x <- tar_target(x, { + a <- 1 + b + }, resources = list(cpu = 1, mem = 2)) out <- utils::capture.output(print(x)) expect_true(any(grepl("stem", out))) }) diff --git a/tests/testthat/test-utils_assert.R b/tests/testthat/test-utils_assert.R index 0fe2e6862..8fb04e21e 100644 --- a/tests/testthat/test-utils_assert.R +++ b/tests/testthat/test-utils_assert.R @@ -103,10 +103,3 @@ tar_test("assert_unique_targets()", { class = "condition_validate" ) }) - -tar_test("warn_template()", { - expect_warning( - tar_target(x, 1, template = list(x = 1)), - class = "condition_validate" - ) -})