Skip to content

Commit a679876

Browse files
committed
Record what brms needs from this design
brms keeps a whole CmdStanModel in attributes(fit$fit), so a saved brmsfit carries our object and its absolute paths to whatever machine next opens it. That case needs nothing new: assessment already reads the filesystem each time it is asked, and the artifact hash already covers a different executable sitting at the same path. What would break it is caching the verdict on the object, which is right within a session and silently wrong after deserialization, so section 5 now prohibits that while the reason is still visible. It also records that $exe_file() has to stay a plain, non-erroring accessor. brms calls file.exists() on its result to decide whether to recompile, and that is what handles a relocated fit before anything asks us for a verdict. The release candidate section now names brms alongside instantiate. Its throwaway compile = FALSE object, built only to call $check_syntax() and $code(), is exactly the case the standalone family was designed to replace, which is the best confirmation yet that the family is shaped correctly. We open those pull requests ourselves, against the candidate, guarded by version since brms is on CRAN and cmdstanr is not.
1 parent c78ebcd commit a679876

1 file changed

Lines changed: 41 additions & 3 deletions

File tree

dev-notes/compilation-state.md

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -485,6 +485,30 @@ Cost is **~8.8 ms**: source hashes plus the executable hash. An earlier draft ci
485485
include re-resolution runs here too (§6) add ~30 ms. Still negligible against a
486486
sampling run, and the accurate number belongs in the document.
487487
488+
### The verdict is not state, and must not be stored as state
489+
490+
Assessment reads the filesystem each time it is asked, and its answer is good only
491+
for that moment. That matters most where a model object outlives the machine that
492+
built it, which is not hypothetical: brms keeps a whole `CmdStanModel` in
493+
`attributes(fit$fit)`, so a saved `brmsfit` carries our object — and its recorded
494+
absolute paths — to wherever the fit is next opened.
495+
496+
That case needs nothing extra. Where the executable is absent the assessment fails
497+
and the operation errors; where a *different* executable sits at the same path, the
498+
artifact hash (§4) catches it rather than running the wrong binary. Both fall out of
499+
the contract above.
500+
501+
What would break it is memoization. **~8.8 ms per operation is exactly the number
502+
someone later decides to cache on the object**, and a cached verdict is right within
503+
a session and wrong the moment the object is deserialized somewhere else. That
504+
failure is silent and looks like a caching bug rather than a violated contract,
505+
which is why it is worth prohibiting here while the reason is still visible.
506+
507+
One knock-on for consumers: **`$exe_file()` must stay a plain, non-erroring
508+
accessor.** brms's `needs_recompilation()` reads it and calls `file.exists()` on the
509+
result (`brms/R/backends.R:377-383`), and that is how a relocated fit gets
510+
recompiled before anything asks us for a verdict at all. #1253 must not change it.
511+
488512
### Introspection is a construction-time snapshot
489513

490514
Pre-operation validation does **not** make cached introspection safe: `$code()` and
@@ -919,9 +943,23 @@ candidate, or after 1.0.
919943
920944
### The release candidate
921945
922-
A 1.0 candidate ships after Stage 4, so packages built around precompiled models —
923-
`instantiate` most directly — have something to migrate against rather than a
924-
release note. That is what makes §8's breaking change affordable.
946+
A 1.0 candidate ships after Stage 4, so downstream packages have something to
947+
migrate against rather than a release note. That is what makes §8's breaking change
948+
affordable.
949+
950+
Two are known to be affected. `instantiate` is built around precompiled models.
951+
**brms** uses `cmdstan_model(compile = FALSE)` in `.parse_model_cmdstanr()`
952+
(`brms/R/backends.R:23-34`) to build a throwaway object solely for `$check_syntax()`
953+
and `$code()`precisely the case §8's standalone family replaces, and the clearest
954+
confirmation so far that the family is the right shape. Note that it forwards `...`,
955+
so `check_syntax_stan_file()` has to accept what users pass through `brm()`. brms
956+
also sets `cpp_options$stan_threads` only when threading is requested, so its users
957+
meet §1's threading policy as a rebuild when they toggle it off.
958+
959+
We open those pull requests ourselves rather than waiting to be asked; the candidate
960+
is what they are written and tested against. They need to work against both the old
961+
and the new cmdstanrbrms is on CRAN and cmdstanr is notso a version guard
962+
rather than a clean switch.
925963

926964
The formatting and linting work is scheduled around this, and the formatter and the
927965
linter go to different places.

0 commit comments

Comments
 (0)