Merge pull request #1226 from stan-dev/bugfix-issue-820

Fix include paths with spaces and resolve them at model creation

Author: jgabry

NEWS.md

@@ -15,6 +15,13 @@ variables are, instead of erroring. (#1225)
 which previously errored. (#1225)
 * The `CMDSTANR_NO_VER_CHECK` R option and environment variable are deprecated 
 as of CmdStanR 1.0.0; use the lowercase `cmdstanr_no_ver_check` forms instead.
+* CmdStanModel methods now correctly handle `#include` directories with spaces
+in their paths. (#820)
+* `$include_paths()` now returns absolute paths, and relative include paths are
+resolved when the model object is created or `$compile()` is called rather than
+on each `stanc` call. Previously a model created from a relative path could
+resolve `#include` directives against the wrong directory if the working
+directory changed. (#1229)
 * `$cpp_options()` no longer includes a `STAN_VERSION` entry read from the model 
 executable's metadata. It was never a C++ option; use `$cmdstan_version()` instead. (#1215)
 * CmdStanModel methods now use executable metadata regardless of the 

R/model.R

@@ -259,7 +259,7 @@ CmdStanModel <- R6::R6Class(
       if (!is.null(stan_file)) {
         assert_file_exists(stan_file, access = "r", extension = c("stan", "stanfunctions"))
         checkmate::assert_flag(compile)
-        private$stan_file_ <- absolute_path(stan_file)
+        private$stan_file_ <- resolve_path(stan_file)
         private$stan_code_ <- readLines(stan_file)
         private$model_name_ <- gsub(" ", "_", strip_ext(basename(private$stan_file_)))
         private$precompile_cpp_options_ <- args$cpp_options %||% list()
@@ -269,20 +269,20 @@ CmdStanModel <- R6::R6Class(
           private$using_user_header_ <- TRUE
         }
         if (is.null(args$include_paths) && any(grepl("#include" , private$stan_code_))) {
-          private$precompile_include_paths_ <- dirname(stan_file)
+          private$precompile_include_paths_ <- dirname(private$stan_file_)
         } else {
-          private$precompile_include_paths_ <- args$include_paths
+          private$precompile_include_paths_ <- resolve_path(args$include_paths)
         }
       }
       if (!is.null(exe_file)) {
         ext <- if (os_is_windows() && !os_is_wsl()) "exe" else ""
-        private$exe_file_ <- repair_path(absolute_path(exe_file))
+        private$exe_file_ <- resolve_path(exe_file)
         if (is.null(stan_file)) {
           assert_file_exists(private$exe_file_, access = "r", extension = ext)
           private$model_name_ <- gsub(" ", "_", strip_ext(basename(private$exe_file_)))
         }
         private$include_paths_ <-
-          private$precompile_include_paths_ %||% args$include_paths
+          private$precompile_include_paths_ %||% resolve_path(args$include_paths)
       }
       if (!is.null(stan_file) && compile) {
         self$compile(...)
@@ -422,7 +422,7 @@ CmdStanModel <- R6::R6Class(
 #' * `$model_name()` returns the model name as a string.
 #' * `$exe_file()` returns a path as a string, or `character(0)` if no
 #'   executable path is set.
-#' * `$include_paths()` returns a character vector of paths or `NULL`.
+#' * `$include_paths()` returns a character vector of absolute paths or `NULL`.
 #' * `$cmdstan_version()` returns a CmdStan version as a string.
 #' * `$cpp_options()` returns a named list of C++ options.
 #' * `$hpp_file()` returns the path to the `.hpp` file as a string when C++ code
@@ -480,7 +480,10 @@ NULL
 #'   [`$check_syntax()`][model-method-check_syntax] method can be used instead.
 #' @param include_paths (character vector) Paths to directories where Stan
 #'   should look for files specified in `#include` directives in the Stan
-#'   program.
+#'   program. Relative paths are resolved against the working directory when
+#'   the model object is created (or when `$compile()` is called) and stored as
+#'   absolute paths, so subsequent changes to the working directory do not
+#'   affect them.
 #' @param user_header (string) The path to a C++ file (with a .hpp extension)
 #'   to compile with the Stan model.
 #' @param cpp_options (list) Any makefile options to be used when compiling the
@@ -594,7 +597,7 @@ compile <- function(quiet = TRUE,
   if (is.null(include_paths) && !is.null(private$precompile_include_paths_)) {
     include_paths <- private$precompile_include_paths_
   }
-  private$include_paths_ <- include_paths
+  private$include_paths_ <- resolve_path(include_paths)
   if (is.null(dir) && !is.null(private$dir_)) {
     dir <- absolute_path(private$dir_)
   } else if (!is.null(dir)) {
@@ -724,7 +727,7 @@ compile <- function(quiet = TRUE,
   if (length(stancflags_local) > 0) {
     stancflags_combined <- c(stancflags_combined, stancflags_local)
   }
-  stanc_inc_paths <- include_paths_stanc3_args(include_paths, standalone_call = TRUE)
+  stanc_inc_paths <- include_paths_stanc3_args(include_paths, direct_call = TRUE)
   stancflags_standalone <- c("--standalone-functions", stanc_inc_paths, stancflags_combined)
   self$functions$hpp_code <- get_standalone_hpp(temp_stan_file, stancflags_standalone)
   private$model_methods_env_ <- new.env()
@@ -980,7 +983,10 @@ check_syntax <- function(pedantic = FALSE,
     stanc_options[["warn-pedantic"]] <- TRUE
   }
 
-  stancflags_val <- include_paths_stanc3_args(include_paths)
+  stancflags_val <- include_paths_stanc3_args(
+    include_paths,
+    direct_call = TRUE
+  )
 
   if (is.null(stanc_options[["name"]])) {
     stanc_options[["name"]] <- paste0(self$model_name(), "_model")
@@ -1106,7 +1112,10 @@ format <- function(overwrite_file = FALSE,
     lower = 1, len = 1, null.ok = TRUE
   )
   stanc_options <- private$precompile_stanc_options_
-  stancflags_val <- include_paths_stanc3_args(self$include_paths())
+  stancflags_val <- include_paths_stanc3_args(
+    self$include_paths(),
+    direct_call = TRUE
+  )
   stanc_options["auto-format"] <- TRUE
   if (!is.null(max_line_length)) {
     stanc_options["max-line-length"] <- max_line_length
@@ -2462,19 +2471,34 @@ assert_stan_file_exists <- function(stan_file) {
   }
 }
 
-include_paths_stanc3_args <- function(include_paths = NULL, standalone_call = FALSE) {
+#' Build stanc include-path arguments
+#'
+#' Make receives include paths through `STANCFLAGS` and needs paths containing
+#' spaces to be shell-quoted within a single `--include-paths=` flag. Direct
+#' calls through processx instead need the flag and comma-separated paths as
+#' separate, unquoted arguments.
+#'
+#' @param include_paths A character vector of directories containing files used
+#'   in Stan `#include` directives, or `NULL`.
+#' @param direct_call A logical indicating whether the arguments will be passed
+#'   directly to stanc through processx instead of through Make.
+#'
+#' @return `NULL` if `include_paths` is `NULL`; otherwise, a single
+#'   `--include-paths=` argument for Make or two arguments for a direct call.
+#' @noRd
+include_paths_stanc3_args <- function(include_paths = NULL, direct_call = FALSE) {
   stancflags <- NULL
   if (!is.null(include_paths)) {
     assert_dir_exists(include_paths, access = "r")
     include_paths <- sapply(absolute_path(include_paths), wsl_safe_path)
     # Calling stanc3 directly through processx::run does not need quoting
-    if (!isTRUE(standalone_call)) {
+    if (!isTRUE(direct_call)) {
       paths_w_space <- grep(" ", include_paths)
       include_paths[paths_w_space] <- paste0("'", include_paths[paths_w_space], "'")
     }
     include_paths <- paste0(include_paths, collapse = ",")
     include_paths_flag <- "--include-paths="
-    if (isTRUE(standalone_call)) {
+    if (isTRUE(direct_call)) {
       stancflags <- c(stancflags, "--include-paths", include_paths)
     } else {
       stancflags <- paste0(stancflags, include_paths_flag, include_paths)
@@ -2494,7 +2518,10 @@ model_variables <- function(stan_file, include_paths = NULL, allow_undefined = F
     command = stanc_cmd(),
     args = c(wsl_safe_path(stan_file),
               "--info",
-              include_paths_stanc3_args(include_paths),
+              include_paths_stanc3_args(
+                include_paths,
+                direct_call = TRUE
+              ),
               allow_undefined_arg),
     wd = cmdstan_path(),
     echo = FALSE,

R/utils.R

@@ -194,6 +194,17 @@ strip_ext <- function(file) {
 }
 absolute_path <- Vectorize(.absolute_path, USE.NAMES = FALSE)
 
+# Resolve paths when they are stored on a model object rather than when they are
+# used, so that later use doesn't depend on the working directory. Empty input
+# becomes NULL, the value callers treat as "not set" (absolute_path() would
+# otherwise return list()).
+resolve_path <- function(path) {
+  if (!length(path)) {
+    return(NULL)
+  }
+  repair_path(absolute_path(path))
+}
+
 # read, write, and copy files --------------------------------------------
 
 #' Copy temporary files (e.g., output, data) to a different location

man/model-method-compile.Rd

@@ -9,6 +9,24 @@ testing_stan_file <- function(name) {
   test_path("resources", "stan", paste0(name, ".stan"))
 }
 
+local_include_model_with_spaces <- function(.local_envir = parent.frame()) {
+  model_dir <- withr::local_tempdir(
+    pattern = "include path",
+    .local_envir = .local_envir
+  )
+  source_files <- c(
+    testing_stan_file("bernoulli_include"),
+    testing_stan_file("divide_real_by_two")
+  )
+  if (!all(file.copy(source_files, model_dir))) {
+    stop("Failed to copy Stan include test fixtures.", call. = FALSE)
+  }
+  list(
+    stan_file = file.path(model_dir, "bernoulli_include.stan"),
+    include_paths = model_dir
+  )
+}
+
 cmdstan_example_file <- function() {
   # stan program in different directory from the others
   file.path(cmdstan_path(), "examples", "bernoulli", "bernoulli.stan")

man/model-method-model-info.Rd

@@ -480,19 +480,18 @@ test_that("draws are returned for model with spaces", {
   expect_equal(dim(fit$draws()), c(1000, 1, 1))
 })
 
-test_that("sampling with inits works with include_paths", {
-  stan_program_w_include <- testing_stan_file("bernoulli_include")
-  exe <- cmdstan_ext(strip_ext(stan_program_w_include))
-  if (file.exists(exe)) {
-    file.remove(exe)
-  }
+test_that("sampling works with explicit and inferred include paths containing spaces", {
+  include_model <- local_include_model_with_spaces()
 
-  mod_w_include <- cmdstan_model(stan_file = stan_program_w_include,
-                                 include_paths = test_path("resources", "stan"))
+  mod_inferred <- cmdstan_model(stan_file = include_model$stan_file)
+  expect_equal(
+    repair_path(mod_inferred$include_paths()),
+    repair_path(include_model$include_paths)
+  )
 
   data_list <- list(N = 10, y = c(0,1,0,0,0,0,0,0,0,1))
   expect_no_error(utils::capture.output(
-    fit <- mod_w_include$sample(
+    fit <- mod_inferred$sample(
       data = data_list,
       seed = 123,
       chains = 4,
@@ -504,6 +503,26 @@ test_that("sampling with inits works with include_paths", {
                   list(theta = 0.25))
     )
   ))
+
+  mod_explicit <- cmdstan_model(
+    stan_file = include_model$stan_file,
+    exe_file = mod_inferred$exe_file(),
+    include_paths = include_model$include_paths,
+    compile = FALSE
+  )
+  expect_equal(
+    repair_path(mod_explicit$include_paths()),
+    repair_path(include_model$include_paths)
+  )
+  expect_no_error(utils::capture.output(
+    mod_explicit$sample(
+      data = data_list,
+      seed = 123,
+      chains = 1,
+      refresh = 500,
+      init = list(list(theta = 0.25))
+    )
+  ))
 })
 
 test_that("CmdStanModel created with exe_file works", {