Merge pull request #724 from metrumresearchgroup/slurm

Add submission mode for Slurm

DESCRIPTION

@@ -98,5 +98,5 @@ VignetteBuilder:
 Encoding: UTF-8
 LazyData: true
 Roxygen: list(markdown = TRUE)
-RoxygenNote: 7.3.1
+RoxygenNote: 7.3.2
 SystemRequirements: bbi (>= 3.0.2)

R/aaa.R

@@ -5,7 +5,7 @@ BBI_DEFAULT_PATH <- if (ON_WINDOWS) {
   "bbi"
 }
 
-BBI_VALID_MODES <- c("local", "sge")
+BBI_VALID_MODES <- c("local", "sge", "slurm")
 
 CACHE_ENV <- new.env(parent = emptyenv())
 CACHE_ENV$bbi_exe_paths <- list()

R/submit-model.R

@@ -30,9 +30,9 @@
 #'   4)`. Run [print_bbi_args()] to see valid arguments. Note that bbr does
 #'   not support changing the output directory (including through the model or
 #'   global YAML files).
-#' @param .mode Either `"sge"`, the default, to submit model(s) to the grid or
-#'   `"local"` for local execution. This can be passed directly to this argument
-#'   or set globally with `options("bbr.bbi_exe_mode")`.
+#' @param .mode Mode for model submission: "local", "sge", or "slurm". If
+#'   unspecified, the value is set to the value of the `bbr.bbi_exe_mode`
+#'   option. This option defaults to "sge" on Linux and "local" otherwise.
 #' @param ... args passed through to `bbi_exec()`
 #' @param .overwrite Logical to specify whether or not to overwrite existing
 #'   model output from a previous run. If `NULL`, the default, will defer to
@@ -356,6 +356,38 @@ check_mode_argument <- function(.mode) {
     stop(BBI_EXE_MODE_INVALID_ERR_MSG, call. = FALSE)
   }
 
+  if (isTRUE(getOption("bbr.DEV_skip_system_mode_checks"))) {
+    # ^ Allow dry-run tests to skip the system mode checks.
+    return(invisible(TRUE))
+  }
+
+  # All of the remaining checks depend on system details.
+
+  if (identical(.mode, "slurm") && !test_bbi_version(.min_version = "3.4.0")) {
+    stop(
+      "Installed bbi version is ", bbi_version(),
+      ", but .mode='slurm' requires at least version 3.4.0"
+    )
+  }
+
+  if (identical(.mode, "sge")) {
+    qsub <- unname(Sys.which("qsub"))
+    if (identical(qsub, "")) {
+      stop(".mode='sge' but qsub is not available on system")
+    }
+
+    # This guard is Metworx (or really ParallelCluster) specific. Slurm ships
+    # with a qsub shim. That leads to a confusing situation where 'bbi nonmem
+    # run sge ...' will not abort upfront, but the execution doesn't entirely
+    # match the expected behavior (especially under --parallel).
+    if (identical(qsub, "/opt/slurm/bin/qsub")) {
+      stop(
+        ".mode is 'sge' but qsub points to Slurm shim.\n",
+        "Instead set .mode to 'slurm' (requires bbi 3.4.0 or later)\n"
+      )
+    }
+  }
+
   return(invisible(TRUE))
 }
 

tests/testthat/test-print.R

