Skip to content

Commit 1cf6b4e

Browse files
jgabryjgabry
authored
Keep model state consistent with the executable, and stop dropping compile-time inputs (#1235)
The model object could describe an executable it did not match. Several separate paths caused it. $code() and $variables() were captured once and never invalidated, so editing the Stan file and recompiling through the same object left both describing the old program. Because the fitting methods pass variables() into the data and init checks, a recompiled model validated against the old parameter set and warned about parameters that no longer existed. Successful replacement of the executable is now the synchronization point: the code snapshot is re-read from the file that was actually compiled, the variables cache is cleared, and the functions environment is emptied in place and repopulated. A real recompilation therefore drops previously exposed standalone functions, and a dry run no longer writes into self$functions. Include paths and the user header were cleared at the end of compile() and never fed back, so a second compile() ran without them and a model with #include directives could not be recompiled at all. They are inputs the program needs in order to translate rather than build options, so they now persist for the life of the object and are replaced when new ones are supplied. cpp_options and stanc_options keep their one-shot behaviour deliberately, and a test pins the asymmetry. cmdstan_model() queried the executable twice when it was already up to date. It now queries exactly once on every path. A later review round found three more instances of the same shape. The user header is recorded before its existence is checked, so a failed compile no longer leaves the object claiming no header was requested. Executable metadata is merged after a successful compile, so options inherited from make/local survive a recompilation rather than silently dropping a threaded binary to single-threaded. And functions$existing_exe has a default, so expose_functions() after a dry run gives an informative error rather than failing on a length-zero argument. Separately, check_syntax() and format() never consulted the user-header state, so any model with an external C++ function was reported as a syntax error, even one that had never been compiled. closes #1228 closes #1234 closes #1236
1 parent 653afb5 commit 1cf6b4e

19 files changed

Lines changed: 3129 additions & 232 deletions

‎.Rbuildignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,3 +13,4 @@
1313
^vignettes/articles-online-only$
1414
^release-prep\.R$
1515
^\.vscode$
16+
^dev-notes$

‎NEWS.md‎

Lines changed: 87 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,98 @@ as of CmdStanR 1.0.0; use the lowercase `cmdstanr_no_ver_check` forms instead.
2525
`canonicalize`. The values were shell-quoted for Make and the same quoted
2626
strings were also passed to `stanc` directly, which rejected them. (#1227)
2727
* `$compile()` now enables `allow-undefined` for user headers supplied through
28-
`cpp_options`, not just through the `user_header` argument. (#1227)
28+
`cpp_options`, not just through the `user_header` argument. `$check_syntax()`
29+
and `$format()` also now correctly enable `allow-undefined` for models that use
30+
a user header. (#1227, #1234)
2931
* `stanc` failures during `$compile()` are now reported immediately, with the
3032
`stanc` error message. Previously they surfaced several steps later. (#1227)
3133
* Errors for include paths that do not exist now report the resolved absolute
3234
path. (#1227)
3335
* Numeric `stanc_options` values such as `list("max-line-length" = 78)` are no
3436
longer dropped. (#1233)
37+
* `$compile()` now refreshes `$code()` and `$variables()` after a successful
38+
compilation. (#1228)
39+
* `$compile()` now discards standalone functions exposed from an earlier
40+
version of the Stan program. They must be exposed again with
41+
`$expose_functions()` after a recompilation. (#1228)
42+
* `$compile()` now reuses the include paths and the user header of the previous
43+
compilation when they are not supplied again. Recompiling a model that uses
44+
`#include` directives or a user header through the same object previously
45+
failed because those inputs were dropped. (#1234)
46+
* `$compile()` now recompiles when `include_paths` change. Previously the model
47+
went on using the executable built against the old paths while `$variables()`
48+
and `$include_paths()` described the new ones, so data and initial values were
49+
validated against a program that was not running. (#1235)
50+
* A `user_header` supplied to `cmdstan_model()` is now used by a later
51+
`$compile()`. Previously it was only honored when the model was compiled
52+
immediately. (#1234)
53+
* `$compile()` now accepts `user_header = NULL` to compile without a user
54+
header. Previously a header, once supplied, could not be removed. (#1235)
55+
* `$compile()` now recompiles when the user header changes. Previously a
56+
different header was ignored if the executable was otherwise up to date. (#1235)
57+
* `$compile()` now reduces duplicate `USER_HEADER`/`user_header` entries in
58+
`cpp_options` to the one actually used, so `$cpp_options()` no longer reports
59+
the ignored spelling after a successful compilation. (#1235)
60+
* A `$compile()` call that finds the executable up to date no longer erases
61+
`$cpp_options()`. (#1235)
62+
* `$expose_functions()` now works after a `$compile()` call that found the
63+
executable up to date. (#1235)
64+
* `$expose_functions()` on a model with no executable, such as one created with
65+
`compile = FALSE` or compiled with `dry_run = TRUE`, now reports that it cannot
66+
expose functions instead of failing with "argument is of length zero". (#1235)
67+
* A failed compilation no longer moves `$exe_file()` or replaces the generated
68+
C++ used by `$hpp_file()` and `fit$init_model_methods()`. Previously a failure
69+
at the C++ stage left the old executable paired with model methods generated
70+
from the new program. (#1235)
71+
* `$compile()` now warns when `cpp_options` are supplied but the existing
72+
executable is up to date, so nothing is rebuilt and the options are not applied.
73+
The check is best effort. For an executable the model object compiled itself it
74+
compares the options passed to `Make` against those requested, and treats
75+
anything the binary reports but was never passed as inherited from `make/local`
76+
and so unchanged by a rebuild. For one adopted from an earlier session only the
77+
few `STAN_*` flags the binary reports can be checked, and anything else passes
78+
unremarked. It can also warn when nothing would in fact change: an option
79+
inherited from `make/local` that the binary does not report looks like a request
80+
the executable lacks, and one that was both passed explicitly and set in
81+
`make/local` looks like something a rebuild would drop when it would be
82+
inherited again. Use `force_recompile = TRUE` when a supplied option has to take
83+
effect. (#1235)
84+
* `$cpp_options()` now also reports options the executable was built with that
85+
were never passed to `$compile()`, such as those inherited from `make/local`,
86+
when the binary reports them. `$sample()` and friends previously refused
87+
`threads_per_chain` for an executable that did have threading. (#1019, #1235)
88+
* `cmdstan_model()` no longer runs the model executable twice to read its build
89+
metadata. (#1236)
90+
* `$cpp_options()` no longer reports options the executable was not built with.
91+
Previously a request that did not rebuild the model was recorded as though it
92+
had, so `$sample()` could fail with "the model executable was built with
93+
threading enabled" for a binary that had no threading. (#1019, #1235)
94+
* `$format(overwrite_file = TRUE)` now refreshes `$variables()` along with
95+
`$code()`, which previously kept describing the program as it was before
96+
formatting. (#1235)
97+
* `$compile()` now errors if the newly compiled executable cannot be installed,
98+
restoring the previous executable. Previously the replacement was unchecked, so
99+
a failure could silently leave the model with no executable at all. (#1235)
100+
* `$compile()` now errors instead of installing an executable over a directory.
101+
An executable path that names a directory, which `$exe_file()` and
102+
`cmdstan_model(exe_file = )` both accept without checking, previously had that
103+
directory renamed aside as though it were the old executable and a file put in
104+
its place. (#1235)
105+
* `$compile()` now checks that it can record the compiled model before replacing
106+
the executable, so a failure at that point can no longer leave a new executable
107+
on disk that the model object knows nothing about. (#1235)
108+
* A duplicated `USER_HEADER` or `user_header` entry in `cpp_options` now selects
109+
the last one, matching what `Make` does with repeated assignments. Previously
110+
the first was compiled with and the rest were left in `cpp_options`. (#1235)
111+
* A `USER_HEADER` or `user_header` entry in `cpp_options` set to `NULL` now
112+
clears a previously configured user header instead of being ignored. It stands
113+
for an explicit `USER_HEADER=`, which `Make` takes as clearing anything set
114+
before it, so the model previously compiled with no header while continuing to
115+
report the old one. (#1235)
116+
* A `$compile()` that fails because the user header does not exist now still
117+
records the header. Previously the request was discarded, so `$check_syntax()`
118+
and `$format()` reported the model's own undefined functions as errors and a
119+
bare retry after creating the file compiled with no header at all. (#1235)
35120
* CmdStanModel methods now correctly handle `#include` directories with spaces
36121
in their paths. (#820)
37122
* `$include_paths()` now returns absolute paths, and relative include paths are
@@ -113,6 +198,7 @@ are recompiled lazily if needed. (#1158)
113198
- `stepsize` (`step_size`)
114199

115200

201+
116202
# cmdstanr 0.9.0
117203

118204
## General Improvements/Changes

‎R/cpp_opts.R‎

Lines changed: 137 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,59 @@ model_compile_info <- function(exe_file, version) {
7373
info
7474
}
7575

76+
# Merge build options reported by the executable. Ignore STAN_VERSION and false
77+
# flags (passing FLAG=FALSE back to CmdStan can enable the flag).
78+
merge_exe_info_cpp_options <- function(cpp_options, exe_info) {
79+
for (option_name in names(exe_info)) {
80+
value <- exe_info[[option_name]]
81+
if (tolower(option_name) != "stan_version" &&
82+
(!is.logical(value) || isTRUE(value))) {
83+
cpp_options[[option_name]] <- value
84+
}
85+
}
86+
cpp_options
87+
}
88+
89+
# Normalize the flags sent to make. Assignment names are case-insensitive and
90+
# the last value wins. Nonassignments keep their order. Headers are handled
91+
# separately.
92+
parsed_cpp_options <- function(cpp_options) {
93+
assignments <- list()
94+
opaque <- character()
95+
for (flag in cpp_options_to_compile_flags(cpp_options)) {
96+
if (!grepl("^[A-Za-z_][A-Za-z0-9_]*=", flag)) {
97+
opaque <- c(opaque, flag)
98+
next
99+
}
100+
option_name <- tolower(sub("=.*$", "", flag))
101+
if (option_name %in% c("user_header", "stan_version")) {
102+
next
103+
}
104+
assignments[[option_name]] <- sub("^[^=]*=", "", flag)
105+
}
106+
list(assignments = assignments, opaque = opaque)
107+
}
108+
109+
normalized_cpp_options <- function(cpp_options) {
110+
parsed <- parsed_cpp_options(cpp_options)
111+
reduced <- character()
112+
if (length(parsed$assignments) > 0) {
113+
reduced <- paste0(
114+
names(parsed$assignments), "=",
115+
unlist(parsed$assignments, use.names = FALSE)
116+
)
117+
}
118+
c(sort(reduced), parsed$opaque)
119+
}
120+
121+
# Omitted recorded options count as changes because cpp_options are one-shot.
122+
cpp_options_disagree <- function(requested, recorded) {
123+
!identical(
124+
normalized_cpp_options(requested),
125+
normalized_cpp_options(recorded)
126+
)
127+
}
128+
76129
# convert to compile flags --------------------
77130
# from list(flag1=TRUE, flag2=FALSE) to "FLAG1=TRUE\nFLAG2=FALSE"
78131
cpp_options_to_compile_flags <- function(cpp_options) {
@@ -128,6 +181,78 @@ validate_cpp_options <- function(cpp_options) {
128181
cpp_options
129182
}
130183

184+
# user headers ---------------------------------------------------------
185+
# Resolve one header and remove both header spellings from cpp_options.
186+
# Precedence is explicit user_header (including NULL), USER_HEADER,
187+
# user_header, then previous. `supplied` distinguishes NULL from omission.
188+
# `cpp_options_supplied` limits conflict warnings to this call.
189+
resolve_user_header <- function(user_header,
190+
supplied,
191+
cpp_options,
192+
cpp_options_supplied = TRUE,
193+
previous = NULL) {
194+
# Use positions so duplicate options follow make's last-value-wins behavior.
195+
upper_at <- which(names(cpp_options) == "USER_HEADER")
196+
lower_at <- which(names(cpp_options) == "user_header")
197+
last_of <- function(positions) {
198+
if (length(positions) == 0) {
199+
NULL
200+
} else {
201+
cpp_options[[positions[[length(positions)]]]]
202+
}
203+
}
204+
# NULL is still present here because it emits an empty USER_HEADER= assignment.
205+
has_upper <- length(upper_at) > 0
206+
has_lower <- length(lower_at) > 0
207+
from_upper <- last_of(upper_at)
208+
from_lower <- last_of(lower_at)
209+
conflict <- NULL
210+
spelling <- "USER_HEADER"
211+
212+
if (supplied) {
213+
if (cpp_options_supplied && (has_upper || has_lower)) {
214+
conflict <- "argument"
215+
}
216+
header <- user_header
217+
} else if (has_upper) {
218+
if (has_lower) {
219+
conflict <- "cpp_options"
220+
}
221+
header <- from_upper
222+
} else if (has_lower) {
223+
header <- from_lower
224+
spelling <- "user_header"
225+
} else {
226+
header <- previous
227+
}
228+
229+
# Validate the value now and check file existence when compiling.
230+
if (!is.null(header)) {
231+
checkmate::assert_string(header, .var.name = "user_header")
232+
}
233+
# Guarded because x[-integer(0)] is empty.
234+
header_at <- c(upper_at, lower_at)
235+
if (length(header_at) > 0) {
236+
cpp_options <- cpp_options[-header_at]
237+
}
238+
239+
list(
240+
user_header = header,
241+
spelling = spelling,
242+
cpp_options = cpp_options,
243+
conflict = conflict
244+
)
245+
}
246+
247+
warn_user_header_conflict <- function(conflict) {
248+
if (identical(conflict, "argument")) {
249+
warning("User header specified both via user_header argument and via cpp_options arguments")
250+
} else if (identical(conflict, "cpp_options")) {
251+
warning('User header specified both via cpp_options[["USER_HEADER"]] and cpp_options[["user_header"]].', call. = FALSE)
252+
}
253+
invisible(NULL)
254+
}
255+
131256
# check specific options for validity ---------------------------------
132257
cpp_option_value <- function(cpp_options, option) {
133258
# CmdStanR input and executable metadata can use different casing. Prefer
@@ -206,11 +331,17 @@ exe_info_reflects_cpp_options <- function(exe_info, cpp_options) {
206331
}
207332
if (is.null(cpp_options)) return(TRUE)
208333

209-
cpp_options <- exe_info_style_cpp_options(cpp_options)[tolower(names(cpp_options))]
210-
overlap <- names(cpp_options)[names(cpp_options) %in% names(exe_info)]
334+
# Compare only options reported by the executable. Other options are unknown.
335+
# Parse the emitted flags so duplicates and unnamed assignments match make.
336+
assignments <- parsed_cpp_options(cpp_options)$assignments
337+
reported <- intersect(names(assignments), tolower(names(exe_info)))
211338

212-
if (length(overlap) == 0) TRUE else all.equal(
213-
exe_info[overlap],
214-
cpp_options[overlap]
215-
)
339+
for (option_name in reported) {
340+
# CmdStan treats any nonempty make value as enabled.
341+
requested <- nzchar(assignments[[option_name]])
342+
if (requested != isTRUE(cpp_option_value(exe_info, option_name))) {
343+
return(FALSE)
344+
}
345+
}
346+
TRUE
216347
}

0 commit comments

Comments
 (0)