From 5681ba589743b486d588fe8a44f90da3262a47f4 Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 11:14:57 -0600 Subject: [PATCH 01/34] Refresh source-derived model state on recompilation A CmdStanModel kept describing the program it was created from after the Stan file was edited and the same object recompiled. private$stan_code_ was read once in initialize() and only ever refreshed by $format(overwrite_file = TRUE), and private$variables_ was populated lazily by $variables() and never invalidated. This was not only cosmetic: the fitting methods pass self$variables() into the data and init checks, so a recompiled model validated against the old parameter set and warned about parameters that no longer existed. Two adjacent pieces of state had the same problem. self$functions had its hpp_code overwritten before the make call while the compiled flag, the function names and the old Rcpp bindings survived, so expose_stan_functions() short-circuited on compiled and kept serving the previous implementations. private$using_user_header_ was only ever set to TRUE, before compilation ran, and never reset. Make successful replacement of the executable the synchronization point. Everything derived from the Stan program is now committed in one block after the exe copy: the code snapshot is taken from the temp file that was actually compiled, variables_ is cleared so the next $variables() reparses lazily, using_user_header_ is set from the arguments resolved for this compilation in both directions, and the functions environment is emptied in place and repopulated. Clearing it in place preserves its identity, and existing fit objects are unaffected because CmdStanFit copies the contents into its own environment at construction. The standalone hpp and the external/existing_exe values are assigned to locals instead of being written into self$functions early, and the compile_standalone exposure moves from before the make call to after the commit block. That is what makes a failed compilation atomic: a dry run, a stanc failure or a C++ failure now all leave the previously compiled state untouched. Two consequences. $compile(dry_run = TRUE) no longer writes anything into self$functions. After a real recompilation with compile_standalone = FALSE previously exposed functions are gone and must be exposed again. fixes #1228 --- NEWS.md | 5 ++ R/model.R | 30 +++++++---- tests/testthat/test-model-code-print.R | 11 +++- .../testthat/test-model-compile-user_header.R | 30 +++++++++++ tests/testthat/test-model-compile.R | 52 +++++++++++++++++++ tests/testthat/test-model-expose-functions.R | 49 +++++++++++++++++ tests/testthat/test-model-variables.R | 51 ++++++++++++++++++ 7 files changed, 216 insertions(+), 12 deletions(-) diff --git a/NEWS.md b/NEWS.md index 21b4e6494..f70cd6a23 100644 --- a/NEWS.md +++ b/NEWS.md @@ -26,6 +26,11 @@ strings were also passed to `stanc` directly, which rejected them. (#1227) path. (#1227) * Numeric `stanc_options` values such as `list("max-line-length" = 78)` are no longer dropped. (#1233) +* `$compile()` now refreshes `$code()` and `$variables()` after a successful +compilation. (#1228) +* `$compile()` now discards standalone functions exposed from an earlier +version of the Stan program. They must be exposed again with +`$expose_functions()` after a recompilation. (#1228) * CmdStanModel methods now correctly handle `#include` directories with spaces in their paths. (#820) * `$include_paths()` now returns absolute paths, and relative include paths are diff --git a/R/model.R b/R/model.R index 876590422..8e631d13c 100644 --- a/R/model.R +++ b/R/model.R @@ -631,7 +631,6 @@ compile <- function(quiet = TRUE, } cpp_options[["USER_HEADER"]] <- wsl_safe_path(absolute_path(user_header)) - private$using_user_header_ <- TRUE } else if (!is.null(cpp_options[["USER_HEADER"]])) { if (!is.null(cpp_options[["user_header"]])) { warning('User header specified both via cpp_options[["USER_HEADER"]] and cpp_options[["user_header"]].', call. = FALSE) @@ -639,15 +638,14 @@ compile <- function(quiet = TRUE, user_header <- cpp_options[["USER_HEADER"]] cpp_options[["USER_HEADER"]] <- wsl_safe_path(absolute_path(cpp_options[["USER_HEADER"]])) - private$using_user_header_ <- TRUE } else if (!is.null(cpp_options[["user_header"]])) { user_header <- cpp_options[["user_header"]] cpp_options[["user_header"]] <- wsl_safe_path(absolute_path(cpp_options[["user_header"]])) - private$using_user_header_ <- TRUE } - if (!is.null(user_header)) { + using_user_header <- !is.null(user_header) + if (using_user_header) { stanc_options[["allow-undefined"]] <- TRUE user_header <- absolute_path(user_header) # As mentioned above, just absolute, not wsl_safe_path() if (!file.exists(user_header)) { @@ -719,20 +717,14 @@ compile <- function(quiet = TRUE, } stanc_inc_paths <- include_paths_stanc3_args(include_paths, direct_call = TRUE) stancflags_standalone <- c("--standalone-functions", stanc_inc_paths, stancflags_direct) - self$functions$hpp_code <- get_standalone_hpp(temp_stan_file, stancflags_standalone) + standalone_hpp_code <- get_standalone_hpp(temp_stan_file, stancflags_standalone) private$model_methods_env_ <- new.env() private$model_methods_env_$hpp_code_ <- get_standalone_hpp(temp_stan_file, c(stanc_inc_paths, stancflags_direct)) - self$functions$external <- !is.null(user_header) - self$functions$existing_exe <- FALSE stancflags_val <- paste0("STANCFLAGS += ", stancflags_val, paste0(" ", stancflags_combined, collapse = " ")) if (!dry_run) { - if (compile_standalone) { - expose_stan_functions(self$functions, verbose = !quiet) - } - withr::with_envvar( c("HOME" = short_path(Sys.getenv("HOME"))), withr::with_path( @@ -801,6 +793,22 @@ compile <- function(quiet = TRUE, ) } + # The new executable is in place, so everything derived from the Stan + # program that was just compiled can be committed. A dry run or a failed + # compilation leaves the previously compiled state untouched. (#1228) + rm(list = ls(self$functions, all.names = TRUE), envir = self$functions) + self$functions$compiled <- FALSE + self$functions$hpp_code <- standalone_hpp_code + self$functions$external <- using_user_header + self$functions$existing_exe <- FALSE + private$stan_code_ <- readLines(temp_stan_file) + private$variables_ <- NULL + private$using_user_header_ <- using_user_header + + if (compile_standalone) { + expose_stan_functions(self$functions, verbose = !quiet) + } + writeLines(private$model_methods_env_$hpp_code_, con = wsl_safe_path(private$hpp_file_, revert = TRUE)) } # End - if(!dry_run) diff --git a/tests/testthat/test-model-code-print.R b/tests/testthat/test-model-code-print.R index 816218937..9b4908523 100644 --- a/tests/testthat/test-model-code-print.R +++ b/tests/testthat/test-model-code-print.R @@ -23,7 +23,7 @@ test_that("code() and print() still work if file is removed", { expect_identical(mod_removed_stan_file$code(), code_answer) }) -test_that("code() doesn't change when file changes (unless model is recreated)", { +test_that("code() doesn't change when file changes (unless recompiled or recreated)", { code_1 <- " parameters { real y; @@ -52,11 +52,20 @@ test_that("code() doesn't change when file changes (unless model is recreated)", # overwrite with new code, but mod$code() shouldn't change file.copy(stan_file_2, stan_file_1, overwrite = TRUE) expect_identical(mod$code(), code_1_answer) + expect_identical(utils::capture.output(mod$print()), code_1_answer) # recreate CmdStanModel object, now mod$code() should change mod <- cmdstan_model(stan_file_1, compile = FALSE) expect_identical(mod$code(), code_2_answer) expect_identical(utils::capture.output(mod$print()), code_2_answer) + + # overwrite with the original code, mod$code() shouldn't change until the + # model is successfully recompiled (#1228) + writeLines(code_1_answer, stan_file_1) + expect_identical(mod$code(), code_2_answer) + mod$compile() + expect_identical(mod$code(), code_1_answer) + expect_identical(utils::capture.output(mod$print()), code_1_answer) }) test_that("code() warns and print() errors if only exe and no Stan file", { diff --git a/tests/testthat/test-model-compile-user_header.R b/tests/testthat/test-model-compile-user_header.R index d86de0628..5a6512837 100644 --- a/tests/testthat/test-model-compile-user_header.R +++ b/tests/testthat/test-model-compile-user_header.R @@ -33,6 +33,36 @@ test_that("cpp_options user headers allow undefined functions", { ) }) +# Also above the file-level skip_if() below: the compiler is mocked, so this +# needs no toolchain either. +test_that("compile() commits the user header setting after compiling", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli_external.stan") + file.copy(testing_stan_file("bernoulli_external"), stan_file) + user_header <- withr::local_tempfile(lines = "", fileext = ".hpp") + local_mocked_bindings( + get_cmdstan_flags = function(flag_name) character(), + get_standalone_hpp = function(stan_file, stancflags) "" + ) + model <- cmdstan_model(stan_file, compile = FALSE) + expect_false(model$.__enclos_env__$private$using_user_header_) + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 0), + code = model$compile(user_header = user_header, force_recompile = TRUE) + ) + expect_true(model$.__enclos_env__$private$using_user_header_) + + # a bare recompile doesn't carry the user header over, so the setting is + # committed as FALSE, matching what was actually compiled + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 0), + code = model$compile(force_recompile = TRUE) + ) + expect_false(model$.__enclos_env__$private$using_user_header_) +}) + skip_if(os_is_macos()) w_path <- function(f) { diff --git a/tests/testthat/test-model-compile.R b/tests/testthat/test-model-compile.R index b06db28e0..c74366e70 100644 --- a/tests/testthat/test-model-compile.R +++ b/tests/testthat/test-model-compile.R @@ -304,6 +304,58 @@ test_that("compile() performs stanc checks during dry runs", { ) }) +test_that("compile() with dry_run = TRUE doesn't refresh cached model state", { + model_dir <- withr::local_tempdir() + stan_file <- write_stan_file( + "parameters { real alpha; } model { alpha ~ std_normal(); }", + dir = model_dir, + basename = "issue1228-dry-run.stan" + ) + model <- cmdstan_model(stan_file, compile = FALSE) + code_before <- model$code() + variables_before <- model$variables() + local_mocked_bindings( + get_cmdstan_flags = function(flag_name) character(), + get_standalone_hpp = function(stan_file, stancflags) "" + ) + + write_stan_file( + "parameters { real beta; } model { beta ~ std_normal(); }", + dir = model_dir, + basename = "issue1228-dry-run.stan" + ) + model$compile(force_recompile = TRUE, dry_run = TRUE) + + expect_identical(model$code(), code_before) + expect_identical(model$variables(), variables_before) + expect_equal(ls(model$functions), "compiled") + expect_false(model$functions$compiled) +}) + +test_that("a failed compile() doesn't refresh cached model state", { + model_dir <- withr::local_tempdir() + stan_file <- write_stan_file( + "parameters { real alpha; } model { alpha ~ std_normal(); }", + dir = model_dir, + basename = "issue1228-failed-compile.stan" + ) + model <- cmdstan_model(stan_file, compile = FALSE) + code_before <- model$code() + variables_before <- model$variables() + + file.copy(testing_stan_file("fail"), stan_file, overwrite = TRUE) + expect_error( + model$compile(force_recompile = TRUE), + "An error occurred during compilation!", + fixed = TRUE + ) + + expect_identical(model$code(), code_before) + expect_identical(model$variables(), variables_before) + expect_equal(ls(model$functions), "compiled") + expect_false(model$functions$compiled) +}) + test_that("dir arg works for cmdstan_model and $compile()", { tmp_dir <- tempdir() tmp_dir_2 <- tempdir() diff --git a/tests/testthat/test-model-expose-functions.R b/tests/testthat/test-model-expose-functions.R index 0cbc4f8ba..ab65e0653 100644 --- a/tests/testthat/test-model-expose-functions.R +++ b/tests/testthat/test-model-expose-functions.R @@ -311,6 +311,55 @@ test_that("Functions can be compiled with model", { ) }) +test_that("recompiling drops previously exposed functions", { + model_dir <- withr::local_tempdir() + write_model <- function(code) { + write_stan_file(code, dir = model_dir, basename = "issue1228.stan") + } + code_two_functions <- " + functions { + real times_two(real x) { return 2 * x; } + real times_three(real x) { return 3 * x; } + } + parameters { + real y; + } + model { + y ~ std_normal(); + } + " + stan_file <- write_model(code_two_functions) + mod <- cmdstan_model(stan_file) + mod$expose_functions() + expect_equal(mod$functions$times_two(1), 2) + expect_equal(mod$functions$times_three(1), 3) + + # change one function and remove the other, then recompile + write_model(" + functions { + real times_two(real x) { return 20 * x; } + } + parameters { + real y; + } + model { + y ~ std_normal(); + } + ") + mod$compile() + expect_false(mod$functions$compiled) + expect_false("times_three" %in% ls(mod$functions)) + mod$expose_functions() + expect_equal(mod$functions$times_two(1), 20) + expect_false("times_three" %in% ls(mod$functions)) + + # compile_standalone exposes the functions of the recompiled model + write_model(code_two_functions) + mod$compile(compile_standalone = TRUE) + expect_equal(mod$functions$times_two(1), 2) + expect_equal(mod$functions$times_three(1), 3) +}) + test_that("compile_standalone warns but doesn't error if no functions", { stan_no_funs_block <- write_stan_file(" parameters { diff --git a/tests/testthat/test-model-variables.R b/tests/testthat/test-model-variables.R index a87cf4c1a..4abb9a622 100644 --- a/tests/testthat/test-model-variables.R +++ b/tests/testthat/test-model-variables.R @@ -70,6 +70,57 @@ test_that("$variables() work correctly with multidimensional variables", { expect_equal(mod$variables()$transformed_parameters$pp$dimensions, 3) }) +test_that("$variables() is refreshed when the model is recompiled", { + model_dir <- withr::local_tempdir() + stan_file <- write_stan_file( + " + parameters { + real alpha; + } + model { + alpha ~ std_normal(); + } + ", + dir = model_dir, + basename = "issue1228.stan" + ) + mod <- cmdstan_model(stan_file) + expect_equal(names(mod$variables()$parameters), "alpha") + + write_stan_file( + " + parameters { + real beta; + } + model { + beta ~ std_normal(); + } + ", + dir = model_dir, + basename = "issue1228.stan" + ) + # editing the file alone doesn't invalidate the cached variables + expect_equal(names(mod$variables()$parameters), "alpha") + + # the edited file is newer than the executable, so this recompiles + mod$compile() + expect_equal(names(mod$variables()$parameters), "beta") + + # the fitting methods validate inits against the refreshed variables + expect_no_message( + utils::capture.output( + mod$sample( + chains = 1, + iter_warmup = 100, + iter_sampling = 100, + refresh = 0, + init = list(list(beta = 0)) + ) + ), + message = "Init values were only set for a subset of parameters" + ) +}) + test_that("$variables() errors on no stan_file", { code <- " parameters { From 3f2c33d97e2914b7091fecd6a6094fb54a90f145 Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 13:06:03 -0600 Subject: [PATCH 02/34] Reuse the include paths and user header on recompilation Compile-time inputs supplied to cmdstan_model() or $compile() were consumed by a single compilation and then forgotten: $compile() cleared the precompile_* fields at the end and nothing fed include_paths_ back in, so a second $compile() through the same object ran with no include paths and no user header. A model using #include directives or a user header could not be recompiled at all, and a header that overrides an existing definition rather than supplying an undeclared one produced a different executable with no error at all. Include paths and a user header are not build options, they are inputs the program needs in order to translate, so they now persist for the life of the model object and are replaced whenever new ones are supplied. cpp_options and stanc_options keep their one-shot behavior: a bare $compile() producing an unconfigured build is a tested workflow, and sticky stanc_options would leak values such as a stanc name= into every later compilation of the same object. $compile() now falls back to include_paths_ and then to precompile_include_paths_, and a fourth branch of the existing user header chain reuses the stored header when neither the argument nor a cpp_options entry is given. Putting it in that chain keeps the "specified both via" warnings from firing on a reused header. The header is committed with the rest of the compiled state, so a failed compilation does not record a header it never used. cmdstan_model() now stores the user_header argument. It was only passed through to $compile(), so with compile = FALSE it was lost entirely and even the first $compile() failed, while using_user_header_ still claimed the model had a header. The three precompile_* <- NULL assignments move inside if (!dry_run). Clearing them ran even when nothing had been compiled, which discarded the options given to cmdstan_model() and was also what kept a user header supplied through cpp_options from surviving a dry run. $include_paths() no longer gates on the executable existing. It returned NULL after $compile(dry_run = TRUE) or once the executable had been removed, and $variables(), $check_syntax() and $format() all read it. fixes #1234 --- NEWS.md | 11 ++++ R/model.R | 30 ++++++----- man/model-method-compile.Rd | 6 ++- .../testthat/test-model-compile-user_header.R | 50 ++++++++++++++++--- tests/testthat/test-model-compile.R | 47 +++++++++++++++++ 5 files changed, 123 insertions(+), 21 deletions(-) diff --git a/NEWS.md b/NEWS.md index f70cd6a23..bc5120f78 100644 --- a/NEWS.md +++ b/NEWS.md @@ -31,6 +31,17 @@ compilation. (#1228) * `$compile()` now discards standalone functions exposed from an earlier version of the Stan program. They must be exposed again with `$expose_functions()` after a recompilation. (#1228) +* `$compile()` now reuses the include paths and the user header of the previous +compilation when they are not supplied again. Recompiling a model that uses +`#include` directives or a user header through the same object previously +failed because those inputs were dropped. (#1234) +* A `user_header` supplied to `cmdstan_model()` is now used by a later +`$compile()`. Previously it was only honored when the model was compiled +immediately. (#1234) +* `$compile(dry_run = TRUE)` no longer discards the `cpp_options`, +`stanc_options` and `include_paths` supplied to `cmdstan_model()`. (#1234) +* `$include_paths()` no longer returns `NULL` for a model whose executable does +not exist, such as after `$compile(dry_run = TRUE)`. (#1234) * CmdStanModel methods now correctly handle `#include` directories with spaces in their paths. (#820) * `$include_paths()` now returns absolute paths, and relative include paths are diff --git a/R/model.R b/R/model.R index 8e631d13c..fe94abb0b 100644 --- a/R/model.R +++ b/R/model.R @@ -242,6 +242,7 @@ CmdStanModel <- R6::R6Class( cpp_options_ = list(), stanc_options_ = list(), include_paths_ = NULL, + user_header_ = NULL, using_user_header_ = FALSE, precompile_cpp_options_ = NULL, precompile_stanc_options_ = NULL, @@ -268,6 +269,7 @@ CmdStanModel <- R6::R6Class( !is.null(args$cpp_options[["user_header"]])) { private$using_user_header_ <- TRUE } + private$user_header_ <- args$user_header if (is.null(args$include_paths) && any(grepl("#include" , private$stan_code_))) { private$precompile_include_paths_ <- dirname(private$stan_file_) } else { @@ -305,11 +307,7 @@ CmdStanModel <- R6::R6Class( invisible(self) }, include_paths = function() { - if (length(self$exe_file()) > 0 && file.exists(self$exe_file())) { - return(private$include_paths_) - } else { - return(private$precompile_include_paths_) - } + private$include_paths_ %||% private$precompile_include_paths_ }, code = function() { if (length(private$stan_code_) == 0) { @@ -483,9 +481,11 @@ NULL #' 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. +#' affect them. If `$compile()` is called again without `include_paths`, the +#' paths used for the previous compilation are reused. #' @param user_header (string) The path to a C++ file (with a .hpp extension) -#' to compile with the Stan model. +#' to compile with the Stan model. If `$compile()` is called again without +#' `user_header`, the header used for the previous compilation is reused. #' @param cpp_options (list) Any makefile options to be used when compiling the #' model (`stan_threads`, `stan_mpi`, `stan_opencl`, etc.). Anything you would #' otherwise write in the `make/local` file. For an example of using threading @@ -594,8 +594,8 @@ compile <- function(quiet = TRUE, stanc_options <- private$precompile_stanc_options_ } stanc_options <- assert_valid_stanc_options(stanc_options) - if (is.null(include_paths) && !is.null(private$precompile_include_paths_)) { - include_paths <- private$precompile_include_paths_ + if (is.null(include_paths)) { + include_paths <- private$include_paths_ %||% private$precompile_include_paths_ } private$include_paths_ <- resolve_path(include_paths) include_paths <- private$include_paths_ @@ -641,6 +641,11 @@ compile <- function(quiet = TRUE, } else if (!is.null(cpp_options[["user_header"]])) { user_header <- cpp_options[["user_header"]] cpp_options[["user_header"]] <- wsl_safe_path(absolute_path(cpp_options[["user_header"]])) + } else if (!is.null(private$user_header_)) { + # the header the model was last compiled with, or the one supplied to + # cmdstan_model() if it has not been compiled yet + user_header <- private$user_header_ + cpp_options[["USER_HEADER"]] <- wsl_safe_path(absolute_path(user_header)) } @@ -804,6 +809,10 @@ compile <- function(quiet = TRUE, private$stan_code_ <- readLines(temp_stan_file) private$variables_ <- NULL private$using_user_header_ <- using_user_header + private$user_header_ <- user_header + private$precompile_cpp_options_ <- NULL + private$precompile_stanc_options_ <- NULL + private$precompile_include_paths_ <- NULL if (compile_standalone) { expose_stan_functions(self$functions, verbose = !quiet) @@ -816,9 +825,6 @@ compile <- function(quiet = TRUE, private$cmdstan_version_ <- cmdstan_version() private$exe_file_ <- exe private$cpp_options_ <- cpp_options - private$precompile_cpp_options_ <- NULL - private$precompile_stanc_options_ <- NULL - private$precompile_include_paths_ <- NULL if (!dry_run) { if (compile_model_methods) { diff --git a/man/model-method-compile.Rd b/man/model-method-compile.Rd index 83ca7ae92..030831b07 100644 --- a/man/model-method-compile.Rd +++ b/man/model-method-compile.Rd @@ -40,10 +40,12 @@ should look for files specified in \verb{#include} directives in the Stan program. Relative paths are resolved against the working directory when the model object is created (or when \verb{$compile()} is called) and stored as absolute paths, so subsequent changes to the working directory do not -affect them.} +affect them. If \verb{$compile()} is called again without \code{include_paths}, the +paths used for the previous compilation are reused.} \item{user_header}{(string) The path to a C++ file (with a .hpp extension) -to compile with the Stan model.} +to compile with the Stan model. If \verb{$compile()} is called again without +\code{user_header}, the header used for the previous compilation is reused.} \item{cpp_options}{(list) Any makefile options to be used when compiling the model (\code{stan_threads}, \code{stan_mpi}, \code{stan_opencl}, etc.). Anything you would diff --git a/tests/testthat/test-model-compile-user_header.R b/tests/testthat/test-model-compile-user_header.R index 5a6512837..fbe12ada8 100644 --- a/tests/testthat/test-model-compile-user_header.R +++ b/tests/testthat/test-model-compile-user_header.R @@ -33,15 +33,19 @@ test_that("cpp_options user headers allow undefined functions", { ) }) -# Also above the file-level skip_if() below: the compiler is mocked, so this -# needs no toolchain either. -test_that("compile() commits the user header setting after compiling", { +# Also above the file-level skip_if() below: the compiler is mocked, so these +# need no toolchain either. +test_that("compile() reuses the user header from the previous compilation", { stan_file <- file.path(withr::local_tempdir(), "bernoulli_external.stan") file.copy(testing_stan_file("bernoulli_external"), stan_file) user_header <- withr::local_tempfile(lines = "", fileext = ".hpp") + received_stancflags <- list() local_mocked_bindings( get_cmdstan_flags = function(flag_name) character(), - get_standalone_hpp = function(stan_file, stancflags) "" + get_standalone_hpp = function(stan_file, stancflags) { + received_stancflags <<- append(received_stancflags, list(stancflags)) + "" + } ) model <- cmdstan_model(stan_file, compile = FALSE) expect_false(model$.__enclos_env__$private$using_user_header_) @@ -53,14 +57,46 @@ test_that("compile() commits the user header setting after compiling", { ) expect_true(model$.__enclos_env__$private$using_user_header_) - # a bare recompile doesn't carry the user header over, so the setting is - # committed as FALSE, matching what was actually compiled + received_stancflags <- list() with_mocked_cli( compile_ret = list(status = 0), info_ret = list(status = 0), code = model$compile(force_recompile = TRUE) ) - expect_false(model$.__enclos_env__$private$using_user_header_) + expect_true(model$.__enclos_env__$private$using_user_header_) + expect_equal( + model$cpp_options()[["USER_HEADER"]], + wsl_safe_path(absolute_path(user_header)) + ) + expect_equal( + vapply(received_stancflags, function(x) "--allow-undefined" %in% x, logical(1)), + rep(TRUE, 2) + ) +}) + +test_that("compile() uses a user header supplied to cmdstan_model()", { + stan_file <- testing_stan_file("bernoulli_external") + user_header <- withr::local_tempfile(lines = "", fileext = ".hpp") + received_stancflags <- list() + local_mocked_bindings( + get_cmdstan_flags = function(flag_name) character(), + get_standalone_hpp = function(stan_file, stancflags) { + received_stancflags <<- append(received_stancflags, list(stancflags)) + "" + } + ) + + model <- cmdstan_model(stan_file, user_header = user_header, compile = FALSE) + model$compile(force_recompile = TRUE, dry_run = TRUE) + + expect_equal( + model$cpp_options()[["USER_HEADER"]], + wsl_safe_path(absolute_path(user_header)) + ) + expect_equal( + vapply(received_stancflags, function(x) "--allow-undefined" %in% x, logical(1)), + rep(TRUE, 2) + ) }) skip_if(os_is_macos()) diff --git a/tests/testthat/test-model-compile.R b/tests/testthat/test-model-compile.R index c74366e70..7de3cf76a 100644 --- a/tests/testthat/test-model-compile.R +++ b/tests/testthat/test-model-compile.R @@ -220,6 +220,53 @@ test_that("relative include_paths given to $compile() are resolved when it is ca expect_true(mod$check_syntax(quiet = TRUE)) }) +test_that("$compile() reuses include paths from the previous compilation", { + model_dir <- withr::local_tempdir() + include_dir <- file.path(model_dir, "includes") + dir.create(include_dir) + file.copy(testing_stan_file("bernoulli_include"), model_dir) + file.copy(testing_stan_file("divide_real_by_two"), include_dir) + + mod <- cmdstan_model( + file.path(model_dir, "bernoulli_include.stan"), + include_paths = include_dir, + compile = FALSE + ) + mod$compile(dry_run = TRUE, quiet = TRUE) + + # the include path isn't supplied again, but the included file is still found + expect_no_error(mod$compile(force_recompile = TRUE, dry_run = TRUE, quiet = TRUE)) + expect_equal(mod$include_paths(), resolve_path(include_dir)) +}) + +test_that("$compile() doesn't reuse cpp and stanc options from the previous compilation", { + stan_file <- testing_stan_file("bernoulli") + model <- cmdstan_model(stan_file, compile = FALSE) + received_stancflags <- list() + local_mocked_bindings( + get_cmdstan_flags = function(flag_name) character(), + get_standalone_hpp = function(stan_file, stancflags) { + received_stancflags <<- append(received_stancflags, list(stancflags)) + "" + } + ) + + model$compile( + cpp_options = list(stan_threads = TRUE), + stanc_options = list("warn-pedantic" = TRUE), + force_recompile = TRUE, + dry_run = TRUE + ) + received_stancflags <- list() + model$compile(force_recompile = TRUE, dry_run = TRUE) + + expect_null(model$cpp_options()[["stan_threads"]]) + expect_equal( + vapply(received_stancflags, function(x) "--warn-pedantic" %in% x, logical(1)), + rep(FALSE, 2) + ) +}) + test_that("name in STANCFLAGS is set correctly", { local_reproducible_output() out <- utils::capture.output(mod$compile(quiet = FALSE, force_recompile = TRUE)) From 2e3325593dfa9318f4a6f5d8702dcb3cb4a67acf Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 13:22:10 -0600 Subject: [PATCH 03/34] Allow undefined functions in check_syntax() and format() $check_syntax() and $format() build their stanc arguments from precompile_stanc_options_ and never consulted using_user_header_, so a model with a function that is declared in the Stan program and defined in a user header was reported as a syntax error. $compile() derives --allow-undefined from the resolved user header and $variables() derives it from using_user_header_; these two methods were the only ones that did not. The failure does not depend on the model having been compiled: it happens on a model created with compile = FALSE as well, so it is not a consequence of the compile-time options being consumed once. --- NEWS.md | 8 +++----- R/model.R | 6 ++++++ tests/testthat/test-model-compile.R | 12 ++++++++++++ 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/NEWS.md b/NEWS.md index bc5120f78..a9ab6f108 100644 --- a/NEWS.md +++ b/NEWS.md @@ -19,7 +19,9 @@ as of CmdStanR 1.0.0; use the lowercase `cmdstanr_no_ver_check` forms instead. `canonicalize`. The values were shell-quoted for Make and the same quoted strings were also passed to `stanc` directly, which rejected them. (#1227) * `$compile()` now enables `allow-undefined` for user headers supplied through -`cpp_options`, not just through the `user_header` argument. (#1227) +`cpp_options`, not just through the `user_header` argument. `$check_syntax()` +and `$format()` also now correctly enable `allow-undefined` for models that use +a user header. (#1227) * `stanc` failures during `$compile()` are now reported immediately, with the `stanc` error message. Previously they surfaced several steps later. (#1227) * Errors for include paths that do not exist now report the resolved absolute @@ -38,10 +40,6 @@ failed because those inputs were dropped. (#1234) * A `user_header` supplied to `cmdstan_model()` is now used by a later `$compile()`. Previously it was only honored when the model was compiled immediately. (#1234) -* `$compile(dry_run = TRUE)` no longer discards the `cpp_options`, -`stanc_options` and `include_paths` supplied to `cmdstan_model()`. (#1234) -* `$include_paths()` no longer returns `NULL` for a model whose executable does -not exist, such as after `$compile(dry_run = TRUE)`. (#1234) * CmdStanModel methods now correctly handle `#include` directories with spaces in their paths. (#820) * `$include_paths()` now returns absolute paths, and relative include paths are diff --git a/R/model.R b/R/model.R index fe94abb0b..6446c33f3 100644 --- a/R/model.R +++ b/R/model.R @@ -975,6 +975,9 @@ check_syntax <- function(pedantic = FALSE, if (is.null(include_paths) && !is.null(self$include_paths())) { include_paths <- self$include_paths() } + if (private$using_user_header_) { + stanc_options[["allow-undefined"]] <- TRUE + } temp_hpp_file <- tempfile(pattern = "model-", fileext = ".hpp") stanc_options[["o"]] <- wsl_safe_path(temp_hpp_file) @@ -1106,6 +1109,9 @@ format <- function(overwrite_file = FALSE, self$include_paths(), direct_call = TRUE ) + if (private$using_user_header_) { + stanc_options[["allow-undefined"]] <- TRUE + } stanc_options[["auto-format"]] <- TRUE if (!is.null(max_line_length)) { stanc_options[["max-line-length"]] <- max_line_length diff --git a/tests/testthat/test-model-compile.R b/tests/testthat/test-model-compile.R index 7de3cf76a..c6ec082c4 100644 --- a/tests/testthat/test-model-compile.R +++ b/tests/testthat/test-model-compile.R @@ -626,6 +626,18 @@ test_that("check_syntax() works with include_paths on compiled model", { }) +test_that("check_syntax() and format() allow undefined functions with a user header", { + stan_file <- testing_stan_file("bernoulli_external") + # both methods only run stanc, which never reads the user header, so an empty + # one is enough here. Compiling against a real header is tested in + # test-model-compile-user_header.R + user_header <- withr::local_tempfile(lines = "", fileext = ".hpp") + mod <- cmdstan_model(stan_file, user_header = user_header, compile = FALSE) + + expect_true(mod$check_syntax(quiet = TRUE)) + expect_output(mod$format(), "make_odds", fixed = TRUE) +}) + test_that("compile() and check_syntax() error on removed syntax", { model_code <- " transformed data { From eeed5baf3ff251ebeb8917d50244aff596568da6 Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 13:22:10 -0600 Subject: [PATCH 04/34] Allow undefined functions in check_syntax() and format() $check_syntax() and $format() build their stanc arguments from precompile_stanc_options_ and never consulted using_user_header_, so a model with a function that is declared in the Stan program and defined in a user header was reported as a syntax error. $compile() derives --allow-undefined from the resolved user header and $variables() derives it from using_user_header_; these two methods were the only ones that did not. The failure does not depend on the model having been compiled: it happens on a model created with compile = FALSE as well, so it is not a consequence of the compile-time options being consumed once. --- NEWS.md | 8 +++----- R/model.R | 6 ++++++ tests/testthat/test-model-compile.R | 12 ++++++++++++ 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/NEWS.md b/NEWS.md index bc5120f78..91b79596a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -19,7 +19,9 @@ as of CmdStanR 1.0.0; use the lowercase `cmdstanr_no_ver_check` forms instead. `canonicalize`. The values were shell-quoted for Make and the same quoted strings were also passed to `stanc` directly, which rejected them. (#1227) * `$compile()` now enables `allow-undefined` for user headers supplied through -`cpp_options`, not just through the `user_header` argument. (#1227) +`cpp_options`, not just through the `user_header` argument. `$check_syntax()` +and `$format()` also now correctly enable `allow-undefined` for models that use +a user header. (#1227, #1234) * `stanc` failures during `$compile()` are now reported immediately, with the `stanc` error message. Previously they surfaced several steps later. (#1227) * Errors for include paths that do not exist now report the resolved absolute @@ -38,10 +40,6 @@ failed because those inputs were dropped. (#1234) * A `user_header` supplied to `cmdstan_model()` is now used by a later `$compile()`. Previously it was only honored when the model was compiled immediately. (#1234) -* `$compile(dry_run = TRUE)` no longer discards the `cpp_options`, -`stanc_options` and `include_paths` supplied to `cmdstan_model()`. (#1234) -* `$include_paths()` no longer returns `NULL` for a model whose executable does -not exist, such as after `$compile(dry_run = TRUE)`. (#1234) * CmdStanModel methods now correctly handle `#include` directories with spaces in their paths. (#820) * `$include_paths()` now returns absolute paths, and relative include paths are diff --git a/R/model.R b/R/model.R index fe94abb0b..6446c33f3 100644 --- a/R/model.R +++ b/R/model.R @@ -975,6 +975,9 @@ check_syntax <- function(pedantic = FALSE, if (is.null(include_paths) && !is.null(self$include_paths())) { include_paths <- self$include_paths() } + if (private$using_user_header_) { + stanc_options[["allow-undefined"]] <- TRUE + } temp_hpp_file <- tempfile(pattern = "model-", fileext = ".hpp") stanc_options[["o"]] <- wsl_safe_path(temp_hpp_file) @@ -1106,6 +1109,9 @@ format <- function(overwrite_file = FALSE, self$include_paths(), direct_call = TRUE ) + if (private$using_user_header_) { + stanc_options[["allow-undefined"]] <- TRUE + } stanc_options[["auto-format"]] <- TRUE if (!is.null(max_line_length)) { stanc_options[["max-line-length"]] <- max_line_length diff --git a/tests/testthat/test-model-compile.R b/tests/testthat/test-model-compile.R index 7de3cf76a..c6ec082c4 100644 --- a/tests/testthat/test-model-compile.R +++ b/tests/testthat/test-model-compile.R @@ -626,6 +626,18 @@ test_that("check_syntax() works with include_paths on compiled model", { }) +test_that("check_syntax() and format() allow undefined functions with a user header", { + stan_file <- testing_stan_file("bernoulli_external") + # both methods only run stanc, which never reads the user header, so an empty + # one is enough here. Compiling against a real header is tested in + # test-model-compile-user_header.R + user_header <- withr::local_tempfile(lines = "", fileext = ".hpp") + mod <- cmdstan_model(stan_file, user_header = user_header, compile = FALSE) + + expect_true(mod$check_syntax(quiet = TRUE)) + expect_output(mod$format(), "make_odds", fixed = TRUE) +}) + test_that("compile() and check_syntax() error on removed syntax", { model_code <- " transformed data { From c6c1386a52dcdbb550ca5f9ded4fc308145b9fe7 Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 17:30:38 -0600 Subject: [PATCH 05/34] Make the mocked compiler produce the executable it was asked for with_mocked_cli() returned status 0 without writing anything to the path make was given, so any code that installs the compiled artifact had nothing to install and no test could observe it failing. The mock now creates that file, and only when the mocked compile succeeds, since a failed make must not leave one behind. The isTRUE() guard is needed because existing callers pass compile_ret = list(), where a bare comparison would be if (logical(0)) and error. args[1] is a WSL-safe path, so it is converted back before use. That makes the destination of a mocked compile matter. The tests in test-model-recompile-logic.R compiled the CmdStan installation's own bernoulli example in place, which a faithful mock overwrites with an empty file; they now work on a temporary copy. Since that executable is not part of the repository and a truncating overwrite leaves no diff at all, the file also carries a guard test comparing its size and mtime before and after. --- tests/testthat/helper-mock-cli.R | 7 +++ tests/testthat/test-model-recompile-logic.R | 62 ++++++++++++++++++++- 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/tests/testthat/helper-mock-cli.R b/tests/testthat/helper-mock-cli.R index 799e8d1d0..19fa6bfa0 100644 --- a/tests/testthat/helper-mock-cli.R +++ b/tests/testthat/helper-mock-cli.R @@ -12,6 +12,13 @@ with_mocked_cli <- function(code, compile_ret, info_ret) { && startsWith(basename(args[1]), "model-") ) { message("mock-compile-was-called") + # Real `make` writes the executable named by args[1] when it succeeds and + # writes nothing when it fails. Without this, code that installs the + # compiled artifact silently has nothing to install. `isTRUE()` because + # callers may pass a `compile_ret` with no status at all. + if (isTRUE(compile_ret$status == 0)) { + file.create(wsl_safe_path(args[1], revert = TRUE)) + } compile_ret } else if (!is.null(args) && args[1] == "info") { info_ret diff --git a/tests/testthat/test-model-recompile-logic.R b/tests/testthat/test-model-recompile-logic.R index c9d7a3856..3e5440a26 100644 --- a/tests/testthat/test-model-recompile-logic.R +++ b/tests/testthat/test-model-recompile-logic.R @@ -1,4 +1,16 @@ -stan_program <- cmdstan_example_file() +# A mocked compile produces a real (empty) executable at the destination, so the +# model compiled here must be a temporary copy. Compiling the installed example +# in place would replace the CmdStan installation's own executable. +example_exe <- cmdstan_ext(strip_ext(cmdstan_example_file())) +example_exe_before <- file.info(example_exe)[, c("size", "mtime")] + +model_dir <- withr::local_tempdir() +stan_program <- file.path(model_dir, "bernoulli.stan") +file.copy(cmdstan_example_file(), stan_program) +# Keep the program older than the placeholder executables created below, so the +# up-to-date check doesn't force a recompile on timestamp resolution alone. +Sys.setFileTime(stan_program, Sys.time() - 60) + file_that_doesnt_exist <- withr::local_tempfile(pattern = "placeholder_doesnt_exist") file_that_exists <- withr::local_tempfile(pattern = "placeholder_exists") file.create(file_that_exists) @@ -23,13 +35,48 @@ test_that("warning when no recompile and no info", { test_that("recompiles when force_recompile flag set", with_mocked_cli( compile_ret = list(status = 0), - info_ret = list(), + # The mocked compile now leaves an executable behind, so the constructor + # queries it for compilation info. Report a failure rather than an empty + # list, which model_compile_info() cannot interpret. + info_ret = list(status = 1), code = expect_mock_compile({ mod <- cmdstan_model(stan_file = stan_program, force_recompile = TRUE) }) ) ) +test_that("a mocked successful compile installs an executable", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + exe <- cmdstan_ext(strip_ext(stan_file)) + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = mod <- cmdstan_model(stan_file = stan_file, force_recompile = TRUE) + ) + + expect_equal(mod$exe_file(), exe) + expect_true(file.exists(exe)) +}) + +test_that("a mocked failed compile installs no executable", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + exe <- cmdstan_ext(strip_ext(stan_file)) + + with_mocked_cli( + compile_ret = list(status = 1), + info_ret = list(status = 1), + code = expect_error( + cmdstan_model(stan_file = stan_file, force_recompile = TRUE), + "An error occurred during compilation" + ) + ) + + expect_false(file.exists(exe)) +}) + test_that("no mismatch results in no recompile", with_mocked_cli( compile_ret = list(status = 0), info_ret = list( @@ -103,3 +150,14 @@ test_that("recompile when cpp args don't match binary", { }) ) }) + +# Deliberately the last test in this file: it checks that none of the mocked +# compiles above installed anything over the CmdStan installation's own example +# executable. A git diff would not catch this, since that executable is not part +# of the repository, and a truncating overwrite changes no tracked file at all. +test_that("mocked compiles leave the CmdStan installation untouched", { + expect_equal( + file.info(example_exe)[, c("size", "mtime")], + example_exe_before + ) +}) From f625bac59a4499758e7c5ffc3002f682b6358fe5 Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 17:34:54 -0600 Subject: [PATCH 06/34] Commit compile state only after the executable is replaced Successful replacement of the executable is the point at which state describing the compiled artifact may be committed, but several mutations still happened before the make call or on paths where nothing was compiled at all. The model-method environment and the generated .hpp path were assigned before the compiler ran, so a failure at the C++ stage left the old executable paired with model-method code generated from the new source. That environment is handed to every fit, so fit$init_model_methods() would compile log_prob() from a program the draws did not come from. Both are now staged in locals and committed with everything else, along with reading the Stan source, and the model-method header is written before the executable is replaced rather than after. A compile that finds the executable up to date compiles nothing, so it may no longer consume or overwrite what describes the current executable. It previously replaced cpp_options_ with whatever the call supplied, which erased stan_threads and made assert_valid_threads() run a threaded executable single-threaded; it cleared the precompile options a later forced recompilation needs; and it asserted existing_exe unconditionally, so $expose_functions() failed on a model that had compiled itself. When the object is instead adopting an executable it did not build, the options are recovered from the binary itself on a best-effort basis, reusing the filtering the constructor already did. Resolving to a different executable than the object describes now forces compilation rather than adopting it, since keeping this object's generated C++ alongside another binary is the same hybrid. Paths are compared canonically so symlink aliases and Windows casing do not cause needless rebuilds. --- R/cpp_opts.R | 16 +++ R/model.R | 74 ++++++++---- R/utils.R | 19 ++++ .../testthat/test-model-compile-user_header.R | 39 +++++++ tests/testthat/test-model-compile.R | 75 ++++++++++++ tests/testthat/test-model-recompile-logic.R | 107 ++++++++++++++++++ 6 files changed, 307 insertions(+), 23 deletions(-) diff --git a/R/cpp_opts.R b/R/cpp_opts.R index b13f0e3f5..5f4cd4cf5 100644 --- a/R/cpp_opts.R +++ b/R/cpp_opts.R @@ -73,6 +73,22 @@ model_compile_info <- function(exe_file, version) { info } +# Merge the options an executable reports about itself into the options already +# recorded for it. STAN_VERSION describes the toolchain rather than a make +# option, and a flag the executable reports as FALSE was never set at all, so +# recording it would pass "FLAG=FALSE" to make, which CmdStan reads as enabling +# the flag. +merge_exe_info_cpp_options <- function(cpp_options, exe_info) { + for (option_name in names(exe_info)) { + value <- exe_info[[option_name]] + if (tolower(option_name) != "stan_version" && + (!is.logical(value) || isTRUE(value))) { + cpp_options[[option_name]] <- value + } + } + cpp_options +} + # convert to compile flags -------------------- # from list(flag1=TRUE, flag2=FALSE) to "FLAG1=TRUE\nFLAG2=FALSE" cpp_options_to_compile_flags <- function(cpp_options) { diff --git a/R/model.R b/R/model.R index 6446c33f3..1d709dce7 100644 --- a/R/model.R +++ b/R/model.R @@ -296,13 +296,10 @@ CmdStanModel <- R6::R6Class( # as the version the model was compiled with private$cmdstan_version_ <- cmdstan_version() if (length(self$exe_file()) > 0 && file.exists(self$exe_file())) { - cpp_options <- model_compile_info(self$exe_file(), self$cmdstan_version()) - for (cpp_option_name in names(cpp_options)) { - if (tolower(cpp_option_name) != "stan_version" && - (!is.logical(cpp_options[[cpp_option_name]]) || isTRUE(cpp_options[[cpp_option_name]]))) { - private$cpp_options_[[cpp_option_name]] <- cpp_options[[cpp_option_name]] - } - } + private$cpp_options_ <- merge_exe_info_cpp_options( + private$cpp_options_, + model_compile_info(self$exe_file(), self$cmdstan_version()) + ) } invisible(self) }, @@ -607,9 +604,6 @@ compile <- function(quiet = TRUE, if (!is.null(dir)) { dir <- repair_path(dir) assert_dir_exists(dir, access = "rw") - if (length(self$exe_file()) != 0) { - private$exe_file_ <- file.path(dir, basename(self$exe_file())) - } } exe <- resolve_exe_path(dir, private$dir_, self$exe_file(), self$stan_file()) @@ -658,14 +652,22 @@ compile <- function(quiet = TRUE, } } + # The resolved destination need not be the executable this object describes, + # e.g. $compile(dir = ) aimed at a directory that already holds a current + # executable. Adopting that binary while keeping this object's generated C++ + # and metadata would produce a hybrid, so build the model there instead. + exe_changed <- length(private$exe_file_) > 0 && !same_path(exe, private$exe_file_) + # compile if: # - the user forced compilation, # - the executable does not exist + # - the destination is not the executable this object already describes # - the stan model was changed since last compilation # - a user header is used and the user header changed since last compilation (#813) - self$exe_file(exe) if (!file.exists(exe)) { force_recompile <- TRUE + } else if (exe_changed) { + force_recompile <- TRUE } else if (file.exists(self$stan_file()) && file.mtime(exe) < file.mtime(self$stan_file())) { force_recompile <- TRUE @@ -679,11 +681,28 @@ compile <- function(quiet = TRUE, if (rlang::is_interactive()) { message("Model executable is up to date!") } - private$cpp_options_ <- cpp_options - private$precompile_cpp_options_ <- NULL - private$precompile_stanc_options_ <- NULL - private$precompile_include_paths_ <- NULL - self$functions$existing_exe <- TRUE + # Nothing was compiled, so nothing describing the current executable may be + # consumed or overwritten by configuration this call merely proposed. + if (length(private$exe_file_) == 0) { + # This object is adopting an executable it did not build: it holds no + # generated C++ for it, and the only available description of it is what + # the binary reports about itself. Best effort, because + # model_compile_info() runs the executable and errors outright rather than + # returning a status when the file is not runnable. + self$functions$existing_exe <- TRUE + private$cpp_options_ <- tryCatch( + merge_exe_info_cpp_options( + private$cpp_options_, + model_compile_info(exe, self$cmdstan_version()) + ), + error = function(e) private$cpp_options_ + ) + } else { + # The flag means "we don't hold the generated C++ for this executable", + # which is not the same as "this call compiled nothing". + self$functions$existing_exe <- is.null(self$functions$hpp_code) + } + private$exe_file_ <- exe return(invisible(self)) } else { if (rlang::is_interactive()) { @@ -706,7 +725,7 @@ compile <- function(quiet = TRUE, if (os_is_windows() && !os_is_wsl()) { tmp_exe <- utils::shortPathName(tmp_exe) } - private$hpp_file_ <- paste0(temp_file_no_ext, ".hpp") + hpp_file <- paste0(temp_file_no_ext, ".hpp") stancflags_val <- include_paths_stanc3_args(include_paths) @@ -723,8 +742,10 @@ compile <- function(quiet = TRUE, stanc_inc_paths <- include_paths_stanc3_args(include_paths, direct_call = TRUE) stancflags_standalone <- c("--standalone-functions", stanc_inc_paths, stancflags_direct) standalone_hpp_code <- get_standalone_hpp(temp_stan_file, stancflags_standalone) - private$model_methods_env_ <- new.env() - private$model_methods_env_$hpp_code_ <- get_standalone_hpp(temp_stan_file, c(stanc_inc_paths, stancflags_direct)) + # Staged in a local: this describes the program being compiled now, so it may + # not reach the object unless that program's executable is installed. (#1228) + model_methods_env <- new.env() + model_methods_env$hpp_code_ <- get_standalone_hpp(temp_stan_file, c(stanc_inc_paths, stancflags_direct)) stancflags_val <- paste0("STANCFLAGS += ", stancflags_val, paste0(" ", stancflags_combined, collapse = " ")) @@ -786,6 +807,14 @@ compile <- function(quiet = TRUE, stop("An error occurred during compilation! See the message above for more information.", call. = FALSE) } + # Everything that can still fail happens before the executable is replaced, + # so that the commit block below is only assignments. Writing the + # model-method header is fallible and has to come after make, which + # generates its own .hpp at the same path. + stan_code <- readLines(temp_stan_file) + writeLines(model_methods_env$hpp_code_, + con = wsl_safe_path(hpp_file, revert = TRUE)) + if (file.exists(exe)) { file.remove(exe) } @@ -806,10 +835,12 @@ compile <- function(quiet = TRUE, self$functions$hpp_code <- standalone_hpp_code self$functions$external <- using_user_header self$functions$existing_exe <- FALSE - private$stan_code_ <- readLines(temp_stan_file) + private$stan_code_ <- stan_code private$variables_ <- NULL private$using_user_header_ <- using_user_header private$user_header_ <- user_header + private$hpp_file_ <- hpp_file + private$model_methods_env_ <- model_methods_env private$precompile_cpp_options_ <- NULL private$precompile_stanc_options_ <- NULL private$precompile_include_paths_ <- NULL @@ -817,9 +848,6 @@ compile <- function(quiet = TRUE, if (compile_standalone) { expose_stan_functions(self$functions, verbose = !quiet) } - - writeLines(private$model_methods_env_$hpp_code_, - con = wsl_safe_path(private$hpp_file_, revert = TRUE)) } # End - if(!dry_run) private$cmdstan_version_ <- cmdstan_version() diff --git a/R/utils.R b/R/utils.R index eed238bc3..c390e031f 100644 --- a/R/utils.R +++ b/R/utils.R @@ -205,6 +205,25 @@ resolve_path <- function(path) { repair_path(absolute_path(path)) } +# Do two paths name the same file? Compared canonically rather than as strings, +# so that symlink aliases, ".." components, separator differences and (on +# Windows) casing don't make one file look like two. +# +# mustWork = FALSE is deliberate: neither path is guaranteed to exist, and the +# default mustWork = NA warns when a path cannot be normalized, which +# options(warn = 2) would turn into an error. A path that cannot be normalized +# is compared as given, which for a missing path against an existing one means +# "different" -- the safe answer for both callers. +same_path <- function(x, y) { + if (length(x) == 0 || length(y) == 0) { + return(length(x) == length(y)) + } + identical( + normalizePath(x, winslash = "/", mustWork = FALSE), + normalizePath(y, winslash = "/", mustWork = FALSE) + ) +} + # read, write, and copy files -------------------------------------------- #' Copy temporary files (e.g., output, data) to a different location diff --git a/tests/testthat/test-model-compile-user_header.R b/tests/testthat/test-model-compile-user_header.R index fbe12ada8..03b409aee 100644 --- a/tests/testthat/test-model-compile-user_header.R +++ b/tests/testthat/test-model-compile-user_header.R @@ -74,6 +74,45 @@ test_that("compile() reuses the user header from the previous compilation", { ) }) +test_that("a no-op compile preserves a header supplied via cpp_options", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli_external.stan") + file.copy(testing_stan_file("bernoulli_external"), stan_file) + user_header <- withr::local_tempfile(lines = "", fileext = ".hpp") + local_mocked_bindings( + get_cmdstan_flags = function(flag_name) character(), + get_standalone_hpp = function(stan_file, stancflags) "" + ) + model <- cmdstan_model(stan_file, compile = FALSE) + + # The lowercase spelling is the telling one: a bare recompile re-derives the + # header under the USER_HEADER spelling, so only this one shows whether the + # no-op path rebuilt the recorded options or left them alone. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = model$compile( + cpp_options = list(user_header = user_header), + force_recompile = TRUE + ) + ) + expect_equal( + model$cpp_options()[["user_header"]], + wsl_safe_path(absolute_path(user_header)) + ) + + # The executable is up to date, so this call compiles nothing and must leave + # the options describing it alone. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile(model$compile()) + ) + expect_equal( + model$cpp_options()[["user_header"]], + wsl_safe_path(absolute_path(user_header)) + ) +}) + test_that("compile() uses a user header supplied to cmdstan_model()", { stan_file <- testing_stan_file("bernoulli_external") user_header <- withr::local_tempfile(lines = "", fileext = ".hpp") diff --git a/tests/testthat/test-model-compile.R b/tests/testthat/test-model-compile.R index c6ec082c4..54cf06084 100644 --- a/tests/testthat/test-model-compile.R +++ b/tests/testthat/test-model-compile.R @@ -403,6 +403,81 @@ test_that("a failed compile() doesn't refresh cached model state", { expect_false(model$functions$compiled) }) +# A compiled model whose compiler is mocked, so that stanc runs for real and +# only the C++ stage can be made to fail. That is the case the tests above miss: +# the generated C++ for the new program is already in hand, so any state derived +# from it that is recorded before the executable is replaced would outlive a +# failure and describe a program the executable was never built from. +# +# The copy is temporary because a mocked compile installs a real (empty) +# executable, and the model here is the CmdStan installation's own example. +local_mocked_bernoulli_model <- function(.local_envir = parent.frame()) { + stan_file <- file.path( + withr::local_tempdir(.local_envir = .local_envir), + "bernoulli.stan" + ) + file.copy(cmdstan_example_file(), stan_file) + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = cmdstan_model(stan_file) + ) +} + +test_that("a failed C++ compile doesn't refresh generated-code state", { + model <- local_mocked_bernoulli_model() + private <- model$.__enclos_env__$private + + code_before <- model$code() + variables_before <- model$variables() + functions_before <- as.list(model$functions) + hpp_file_before <- model$hpp_file() + hpp_code_before <- private$model_methods_env_$hpp_code_ + expect_true(any(nzchar(hpp_code_before))) + + # The model-method environment is handed to every fit, so a stale pairing here + # means fit$init_model_methods() compiles log_prob() from a program the draws + # did not come from. + writeLines( + "parameters { real beta; } model { beta ~ std_normal(); }", + model$stan_file() + ) + with_mocked_cli( + compile_ret = list(status = 1), + info_ret = list(status = 1), + code = expect_error( + model$compile(force_recompile = TRUE), + "An error occurred during compilation!", + fixed = TRUE + ) + ) + + expect_identical(model$code(), code_before) + expect_identical(model$variables(), variables_before) + expect_identical(as.list(model$functions), functions_before) + expect_identical(model$hpp_file(), hpp_file_before) + expect_identical(private$model_methods_env_$hpp_code_, hpp_code_before) +}) + +test_that("a failed C++ compile doesn't move the executable path", { + model <- local_mocked_bernoulli_model() + exe_before <- model$exe_file() + other_dir <- withr::local_tempdir() + + with_mocked_cli( + compile_ret = list(status = 1), + info_ret = list(status = 1), + code = expect_error( + model$compile(dir = other_dir, force_recompile = TRUE), + "An error occurred during compilation!", + fixed = TRUE + ) + ) + + expect_identical(model$exe_file(), exe_before) + expect_true(file.exists(exe_before)) +}) + test_that("dir arg works for cmdstan_model and $compile()", { tmp_dir <- tempdir() tmp_dir_2 <- tempdir() diff --git a/tests/testthat/test-model-recompile-logic.R b/tests/testthat/test-model-recompile-logic.R index 3e5440a26..fa18c3221 100644 --- a/tests/testthat/test-model-recompile-logic.R +++ b/tests/testthat/test-model-recompile-logic.R @@ -60,6 +60,113 @@ test_that("a mocked successful compile installs an executable", { expect_true(file.exists(exe)) }) +test_that("a no-op compile preserves what the previous compilation recorded", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + + mod <- cmdstan_model(stan_file, compile = FALSE) + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = mod$compile( + cpp_options = list(stan_threads = TRUE), + force_recompile = TRUE + ) + ) + expect_true(mod$cpp_options()$stan_threads) + expect_false(mod$functions$existing_exe) + + # The second call finds the executable up to date and compiles nothing, so it + # must not discard the options the executable was actually built with: an + # erased stan_threads makes assert_valid_threads() drop 'threads' and run a + # threaded executable single-threaded. It must not claim the executable is + # pre-compiled either, or $expose_functions() fails on a model that built + # itself. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile(mod$compile()) + ) + expect_true(mod$cpp_options()$stan_threads) + expect_false(mod$functions$existing_exe) + expect_warning(mod$expose_functions(), "No standalone functions found") +}) + +test_that("a no-op compile adopts an executable the object did not build", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + exe <- cmdstan_ext(strip_ext(stan_file)) + + # Build the executable through one object, then let a second, freshly + # constructed object find it up to date. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = cmdstan_model(stan_file, force_recompile = TRUE) + ) + mod <- cmdstan_model(stan_file, compile = FALSE) + expect_length(mod$exe_file(), 0) + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list( + status = 0, + stdout = "stan_version_major=2\nstan_version_minor=39\nstan_version_patch=0\nSTAN_THREADS=true\nSTAN_OPENCL=false" + ), + code = expect_no_mock_compile(mod$compile()) + ) + + expect_equal(mod$exe_file(), exe) + expect_true(mod$functions$existing_exe) + # Hydrated from the executable itself: STAN_THREADS is reported, STAN_OPENCL + # is reported as FALSE (i.e. never set) and STAN_VERSION is not a make option. + expect_true(mod$cpp_options()$STAN_THREADS) + expect_null(mod$cpp_options()$STAN_OPENCL) + expect_null(mod$cpp_options()$STAN_VERSION) +}) + +test_that("a no-op compile tolerates an executable it cannot query", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = cmdstan_model(stan_file, force_recompile = TRUE) + ) + mod <- cmdstan_model(stan_file, compile = FALSE) + + # The mocked executable is empty, so running it errors rather than returning + # a non-zero status. Adopting it is best-effort and must still succeed. + expect_no_error(mod$compile()) + expect_true(mod$functions$existing_exe) +}) + +test_that("compiling into a directory with a different executable recompiles", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = mod <- cmdstan_model(stan_file) + ) + + # A current executable already sits in the target directory. Adopting it would + # leave the object describing this program's C++ while running that binary, so + # the model is rebuilt there instead. + other_dir <- withr::local_tempdir() + other_exe <- cmdstan_ext(file.path(other_dir, "bernoulli")) + file.create(other_exe) + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_mock_compile(mod$compile(dir = other_dir)) + ) + expect_equal(mod$exe_file(), other_exe) +}) + test_that("a mocked failed compile installs no executable", { stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") file.copy(stan_program, stan_file) From 5f04a7deeb9235cb1dc14971e0ed372ab1b9865d Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 17:36:29 -0600 Subject: [PATCH 07/34] Replace the model executable through a staged, recoverable swap The old executable was removed and the new one copied over it with both return values discarded, so a copy that failed after a successful remove left the model with no executable and no error. The file.remove() had no recorded rationale; it was added in 2021 in a commit titled "fix syntax" with an empty body. install_executable() stages the new executable beside the destination, moves any existing one to a sibling backup, and only then renames the staged copy into place, restoring the backup if that rename fails. Both temporary names come from tempfile() rather than fixed .new/.bak suffixes, which would collide with stale files and parallel builds. WSL's chmod +x moves onto the staged candidate and has its status checked, since an unchecked chmod after installation is another boundary where the executable is in place but not safely committed. Every filesystem call is wrapped in suppressWarnings() and checked by value. file.copy() and file.rename() warn on failure, so under options(warn = 2) base throws before returning FALSE, and on the candidate-to-destination rename that would skip the rollback entirely and strand the only good executable at the backup path. unlink() reports a status without signalling, so it needs no such treatment, but it returns 0L rather than TRUE. A backup that cannot be removed after a successful install is returned, not signalled. Under warn = 2 a warning here would unwind before the caller could record the state describing the executable just installed, which is precisely the hybrid this work exists to prevent, so the caller warns only after the optional exposure work has run. This is staged and rollback-capable rather than transactional: a crash between the two renames can still leave only the backup. --- R/model.R | 26 +++-- R/utils.R | 93 ++++++++++++++++ tests/testthat/_snaps/model-compile.md | 11 ++ tests/testthat/_snaps/utils.md | 32 ++++++ tests/testthat/test-model-compile.R | 110 +++++++++++++++++++ tests/testthat/test-utils.R | 144 +++++++++++++++++++++++++ 6 files changed, 405 insertions(+), 11 deletions(-) create mode 100644 tests/testthat/_snaps/model-compile.md diff --git a/R/model.R b/R/model.R index 1d709dce7..b9df5dd10 100644 --- a/R/model.R +++ b/R/model.R @@ -815,17 +815,11 @@ compile <- function(quiet = TRUE, writeLines(model_methods_env$hpp_code_, con = wsl_safe_path(hpp_file, revert = TRUE)) - if (file.exists(exe)) { - file.remove(exe) - } - file.copy(tmp_exe, exe, overwrite = TRUE) - if (os_is_wsl()) { - res <- processx::run( - command = "wsl", - args = c("chmod", "+x", wsl_safe_path(exe)), - error_on_status = FALSE - ) - } + # Errors if the executable could not be replaced, so a failure here can + # never leave the model with no executable at all. A backup that could not + # be cleaned up afterwards is reported rather than signalled, and warned + # about only once all the optional work below has had its chance to run. + leftover_backup <- install_executable(tmp_exe, exe) # The new executable is in place, so everything derived from the Stan # program that was just compiled can be committed. A dry run or a failed @@ -858,6 +852,16 @@ compile <- function(quiet = TRUE, if (compile_model_methods) { expose_model_methods(env = private$model_methods_env_, verbose = !quiet) } + if (!is.null(leftover_backup)) { + # Deliberately last: under options(warn = 2) this is an error, and raising + # it any earlier would abort exposure the user asked for -- or worse, roll + # back before the state describing the installed executable was recorded. + warning( + "The previously compiled executable could not be removed. ", + "It has been left at '", leftover_backup, "'.", + call. = FALSE + ) + } } invisible(self) } diff --git a/R/utils.R b/R/utils.R index c390e031f..fa28e50f1 100644 --- a/R/utils.R +++ b/R/utils.R @@ -275,6 +275,99 @@ copy_temp_files <- absolute_path(destinations) } +#' Replace a model executable with a newly compiled one +#' +#' Stages the new executable beside the destination, moves any existing +#' executable aside, and only then renames the staged copy into place. Every step +#' that fails before the final rename leaves the destination exactly as it was; +#' a failed final rename restores the previous executable. +#' +#' Staged and rollback-capable rather than transactional: a crash between the two +#' renames can still leave the previous executable at the backup path only. +#' +#' Every filesystem call is wrapped in `suppressWarnings()` and checked by value. +#' `file.copy()` and `file.rename()` warn on failure, so under +#' `options(warn = 2)` base would throw before returning `FALSE` -- and if that +#' happened on the final rename, the rollback below would never run and the only +#' good executable would be stranded at the backup path. `unlink()` reports a +#' status without signalling, so it needs no such treatment, but it returns +#' `0L` for success rather than `TRUE`. +#' +#' @noRd +#' @param from Path to the newly compiled executable. +#' @param to Path the executable should be installed at. +#' @return `NULL` on a clean install, or the path to a backup of the previous +#' executable that could not be removed afterwards. Callers must not treat a +#' returned path as a failure: the new executable is installed either way, and +#' signalling from here would unwind before the caller could record the state +#' describing it. +install_executable <- function(from, to) { + candidate <- tempfile(pattern = "exe-new-", tmpdir = dirname(to)) + if (!isTRUE(suppressWarnings(file.copy(from, candidate)))) { + stop( + "Could not stage the compiled executable at '", candidate, "'. ", + "The model executable at '", to, "' was not modified.", + call. = FALSE + ) + } + if (os_is_wsl()) { + chmod <- processx::run( + command = "wsl", + args = c("chmod", "+x", wsl_safe_path(candidate)), + error_on_status = FALSE + ) + if (is.na(chmod$status) || chmod$status != 0) { + unlink(candidate) + stop( + "Could not make the compiled executable executable. ", + "The model executable at '", to, "' was not modified.", + call. = FALSE + ) + } + } + + backup <- NULL + if (file.exists(to)) { + backup <- tempfile(pattern = "exe-old-", tmpdir = dirname(to)) + if (!isTRUE(suppressWarnings(file.rename(to, backup)))) { + unlink(candidate) + stop( + "Could not move the existing executable '", to, "' aside. ", + "It was not modified.", + call. = FALSE + ) + } + } + + if (!isTRUE(suppressWarnings(file.rename(candidate, to)))) { + unlink(candidate) + if (is.null(backup)) { + stop( + "Could not install the compiled executable at '", to, "'.", + call. = FALSE + ) + } + if (!isTRUE(suppressWarnings(file.rename(backup, to)))) { + stop( + "Could not install the compiled executable at '", to, "' and the ", + "previously compiled executable could not be restored. It has been ", + "kept at '", backup, "'.", + call. = FALSE + ) + } + stop( + "Could not install the compiled executable at '", to, "'. ", + "The previously compiled executable has been restored.", + call. = FALSE + ) + } + + if (!is.null(backup) && unlink(backup) != 0L) { + return(backup) + } + NULL +} + # generate new file names # see doc above for copy_temp_files generate_file_names <- diff --git a/tests/testthat/_snaps/model-compile.md b/tests/testthat/_snaps/model-compile.md new file mode 100644 index 000000000..c048e1049 --- /dev/null +++ b/tests/testthat/_snaps/model-compile.md @@ -0,0 +1,11 @@ +# a leftover backup doesn't unwind a compile when warnings are errors + + Code + withr::with_options(list(warn = 2), model$compile(cpp_options = list( + stan_threads = TRUE), force_recompile = TRUE)) + Message + mock-compile-was-called + Condition + Error: + ! (converted from warning) The previously compiled executable could not be removed. It has been left at '/exe-old-'. + diff --git a/tests/testthat/_snaps/utils.md b/tests/testthat/_snaps/utils.md index 7e76d4c14..ffd35ac53 100644 --- a/tests/testthat/_snaps/utils.md +++ b/tests/testthat/_snaps/utils.md @@ -28,3 +28,35 @@ Error: ! Failed to move files: one or more files could not be copied. No original files were removed. +# install_executable() leaves the destination alone if staging fails + + Code + install_executable(fixture$from, fixture$to) + Condition + Error: + ! Could not stage the compiled executable at '/exe-new-'. The model executable at '/model-exe' was not modified. + +# install_executable() leaves the destination alone if the backup fails + + Code + install_executable(fixture$from, fixture$to) + Condition + Error: + ! Could not move the existing executable '/model-exe' aside. It was not modified. + +# install_executable() restores the backup if the install fails + + Code + install_executable(fixture$from, fixture$to) + Condition + Error: + ! Could not install the compiled executable at '/model-exe'. The previously compiled executable has been restored. + +# install_executable() keeps the backup if it cannot be restored + + Code + install_executable(fixture$from, fixture$to) + Condition + Error: + ! Could not install the compiled executable at '/model-exe' and the previously compiled executable could not be restored. It has been kept at '/exe-old-'. + diff --git a/tests/testthat/test-model-compile.R b/tests/testthat/test-model-compile.R index 54cf06084..7056daa19 100644 --- a/tests/testthat/test-model-compile.R +++ b/tests/testthat/test-model-compile.R @@ -478,6 +478,116 @@ test_that("a failed C++ compile doesn't move the executable path", { expect_true(file.exists(exe_before)) }) +test_that("compile() errors if the executable cannot be replaced", { + model <- local_mocked_bernoulli_model() + exe <- model$exe_file() + writeLines("old executable", exe) + + # Fail the first attempt to put a file at the destination. A replacement that + # fails unnoticed leaves the model with no executable at all. + real_file_copy <- base::file.copy + real_file_rename <- base::file.rename + installs <- 0 + local_mocked_bindings( + file.copy = function(from, to, ...) { + if (identical(to, exe)) { + installs <<- installs + 1 + if (installs == 1L) return(FALSE) + } + real_file_copy(from, to, ...) + }, + file.rename = function(from, to) { + if (identical(to, exe)) { + installs <<- installs + 1 + if (installs == 1L) return(FALSE) + } + real_file_rename(from, to) + }, + .package = "base" + ) + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_error(model$compile(force_recompile = TRUE)) + ) + expect_true(file.exists(exe)) + expect_identical(readLines(exe), "old executable") +}) + +# Set up a model whose executable is about to be replaced by a program that can +# be told apart from it, with removal of the old executable's backup failing. +local_leftover_backup_model <- function(.local_envir = parent.frame()) { + model <- local_mocked_bernoulli_model(.local_envir = .local_envir) + writeLines("old executable", model$exe_file()) + writeLines( + "parameters { real beta; } model { beta ~ std_normal(); }", + model$stan_file() + ) + local_mocked_bindings( + unlink = function(...) 1L, + .package = "base", + .env = .local_envir + ) + model +} + +expect_describes_new_program <- function(model) { + private <- model$.__enclos_env__$private + expect_identical( + model$code(), + "parameters { real beta; } model { beta ~ std_normal(); }" + ) + expect_equal(model$variables()$parameters$beta$dimensions, 0) + expect_match(paste(private$model_methods_env_$hpp_code_, collapse = "\n"), "beta") + expect_match(paste(readLines(model$hpp_file()), collapse = "\n"), "beta") + expect_true(model$cpp_options()$stan_threads) + # The mocked replacement is empty, the executable it replaced was not. + expect_equal(file.size(model$exe_file()), 0) +} + +test_that("a leftover backup warns without discarding a successful compile", { + model <- local_leftover_backup_model() + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_warning( + model$compile(cpp_options = list(stan_threads = TRUE), force_recompile = TRUE), + "could not be removed" + ) + ) + + expect_describes_new_program(model) +}) + +test_that("a leftover backup doesn't unwind a compile when warnings are errors", { + model <- local_leftover_backup_model() + model_dir <- dirname(model$exe_file()) + + # Signalling the cleanup failure before the state is committed would install + # the new executable while the object still described the old program -- the + # exact hybrid this all exists to prevent. The option is scoped to the call so + # that testthat's own snapshot warnings stay warnings. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_snapshot( + error = TRUE, + withr::with_options( + list(warn = 2), + model$compile(cpp_options = list(stan_threads = TRUE), force_recompile = TRUE) + ), + transform = function(lines) { + lines <- gsub(model_dir, "", lines, fixed = TRUE) + gsub("exe-old-[0-9a-f]+", "exe-old-", lines) + } + ) + ) + + expect_describes_new_program(model) +}) + test_that("dir arg works for cmdstan_model and $compile()", { tmp_dir <- tempdir() tmp_dir_2 <- tempdir() diff --git a/tests/testthat/test-utils.R b/tests/testthat/test-utils.R index c38390498..b68ad3e18 100644 --- a/tests/testthat/test-utils.R +++ b/tests/testthat/test-utils.R @@ -209,6 +209,150 @@ test_that("copy_temp_files retains sources if any copy fails", { expect_identical(file.exists(source_paths), c(TRUE, TRUE)) }) +local_exe_fixture <- function(destination_exists = TRUE, + .local_envir = parent.frame()) { + dir <- withr::local_tempdir(.local_envir = .local_envir) + fixture <- list( + dir = dir, + from = file.path(dir, "compiled-exe"), + to = file.path(dir, "model-exe") + ) + writeLines("new executable", fixture$from) + if (destination_exists) { + writeLines("old executable", fixture$to) + } + fixture +} + +# Normalize the random staging and backup names out of a snapshot, keeping the +# structure of the paths the diagnostics name. +exe_path_transform <- function(fixture) { + function(lines) { + lines <- gsub(fixture$dir, "", lines, fixed = TRUE) + gsub("exe-(new|old)-[0-9a-f]+", "exe-\\1-", lines) + } +} + +# Make the n-th file.rename() call fail, optionally warning first, as base does. +local_failing_file_rename <- function(fail_on, + warn = FALSE, + .local_envir = parent.frame()) { + real_file_rename <- base::file.rename + calls <- 0 + local_mocked_bindings( + file.rename = function(from, to) { + calls <<- calls + 1 + if (calls %in% fail_on) { + if (warn) warning("cannot rename file") + return(FALSE) + } + real_file_rename(from, to) + }, + .package = "base", + .env = .local_envir + ) +} + +test_that("install_executable() installs when there is no existing executable", { + fixture <- local_exe_fixture(destination_exists = FALSE) + + expect_null(install_executable(fixture$from, fixture$to)) + expect_identical(readLines(fixture$to), "new executable") + expect_setequal(list.files(fixture$dir), basename(c(fixture$from, fixture$to))) +}) + +test_that("install_executable() replaces an executable and removes the backup", { + fixture <- local_exe_fixture() + + expect_null(install_executable(fixture$from, fixture$to)) + expect_identical(readLines(fixture$to), "new executable") + expect_setequal(list.files(fixture$dir), basename(c(fixture$from, fixture$to))) +}) + +test_that("install_executable() leaves the destination alone if staging fails", { + fixture <- local_exe_fixture() + local_mocked_bindings(file.copy = function(...) FALSE, .package = "base") + + expect_snapshot( + error = TRUE, + install_executable(fixture$from, fixture$to), + transform = exe_path_transform(fixture) + ) + expect_identical(readLines(fixture$to), "old executable") + expect_setequal(list.files(fixture$dir), basename(c(fixture$from, fixture$to))) +}) + +test_that("install_executable() leaves the destination alone if the backup fails", { + fixture <- local_exe_fixture() + local_failing_file_rename(fail_on = 1) + + expect_snapshot( + error = TRUE, + install_executable(fixture$from, fixture$to), + transform = exe_path_transform(fixture) + ) + expect_identical(readLines(fixture$to), "old executable") + expect_setequal(list.files(fixture$dir), basename(c(fixture$from, fixture$to))) +}) + +test_that("install_executable() restores the backup if the install fails", { + fixture <- local_exe_fixture() + local_failing_file_rename(fail_on = 2) + + expect_snapshot( + error = TRUE, + install_executable(fixture$from, fixture$to), + transform = exe_path_transform(fixture) + ) + expect_identical(readLines(fixture$to), "old executable") + expect_setequal(list.files(fixture$dir), basename(c(fixture$from, fixture$to))) +}) + +test_that("install_executable() keeps the backup if it cannot be restored", { + fixture <- local_exe_fixture() + local_failing_file_rename(fail_on = c(2, 3)) + + expect_snapshot( + error = TRUE, + install_executable(fixture$from, fixture$to), + transform = exe_path_transform(fixture) + ) + # The destination is gone, so the error has to name a real recovery path. + expect_false(file.exists(fixture$to)) + leftover <- setdiff(list.files(fixture$dir), basename(fixture$from)) + expect_match(leftover, "^exe-old-") + expect_identical(readLines(file.path(fixture$dir, leftover)), "old executable") +}) + +test_that("install_executable() rolls back when warnings are errors", { + fixture <- local_exe_fixture() + # base warns and returns FALSE; under warn = 2 the warning alone would throw + # from inside file.rename(), skipping the rollback and stranding the only good + # executable at the backup path. + local_failing_file_rename(fail_on = 2, warn = TRUE) + withr::local_options(warn = 2) + + expect_error( + install_executable(fixture$from, fixture$to), + "previously compiled executable has been restored", + fixed = TRUE + ) + expect_identical(readLines(fixture$to), "old executable") +}) + +test_that("install_executable() reports a backup it could not remove", { + fixture <- local_exe_fixture() + local_mocked_bindings(unlink = function(...) 1L, .package = "base") + + # Reporting rather than signalling is the whole point: a warning here would, + # under options(warn = 2), unwind before the caller could record the state + # describing the executable that was just installed. + expect_no_warning(leftover <- install_executable(fixture$from, fixture$to)) + expect_identical(readLines(fixture$to), "new executable") + expect_true(file.exists(leftover)) + expect_identical(readLines(leftover), "old executable") +}) + test_that("repair_path() fixes slashes", { # all slashes should be single "/", and no trailing slash expect_equal(repair_path("a//b\\c/"), "a/b/c") From 6d634fccc0d1c87582bd4e87322b66b6598f23ea Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 17:40:06 -0600 Subject: [PATCH 08/34] Resolve the user header through one shared precedence rule The header precedence lived in a four-branch chain inside compile() and was insufficient in two ways. cpp_options may already have been repopulated from precompile_cpp_options_ by the time it ran, so an explicit user_header = NULL still selected an inherited USER_HEADER; and cmdstan_model(compile = FALSE) never enters compile() at all, so constructing a model with an explicit NULL alongside a cpp_options header silently kept the header and built with it. The precedence is now a small pure resolver called from both initialize() and compile(). An explicit non-NULL argument wins; an explicit NULL clears both cpp_options spellings; only an omitted argument consults cpp_options and then the stored header. Supplied-ness is captured before anything is reassigned, since user_header = NULL is also the default and cannot otherwise be told from an omitted argument -- with missing() in compile() and, for arguments arriving through ..., with names(), which list(...) preserves for NULL entries. Warnings are emitted at the call sites so a model compiled at construction warns once rather than twice. Both spellings are reduced to the one actually used, so $cpp_options() no longer reports the ignored duplicate. A header changing identity now forces compilation, through a dirty flag rather than by inferring it from cpp_options_: a compile through the lowercase spelling never leaves USER_HEADER behind, stored options are WSL-safe paths while user_header is deliberately a host path, and an absent entry conflates "no header" with "unknown". The flag is latched rather than assigned, because on a bare retry after a failed compile the reuse branch resolves back to the same header and nothing looks changed. It is cleared only by a successful executable replacement. user_header_ and using_user_header_ are configuration for the next invocation rather than a description of the executable, so they are assigned as soon as they are validated. A failed compile with a new header is usually a bug in that header, and a bare retry after fixing it must build the header the user supplied. This also stops a failed compile from leaving using_user_header_ FALSE, which made $check_syntax() report the bogus "declared without specifying a definition" error again. Shape is validated wherever a header is accepted, so character(0) is rejected informatively, while existence is checked only when compiling, keeping a header created between construction and $compile() working. --- R/cpp_opts.R | 74 +++++ R/model.R | 89 ++++-- .../testthat/test-model-compile-user_header.R | 267 +++++++++++++++--- 3 files changed, 361 insertions(+), 69 deletions(-) diff --git a/R/cpp_opts.R b/R/cpp_opts.R index 5f4cd4cf5..ad97a99b1 100644 --- a/R/cpp_opts.R +++ b/R/cpp_opts.R @@ -144,6 +144,80 @@ validate_cpp_options <- function(cpp_options) { cpp_options } +# user headers --------------------------------------------------------- +# Decide which user header a compilation should use and reduce cpp_options to a +# single, unambiguous source for it. +# +# Precedence: +# 1. an explicit non-NULL `user_header` argument; +# 2. an explicit `user_header = NULL`, which clears any header carried in +# cpp_options as well; +# 3. only when the argument is omitted, cpp_options -- USER_HEADER ahead of +# user_header whichever order they appear in -- and then `previous`, the +# header the model already holds. +# +# `supplied` is what makes (2) expressible at all: `user_header = NULL` is also +# the default, so the value alone cannot separate "cleared" from "not +# mentioned". `cpp_options_supplied` separates a header passed in the same call, +# a conflict worth warning about, from one inherited from an earlier call. +# +# Both spellings are always dropped from cpp_options; callers reinsert the +# selected header under `spelling`, in whatever form they store. The header is +# returned as supplied, neither made absolute nor WSL-safe, because callers +# differ on which they need. +resolve_user_header <- function(user_header, + supplied, + cpp_options, + cpp_options_supplied = TRUE, + previous = NULL) { + from_upper <- cpp_options[["USER_HEADER"]] + from_lower <- cpp_options[["user_header"]] + conflict <- NULL + spelling <- "USER_HEADER" + + if (supplied) { + if (cpp_options_supplied && (!is.null(from_upper) || !is.null(from_lower))) { + conflict <- "argument" + } + header <- user_header + } else if (!is.null(from_upper)) { + if (!is.null(from_lower)) { + conflict <- "cpp_options" + } + header <- from_upper + } else if (!is.null(from_lower)) { + header <- from_lower + spelling <- "user_header" + } else { + header <- previous + } + + # Shape is checked wherever a header is accepted; whether it exists is checked + # only when compiling, so that a header created between construction and + # $compile() still works. + if (!is.null(header)) { + checkmate::assert_string(header, .var.name = "user_header") + } + cpp_options[["USER_HEADER"]] <- NULL + cpp_options[["user_header"]] <- NULL + + list( + user_header = header, + spelling = spelling, + cpp_options = cpp_options, + conflict = conflict + ) +} + +warn_user_header_conflict <- function(conflict) { + if (identical(conflict, "argument")) { + warning("User header specified both via user_header argument and via cpp_options arguments") + } else if (identical(conflict, "cpp_options")) { + warning('User header specified both via cpp_options[["USER_HEADER"]] and cpp_options[["user_header"]].', call. = FALSE) + } + invisible(NULL) +} + # check specific options for validity --------------------------------- cpp_option_value <- function(cpp_options, option) { # CmdStanR input and executable metadata can use different casing. Prefer diff --git a/R/model.R b/R/model.R index b9df5dd10..ca1d0ed53 100644 --- a/R/model.R +++ b/R/model.R @@ -244,6 +244,10 @@ CmdStanModel <- R6::R6Class( include_paths_ = NULL, user_header_ = NULL, using_user_header_ = FALSE, + # Neither configuration nor a description of the executable: a record that + # the two have drifted apart. Set by a change of header, cleared only by a + # successful executable replacement. + user_header_dirty_ = FALSE, precompile_cpp_options_ = NULL, precompile_stanc_options_ = NULL, precompile_include_paths_ = NULL, @@ -263,13 +267,26 @@ CmdStanModel <- R6::R6Class( 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() private$precompile_stanc_options_ <- assert_valid_stanc_options(args$stanc_options) %||% list() - if (!is.null(args$user_header) || !is.null(args$cpp_options[["USER_HEADER"]]) || - !is.null(args$cpp_options[["user_header"]])) { - private$using_user_header_ <- TRUE + # The same precedence $compile() applies, run here too: a model created + # with compile = FALSE never enters $compile(), so resolving the paths + # alone would let an explicit user_header = NULL be ignored. `...` + # preserves NULL entries, so names() is the missing() equivalent here. + resolved_header <- resolve_user_header( + user_header = args$user_header, + supplied = "user_header" %in% names(args), + cpp_options = args$cpp_options %||% list() + ) + if (!compile) { + # Otherwise $compile() receives the original arguments below and warns + # once; warning here as well would double up. + warn_user_header_conflict(resolved_header$conflict) } - private$user_header_ <- args$user_header + private$precompile_cpp_options_ <- resolved_header$cpp_options + # Prepopulating this also keeps the first $compile() from seeing a + # change of header and rebuilding an already-current executable. + private$user_header_ <- resolve_path(resolved_header$user_header) + private$using_user_header_ <- !is.null(resolved_header$user_header) if (is.null(args$include_paths) && any(grepl("#include" , private$stan_code_))) { private$precompile_include_paths_ <- dirname(private$stan_file_) } else { @@ -584,6 +601,11 @@ compile <- function(quiet = TRUE, ) } assert_stan_file_exists(self$stan_file()) + # Captured before either is reassigned below: an explicit user_header = NULL + # is indistinguishable from an omitted argument by value alone, and a header + # inherited from an earlier call is not a conflict worth warning about. + user_header_supplied <- !missing(user_header) + cpp_options_supplied <- length(cpp_options) > 0 if (length(cpp_options) == 0 && !is.null(private$precompile_cpp_options_)) { cpp_options <- private$precompile_cpp_options_ } @@ -617,41 +639,44 @@ compile <- function(quiet = TRUE, stanc_options[["use-opencl"]] <- TRUE } - # Note that unlike cpp_options["USER_HEADER"], the user_header variable is deliberately - # not transformed with wsl_safe_path() as that breaks the check below on WSLv1 - if (!is.null(user_header)) { - if (!is.null(cpp_options[["USER_HEADER"]]) || !is.null(cpp_options[["user_header"]])) { - warning("User header specified both via user_header argument and via cpp_options arguments") - } - - cpp_options[["USER_HEADER"]] <- wsl_safe_path(absolute_path(user_header)) - } else if (!is.null(cpp_options[["USER_HEADER"]])) { - if (!is.null(cpp_options[["user_header"]])) { - warning('User header specified both via cpp_options[["USER_HEADER"]] and cpp_options[["user_header"]].', call. = FALSE) - } - - user_header <- cpp_options[["USER_HEADER"]] - cpp_options[["USER_HEADER"]] <- wsl_safe_path(absolute_path(cpp_options[["USER_HEADER"]])) - } else if (!is.null(cpp_options[["user_header"]])) { - user_header <- cpp_options[["user_header"]] - cpp_options[["user_header"]] <- wsl_safe_path(absolute_path(cpp_options[["user_header"]])) - } else if (!is.null(private$user_header_)) { + resolved_header <- resolve_user_header( + user_header = user_header, + supplied = user_header_supplied, + cpp_options = cpp_options, + cpp_options_supplied = cpp_options_supplied, # the header the model was last compiled with, or the one supplied to # cmdstan_model() if it has not been compiled yet - user_header <- private$user_header_ - cpp_options[["USER_HEADER"]] <- wsl_safe_path(absolute_path(user_header)) - } - + previous = private$user_header_ + ) + warn_user_header_conflict(resolved_header$conflict) + user_header <- resolved_header$user_header + cpp_options <- resolved_header$cpp_options using_user_header <- !is.null(user_header) if (using_user_header) { stanc_options[["allow-undefined"]] <- TRUE - user_header <- absolute_path(user_header) # As mentioned above, just absolute, not wsl_safe_path() + # Note that unlike cpp_options["USER_HEADER"], the user_header variable is + # deliberately not transformed with wsl_safe_path() as that breaks the check + # below on WSLv1 + user_header <- resolve_path(user_header) if (!file.exists(user_header)) { stop(paste0("User header file '", user_header, "' does not exist."), call. = FALSE) } + cpp_options[[resolved_header$spelling]] <- wsl_safe_path(user_header) } + # Source configuration, so assigned eagerly: it says what the next stanc or + # make invocation should use, and a failed compile must not revert it. The + # usual route to a failed compile with a new header is a bug in that header, + # and a bare retry after fixing it has to build the header the user supplied. + # + # The divergence marker is latched rather than assigned, because on that retry + # the reuse branch resolves back to the same header and nothing looks changed. + private$user_header_dirty_ <- isTRUE(private$user_header_dirty_) || + !same_path(user_header, private$user_header_) + private$user_header_ <- user_header + private$using_user_header_ <- using_user_header + # The resolved destination need not be the executable this object describes, # e.g. $compile(dir = ) aimed at a directory that already holds a current # executable. Adopting that binary while keeping this object's generated C++ @@ -662,12 +687,15 @@ compile <- function(quiet = TRUE, # - the user forced compilation, # - the executable does not exist # - the destination is not the executable this object already describes + # - the user header in use is not the one the executable was built against # - the stan model was changed since last compilation # - a user header is used and the user header changed since last compilation (#813) if (!file.exists(exe)) { force_recompile <- TRUE } else if (exe_changed) { force_recompile <- TRUE + } else if (isTRUE(private$user_header_dirty_)) { + force_recompile <- TRUE } else if (file.exists(self$stan_file()) && file.mtime(exe) < file.mtime(self$stan_file())) { force_recompile <- TRUE @@ -831,8 +859,7 @@ compile <- function(quiet = TRUE, self$functions$existing_exe <- FALSE private$stan_code_ <- stan_code private$variables_ <- NULL - private$using_user_header_ <- using_user_header - private$user_header_ <- user_header + private$user_header_dirty_ <- FALSE private$hpp_file_ <- hpp_file private$model_methods_env_ <- model_methods_env private$precompile_cpp_options_ <- NULL diff --git a/tests/testthat/test-model-compile-user_header.R b/tests/testthat/test-model-compile-user_header.R index 03b409aee..47b6c5e16 100644 --- a/tests/testthat/test-model-compile-user_header.R +++ b/tests/testthat/test-model-compile-user_header.R @@ -1,3 +1,31 @@ +local_mocked_stanc <- function(.local_envir = parent.frame()) { + local_mocked_bindings( + get_cmdstan_flags = function(flag_name) character(), + get_standalone_hpp = function(stan_file, stancflags) "", + .env = .local_envir + ) +} + +# A mocked compile installs an executable, so anything that compiles for real +# (even with a mocked compiler) works on a temporary copy rather than writing +# into the package's test resources. +local_external_model <- function(.local_envir = parent.frame()) { + stan_file <- file.path( + withr::local_tempdir(.local_envir = .local_envir), + "bernoulli_external.stan" + ) + file.copy(testing_stan_file("bernoulli_external"), stan_file) + stan_file +} + +user_header_routes <- function(header) { + list( + list(user_header = header), + list(cpp_options = list(USER_HEADER = header)), + list(cpp_options = list(user_header = header)) + ) +} + # This test is deliberately placed above the file-level skip_if(os_is_macos()) # below: it mocks the stanc call and never compiles, so it needs no toolchain # and should run on every platform. @@ -138,6 +166,173 @@ test_that("compile() uses a user header supplied to cmdstan_model()", { ) }) +test_that("cmdstan_model() records a user header from every supply route", { + header <- withr::local_tempfile(lines = "", fileext = ".hpp") + + for (route in user_header_routes(header)) { + model <- do.call( + cmdstan_model, + c(list(testing_stan_file("bernoulli_external"), compile = FALSE), route) + ) + private <- model$.__enclos_env__$private + expect_equal(private$user_header_, resolve_path(header)) + expect_true(private$using_user_header_) + expect_false(private$user_header_dirty_) + } +}) + +test_that("cmdstan_model() honours an explicit user_header = NULL", { + header <- withr::local_tempfile(lines = "", fileext = ".hpp") + + expect_warning( + model <- cmdstan_model( + testing_stan_file("bernoulli_external"), + compile = FALSE, + user_header = NULL, + cpp_options = list(USER_HEADER = header) + ), + "User header specified both" + ) + + private <- model$.__enclos_env__$private + expect_null(private$user_header_) + expect_false(private$using_user_header_) + expect_null(private$precompile_cpp_options_[["USER_HEADER"]]) + expect_null(private$precompile_cpp_options_[["user_header"]]) +}) + +test_that("cmdstan_model() rejects an empty user header", { + expect_error( + cmdstan_model( + testing_stan_file("bernoulli_external"), + compile = FALSE, + user_header = character(0) + ), + "user_header" + ) + model <- cmdstan_model(testing_stan_file("bernoulli_external"), compile = FALSE) + expect_error(model$compile(user_header = character(0)), "user_header") +}) + +test_that("a relative cpp_options user header survives a directory change", { + model_dir <- withr::local_tempdir() + file.copy(testing_stan_file("bernoulli_external"), model_dir) + writeLines("", file.path(model_dir, "header.hpp")) + local_mocked_stanc() + + model <- withr::with_dir( + model_dir, + cmdstan_model( + "bernoulli_external.stan", + compile = FALSE, + cpp_options = list(USER_HEADER = "header.hpp") + ) + ) + + expect_equal( + normalizePath(model$.__enclos_env__$private$user_header_), + normalizePath(file.path(model_dir, "header.hpp")) + ) + # The compile happens from the test's own working directory. + expect_no_error(model$compile(force_recompile = TRUE, dry_run = TRUE)) +}) + +test_that("a bare retry after a failed compile keeps the newly supplied header", { + stan_file <- local_external_model() + h1 <- withr::local_tempfile(lines = "", fileext = ".hpp") + h2 <- withr::local_tempfile(lines = "", fileext = ".hpp") + local_mocked_stanc() + model <- cmdstan_model(stan_file, compile = FALSE) + private <- model$.__enclos_env__$private + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = model$compile(user_header = h1, force_recompile = TRUE) + ) + expect_equal(private$user_header_, resolve_path(h1)) + expect_false(private$user_header_dirty_) + + # The usual route to this is a bug in h2 itself, so the header the user just + # supplied has to survive the failure. + with_mocked_cli( + compile_ret = list(status = 1), + info_ret = list(status = 1), + code = expect_error(model$compile(user_header = h2), "An error occurred") + ) + expect_equal(private$user_header_, resolve_path(h2)) + expect_true(private$user_header_dirty_) + + # A bare retry must build h2 rather than reverting to h1 or no-op'ing: the + # reuse branch resolves back to h2, so nothing here looks like a change. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_mock_compile(model$compile()) + ) + expect_equal(private$user_header_, resolve_path(h2)) + expect_false(private$user_header_dirty_) + expect_equal( + model$cpp_options()[["USER_HEADER"]], + wsl_safe_path(resolve_path(h2)) + ) +}) + +test_that("changing the user header forces compilation", { + stan_file <- local_external_model() + h1 <- withr::local_tempfile(lines = "", fileext = ".hpp") + h2 <- withr::local_tempfile(lines = "", fileext = ".hpp") + local_mocked_stanc() + model <- cmdstan_model(stan_file, compile = FALSE) + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = model$compile(user_header = h1, force_recompile = TRUE) + ) + # Older than the executable, so only the change of header identity can force + # a rebuild here (#813 only covers a header that was modified in place). + Sys.setFileTime(h2, file.mtime(model$exe_file()) - 60) + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_mock_compile(model$compile(user_header = h2)) + ) + expect_equal( + model$cpp_options()[["USER_HEADER"]], + wsl_safe_path(resolve_path(h2)) + ) +}) + +test_that("user_header = NULL clears a header from every supply route", { + header <- withr::local_tempfile(lines = "", fileext = ".hpp") + local_mocked_stanc() + + for (route in user_header_routes(header)) { + model <- cmdstan_model(local_external_model(), compile = FALSE) + private <- model$.__enclos_env__$private + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = do.call(model$compile, c(route, list(force_recompile = TRUE))) + ) + expect_true(private$using_user_header_) + + # The executable is up to date but was built against a header the model no + # longer uses, so clearing has to force a rebuild rather than no-op. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_mock_compile(model$compile(user_header = NULL)) + ) + expect_null(private$user_header_) + expect_false(private$using_user_header_) + expect_null(model$cpp_options()[["USER_HEADER"]]) + expect_null(model$cpp_options()[["user_header"]]) + } +}) + skip_if(os_is_macos()) w_path <- function(f) { @@ -192,7 +387,10 @@ test_that("cmdstan_model works with user_header with mock", { with_mocked_cli( compile_ret = list(status = 0), - info_ret = list(), + # The mocked compile installs an executable, so the constructor queries it + # for compilation info; report a failure rather than an empty list, which + # model_compile_info() cannot interpret. + info_ret = list(status = 1), code = expect_mock_compile({ mod_2 <- cmdstan_model( stan_file = testing_stan_file("bernoulli_external"), @@ -205,8 +403,8 @@ test_that("cmdstan_model works with user_header with mock", { # Check recompilation upon changing header exe_mtime <- header_mtime + 10 - # Mocked compile does not create the executable that real compilation writes. - file.create(file_that_exists) + # The mocked compile above installed the executable with a fresh mtime; pin it + # so the up-to-date check below compares against a known value. Sys.setFileTime(file_that_exists, exe_mtime) with_mocked_cli( compile_ret = list(status = 0), @@ -226,8 +424,6 @@ test_that("cmdstan_model works with user_header with mock", { }) ) - # Mocked compile does not create the executable that real compilation writes. - file.create(mod$exe_file()) Sys.setFileTime(mod$exe_file(), header_mtime + 10) # make exe newer than header # Alternative spec of user header @@ -353,85 +549,80 @@ test_that("user_header precedence order is correct", { .local_envir = parent.frame(3) )) + local_mocked_stanc() + # Asserted after a mocked successful compile rather than a dry run: only a + # compilation that produced an executable records the options describing it. + # The ignored spelling is dropped in every case, so the next compile has a + # single source for the header. + # Case # 1: all 3 specified + mod <- cmdstan_model(local_external_model(), compile = FALSE) with_mocked_cli( - compile_ret = list(status = 1), - info_ret = list(), + compile_ret = list(status = 0), + info_ret = list(status = 1), code = expect_warning({ - mod <- cmdstan_model( - stan_file = testing_stan_file("bernoulli_external"), + mod$compile( user_header = tmp_files[1], cpp_options = list( USER_HEADER = tmp_files[2], user_header = tmp_files[3] ), - dry_run = TRUE + force_recompile = TRUE ) }, "User header specified both") ) # In this case: # cpp_options[['USER_HEADER']] == tmp_files[1] <- actually used - # cpp_options[['user_header']] == tmp_files[3] <- ignored - # tmp_files[2] is not stored + # tmp_files[2] and tmp_files[3] are not stored expect_equal( match(!!(mod$cpp_options()[['USER_HEADER']]), w_path(tmp_files)), 1 ) - expect_equal( - match(!!(mod$cpp_options()[['user_header']]), tmp_files), - 3 - ) + expect_null(mod$cpp_options()[['user_header']]) # Case # 2: Both opts, but no arg + mod <- cmdstan_model(local_external_model(), compile = FALSE) with_mocked_cli( - compile_ret = list(status = 1), - info_ret = list(), + compile_ret = list(status = 0), + info_ret = list(status = 1), code = expect_warning({ - mod <- cmdstan_model( - stan_file = testing_stan_file("bernoulli_external"), + mod$compile( cpp_options = list( USER_HEADER = tmp_files[2], user_header = tmp_files[3] ), - dry_run = TRUE + force_recompile = TRUE ) }, "User header specified both") ) # In this case: - # cpp_options[['USER_HEADER']] == tmp_files[2] - # cpp_options[['user_header']] == tmp_files[3] - # tmp_files[2] is not stored + # cpp_options[['USER_HEADER']] == tmp_files[2] <- actually used + # tmp_files[3] is not stored expect_equal( match(!!(mod$cpp_options()[['USER_HEADER']]), w_path(tmp_files)), 2 ) - expect_equal( - match(!!(mod$cpp_options()[['user_header']]), tmp_files), - 3 - ) + expect_null(mod$cpp_options()[['user_header']]) # Case # 3: Both opts, other order + mod <- cmdstan_model(local_external_model(), compile = FALSE) with_mocked_cli( - compile_ret = list(status = 1), - info_ret = list(), + compile_ret = list(status = 0), + info_ret = list(status = 1), code = expect_warning({ - mod <- cmdstan_model( - stan_file = testing_stan_file("bernoulli_external"), + mod$compile( cpp_options = list( user_header = tmp_files[3], USER_HEADER = tmp_files[2] ), - dry_run = TRUE + force_recompile = TRUE ) }, "User header specified both") ) - # Same as Case #2 + # Same as Case #2: USER_HEADER wins whichever order the two appear in expect_equal( match(!!(mod$cpp_options()[['USER_HEADER']]), w_path(tmp_files)), 2 ) - expect_equal( - match(!!(mod$cpp_options()[['user_header']]), tmp_files), - 3 - ) + expect_null(mod$cpp_options()[['user_header']]) }) From e780856743b8a32df2e4a9856986d0ea58a57e0e Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 17:40:27 -0600 Subject: [PATCH 09/34] Commit cpp_options only on a real compilation A dry run builds nothing, so it no longer records cpp_options_ or moves hpp_file_; the latter previously pointed $hpp_file() at a temporary file the dry run never wrote. exe_file_ and cmdstan_version_ stay in the tail, commented as the deliberate exceptions: during a dry run they are also the configured destination and the toolchain version, so they are assigned on dry runs and on success but never on a failure. cmdstan_version() is now evaluated into a local before any compilation work rather than after the executable is installed. It is not infallible despite being an accessor: set_cmdstan_path() stores PATH and VERSION together, and when read_cmdstan_version() returns NULL the guard falls through and leaves PATH set with VERSION NULL. In that state stanc and make both run and only this call errors. It cannot be hoisted any higher, since an ordinary no-op returns before ever reaching it and would gain a failure mode it does not have today. If discarding the staged candidate fails while another error is being raised, the diagnostic now names the leftover path instead of implying it was removed. The two tests that asserted on $cpp_options() after a dry run move to mocked successful compiles, and the header precedence test asserts that the ignored spelling is dropped rather than retained. --- NEWS.md | 24 ++++++++++ R/model.R | 23 ++++++++-- R/utils.R | 20 ++++++-- man/model-method-compile.Rd | 7 ++- .../testthat/test-model-compile-user_header.R | 46 +++++++++++-------- tests/testthat/test-model-compile.R | 6 +++ 6 files changed, 99 insertions(+), 27 deletions(-) diff --git a/NEWS.md b/NEWS.md index 68d47cda3..514e0df29 100644 --- a/NEWS.md +++ b/NEWS.md @@ -40,6 +40,30 @@ failed because those inputs were dropped. (#1234) * A `user_header` supplied to `cmdstan_model()` is now used by a later `$compile()`. Previously it was only honored when the model was compiled immediately. (#1234) +* `$compile()` now accepts `user_header = NULL` to compile without a user +header. Previously a header, once supplied, could not be removed. (#1235) +* `$compile()` now recompiles when the user header changes. Previously a +different header was ignored if the executable was otherwise up to date. (#1235) +* `$compile()` now reduces duplicate `USER_HEADER`/`user_header` entries in +`cpp_options` to the one actually used, so `$cpp_options()` no longer reports +the ignored spelling after a successful compilation. (#1235) +* A `$compile()` call that finds the executable up to date no longer erases +`$cpp_options()`. Previously the recorded options were replaced with whatever +the call supplied, so a bare `$compile()` on a model built with +`cpp_options = list(stan_threads = TRUE)` dropped `stan_threads` and the +executable then ran single-threaded. (#1235) +* `$expose_functions()` now works after a `$compile()` call that found the +executable up to date. It previously failed with "not possible with a +pre-compiled Stan model" on a model that had compiled itself. (#1235) +* A failed compilation no longer moves `$exe_file()` or replaces the generated +C++ used by `$hpp_file()` and `fit$init_model_methods()`. Previously a failure +at the C++ stage left the old executable paired with model methods generated +from the new program. A `dry_run = TRUE` compilation likewise no longer moves +`$hpp_file()`, which previously pointed at a temporary file it never wrote. +(#1235) +* `$compile()` now errors if the newly compiled executable cannot be installed, +restoring the previous executable. Previously the replacement was unchecked, so +a failure could silently leave the model with no executable at all. (#1235) * CmdStanModel methods now correctly handle `#include` directories with spaces in their paths. (#820) * `$include_paths()` now returns absolute paths, and relative include paths are diff --git a/R/model.R b/R/model.R index ca1d0ed53..0fd9b809e 100644 --- a/R/model.R +++ b/R/model.R @@ -496,10 +496,13 @@ NULL #' 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. If `$compile()` is called again without `include_paths`, the -#' paths used for the previous compilation are reused. +#' most recently supplied paths are reused. #' @param user_header (string) The path to a C++ file (with a .hpp extension) #' to compile with the Stan model. If `$compile()` is called again without -#' `user_header`, the header used for the previous compilation is reused. +#' `user_header`, the most recently supplied header is reused, and changing +#' it forces recompilation. Pass `user_header = NULL` to compile without one. +#' A header can also be supplied via `cpp_options` as `USER_HEADER` or +#' `user_header`; the `user_header` argument takes precedence over both. #' @param cpp_options (list) Any makefile options to be used when compiling the #' model (`stan_threads`, `stan_mpi`, `stan_opencl`, etc.). Anything you would #' otherwise write in the `make/local` file. For an example of using threading @@ -738,6 +741,14 @@ compile <- function(quiet = TRUE, } } + # Evaluated here rather than in the tail: cmdstan_version() is not infallible + # despite being an accessor, because set_cmdstan_path() can leave PATH set + # while VERSION stays NULL, and in that state stanc and make both run but this + # errors. Staging it removes a failure point after the executable is already + # installed. It cannot be hoisted above the no-op return, which today never + # reaches this call at all. + compiled_cmdstan_version <- cmdstan_version() + if (os_is_wsl() && (compile_model_methods || compile_standalone)) { warning("Additional model methods and standalone functions are not ", "currently available with WSLv1 CmdStan and will not be compiled.", @@ -862,6 +873,7 @@ compile <- function(quiet = TRUE, private$user_header_dirty_ <- FALSE private$hpp_file_ <- hpp_file private$model_methods_env_ <- model_methods_env + private$cpp_options_ <- cpp_options private$precompile_cpp_options_ <- NULL private$precompile_stanc_options_ <- NULL private$precompile_include_paths_ <- NULL @@ -871,9 +883,12 @@ compile <- function(quiet = TRUE, } } # End - if(!dry_run) - private$cmdstan_version_ <- cmdstan_version() + # Both are exceptions to the rule that state describing the compiled artifact + # is committed only above: during a dry run they are also the configured + # destination and the toolchain version, so they are assigned on a dry run and + # on success -- and, for exe_file_, on a no-op -- but never on a failure. + private$cmdstan_version_ <- compiled_cmdstan_version private$exe_file_ <- exe - private$cpp_options_ <- cpp_options if (!dry_run) { if (compile_model_methods) { diff --git a/R/utils.R b/R/utils.R index fa28e50f1..77d70ae11 100644 --- a/R/utils.R +++ b/R/utils.R @@ -303,6 +303,17 @@ copy_temp_files <- #' describing it. install_executable <- function(from, to) { candidate <- tempfile(pattern = "exe-new-", tmpdir = dirname(to)) + # Discarding the staged copy can fail too, so the diagnostics say where it was + # left rather than implying it is gone and sending the user looking for a file + # that is still there. + discard_candidate <- function() { + if (unlink(candidate) == 0L) { + "" + } else { + paste0(" The staged copy has been left at '", candidate, "'.") + } + } + if (!isTRUE(suppressWarnings(file.copy(from, candidate)))) { stop( "Could not stage the compiled executable at '", candidate, "'. ", @@ -317,10 +328,10 @@ install_executable <- function(from, to) { error_on_status = FALSE ) if (is.na(chmod$status) || chmod$status != 0) { - unlink(candidate) stop( "Could not make the compiled executable executable. ", "The model executable at '", to, "' was not modified.", + discard_candidate(), call. = FALSE ) } @@ -330,20 +341,21 @@ install_executable <- function(from, to) { if (file.exists(to)) { backup <- tempfile(pattern = "exe-old-", tmpdir = dirname(to)) if (!isTRUE(suppressWarnings(file.rename(to, backup)))) { - unlink(candidate) stop( "Could not move the existing executable '", to, "' aside. ", "It was not modified.", + discard_candidate(), call. = FALSE ) } } if (!isTRUE(suppressWarnings(file.rename(candidate, to)))) { - unlink(candidate) + leftover_candidate <- discard_candidate() if (is.null(backup)) { stop( "Could not install the compiled executable at '", to, "'.", + leftover_candidate, call. = FALSE ) } @@ -352,12 +364,14 @@ install_executable <- function(from, to) { "Could not install the compiled executable at '", to, "' and the ", "previously compiled executable could not be restored. It has been ", "kept at '", backup, "'.", + leftover_candidate, call. = FALSE ) } stop( "Could not install the compiled executable at '", to, "'. ", "The previously compiled executable has been restored.", + leftover_candidate, call. = FALSE ) } diff --git a/man/model-method-compile.Rd b/man/model-method-compile.Rd index 030831b07..f262efbd7 100644 --- a/man/model-method-compile.Rd +++ b/man/model-method-compile.Rd @@ -41,11 +41,14 @@ program. Relative paths are resolved against the working directory when the model object is created (or when \verb{$compile()} is called) and stored as absolute paths, so subsequent changes to the working directory do not affect them. If \verb{$compile()} is called again without \code{include_paths}, the -paths used for the previous compilation are reused.} +most recently supplied paths are reused.} \item{user_header}{(string) The path to a C++ file (with a .hpp extension) to compile with the Stan model. If \verb{$compile()} is called again without -\code{user_header}, the header used for the previous compilation is reused.} +\code{user_header}, the most recently supplied header is reused, and changing +it forces recompilation. Pass \code{user_header = NULL} to compile without one. +A header can also be supplied via \code{cpp_options} as \code{USER_HEADER} or +\code{user_header}; the \code{user_header} argument takes precedence over both.} \item{cpp_options}{(list) Any makefile options to be used when compiling the model (\code{stan_threads}, \code{stan_mpi}, \code{stan_opencl}, etc.). Anything you would diff --git a/tests/testthat/test-model-compile-user_header.R b/tests/testthat/test-model-compile-user_header.R index 47b6c5e16..317a04bb2 100644 --- a/tests/testthat/test-model-compile-user_header.R +++ b/tests/testthat/test-model-compile-user_header.R @@ -142,7 +142,6 @@ test_that("a no-op compile preserves a header supplied via cpp_options", { }) test_that("compile() uses a user header supplied to cmdstan_model()", { - stan_file <- testing_stan_file("bernoulli_external") user_header <- withr::local_tempfile(lines = "", fileext = ".hpp") received_stancflags <- list() local_mocked_bindings( @@ -153,8 +152,18 @@ test_that("compile() uses a user header supplied to cmdstan_model()", { } ) - model <- cmdstan_model(stan_file, user_header = user_header, compile = FALSE) - model$compile(force_recompile = TRUE, dry_run = TRUE) + model <- cmdstan_model( + local_external_model(), + user_header = user_header, + compile = FALSE + ) + # A mocked compile rather than a dry run: a dry run builds nothing, so it + # records nothing about a compiled artifact. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = model$compile(force_recompile = TRUE) + ) expect_equal( model$cpp_options()[["USER_HEADER"]], @@ -482,15 +491,18 @@ test_that("cmdstan_model works with user_header with mock", { test_that("wsl path conversion is done as expected", { tmp_file <- withr::local_tempfile(lines = hpp, fileext = ".hpp") + local_mocked_stanc() + # Mocked successful compiles rather than dry runs: only a compilation that + # produced an executable records the options describing it. + # Case 1: arg with_mocked_cli( - compile_ret = list(status = 1), - info_ret = list(), + compile_ret = list(status = 0), + info_ret = list(status = 1), code = { mod <- cmdstan_model( - stan_file = testing_stan_file("bernoulli_external"), - user_header = tmp_file, - dry_run = TRUE + stan_file = local_external_model(), + user_header = tmp_file ) } ) @@ -502,15 +514,14 @@ test_that("wsl path conversion is done as expected", { # Case 2: cpp opt USER_HEADER with_mocked_cli( - compile_ret = list(status = 1), - info_ret = list(), + compile_ret = list(status = 0), + info_ret = list(status = 1), code = { mod <- cmdstan_model( - stan_file = testing_stan_file("bernoulli_external"), + stan_file = local_external_model(), cpp_options = list( USER_HEADER = tmp_file - ), - dry_run = TRUE + ) ) } ) @@ -522,15 +533,14 @@ test_that("wsl path conversion is done as expected", { # Case # 3: only user_header opt with_mocked_cli( - compile_ret = list(status = 1), - info_ret = list(), + compile_ret = list(status = 0), + info_ret = list(status = 1), code = { mod <- cmdstan_model( - stan_file = testing_stan_file("bernoulli_external"), + stan_file = local_external_model(), cpp_options = list( user_header = tmp_file - ), - dry_run = TRUE + ) ) } ) diff --git a/tests/testthat/test-model-compile.R b/tests/testthat/test-model-compile.R index 7056daa19..af7209ceb 100644 --- a/tests/testthat/test-model-compile.R +++ b/tests/testthat/test-model-compile.R @@ -692,9 +692,15 @@ test_that("*hpp_file() functions work", { expect_equal(mod$hpp_file(), file.path(dirname(mod$stan_file()), "bernoulli.hpp")) mod$save_hpp_file(tmp_dir) expect_equal(mod$hpp_file(), file.path(tmp_dir, "bernoulli.hpp")) + # A dry run generates no C++, so it leaves the saved location alone rather + # than pointing $hpp_file() at a temporary file that was never written. mod$compile(force_recompile = TRUE, dry_run = TRUE) + expect_equal(mod$hpp_file(), file.path(tmp_dir, "bernoulli.hpp")) + # A real recompilation does write it, to a fresh temporary location. + expect_call_compilation(mod$compile(force_recompile = TRUE)) expect_false(isTRUE(all.equal(mod$hpp_file(), file.path(tmp_dir, "bernoulli.hpp")))) expect_false(isTRUE(all.equal(mod$hpp_file(), file.path(dirname(mod$stan_file()), "bernoulli.hpp")))) + checkmate::expect_file_exists(mod$hpp_file()) }) test_that("check_syntax() works", { From 2362ab2c432887c9bfa9018bfd68d250486b788e Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 17:44:45 -0600 Subject: [PATCH 10/34] Document why the stored options carry no user header The resolver strips both header spellings from cpp_options and only $compile() reinserts the selected one, so precompile_cpp_options_ never carries a header. That is deliberate but not self-evident: storing it there would store a WSL-safe path, which the next $compile() would then select as its user_header, and that is a host path by design because file.exists() on a WSL-safe path fails under WSLv1. Also records in NEWS that a dry run no longer sets $cpp_options(), alongside the existing note about $hpp_file(). --- NEWS.md | 6 +++--- R/model.R | 6 ++++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/NEWS.md b/NEWS.md index 514e0df29..47e142742 100644 --- a/NEWS.md +++ b/NEWS.md @@ -58,9 +58,9 @@ pre-compiled Stan model" on a model that had compiled itself. (#1235) * A failed compilation no longer moves `$exe_file()` or replaces the generated C++ used by `$hpp_file()` and `fit$init_model_methods()`. Previously a failure at the C++ stage left the old executable paired with model methods generated -from the new program. A `dry_run = TRUE` compilation likewise no longer moves -`$hpp_file()`, which previously pointed at a temporary file it never wrote. -(#1235) +from the new program. A `dry_run = TRUE` compilation likewise no longer records +`$cpp_options()` or moves `$hpp_file()`, which previously pointed at a temporary +file it never wrote. (#1235) * `$compile()` now errors if the newly compiled executable cannot be installed, restoring the previous executable. Previously the replacement was unchecked, so a failure could silently leave the model with no executable at all. (#1235) diff --git a/R/model.R b/R/model.R index 0fd9b809e..5b644ccba 100644 --- a/R/model.R +++ b/R/model.R @@ -282,6 +282,12 @@ CmdStanModel <- R6::R6Class( # once; warning here as well would double up. warn_user_header_conflict(resolved_header$conflict) } + # Deliberately without the header: the resolver strips both spellings + # and only $compile() reinserts the selected one. Storing it here would + # mean storing a WSL-safe path, which the next $compile() would pick up + # as its host-path `user_header` and fail to find on WSLv1 -- the hazard + # the comment at the resolver call in $compile() describes. user_header_ + # below is the single source instead. private$precompile_cpp_options_ <- resolved_header$cpp_options # Prepopulating this also keeps the first $compile() from seeing a # change of header and rebuilding an already-current executable. From 94493216dd882029bdf86bcfde66ca70d25e005e Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 19:29:51 -0600 Subject: [PATCH 11/34] Keep the requested options when adopting an existing executable A compile that finds the executable up to date and is adopting one it did not build described it only by what the binary reports about itself, dropping the cpp_options the call asked for. Constructing a model over an already-compiled, unthreaded executable with stan_threads = TRUE therefore left stan_threads unset, so assert_valid_threads() discarded threads_per_chain and the model ran single-threaded without the caller ever asking for that. The options are now seeded from the request and filled in from the binary. Nothing is overwritten, since an object adopting an executable holds no options yet. Describing the executable purely by its own metadata would be the more honest answer, but cmdstanr does not yet rebuild when the requested options disagree with the binary -- the tests covering that are still skipped as "to be fixed in a later version" -- so until it does, the request is part of how an adopted executable is described. --- R/model.R | 11 +++++++-- tests/testthat/test-model-recompile-logic.R | 27 +++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/R/model.R b/R/model.R index 5b644ccba..32c0e99b9 100644 --- a/R/model.R +++ b/R/model.R @@ -727,12 +727,19 @@ compile <- function(quiet = TRUE, # model_compile_info() runs the executable and errors outright rather than # returning a status when the file is not runnable. self$functions$existing_exe <- TRUE + # Seeded with the options this call asked for, then filled in from the + # binary. Nothing is overwritten, because there is nothing here yet, and + # dropping the request would silently disable a feature the caller asked + # for: cmdstanr does not yet rebuild when the requested options disagree + # with the executable (see the skipped tests in + # test-model-recompile-logic.R), so an adopted executable is described by + # the request plus whatever it reports about itself. private$cpp_options_ <- tryCatch( merge_exe_info_cpp_options( - private$cpp_options_, + cpp_options, model_compile_info(exe, self$cmdstan_version()) ), - error = function(e) private$cpp_options_ + error = function(e) cpp_options ) } else { # The flag means "we don't hold the generated C++ for this executable", diff --git a/tests/testthat/test-model-recompile-logic.R b/tests/testthat/test-model-recompile-logic.R index fa18c3221..7967bd7a9 100644 --- a/tests/testthat/test-model-recompile-logic.R +++ b/tests/testthat/test-model-recompile-logic.R @@ -125,6 +125,33 @@ test_that("a no-op compile adopts an executable the object did not build", { expect_null(mod$cpp_options()$STAN_VERSION) }) +test_that("adopting an executable keeps the options the call asked for", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = cmdstan_model(stan_file, force_recompile = TRUE) + ) + + # The executable is up to date but was not built with threading. Until + # cmdstanr rebuilds on a cpp_options mismatch, dropping the request would + # make assert_valid_threads() discard 'threads' and run single-threaded + # without the caller ever asking for that. + mod <- with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list( + status = 0, + stdout = "stan_version_major=2\nstan_version_minor=39\nstan_version_patch=0\nSTAN_THREADS=false" + ), + code = cmdstan_model(stan_file, cpp_options = list(stan_threads = TRUE)) + ) + + expect_true(mod$cpp_options()$stan_threads) + expect_true(mod$functions$existing_exe) +}) + test_that("a no-op compile tolerates an executable it cannot query", { stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") file.copy(stan_program, stan_file) From d2da0cba247c5cec321a4390a9664d7f4b3c806f Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 19:38:38 -0600 Subject: [PATCH 12/34] Record cpp_options supplied to a no-op compile Seeding the options from the request covered only the case where the object was adopting an executable it did not build. Calling $compile(cpp_options = list(stan_threads = TRUE)) on an object that already describes an up-to-date executable took the other branch, which preserves the recorded options and so ignored the request just the same. Both branches now share one rule: options supplied to this call are recorded, since they are the caller's declared intent and cmdstanr does not yet rebuild when they disagree with the executable; a bare $compile() supplies none and must not erase what is already recorded, which is the erasure that made a threaded executable run single-threaded. --- R/model.R | 27 ++++++++++++--------- tests/testthat/test-model-recompile-logic.R | 24 ++++++++++++++++++ 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/R/model.R b/R/model.R index 32c0e99b9..9c668f344 100644 --- a/R/model.R +++ b/R/model.R @@ -720,32 +720,35 @@ compile <- function(quiet = TRUE, } # Nothing was compiled, so nothing describing the current executable may be # consumed or overwritten by configuration this call merely proposed. + # Options supplied to this call are the caller's declared intent, so they + # are recorded even though nothing was compiled: cmdstanr does not yet + # rebuild when they disagree with the executable (see the skipped tests in + # test-model-recompile-logic.R), and dropping them would disable a feature + # that was explicitly asked for. A bare $compile() supplies none, and must + # not erase what is already recorded -- an erased stan_threads makes + # assert_valid_threads() run a threaded executable single-threaded. + recorded_cpp_options <- + if (cpp_options_supplied) cpp_options else private$cpp_options_ if (length(private$exe_file_) == 0) { # This object is adopting an executable it did not build: it holds no - # generated C++ for it, and the only available description of it is what - # the binary reports about itself. Best effort, because + # generated C++ for it, and the only description of it beyond the request + # is what the binary reports about itself. Best effort, because # model_compile_info() runs the executable and errors outright rather than # returning a status when the file is not runnable. self$functions$existing_exe <- TRUE - # Seeded with the options this call asked for, then filled in from the - # binary. Nothing is overwritten, because there is nothing here yet, and - # dropping the request would silently disable a feature the caller asked - # for: cmdstanr does not yet rebuild when the requested options disagree - # with the executable (see the skipped tests in - # test-model-recompile-logic.R), so an adopted executable is described by - # the request plus whatever it reports about itself. - private$cpp_options_ <- tryCatch( + recorded_cpp_options <- tryCatch( merge_exe_info_cpp_options( - cpp_options, + recorded_cpp_options, model_compile_info(exe, self$cmdstan_version()) ), - error = function(e) cpp_options + error = function(e) recorded_cpp_options ) } else { # The flag means "we don't hold the generated C++ for this executable", # which is not the same as "this call compiled nothing". self$functions$existing_exe <- is.null(self$functions$hpp_code) } + private$cpp_options_ <- recorded_cpp_options private$exe_file_ <- exe return(invisible(self)) } else { diff --git a/tests/testthat/test-model-recompile-logic.R b/tests/testthat/test-model-recompile-logic.R index 7967bd7a9..e9b350478 100644 --- a/tests/testthat/test-model-recompile-logic.R +++ b/tests/testthat/test-model-recompile-logic.R @@ -152,6 +152,30 @@ test_that("adopting an executable keeps the options the call asked for", { expect_true(mod$functions$existing_exe) }) +test_that("a no-op compile records options supplied to that same call", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + + mod <- with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = cmdstan_model(stan_file) + ) + expect_null(mod$cpp_options()$stan_threads) + + # Same object, same executable, but this call explicitly asks for threading. + # Preserving the recorded options is right for a bare $compile(); ignoring + # options the caller just supplied is not. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile( + mod$compile(cpp_options = list(stan_threads = TRUE)) + ) + ) + expect_true(mod$cpp_options()$stan_threads) +}) + test_that("a no-op compile tolerates an executable it cannot query", { stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") file.copy(stan_program, stan_file) From 16c7307686b06f5955d526a47f72356d76052f6d Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 20:26:22 -0600 Subject: [PATCH 13/34] Warn when the executable was not built with the requested cpp_options Supplying cpp_options to a compile that finds the executable up to date records them without rebuilding anything, so a requested stan_threads produced the "N thread(s) per chain" message while the binary, compiled without STAN_THREADS, ran single-threaded. The options were reported as though they applied. Rebuilding on a mismatch is the real fix and is still outstanding, so until then say plainly that they had no effect and point at force_recompile = TRUE. This wires up exe_info_reflects_cpp_options(), which existed and was tested but had no caller. It compares lower-case names while model_compile_info() reports upper-case ones, so feeding it the current parser's output finds no overlap and always reports agreement; the names are aligned before the comparison. The check runs only when this call supplied cpp_options and the executable could be queried, so ordinary reuse stays quiet. Tests cover both routes that reach it, a fresh object adopting an executable and a second $compile() on the object that built one, and that no warning is raised when the executable already has the requested options. --- NEWS.md | 5 ++ R/model.R | 57 +++++++++++++------ .../testthat/test-model-generate_quantities.R | 7 ++- tests/testthat/test-model-recompile-logic.R | 41 +++++++++++-- 4 files changed, 89 insertions(+), 21 deletions(-) diff --git a/NEWS.md b/NEWS.md index 47e142742..707f870b3 100644 --- a/NEWS.md +++ b/NEWS.md @@ -61,6 +61,11 @@ at the C++ stage left the old executable paired with model methods generated from the new program. A `dry_run = TRUE` compilation likewise no longer records `$cpp_options()` or moves `$hpp_file()`, which previously pointed at a temporary file it never wrote. (#1235) +* `$compile()` now warns when `cpp_options` are supplied but the existing +executable is up to date and was not built with them, so nothing is rebuilt and +the options have no effect. Previously they were recorded and reported as if +they applied, so a model could print "2 thread(s) per chain" while running +single-threaded. Use `force_recompile = TRUE` to rebuild. (#1235) * `$compile()` now errors if the newly compiled executable cannot be installed, restoring the previous executable. Previously the replacement was unchecked, so a failure could silently leave the model with no executable at all. (#1235) diff --git a/R/model.R b/R/model.R index 9c668f344..c0190e115 100644 --- a/R/model.R +++ b/R/model.R @@ -721,28 +721,53 @@ compile <- function(quiet = TRUE, # Nothing was compiled, so nothing describing the current executable may be # consumed or overwritten by configuration this call merely proposed. # Options supplied to this call are the caller's declared intent, so they - # are recorded even though nothing was compiled: cmdstanr does not yet - # rebuild when they disagree with the executable (see the skipped tests in - # test-model-recompile-logic.R), and dropping them would disable a feature - # that was explicitly asked for. A bare $compile() supplies none, and must - # not erase what is already recorded -- an erased stan_threads makes - # assert_valid_threads() run a threaded executable single-threaded. + # are recorded even though nothing was compiled. A bare $compile() supplies + # none, and must not erase what is already recorded -- an erased + # stan_threads makes assert_valid_threads() run a threaded executable + # single-threaded. recorded_cpp_options <- if (cpp_options_supplied) cpp_options else private$cpp_options_ + + # Asking the executable about itself. Best effort, because + # model_compile_info() runs it and errors outright rather than returning a + # status when the file is not runnable. + exe_info <- NULL + if (cpp_options_supplied || length(private$exe_file_) == 0) { + exe_info <- tryCatch( + model_compile_info(exe, self$cmdstan_version()), + error = function(e) NULL + ) + } + + # Recording options the executable does not have would otherwise be a quiet + # lie: nothing was rebuilt, so a requested stan_threads produces the + # "N thread(s) per chain" message while the binary, compiled without + # STAN_THREADS, runs single-threaded. Rebuilding on a mismatch is the real + # fix and is still outstanding (see the skipped tests in + # test-model-recompile-logic.R); until then, say so. + if (cpp_options_supplied && length(exe_info) > 0) { + # model_compile_info() reports upper-case names while + # exe_info_reflects_cpp_options() compares lower-case ones, so without + # aligning them the comparison finds no overlap and always agrees. + reported <- exe_info + names(reported) <- tolower(names(reported)) + if (!isTRUE(exe_info_reflects_cpp_options(reported, cpp_options))) { + warning( + "The existing executable was not built with the requested ", + "'cpp_options' and was not rebuilt, so they will have no effect. ", + "Use 'force_recompile = TRUE' to rebuild the model.", + call. = FALSE + ) + } + } + if (length(private$exe_file_) == 0) { # This object is adopting an executable it did not build: it holds no # generated C++ for it, and the only description of it beyond the request - # is what the binary reports about itself. Best effort, because - # model_compile_info() runs the executable and errors outright rather than - # returning a status when the file is not runnable. + # is what the binary reports about itself. self$functions$existing_exe <- TRUE - recorded_cpp_options <- tryCatch( - merge_exe_info_cpp_options( - recorded_cpp_options, - model_compile_info(exe, self$cmdstan_version()) - ), - error = function(e) recorded_cpp_options - ) + recorded_cpp_options <- + merge_exe_info_cpp_options(recorded_cpp_options, exe_info) } else { # The flag means "we don't hold the generated C++ for this executable", # which is not the same as "this call compiled nothing". diff --git a/tests/testthat/test-model-generate_quantities.R b/tests/testthat/test-model-generate_quantities.R index ff4b9072e..43d913be6 100644 --- a/tests/testthat/test-model-generate_quantities.R +++ b/tests/testthat/test-model-generate_quantities.R @@ -55,7 +55,12 @@ test_that("generate_quantities work for different chains and parallel_chains", { expect_gq_output( mod_gq$generate_quantities(data = data_list, fitted_params = fit, parallel_chains = 4) ) - mod_gq <- cmdstan_model(testing_stan_file("bernoulli_ppc"), cpp_options = list(stan_threads = TRUE)) + # The executable is already built without threading and is up to date, so this + # does not rebuild it and the request has no effect on the binary. + expect_warning( + mod_gq <- cmdstan_model(testing_stan_file("bernoulli_ppc"), cpp_options = list(stan_threads = TRUE)), + "was not built with the requested" + ) expect_gq_output( mod_gq$generate_quantities(data = data_list, fitted_params = fit_1_chain, threads_per_chain = 2) ) diff --git a/tests/testthat/test-model-recompile-logic.R b/tests/testthat/test-model-recompile-logic.R index e9b350478..d0fc3359c 100644 --- a/tests/testthat/test-model-recompile-logic.R +++ b/tests/testthat/test-model-recompile-logic.R @@ -139,13 +139,16 @@ test_that("adopting an executable keeps the options the call asked for", { # cmdstanr rebuilds on a cpp_options mismatch, dropping the request would # make assert_valid_threads() discard 'threads' and run single-threaded # without the caller ever asking for that. - mod <- with_mocked_cli( + with_mocked_cli( compile_ret = list(status = 0), info_ret = list( status = 0, stdout = "stan_version_major=2\nstan_version_minor=39\nstan_version_patch=0\nSTAN_THREADS=false" ), - code = cmdstan_model(stan_file, cpp_options = list(stan_threads = TRUE)) + code = expect_warning( + mod <- cmdstan_model(stan_file, cpp_options = list(stan_threads = TRUE)), + "was not built with the requested" + ) ) expect_true(mod$cpp_options()$stan_threads) @@ -168,14 +171,44 @@ test_that("a no-op compile records options supplied to that same call", { # options the caller just supplied is not. with_mocked_cli( compile_ret = list(status = 0), - info_ret = list(status = 1), + info_ret = list( + status = 0, + stdout = "stan_version_major=2\nstan_version_minor=39\nstan_version_patch=0\nSTAN_THREADS=false" + ), code = expect_no_mock_compile( - mod$compile(cpp_options = list(stan_threads = TRUE)) + expect_warning( + mod$compile(cpp_options = list(stan_threads = TRUE)), + "was not built with the requested" + ) ) ) expect_true(mod$cpp_options()$stan_threads) }) +test_that("no mismatch warning when the executable already has the options", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = mod <- cmdstan_model(stan_file, force_recompile = TRUE) + ) + + # The executable reports exactly what is being asked for, so re-stating it + # must stay quiet -- otherwise the warning fires on ordinary reuse. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list( + status = 0, + stdout = "stan_version_major=2\nstan_version_minor=39\nstan_version_patch=0\nSTAN_THREADS=true" + ), + code = expect_no_warning( + mod$compile(cpp_options = list(stan_threads = TRUE)) + ) + ) +}) + test_that("a no-op compile tolerates an executable it cannot query", { stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") file.copy(stan_program, stan_file) From 7614efd7dbc1bf37bca56d74a76c35c64af8e890 Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 22:10:03 -0600 Subject: [PATCH 14/34] Normalize path separators in the executable-install snapshots The snapshot transforms matched the fixture directory literally, which holds only on platforms with one path separator. On Windows the paths in these diagnostics arrive with a mixture: dirname() converts to forward slashes while withr::local_tempdir() and tempfile() use backslashes, so tempfile(tmpdir = dirname(to)) produces "C:/a/b\exe-new-1234". The directory prefix then failed to match and the staged and backup names appeared in full, and where the prefix did match the separator in front of the random name still differed. Separators are normalized before the substitutions, which leaves the recorded snapshots unchanged on platforms that already agree. --- tests/testthat/test-model-compile.R | 5 ++++- tests/testthat/test-utils.R | 9 ++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-model-compile.R b/tests/testthat/test-model-compile.R index af7209ceb..55bc2b9b0 100644 --- a/tests/testthat/test-model-compile.R +++ b/tests/testthat/test-model-compile.R @@ -578,8 +578,11 @@ test_that("a leftover backup doesn't unwind a compile when warnings are errors", list(warn = 2), model$compile(cpp_options = list(stan_threads = TRUE), force_recompile = TRUE) ), + # Separators are normalized first: on Windows the backup path arrives as + # "\exe-old-1234", since tempfile() joins with a backslash. transform = function(lines) { - lines <- gsub(model_dir, "", lines, fixed = TRUE) + lines <- gsub("\\\\", "/", lines) + lines <- gsub(gsub("\\\\", "/", model_dir), "", lines, fixed = TRUE) gsub("exe-old-[0-9a-f]+", "exe-old-", lines) } ) diff --git a/tests/testthat/test-utils.R b/tests/testthat/test-utils.R index b68ad3e18..edf285c11 100644 --- a/tests/testthat/test-utils.R +++ b/tests/testthat/test-utils.R @@ -226,9 +226,16 @@ local_exe_fixture <- function(destination_exists = TRUE, # Normalize the random staging and backup names out of a snapshot, keeping the # structure of the paths the diagnostics name. +# +# Separators are normalized first because on Windows these paths arrive with a +# mixture: dirname() converts to forward slashes, while withr::local_tempdir() +# and tempfile() use backslashes, so tempfile(tmpdir = dirname(to)) yields +# "C:/a/b\exe-new-1234". exe_path_transform <- function(fixture) { + dir <- gsub("\\\\", "/", fixture$dir) function(lines) { - lines <- gsub(fixture$dir, "", lines, fixed = TRUE) + lines <- gsub("\\\\", "/", lines) + lines <- gsub(dir, "", lines, fixed = TRUE) gsub("exe-(new|old)-[0-9a-f]+", "exe-\\1-", lines) } } From 3cbcc59536114fe35c34741045cc13d6d9aaa8e3 Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 22:13:28 -0600 Subject: [PATCH 15/34] Clean up NEWS.md --- NEWS.md | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/NEWS.md b/NEWS.md index 707f870b3..8fc188f86 100644 --- a/NEWS.md +++ b/NEWS.md @@ -48,24 +48,16 @@ different header was ignored if the executable was otherwise up to date. (#1235) `cpp_options` to the one actually used, so `$cpp_options()` no longer reports the ignored spelling after a successful compilation. (#1235) * A `$compile()` call that finds the executable up to date no longer erases -`$cpp_options()`. Previously the recorded options were replaced with whatever -the call supplied, so a bare `$compile()` on a model built with -`cpp_options = list(stan_threads = TRUE)` dropped `stan_threads` and the -executable then ran single-threaded. (#1235) +`$cpp_options()`. (#1235) * `$expose_functions()` now works after a `$compile()` call that found the -executable up to date. It previously failed with "not possible with a -pre-compiled Stan model" on a model that had compiled itself. (#1235) +executable up to date. (#1235) * A failed compilation no longer moves `$exe_file()` or replaces the generated C++ used by `$hpp_file()` and `fit$init_model_methods()`. Previously a failure at the C++ stage left the old executable paired with model methods generated -from the new program. A `dry_run = TRUE` compilation likewise no longer records -`$cpp_options()` or moves `$hpp_file()`, which previously pointed at a temporary -file it never wrote. (#1235) +from the new program. (#1235) * `$compile()` now warns when `cpp_options` are supplied but the existing executable is up to date and was not built with them, so nothing is rebuilt and -the options have no effect. Previously they were recorded and reported as if -they applied, so a model could print "2 thread(s) per chain" while running -single-threaded. Use `force_recompile = TRUE` to rebuild. (#1235) +the options have no effect. Use `force_recompile = TRUE` to rebuild. (#1235) * `$compile()` now errors if the newly compiled executable cannot be installed, restoring the previous executable. Previously the replacement was unchecked, so a failure could silently leave the model with no executable at all. (#1235) From b0e8d259ddfd03719fb30775e78cfcb26a48f086 Mon Sep 17 00:00:00 2001 From: jgabry Date: Mon, 27 Jul 2026 23:24:53 -0600 Subject: [PATCH 16/34] Commit the executable path before the optional exposure work Removing the early self$exe_file(exe) left compile_standalone's call to expose_stan_functions() ahead of the assignment in the tail, so a failure there installed the executable and then returned an object that could not find it. A later $compile() would find that executable up to date, take the adoption branch because exe_file_ was still empty, and set existing_exe, after which $expose_functions() refused permanently. Both optional exposures now run after every field describing the installed executable is committed. The cpp_options mismatch warning moves after the no-op branch records cpp_options_ and exe_file_, for the reason the leftover-backup warning is raised last: under options(warn = 2) it is an error, and raising it earlier unwound with the object half-updated. That warning, and the decision to record the requested options at all, now key off whether options are available rather than whether they arrived with this call. Options held from cmdstan_model(compile = FALSE) are equally the caller's intent, and were being discarded; the supplied-ness flag remains for the narrower question of whether a header conflict occurred within a single call. unlink() glob-expands by default, unlike the file.remove() it replaced, so a model directory containing [, ], * or ? matched nothing and reported success while a full copy of the previous executable stayed on disk. Both call sites pass expand = FALSE. --- NEWS.md | 22 +++++++++++----------- R/model.R | 43 ++++++++++++++++++++++++++++--------------- R/utils.R | 4 ++-- 3 files changed, 41 insertions(+), 28 deletions(-) diff --git a/NEWS.md b/NEWS.md index 8fc188f86..568e0b210 100644 --- a/NEWS.md +++ b/NEWS.md @@ -13,7 +13,7 @@ variables are, instead of erroring. (#1225) * Supplying a factor for a variable not declared as `int` is now an error. (#1225) * Factors are now accepted for length-1 `int` arrays (e.g. `array[1] int x`), which previously errored. (#1225) -* The `CMDSTANR_NO_VER_CHECK` R option and environment variable are deprecated +* 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. * `$compile()` now works with named `stanc_options` values such as `canonicalize`. The values were shell-quoted for Make and the same quoted @@ -68,17 +68,17 @@ 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 +* `$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 -capitalization of C++ option names. Any executable reporting threading enabled +* CmdStanModel methods now use executable metadata regardless of the +capitalization of C++ option names. Any executable reporting threading enabled requires the corresponding `threads` or `threads_per_chain` argument. (#765, #1100) -* Pathfinder fits used as initial values now use uniform weights when CmdStan +* Pathfinder fits used as initial values now use uniform weights when CmdStan already PSIS-resampled their draws, avoiding a second application of importance weights. (#1206) -* Pathfinder fits used as initial values now correctly treat draws with different -initialization parameter values as distinct even when their log weights are equal, +* Pathfinder fits used as initial values now correctly treat draws with different +initialization parameter values as distinct even when their log weights are equal, and collapse duplicate resampled draws while retaining their selection frequency. (#1207) -* `pathfinder()` now passes separately supplied initial values to every path +* `pathfinder()` now passes separately supplied initial values to every path instead of using only the first path's initial values. (#1206) * `pathfinder()` now respects `save_single_paths = TRUE` instead of always passing `0` to CmdStan. @@ -87,10 +87,10 @@ to be consistent with other methods. * The `num_paths` documentation for `pathfinder()` now notes that running multiple paths in parallel requires compiling with `cpp_options = list(stan_threads = TRUE)` and setting `threads`. (#896) -* The `save_latent_dynamics` argument is now limited to `$sample()`, -`$sample_mpi()`, and `$variational()`, matching the CmdStan algorithms +* The `save_latent_dynamics` argument is now limited to `$sample()`, +`$sample_mpi()`, and `$variational()`, matching the CmdStan algorithms that support diagnostic CSV output. -* Informative error when exposing functions using names that are reserved +* Informative error when exposing functions using names that are reserved keywords (@VisruthSK, #1154) * `save_cmdstan_config` and `save_metric` default to `FALSE` but can be set to `TRUE` for an entire R session via new global options. (#1159) diff --git a/R/model.R b/R/model.R index c0190e115..8ee0d1200 100644 --- a/R/model.R +++ b/R/model.R @@ -618,6 +618,10 @@ compile <- function(quiet = TRUE, if (length(cpp_options) == 0 && !is.null(private$precompile_cpp_options_)) { cpp_options <- private$precompile_cpp_options_ } + # Distinct from cpp_options_supplied, which is only about whether this call + # carried a conflicting header: options held from cmdstan_model(compile = + # FALSE) did not arrive with this call but are still the caller's intent. + cpp_options_available <- length(cpp_options) > 0 if (length(stanc_options) == 0 && !is.null(private$precompile_stanc_options_)) { stanc_options <- private$precompile_stanc_options_ } @@ -726,13 +730,13 @@ compile <- function(quiet = TRUE, # stan_threads makes assert_valid_threads() run a threaded executable # single-threaded. recorded_cpp_options <- - if (cpp_options_supplied) cpp_options else private$cpp_options_ + if (cpp_options_available) cpp_options else private$cpp_options_ # Asking the executable about itself. Best effort, because # model_compile_info() runs it and errors outright rather than returning a # status when the file is not runnable. exe_info <- NULL - if (cpp_options_supplied || length(private$exe_file_) == 0) { + if (cpp_options_available || length(private$exe_file_) == 0) { exe_info <- tryCatch( model_compile_info(exe, self$cmdstan_version()), error = function(e) NULL @@ -745,20 +749,15 @@ compile <- function(quiet = TRUE, # STAN_THREADS, runs single-threaded. Rebuilding on a mismatch is the real # fix and is still outstanding (see the skipped tests in # test-model-recompile-logic.R); until then, say so. - if (cpp_options_supplied && length(exe_info) > 0) { + options_mismatch <- FALSE + if (cpp_options_available && length(exe_info) > 0) { # model_compile_info() reports upper-case names while # exe_info_reflects_cpp_options() compares lower-case ones, so without # aligning them the comparison finds no overlap and always agrees. reported <- exe_info names(reported) <- tolower(names(reported)) - if (!isTRUE(exe_info_reflects_cpp_options(reported, cpp_options))) { - warning( - "The existing executable was not built with the requested ", - "'cpp_options' and was not rebuilt, so they will have no effect. ", - "Use 'force_recompile = TRUE' to rebuild the model.", - call. = FALSE - ) - } + options_mismatch <- + !isTRUE(exe_info_reflects_cpp_options(reported, cpp_options)) } if (length(private$exe_file_) == 0) { @@ -775,6 +774,17 @@ compile <- function(quiet = TRUE, } private$cpp_options_ <- recorded_cpp_options private$exe_file_ <- exe + # Warned about only once the state above is recorded: under + # options(warn = 2) this is an error, and raising it earlier would unwind + # with the object half-updated. + if (options_mismatch) { + warning( + "The existing executable was not built with the requested ", + "'cpp_options' and was not rebuilt, so they will have no effect. ", + "Use 'force_recompile = TRUE' to rebuild the model.", + call. = FALSE + ) + } return(invisible(self)) } else { if (rlang::is_interactive()) { @@ -918,10 +928,6 @@ compile <- function(quiet = TRUE, private$precompile_cpp_options_ <- NULL private$precompile_stanc_options_ <- NULL private$precompile_include_paths_ <- NULL - - if (compile_standalone) { - expose_stan_functions(self$functions, verbose = !quiet) - } } # End - if(!dry_run) # Both are exceptions to the rule that state describing the compiled artifact @@ -932,6 +938,13 @@ compile <- function(quiet = TRUE, private$exe_file_ <- exe if (!dry_run) { + # Both exposures are optional and fallible, and both run only once every + # field describing the installed executable has been committed -- including + # exe_file_ above. Failing here must not leave the object unable to find an + # executable that is sitting on disk. + if (compile_standalone) { + expose_stan_functions(self$functions, verbose = !quiet) + } if (compile_model_methods) { expose_model_methods(env = private$model_methods_env_, verbose = !quiet) } diff --git a/R/utils.R b/R/utils.R index 77d70ae11..7bba45add 100644 --- a/R/utils.R +++ b/R/utils.R @@ -307,7 +307,7 @@ install_executable <- function(from, to) { # left rather than implying it is gone and sending the user looking for a file # that is still there. discard_candidate <- function() { - if (unlink(candidate) == 0L) { + if (unlink(candidate, expand = FALSE) == 0L) { "" } else { paste0(" The staged copy has been left at '", candidate, "'.") @@ -376,7 +376,7 @@ install_executable <- function(from, to) { ) } - if (!is.null(backup) && unlink(backup) != 0L) { + if (!is.null(backup) && unlink(backup, expand = FALSE) != 0L) { return(backup) } NULL From 03592e9ec6c68b926a4e50f6fd72526f13763bad Mon Sep 17 00:00:00 2001 From: jgabry Date: Tue, 28 Jul 2026 08:17:13 -0600 Subject: [PATCH 17/34] Repair the separators on the staged and backup executable paths tempfile() joins with a backslash on Windows, so staging beside a WSL destination produced "//wsl$/distro/path/to/dir\exe-new-1234". The Win32 calls tolerate the mixed separators, but wsl_safe_path() only rewrites the prefix, so the POSIX chmod inside WSL was handed a path that does not exist and every real compile failed. The previous code chmod'ed the destination, which had been through repair_path() already, and ignored the status besides, so this only surfaced once the staged candidate became the thing being made executable and its status was checked. Both temporary paths now go through repair_path(). That also collapses the duplicated separator withr::local_tempdir() can return, so the snapshot transforms match every spelling of the fixture directory rather than the one literal form. Also clears variables_ in format(overwrite_file = TRUE). The program on disk is rewritten and stan_code_ reloaded from it, but anything already parsed stayed cached, so $code() and $variables() could describe different programs and the fitting methods validate data and initial values against $variables(). --- R/model.R | 5 +++++ R/utils.R | 8 ++++++-- tests/testthat/test-model-compile.R | 29 ++++++++++++++++++++++++++++- tests/testthat/test-utils.R | 13 +++++++++++-- 4 files changed, 50 insertions(+), 5 deletions(-) diff --git a/R/model.R b/R/model.R index 8ee0d1200..f5c3b98e5 100644 --- a/R/model.R +++ b/R/model.R @@ -1291,6 +1291,11 @@ format <- function(overwrite_file = FALSE, cat(run_log$stdout, file = out_file, sep = "\n") if (isTRUE(overwrite_file)) { private$stan_code_ <- readLines(self$stan_file()) + # The program on disk has been rewritten, so anything parsed from it is + # stale. $variables() reparses when this is NULL; leaving it would let + # $code() and $variables() describe different programs, and the fitting + # methods validate data and inits against $variables(). (#1228) + private$variables_ <- NULL } invisible(TRUE) diff --git a/R/utils.R b/R/utils.R index 7bba45add..ac64176e7 100644 --- a/R/utils.R +++ b/R/utils.R @@ -302,7 +302,11 @@ copy_temp_files <- #' signalling from here would unwind before the caller could record the state #' describing it. install_executable <- function(from, to) { - candidate <- tempfile(pattern = "exe-new-", tmpdir = dirname(to)) + # repair_path() because tempfile() joins with a backslash on Windows, giving + # "//wsl$/distro/path/to/dir\\exe-new-1234". The Win32 calls below tolerate the + # mixed separators, but wsl_safe_path() only rewrites the prefix, so the POSIX + # chmod inside WSL would be handed a path that does not exist. + candidate <- repair_path(tempfile(pattern = "exe-new-", tmpdir = dirname(to))) # Discarding the staged copy can fail too, so the diagnostics say where it was # left rather than implying it is gone and sending the user looking for a file # that is still there. @@ -339,7 +343,7 @@ install_executable <- function(from, to) { backup <- NULL if (file.exists(to)) { - backup <- tempfile(pattern = "exe-old-", tmpdir = dirname(to)) + backup <- repair_path(tempfile(pattern = "exe-old-", tmpdir = dirname(to))) if (!isTRUE(suppressWarnings(file.rename(to, backup)))) { stop( "Could not move the existing executable '", to, "' aside. ", diff --git a/tests/testthat/test-model-compile.R b/tests/testthat/test-model-compile.R index 55bc2b9b0..32231ed1a 100644 --- a/tests/testthat/test-model-compile.R +++ b/tests/testthat/test-model-compile.R @@ -582,7 +582,9 @@ test_that("a leftover backup doesn't unwind a compile when warnings are errors", # "\exe-old-1234", since tempfile() joins with a backslash. transform = function(lines) { lines <- gsub("\\\\", "/", lines) - lines <- gsub(gsub("\\\\", "/", model_dir), "", lines, fixed = TRUE) + for (dir in unique(c(model_dir, repair_path(model_dir)))) { + lines <- gsub(dir, "", lines, fixed = TRUE) + } gsub("exe-old-[0-9a-f]+", "exe-old-", lines) } ) @@ -1107,6 +1109,31 @@ test_that("cmdstan_model cpp_options dont captialize cxxflags ", { expect_output(print(out), "-Dsomething_not_used") }) +test_that("format(overwrite_file = TRUE) refreshes cached variables", { + model_dir <- withr::local_tempdir() + stan_file <- write_stan_file( + "parameters { real alpha; } model { alpha ~ std_normal(); }", + dir = model_dir, + basename = "reformat.stan" + ) + model <- cmdstan_model(stan_file, compile = FALSE) + expect_equal(names(model$variables()$parameters), "alpha") + + # The program is edited behind the object's back, then formatted in place. + # $format() reloads $code() from disk, so a cached $variables() would go on + # describing a different program than $code() does -- and the fitting methods + # validate data and initial values against $variables(). (#1228) + writeLines( + "parameters { real beta; } model { beta ~ std_normal(); }", + stan_file + ) + model$format(overwrite_file = TRUE, quiet = TRUE) + + expect_equal(names(model$variables()$parameters), "beta") + expect_match(paste(model$code(), collapse = " "), "beta") +}) + + test_that("format() works", { code <- " parameters { diff --git a/tests/testthat/test-utils.R b/tests/testthat/test-utils.R index edf285c11..f51795681 100644 --- a/tests/testthat/test-utils.R +++ b/tests/testthat/test-utils.R @@ -232,10 +232,19 @@ local_exe_fixture <- function(destination_exists = TRUE, # and tempfile() use backslashes, so tempfile(tmpdir = dirname(to)) yields # "C:/a/b\exe-new-1234". exe_path_transform <- function(fixture) { - dir <- gsub("\\\\", "/", fixture$dir) + # Every spelling the directory can appear in: withr::local_tempdir() can + # return "/tmp//Rtmpx", file.path() keeps that, and install_executable() + # passes its own paths through repair_path(), which collapses it. + dirs <- unique(c( + fixture$dir, + repair_path(fixture$dir), + gsub("\\\\", "/", fixture$dir) + )) function(lines) { lines <- gsub("\\\\", "/", lines) - lines <- gsub(dir, "", lines, fixed = TRUE) + for (dir in dirs) { + lines <- gsub(dir, "", lines, fixed = TRUE) + } gsub("exe-(new|old)-[0-9a-f]+", "exe-\\1-", lines) } } From a263498e9dd8be40c757eb814feb01369842786b Mon Sep 17 00:00:00 2001 From: jgabry Date: Tue, 28 Jul 2026 08:22:10 -0600 Subject: [PATCH 18/34] Let the install snapshots fail on an unrepaired path The snapshot transforms normalized backslashes out of the diagnostics. That was added to make the tests pass on Windows before the paths install_executable() builds were repaired, and it outlived its reason: with those paths repaired, a backslash reaching one of these messages is the WSL regression itself, and normalizing it away meant no snapshot could ever catch it. Only the fixture directory is still normalized, and only in the value being matched rather than in the message, because withr::local_tempdir() and repair_path() disagree about a duplicated separator. --- tests/testthat/test-model-compile.R | 1 - tests/testthat/test-utils.R | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-model-compile.R b/tests/testthat/test-model-compile.R index 32231ed1a..28c0b10bc 100644 --- a/tests/testthat/test-model-compile.R +++ b/tests/testthat/test-model-compile.R @@ -581,7 +581,6 @@ test_that("a leftover backup doesn't unwind a compile when warnings are errors", # Separators are normalized first: on Windows the backup path arrives as # "\exe-old-1234", since tempfile() joins with a backslash. transform = function(lines) { - lines <- gsub("\\\\", "/", lines) for (dir in unique(c(model_dir, repair_path(model_dir)))) { lines <- gsub(dir, "", lines, fixed = TRUE) } diff --git a/tests/testthat/test-utils.R b/tests/testthat/test-utils.R index f51795681..7d6871141 100644 --- a/tests/testthat/test-utils.R +++ b/tests/testthat/test-utils.R @@ -240,8 +240,10 @@ exe_path_transform <- function(fixture) { repair_path(fixture$dir), gsub("\\\\", "/", fixture$dir) )) + # Deliberately not normalizing separators in the message itself: + # install_executable() repairs the paths it builds, so a backslash reaching a + # diagnostic is a regression these snapshots should catch, not hide. function(lines) { - lines <- gsub("\\\\", "/", lines) for (dir in dirs) { lines <- gsub(dir, "", lines, fixed = TRUE) } From 0a27b2cb13e8c48fec37e2a3d5e60c9b3916617f Mon Sep 17 00:00:00 2001 From: jgabry Date: Tue, 28 Jul 2026 08:27:52 -0600 Subject: [PATCH 19/34] Check the reported backup is real, not merely mentioned The default-warn companion to the warn = 2 test asserted only that a warning was raised and that the object described the new program, which the signalling implementation this design rejected would also satisfy. Reporting the backup rather than deleting it is worth something only if the path named is real and still holds the previous executable, so the test now takes the path out of the message and checks it, rather than trusting that some path was mentioned. --- tests/testthat/test-model-compile.R | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-model-compile.R b/tests/testthat/test-model-compile.R index 28c0b10bc..b2f856200 100644 --- a/tests/testthat/test-model-compile.R +++ b/tests/testthat/test-model-compile.R @@ -552,12 +552,20 @@ test_that("a leftover backup warns without discarding a successful compile", { with_mocked_cli( compile_ret = list(status = 0), info_ret = list(status = 1), - code = expect_warning( + code = condition <- expect_warning( model$compile(cpp_options = list(stan_threads = TRUE), force_recompile = TRUE), "could not be removed" ) ) + # Reporting the backup instead of deleting it is only worth anything if the + # path named is real and still holds the previous executable, so check the + # path out of the message rather than trusting that one was mentioned. + leftover <- sub(".*left at '([^']*)'.*", "\\1", conditionMessage(condition)) + expect_true(file.exists(leftover)) + expect_identical(readLines(leftover), "old executable") + expect_false(same_path(leftover, model$exe_file())) + expect_describes_new_program(model) }) From 831f206a398701f11da7abb0a25662b7fdab9f43 Mon Sep 17 00:00:00 2001 From: jgabry Date: Tue, 28 Jul 2026 11:03:27 -0600 Subject: [PATCH 20/34] Skip the standalone-functions check on WSL expose_stan_functions() rejects WSL before it consults existing_exe, so the expose_functions() call asserting a self-built model is not marked pre-compiled errored there no matter what the compile logic recorded. The two assertions it backs up still run on WSL; only the observable consequence is guarded, matching the file-level skip in test-model-expose-functions.R. --- tests/testthat/test-model-recompile-logic.R | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-model-recompile-logic.R b/tests/testthat/test-model-recompile-logic.R index d0fc3359c..4e9726eec 100644 --- a/tests/testthat/test-model-recompile-logic.R +++ b/tests/testthat/test-model-recompile-logic.R @@ -89,7 +89,11 @@ test_that("a no-op compile preserves what the previous compilation recorded", { ) expect_true(mod$cpp_options()$stan_threads) expect_false(mod$functions$existing_exe) - expect_warning(mod$expose_functions(), "No standalone functions found") + # Standalone functions are rejected outright on WSL, before existing_exe is + # consulted, so only there can this consequence not be observed. + if (!os_is_wsl()) { + expect_warning(mod$expose_functions(), "No standalone functions found") + } }) test_that("a no-op compile adopts an executable the object did not build", { From ae2d4eda25af8002ff34f807db3b287ecff55354 Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 29 Jul 2026 10:13:30 -0600 Subject: [PATCH 21/34] Record only the options the executable was built with A $compile() call that found the executable up to date recorded the cpp_options it was handed, even after detecting that the binary did not have them. assert_valid_threads() and the OpenCL checks read those back as fact, so a plain $sample() failed with "the model executable was built with threading enabled but 'threads_per_chain' was not set" for a binary compiled without STAN_THREADS -- an error that is false and that no argument to $sample() can avoid. Nothing was rebuilt, so what is already recorded still describes the executable on disk and is carried forward untouched. The caller learns their request had no effect from the warning added earlier in this branch rather than from a field that claims it succeeded. Rebuilding on a mismatch remains the real fix; the skipped tests now name #1019. This also gives cpp_options_ a single meaning -- what the current executable was built with -- which is what a future mismatch check needs as its baseline. Recording the request would have defeated that check: the recorded request matches the next identical request, so a rebuild would never fire. --- NEWS.md | 4 ++ R/model.R | 15 +++--- .../testthat/test-model-generate_quantities.R | 25 +++++++--- tests/testthat/test-model-recompile-logic.R | 50 +++++++++++++++---- 4 files changed, 69 insertions(+), 25 deletions(-) diff --git a/NEWS.md b/NEWS.md index 568e0b210..951b0eccf 100644 --- a/NEWS.md +++ b/NEWS.md @@ -58,6 +58,10 @@ from the new program. (#1235) * `$compile()` now warns when `cpp_options` are supplied but the existing executable is up to date and was not built with them, so nothing is rebuilt and the options have no effect. Use `force_recompile = TRUE` to rebuild. (#1235) +* `$cpp_options()` no longer reports options the executable was not built with. +Previously a request that did not rebuild the model was recorded as though it +had, so `$sample()` could fail with "the model executable was built with +threading enabled" for a binary that had no threading. (#1019, #1235) * `$compile()` now errors if the newly compiled executable cannot be installed, restoring the previous executable. Previously the replacement was unchecked, so a failure could silently leave the model with no executable at all. (#1235) diff --git a/R/model.R b/R/model.R index f5c3b98e5..dcd28a8b3 100644 --- a/R/model.R +++ b/R/model.R @@ -724,13 +724,14 @@ compile <- function(quiet = TRUE, } # Nothing was compiled, so nothing describing the current executable may be # consumed or overwritten by configuration this call merely proposed. - # Options supplied to this call are the caller's declared intent, so they - # are recorded even though nothing was compiled. A bare $compile() supplies - # none, and must not erase what is already recorded -- an erased - # stan_threads makes assert_valid_threads() run a threaded executable - # single-threaded. - recorded_cpp_options <- - if (cpp_options_available) cpp_options else private$cpp_options_ + # Options supplied to this call describe an executable that was not built, + # so they are deliberately not recorded: assert_valid_threads() and the + # OpenCL checks read these back as fact, and a stan_threads the binary + # lacks makes a plain $sample() fail with "the model executable was built + # with threading enabled", which is false and cannot be worked around + # without recompiling. What is already recorded still describes the + # executable that exists, so it is carried forward untouched. (#1019) + recorded_cpp_options <- private$cpp_options_ # Asking the executable about itself. Best effort, because # model_compile_info() runs it and errors outright rather than returning a diff --git a/tests/testthat/test-model-generate_quantities.R b/tests/testthat/test-model-generate_quantities.R index 43d913be6..01034ea9e 100644 --- a/tests/testthat/test-model-generate_quantities.R +++ b/tests/testthat/test-model-generate_quantities.R @@ -56,19 +56,28 @@ test_that("generate_quantities work for different chains and parallel_chains", { mod_gq$generate_quantities(data = data_list, fitted_params = fit, parallel_chains = 4) ) # The executable is already built without threading and is up to date, so this - # does not rebuild it and the request has no effect on the binary. + # does not rebuild it and the request has no effect on the binary. The request + # is therefore not recorded either, so 'threads_per_chain' is refused rather + # than reported back as though it had taken effect. (#1019) expect_warning( mod_gq <- cmdstan_model(testing_stan_file("bernoulli_ppc"), cpp_options = list(stan_threads = TRUE)), "was not built with the requested" ) - expect_gq_output( - mod_gq$generate_quantities(data = data_list, fitted_params = fit_1_chain, threads_per_chain = 2) - ) - expect_output( - mod_gq$generate_quantities(data = data_list, fitted_params = fit_1_chain, threads_per_chain = 2), - "2 thread(s) per chain", - fixed = TRUE + expect_warning( + expect_gq_output( + mod_gq$generate_quantities(data = data_list, fitted_params = fit_1_chain, threads_per_chain = 2) + ), + "'threads_per_chain' is set but the model was not compiled with" + ) + # This used to report "2 thread(s) per chain" for a binary compiled without + # STAN_THREADS, which ran single-threaded regardless. + threads_output <- capture.output( + expect_warning( + mod_gq$generate_quantities(data = data_list, fitted_params = fit_1_chain, threads_per_chain = 2), + "'threads_per_chain' is set but the model was not compiled with" + ) ) + expect_false(any(grepl("thread(s) per chain", threads_output, fixed = TRUE))) }) test_that("generate_quantities works with draws_array", { diff --git a/tests/testthat/test-model-recompile-logic.R b/tests/testthat/test-model-recompile-logic.R index 4e9726eec..18dcd575d 100644 --- a/tests/testthat/test-model-recompile-logic.R +++ b/tests/testthat/test-model-recompile-logic.R @@ -15,7 +15,7 @@ file_that_doesnt_exist <- withr::local_tempfile(pattern = "placeholder_doesnt_ex file_that_exists <- withr::local_tempfile(pattern = "placeholder_exists") file.create(file_that_exists) -skip_message <- "To be fixed in a later version." +skip_message <- "To be fixed in a later version. See #1019." test_that("warning when no recompile and no info", { skip(skip_message) @@ -96,6 +96,36 @@ test_that("a no-op compile preserves what the previous compilation recorded", { } }) +test_that("a no-op compile does not record cpp_options the executable lacks", { + # A real executable, up to date and built without threading. + testing_model("bernoulli") + + expect_warning( + mod <- cmdstan_model( + testing_stan_file("bernoulli"), + cpp_options = list(stan_threads = TRUE) + ), + "was not built with the requested" + ) + + # Nothing was rebuilt, so the request describes no executable that exists. + # Recording it anyway left assert_valid_threads() trusting it, and a plain + # $sample() then failed with "The model executable was built with threading + # enabled but 'threads_per_chain' was not set!" -- an error that is both + # false and inescapable without recompiling. (#1019) + expect_false(isTRUE(mod$cpp_options()$stan_threads)) + expect_no_error( + mod$sample( + data = testing_data("bernoulli"), + chains = 1, + iter_warmup = 100, + iter_sampling = 100, + refresh = 0, + show_messages = FALSE + ) + ) +}) + test_that("a no-op compile adopts an executable the object did not build", { stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") file.copy(stan_program, stan_file) @@ -129,7 +159,7 @@ test_that("a no-op compile adopts an executable the object did not build", { expect_null(mod$cpp_options()$STAN_VERSION) }) -test_that("adopting an executable keeps the options the call asked for", { +test_that("adopting an executable describes the binary, not the request", { stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") file.copy(stan_program, stan_file) @@ -140,9 +170,9 @@ test_that("adopting an executable keeps the options the call asked for", { ) # The executable is up to date but was not built with threading. Until - # cmdstanr rebuilds on a cpp_options mismatch, dropping the request would - # make assert_valid_threads() discard 'threads' and run single-threaded - # without the caller ever asking for that. + # cmdstanr rebuilds on a cpp_options mismatch (#1019), the request describes + # an executable that does not exist, so it is reported as a warning rather + # than recorded as fact. with_mocked_cli( compile_ret = list(status = 0), info_ret = list( @@ -155,11 +185,11 @@ test_that("adopting an executable keeps the options the call asked for", { ) ) - expect_true(mod$cpp_options()$stan_threads) + expect_null(mod$cpp_options()$stan_threads) expect_true(mod$functions$existing_exe) }) -test_that("a no-op compile records options supplied to that same call", { +test_that("a no-op compile does not adopt options the executable lacks", { stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") file.copy(stan_program, stan_file) @@ -171,8 +201,8 @@ test_that("a no-op compile records options supplied to that same call", { expect_null(mod$cpp_options()$stan_threads) # Same object, same executable, but this call explicitly asks for threading. - # Preserving the recorded options is right for a bare $compile(); ignoring - # options the caller just supplied is not. + # Nothing was rebuilt, so what is recorded still has to describe the binary + # on disk; the caller learns their request had no effect from the warning. with_mocked_cli( compile_ret = list(status = 0), info_ret = list( @@ -186,7 +216,7 @@ test_that("a no-op compile records options supplied to that same call", { ) ) ) - expect_true(mod$cpp_options()$stan_threads) + expect_null(mod$cpp_options()$stan_threads) }) test_that("no mismatch warning when the executable already has the options", { From f078733ac3bc98d3aae498b224233615ecd88c51 Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 29 Jul 2026 10:13:41 -0600 Subject: [PATCH 22/34] Note the format() variables refresh in NEWS $format(overwrite_file = TRUE) has cleared the cached variables since the commit that fixed it, but it was the one user-visible change in this branch without an entry. --- NEWS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index 951b0eccf..88a0d6176 100644 --- a/NEWS.md +++ b/NEWS.md @@ -62,6 +62,9 @@ the options have no effect. Use `force_recompile = TRUE` to rebuild. (#1235) Previously a request that did not rebuild the model was recorded as though it had, so `$sample()` could fail with "the model executable was built with threading enabled" for a binary that had no threading. (#1019, #1235) +* `$format(overwrite_file = TRUE)` now refreshes `$variables()` along with +`$code()`, which previously kept describing the program as it was before +formatting. (#1235) * `$compile()` now errors if the newly compiled executable cannot be installed, restoring the previous executable. Previously the replacement was unchecked, so a failure could silently leave the model with no executable at all. (#1235) From c36009ebfdbd6379a51721ee62914052f702bbe6 Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 29 Jul 2026 10:31:11 -0600 Subject: [PATCH 23/34] Recompile when the include paths change A #include directive resolves against the include paths, so two path vectors can build two different programs from the same Stan file. $compile() replaced the stored paths eagerly but never consulted them when deciding whether to rebuild, so $compile(include_paths = ) on an already-compiled model reported the new paths and, once the cached value was cleared, the new $variables(), while continuing to run the binary built from the old ones. Initial values were then validated against a program that was not running and the chains failed inside CmdStan. Marked with a latch rather than compared in the decision itself, for the reason the user header uses one: a failed compile keeps the new paths, so on the retry they resolve back to themselves and nothing looks changed. The comparison is ordered, since order decides which directory a directive resolves from. The first configuration of an object is not a change. Treating it as one would rebuild an up-to-date executable in every new R session, which leaves an executable adopted from an earlier session unproven; that limit is documented under force_recompile. --- NEWS.md | 4 ++ R/model.R | 22 +++++++++- tests/testthat/test-model-recompile-logic.R | 46 +++++++++++++++++++++ 3 files changed, 71 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 88a0d6176..dffebdce5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -37,6 +37,10 @@ version of the Stan program. They must be exposed again with compilation when they are not supplied again. Recompiling a model that uses `#include` directives or a user header through the same object previously failed because those inputs were dropped. (#1234) +* `$compile()` now recompiles when `include_paths` change. Previously the model +went on using the executable built against the old paths while `$variables()` +and `$include_paths()` described the new ones, so data and initial values were +validated against a program that was not running. (#1235) * A `user_header` supplied to `cmdstan_model()` is now used by a later `$compile()`. Previously it was only honored when the model was compiled immediately. (#1234) diff --git a/R/model.R b/R/model.R index dcd28a8b3..d992a0e5d 100644 --- a/R/model.R +++ b/R/model.R @@ -248,6 +248,8 @@ CmdStanModel <- R6::R6Class( # the two have drifted apart. Set by a change of header, cleared only by a # successful executable replacement. user_header_dirty_ = FALSE, + # The same, for a change of include paths. + include_paths_dirty_ = FALSE, precompile_cpp_options_ = NULL, precompile_stanc_options_ = NULL, precompile_include_paths_ = NULL, @@ -629,7 +631,21 @@ compile <- function(quiet = TRUE, if (is.null(include_paths)) { include_paths <- private$include_paths_ %||% private$precompile_include_paths_ } - private$include_paths_ <- resolve_path(include_paths) + resolved_include_paths <- resolve_path(include_paths) + # Compared before the assignment below, and latched for the same reason the + # header's marker is: a failed compile must keep the new paths, so on the + # retry they resolve back to themselves and nothing looks changed. Order is + # significant -- it decides which directory a directive resolves from -- so + # this is an ordered comparison, which same_path() already does. + # + # Only a change counts, never the first configuration: a fresh object holds no + # paths, so comparing against nothing would force a rebuild in every new R + # session. That leaves an executable adopted from an earlier session unproven, + # which is documented under `force_recompile`. + private$include_paths_dirty_ <- isTRUE(private$include_paths_dirty_) || + (length(private$include_paths_) > 0 && + !same_path(resolved_include_paths, private$include_paths_)) + private$include_paths_ <- resolved_include_paths include_paths <- private$include_paths_ if (is.null(dir) && !is.null(private$dir_)) { dir <- absolute_path(private$dir_) @@ -701,6 +717,7 @@ compile <- function(quiet = TRUE, # - the executable does not exist # - the destination is not the executable this object already describes # - the user header in use is not the one the executable was built against + # - the include paths in use are not the ones the executable was built against # - the stan model was changed since last compilation # - a user header is used and the user header changed since last compilation (#813) if (!file.exists(exe)) { @@ -709,6 +726,8 @@ compile <- function(quiet = TRUE, force_recompile <- TRUE } else if (isTRUE(private$user_header_dirty_)) { force_recompile <- TRUE + } else if (isTRUE(private$include_paths_dirty_)) { + force_recompile <- TRUE } else if (file.exists(self$stan_file()) && file.mtime(exe) < file.mtime(self$stan_file())) { force_recompile <- TRUE @@ -923,6 +942,7 @@ compile <- function(quiet = TRUE, private$stan_code_ <- stan_code private$variables_ <- NULL private$user_header_dirty_ <- FALSE + private$include_paths_dirty_ <- FALSE private$hpp_file_ <- hpp_file private$model_methods_env_ <- model_methods_env private$cpp_options_ <- cpp_options diff --git a/tests/testthat/test-model-recompile-logic.R b/tests/testthat/test-model-recompile-logic.R index 18dcd575d..67cdf097d 100644 --- a/tests/testthat/test-model-recompile-logic.R +++ b/tests/testthat/test-model-recompile-logic.R @@ -126,6 +126,52 @@ test_that("a no-op compile does not record cpp_options the executable lacks", { ) }) +test_that("changing include_paths forces recompilation", { + model_dir <- withr::local_tempdir() + dir_a <- file.path(model_dir, "a") + dir_b <- file.path(model_dir, "b") + dir.create(dir_a) + dir.create(dir_b) + stan_file <- file.path(model_dir, "bernoulli.stan") + file.copy(stan_program, stan_file) + Sys.setFileTime(stan_file, Sys.time() - 60) + + mod <- with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = cmdstan_model(stan_file, include_paths = dir_a, force_recompile = TRUE) + ) + + # The same #include directive can resolve to a different file under a + # different include directory, so an executable built against one does not + # describe the program the other produces. Without this the object reported + # the new paths and the new $variables() while still running the old binary, + # which is the stale-validation failure #1228 is about. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_mock_compile(mod$compile(include_paths = dir_b)) + ) + expect_equal(mod$include_paths(), resolve_path(dir_b)) + + # The recorded paths are now the ones just built against, so a bare call has + # nothing new to build. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile(mod$compile()) + ) + + # Same directory, different spelling: not a change. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile( + mod$compile(include_paths = file.path(dir_b, ".")) + ) + ) +}) + test_that("a no-op compile adopts an executable the object did not build", { stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") file.copy(stan_program, stan_file) From 0f59faad136e87d81867c26ce1d6339c9ec91360 Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 29 Jul 2026 10:57:10 -0600 Subject: [PATCH 24/34] Test the option handoff across a successful compilation "$compile() doesn't reuse cpp and stanc options from the previous compilation" ran two dry runs. The precompile state those options travel in is cleared inside the commit block, which a dry run never enters, so the test passed only because arguments to one call are absent from another and never exercised the clearing it is named for. It now compiles for real through the mocked CLI, and asserts the flags were present on the first build rather than only absent from the second. That alone still misses the clearing, because options handed straight to $compile() are locals that never enter the precompile state, so the constructor route it does govern is covered by a second test. Removing the two clears fails that test on both assertions. Both build a temporary copy: a mocked compile installs a real, empty executable, which against the shared model would replace it. --- tests/testthat/test-model-compile.R | 81 ++++++++++++++++++++++++++--- 1 file changed, 75 insertions(+), 6 deletions(-) diff --git a/tests/testthat/test-model-compile.R b/tests/testthat/test-model-compile.R index b2f856200..f73d5e945 100644 --- a/tests/testthat/test-model-compile.R +++ b/tests/testthat/test-model-compile.R @@ -240,7 +240,11 @@ test_that("$compile() reuses include paths from the previous compilation", { }) test_that("$compile() doesn't reuse cpp and stanc options from the previous compilation", { - stan_file <- testing_stan_file("bernoulli") + # A mocked compile installs a real (empty) executable, so this has to build a + # temporary copy rather than the shared test model. + model_dir <- withr::local_tempdir() + stan_file <- file.path(model_dir, "bernoulli.stan") + file.copy(testing_stan_file("bernoulli"), stan_file) model <- cmdstan_model(stan_file, compile = FALSE) received_stancflags <- list() local_mocked_bindings( @@ -251,14 +255,79 @@ test_that("$compile() doesn't reuse cpp and stanc options from the previous comp } ) - model$compile( + # Successful compiles rather than dry runs: the precompile state these options + # travel in is cleared only once an executable has been installed, so a pair + # of dry runs never reaches the transition this test is named for and passes + # merely because arguments to one call are absent from another. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = model$compile( + cpp_options = list(stan_threads = TRUE), + stanc_options = list("warn-pedantic" = TRUE), + force_recompile = TRUE + ) + ) + expect_true(model$cpp_options()[["stan_threads"]]) + expect_equal( + vapply(received_stancflags, function(x) "--warn-pedantic" %in% x, logical(1)), + rep(TRUE, 2) + ) + + received_stancflags <- list() + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = model$compile(force_recompile = TRUE) + ) + + expect_null(model$cpp_options()[["stan_threads"]]) + expect_equal( + vapply(received_stancflags, function(x) "--warn-pedantic" %in% x, logical(1)), + rep(FALSE, 2) + ) +}) + +test_that("$compile() doesn't reuse cpp and stanc options supplied to cmdstan_model()", { + model_dir <- withr::local_tempdir() + stan_file <- file.path(model_dir, "bernoulli.stan") + file.copy(testing_stan_file("bernoulli"), stan_file) + # Options given to the constructor are held until the first compilation + # consumes them, unlike the include paths and user header, which persist. + model <- cmdstan_model( + stan_file, + compile = FALSE, cpp_options = list(stan_threads = TRUE), - stanc_options = list("warn-pedantic" = TRUE), - force_recompile = TRUE, - dry_run = TRUE + stanc_options = list("warn-pedantic" = TRUE) ) received_stancflags <- list() - model$compile(force_recompile = TRUE, dry_run = TRUE) + local_mocked_bindings( + get_cmdstan_flags = function(flag_name) character(), + get_standalone_hpp = function(stan_file, stancflags) { + received_stancflags <<- append(received_stancflags, list(stancflags)) + "" + } + ) + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = model$compile(force_recompile = TRUE) + ) + expect_true(model$cpp_options()[["stan_threads"]]) + expect_equal( + vapply(received_stancflags, function(x) "--warn-pedantic" %in% x, logical(1)), + rep(TRUE, 2) + ) + + # The held options are released only by a successful compilation, so this is + # the transition that a pair of dry runs cannot reach. + received_stancflags <- list() + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = model$compile(force_recompile = TRUE) + ) expect_null(model$cpp_options()[["stan_threads"]]) expect_equal( From c640d4d454179287fb652d34491d2c7b8f60b25b Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 29 Jul 2026 10:59:05 -0600 Subject: [PATCH 25/34] Document what the up-to-date check does not cover force_recompile asked whether the model should be rebuilt "even if it has not been modified" without saying what counts as a modification. Only the Stan program and the user header are stat'ed, so an edit to a file reached by #include goes unnoticed at any depth, including one level down. A fresh object also cannot tell which header or include paths an existing executable was built with, because nothing records them and the binary cannot report them, so configuring different ones does not rebuild it. Both are long-standing limits rather than new ones, but the escape hatch is only useful to someone who knows when to reach for it. --- R/model.R | 15 ++++++++++++++- man/model-method-compile.Rd | 19 ++++++++++++++++--- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/R/model.R b/R/model.R index d992a0e5d..70268e26d 100644 --- a/R/model.R +++ b/R/model.R @@ -504,13 +504,17 @@ NULL #' 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. If `$compile()` is called again without `include_paths`, the -#' most recently supplied paths are reused. +#' most recently supplied paths are reused, and changing them forces +#' recompilation. Edits to the included files themselves do not; see +#' `force_recompile`. #' @param user_header (string) The path to a C++ file (with a .hpp extension) #' to compile with the Stan model. If `$compile()` is called again without #' `user_header`, the most recently supplied header is reused, and changing #' it forces recompilation. Pass `user_header = NULL` to compile without one. #' A header can also be supplied via `cpp_options` as `USER_HEADER` or #' `user_header`; the `user_header` argument takes precedence over both. +#' See `force_recompile` for the case of a header supplied for a program +#' whose executable is already up to date. #' @param cpp_options (list) Any makefile options to be used when compiling the #' model (`stan_threads`, `stan_mpi`, `stan_opencl`, etc.). Anything you would #' otherwise write in the `make/local` file. For an example of using threading @@ -529,6 +533,15 @@ NULL #' @param force_recompile (logical) Should the model be recompiled even if it #' has not been modified since it was last compiled? The default is `FALSE`. #' Can also be set via a global `cmdstanr_force_recompile` option. +#' +#' Only the Stan program itself and the user header (if any) are checked for +#' modification. Files pulled in by `#include` directives are not, at any +#' depth, so editing an included file does not on its own trigger +#' recompilation. Use `force_recompile = TRUE` after changing one. Similarly, +#' when a model object is created for a Stan program whose executable already +#' exists and is up to date, CmdStanR cannot tell which `user_header` or +#' `include_paths` that executable was built with, so supplying different +#' ones does not force a rebuild. #' @param compile_model_methods (logical) Compile additional model methods #' (`log_prob()`, `grad_log_prob()`, `hessian()`, `constrain_variables()`, #' `unconstrain_variables()`, `unconstrain_draws()`, and diff --git a/man/model-method-compile.Rd b/man/model-method-compile.Rd index f262efbd7..4e4ad866d 100644 --- a/man/model-method-compile.Rd +++ b/man/model-method-compile.Rd @@ -41,14 +41,18 @@ program. Relative paths are resolved against the working directory when the model object is created (or when \verb{$compile()} is called) and stored as absolute paths, so subsequent changes to the working directory do not affect them. If \verb{$compile()} is called again without \code{include_paths}, the -most recently supplied paths are reused.} +most recently supplied paths are reused, and changing them forces +recompilation. Edits to the included files themselves do not; see +\code{force_recompile}.} \item{user_header}{(string) The path to a C++ file (with a .hpp extension) to compile with the Stan model. If \verb{$compile()} is called again without \code{user_header}, the most recently supplied header is reused, and changing it forces recompilation. Pass \code{user_header = NULL} to compile without one. A header can also be supplied via \code{cpp_options} as \code{USER_HEADER} or -\code{user_header}; the \code{user_header} argument takes precedence over both.} +\code{user_header}; the \code{user_header} argument takes precedence over both. +See \code{force_recompile} for the case of a header supplied for a program +whose executable is already up to date.} \item{cpp_options}{(list) Any makefile options to be used when compiling the model (\code{stan_threads}, \code{stan_mpi}, \code{stan_opencl}, etc.). Anything you would @@ -67,7 +71,16 @@ on available options.} \item{force_recompile}{(logical) Should the model be recompiled even if it has not been modified since it was last compiled? The default is \code{FALSE}. -Can also be set via a global \code{cmdstanr_force_recompile} option.} +Can also be set via a global \code{cmdstanr_force_recompile} option. + +Only the Stan program itself and the user header (if any) are checked for +modification. Files pulled in by \verb{#include} directives are not, at any +depth, so editing an included file does not on its own trigger +recompilation. Use \code{force_recompile = TRUE} after changing one. Similarly, +when a model object is created for a Stan program whose executable already +exists and is up to date, CmdStanR cannot tell which \code{user_header} or +\code{include_paths} that executable was built with, so supplying different +ones does not force a rebuild.} \item{compile_model_methods}{(logical) Compile additional model methods (\code{log_prob()}, \code{grad_log_prob()}, \code{hessian()}, \code{constrain_variables()}, From 0cd49de3a9a9e9e3f66bd168d098c3c9784e3f27 Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 29 Jul 2026 10:59:13 -0600 Subject: [PATCH 26/34] Pin the header provenance limit with a test A user header configured on a fresh object whose executable is already up to date does not rebuild it, and $cpp_options() does not report the header, because neither the binary nor anything beside it records which header produced it. That is a deliberate choice rather than an oversight: the alternative is recompiling in every new R session for anyone using a user header, which is the population with the most expensive builds. Covers all three supply routes, and asserts that using_user_header_ still holds, since source configuration and the description of the artifact are separate axes and only the latter is unprovable here. --- .../testthat/test-model-compile-user_header.R | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/testthat/test-model-compile-user_header.R b/tests/testthat/test-model-compile-user_header.R index 317a04bb2..d8115dabf 100644 --- a/tests/testthat/test-model-compile-user_header.R +++ b/tests/testthat/test-model-compile-user_header.R @@ -175,6 +175,47 @@ test_that("compile() uses a user header supplied to cmdstan_model()", { ) }) +test_that("a header configured over a current executable does not rebuild", { + stan_file <- local_external_model() + header <- withr::local_tempfile(lines = "", fileext = ".hpp") + local_mocked_stanc() + + # An executable that already exists and is newer than both the program and + # the header, built through a different object. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = cmdstan_model(stan_file, force_recompile = TRUE) + ) + exe <- cmdstan_ext(strip_ext(stan_file)) + Sys.setFileTime(stan_file, Sys.time() - 60) + Sys.setFileTime(header, Sys.time() - 60) + Sys.setFileTime(exe, Sys.time()) + + # Nothing records which header an executable was built with -- the binary + # cannot report it and nothing is written alongside it -- so a fresh object + # cannot tell a header it was configured with from the one already compiled + # in. Rebuilding on the possibility would recompile in every new R session, + # so the up-to-date executable is kept and $cpp_options() does not claim a + # header it cannot vouch for. Documented under `force_recompile`. + for (route in user_header_routes(header)) { + model <- do.call( + cmdstan_model, + c(list(stan_file, compile = FALSE), route) + ) + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile(model$compile()) + ) + expect_null(model$cpp_options()[["USER_HEADER"]]) + expect_null(model$cpp_options()[["user_header"]]) + # Source configuration is a separate axis and still reflects the request: + # it is what makes stanc accept the undefined functions the header defines. + expect_true(model$.__enclos_env__$private$using_user_header_) + } +}) + test_that("cmdstan_model() records a user header from every supply route", { header <- withr::local_tempfile(lines = "", fileext = ".hpp") From c6c34e05224e35a69a1e75c4308527cceacd0a47 Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 29 Jul 2026 11:06:34 -0600 Subject: [PATCH 27/34] State the commit rule in one place The three kinds of state this function juggles are explained where each is assigned, but the rule they are instances of was never written down, so the reasoning had to be reconstructed from the individual comments. --- R/model.R | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/R/model.R b/R/model.R index 70268e26d..4204fd73e 100644 --- a/R/model.R +++ b/R/model.R @@ -606,6 +606,24 @@ NULL #' # same as mod <- cmdstan_model(file_pedantic, pedantic = TRUE) #' } #' +# The object holds three kinds of state, committed at different moments. The +# comments through this function explain individual assignments; the rule they +# are all instances of is: +# +# Source configuration -- what the next build should use. Assigned eagerly and +# kept through a failure, because a bare retry after fixing a bad header or +# a wrong path has to build what the user last asked for. +# Artifact description -- what the executable on disk actually is. Committed +# only after that executable has been successfully replaced, so a dry run, a +# failed compile, or a failed install can never leave the object describing +# a program that was never built. (#1228) +# Divergence markers -- a record that the two have drifted apart, which is +# neither of the above. Latched rather than assigned, because on a retry the +# configuration resolves back to itself and nothing looks changed. +# +# exe_file_ and cmdstan_version_ are deliberate exceptions: they are also the +# configured destination and the toolchain version, so they are assigned on a +# dry run and on a no-op, but never on a failure. compile <- function(quiet = TRUE, dir = NULL, pedantic = FALSE, From c5c5c87fbcaf522b70ac124b3adcdbcbcb3f1d38 Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 29 Jul 2026 12:14:06 -0600 Subject: [PATCH 28/34] Detect option mismatches the executable cannot report exe_info_reflects_cpp_options() treats "no overlapping metadata keys" as agreement, and the binary reports only a handful of STAN_* flags. So an explicit request for an option outside that set -- stan_cpp_optims, which CmdStan 2.39 omits, or any arbitrary make variable -- was neither applied nor mentioned, despite NEWS promising a warning. When the object compiled the executable it holds the generated C++ for it, and what make was run with is recorded exactly; no metadata query can improve on that, so the request is compared against the record instead. The comparison is symmetric, because cpp_options are one-shot: an option the executable has and the request omits would be dropped by a recompilation. Names are compared case-insensitively, values as strings, NULL and FALSE count as omission, and header entries are excluded because header identity forces a rebuild on its own. An adopted executable keeps the metadata comparison, and options it cannot speak to stay unremarked rather than being reported as a mismatch: unverifiable is not wrong, and warning whenever provenance is unknown would fire on ordinary reuse. #1238 is the fix for that. Also stops querying the binary when the answer is already recorded, and corrects NEWS, which claimed more than the check delivers. Two include-path tests are strengthened here as well, since they share a file with the tests above. "changing include_paths forces recompilation" used a model with no #include at all, so it showed that make ran but not that the program changed; it now resolves one directive against two directories and asserts $variables() moves with it. "$compile() reuses include paths from the previous compilation" ran two dry runs, which leave precompile_include_paths_ in place, so reuse through the compiled state was never exercised; it now compiles first and checks the path still reaches stanc. --- NEWS.md | 7 +- R/cpp_opts.R | 35 ++++ R/model.R | 38 ++++- tests/testthat/test-model-compile.R | 34 +++- tests/testthat/test-model-recompile-logic.R | 179 ++++++++++++++++++-- 5 files changed, 269 insertions(+), 24 deletions(-) diff --git a/NEWS.md b/NEWS.md index dffebdce5..b90697b00 100644 --- a/NEWS.md +++ b/NEWS.md @@ -61,7 +61,12 @@ at the C++ stage left the old executable paired with model methods generated from the new program. (#1235) * `$compile()` now warns when `cpp_options` are supplied but the existing executable is up to date and was not built with them, so nothing is rebuilt and -the options have no effect. Use `force_recompile = TRUE` to rebuild. (#1235) +the options have no effect. The check is best effort: for an executable the +model object compiled itself the options are known exactly and any difference is +reported, but for one adopted from an earlier session only the few `STAN_*` +flags the binary reports about itself can be checked, and anything else passes +unremarked. Use `force_recompile = TRUE` when a supplied option has to take +effect. (#1235) * `$cpp_options()` no longer reports options the executable was not built with. Previously a request that did not rebuild the model was recorded as though it had, so `$sample()` could fail with "the model executable was built with diff --git a/R/cpp_opts.R b/R/cpp_opts.R index ad97a99b1..91f09f59f 100644 --- a/R/cpp_opts.R +++ b/R/cpp_opts.R @@ -89,6 +89,41 @@ merge_exe_info_cpp_options <- function(cpp_options, exe_info) { cpp_options } +# The options a compilation would actually be run with, normalized so that a +# request can be compared against what an executable was built with. Names are +# lower-cased because CmdStanR input and executable metadata disagree on case; +# header entries are dropped because header identity is tracked separately and +# forces a rebuild on its own; NULL and FALSE are dropped because neither asks +# make for anything; and values are compared as strings so that TRUE and "TRUE" +# are not read as different requests. +normalized_cpp_options <- function(cpp_options) { + normalized <- list() + for (option_name in names(cpp_options)) { + value <- cpp_options[[option_name]] + if (tolower(option_name) %in% c("user_header", "stan_version")) { + next + } + if (is.null(value) || isFALSE(value)) { + next + } + normalized[[tolower(option_name)]] <- as.character(value) + } + if (length(normalized) == 0) { + return(normalized) + } + normalized[order(names(normalized))] +} + +# Whether an executable built with `recorded` would differ from one built with +# `requested`. Symmetric, because cpp_options are one-shot: a recompilation +# carrying `requested` would drop anything `recorded` holds that it does not. +cpp_options_disagree <- function(requested, recorded) { + !identical( + normalized_cpp_options(requested), + normalized_cpp_options(recorded) + ) +} + # convert to compile flags -------------------- # from list(flag1=TRUE, flag2=FALSE) to "FLAG1=TRUE\nFLAG2=FALSE" cpp_options_to_compile_flags <- function(cpp_options) { diff --git a/R/model.R b/R/model.R index 4204fd73e..340b779d3 100644 --- a/R/model.R +++ b/R/model.R @@ -786,8 +786,16 @@ compile <- function(quiet = TRUE, # Asking the executable about itself. Best effort, because # model_compile_info() runs it and errors outright rather than returning a # status when the file is not runnable. + # This object holds the generated C++ for an executable only if it compiled + # it, and in that case what make was run with is recorded exactly. No + # metadata query can improve on that, and metadata cannot speak to options + # like STAN_CPP_OPTIMS -- absent from CmdStan 2.39's output -- or to + # arbitrary make variables at all. + built_here <- !is.null(self$functions$hpp_code) + exe_info <- NULL - if (cpp_options_available || length(private$exe_file_) == 0) { + if (length(private$exe_file_) == 0 || + (cpp_options_available && !built_here)) { exe_info <- tryCatch( model_compile_info(exe, self$cmdstan_version()), error = function(e) NULL @@ -801,14 +809,26 @@ compile <- function(quiet = TRUE, # fix and is still outstanding (see the skipped tests in # test-model-recompile-logic.R); until then, say so. options_mismatch <- FALSE - if (cpp_options_available && length(exe_info) > 0) { - # model_compile_info() reports upper-case names while - # exe_info_reflects_cpp_options() compares lower-case ones, so without - # aligning them the comparison finds no overlap and always agrees. - reported <- exe_info - names(reported) <- tolower(names(reported)) - options_mismatch <- - !isTRUE(exe_info_reflects_cpp_options(reported, cpp_options)) + if (cpp_options_available) { + if (built_here) { + options_mismatch <- + cpp_options_disagree(cpp_options, private$cpp_options_) + } else if (length(exe_info) > 0) { + # An adopted executable can only be asked about itself, and it answers + # about a handful of STAN_* flags. Options outside that set are left + # alone rather than reported as a mismatch: unverifiable is not the same + # as wrong, and warning whenever provenance is unknown would fire on + # ordinary reuse. Recording provenance beside the executable is the + # fix (#1238). + # + # model_compile_info() reports upper-case names while + # exe_info_reflects_cpp_options() compares lower-case ones, so without + # aligning them the comparison finds no overlap and always agrees. + reported <- exe_info + names(reported) <- tolower(names(reported)) + options_mismatch <- + !isTRUE(exe_info_reflects_cpp_options(reported, cpp_options)) + } } if (length(private$exe_file_) == 0) { diff --git a/tests/testthat/test-model-compile.R b/tests/testthat/test-model-compile.R index f73d5e945..880fa6ad6 100644 --- a/tests/testthat/test-model-compile.R +++ b/tests/testthat/test-model-compile.R @@ -227,16 +227,44 @@ test_that("$compile() reuses include paths from the previous compilation", { file.copy(testing_stan_file("bernoulli_include"), model_dir) file.copy(testing_stan_file("divide_real_by_two"), include_dir) + received_stancflags <- list() + local_mocked_bindings( + get_cmdstan_flags = function(flag_name) character(), + get_standalone_hpp = function(stan_file, stancflags) { + received_stancflags <<- append(received_stancflags, list(stancflags)) + "" + } + ) + mod <- cmdstan_model( file.path(model_dir, "bernoulli_include.stan"), include_paths = include_dir, compile = FALSE ) - mod$compile(dry_run = TRUE, quiet = TRUE) + # A successful compile rather than a dry run: a dry run leaves + # precompile_include_paths_ in place, so the call after it can find the paths + # there and the reuse through the compiled state is never exercised. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = mod$compile(force_recompile = TRUE, quiet = TRUE) + ) + expect_null(mod$.__enclos_env__$private$precompile_include_paths_) - # the include path isn't supplied again, but the included file is still found - expect_no_error(mod$compile(force_recompile = TRUE, dry_run = TRUE, quiet = TRUE)) + # The include path isn't supplied again, but the included file is still found + # and the path still reaches stanc. + received_stancflags <- list() + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_error(mod$compile(force_recompile = TRUE, quiet = TRUE)) + ) expect_equal(mod$include_paths(), resolve_path(include_dir)) + expect_true(all(vapply( + received_stancflags, + function(x) any(grepl(mod$include_paths(), x, fixed = TRUE)), + logical(1) + ))) }) test_that("$compile() doesn't reuse cpp and stanc options from the previous compilation", { diff --git a/tests/testthat/test-model-recompile-logic.R b/tests/testthat/test-model-recompile-logic.R index 67cdf097d..ba8507e2c 100644 --- a/tests/testthat/test-model-recompile-logic.R +++ b/tests/testthat/test-model-recompile-logic.R @@ -132,8 +132,11 @@ test_that("changing include_paths forces recompilation", { dir_b <- file.path(model_dir, "b") dir.create(dir_a) dir.create(dir_b) - stan_file <- file.path(model_dir, "bernoulli.stan") - file.copy(stan_program, stan_file) + # One directive, two directories, two different programs. + writeLines("parameters { real alpha; }", file.path(dir_a, "params.stan")) + writeLines("parameters { real beta; }", file.path(dir_b, "params.stan")) + stan_file <- file.path(model_dir, "included.stan") + writeLines(c("#include params.stan", "model { target += 0; }"), stan_file) Sys.setFileTime(stan_file, Sys.time() - 60) mod <- with_mocked_cli( @@ -141,18 +144,20 @@ test_that("changing include_paths forces recompilation", { info_ret = list(status = 1), code = cmdstan_model(stan_file, include_paths = dir_a, force_recompile = TRUE) ) + expect_equal(names(mod$variables()$parameters), "alpha") - # The same #include directive can resolve to a different file under a - # different include directory, so an executable built against one does not - # describe the program the other produces. Without this the object reported - # the new paths and the new $variables() while still running the old binary, - # which is the stale-validation failure #1228 is about. + # The same #include directive resolves to a different file under the other + # directory, so an executable built against one does not describe the program + # the other produces. Without this the object reported the new paths and the + # new $variables() while still running the old binary, which is the + # stale-validation failure #1228 is about. with_mocked_cli( compile_ret = list(status = 0), info_ret = list(status = 1), code = expect_mock_compile(mod$compile(include_paths = dir_b)) ) expect_equal(mod$include_paths(), resolve_path(dir_b)) + expect_equal(names(mod$variables()$parameters), "beta") # The recorded paths are now the ones just built against, so a bare call has # nothing new to build. @@ -265,6 +270,148 @@ test_that("a no-op compile does not adopt options the executable lacks", { expect_null(mod$cpp_options()$stan_threads) }) +test_that("a no-op compile warns about options the executable cannot report", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + + mod <- with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = cmdstan_model(stan_file, force_recompile = TRUE) + ) + + # CmdStan 2.39 reports no STAN_CPP_OPTIMS, and reports nothing at all about + # arbitrary make variables, so the binary's own metadata can neither confirm + # nor deny either request. This object compiled this executable, though, so + # what it was built with is known exactly and both requests plainly disagree + # with it. Detecting these through the metadata alone silently ignored them. + info <- paste0( + "stan_version_major=2\nstan_version_minor=39\nstan_version_patch=0\n", + "STAN_THREADS=false" + ) + for (requested in list( + list(stan_cpp_optims = TRUE), + list(my_custom_make_flag = "1") + )) { + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 0, stdout = info), + code = expect_no_mock_compile( + expect_warning( + mod$compile(cpp_options = requested), + "was not built with the requested" + ) + ) + ) + expect_null(cpp_option_value(mod$cpp_options(), names(requested))) + } +}) + +test_that("a no-op compile stays quiet about options it was built with", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + + mod <- with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = cmdstan_model( + stan_file, + cpp_options = list(stan_cpp_optims = TRUE, my_custom_make_flag = "1"), + force_recompile = TRUE + ) + ) + + # Same unreportable options, but this executable really was built with them, + # so re-supplying them is ordinary reuse and must not warn. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile( + expect_no_warning( + mod$compile( + cpp_options = list(stan_cpp_optims = TRUE, my_custom_make_flag = "1") + ) + ) + ) + ) +}) + +test_that("option comparison ignores spelling, NULL and omission", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + + mod <- with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = cmdstan_model( + stan_file, + cpp_options = list(STAN_CPP_OPTIMS = TRUE), + force_recompile = TRUE + ) + ) + + quietly <- function(requested) { + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile( + expect_no_warning(mod$compile(cpp_options = requested)) + ) + ) + } + # Same option, other spelling, and the string a makefile would carry. + quietly(list(stan_cpp_optims = TRUE)) + quietly(list(stan_cpp_optims = "TRUE")) + # NULL asks make for nothing, so it is omission, not a request to unset. + quietly(list(stan_cpp_optims = TRUE, stan_threads = NULL)) + + # Dropping a recorded option is still a change: cpp_options are one-shot, so + # recompiling with this list would build without STAN_CPP_OPTIMS. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile( + expect_warning( + mod$compile(cpp_options = list(stan_threads = TRUE)), + "was not built with the requested" + ) + ) + ) +}) + +test_that("an adopted executable stays silent about options it cannot report", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = cmdstan_model(stan_file, force_recompile = TRUE) + ) + + # A second object adopts that executable and holds no generated C++ for it, + # so the binary's own metadata is the only description available and it + # reports nothing about STAN_CPP_OPTIMS. Unverifiable is not a mismatch, so + # this neither warns nor records the request. (#1238) + info <- paste0( + "stan_version_major=2\nstan_version_minor=39\nstan_version_patch=0\n", + "STAN_THREADS=false" + ) + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 0, stdout = info), + code = expect_no_mock_compile( + expect_no_warning( + mod <- cmdstan_model( + stan_file, + cpp_options = list(stan_cpp_optims = TRUE) + ) + ) + ) + ) + expect_null(mod$cpp_options()$stan_cpp_optims) +}) + test_that("no mismatch warning when the executable already has the options", { stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") file.copy(stan_program, stan_file) @@ -272,11 +419,18 @@ test_that("no mismatch warning when the executable already has the options", { with_mocked_cli( compile_ret = list(status = 0), info_ret = list(status = 1), - code = mod <- cmdstan_model(stan_file, force_recompile = TRUE) + code = cmdstan_model( + stan_file, + cpp_options = list(stan_threads = TRUE), + force_recompile = TRUE + ) ) - # The executable reports exactly what is being asked for, so re-stating it - # must stay quiet -- otherwise the warning fires on ordinary reuse. + # Adopted by a second object, so the binary's metadata is the only account of + # it available -- an object that compiled the executable is answered from what + # it recorded instead. The metadata reports exactly what is being asked for, + # so re-stating it must stay quiet, otherwise the warning fires on ordinary + # reuse. with_mocked_cli( compile_ret = list(status = 0), info_ret = list( @@ -284,9 +438,12 @@ test_that("no mismatch warning when the executable already has the options", { stdout = "stan_version_major=2\nstan_version_minor=39\nstan_version_patch=0\nSTAN_THREADS=true" ), code = expect_no_warning( - mod$compile(cpp_options = list(stan_threads = TRUE)) + mod <- cmdstan_model(stan_file, cpp_options = list(stan_threads = TRUE)) ) ) + # Hydrated from metadata, so it carries the metadata's spelling; the accessor + # the fitting methods use is case-insensitive. + expect_true(cpp_option_value(mod$cpp_options(), "stan_threads")) }) test_that("a no-op compile tolerates an executable it cannot query", { From f040210f95d82ec3ad85efb453e20a70caf7c39f Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 29 Jul 2026 12:45:19 -0600 Subject: [PATCH 29/34] Compare options the way make receives them normalized_cpp_options() was written from the shape of the R list rather than from cpp_options_to_compile_flags(), which is what decides whether two builds differ. Three shapes were mishandled. FALSE was treated as omission, but it reaches make as STAN_CPP_OPTIMS=FALSE, and CmdStan enables some options whenever their make variable is non-empty -- as the cpp_options documentation says of stan_threads. So requesting FALSE against an executable built without it would change the executable, and went unmentioned. Only NULL is omission. FALSE keeps its literal value rather than being folded into TRUE for known boolean flags: that needs no list of such flags to maintain, and a TRUE to FALSE request still warns, which suits a user who believes they are turning something off when the makefile disagrees. Unnamed entries are raw make arguments and were dropped entirely, so a model configured only with list("STAN_THREADS=TRUE") normalized to nothing. Duplicate names took the first occurrence, because subsetting by name repeatedly returns the first, while every duplicate reaches make and a makefile takes the last. Also compares the include-path reuse test against the arguments stanc is handed rather than the stored path: under WSL the model holds a Windows host path while include_paths_stanc3_args() converts the argument to /mnt//..., so that assertion would have failed there. --- R/cpp_opts.R | 44 +++++++++----- tests/testthat/test-model-compile.R | 6 +- tests/testthat/test-model-recompile-logic.R | 66 +++++++++++++++++++++ 3 files changed, 101 insertions(+), 15 deletions(-) diff --git a/R/cpp_opts.R b/R/cpp_opts.R index 91f09f59f..8ce135c02 100644 --- a/R/cpp_opts.R +++ b/R/cpp_opts.R @@ -90,28 +90,44 @@ merge_exe_info_cpp_options <- function(cpp_options, exe_info) { } # The options a compilation would actually be run with, normalized so that a -# request can be compared against what an executable was built with. Names are -# lower-cased because CmdStanR input and executable metadata disagree on case; -# header entries are dropped because header identity is tracked separately and -# forces a rebuild on its own; NULL and FALSE are dropped because neither asks -# make for anything; and values are compared as strings so that TRUE and "TRUE" -# are not read as different requests. +# request can be compared against what an executable was built with. This has to +# follow cpp_options_to_compile_flags(), because what make is given is what +# decides whether two builds differ: +# +# - names are lower-cased, since CmdStanR input and executable metadata +# disagree on case, and values compared as strings so TRUE and "TRUE" are +# not read as two different requests; +# - an unnamed entry is a raw make argument and is compared as written; +# - a later entry with the same name wins, because every duplicate reaches +# make and a makefile takes the last; +# - only NULL is omission. FALSE is *not*: it reaches make as +# STAN_THREADS=FALSE, and CmdStan enables some options whenever their make +# variable is non-empty, so requesting FALSE can change the executable; +# - header entries are dropped, header identity being tracked separately and +# forcing a rebuild on its own. normalized_cpp_options <- function(cpp_options) { - normalized <- list() - for (option_name in names(cpp_options)) { - value <- cpp_options[[option_name]] + named <- list() + raw <- character() + for (i in seq_along(cpp_options)) { + option_name <- names(cpp_options)[i] + value <- cpp_options[[i]] + if (is.null(option_name) || is.na(option_name) || !nzchar(option_name)) { + raw <- c(raw, as.character(value)) + next + } if (tolower(option_name) %in% c("user_header", "stan_version")) { next } - if (is.null(value) || isFALSE(value)) { + if (is.null(value)) { next } - normalized[[tolower(option_name)]] <- as.character(value) + named[[tolower(option_name)]] <- paste(as.character(value), collapse = ",") } - if (length(normalized) == 0) { - return(normalized) + entries <- character() + if (length(named) > 0) { + entries <- paste0(names(named), "=", unlist(named, use.names = FALSE)) } - normalized[order(names(normalized))] + sort(c(raw, entries)) } # Whether an executable built with `recorded` would differ from one built with diff --git a/tests/testthat/test-model-compile.R b/tests/testthat/test-model-compile.R index 880fa6ad6..efe2fabf3 100644 --- a/tests/testthat/test-model-compile.R +++ b/tests/testthat/test-model-compile.R @@ -260,9 +260,13 @@ test_that("$compile() reuses include paths from the previous compilation", { code = expect_no_error(mod$compile(force_recompile = TRUE, quiet = TRUE)) ) expect_equal(mod$include_paths(), resolve_path(include_dir)) + # Compared against the arguments stanc is actually handed, not the stored + # path: under WSL the model holds a Windows host path while the stanc + # argument is converted to /mnt//..., so the two do not match. + include_args <- include_paths_stanc3_args(mod$include_paths(), direct_call = TRUE) expect_true(all(vapply( received_stancflags, - function(x) any(grepl(mod$include_paths(), x, fixed = TRUE)), + function(x) all(include_args %in% x), logical(1) ))) }) diff --git a/tests/testthat/test-model-recompile-logic.R b/tests/testthat/test-model-recompile-logic.R index ba8507e2c..41a2511f6 100644 --- a/tests/testthat/test-model-recompile-logic.R +++ b/tests/testthat/test-model-recompile-logic.R @@ -379,6 +379,72 @@ test_that("option comparison ignores spelling, NULL and omission", { ) }) +test_that("option comparison follows what make is actually given", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + + mod <- with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = cmdstan_model( + stan_file, + cpp_options = list(stan_cpp_optims = TRUE), + force_recompile = TRUE + ) + ) + no_op <- function(requested, expectation) { + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile(expectation(mod$compile(cpp_options = requested))) + ) + } + warns <- function(requested) { + no_op(requested, function(code) { + expect_warning(code, "was not built with the requested") + }) + } + quietly <- function(requested) no_op(requested, expect_no_warning) + + # FALSE is not omission. It reaches make as STAN_CPP_OPTIMS=FALSE, and CmdStan + # enables some options whenever their make variable is non-empty, so asking + # for it would build a different executable than the recorded TRUE did. + warns(list(stan_cpp_optims = FALSE)) + warns(list(stan_cpp_optims = TRUE, stan_threads = FALSE)) + + # Every duplicate reaches make, and a makefile takes the last. + quietly(list(stan_cpp_optims = FALSE, stan_cpp_optims = TRUE)) + warns(list(stan_cpp_optims = TRUE, stan_cpp_optims = FALSE)) + + # An unnamed entry is a raw make argument rather than something to skip. + warns(list("STAN_THREADS=TRUE")) +}) + +test_that("a raw make argument round-trips through the option comparison", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + + mod <- with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = cmdstan_model( + stan_file, + cpp_options = list("STAN_CPP_OPTIMS=TRUE"), + force_recompile = TRUE + ) + ) + + # Re-supplying exactly what the executable was built with is ordinary reuse, + # even when the option never had a name to compare by. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile( + expect_no_warning(mod$compile(cpp_options = list("STAN_CPP_OPTIMS=TRUE"))) + ) + ) +}) + test_that("an adopted executable stays silent about options it cannot report", { stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") file.copy(stan_program, stan_file) From 3c0c4998bfb73c26dc13bba5c576349a9e887db6 Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 29 Jul 2026 13:21:41 -0600 Subject: [PATCH 30/34] Canonicalize what make receives, rather than re-reading the list The previous fix mirrored cpp_options_to_compile_flags() instead of calling it, which is still a second implementation of make's semantics and drifted from the first in three ways. Sorting named and raw entries separately lost their relative order, so list("A=1", "A=2") and list("A=2", "A=1") compared equal despite make ending on different values; the same held across the raw/named boundary. A vector value expands to one assignment per element, which the mirror collapsed into a single comma-joined value. normalized_cpp_options() now canonicalizes the converter's output. Assignments reduce last-wins by lower-cased name, as a makefile does, and anything that is not an assignment keeps its position so a -DFOO is not read as a variable named -DFOO. Duplicate names, vector values, and NULLs are all already resolved by the time the flags exist, so there is nothing left to reinterpret. That also corrects NULL, which is not omission: it reaches make as an empty STAN_THREADS=. Since these are command-line assignments they override make/local, and CmdStan tests them with ifdef, which is a non-empty test -- so NULL disables the option regardless of make/local while omitting it leaves make/local in force. --- R/cpp_opts.R | 56 ++++++++++----------- tests/testthat/test-model-recompile-logic.R | 37 ++++++++++++-- 2 files changed, 60 insertions(+), 33 deletions(-) diff --git a/R/cpp_opts.R b/R/cpp_opts.R index 8ce135c02..6ff8e02b6 100644 --- a/R/cpp_opts.R +++ b/R/cpp_opts.R @@ -90,44 +90,40 @@ merge_exe_info_cpp_options <- function(cpp_options, exe_info) { } # The options a compilation would actually be run with, normalized so that a -# request can be compared against what an executable was built with. This has to -# follow cpp_options_to_compile_flags(), because what make is given is what -# decides whether two builds differ: +# request can be compared against what an executable was built with. # -# - names are lower-cased, since CmdStanR input and executable metadata -# disagree on case, and values compared as strings so TRUE and "TRUE" are -# not read as two different requests; -# - an unnamed entry is a raw make argument and is compared as written; -# - a later entry with the same name wins, because every duplicate reaches -# make and a makefile takes the last; -# - only NULL is omission. FALSE is *not*: it reaches make as -# STAN_THREADS=FALSE, and CmdStan enables some options whenever their make -# variable is non-empty, so requesting FALSE can change the executable; -# - header entries are dropped, header identity being tracked separately and -# forcing a rebuild on its own. +# Deliberately canonicalizes the output of cpp_options_to_compile_flags() rather +# than reading the list itself: what make is handed is what decides whether two +# builds differ, and any second reading of these lists drifts from the first. +# Named and unnamed entries, duplicate names, vector values that expand into +# several assignments and NULLs that expand into none are all already resolved +# by the time the flags exist. +# +# Assignments are reduced last-wins, as a makefile does, and compared by +# lower-cased name so that spelling is not a difference. Anything that is not an +# assignment is opaque, so it keeps its position. Header entries are dropped: +# header identity is tracked separately and forces a rebuild on its own. normalized_cpp_options <- function(cpp_options) { - named <- list() - raw <- character() - for (i in seq_along(cpp_options)) { - option_name <- names(cpp_options)[i] - value <- cpp_options[[i]] - if (is.null(option_name) || is.na(option_name) || !nzchar(option_name)) { - raw <- c(raw, as.character(value)) + assignments <- list() + opaque <- character() + for (flag in cpp_options_to_compile_flags(cpp_options)) { + if (!grepl("^[A-Za-z_][A-Za-z0-9_]*=", flag)) { + opaque <- c(opaque, flag) next } - if (tolower(option_name) %in% c("user_header", "stan_version")) { + option_name <- tolower(sub("=.*$", "", flag)) + if (option_name %in% c("user_header", "stan_version")) { next } - if (is.null(value)) { - next - } - named[[tolower(option_name)]] <- paste(as.character(value), collapse = ",") + assignments[[option_name]] <- sub("^[^=]*=", "", flag) } - entries <- character() - if (length(named) > 0) { - entries <- paste0(names(named), "=", unlist(named, use.names = FALSE)) + reduced <- character() + if (length(assignments) > 0) { + reduced <- paste0( + names(assignments), "=", unlist(assignments, use.names = FALSE) + ) } - sort(c(raw, entries)) + c(sort(reduced), opaque) } # Whether an executable built with `recorded` would differ from one built with diff --git a/tests/testthat/test-model-recompile-logic.R b/tests/testthat/test-model-recompile-logic.R index 41a2511f6..5262e7f50 100644 --- a/tests/testthat/test-model-recompile-logic.R +++ b/tests/testthat/test-model-recompile-logic.R @@ -336,7 +336,7 @@ test_that("a no-op compile stays quiet about options it was built with", { ) }) -test_that("option comparison ignores spelling, NULL and omission", { +test_that("option comparison ignores spelling but not an empty assignment", { stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") file.copy(stan_program, stan_file) @@ -362,8 +362,19 @@ test_that("option comparison ignores spelling, NULL and omission", { # Same option, other spelling, and the string a makefile would carry. quietly(list(stan_cpp_optims = TRUE)) quietly(list(stan_cpp_optims = "TRUE")) - # NULL asks make for nothing, so it is omission, not a request to unset. - quietly(list(stan_cpp_optims = TRUE, stan_threads = NULL)) + + # NULL is not omission either: it reaches make as an empty STAN_THREADS=, + # which overrides whatever make/local sets rather than leaving it alone. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile( + expect_warning( + mod$compile(cpp_options = list(stan_cpp_optims = TRUE, stan_threads = NULL)), + "was not built with the requested" + ) + ) + ) # Dropping a recorded option is still a change: cpp_options are one-shot, so # recompiling with this list would build without STAN_CPP_OPTIMS. @@ -418,6 +429,26 @@ test_that("option comparison follows what make is actually given", { # An unnamed entry is a raw make argument rather than something to skip. warns(list("STAN_THREADS=TRUE")) + + # Order survives normalization: these reach make as the same two assignments + # in opposite orders, so exactly one of them matches the recorded TRUE. + quietly(list("STAN_CPP_OPTIMS=FALSE", "STAN_CPP_OPTIMS=TRUE")) + warns(list("STAN_CPP_OPTIMS=TRUE", "STAN_CPP_OPTIMS=FALSE")) + + # The same, across the boundary between a named entry and a raw one. + quietly(structure( + list(FALSE, "STAN_CPP_OPTIMS=TRUE"), + names = c("stan_cpp_optims", "") + )) + warns(structure( + list("STAN_CPP_OPTIMS=TRUE", FALSE), + names = c("", "stan_cpp_optims") + )) + + # A vector value expands into one assignment per element, so it is the last + # element that decides, not the vector as a whole. + quietly(list(stan_cpp_optims = c(FALSE, TRUE))) + warns(list(stan_cpp_optims = c(TRUE, FALSE))) }) test_that("a raw make argument round-trips through the option comparison", { From 5e3d83f56e056e4b13f649531c5413386137fa66 Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 29 Jul 2026 13:21:48 -0600 Subject: [PATCH 31/34] Distinguish omitting a cpp_option from setting it to NULL The two were offered as interchangeable ways to leave threading disabled. They are not: cpp_options reach make as command-line assignments, which override make/local, so NULL passes an empty STAN_THREADS= and disables threading whatever make/local says, while omitting the option leaves make/local's value in force. They coincide only when make/local is silent about that variable. The distinction decides whether the up-to-date check treats NULL as a request that would change the executable, so the documentation should not imply the opposite. --- R/model.R | 6 ++++-- man/model-method-compile.Rd | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/R/model.R b/R/model.R index 340b779d3..c3c606930 100644 --- a/R/model.R +++ b/R/model.R @@ -523,8 +523,10 @@ NULL #' **Note:** For historical reasons, CmdStan treats some options as enabled #' whenever their `Make` variable is non-empty. In particular, setting #' `stan_threads` to `FALSE` passes `STAN_THREADS=FALSE` to `Make`, which -#' still enables threading! To leave threading disabled, simply omit -#' `stan_threads` entirely or set it to `NULL`. +#' still enables threading! To leave threading disabled, either omit +#' `stan_threads` entirely, which leaves any setting in `make/local` in +#' place, or set it to `NULL`, which passes an empty `STAN_THREADS=` and so +#' overrides `make/local` too. #' @param stanc_options (list) Any Stan-to-C++ transpiler options to be used #' when compiling the model. See the **Examples** section below as well as the #' [`stanc` chapter of the CmdStan User's diff --git a/man/model-method-compile.Rd b/man/model-method-compile.Rd index 4e4ad866d..6b1fbfe5e 100644 --- a/man/model-method-compile.Rd +++ b/man/model-method-compile.Rd @@ -61,8 +61,10 @@ see the Stan case study \href{https://mc-stan.org/users/documentation/case-studi \strong{Note:} For historical reasons, CmdStan treats some options as enabled whenever their \code{Make} variable is non-empty. In particular, setting \code{stan_threads} to \code{FALSE} passes \code{STAN_THREADS=FALSE} to \code{Make}, which -still enables threading! To leave threading disabled, simply omit -\code{stan_threads} entirely or set it to \code{NULL}.} +still enables threading! To leave threading disabled, either omit +\code{stan_threads} entirely, which leaves any setting in \code{make/local} in +place, or set it to \code{NULL}, which passes an empty \verb{STAN_THREADS=} and so +overrides \code{make/local} too.} \item{stanc_options}{(list) Any Stan-to-C++ transpiler options to be used when compiling the model. See the \strong{Examples} section below as well as the From a8cb5fe199e0defc5c1651e9eb174c9d644f5203 Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 29 Jul 2026 13:39:56 -0600 Subject: [PATCH 32/34] Correct two claims in the option-comparison comment NULL expands to an empty NAME= rather than to nothing, which the comment still described the old way. And sorting the assignments moves the opaque arguments after them, so only their order relative to each other is preserved, not their original positions. --- R/cpp_opts.R | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/R/cpp_opts.R b/R/cpp_opts.R index 6ff8e02b6..d3b5fc9b9 100644 --- a/R/cpp_opts.R +++ b/R/cpp_opts.R @@ -96,13 +96,15 @@ merge_exe_info_cpp_options <- function(cpp_options, exe_info) { # than reading the list itself: what make is handed is what decides whether two # builds differ, and any second reading of these lists drifts from the first. # Named and unnamed entries, duplicate names, vector values that expand into -# several assignments and NULLs that expand into none are all already resolved -# by the time the flags exist. +# several assignments and NULLs that expand into an empty NAME= are all already +# resolved by the time the flags exist. # # Assignments are reduced last-wins, as a makefile does, and compared by # lower-cased name so that spelling is not a difference. Anything that is not an -# assignment is opaque, so it keeps its position. Header entries are dropped: -# header identity is tracked separately and forces a rebuild on its own. +# assignment is opaque and keeps its order relative to the other opaque +# arguments, though not its position among the assignments. Header entries are +# dropped: header identity is tracked separately and forces a rebuild on its +# own. normalized_cpp_options <- function(cpp_options) { assignments <- list() opaque <- character() From 9394c7a664a5285b77296e3ddaec11c91b99e7b4 Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 29 Jul 2026 14:48:35 -0600 Subject: [PATCH 33/34] Account for options the executable inherited from make/local The self-built comparison treated the recorded options as the whole artifact, but the record only holds what was passed to make. Options inherited from make/local never reach $compile(), so a model built with make/local's STAN_THREADS=true and then handed cpp_options = list(stan_threads = TRUE) was told its executable lacked threading. That was a regression from routing around the binary's own metadata, which had reported the truth. The binary is now consulted on this route too, and what it reports but the record never held is taken to have come from make/local. Such options are applied to both sides of the comparison, because a rebuild would inherit them again, and merged into cpp_options_ so that later validation learns them -- suppressing the warning while leaving $cpp_options() ignorant would still have assert_valid_threads() refuse threads_per_chain for an executable that does have threading. Distinguishing inherited from explicit needs the options actually passed to make, which cpp_options_ can no longer supply once metadata is merged into it, so built_cpp_options_ records them. Without that split the merge defeats itself: a metadata-derived STAN_THREADS is indistinguishable from an explicit one on the next call, and a request that never mentioned threading reads as a change. The warning no longer says how the executable was built, since options inherited from make/local are invisible unless the binary reports them. What is known is that the two descriptions disagree. --- NEWS.md | 15 +- R/model.R | 65 ++++++--- .../testthat/test-model-generate_quantities.R | 2 +- tests/testthat/test-model-recompile-logic.R | 130 +++++++++++++++++- 4 files changed, 182 insertions(+), 30 deletions(-) diff --git a/NEWS.md b/NEWS.md index b90697b00..b1914f336 100644 --- a/NEWS.md +++ b/NEWS.md @@ -60,13 +60,18 @@ C++ used by `$hpp_file()` and `fit$init_model_methods()`. Previously a failure at the C++ stage left the old executable paired with model methods generated from the new program. (#1235) * `$compile()` now warns when `cpp_options` are supplied but the existing -executable is up to date and was not built with them, so nothing is rebuilt and -the options have no effect. The check is best effort: for an executable the -model object compiled itself the options are known exactly and any difference is -reported, but for one adopted from an earlier session only the few `STAN_*` -flags the binary reports about itself can be checked, and anything else passes +executable is up to date, so nothing is rebuilt and the options are not applied. +The check is best effort. For an executable the model object compiled itself it +compares the options passed to `Make` against those requested, and treats +anything the binary reports but was never passed as inherited from `make/local` +and so unchanged by a rebuild. For one adopted from an earlier session only the +few `STAN_*` flags the binary reports can be checked, and anything else passes unremarked. Use `force_recompile = TRUE` when a supplied option has to take effect. (#1235) +* `$cpp_options()` now also reports options the executable was built with that +were never passed to `$compile()`, such as those inherited from `make/local`, +when the binary reports them. `$sample()` and friends previously refused +`threads_per_chain` for an executable that did have threading. (#1019, #1235) * `$cpp_options()` no longer reports options the executable was not built with. Previously a request that did not rebuild the model was recorded as though it had, so `$sample()` could fail with "the model executable was built with diff --git a/R/model.R b/R/model.R index c3c606930..3ba00d49d 100644 --- a/R/model.R +++ b/R/model.R @@ -250,6 +250,12 @@ CmdStanModel <- R6::R6Class( user_header_dirty_ = FALSE, # The same, for a change of include paths. include_paths_dirty_ = FALSE, + # The cpp_options actually passed to make for the current executable, as + # distinct from cpp_options_, which also carries what the binary reports + # about itself. Only these would be dropped by a recompilation that omitted + # them; anything else the binary has came from make/local and would be + # inherited again. + built_cpp_options_ = NULL, precompile_cpp_options_ = NULL, precompile_stanc_options_ = NULL, precompile_include_paths_ = NULL, @@ -783,27 +789,32 @@ compile <- function(quiet = TRUE, # with threading enabled", which is false and cannot be worked around # without recompiling. What is already recorded still describes the # executable that exists, so it is carried forward untouched. (#1019) - recorded_cpp_options <- private$cpp_options_ + # This object holds the generated C++ for an executable only if it compiled + # it. Even then the record is only what was passed to make: options + # inherited from make/local never reach $compile() and so were never + # recorded, which is why the record alone is not the artifact. + built_here <- !is.null(self$functions$hpp_code) # Asking the executable about itself. Best effort, because # model_compile_info() runs it and errors outright rather than returning a # status when the file is not runnable. - # This object holds the generated C++ for an executable only if it compiled - # it, and in that case what make was run with is recorded exactly. No - # metadata query can improve on that, and metadata cannot speak to options - # like STAN_CPP_OPTIMS -- absent from CmdStan 2.39's output -- or to - # arbitrary make variables at all. - built_here <- !is.null(self$functions$hpp_code) - exe_info <- NULL - if (length(private$exe_file_) == 0 || - (cpp_options_available && !built_here)) { + if (cpp_options_available || length(private$exe_file_) == 0) { exe_info <- tryCatch( model_compile_info(exe, self$cmdstan_version()), error = function(e) NULL ) } + # The two accounts of the executable, combined: metadata reports the + # STAN_* flags actually compiled in, including any inherited from + # make/local, and the record holds the options metadata cannot report. + # Metadata wins where both speak, since it describes the binary; a metadata + # FALSE is skipped by the merge, so an explicit NULL survives as an empty + # assignment. A failed query leaves the record untouched. + recorded_cpp_options <- + merge_exe_info_cpp_options(private$cpp_options_, exe_info) + # Recording options the executable does not have would otherwise be a quiet # lie: nothing was rebuilt, so a requested stan_threads produces the # "N thread(s) per chain" message while the binary, compiled without @@ -813,8 +824,24 @@ compile <- function(quiet = TRUE, options_mismatch <- FALSE if (cpp_options_available) { if (built_here) { - options_mismatch <- - cpp_options_disagree(cpp_options, private$cpp_options_) + # Options the binary reports but the record never held cannot have come + # from $compile(), so they came from make/local -- and a rebuild would + # inherit them again. Applying them to both sides keeps an option the + # request never mentioned, and never had to, from reading as a change. + # Anything the record does hold was passed on the command line and + # would be dropped by a rebuild that omits it, so it stays subject to + # the symmetric comparison. + built_options <- private$built_cpp_options_ + inherited <- merge_exe_info_cpp_options(list(), exe_info) + inherited <- inherited[ + !tolower(names(inherited)) %in% tolower(names(built_options)) + ] + # Explicit options are appended last because cpp_options reach make on + # the command line, which overrides make/local. + options_mismatch <- cpp_options_disagree( + c(inherited, cpp_options), + c(inherited, built_options) + ) } else if (length(exe_info) > 0) { # An adopted executable can only be asked about itself, and it answers # about a handful of STAN_* flags. Options outside that set are left @@ -838,8 +865,6 @@ compile <- function(quiet = TRUE, # generated C++ for it, and the only description of it beyond the request # is what the binary reports about itself. self$functions$existing_exe <- TRUE - recorded_cpp_options <- - merge_exe_info_cpp_options(recorded_cpp_options, exe_info) } else { # The flag means "we don't hold the generated C++ for this executable", # which is not the same as "this call compiled nothing". @@ -851,10 +876,15 @@ compile <- function(quiet = TRUE, # options(warn = 2) this is an error, and raising it earlier would unwind # with the object half-updated. if (options_mismatch) { + # Deliberately not phrased as a claim about how the executable was built: + # options inherited from make/local are invisible here unless the binary + # reports them, so what is actually known is that the two descriptions + # disagree. (#1238) warning( - "The existing executable was not built with the requested ", - "'cpp_options' and was not rebuilt, so they will have no effect. ", - "Use 'force_recompile = TRUE' to rebuild the model.", + "The 'cpp_options' recorded or reported for the existing executable ", + "do not match the ones requested. The executable was not rebuilt, so ", + "this call did not apply them. Use 'force_recompile = TRUE' to ", + "rebuild the model.", call. = FALSE ) } @@ -999,6 +1029,7 @@ compile <- function(quiet = TRUE, private$hpp_file_ <- hpp_file private$model_methods_env_ <- model_methods_env private$cpp_options_ <- cpp_options + private$built_cpp_options_ <- cpp_options private$precompile_cpp_options_ <- NULL private$precompile_stanc_options_ <- NULL private$precompile_include_paths_ <- NULL diff --git a/tests/testthat/test-model-generate_quantities.R b/tests/testthat/test-model-generate_quantities.R index 01034ea9e..f18d734cb 100644 --- a/tests/testthat/test-model-generate_quantities.R +++ b/tests/testthat/test-model-generate_quantities.R @@ -61,7 +61,7 @@ test_that("generate_quantities work for different chains and parallel_chains", { # than reported back as though it had taken effect. (#1019) expect_warning( mod_gq <- cmdstan_model(testing_stan_file("bernoulli_ppc"), cpp_options = list(stan_threads = TRUE)), - "was not built with the requested" + "do not match the ones requested" ) expect_warning( expect_gq_output( diff --git a/tests/testthat/test-model-recompile-logic.R b/tests/testthat/test-model-recompile-logic.R index 5262e7f50..f2dd0043f 100644 --- a/tests/testthat/test-model-recompile-logic.R +++ b/tests/testthat/test-model-recompile-logic.R @@ -105,7 +105,7 @@ test_that("a no-op compile does not record cpp_options the executable lacks", { testing_stan_file("bernoulli"), cpp_options = list(stan_threads = TRUE) ), - "was not built with the requested" + "do not match the ones requested" ) # Nothing was rebuilt, so the request describes no executable that exists. @@ -232,7 +232,7 @@ test_that("adopting an executable describes the binary, not the request", { ), code = expect_warning( mod <- cmdstan_model(stan_file, cpp_options = list(stan_threads = TRUE)), - "was not built with the requested" + "do not match the ones requested" ) ) @@ -263,7 +263,7 @@ test_that("a no-op compile does not adopt options the executable lacks", { code = expect_no_mock_compile( expect_warning( mod$compile(cpp_options = list(stan_threads = TRUE)), - "was not built with the requested" + "do not match the ones requested" ) ) ) @@ -299,7 +299,7 @@ test_that("a no-op compile warns about options the executable cannot report", { code = expect_no_mock_compile( expect_warning( mod$compile(cpp_options = requested), - "was not built with the requested" + "do not match the ones requested" ) ) ) @@ -371,7 +371,7 @@ test_that("option comparison ignores spelling but not an empty assignment", { code = expect_no_mock_compile( expect_warning( mod$compile(cpp_options = list(stan_cpp_optims = TRUE, stan_threads = NULL)), - "was not built with the requested" + "do not match the ones requested" ) ) ) @@ -384,7 +384,7 @@ test_that("option comparison ignores spelling but not an empty assignment", { code = expect_no_mock_compile( expect_warning( mod$compile(cpp_options = list(stan_threads = TRUE)), - "was not built with the requested" + "do not match the ones requested" ) ) ) @@ -412,7 +412,7 @@ test_that("option comparison follows what make is actually given", { } warns <- function(requested) { no_op(requested, function(code) { - expect_warning(code, "was not built with the requested") + expect_warning(code, "do not match the ones requested") }) } quietly <- function(requested) no_op(requested, expect_no_warning) @@ -476,6 +476,122 @@ test_that("a raw make argument round-trips through the option comparison", { ) }) +test_that("options inherited from make/local are learned, not warned about", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + # make/local supplies STAN_THREADS=true, so the executable is threaded even + # though nothing was passed to $compile() and nothing could be recorded. + threaded <- paste0( + "stan_version_major=2\nstan_version_minor=39\nstan_version_patch=0\n", + "STAN_THREADS=true" + ) + + mod <- cmdstan_model(stan_file, compile = FALSE) + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 0, stdout = threaded), + code = mod$compile(force_recompile = TRUE) + ) + expect_null(mod$cpp_options()$stan_threads) + + # Comparing against the record alone reported a mismatch for a binary that + # does have threading. The binary's own account fills the gap, and is kept: + # suppressing the warning without recording what it revealed would leave + # assert_valid_threads() still dropping 'threads_per_chain'. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 0, stdout = threaded), + code = expect_no_mock_compile( + expect_no_warning(mod$compile(cpp_options = list(stan_threads = TRUE))) + ) + ) + expect_true(cpp_option_value(mod$cpp_options(), "stan_threads")) + + # An option only the record knows about still combines with one only the + # metadata knows about. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 0, stdout = threaded), + code = mod$compile( + cpp_options = list(stan_cpp_optims = TRUE), + force_recompile = TRUE + ) + ) + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 0, stdout = threaded), + code = expect_no_mock_compile( + expect_no_warning( + mod$compile(cpp_options = list(stan_cpp_optims = TRUE, stan_threads = TRUE)) + ) + ) + ) + # ...and changing the unreportable one is still caught. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 0, stdout = threaded), + code = expect_no_mock_compile( + expect_warning( + mod$compile(cpp_options = list(stan_cpp_optims = FALSE, stan_threads = TRUE)), + "do not match the ones requested" + ) + ) + ) + + # With no metadata to be had, the record is all there is and still answers + # for the option it holds. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile( + expect_no_warning(mod$compile(cpp_options = list(stan_cpp_optims = TRUE))) + ) + ) +}) + +test_that("an executable built with an explicit NULL accepts NULL again", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + + mod <- cmdstan_model(stan_file, compile = FALSE) + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = mod$compile( + cpp_options = list(stan_threads = NULL), + force_recompile = TRUE + ) + ) + + # An empty STAN_THREADS= is what was built with, so re-stating it matches. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile( + expect_no_warning(mod$compile(cpp_options = list(stan_threads = NULL))) + ) + ) + + # Omission is a different request: it would leave make/local in force rather + # than overriding it, so it does not match a build that overrode it. + mod_omitted <- cmdstan_model(stan_file, compile = FALSE) + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = mod_omitted$compile(force_recompile = TRUE) + ) + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 1), + code = expect_no_mock_compile( + expect_warning( + mod_omitted$compile(cpp_options = list(stan_threads = NULL)), + "do not match the ones requested" + ) + ) + ) +}) + test_that("an adopted executable stays silent about options it cannot report", { stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") file.copy(stan_program, stan_file) From 82c8cdf0b09e6a312acd68e5660db818e95970a8 Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 29 Jul 2026 15:51:55 -0600 Subject: [PATCH 34/34] Make the flag parser the only reader of cpp_options Two places still interpreted the R list rather than what make is given, and both got it wrong in the same ways. The inheritance filter read names(built_cpp_options_), so an unnamed raw "STAN_THREADS=TRUE" was invisible to it: with the binary reporting threading, an explicit raw assignment was mistaken for a make/local contribution and omitting it raised no warning. exe_info_reflects_cpp_options() read its own names and values, so a raw assignment was ignored, duplicates took the first rather than the last, and a vector value errored outright. parsed_cpp_options() now does the parsing for all of them, from the converter's output, and the adopted route compares only the assignments the binary reports: empty means disabled, any non-empty value means enabled, matching the ifdef test CmdStan actually uses. Unreportable assignments stay ignored, so the adopted route's asymmetry is unchanged. None of this is established behaviour being altered. exe_info_reflects_cpp_options() had no production caller before this branch, so these semantics ship for the first time here either way. The comparator is now case-insensitive about metadata names, which retires the alignment the call site was doing. All six existing assertions hold unchanged: exe_info_style_cpp_options() already treated non-empty as enabled for the five names it knows, and its bugs were in shapes those tests never used. It is left alone, being dead at the branch point rather than orphaned here. --- NEWS.md | 6 ++- R/cpp_opts.R | 36 ++++++++++----- R/model.R | 16 +++---- tests/testthat/test-cpp_opts.R | 38 ++++++++++++++++ tests/testthat/test-model-recompile-logic.R | 49 ++++++++++++++++++++- 5 files changed, 122 insertions(+), 23 deletions(-) diff --git a/NEWS.md b/NEWS.md index b1914f336..3194c2b70 100644 --- a/NEWS.md +++ b/NEWS.md @@ -66,7 +66,11 @@ compares the options passed to `Make` against those requested, and treats anything the binary reports but was never passed as inherited from `make/local` and so unchanged by a rebuild. For one adopted from an earlier session only the few `STAN_*` flags the binary reports can be checked, and anything else passes -unremarked. Use `force_recompile = TRUE` when a supplied option has to take +unremarked. It can also warn when nothing would in fact change: an option +inherited from `make/local` that the binary does not report looks like a request +the executable lacks, and one that was both passed explicitly and set in +`make/local` looks like something a rebuild would drop when it would be +inherited again. Use `force_recompile = TRUE` when a supplied option has to take effect. (#1235) * `$cpp_options()` now also reports options the executable was built with that were never passed to `$compile()`, such as those inherited from `make/local`, diff --git a/R/cpp_opts.R b/R/cpp_opts.R index d3b5fc9b9..2dc2ef75e 100644 --- a/R/cpp_opts.R +++ b/R/cpp_opts.R @@ -105,7 +105,7 @@ merge_exe_info_cpp_options <- function(cpp_options, exe_info) { # arguments, though not its position among the assignments. Header entries are # dropped: header identity is tracked separately and forces a rebuild on its # own. -normalized_cpp_options <- function(cpp_options) { +parsed_cpp_options <- function(cpp_options) { assignments <- list() opaque <- character() for (flag in cpp_options_to_compile_flags(cpp_options)) { @@ -119,13 +119,19 @@ normalized_cpp_options <- function(cpp_options) { } assignments[[option_name]] <- sub("^[^=]*=", "", flag) } + list(assignments = assignments, opaque = opaque) +} + +normalized_cpp_options <- function(cpp_options) { + parsed <- parsed_cpp_options(cpp_options) reduced <- character() - if (length(assignments) > 0) { + if (length(parsed$assignments) > 0) { reduced <- paste0( - names(assignments), "=", unlist(assignments, use.names = FALSE) + names(parsed$assignments), "=", + unlist(parsed$assignments, use.names = FALSE) ) } - c(sort(reduced), opaque) + c(sort(reduced), parsed$opaque) } # Whether an executable built with `recorded` would differ from one built with @@ -345,11 +351,21 @@ exe_info_reflects_cpp_options <- function(exe_info, cpp_options) { } if (is.null(cpp_options)) return(TRUE) - cpp_options <- exe_info_style_cpp_options(cpp_options)[tolower(names(cpp_options))] - overlap <- names(cpp_options)[names(cpp_options) %in% names(exe_info)] + # Only the assignments the binary can speak to. Anything else is left alone + # rather than reported as a mismatch: an adopted executable carries no record + # of what produced it, so an unreportable option is unverifiable, not wrong. + # Read through parsed_cpp_options() so that unnamed raw assignments, duplicate + # names and vector values mean here what they mean to make. + assignments <- parsed_cpp_options(cpp_options)$assignments + reported <- intersect(names(assignments), tolower(names(exe_info))) - if (length(overlap) == 0) TRUE else all.equal( - exe_info[overlap], - cpp_options[overlap] - ) + for (option_name in reported) { + # CmdStan enables these whenever the make variable is non-empty, so an empty + # assignment is the only way to ask for one to be off. + requested <- nzchar(assignments[[option_name]]) + if (requested != isTRUE(cpp_option_value(exe_info, option_name))) { + return(FALSE) + } + } + TRUE } diff --git a/R/model.R b/R/model.R index 3ba00d49d..808af216c 100644 --- a/R/model.R +++ b/R/model.R @@ -833,9 +833,11 @@ compile <- function(quiet = TRUE, # the symmetric comparison. built_options <- private$built_cpp_options_ inherited <- merge_exe_info_cpp_options(list(), exe_info) - inherited <- inherited[ - !tolower(names(inherited)) %in% tolower(names(built_options)) - ] + # Which options were passed explicitly is a question about what make was + # given, not about the shape of the list: an unnamed "STAN_THREADS=TRUE" + # is as explicit as a named entry, and names() cannot see it. + explicit <- names(parsed_cpp_options(built_options)$assignments) + inherited <- inherited[!tolower(names(inherited)) %in% explicit] # Explicit options are appended last because cpp_options reach make on # the command line, which overrides make/local. options_mismatch <- cpp_options_disagree( @@ -849,14 +851,8 @@ compile <- function(quiet = TRUE, # as wrong, and warning whenever provenance is unknown would fire on # ordinary reuse. Recording provenance beside the executable is the # fix (#1238). - # - # model_compile_info() reports upper-case names while - # exe_info_reflects_cpp_options() compares lower-case ones, so without - # aligning them the comparison finds no overlap and always agrees. - reported <- exe_info - names(reported) <- tolower(names(reported)) options_mismatch <- - !isTRUE(exe_info_reflects_cpp_options(reported, cpp_options)) + !isTRUE(exe_info_reflects_cpp_options(exe_info, cpp_options)) } } diff --git a/tests/testthat/test-cpp_opts.R b/tests/testthat/test-cpp_opts.R index c7e3a8682..75630fb70 100644 --- a/tests/testthat/test-cpp_opts.R +++ b/tests/testthat/test-cpp_opts.R @@ -155,3 +155,41 @@ test_that("exe_info cpp_options comparison works", { "Recompiling is recommended" ) }) + +test_that("exe_info comparison reads cpp_options the way make does", { + # Upper-case, as model_compile_info() reports it. + disabled <- list(STAN_THREADS = FALSE) + + # An unnamed raw assignment is as much a request as a named one; reading the + # list's names cannot see it. + expect_not_true( + exe_info_reflects_cpp_options(disabled, list("STAN_THREADS=TRUE")) + ) + + # Every duplicate reaches make and a makefile takes the last, so the order + # decides which of these agrees. + expect_true(exe_info_reflects_cpp_options( + disabled, + list(stan_threads = TRUE, stan_threads = NULL) + )) + expect_not_true(exe_info_reflects_cpp_options( + disabled, + list(stan_threads = NULL, stan_threads = TRUE) + )) + + # A vector value expands into one assignment per element. This used to error. + expect_not_true(exe_info_reflects_cpp_options( + disabled, + list(stan_threads = c(TRUE, FALSE)) + )) + + # Non-empty enables whatever the value, so FALSE does not ask for "off". + expect_not_true( + exe_info_reflects_cpp_options(disabled, list(stan_threads = FALSE)) + ) + + # An option the binary cannot report is unverifiable, not a mismatch. + expect_true( + exe_info_reflects_cpp_options(disabled, list(my_custom_make_flag = TRUE)) + ) +}) diff --git a/tests/testthat/test-model-recompile-logic.R b/tests/testthat/test-model-recompile-logic.R index f2dd0043f..2dc1ce905 100644 --- a/tests/testthat/test-model-recompile-logic.R +++ b/tests/testthat/test-model-recompile-logic.R @@ -549,14 +549,59 @@ test_that("options inherited from make/local are learned, not warned about", { ) }) +test_that("an explicitly passed raw assignment is not taken for make/local", { + stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") + file.copy(stan_program, stan_file) + threaded <- paste0( + "stan_version_major=2\nstan_version_minor=39\nstan_version_patch=0\n", + "STAN_THREADS=true" + ) + + mod <- cmdstan_model(stan_file, compile = FALSE) + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 0, stdout = threaded), + code = mod$compile( + cpp_options = structure( + list("STAN_THREADS=TRUE", TRUE), + names = c("", "stan_cpp_optims") + ), + force_recompile = TRUE + ) + ) + + # The binary reports threading and this call did not name stan_threads, but it + # did pass STAN_THREADS=TRUE as a raw assignment, so the flag is not inherited + # from make/local and omitting it would drop it. Reading names() rather than + # what make was given missed that and stayed quiet. + with_mocked_cli( + compile_ret = list(status = 0), + info_ret = list(status = 0, stdout = threaded), + code = expect_no_mock_compile( + expect_warning( + mod$compile(cpp_options = list(stan_cpp_optims = TRUE)), + "do not match the ones requested" + ) + ) + ) +}) + test_that("an executable built with an explicit NULL accepts NULL again", { stan_file <- file.path(withr::local_tempdir(), "bernoulli.stan") file.copy(stan_program, stan_file) + # Metadata reporting threading off, rather than no metadata at all, so the + # merge is exercised: a reported FALSE is skipped, leaving the explicit NULL + # to stand as the empty assignment it is. + disabled <- paste0( + "stan_version_major=2\nstan_version_minor=39\nstan_version_patch=0\n", + "STAN_THREADS=false" + ) + mod <- cmdstan_model(stan_file, compile = FALSE) with_mocked_cli( compile_ret = list(status = 0), - info_ret = list(status = 1), + info_ret = list(status = 0, stdout = disabled), code = mod$compile( cpp_options = list(stan_threads = NULL), force_recompile = TRUE @@ -566,7 +611,7 @@ test_that("an executable built with an explicit NULL accepts NULL again", { # An empty STAN_THREADS= is what was built with, so re-stating it matches. with_mocked_cli( compile_ret = list(status = 0), - info_ret = list(status = 1), + info_ret = list(status = 0, stdout = disabled), code = expect_no_mock_compile( expect_no_warning(mod$compile(cpp_options = list(stan_threads = NULL))) )