@@ -38,6 +38,8 @@ withr::with_options(list(bbr.bbi_exe_path = read_bbi_path()), {
     fs::dir_create(temp_dir)
     on.exit(fs::dir_delete(temp_dir))
 
+    withr::local_options(list(bbr.DEV_skip_system_mode_checks = TRUE))
+
     readr::write_file("created_by: test-print", file.path(temp_dir, "bbi.yaml"))
 
     mods <- purrr::map(1:50, ~copy_model_from(MOD1, file.path("print-test", .x)))

tests/testthat/test-submit-model.R

@@ -1,117 +1,158 @@
-context("submit_model(.dry_run=T)")
+context("submit_model(.dry_run=TRUE)")
 
 ###################################
 # testing single model submission
 ###################################
 
-
-
 model_dir <- ABS_MODEL_DIR
 mod_ctl_path <- file.path(model_dir, CTL_FILENAME)
 
 # create fake bbi.yaml
 readr::write_file("created_by: test-submit-model", file.path(model_dir, "bbi.yaml"))
-on.exit({ fs::file_delete(file.path(model_dir, "bbi.yaml"))})
-
-withr::with_options(list(bbr.bbi_exe_path = read_bbi_path()), {
-  default_mode <- getOption("bbr.bbi_exe_mode")
-  cmd_prefix <- paste("cd", model_dir, ";",
-                      read_bbi_path(), "nonmem", "run",
-                      default_mode)
-  test_that("submit_model(.dry_run=T) returns correct command string [BBR-SBMT-001]",
-            {
-
-              # correctly parsing yaml
-              expect_identical(
-                submit_model(MOD1, .dry_run = T)[[PROC_CALL]],
-                as.character(glue("{cmd_prefix} {mod_ctl_path} --overwrite --threads=4 --parallel"))
-              )
-
-              # switch to local mode
-              expect_identical(
-                submit_model(MOD1, .mode = "local", .dry_run = T)[[PROC_CALL]],
-                as.character(glue("cd {model_dir} ; {read_bbi_path()} nonmem run local {mod_ctl_path} --overwrite --threads=4 --parallel"))
-              )
-
-              # over-riding yaml arg with passed args
-              expect_identical(
-                submit_model(MOD1,
-                             .bbi_args=list(
-                               "json" = T,
-                               "threads" = 2,
-                               "nm_version" = "nm74"
-                             ),
-                             .dry_run = T)[[PROC_CALL]],
-                as.character(glue("{cmd_prefix} {mod_ctl_path} --overwrite --threads=2 --json --nm_version=nm74 --parallel"))
-              )
-            })
-
-  test_that("submit_model(.dry_run=T) with bbi_nonmem_model object parses correctly [BBR-SBMT-002]",
-            {
-              # correctly parsing yaml
-              expect_identical(
-                submit_model(MOD1, .dry_run = T)[[PROC_CALL]],
-                as.character(glue("{cmd_prefix} {mod_ctl_path} --overwrite --threads=4 --parallel"))
-              )
-
-              # over-riding yaml arg with passed arg
-              expect_identical(
-                submit_model(MOD1, list(threads=2), .dry_run = T)[[PROC_CALL]],
-                as.character(glue("{cmd_prefix} {mod_ctl_path} --overwrite --threads=2 --parallel"))
-              )
-            })
-
-  test_that("submit_model() creates correct call for non-NULL .config_path [BBR-SBMT-003]", {
-
-    temp_config <- tempfile(fileext = ".yaml")
-    readr::write_file("foo", temp_config)
-    temp_config <- normalizePath(temp_config)
-    on.exit(fs::file_delete(temp_config))
-
-    res <- submit_model(MOD1, .config_path = temp_config, .dry_run = TRUE)
-    expect_identical(
-      res[[PROC_CALL]],
-      as.character(
-        glue::glue(
-          "{cmd_prefix} {mod_ctl_path} --overwrite --threads=4 --parallel",
-          "--config={temp_config}",
-          .sep = " "
-        )
+on.exit(fs::file_delete(file.path(model_dir, "bbi.yaml")))
+
+default_mode <- getOption("bbr.bbi_exe_mode")
+cmd_prefix <- paste("cd", model_dir, ";", read_bbi_path(), "nonmem", "run")
+
+withr::local_options(list(
+  bbr.bbi_exe_path = read_bbi_path(),
+  bbr.DEV_skip_system_mode_checks = TRUE
+))
+
+test_that("submit_model(.dry_run=TRUE) returns correct command string", {
+  # correctly parsing yaml
+  expect_identical(
+    submit_model(MOD1, .dry_run = TRUE)[[PROC_CALL]],
+    as.character(glue("{cmd_prefix} {default_mode} {mod_ctl_path} --overwrite --threads=4 --parallel"))
+  )
+
+  # over-riding yaml arg with passed args
+  expect_identical(
+    submit_model(MOD1,
+      .bbi_args = list(
+        "json" = TRUE,
+        "threads" = 2,
+        "nm_version" = "nm74"
+      ),
+      .dry_run = TRUE
+    )[[PROC_CALL]],
+    as.character(glue("{cmd_prefix} {default_mode} {mod_ctl_path} --overwrite --threads=2 --json --nm_version=nm74 --parallel"))
+  )
+})
+
+test_that("submit_model(.dry_run=TRUE) with bbi_nonmem_model object parses correctly", {
+  # correctly parsing yaml
+  expect_identical(
+    submit_model(MOD1, .dry_run = TRUE)[[PROC_CALL]],
+    as.character(glue("{cmd_prefix} {default_mode} {mod_ctl_path} --overwrite --threads=4 --parallel"))
+  )
+
+  # over-riding yaml arg with passed arg
+  expect_identical(
+    submit_model(MOD1, list(threads = 2), .dry_run = TRUE)[[PROC_CALL]],
+    as.character(glue("{cmd_prefix} {default_mode} {mod_ctl_path} --overwrite --threads=2 --parallel"))
+  )
+})
+
+test_that("submit_model() creates correct call for non-NULL .config_path", {
+  temp_config <- tempfile(fileext = ".yaml")
+  readr::write_file("foo", temp_config)
+  temp_config <- normalizePath(temp_config)
+  on.exit(fs::file_delete(temp_config))
+
+  res <- submit_model(MOD1, .config_path = temp_config, .dry_run = TRUE)
+  expect_identical(
+    res[[PROC_CALL]],
+    as.character(
+      glue(
+        "{cmd_prefix} {default_mode} {mod_ctl_path} --overwrite --threads=4 --parallel",
+        "--config={temp_config}",
+        .sep = " "
       )
     )
-  })
+  )
+})
 
-  test_that("submit_model() throws an error if passed `output_dir` bbi arg [BBR-SBMT-004]", {
-    expect_error(
-      submit_model(MOD1, .bbi_args = list(output_dir = "foo")),
-      "is not a valid argument"
-    )
-  })
+test_that("submit_model() throws an error if passed `output_dir` bbi arg", {
+  expect_error(
+    submit_model(MOD1, .bbi_args = list(output_dir = "foo")),
+    "is not a valid argument"
+  )
+})
 
-  test_that("submit_model(.mode) inherits option [BBR-SBMT-005]", {
-    other_mode <- switch(default_mode, sge = "local", "sge")
-    withr::with_options(list(bbr.bbi_exe_mode = other_mode), {
-      expect_identical(
-        submit_model(MOD1, .dry_run = T)[[PROC_CALL]],
-        as.character(glue("cd {model_dir} ; {read_bbi_path()} nonmem run {other_mode} {mod_ctl_path} --overwrite --threads=4 --parallel"))
-      )
-    })
-  })
+test_that("submit_model() .mode argument overrides bbr.bbi_exe_mode default", {
+  other_mode <- if (identical(default_mode, "local")) "slurm" else "local"
+  expect_identical(
+    submit_model(MOD1, .mode = other_mode, .dry_run = TRUE)[[PROC_CALL]],
+    as.character(glue("{cmd_prefix} {other_mode} {mod_ctl_path} --overwrite --threads=4 --parallel"))
+  )
+})
 
-  test_that("submit_model(.mode) errors when NULL [BBR-SBMT-006]", {
-    withr::with_options(list(bbr.bbi_exe_mode = NULL), {
-      expect_error(
-        submit_model(MOD1, .dry_run = T),
-        regexp = "Nothing was passed.+mode"
-      )
-    })
+test_that("submit_model(.mode) inherits option", {
+  other_mode <- if (identical(default_mode, "local")) "slurm" else "local"
+  withr::with_options(list(bbr.bbi_exe_mode = other_mode), {
+    expect_identical(
+      submit_model(MOD1, .dry_run = TRUE)[[PROC_CALL]],
+      as.character(glue("{cmd_prefix} {other_mode} {mod_ctl_path} --overwrite --threads=4 --parallel"))
+    )
   })
+})
 
-  test_that("submit_model(.mode) errors when invalid [BBR-SBMT-007]", {
+test_that("submit_model(.mode) errors when NULL", {
+  withr::with_options(list(bbr.bbi_exe_mode = NULL), {
     expect_error(
-      submit_model(MOD1, .dry_run = T, .mode = "naw"),
-      regexp = "Invalid value passed.+mode"
+      submit_model(MOD1, .dry_run = TRUE),
+      regexp = "Nothing was passed.+mode"
     )
   })
 })
 
+test_that("submit_model(.mode) errors when invalid", {
+  expect_error(
+    submit_model(MOD1, .dry_run = TRUE, .mode = "naw"),
+    regexp = "Invalid value passed.+mode"
+  )
+})
+
+test_that("submit_model aborts if .mode='sge' is used with Slurm's qsub", {
+  skip_if_old_bbi("3.4.0")
+
+  sbatch <- unname(Sys.which("sbatch"))
+  if (identical(sbatch, "") || identical(Sys.getenv("METWORX_VERSION"), "")) {
+    skip("not on Metworx with Slurm")
+  }
+
+  withr::local_options(list(bbr.DEV_skip_system_mode_checks = FALSE))
+
+  expect_error(
+    submit_model(MOD1, .dry_run = TRUE, .mode = "sge"),
+    regexp = "Slurm shim"
+  )
+})
+
+test_that("submit_model aborts if .mode='sge' and qsub is not available", {
+  if (!identical(unname(Sys.which("qsub")), "")) {
+    skip("qsub is available")
+  }
+
+  withr::local_options(list(bbr.DEV_skip_system_mode_checks = FALSE))
+
+  expect_error(
+    submit_model(MOD1, .dry_run = TRUE, .mode = "sge"),
+    regexp = "qsub is not available"
+  )
+})
+
+test_that("submit_model aborts if .mode='slurm' is used with incompatible bbi", {
+  if (test_bbi_version(read_bbi_path(), "3.4.0")) {
+    skip("installed bbi version is compatible with Slurm")
+  }
+
+  withr::local_options(list(bbr.DEV_skip_system_mode_checks = FALSE))
+
+  expect_error(
+    submit_model(MOD1, .dry_run = TRUE, .mode = "slurm"),
+    regexp = "at least version 3.4.0",
+    fixed = TRUE
+  )
+})