Design note: v1.0 compilation state and C++ options - #1254
Merged
Conversation
Records the contracts behind #1228, #1234, #1019, #1237 and #1238. These have not been independent defects: each was rediscovered by being violated, because the rules they violate were never written down anywhere. Describes what is recorded about an executable and when, what a configuration means once it reaches make, when that record is validated, and what can be known about an executable cmdstanr did not build. Two decisions reverse earlier ones: options become one-shot at cmdstan_model(), and deferred compilation is removed. The note is a draft for discussion and is deliberately ahead of the tracker. Several issues still assert decisions it supersedes, #1248 most of all, so it lists them explicitly rather than leaving someone to read a stale issue as current. Updating those issues is held until the design settles.
The architecture is unchanged. This resolves contracts that were internally
inconsistent or underspecified.
Validation becomes a pure freshness assessment with two caller behaviours
rather than one rule: cmdstan_model() rebuilds on a trigger, and every
operation that runs or derives state from the binary errors. Stating both as
a single contract read as a contradiction between sections 5 and 6. The error
no longer advises force_recompile after source or configuration changes, since
the constructor detects those on its own; that advice is reserved for corrupt
records, artifact mismatches and explicit distrust.
Executable-only models split into two cases. One produced by compile_stan_file()
and then adopted has a valid hash-bound record, and treating every adopted
executable as unprovenanced discarded information the package itself wrote.
Raw NAME+=value and its siblings are classified as assignments rather than
opaque arguments. Verified against make: every operator collapses to = with
command-line origin, so list("FOO+=x") and list(foo = "x") describe the same
build and must compare equal. Include re-resolution invokes stanc rather than
reimplementing its rules, since stanc --info measures 29.9 ms against a 30-90
second compile and reproducing those rules imperfectly would reintroduce the
silent-stale-binary problem.
provenance_complete becomes known_untracked_dependencies. A regex can show
that a gap exists but never that none does, and the note already warned
against exactly this reasoning for reported_features.
The stages reorder so the deferred-compilation lifecycle is removed before the
record drives any decision, which avoids implementing transitional behaviour
the final design does not specify.
Fourth review round. No architectural change; these are implementation contracts that were underspecified or that the new choices made inconsistent. The introspection snapshot is captured eagerly. $variables() parses from disk on first call, so an edit made before that call would describe the new source while claiming to describe the built one, violating the contract by the mechanism meant to implement it. The assessment already invokes stanc --info for include resolution and the same output carries the variables, so the constructor commits it after a successful build. $format(overwrite_file = TRUE) no longer refreshes the caches: formatting makes the object stale rather than updating it. Include comparison drops the recorded spelling, search roots and selected path in favour of the included_files vector stanc --info already returns, verified to come back fully resolved. Re-resolution invokes stanc from the recorded builder rather than whichever installation is currently selected, and builder identity is checked first so a mismatch is reported without re-resolving. The tri-state reported_features contract gains the consumer policy it was missing. Unknown status errors when an operation requires the feature, scoped to runtime arguments that depend on a build feature so that permanently unreportable options like CXXFLAGS do not error on everything. assert_valid_threads() changes rather than being preserved: it currently stops when a threaded binary has no threads argument but merely warns and discards the argument in the converse case, and both are the same mismatch. The API change and the decision engine become one stage. Separating them leaves a window where an existing unthreaded executable is reused while $compile(), the only escape route, is already gone.
Fifth review round, and the last one: approved after this. The tri-state consumer table was doing two jobs. It now covers one case explicitly — a runtime argument asking for a build feature — where known disabled and unknown both error. The converse, an artifact carrying a feature nobody asked to use, is stated as its own policy rather than an instance of the table, because it is not a mismatch at all. That policy keeps today's error for a threading-enabled binary run without a threads argument, on the grounds that building with threading and not using it is more likely a mistake than an intention. Two things make that conservative rather than new: it has five assertion sites in test-threads.R plus snapshots, and it is already reachable for threading inherited from make/local, since $cpp_options() has merged executable metadata on the construction and no-op paths for some time. #1235 extends that merge to the fresh-compile path, making the behaviour uniform rather than introducing it. The cost is now stated: a user with STAN_THREADS=true in make/local must pass a threads argument every run. Path normalisation is settled rather than open. Normalised absolute paths, and relocating a project rebuilds. Relocatable records would require defining roots, symlink behaviour and out-of-project paths for little benefit, and the case where rebuilding is impossible is already covered by executable-only models.
Two gaps found while checking the note against a summary written from it. The record had no format_version. The third draft moved the field enumeration into the vocabulary section and dropped it, leaving the forward-compatibility rule with nothing to check. It is restored, along with known_untracked_dependencies, which had the same problem: specified in the rebuild section but absent from the list of what a record holds. The rebuild trigger list was source-side only. A replaced or corrupt executable, an unreadable record, and an executable predating records are all reasons to rebuild, and omitting them left the canonical list disagreeing with the sections that describe them. A record whose format_version is newer than we understand is deliberately not among them: rebuilding would install a replacement over a record written by something that knows more, which is what the forward-compatibility rule exists to prevent. Unreadable and readable-but-newer look alike and are now stated as distinct, since conflating them is how the rule gets broken.
A reader currently passes about a hundred lines of purpose and history before reaching a concrete decision. This gives the shape in one screen: what the API becomes, what triggers a rebuild, and what the record holds. It is explicitly orientation rather than specification, so the sections below stay the single place the contract lives. It also takes over some of the orienting work the history section does, which is due to be removed once the tracker catches up.
The directory is developer documentation, not package content, so R CMD check would otherwise flag it as a non-standard top-level file. PR #1235 adds the same line on its own branch; this makes it independent of that PR's merge order.
This was referenced Aug 27, 2026
Closed
Open
The note carried two things whose only job was to survive the gap between the design settling and the issues catching up: a narrative of the two superseded drafts, and a list of issues that would mislead a reader by still asserting decisions this reverses. Both are now false rather than merely unnecessary. #1247, #1248 and #1252 are closed with their reasoning, #1238, #1250 and #1253 are rescoped, and #1255, #1256 and #1257 carry the new work. The trust direction goes back to normal: the issues are the specification, this note is the reasoning behind it. The rejection of persistent options survives, distilled into section 2. It is the most tempting alternative in this design and the one most likely to be proposed again, so the argument for it and the reason it fails belong with the contract rather than in a history section.
…model() An earlier version said to export it only if a consumer committed to it, on the grounds that citing instantiate as motivation was speculative. That was the wrong bar. The argument is parity rather than demand: cmdstanpy already has compile_stan_file, and exporting format_stan_file() and check_syntax_stan_file() while withholding the compile step is arbitrary — with compile = FALSE gone there would be no way to build without constructing an R6 object. Both entry points call one internal, which returns the executable path plus the record, the stanc --info output and the generated C++. Returning only a path would make cmdstan_model() re-read the record and re-run stanc, which is duplication in its most wasteful form; the src_info is what feeds the eager introspection snapshot, and the presence or absence of hpp_code is what answers the generated-C++ question in #1245. dry_run stays on the internal, which is the only argument the public wrapper omits. compile_stan_file() performs the same up-to-date check rather than always compiling, and writes the record, so adopting its output later carries provenance. force_recompile keeps cmdstanr's spelling rather than cmdstanpy's force. Matching the function name is what makes the two APIs teachable together; matching every argument at the cost of internal consistency is not.
jgabry
marked this pull request as ready for review
August 27, 2026 22:13
Member
Author
|
@andrjohns when you have a chance take a peek at my list above (you don't need to read the full document in the PR that Claude, it's just more details on all the items that I wrote in that list above). This redesign of the compilation/build process for 1.0 came out of discussion with @WardBrian and @SteveBronder. I think it's a much cleaner design than what we currently do (and actually simpler in many ways, despite the additional build record) and it replaces the previous half-done C++ options work that never got finished. You can ignore all the issues that have been opened lately, they're just based off of this list and I'll close them as I go through the implementation. I'm hoping to start working on this ASAP. |
Section 9 gave the ordering but said nothing about execution. Adds the release candidate as a third constraint on the order: stages 0-4 must all be in it, because the API removal is the breaking change downstream packages need to see, while stage 5 only adds a function and can follow. The candidate period is also the real use stage 5 was already waiting on. One constraint falls out of that. The repo-wide formatting and linting work (#1153, #1172) has to land before stage 1 or after 1.0, never between stage 4 and the candidate, where a reformatting diff on top of the API removal would hide what actually broke. Adds a note on how the stages are run: one pull request each, stage 4 built as a tested pure engine before the wiring and the API removal, and only one compiling task at a time, since make/local and the precompiled headers live in the CmdStan installation rather than the checkout and separate checkouts do not separate them. Drops the joint cmdstanpy naming process. Where cmdstanpy already has a name we copy it, and otherwise we pick one and they can copy it; nothing here needs to wait on that. Also brings the issue-consolidation note up to date. It still described the work as pending and pointed at a section that has since been removed.
JSON, named <model>.cmdstanr.json beside the executable. jsonlite is already an import, so the format costs nothing; the name stays clear of .dep and .d, which make and the C++ toolchain already claim in that directory. Stage 2 no longer has to settle this, but portability and the git-ignore story are still open and still have to be answered before anything writes a file. Choosing JSON adds a third way to get the tri-state fields wrong, so the note in section 10 now says that unknown has to round-trip as distinct from both absent and false, and that this is a property to test rather than assume.
Section 6 treats an executable predating records as a rebuild trigger. Since 0.9 stays installable from GitHub, that transition could be tested in CI rather than waited for in the wild. Recorded as a possibility for stage 4 to weigh, not as a commitment; building such an executable by hand when it is needed may well be enough. Also corrects stage 4's issue references. It still said the stage closes #1252, which the consolidation already closed, and pointed at #1019 rather than the #1255 and #1256 that were opened to carry this work.
Air's one-time whole-repo format goes last, immediately before 1.0. It is whitespace-only and deterministic, so shipping it after the candidate is cheap, and by then nothing is left for it to conflict with. Its pull request review action is a separate matter and is better landed early, while stages 2 to 4 are writing the code it would otherwise reformat afterwards. Jarl is not the same kind of change. Adopting it is additive, but acting on its findings is semantic editing, and that cannot follow the candidate without 1.0 shipping code in a form nobody tested. Those are ordinary reviewed changes. The previous note offered "before stage 1" as an option. That was never really available, with #1235 and #1254 both open.
1.0 reads exactly the format it writes, and any other version is an artifact-side reason: rebuild with a source, unprovenanced without one, reported as unsupported_format and never refused. That was already the rule; it sat under five paragraphs telling future releases when a bump is owed, how hashing and canonicalization changes interact with one, and why refusing to replace a forward-version record was rejected. None of that binds 1.0, and a release that widens the readable set will write its own rule when it does.
The stanc_name row stays: the build bakes --name into the binary, no other compared field pins it down, and a coordinated rename would otherwise reuse a binary that stamps the old name into every CSV. The row was carried by six paragraphs arguing the CSV boundary, the mangling measurements and the limits, which is more defence than a row that meets the table's own criterion needs. One paragraph now states the reason, the raw-versus-mangled rule, and the executable-only limit.
4 tasks
The rule moved out in the previous commit is filed as #1261, so the three places that named it by title point at the number.
Nothing said what happens when the stanc info call fails while the sources are being re-resolved for an assessment. A reviewer read the gap as needing a third engine state. It does not: resolving is the caller's job and the engine never sees a failure, so the caller raises it. At construction and at every guarded method the error carries stanc's message, nothing runs and nothing rebuilds, since the build's own stanc call would fail at the same point. #1237's request that an unresolvable include fail toward rebuilding is reworded there to match.
Read literally, the guarded-method precondition asked every model for a valid record before running, and an executable adopted on its info output alone never has one, so every instantiate fit would have errored. The check for an executable-only object is now stated once: compare the executable's hash to the one the object was constructed with, and nothing else, whether or not a record was present at adoption. A replaced binary errors, an unchanged one runs, and a record that goes away after construction changes nothing.
Passages that argued against an earlier draft, a reviewer's proposal or a full review round explained how the document got here rather than what it says, and the rejected alternatives were argued at the length needed to win them the first time. Both go. Each rejected idea is now one line in an appendix, the idea and why it lost, and the section that owns the replacing rule keeps only the reason the rule needs. The longest of them, path-and-content identity for sources, keeps its worked example since it is the closest call in the document.
The table in §4 is the single statement of what is recorded and what is compared, and the paragraph above it says prose elsewhere must not restate a row. Seven passages did, each restating a row's answer and then citing the table for it. They now point and stop, and the two that were nothing but a restatement are gone.
A pass over every section for the register the document was written in: em dashes, bold-led bullets, negative listings, "deliberately" and "genuinely" doing no work, rhetorical setups, and abstraction nouns standing in for a verb. Rules, tables, measurements, code and file references are unchanged; the two remaining "earlier draft" remarks in §10 are gone with them. Stage headings use a colon instead of a dash.
A hunk-by-hunk read of the rewrite against its parent found three places where a sentence lost a claim rather than a mannerism. The validator paragraph in §1 pointed at the consumer table and now does again. The stanc_name row said the CSV header carries both the raw name and the mangled one, which is the observable the row rests on. And §7 stated outright that executable-only models are kept, which the adoption paragraph had reduced to an implication.
…d version once each Three figures the doc measured or quoted in one place were repeated as literals in two or three others. The secondary mentions now point at the owning passage, so a remeasurement or a moved line in CmdStan's example file changes one place.
Two cited a neighbouring line (cpp_options_to_compile_flags is defined at R/cpp_opts.R:131, the space-to-underscore substitution is R/model.R:273), one cited the roxygen line before the one naming $expose_functions(), and three bare :NNN references sat closer to a different file than the one they meant, so they now name it. The §4 include-path aside said $sample() calls $variables() unconditionally; R/model.R:1410 guards that call on a registered Stan file, so it now says so.
The §3 rejection rule matched a named entry on its name and an unnamed
entry on its value, and the fresh review found the spelling that slips
between the arms: list("include-paths=/b" = TRUE) has a name that is not
include-paths, and the converter's logical branch emits --include-paths=/b.
The name slot has to hold a flag name for the named arm to be sound, so a
name containing = is refused, the shape check assert_valid_stanc_options()
already makes for a leading --. This is the move §3 made for cpp_options
names and +.
Measuring the case turned up a version dependence the doc did not have:
stanc 2.35 and 2.36 search the last-given include path first, 2.38 and
2.39 the first-given, and 2.37 refuses a repeated --include-paths flag,
so §6's claim that stanc accumulates repeated flags now says so.
§8 said fit$init_model_methods() does not depend on the reuse path. It does: the C++ it compiles model methods from is generated only inside the build branch (R/model.R:848), the fit copies an empty environment when an executable was reused, and test-model-methods.R:108 asserts the resulting error. $hpp_file() points at the same text and has the same hole, which its roxygen documents. §5 already makes introspection a construction-time snapshot because construction is the one moment source and executable are verified to agree. The model's C++ is the same kind of thing, consumed by a fit that cannot validate, so the constructor now generates it on both paths, with the call the build branch already makes moved ahead of the branch. The standalone-functions C++ stays on demand, since its consumer is guarded. Cost measured on 2.39.0: 28 ms on bernoulli, 112 ms on an 807-line model. #1245's discriminator dissolves: a source-backed model always has the C++, an executable-only one never does. §7's list of what executable-only construction gives up grows accordingly.
§9 had one pull request per stage merged to master, arguing that a long-lived branch spends more time being rebased than reviewed. That weighed the wrong cost. Enough people install cmdstanr from GitHub master that the breaking stages would reach them one at a time over weeks. The stages now land on a v1.0 branch, still one reviewed pull request each, and the branch merges to master at the release candidate. The dev-version boundary brms guards on is the branch's rather than master's.
§3 deleted exe_info_reflects_cpp_options() in Stage 1 on the grounds that §7's rejection of cpp_options beside exe_file left it no input. Its caller is the reuse branch of $compile() (R/model.R:777), which every fresh session reaches when it constructs a source-backed model whose executable already exists, and twelve test assertions expect the warning it raises. The real reason it ends is Stage 4: the engine compares the request against the recorded cpp_options_supplied and rebuilds, so a request-against-report diff has nothing left to do. Stage 1 now drops the tolower() fold that canonicalization would otherwise empty, and the helper goes with its branch and tests in the pull request that wires the engine.
§1 said a threaded binary run without a threads argument is correct and serial. It is not serial once the error is removed: the four launch sites set STAN_NUM_THREADS in the R session when threads are supplied, CmdStan reads that variable as its default when num_threads is absent, and the variable persists, so an omitted call inherits the count of the last call that gave one. Thirteen test assertions check the session variable and so encode the leak. The variable now goes to the child process through processx's env argument, set when threads are supplied and absent otherwise, so an omitted call sees the user's own environment and nothing cmdstanr wrote, the treatment §6 gives Make variables. Passing num_threads= on the command line was rejected because CmdStan refuses to start when it disagrees with a set STAN_NUM_THREADS.
§9 settled precedence for --filename-in-msg between injection and stanc_options and said nothing about make/local's STANCFLAGS, which cmdstanr appends to its own flags for both stanc invocations. Measuring the repeat showed the gap is wider than one flag: stanc 2.35 and 2.36 accept a repeated flag, 2.37 refuses every repeat, and 2.38 and 2.39 refuse a repeated valued flag and warn on a boolean one. So pedantic = TRUE against the --warn-pedantic line make/local.example suggests, which §6 called fine, fails the build on 2.37 today. §6 now has one rule for the whole class: a flag the call emits, supplied or injected, wins, and the make/local occurrence is dropped from the resolved vector before either invocation, matched the way §3 matches an unnamed entry. That is the principle §3 already states for cpp_options. The include-path rejection is unchanged, since it exists for resolution consistency rather than duplication.
The include-shadowing subsection said to store the normalised included_files vector and compare it, which reads as a path comparison and contradicts the positional hash rule fifty lines earlier, where normalisation matters only for built_from. It now names the hash comparison and defers to it. Its restatement of the current-call include-path rule shrinks to a pointer at the §6 copy #1255 quotes, and §10's restatement of the resolve-before-recording rule shrinks to a pointer at the §8 copy #1258 quotes, keeping only its one fact of its own, that $include_paths() already reports the defaulted value.
A make/local STANCFLAGS entry such as --filename-in-msg published.stan splits into two elements. Dropping only the flag under the precedence rule left the value behind as a second positional argument, which stanc refuses. The element after a bare-flag match goes with it when it does not begin with --, which needs no knowledge of which flags take values.
stanc has single-hyphen options such as -fno-soa. Testing the element after a dropped flag for a leading -- would consume one as the flag's value and silently change the generated C++. A value never starts with a hyphen, since 2.37 and 2.39 read any hyphen-led token as an option.
compilation-state.md is 31,000 words of rules and the reasons for them, and a reader who only wants to know what happens for a given record, executable and call has to find the rules inside the reasoning. The normative blocks are now marked with HTML comments, invisible when the note renders, and build-contract.R copies them out in order under the heading each sits beneath, with a link back to that heading, into compilation-state-contract.md. Each rule keeps its one owner; the view is regenerated, never edited. The script refuses unmatched or nested markers and has a --check mode that fails when the view is stale.
The first pass marked whole paragraphs wherever a rule was spread through one, which carried the reasoning along with it and made the view a third of the note. Blocks now end where the rule ends: the lead sentence, the definitions the rule depends on, and consequences stated as behaviour. Tables and the two API code blocks are unchanged. 217 blocks, about 8,900 words against 10,100.
stanc 2.38 added -I as the short spelling of --include-paths, so a make/local STANCFLAGS carrying -I /b reached the build and not the stanc --info call re-resolution is built on, bypassing the rejection that exists to keep the two consistent. The effective-STANCFLAGS check now also refuses an element beginning with -I. The precedence rule matches the spelling the call emits; 2.39 accepts no abbreviations and refuses a repeat in another spelling by name.
The §4 version block stopped before the sentences saying only a readable format_version proceeds to schema validation, so the contract stated the unsupported_format outcome without its condition. The §3 USER_HEADER block now starts one sentence earlier so "It reaches it as a flag" has its antecedent.
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.
Adds
dev-notes/compilation-state.md. No code changes.This is the current plan for v1.0, developed based on conversation with @SteveBronder and @WardBrian.
AI disclosure: the compilation-state.md file is being written iteratively via a back and forth with Claude and Codex. It is based on the list below, which is my own summary of the planned changes. This is somewhat of an experiment, and if it goes poorly I may end up writing the document myself from scratch.
New API
cmdstan_model()checks if the existing executable matches the requested Stan file (and includes and user headers) and options (see section on new build record below). If everything matches we reuse the executable, otherwise we recompile.cmdstan_model(exe_file = )stays. A pre-built executable still works, but$code(),$variables(),$check_syntax()and$format()need a Stan file, and passing build options (cpp_options,stanc_options,include_paths,user_header,force_recompile,pedantic) with only an executable is an error, since there's nothing to rebuild.cmdstan_model(stan_file = , exe_file = )together is now an error. That usage ofexe_filewas just used to indicate where to put it, anddirdoes that anyway (albeit without filename customization).compile = FALSEargument tocmdstan_modeland the$compile()method). This means we losecompilearguments likecompile_model_methodsandcompile_standalone. But$expose_functions()and$init_model_methods()already do the same job.format_stan_file()check_syntax_stan_file()stan_variables()compile_stan_file()stan_build_info()for inspecting how an executable was built$code()and$variables()refer to the Stan file used to build the executable, even if the Stan file has changed since (needs to be recompiled)cmdstan_model()again, notforce_recompile = TRUEsincecmdstan_model()will now know what to docpp_optionsare rejected, e.g.list("STAN_THREADS=TRUE")has to belist(stan_threads = TRUE)user_headeris only settable through theuser_headerargument (notcpp_options) and a new method$user_header()is added to read it back.When do we recompile
If the user sets
force_recompile = TRUEor when any of these change:include_paths(when the change means different content, not e.g. a directory rename)cmdstan_model()at a different one) or its pathcpp_optionsorstanc_optionsthe user supplied (options cmdstanr fills in itself are recorded but don't trigger a rebuild, except the model name, which we get from the file name and we do compare, so renaming your .stan file recompiles)Or when we can't trust what we recorded:
If more than one of these applies we report all of them (if possible).
One exception:
cmdstan_model(exe_file = )) can't recompile automatically since there's no Stan file to build fromThe new build record
The current plan is a file
.<exe>.cmdstanr.jsonthat is written next to the executable. It contains:cpp_optionsandstanc_optionsthe user suppliedinclude pathsand user header pathstanc --infoTracking issue: #1258