Launch the model executable with the TBB it was built against

NEWS.md

@@ -237,6 +237,10 @@ as the other methods. (#1205)
 or was built for another platform, now gives an error naming the executable
 and saying how to rebuild it. Previously the fitting methods and
 `$cmdstan_defaults()` surfaced a raw `processx` error. (#1246)
+* On Windows a model executable is now launched with the TBB it was built
+against, which is recorded at build time. Previously the selected CmdStan
+installation's TBB was used, which was wrong once `set_cmdstan_path()` had
+selected a different one. (#1261)
 
 ## Removed and deprecated
 

R/args.R

@@ -31,6 +31,7 @@ CmdStanArgs <- R6::R6Class(
                           model_methods_env = NULL,
                           standalone_env = NULL,
                           exe_file,
+                          tbb_dir = NULL,
                           proc_ids,
                           method_args,
                           data_file = NULL,
@@ -49,6 +50,7 @@ CmdStanArgs <- R6::R6Class(
       self$stan_file <- stan_file
       self$stan_code <- stan_code
       self$exe_file <- exe_file
+      self$tbb_dir <- tbb_dir
       self$model_methods_env <- model_methods_env
       self$standalone_env <- standalone_env
       self$proc_ids <- proc_ids

R/build.R

@@ -159,16 +159,17 @@ build_executable <- function(stan_file,
     run_make(
       c(wsl_safe_path(repair_path(tmp_exe)), make_vars, stancflags_make), quiet
     )
+    tbb_dir <- tbb_dir_from_options(cpp_options)
     record <- new_build_record(
       configuration = append(
         configuration, list(stanc_options_from_make = as.list(from_make)),
         after = 3
       ),
-      reported_features = reported_features_from_exe(tmp_exe),
+      reported_features = reported_features_from_exe(tmp_exe, tbb_dir),
       dependencies = current$dependencies,
       executable_hash = hash_file(tmp_exe),
       cmdstan = current$cmdstan,
-      tbb_dir = tbb_dir_from_options(cpp_options),
+      tbb_dir = tbb_dir,
       untracked_dependencies = untracked_dependencies(
         current$dependencies$make_local$built_from, user_header
       )

R/build_record.R

@@ -288,13 +288,15 @@ new_build_record <- function(configuration, reported_features, dependencies,
 #' Run an executable's `info` command
 #'
 #' @param exe_file Path to the executable.
+#' @param tbb_dir The TBB directory the build resolved, or `NULL` when there
+#'   is no usable record.
 #' @return The `processx::run()` result. A non-zero exit is not an error.
 #' @noRd
-run_info_cli <- function(exe_file) {
+run_exe_info <- function(exe_file, tbb_dir = NULL) {
   withr::with_path(
     c(
       toolchain_PATH_env_var(),
-      tbb_path()
+      tbb_launch_path(tbb_dir)
     ),
     wsl_compatible_run(
       command = wsl_safe_path(exe_file),
@@ -341,12 +343,14 @@ parse_exe_info_string <- function(ret_stdout) {
 #' every feature unknown rather than failing the build.
 #'
 #' @param exe_file Path to the executable.
+#' @param tbb_dir The TBB directory the build resolved, or `NULL` when there
+#'   is no usable record.
 #' @return A named list of what was kept, empty when nothing was.
 #' @noRd
-reported_features_from_exe <- function(exe_file) {
+reported_features_from_exe <- function(exe_file, tbb_dir = NULL) {
   unknown <- structure(list(), names = character())
   tryCatch({
-    result <- run_info_cli(exe_file)
+    result <- run_exe_info(exe_file, tbb_dir)
     if (result$status != 0) {
       unknown
     } else {
@@ -363,19 +367,21 @@ reported_features_from_exe <- function(exe_file) {
 
 #' Where the record says the TBB is
 #'
-#' The first non-empty of `TBB_LIB` and `TBB_BIN` from the call's
-#' `cpp_options`, then the installation's own copy, which is the order the
-#' makefile links in. The options are read as make receives them: the last
-#' assignment wins and `FALSE` is an empty one. A relative directory is
-#' resolved against the installation, where make runs.
-#'
-#' Make can also pick up both variables from `make/local`,
-#' `~/.config/stan/make.local` or the environment. The record does not look
-#' there, so a build configured that way links against one TBB while the
-#' record names the installation's. On Windows the launch puts the recorded
-#' directory on PATH, so such a build runs with the installation's TBB first,
-#' which is what happens today anyway. We decided not to ask make for the real
-#' answer for now, and could reconsider if there is demand for it.
+#' We use `TBB_LIB` if the call set it, otherwise `TBB_BIN`, otherwise the
+#' installation's own copy, which is the order the makefile uses when it
+#' links. The options are read the way make reads them, so the last
+#' assignment wins and `FALSE` counts as empty. A relative directory is
+#' relative to the installation, since that's where make runs. We take the
+#' value as written; `assert_valid_cpp_options()` has already rejected a
+#' make expression, because nothing here could expand it.
+#'
+#' The two variables can also reach make from `make/local`,
+#' `~/.config/stan/make.local` or the environment, and we don't look there.
+#' So a build set up that way links against a different TBB than the record
+#' names, which is the installation's. On Windows that means the launch puts the
+#' installation's TBB on PATH, which is what happened before too. We decided
+#' not to ask make for the real answer for now and can revisit if anyone
+#' needs it.
 #'
 #' @param cpp_options The call's `cpp_options`, as given.
 #' @return The directory. Under WSL it is the Windows path.

R/cpp_options.R

@@ -49,7 +49,8 @@ make_variable_name_pattern <- "[A-Za-z_][A-Za-z0-9_]*"
 #' Every entry must be named and every name must be a Make variable name. The
 #' names are uppercased here so that one spelling reaches everything downstream.
 #' The user header and the stanc flags have their own arguments, so setting them
-#' here is an error.
+#' here is an error. A TBB directory has to be a literal path, since the build
+#' record keeps it as written and never asks make what it expands to.
 #'
 #' @param cpp_options What the user passed, or `NULL`.
 #' @return The options with their names uppercased, or an empty list for
@@ -87,6 +88,19 @@ assert_valid_cpp_options <- function(cpp_options) {
   if ("STANCFLAGS" %in% names(cpp_options)) {
     stop(stancflags_cpp_option_message(), call. = FALSE)
   }
+  for (tbb_at in which(names(cpp_options) %in% c("TBB_LIB", "TBB_BIN"))) {
+    flags <- cpp_options_to_compile_flags(cpp_options[tbb_at])
+    bad <- grep("$", flags, fixed = TRUE, value = TRUE)
+    if (length(bad) > 0) {
+      stop(
+        "`", names(cpp_options)[[tbb_at]], "` must be a literal directory. ",
+        "cmdstanr records it to launch the model with the right TBB and ",
+        "doesn't expand make expressions like `", sub("^[^=]*=", "", bad[[1]]),
+        "`.",
+        call. = FALSE
+      )
+    }
+  }
   cpp_options
 }
 

R/model.R

@@ -1066,6 +1066,7 @@ sample <- function(data = NULL,
     standalone_env = private$standalone_functions(),
     model_name = self$model_name(),
     exe_file = self$exe_file(),
+    tbb_dir = private$record_$tbb_dir,
     proc_ids = checkmate::assert_integerish(chain_ids, lower = 1, len = chains, unique = TRUE, null.ok = FALSE),
     data_file = process_data(data, model_variables),
     save_latent_dynamics = save_latent_dynamics,
@@ -1225,6 +1226,7 @@ sample_mpi <- function(data = NULL,
     standalone_env = private$standalone_functions(),
     model_name = self$model_name(),
     exe_file = self$exe_file(),
+    tbb_dir = private$record_$tbb_dir,
     proc_ids = checkmate::assert_integerish(chain_ids, lower = 1, len = chains, unique = TRUE, null.ok = FALSE),
     data_file = process_data(data, model_variables),
     save_latent_dynamics = save_latent_dynamics,
@@ -1360,6 +1362,7 @@ optimize <- function(data = NULL,
     standalone_env = private$standalone_functions(),
     model_name = self$model_name(),
     exe_file = self$exe_file(),
+    tbb_dir = private$record_$tbb_dir,
     proc_ids = 1,
     data_file = process_data(data, model_variables),
     save_latent_dynamics = FALSE,
@@ -1532,6 +1535,7 @@ laplace <- function(data = NULL,
     standalone_env = private$standalone_functions(),
     model_name = self$model_name(),
     exe_file = self$exe_file(),
+    tbb_dir = private$record_$tbb_dir,
     proc_ids = 1,
     data_file = process_data(data, model_variables),
     save_latent_dynamics = FALSE,
@@ -1664,6 +1668,7 @@ variational <- function(data = NULL,
     standalone_env = private$standalone_functions(),
     model_name = self$model_name(),
     exe_file = self$exe_file(),
+    tbb_dir = private$record_$tbb_dir,
     proc_ids = 1,
     data_file = process_data(data, model_variables),
     save_latent_dynamics = save_latent_dynamics,
@@ -1846,6 +1851,7 @@ pathfinder <- function(data = NULL,
     standalone_env = private$standalone_functions(),
     model_name = self$model_name(),
     exe_file = self$exe_file(),
+    tbb_dir = private$record_$tbb_dir,
     proc_ids = 1,
     data_file = process_data(data, model_variables),
     save_latent_dynamics = FALSE,
@@ -1983,6 +1989,7 @@ generate_quantities <- function(fitted_params,
     standalone_env = private$standalone_functions(),
     model_name = self$model_name(),
     exe_file = self$exe_file(),
+    tbb_dir = private$record_$tbb_dir,
     proc_ids = seq_along(fitted_params_files),
     data_file = process_data(data, model_variables),
     seed = seed,
@@ -2055,6 +2062,7 @@ diagnose <- function(data = NULL,
     standalone_env = private$standalone_functions(),
     model_name = self$model_name(),
     exe_file = self$exe_file(),
+    tbb_dir = private$record_$tbb_dir,
     proc_ids = 1,
     data_file = process_data(data, model_variables),
     seed = seed,
@@ -2160,7 +2168,9 @@ cmdstan_defaults <- function(method = c("sample", "optimize", "variational",
                                         "pathfinder", "laplace")) {
   method <- match.arg(method)
   private$assert_current()
-  parse_cmdstan_args(self$exe_file(), method, self$stan_file())
+  parse_cmdstan_args(
+    self$exe_file(), method, self$stan_file(), private$record_$tbb_dir
+  )
 }
 CmdStanModel$set("public", name = "cmdstan_defaults", value = cmdstan_defaults)
 
@@ -2289,13 +2299,14 @@ assert_stan_file_exists <- function(stan_file) {
 #'   `"variational"`, `"pathfinder"`, or `"laplace"`.
 #' @param stan_file The model's Stan file, for the error when the binary
 #'   will not run.
+#' @param tbb_dir The record's `tbb_dir`, or `NULL` without a usable record.
 #' @return A named list with cmdstanr-style argument names and default
 #'   values.
-parse_cmdstan_args <- function(model_binary, method, stan_file) {
+parse_cmdstan_args <- function(model_binary, method, stan_file, tbb_dir) {
   withr::with_path(
     c(
       toolchain_PATH_env_var(),
-      tbb_path()
+      tbb_launch_path(tbb_dir)
     ),
     ret <- tryCatch(
       wsl_compatible_run(
@@ -2304,7 +2315,7 @@ parse_cmdstan_args <- function(model_binary, method, stan_file) {
         error_on_status = FALSE
       ),
       error = function(e) {
-        stop_cannot_run(model_binary, stan_file, conditionMessage(e))
+        stop_cannot_run(model_binary, stan_file, conditionMessage(e), tbb_dir)
       }
     )
   )
@@ -2314,7 +2325,8 @@ parse_cmdstan_args <- function(model_binary, method, stan_file) {
     output <- trimws(paste0(ret$stderr, ret$stdout))
     stop_cannot_run(
       model_binary, stan_file,
-      if (nzchar(output)) output else paste("exit status", ret$status)
+      if (nzchar(output)) output else paste("exit status", ret$status),
+      tbb_dir
     )
   }
   # CmdStan may write help text to stdout or stderr depending on the platform

R/path.R

@@ -441,3 +441,28 @@ tbb_path <- function(dir = NULL) {
   }
   path_to_TBB
 }
+
+#' The TBB directory to put on PATH when launching a model executable
+#'
+#' On Windows there's no rpath, so the executable looks for tbb.dll on PATH and
+#' we need to put a TBB directory there. We use the one from the build record,
+#' i.e., the TBB the model was actually built against, even if the user has
+#' selected a different CmdStan installation since then. If that directory no
+#' longer exists we don't add anything, in which case the executable either uses
+#' whatever TBB is already on PATH or fails to load if there isn't one. If
+#' there's no usable build record we fall back to the selected installation's
+#' TBB, which is what we always did before.
+#'
+#' @param tbb_dir The record's `tbb_dir`, or `NULL` when there is no usable
+#'   record.
+#' @return The directory, or `NULL` when nothing should go on PATH.
+#' @noRd
+tbb_launch_path <- function(tbb_dir) {
+  if (!os_is_windows()) {
+    return(NULL)
+  }
+  if (is.null(tbb_dir)) {
+    return(tbb_path())
+  }
+  if (dir.exists(tbb_dir)) tbb_dir else NULL
+}