Design note: v1.0 compilation state and C++ options - #1254
Draft
jgabry wants to merge 73 commits into
Draft
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 dep file) 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.
86 tasks
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.
Section 2's title and the raw-assignment rejection both named cmdstan_model()
specifically, which left compile_stan_file() outside two contracts it shares the
implementation for. The rejection was the consequential one: as written,
compile_stan_file(cpp_options = list("STAN_THREADS=true")) bypassed it entirely,
so the rule could be avoided by choosing the other entry point.
The scope qualifier was added to protect cmdstan_make_local(), which passes
unnamed entries into a file where += is real. That distinction is between
configuring a build and writing make/local, not between the two functions that
build, and it now says so.
Three summaries still described the mechanism as recognising records from a newer cmdstanr, which was the rule before it became bidirectional: the field list in section 1 defined format_version as the schema's own version, stage 2 tested "forward versions", and section 10 singled out newer records. All three now say a version this cmdstanr does not support, in either direction, and the field list says what the version actually versions — the build interpretation contract, with a bump obliged when option or build semantics change. Also separates two words that were being used interchangeably. Unreadable means the file could not be parsed; unsupported means a version was found that we do not interpret. Keeping them apart matters because a supported record can also be corrupt, and the messages differ.
"Losing a record never breaks anything" and "the model still samples" contradicted section 5, which errors when an already-constructed source-backed model finds its record gone. Only executable-only adoption keeps fitting without one. The three cases are now separate: source-backed construction rebuilds, guarded operations on an existing source-backed object error and point back at cmdstan_model(), and executable-only adoption continues with reduced provenance. What survives from the original claim is narrower and still worth stating — losing a record never corrupts the executable. Also drops the two stray .hpp files from the test-resources observation. They were scratch from a reviewer's stanc probe rather than a standing property of the branch, and the working tree is clean. The four missing executable entries are real and carry the argument for pattern-based ignores on their own.
"Say something when the user asks for what we cannot deliver" was not an implementable contract — it named neither the response nor the conditions. With no stan_file, explicitly supplied cpp_options, stanc_options, include_paths, user_header or force_recompile is now an error. None of them can configure an artifact that will not be rebuilt, and a valid record exists to be inspected rather than overridden. The check is on whether the argument was supplied, not on its value, and force_recompile is why: its default comes from a global option, so a check on the resolved value would error for every adoption performed by anyone who has that option set, including every instantiate fit, from inside a package the user never chose to look at. Explicit argument errors, option-sourced value is ignored, and the option's help page says it has no effect on executable-only models. A NULL sentinel is preferred to missing(), which is fragile through the wrapper layers instantiate reaches us through. Also states that force_recompile never enters the record. It changes whether we build, not what we build, so two identical builds must produce identical records whether or not one was forced. The rebuild reason should name the option when the value came from there, since "force_recompile = TRUE" means nothing to someone who set it in .Rprofile and passed nothing.
Surfacing it at every construction collided with the instantiate finding, where construction happens on every fit — a user with the suggested make/local include pattern would have received the same note forever. Keying on the record being written fixes it without state: it is the moment the information is new, adoption never writes, so the note reaches whoever ran the untrackable build and nobody else. Once per session would also work but needs a cache of what has been said, and section 5 prohibits stored verdicts for reasons that apply to any sibling. Separately, the clone note claimed the two objects cannot diverge, a few lines after acknowledging that a second $initialize() can retarget state. Qualified to "through the supported API", which preserves the decision not to guard it without claiming the unguarded case is impossible.
Purpose and scope still said the installation path is not compared. That was the version-only rule, and it survived the at-a-glance rewrite because it sits in a different section — the same reviewer finding, in a place the restructure did not reach.
A dependency now matches on its content hash wherever it lives. The absolute path it had at build time is recorded as built_from, which is provenance: never compared, never rewritten, and needing no mechanism since records are replaced whole on every build. Moving a project no longer rebuilds it. Path-and-content identity had a real argument behind it. stanc bakes absolute source paths into locations_array__, so two builds at two paths genuinely produce different binaries. It was rejected because the difference is one string in an exception message that still carries the correct line and column, while the everyday case of renaming a working directory pays a full recompile for a benefit its user never receives. The exotic cases do not rescue it: cross-machine scenarios rebuild on builder identity anyway, so path was only ever the sole trigger for a folder rename. include_paths and the user header's path are recorded but likewise not compared as spellings. Comparing them would have reintroduced path sensitivity through the request for every model with an include or a header, defeating the change for the population it serves. An include path's whole effect is the set of files it resolved, and that set is compared by content and re-resolved through stanc --info. instantiate keeps executable-only adoption, but on new grounds. The previous reasoning was that staged installation builds under a 00LOCK path that no longer exists, forcing a rebuild on every first fit; content identity dissolves that. The durable reason is that registering source makes a compile inside a fit representable at all, which is the promise instantiate cannot break. An edited file in the package library or a second .libPaths() entry both reach it. Corrects the account of #1094. It was not a deferred $compile() losing include paths but $sample() reaching $variables() and invoking stanc with none, so include_paths configures source resolution rather than the build. Two traps follow: the dirname(stan_file) default is load-bearing behaviour rather than scaffolding, since stanc fails outright without it and instantiate's install path supplies no include paths at all; and whatever resolves that default must do so before the request is recorded, or include re-resolution reproduces #1094 at validation time. Adds --filename-in-msg to stage 4. Every runtime exception from every cmdstanr model currently names a deleted tempfile, because compilation runs against a copy rather than the source. Also removes every reference to earlier drafts of this document. The note is a specification and should read as current truth; where a rejected alternative carried an argument worth keeping, the argument stays and the history goes.
jgabry
marked this pull request as draft
August 29, 2026 04:53
Round-five removed include_paths from the rebuild triggers but left section 6 re-resolving includes with the paths held in the record. With the comparison gone, that made changing include_paths a no-op: stanc would be pointed at the old directories, find the old files, report matching hashes, and reuse a binary the caller did not ask for. Re-resolution now uses the effective paths of the current call; the recorded ones are for replay and provenance. The user header needs the opposite treatment and now matches on normalised path as well as content. CmdStan feeds it in as -include, and a quoted include inside it resolves relative to that header's own directory, so two byte-identical headers in different directories are different translation units. A worked example is in section 6: the same top-level hash, implementations returning 0.3261 and -1.0000, and every compared field matching. This is not an exception to content identity but the same rule where our information is incomplete -- a Stan program's include closure is fully visible through stanc, a C++ header's is not, so the directory stands in for the part we cannot hash. Section 6 already does this once, for make/local. Both of those were derived wrongly at least once because the record's rules were spread across three sections and never written down as rules. Section 4 now carries a per-field recorded/compared table and is the authority on both; prose elsewhere refers to it rather than restating it. Recorded-but-not-compared turns out to be the ordinary case, not a list of exceptions, and that reframing settles the injected stanc options: cmdstanr's own additions are recorded and never compared, so toggling pedantic does not force a recompile. Checking for rules stated twice immediately found two stale ones in section 3. Includes are compared as an ordered sequence rather than a set. That is exact where a set is not, and it is also the simpler implementation, since position gives the error message its file name without a second pass. Also here: variables() must be captured at construction, since parsing on first call makes the answer depend on whether anyone asked before an edit; the standalone family all take include_paths, without which format_stan_file() could not format any program containing an include; filename-in-msg precedence is settled rather than left open; stan_build_info() reports built_from and whether it still exists, because it receives only an executable and cannot resolve a current location; and the .libPaths() example is gone, since instantiate derives the executable from the source it just found. The 30-90 second compile figure was never measured. Replaced with 6.7s and 13.8s for bernoulli.stan with and without precompiled headers, stated as a floor, and the one argument that leaned on the number re-grounded on the caller not having asked to build at all.
The user header's compared path was framed as an exception to content identity, which left the -I directories a user can put in cpp_options looking like a second exception needing its own rule. They are not. Both are directories that decide how a quoted #include resolves, both are already compared -- the header's path as a dependency, the flags inside request.cpp_options -- and neither has its contents tracked. Stating it once as a general rule removes the question rather than answering it again, and the untracked-content gap reaches its existing message through the #include in the header, not through the flag, so nothing changes about when that fires. Section 9 restated the stage plan that #1258 also holds, and the two had already diverged: four of #1258's stage 4 items were absent here. Say plainly that #1258 is the work list and this section is only the reasoning behind the order, then drop the enumerations that duplicated it. Also fixes two positional references that had gone stale or wrong -- the rebuild triggers are now grouped the way #1255 groups them instead of being counted -- and replaces a fourth surviving copy of the unmeasured 30-90 second compile figure with a pointer to the measured one.
The pure decision function was the first half of stage 4, so section 6's contract stayed prose until the riskiest change in the plan was ready to ship. It depends only on stage 2's fixtures, compiles nothing and is called by nothing, so it separates cleanly and can be worked alongside stage 3. Separating it is what makes this section checkable. A rebuild trigger written as a decision-table row with a fixture fails loudly when it contradicts another rule; the same two rules written as paragraphs eight lines apart survived a full review round, which is how re-resolution came to use the recorded include paths while the section above said the spelling was not compared. The argument that the API change and the decision engine ship together is unaffected. It is about the engine being live while $compile() is gone, and an unwired function changes nothing a user can observe.
…on origin
Four channels reach the stanc invocation that builds a model: the
include_paths argument, stanc_options, make/local's STANCFLAGS, and
STANCFLAGS set through cpp_options, which arrives as a make command-line
assignment that our own "STANCFLAGS +=" appends to rather than replaces.
Only include_paths reaches model_variables(), the stanc --info call the
verdict re-resolves with, so a path supplied any other way resolves for
the build and nowhere else. That is already a live bug: a model built
with stanc_options = list("include-paths" = p) compiles and then fails on
$sample(), which calls $variables() unconditionally. Section 6's argument
for not comparing include_paths as a spelling assumed a single channel,
so the other three are now rejected and the argument holds by
construction rather than by luck.
The two rejections differ in scope on purpose. cpp_options is our own
argument, so the whole STANCFLAGS variable goes and stanc_options is the
place for stanc flags. make/local is CmdStan's configuration file, which
ships STANCFLAGS+= --warn-pedantic as a suggested line, so only the
include-path flag is refused there.
Option origin is now two stored fields per list rather than one merged
list plus a rule about it. pedantic = TRUE and stanc_options =
list("warn-pedantic" = TRUE) produce the same flag and have to compare
differently, which nothing in a merged blob can express. What we inject
becomes effective minus supplied, computed instead of enumerated, which
removes a list that had to stay in sync in five places. The warn-pedantic
spelling in stanc_options is rejected too: supplied rather than injected
it would be compared, so the first build warns and every later
construction matches the record and prints nothing.
The user header section claimed the path comparison could not skip a
needed rebuild and then described exactly that three lines later.
Narrowed to what it does: it detects re-rooting. The same over-claim was
in the "incomplete information" paragraph in compressed form, and the
make/local analogy inherited it, since hashing that file catches no edit
to a makefile it includes.
Sections 7 and 9 disagreed about whether an instantiate package can
register its source. Section 9 was right that it can, at its final
location with identical content, and that the recommendation therefore
needs a reason independent of content identity. The reason is that
registering source hands the rebuild decision to the session, and builder
comparison guarantees the next install_cmdstan() fires it inside a fit
call for an executable that still works.
Also: replayability now promises an equivalent build under the same
builder and environment rather than the same artifact; the included-file
message says sequence rather than set; and the --filename-in-msg version
caveat closes against cmdstan_min_version(), which is 2.35.
…y replay
Section 8 still said a warn-pedantic supplied through stanc_options
rebuilds and that the split behaviour was documented rather than fixed,
which section 4 had just replaced with a rejection. Section 8 now points
at the rule instead of restating a superseded one, and the rejection
covers every spelling: list("warn-pedantic"), the named TRUE and the
named FALSE. The FALSE case matters because it emits nothing today while
pedantic = TRUE still injects, so it looks like a way to switch pedantic
off and is not one. compile_impl() carries pedantic for the same reason
the rule exists: it is behaviourally significant on the path where
nothing is rebuilt.
Calling the effective fields a replay API could not survive its own
rules. make/local's STANCFLAGS reach the same stanc invocation and are
not in them, so they are not the actual invocation; and one option
cmdstanr injects is now a spelling the build API refuses, so feeding
them back would error. They are scoped instead to the options cmdstanr's
own build call passed, which keeps effective minus supplied equal to
exactly what cmdstanr injected, and make/local's contribution stays
covered by its content hash. Recording serves provenance and diagnosis
throughout, which is a weaker bar than reproduction and the one the
record can actually meet.
Section 4's illustration of why origin has to be stored used the
warn-pedantic pair, which this same change makes impossible. It now uses
--allow-undefined, injected by user_header and supplyable directly,
with --filename-in-msg as the other instance.
$cpp_options() returns cpp_options_effective, stated in section 1 and at
adoption in section 7. Supplied would have been defensible but changes
behaviour: a header passed through the dedicated user_header argument is
visible today as USER_HEADER and would stop being.
Section 1 also claimed the record stores both grains of everything. It
does not: include_paths keeps only the effective value, force_recompile
is deliberately absent, and the supplied option fields are canonicalized
rather than the caller's literal syntax.
The supplied/effective split only existed because stanc_options is a single list written into at R/model.R:673, :677, :693 and :835, which left the record reconstructing the caller's input after the fact. Section 10 answered that with a snapshot taken before the first of those lines, which holds exactly until someone adds a fifth injection site above it. Building the injections into their own list removes the problem instead of documenting it: _supplied and _injected are both values the code already holds, they are disjoint because cmdstanr injects only what the caller did not supply, and nothing has to be derived by subtraction. That also retires "effective", which was a misnomer. It named the list cmdstanr owns, not the stanc command line, since make/local's STANCFLAGS join a stage later as a character vector. Neither stored field now claims to be an invocation. The user header was reachable three ways: the dedicated argument, cpp_options[["USER_HEADER"]] and cpp_options[["user_header"]]. Only the argument survives. resolve_user_header() exists almost entirely to reconcile the three, tracking both casings for make's last-wins rule, walking a four-level precedence chain and raising two conflict warnings; section 8 removes its previous parameter with deferred compilation, and what remains is resolving a path and setting USER_HEADER for make. The codebase already treated the header as not belonging there, since parsed_cpp_options() skips it when canonicalizing. $user_header() is added so the dedicated argument has a dedicated accessor. Without it, $cpp_options()[["USER_HEADER"]] is the only way to read the header back, which was the sole reason to have $cpp_options() report anything other than what the caller supplied. It now reports cpp_options_supplied.
A function declared in Stan and not defined there has to be defined in C++, which is the user header, so --allow-undefined is the flag the header implies rather than an independent setting. stanc_options is no longer a way to ask for it, in either spelling. The flag suppresses exactly one error, verified on 2.39: a function declared and never defined. A call to something never declared at all is still not in scope and still fails. That makes it inert for anything that does not link, so the source-only operations set it unconditionally -- $format(), $check_syntax(), $variables() and their standalone counterparts -- while only the build entry points derive it from user_header. One rule by operation rather than by entry point, so a retained method and its standalone twin cannot disagree, which they would if the standalone functions were permissive and the methods kept consulting using_user_header_. This finishes eeed5ba rather than reverting it. Its conditionals become unconditional and the dependency on using_user_header_ leaves all three source-only methods, which is less code than the version on the branch. The accepted cost is recorded so it is not filed later as a bug: check_syntax_stan_file() reports success on a program that compile_stan_file() rejects, in the one case where a function is declared, never defined, and no header is supplied. The build is where that surfaces. Section 4's illustration of stored origin moves again, to --filename-in-msg and --name, since this rejection makes the allow-undefined pair impossible in the same way the warn-pedantic rejection did. Those two are what still justifies the split: neither has a dedicated argument, and a caller-supplied filename-in-msg deliberately wins over the injected one.
Section 4 justified storing supplied and injected options separately by pointing at flags that can arrive both ways. Two of those examples have since been removed by channel rejections, and a reader watching that happen could reasonably conclude the schema weakens each time. It does not. The verdict compares only what the caller supplied, and a merged list cannot be split back apart without knowing this version's injection rules, which is the reconstruct-after-the-fact fragility the rest of this design removes. That argument holds even if no option can arrive by both routes. --filename-in-msg and --name make the consequence visible and are now labelled as illustrations rather than the reason.
Section 3 accepted that equivalent spellings would cause spurious
rebuilds, on the grounds that canonicalizing would mean enumerating the
semantics of every stanc option. That is true of semantic equivalence and
not of spelling: list("allow-undefined") and list("allow-undefined" =
TRUE) both emit --allow-undefined, and stanc_options_to_args() already
computes exactly that vector.
Comparing the sorted emitted vector rather than the R list therefore
collapses both accepted spellings for nothing. Sorting is safe only
because --include-paths is now rejected from stanc_options; it is the one
order-sensitive flag that could have appeared there, so this simplifica-
tion is downstream of the channel rejection rather than independent of it.
The remaining concession is much narrower: two different flags that mean
the same thing still compare unequal, which is the safe direction.
--use-opencl is what cpp_options$stan_opencl implies, the same shape as --allow-undefined and user_header, so section 3 now states the rule once for both rather than naming one of them. Supplying it alone cannot work rather than merely being unusual: stanc emits matrix_cl members and to_matrix_cl calls, STAN_OPENCL is what makes those types exist, and the build fails with six template errors about a header the user never wrote. Rejecting it costs one entry in a validation list that already exists for the other rejections, and replaces those errors with a sentence naming the argument that owns the setting.
The section said supplying --use-opencl through stanc_options "cannot work" and always died on `no template named 'matrix_cl'`. Measured on 2.39, that only holds for models where stanc has something to move to the device. bernoulli.stan has no matrix and no GLM call, so it emits zero matrix_cl references, generates C++ identical but for the embedded stancflags string, builds cleanly, and reports STAN_OPENCL=false. That makes the rejection more clearly right rather than less. The loud failure was only ever the better half of the outcome; the quieter half hands back a working executable that is not OpenCL-enabled with nothing anywhere indicating the request was dropped.
The rejections were specified by listing the spellings they cover, which
does not terminate. warn-pedantic alone has six that
stanc_options_to_args() treats differently: the unnamed flag, named TRUE
and named FALSE that were listed, plus named NA and named "yes", and
named NULL which emits `--warn-pedantic=`. An implementation built from
the list would let three of those through, reopening the second channel
the rejection exists to close.
Two of the six emit nothing at all, so a validator keyed on the arguments
that reach stanc would pass them too. That matters most for the caller
who writes list("warn-pedantic" = FALSE) believing it disables pedantic
mode and gets silence instead of an error naming the argument.
State the rule once, in section 3, as an occurrence test with an arm for
each entry shape, and have the derived-flag paragraph, section 4, section
6 and section 8 point at it rather than restate it. make/local keeps its
own detection rule, since it is text in CmdStan's file rather than a list
entry.
Also replaces the canonicalization example, which demonstrated collapsing
two allow-undefined spellings that section 3 no longer accepts. O1 is
both valid and a better illustration: it changes codegen, so collapsing
the spellings prevents a real spurious rebuild.
Section 4 justified rejecting warn-pedantic from stanc_options partly on the grounds that a supplied diagnostic evaporates the same way an injected one does: build once, warn once, and never warn again once the record matches. Read as a general rule that lands on --warn-uninitialized and every other supplied diagnostic, and taken that way it asks for stanc to be rerun on every construction of an up-to-date model. The two cases are not the same. A compared option cannot evaporate in the way that matters, because turning it on mismatches the record and rebuilds; only an option that cannot force a rebuild needs a mechanism of its own. So the invariant is narrower than "a diagnostic always re-emits": asking for something you did not have before never yields nothing. warn-pedantic satisfies it through section 8's rerun, warn-uninitialized through being compared. That leaves a real asymmetry on an identical repeat call, where pedantic warns and a supplied diagnostic does not. Named rather than defended as ideal. Buying it would mean cmdstanr knowing which stanc flags are diagnostic-only per CmdStan version, which is the per-option semantics canonicalization already declines, and a misclassification is silent in the unsafe direction: treat a codegen flag as diagnostic and a needed rebuild is skipped. It is also what a C compiler does with -W flags and an up-to-date object file. The warn-pedantic rejection now rests on the one-channel rule instead, where pedantic is the case in which two channels differ in kind rather than in spelling, one injected and uncompared and one supplied and compared.
Named cpp_options are uppercased by cpp_options_to_compile_flags() (R/cpp_opts.R:129), so list(USER_HEADER = h), list(user_header = h) and list(User_Header = h) are one variable to make and three values to R. Rejecting the header by naming two spellings therefore left User_Header and every other casing as a working bypass of the channel the rejection exists to close. Folding case inside the matcher would close it, but the codebase already reconciles this three times in two directions — toupper() on the way out to make, tolower() in parsed_cpp_options() on the way into comparison, and tolower() again in the dormant validate_cpp_options() — so a matcher that folded case would be a fourth, and would have to specify which of the other three it runs before. An ordering constraint inside a contract is a sign the code shape is wrong rather than the rule. Normalizing on entry, ahead of validation, leaves one spelling in play for validation, comparison, the record and $cpp_options() alike. The rejection then matches literals. It also retires parsed_cpp_options()'s exclusion list, which is there because that function is fed a merged list: stan_version arrives from the binary's info output and user_header from cmdstanr's own injection, and comparison runs on _supplied where neither appears. Names assert_valid_cpp_options() as the one place that does normalize-then-check, pairing with assert_valid_stanc_options(), which already does the job for the other list. validate_cpp_options() is deleted rather than left dormant: its one substantive behaviour warns that a logical FALSE turns an option on, which #1251 reverses, so keeping it would document the opposite of v1.0's semantics. The stanc_options side is left alone. Names pass through unchanged and stanc is case-sensitive, so a miscased flag already fails at the build with a better message than anything we would write.
Section 4 framed the difference between pedantic and a supplied diagnostic as an asymmetry to be tolerated: pedantic warns on every construction because it cannot rebuild, warn-uninitialized warns only when a build happens, and warning in both cases would be better if it were affordable. That framing treated repeated warnings as a cost, and they are not. pedantic = TRUE is a request scoped to the call, like quiet. It runs whenever it is asked for, and the way to stop the warnings is to stop asking for them. A supplied stanc flag is part of the build configuration and applies when a build happens. The two behaving differently on an identical repeat is those two things being different, not a compromise between them. The record does make a third policy available, since it stores what cmdstanr injected: run the check only when the recorded injections show pedantic was not already applied. It is rejected here. It would make two identical calls behave differently on the strength of a file the caller cannot see, with no obvious way to ask for the output back, and it would defeat the main reason to put pedantic in a script, which is to have the check run on every execution. Also drops the claim that misclassifying a codegen flag as diagnostic would skip a needed rebuild. A classifier would decide what gets rerun, not what gets compared, and supplied options stay compared either way, so the real cost of a misclassification is an unnecessary rerun. Adds one sentence separating this from the untracked-provenance note in section 6, which fires on writing a record rather than on every construction. The two look contradictory and are not: output the caller asked for runs whenever they ask, output they did not ask for picks its moment.
The claim that neither excluded name could reach a supplied list was too strong. It holds for user_header, whose named spelling is rejected outright, but nothing rejects cpp_options = list(STAN_VERSION = "9.9"). STAN_VERSION is not a Make variable at all: cmdstanr synthesizes the name from the three stan_version_* fields the binary reports, and CmdStan's makefiles never read it. A caller who supplies it is setting an inert variable, so it is recorded and compared like any other, and excluding it would silently drop a supplied entry. That leaves exe_info_reflects_cpp_options() with no input, since §7 already makes build configuration an error beside an adopted executable, so it is deleted rather than re-keyed. Its removal has to be sequenced with canonicalization: it folds case on one side of an intersection and inherits the other from the parser, so canonicalizing while it still exists empties that intersection for every option and the check silently stops checking. The question it asked is put to reported_features by the runtime validators, at the moment the feature is used. Also names the $cpp_options() spelling change as a break, since the accessor currently reports the caller's spelling and the binary's both, and grounds canonicalizing on entry in giving every consumer one representation rather than in an ordering constraint a self-folding matcher would not actually have.
The introduction claimed the document wins any disagreement with an issue, while §9 claimed #1258 wins. Each was stating its own half unqualified, and they overlap: §9 carries commitments as well as ordering, so a reader had two rules and no way to tell which applied. The boundary is by subject rather than by document. Behaviour and contracts are this note's; staging and checklist detail are the issues', which makes #1258 the authority on the order of work. Neither owns code locations — the file:line references throughout both go stale on the next edit, and the source settles them.
A supplied STAN_VERSION was described as inert and as not being a Make variable. Both are false: cpp_options entries go on the make command line and CmdStan -includes make/local before anything else runs, so a user's own file can read $(STAN_VERSION) and change CXXFLAGS with it. The conclusion does not move but its footing does. It is recorded and compared because it can change the artifact, not because recording it is harmless. reported_features is now encoded by presence rather than with a third value. jsonlite writes NA as null and reads null back as NULL, which is also what a missing key yields, so an in-memory tri-state collapses to two states on the way through a file and a validator written as !isTRUE(x) treats unknown and disabled alike. Section 10 had asked for unknown to round-trip distinctly from absent; both states require identical behaviour, so that line now points at section 1 instead of contradicting it. Section 5 counted twenty-seven public methods while listing twenty-eight, having counted the current surface and listed the post-section-3 one. It now also says the completeness claim is enforceable through R6 introspection, since a static table decays at the next added method. Section 7 prices a format_version bump for an adopted executable, which cannot rebuild its way back to a supported version. Section 10 gains cmdstan_version_compare() as a third instance of reading absence as a definite value: it returns -1 for a missing version, so an unknown version compares as older than everything.
The document said an executable always yields a version and left it as an observation. It is not one: model_compile_info() synthesises ".." from three absent info fields, and that string passes every guard cmdstan_version_compare() has, so a model built from a binary that cannot report its version constructs silently and fails later inside a version gate. Section 7 now makes it a checked invariant at adoption, which is the only place a version arrives from an artifact nobody vouched for. The check is syntactic; rejecting a version for being old would defeat the section it lives in. The release order was stated four times in four slightly different ways, which is the inconsistency section 4 warns about arriving on schedule. It is now stated once, and the candidate ships when everything is ready rather than at the earliest defensible moment, so no item has to be adjudicated as safe before or after the tag. The stage 5 rationale changes with it: it goes last because its inputs do not exist until stage 4, not because the candidate period stabilises its schema, which it cannot do for a function that ships first. Section 4 also names the first case that obliges a format_version bump. --filename-in-msg lands a stage after records start being written, and injected options are recorded rather than compared, so without a bump an executable built earlier keeps matching its record and goes on naming a deleted tempfile. Two corrections. The jsonlite argument in sections 1 and 10 claimed the tri-state is destroyed by serialization; names() still recovers it. What is true is that the R type does not survive and the access anyone writes collapses the states, which is a better argument for encoding by presence than the one it replaces. And section 5 counted twenty-eight public methods, a number describing no moment that exists: it is the table's row count, the union of a method section 3 adds and one section 8 removes. The live surface is twenty-seven at both ends. Section 5 also now requires every member exercised rather than one per class, with guarded methods called bare so the matrix checks that validation runs first.
The third absence-of-evidence instance now has an issue behind it (#1260), and the note says why fixing it does not close anything here: the adoption invariant in section 7 is what stops a bad version reaching a model.
Two of the round-13 findings are the same defect. Section 4 fixed the
readable format set at {1} while section 9 said stage 4 bumps it, which
leaves the final reader and writer disagreeing whichever way it is
implemented: perpetual rebuilds, or no bump and the tempfile filename bug
surviving into 1.0. Section 9 then illustrated the downstream version
boundary with 0.9.0.9003, which from 0.9.0.9002 names stage 1 rather than
stage 4, so a brms guard written against it would switch to
compile_stan_file() three stages before that function exists. Both now
name the thing instead of the value: a release reads exactly the format it
writes, and a guard names the stage whose dev version it needs.
The release order moves to #1258 entirely. The document kept saying the
order lived here and nowhere else while also saying staging belongs to the
issue, and the issue printed the order anyway. Constraints stay here,
order lives there, and each links once.
Stage 5 went last for a reason that was false: its inputs exist at stage 3,
which writes the record and captures reported_features. It goes last
because it publishes answers stage 4 settles.
Air keeps its position and loses its justification. Branch conflicts end
when the NEWS reconciliation merges, which is before the tag, so that
argument never reached "after the candidate." What puts it there is that it
is optional, and an optional cosmetic change cannot gate a tag. Its one
real risk is a reflowed roxygen line regenerating .Rd, which R CMD check
would not catch, so re-run roxygen after it.
Corrections to specific claims. Both ".." and "garbage" error inside
compareVersion(); garbage warns first. The earlier probe used
tryCatch(warning=), which unwinds before the stop() runs. "Syntactically
valid" now names the grammar already at R/path.R:298, and says
compareVersion() cannot be the validator, since "2.36" and "2.36.0.1" pass
it silently. The two $format() refresh lines are not the same age: the
variables_ clear is #1235's and unreleased, the stan_code_ reassignment
shipped in 0.7.0, so deleting NEWS.md:94 would drop a released behaviour
change. And R/model.R:709 writes the resolved user header back into
cpp_options, which is what currently makes it a cpp_option at all.
Member
Author
|
@andrjohns One consequence of removing the |
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 by Claude and Codex (with plenty of em dashes and Claude-isms) 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 dependency file below). If everything matches we reuse the executable, otherwise we recompile.cmdstan_model(exe_file = )stays. You can still use a pre-built executable but if we don't know how it was built certain options will be unavailable.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 doWhen 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)Or when we can't trust what we recorded:
If more than one of these applies we report all of them.
One exception:
cmdstan_model(exe_file = )) can't recompile automatically since there's no Stan file to build fromThe new dependency file
The current plan is a file
.<model-name>.cmdstanr.jsonthat is written next to the executable. It contains:cpp_optionsandstanc_optionsthe user suppliedinclude pathsand user header pathstanc --infoTracking issue: #1258