Keep model state consistent with the executable, and stop dropping compile-time inputs - #1235
Draft
jgabry wants to merge 35 commits into
Draft
Keep model state consistent with the executable, and stop dropping compile-time inputs#1235jgabry wants to merge 35 commits into
jgabry wants to merge 35 commits into
Conversation
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
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
$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.
$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.
jgabry
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1235 +/- ##
==========================================
+ Coverage 91.73% 91.92% +0.18%
==========================================
Files 15 15
Lines 6281 6511 +230
==========================================
+ Hits 5762 5985 +223
- Misses 519 526 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
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.
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.
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.
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.
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().
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.
jgabry
marked this pull request as draft
July 28, 2026 01:36
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.
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.
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.
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.
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().
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.
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.
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.
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.
$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.
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.
"$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.
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.
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.
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.
This was referenced Jul 29, 2026
jgabry
changed the title
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.
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/<drive>/..., so that assertion would have failed there.
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.
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.
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Submission Checklist
Summary
Fixes #1228
Fixes #1234
This PR was generated in collaboration with claude code, some code was written by me and some by claude. All code was reviewed by me. I asked claude to generate the summary below.
Issue 1228 — source-derived state is refreshed on recompilation
private$stan_code_ was read once in initialize() and private$variables_ was cached on the first $variables() call; $compile() invalidated neither. Editing the .stan file and recompiling through the same object left $code() and $variables() describing the old program. That isn't 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 exist.
Successful replacement of the executable is now the synchronization point. In one block after the exe copy: the code snapshot is re-read from the temp file that was actually compiled, variables_ is cleared so the next $variables() reparses, the functions environment is emptied in place (preserving its identity) and repopulated. (using_user_header_ was committed here too; it is now assigned eagerly — see the follow-up section below.) The standalone hpp and the external/existing_exe values moved to locals, and the compile_standalone exposure moved to after that block — that's what makes a dry run or a failed compilation leave the previously compiled state untouched.
Two user-visible consequences:
$code() is still a snapshot: editing or deleting the source file alone doesn't change it. $variables() still parses the file on disk, so the two can diverge after an edit until the next compile — that's deliberate, since $variables() is useful on uncompiled models.
Issue 1234 — include paths and the user header persist
The precompile_* fields were cleared at the end of $compile() and nothing fed include_paths_ back in, so a second $compile() ran with no include paths and no user header. A model with #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 would silently produce a different executable.
Include paths and a user header aren't build options — they're inputs the program needs in order to translate — so they now persist for the life of the object and are replaced whenever new ones are supplied:
cpp_options and stanc_options deliberately keep their one-shot behavior. A bare $compile() producing an unconfigured build is a tested workflow ("switching threads on and off works without rebuild"), and sticky stanc options would leak values such as name= into every later compilation of the same object. A test pins that asymmetry.
Also
$check_syntax() and $format() never consulted using_user_header_, so any model with an external C++ function was reported as a syntax error — even one that was never compiled. $compile() and $variables() both already derive --allow-undefined; these two were the only methods that didn't.
Testing
New regression tests in test-model-variables.R (the issue's reproduction, including that inits for the new parameter no longer trigger the "subset of parameters" message), test-model-code-print.R, test-model-expose-functions.R, test-model-compile.R (commit timing under dry_run and a failed compile, include-path reuse, and the cpp/stanc asymmetry) and test-model-compile-user_header.R (header reuse, and a header supplied to cmdstan_model()). Each was confirmed to fail before the corresponding fix.
Run locally on macOS: test-model-compile.R, test-model-compile-user_header.R, test-model-variables.R, test-model-code-print.R, test-model-recompile-logic.R, test-model-expose-functions.R, test-model-methods.R, plus the include-path test in test-fit-shared.R — all pass, with only pre-existing skips. The full suite will be run on CI and wasn't run locally because test-install.R rebuilds CmdStan from source.
Not included
After a real compilation, $check_syntax() and $format() still lose the stanc_options supplied to cmdstan_model(), since precompile_stanc_options_ is their only source. Fixing that means giving them a persistent copy rather than sharing the field $compile() consumes; noted in #1234.
Follow-up: applying the invariant everywhere
Review found the invariant above — successful replacement of the executable is
the point at which state describing the compiled artifact is committed — was
right but incompletely applied. State now splits four ways:
(
include_paths_,user_header_,using_user_header_). Assigned eagerly:a failed compile must not invalidate it, and
$variables(),$check_syntax()and
$format()consume it without compiling. Shape is validated wherever avalue is accepted; existence only when compiling, so a header created between
cmdstan_model(..., compile = FALSE)and$compile()still works. This alsocloses a hole above: a failed
$compile(user_header = h)used to leaveusing_user_header_FALSE, reintroducing the bogus "declared withoutspecifying a definition" error.
stan_code_,variables_,functions,model_methods_env_,hpp_file_,cpp_options_).Committed only after verified replacement.
exe_file_andcmdstan_version_are commented exceptions: during a dry run they are also the configured
destination and toolchain version, so they are assigned on dry runs and on
success, never on failure.
user_header_dirty_andinclude_paths_dirty_, whichare neither: they record that configuration and artifact have drifted apart.
Latched rather than assigned, because on a retry after a failed compile the
configuration resolves back to itself and nothing looks changed.
built_cpp_options_, thecpp_optionsactuallypassed to
makefor the current executable. Distinct fromcpp_options_,which also carries what the binary reports about itself, because only the
options this object passed would be dropped by a rebuild that omitted them;
anything else the binary has came from
make/localand would be inheritedagain. Written only in the commit block, so a no-op may augment
cpp_options_from metadata while leaving this untouched.State transitions
The rule above, made concrete.
exe_file_andcmdstan_version_are thecommented exceptions described in the previous section.
exe_file_cmdstan_model(compile = FALSE)FALSE— first configuration is not a changecmdstan_model(exe_file = )FALSE<exe> info;existing_exe = TRUEcpp_optionsthe binary lackscpp_options_augmented from metadata;built_cpp_options_untouchedTRUEdir =)precompile_*clearedTwo rows carry most of the bugs here. Failed compile previously replaced
$code(),$variables()and the model-method C++ while leaving the oldexecutable in place (#1228). No-op previously erased
$cpp_options()andmarked a self-built model as pre-compiled (#1234).
The up-to-date check reads the mtimes of the Stan program and the user header
only. Files reached by
#includeare not checked at any depth, and nothingrecords which header or include paths produced an existing executable, so a
fresh object cannot verify the provenance of a binary it did not build. Both
limits are now documented under
force_recompile.Edge cases introduced by the new persistence
Persisting the header is what fixes #1234, and it raises two questions the old
code never had to answer.
compile = FALSEwas left unresolved, so alater
$compile()from a different working directory looked for it in thewrong place. Headers now resolve to absolute paths at construction, matching the
include-path convention from Relative include_paths are resolved against the working directory at each stanc call #1229.
$compile()nowaccepts
user_header = NULL, which clears a header supplied through any of thethree routes (
user_header,cpp_options$USER_HEADER,cpp_options$user_header). Because clearing changes what would be built, itforces recompilation rather than taking the up-to-date path — as does changing
from one header to another. Duplicate spellings in
cpp_optionsare reduced tothe one actually used, so
$cpp_options()no longer reports the ignoredduplicate after a successful compile.
The same precedence now runs at construction, not just in
$compile(): previouslycmdstan_model(f, compile = FALSE, user_header = NULL, cpp_options = list(USER_HEADER = h))silently ignored the explicitNULLand built withh.The user-header chain is now a single pure resolver called from both
initialize()andcompile().character(0)is rejected with a message namingthe argument instead of failing later with "invalid 'file' argument".
A quiet lie this surfaced
Supplying
cpp_optionsto a compile that finds the executable up to date recordsthem without rebuilding anything. So
cpp_options = list(stan_threads = TRUE)over an existing unthreaded executable made
$cpp_options()report threading andprinted
", with 2 thread(s) per chain..."— a message cmdstanr emits itself —while the binary, compiled without
STAN_THREADS, ran single-threaded. Resultswere correct; the performance the user asked for silently never happened.
Worse than a missed optimisation, it also produced a false error: with
stan_threadsrecorded, a plain$sample()with nothreads_per_chainhitassert_valid_threads()'s "the model executable was built with threading enabledbut 'threads_per_chain' was not set!" — untrue, and inescapable without
recompiling. The two failure modes chained, since the only way past the error was
to set
threads_per_chainand land back in the silent lie.$compile()now warns in that case and does not record the request. What isalready recorded still describes the binary on disk and is carried forward, so a
bare
$compile()still cannot erase it.assert_valid_threads()then refusesthreads_per_chainwith its existing warning rather than trusting a claimnothing verified.
The warning is best effort, and which of two routes it takes matters:
The object compiled this executable. Then the options passed to
makearerecorded, so the request is compared against those — which catches options the
binary cannot report (
STAN_CPP_OPTIMSis absent from CmdStan 2.39's output;arbitrary make variables never appear). The record is not the whole artifact,
though: options inherited from
make/localnever reach$compile()and sowere never recorded. Anything the binary reports that the record does not hold
is therefore treated as inherited, applied to both sides of the comparison
since a rebuild would inherit it again, and added to
$cpp_options()so thatthreads_per_chainis no longer refused for an executable that does havethreading. The comparison is symmetric, because
cpp_optionsare one-shot: anoption the executable has and the request omits would be dropped by a
recompilation, so that is a difference too. Rather than re-reading the
cpp_optionslist — a second implementation of make's semantics, which drifts— the comparison canonicalizes the output of
cpp_options_to_compile_flags(), so what is compared is literally whatmakeis handed. Assignments reduce last-wins by lower-cased name, as a makefile
does; anything that is not an assignment keeps its order relative to the other
opaque arguments, though not its position among the assignments; and duplicate
names, vector values that expand into several assignments, and header entries
are all resolved before the comparison sees them.
Two shapes that look like omission are not.
FALSEreaches make asSTAN_THREADS=FALSE, and CmdStan enables some options whenever their variableis non-empty.
NULLreaches make as an emptySTAN_THREADS=— and sincethese are command-line assignments they override
make/local, soNULLdisables the option whatever
make/localsays, while omitting it leavesmake/localin force. Thecpp_optionsdocumentation, which offered the twoas interchangeable, now says so.
The executable was adopted from an earlier session. Then the binary's own
metadata is the only account available, and it covers a handful of
STAN_*flags. Anything outside that set passes unremarked rather than being reported
as a mismatch — unverifiable is not the same as wrong, and warning whenever
provenance is unknown would fire on ordinary reuse. Record what an executable was built with, alongside the executable? #1238 is what fixes this
properly.
force_recompile = TRUEremains the way to guarantee a supplied option takeseffect.
Rebuilding automatically on a mismatch is the real fix and remains outstanding
(#1019, and the three
skip()ped tests intest-model-recompile-logic.R, whichnow name that issue). This change is a step toward it rather than a detour:
cpp_options_now means exactly one thing — what the current executable wasbuilt with — which is the baseline such a check needs. Recording the request
would have defeated it, since the recorded request matches the next identical
request and a rebuild would never fire.
This wires up
exe_info_reflects_cpp_options(), which existed and was tested buthad no production caller — so although the function predates this branch, its
behaviour ships here for the first time, and its input handling was corrected as
part of that rather than inherited. It compared lower-case option names against
model_compile_info()'s upper-case output, which is why it had never once fired;it also read the
cpp_optionslist directly, so an unnamed raw assignment wasinvisible to it, duplicate names took the first rather than the last, and a
vector value errored outright. It now reads through the same parse as everything
else and is case-insensitive about metadata names.
Pre-existing coherence defects found while tightening the boundary
None of these are regressions from this PR; they are cases where the object could
end up describing a program its executable was not built from.
at the C++ stage left the old executable paired with model-method code generated
from the new source, and that environment is handed to every fit — so
fit$init_model_methods()would compilelog_prob()from a program the drawsdid not come from. Both are now staged in locals and committed with everything
else.
file.remove(exe)followed byfile.copy(tmp_exe, exe)discarded both return values, so a copy failing after asuccessful remove left the model with no executable and no error. Replacement
now stages the new executable beside the destination, moves the old one aside,
renames into place, and rolls back on failure. Every filesystem call is wrapped
in
suppressWarnings()and checked by value, becausefile.rename()warns onfailure and under
options(warn = 2)would otherwise throw before the rollbackcould run. A backup that cannot be cleaned up afterwards is returned rather
than signalled, and warned about only once the optional exposure work is done —
signalling earlier would unwind before the state describing the newly installed
executable was recorded.
$compile()erased$cpp_options(). The recorded options werereplaced with whatever that call supplied — usually nothing. Erasing
stan_threadsmakesassert_valid_threads()warn and dropthreads, so athreaded executable silently ran single-threaded. Masked for
cmdstan_model()by the constructor's metadata merge, so it only bit direct
$compile()calls.$compile()assertedexisting_exeunconditionally, so$expose_functions()failed with "not possible with a pre-compiled Stan model"on a model that had compiled itself. The flag now means what it says: we do not
hold the generated C++ for this executable.
$compile()cleared the precompile options, so a later forcedrecompilation lost the
cpp_options,stanc_optionsandinclude_pathssupplied to
cmdstan_model().$compile(dir = ...)pointing at a different current executable adopted itwhile keeping this object's generated C++ and metadata. It now rebuilds there.
Paths are compared canonically, so symlink aliases,
..components and Windowscasing do not cause needless rebuilds.
include_pathsdid not rebuild. A#includedirective resolvesagainst the include paths, so two path vectors can build two different programs
from one Stan file.
$compile()replaced the stored paths eagerly but neverconsulted them when deciding whether to rebuild, so
$compile(include_paths = b)on a compiled model reported the new paths and the new$variables()whilestill running 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. 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.
$compile(dry_run = TRUE)also no longer records$cpp_options()or moves$hpp_file(), which previously pointed at a temporary file the dry run neverwrote. Two existing tests asserted on
$cpp_options()after a dry run and now usea mocked successful compile instead.
One implementation note a reviewer may want: the resolver strips both header
spellings from
cpp_optionsand only$compile()reinserts the selected one, soprecompile_cpp_options_deliberately never carries a header. Storing it therewould mean storing a WSL-safe path, which the next
$compile()would then selectas its
user_header— a host path by design, sincefile.exists()on it breaksunder WSLv1.
user_header_is the single source instead.Testing the follow-up
tests/testthat/helper-mock-cli.Rpreviously returnedstatus = 0withoutproducing the executable
makewas asked for, which is why the uncheckedreplacement was invisible to the test suite. It now creates the artifact when and
only when the mocked compile succeeds. That in turn required moving the mocked
compiles in
test-model-recompile-logic.Ronto temporary copies: they targeted<cmdstan>/examples/bernoulli/bernoulli.stan, so a faithful mock overwrites theCmdStan installation's own example executable with an empty file. That file is not
in the repository and a truncating overwrite leaves no diff at all, so the file
carries a guard test checking its size and mtime before and after.
New tests cover the staged replacement and each of its failure modes (snapshotted,
including the recovery paths the diagnostics name), the
warn = 2behaviour atboth the helper and
$compile()level, header clearing and identity changesthrough all three supply routes, construction-time precedence, and the no-op
path's preservation of
cpp_options()and$expose_functions(). Each wasconfirmed to fail before the corresponding fix.
Second review round
Six items, all addressed. Three notes where the outcome differs from what was
asked, with the evidence behind each.
include_pathsdid not rebuild — fixed, as described above. Not aregression: reproduced identically against the PR base, which had the same
eager assignment and no include clause in the decision.
suggested alternative, rebuilding automatically on a mismatch, is deliberately
not done here.
exe_info_reflects_cpp_options()had never executed inproduction before this branch (the case mismatch above meant it always compared
an empty overlap), and the metadata keys it depends on vary by CmdStan version —
2.39 reports no
STAN_CPP_OPTIMSat all, so a request for it cannot be checkedagainst the binary either way. Switching a rebuild onto a comparator with no
field history, over a key set that changes between releases, risks recompiling
on every call for anyone whose requested option is reported but not changed by
the rebuild. Make compile(..., cpp_options()) consistent with makefile options #1019 tracks it.
rebuild — documented rather than changed, and pinned by a test across all
three supply routes. Provenance cannot be established: the binary does not
report its header, so "rebuild unless provenance is proven" reduces to "always
rebuild", once per R session, for exactly the users with the most expensive C++
builds. It would also close only one direction — an executable built with a
header and adopted by an object configured without one is the same gap
reversed. Exposure is narrower than it appears: a model that needs a user
header cannot build without one (
makefails on the missinguser_header.hppand leaves no executable), so an existing up-to-date binaryfor such a model was built with some header; the remaining case is a
different header older than the executable, and the mtime check already
catches every case where it is newer. The durable fix is recording build
provenance beside the executable, which would also retire the metadata
guesswork entirely.
flagged tests ran two dry runs each, and the precompile state they concern is
cleared inside the commit block, which a dry run never enters. Fixed, but
"begin with a successful compilation" was not sufficient on its own: options
handed straight to
$compile()are locals that never enterprecompile_*, soa second test covers the constructor route that actually exercises the
clearing. Verified by deleting the two clears — it fails on both assertions.
force_recompile,with the framing corrected. It is not only nested includes: a directly
included file is not checked either, since only the top-level program and the
user header are stat'ed.
instead. Splitting would spread one state machine across four PRs that each
edit the same
compile()decision block and each need a full CI run, and theproposed fourth slice already exists as Make compile(..., cpp_options()) consistent with makefile options #1019. The commits are sequenced and
individually tested, so
git bisectworks across them.Also in this round:
$format(overwrite_file = TRUE)gained its missing NEWSentry, and the skipped mismatch tests now name #1019 so they read as tracked
rather than abandoned.
A second pass raised two more, both taken:
mismatch only through executable metadata meant a request for
stan_cpp_optims— or any arbitrary make variable — was neither applied normentioned, contradicting what NEWS claimed. An executable the object compiled
itself is now answered from what was recorded rather than from metadata, which
catches those exactly. See the two routes described above.
model with no
#includeat all, so it showedmakeran but not that theprogram changed; it now resolves a single directive against two directories and
asserts
$variables()moves with it. The other ran two dry runs, which leaveprecompile_include_paths_in place, so reuse through the compiled state wasnever exercised.