Design note: v1.0 compilation state and C++ options - #1254
Draft
jgabry wants to merge 114 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.
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.
The trigger could never do the work it promised. Where the recorded installation is gone and a different one is selected, builder already differs and the ordinary comparison has fired, so the missing directory adds only a line to the message. Where the selection is the recorded installation, the trigger is the only one firing and the directory it would rebuild in is the one that vanished: set_cmdstan_path() checks existence once and caches the path, cmdstan_path() never rechecks, so the build runs make in a directory that is not there and reports "cannot start processx process 'make'", which reads as a missing toolchain. So a missing builder is reported and nothing else. The record is kept, the builder comes back with exists = FALSE, and the failure arrives where it was always going to, as a launch error naming the recorded installation with reinstalling or rebuilding as the remedies. That is better than a doomed compile in front of it. A selected installation that has gone is its own error, with or without a record, since both re-resolution and make live inside it. That also finishes the rule about invoking stanc from the recorded builder, which listed the gone installation as already being a trigger: it is not, and the one case it leaves is the selection being the recorded installation, where the model can be neither assessed nor rebuilt.
The missing-installation rule justified itself with both halves of the work being inside the selected installation, which is wrong: assessment re-resolves from the recorded builder, not from the selection. The rule holds anyway, because a build runs make in the selection and because the assessment's stanc is inside it in the one case where the selection is also the recorded builder. Refusing an older format decides the readable set, so it reports unsupported_format. Without saying so the new rule reads as though a record could be refused for a missing field, which is section 6's other diagnosis and a different thing. And the deferred include closure is where the rule first bites. Section 6 said only that the path comparison is what the closure replaces; records written before it cannot answer the comparison that replaces it, so they are refused once.
Two passages still stated the superseded rule that where an option came from decides whether it can force a rebuild. The field table already settles that per field, and stanc_name is the counterexample: it is injected and it is compared. Origin decides which of the two lists a value is recorded in, and nothing beyond that. Section 4's paragraph also read as a claim about the verdict as a whole, where it is a claim about stanc_options.
Deleting the additive and interpretive taxonomy took the writer's obligation out with it. What is left decides whether an older format is still readable, which only answers anything if a change to what a record means arrives with a new version in the first place. That obligation was stated in the tracker and only implied here, and this document is the canonical one. Separately, saying that rebuilding never follows from wanting a binary to be different contradicted force_recompile, which is exactly how a caller asks for that. Scoped to the automatic rebuild.
Saying a gone selection is an error with or without a record read as a condition on holding a model at all, which would refuse the packaged executable-only models section 7 exists to admit. Those neither build nor re-resolve: they hydrate from the record or from exe info and run against the recorded builder's TBB, so a selection none of that touches is nothing to them. What does need it is a model with source. Make runs in the selected installation, and when the selection is also the recorded builder the stanc that re-resolves dependencies is inside it too, so every route a source-backed call takes ends in one of those two. Checking first is what turns them into one message rather than a compiler failure or a doomed compile. It is a precondition on those calls rather than a third answer to the question the assessment asks. Also states the TBB fallback once. It covers a model with no record and a recorded builder whose directory is gone, and the two halves were written in different places.
The rule named building and assessing and then claimed those exhausted the source-backed routes. They do not. check_syntax, format and variables each run stanc with the selected installation as the working directory and conduct no assessment at all, so with the directory gone they still reach the raw processx failure the rule exists to replace. Stating it as a call-site rule is shorter than the enumeration was, and it does not go stale the next time something runs a program out of there. It also needs saying that this is not the staleness validation section 5 deliberately keeps away from those three methods, since the two checks would otherwise look like one rule contradicting another.
The fallback listed two cases, no record and a recorded directory that is gone, and neither covers a record that is present and unusable. The sharp one is a record that does not hash-bind to its executable: it names the installation that built some other binary, which can be in perfect health, and injecting that TBB is exactly the wrong answer with none of the symptoms of a missing directory. The four unusable forms are already section 6's, and they disqualify a builder here for the same reason they disqualify a baseline there. The bolded rule above it also promised more than the section delivers. Two paragraphs later a model built against a system TBB runs happily with the builder tree deleted, so the recorded installation cannot be something that must still exist. State the mechanism instead, which is what the paragraph was always actually about.
Six passages said an unusable record rebuilds, full stop, which is only true of a model that has a source to rebuild from. Section 7 has always said the other half: an executable-only model in the same state becomes unprovenanced and goes on running. One of the six even cited section 7 as its analogy while contradicting it. The reviewer found this for unsupported format versions. It is the whole artifact-side class, though, since a hash mismatch and an unreadable record leave an executable-only model in exactly the same place, so the split is stated once where the class is defined and the members point at it. That also improves the reported_features argument, whose cost is now the accurate one: requiring the four flags would strip provenance from executable-only models as well as rebuilding every source-backed one.
…twice The rule pointed at four runtime call sites and said all four take the TBB bare and all four need the builder's. Neither half survives reading them. Two of the four run a program out of the selected installation rather than the model - stansummary and diagnose, and the make that builds them - where bare is already the right answer and passing a model's builder would be the bug. Two sites that do launch the model were missing: the exe info call an adopted executable hydrates from, which is the executable-only case this rule exists for, and the help-all call behind cmdstan_defaults. Stage 4 would have implemented the list as written, so this was a wrong fix waiting to be made. Enumerating call sites is what got it wrong in both directions, so the rule now follows the binary and the four launch sites are stated as the exhaustive set they were checked to be.
The rejected-alternative note in #1250 said that rejecting the directly expressible assignments closes every accessible route, and that what remained was honestly opaque. It is not. Passing -f other.mk on a build call sets any variable this section gives one channel, without looking like an assignment: make -s show reports STAN_OPENCL and USER_HEADER unset, and the same command with -f other.mk added reports both set from that file. The --eval= spelling is the same route on Make 4.0 and newer, while 3.81 rejects the option outright, so which spellings reach a variable is a property of the user's toolchain. Rejecting the flags is not the reserved-variable guard that note considered and turned down. A guard has to know what each flag does, which is the maintenance it correctly refuses to take on, while rejecting the whole shape needs to know nothing and closes the routes a guard would miss. The cost is one flag. An include line covers -f, a plain assignment covers --eval=, MAKEFLAGS += -j4 covers -j and still parallelises when set inside an included makefile on 3.81, and force_recompile covers -B. Only -e has to be on the command line, and it hands the environment precedence over the flags CmdStan's own makefiles assign, so it dismantles a build rather than configuring one. Nothing in the package passes an unnamed non-assignment entry today. This also closes a hole in the untracked-dependency report, where a makefile named by -f changes the build, changes no recorded field, and is seen by neither of the two regexes.
The one-channel rule for include paths rejected --include-paths in make/local's STANCFLAGS, and the wording pointed an implementer at reading the file. A make/local that includes another makefile then bypasses it. Measured, the file reads one include line while make -s print-STANCFLAGS returns the include-paths flag that line pulled in. That is not only a recording gap. Only the include_paths argument reaches the stanc --info call re-resolution is built on, so a path arriving any other way resolves for the build and nowhere else, and the model compiles and then fails when anything asks for its variables. Asking Make costs nothing, since get_cmdstan_flags already does it on every compile. Two consequences are stated with it. The message names make/local as where the chain starts rather than as where the flag sits, since the flag may be in a file make/local pulled in. And the other scan of make/local, the untracked-dependency detector, must keep reading raw text: run against the effective value it would find nothing, because by then the include has happened.
The selection rule computed the runtime TBB directory from the recorded builder plus the default layout. That is right only for a build that left TBB_BIN alone and named no TBB_LIB. Stan Math bakes an absolute path in either way, both variables are overridable, and cmdstanr recognises tbb_lib and tbb_inc as cpp_options today. Neither branch emits an rpath on Windows, which is the whole reason cmdstanr puts a directory on PATH there, so a user who built against their own TBB would get the bundled directory prepended ahead of the one their binary is linked against. The rule written to load the right TBB is what loads the wrong one. Recording is also the only answer that stays true, since the value depends on make/local as it stood during that build and a later derivation reads it as it stands now. The directory becomes a record field, recorded and not compared: everything that moves it is compared already, through the supplied cpp_options or through make/local's hash. The claim that the binary links TBB at a path inside the installation appeared in four further places. Each keeps its conclusion once corrected and now points at the owning rule rather than restating it. Two notes travel with this. The info call that hydrates an adopted executable runs only when there is no usable record, so it takes the fallback by construction rather than performing the same lookup as the sampling sites. And the make call that reads the directory has to carry the build's own flags, which get_cmdstan_flags does not, or it records the default for exactly the configuration the rule exists to handle.
The single-configuration cache promised that a model object records the artifact identity it was constructed against and that the assessment detects replacement. Nothing downstream took delivery: the engine was specified as taking a record and a request, both read fresh at call time. Two objects built from one Stan file share an executable path and a record path, and the most recent compile owns both. Once a second build has run, the pair on disk is self-consistent and describes a program the first object never saw. Its path is unchanged, the file exists, and the record's own artifact hash matches the binary it now sits beside, since that is the bond it exists to prove. Nothing on disk disagrees, so the first object goes on sampling a program its own eager snapshot does not describe. The assessment now takes what this caller expects and what is on disk, with the expected side coming from the call at the constructor and from the object's own snapshot at a guarded method. The response is unchanged: the constructor rebuilds, anything that runs the binary errors, and the remedy is to reconstruct the model, which usually adopts without compiling. Resolved dependency hashes arrive as part of the observed side, which is what keeps the engine pure. The include-paths row now says whose paths at which caller, closing the door the round-five defect came through.
…doption wording The selected-installation check listed the variables accessor as a call-time stanc site. That describes today's code, which stage 4 removes: the snapshot is captured at construction, so the accessor answers from what it already holds and needs no installation at all. Its standalone twin does run stanc on the spot and stays on the list. An implementation that left the parse-on-first-call in place would put the method back, and the list is the wrong place to discover that the snapshot was never made eager. The executable-only exemption said that a missing selected installation surfaces as a launch failure rather than as a refusal to construct. With no usable record, adoption runs the info call during construction, and an executable that does not identify itself is refused there. It is not a precheck that refuses; the fallback runs as it always does and a failure is reported as the adoption error. Also corrects a stale line reference for the parse-on-first-call, and one phrase this branch made stale two commits ago, where an executable-only model was still said to run against its recorded builder's TBB.
The record's name was given as .<model>.cmdstanr.json, illustrated with a Stan file and an executable whose names coincide. That is the one case where the choice does not matter, so the example settled nothing. The two names do come apart. The model name substitutes underscores for spaces while the executable path keeps them, so my model.stan compiles to an executable called my model and reports a model name of my_model. A directory holding both my model.stan and my_model.stan then produces two executables and, under a model-name scheme, one record, which the second build overwrites. The first executable is left reading a record that describes the other one. The hash bond catches the mismatch rather than running the wrong binary, but the pairing this section exists to guarantee is broken by the naming scheme itself. Deriving the record path from the executable path removes the collision by construction. It also matches what adoption can actually do: an adopting caller supplies an executable path and nothing else, so a name computed from anything else has to be reconstructed through a model name that is itself derived one way from the Stan file and another way from the executable. The platform extension stays in the name. Stripping it would put a derivation back into a rule that otherwise has none, and the gitignore pattern the design already asks for matches either spelling.
The document specified source-backed construction and executable-only adoption and never said what the pair means, while the constructor accepts it and the proposed internal build signature has a directory argument but no destination. The two build entry points are supposed to share one implementation and differ only in the dry-run argument, and they do not: the constructor can name its output and the standalone compile function cannot. Measured, the pair is not adoption at all. The executable argument names the build destination, filename included, with dir overriding the directory and the basename surviving. A stale binary at that path is rebuilt over rather than adopted, so the combination was never a way to use an executable as it stands, only a way to choose its name. Everything else it served is already available through dir, which places the binary in any directory including one the source does not live in, keeps the model source-backed, and lets two configurations of one program coexist. The filename is the caller's either way, from the Stan file's name or from the basename argument of the file writer for generated code. What goes is naming two builds inside a single directory, which a subdirectory covers. Rejecting it also retires a Windows asymmetry rather than requiring a fix for it. The destination resolver skips the platform extension on a supplied path while adoption asserts it, so an executable cmdstanr itself built at a custom path could not be read back.
The silence rule argued that passing an executable argument is itself a statement that cmdstanr did not build the thing, so saying its provenance is unknown only repeats what the caller said. Section 7 refutes that twice over. Compiling with the standalone function and then adopting the result is called a first-class flow fifteen lines earlier, and cmdstanr built that executable and wrote its record. Pre-record executables, named in the section's opening, reach the unprovenanced branch precisely because an older cmdstanr built them before records existed. The argument does not depend on who compiled it. What the caller stated by supplying a finished binary rather than a source is that they were not asking for a build, and silence follows from that whoever produced the file. Restating it that way keeps the surrounding principle intact and stops it resting on a claim the same section spends a table refuting.
A field can be added to a published result later and cannot be removed or reshaped once it ships, and nothing has shipped yet, so this is the last point at which the set costs nothing to choose. The artifact hash, every dependency hash, the injected stanc options and the effective stanc name leave the public result and stay in the record, which goes on comparing the ones it compared before. The reasons differ by field. The artifact hash answers a question the caller can already answer by hashing the file whose path they just passed in, with an algorithm that has a name. The dependency hashes compare only against another record's same field, and the rule bounding that comparison is stated in terms of records at the same format version, so narrowing the version while keeping the hashes would leave the caller unable to check the precondition we impose on them. The last two say how cmdstanr assembled the stanc command line rather than what was asked of it. The format version survives under one reason alone, because the printer has no other source for its direction message and the print method receives nothing but the result. That is a named consumer, which is what the withheld fields lack. Two rules go with the fields. One of them existed only to defuse a trap the public field created, since the effective name carries a suffix that the model name accessor does not, so the field, the trap and eleven lines explaining it leave together. The new rule also states a line already drawn by accident: the TBB directory added earlier this round is recorded and deliberately not surfaced, and until now the only thing saying so was its absence from one table. Three passages elsewhere justified themselves on hashes the result no longer carries, and the worked sketch showed a format version on an available result, which the new rule forbids. Those are repaired here rather than separately, since each is only wrong because of this change.
A review asked for the rule that an injection change never rebuilds existing artifacts to be softened, so that a consequential injection could invalidate older records. Keeping it absolute, for two reasons. The mechanism on offer does not fit the case it is meant for. A compared field of its own works where the injection determines a per-model value, but for a constant injection like --filename-in-msg that field would hold the same value in every record carrying it, which is a format marker wearing a field's name. That leaves a format_version bump, and section 7 already prices one: a source-backed model rebuilds once and is current again, while an executable-only model cannot rebuild at all and drops to unprovenanced. The person handed a binary without its source is the one who can do nothing about the notice. The rule also follows from what the artifact is rather than from preference. CmdStan is part of the executable, through the absolute rpath into its tree and the stanc that produced the C++, which is why a CmdStan upgrade does rebuild through builder. cmdstanr appears nowhere in it, so a release able to invalidate artifacts would make the cmdstanr version build state, recorded through format_version instead of through a field. A stanc code generation bug is CmdStan's, and its fix arrives with a version that already rebuilds. What the review did change is that leaving working models alone now carries an obligation. Section 9 promised a NEWS entry for --filename-in-msg on its own account; that promise becomes a rule in section 4 covering every injection change, and section 9 points at it.
A hash-matched record establishes that the executable is the one it describes, not that this machine can run it, and the review asked whether losing the execute bit should therefore rebuild. It should not. The bit is one member of a family: a hash-matching binary also refuses to launch when it was built for another platform, when the volume forbids execution, and when its recorded TBB directory is gone. Section 6 already answers two of those by reporting rather than rebuilding, and checking the one member a stat can see leaves the rest to fail at the first fit, so the launch error has to be good either way. The state is ordinary rather than exotic. R's own unzip extracts an executable without the execute bit where untar and file.copy keep it, so a project folder shared as a zip and unpacked from R arrives with a valid record and a binary that will not start. Measured, what the fit then gets is a raw processx failure naming a relative path. So the rule goes with the other things force_recompile exists for, and points at 1246 for the message. That issue's own fix is scoped to the adoption helper, which this case never reaches, so it is widened here to every site that launches the model binary.
The review argued that sorting the emitted argument vector before comparing it is unsafe, on the grounds that stanc --O1 --O0 and stanc --O0 --O1 generate different code. Measured on 2.39.0 they do not. Both are byte-identical to plain --O0 except for the flags string stanc embeds, and they differ from each other in that line alone. Pure --O1 is genuinely different code, so the comparison has something real to catch; order is not it. The bullet rested its case on include-paths being the one order-sensitive flag and being rejected elsewhere, which is true but reads as the whole reason. It now gives the rest. The one valued option stanc accumulates on repetition is include-paths, already rejected; every other one it refuses outright, and a duplicated bare flag it drops with a warning. Where two flags settle one setting between them, position does not decide it. The cost is worth writing down rather than leaving for the next reader to rediscover. A caller who reorders the same flags gets no rebuild, so the binary keeps the flags string the first build stamped and its CSVs go on reporting that order. Section 4 already establishes that the string reaches each sampler CSV unparsed, in the recorded-value rule, so this points at it rather than saying it twice. Rebuilding to correct that line would produce identical code at the six to fourteen seconds already measured here. The durability worry is the serious half of the objection and it resolves: a later stanc that made order matter would arrive in a CmdStan release, and a CmdStan upgrade already rebuilds through builder. The sort can only be wrong inside one stanc version.
Section 9 opens by saying it is only the reasoning behind the order and that 1258 is the work list. The release candidate subsection was the standing exception: a package-by-package survey of brms, instantiate and rethinking, the gitignore template, the formatting and linting schedule, and a NEWS reconciliation pass, all of which an issue checklist carries perfectly well. 145 lines out. Two of those were duplicated rather than merely misplaced. The NEWS subsection said less than the inventory already in 1258 and measured less. The instantiate reasoning was in 1258's downstream bullet already, in places sentence for sentence. Half the block stays because section 7 delegates to it. Its rule that a package building at install time belongs in executable-only mode by design ends by saying section 9 carries the argument, and the adoption cost, the silence rule and stage 5's existence flag all point the same way. That half now has a heading naming what it is instead of sitting under the release candidate. Both sentences the tracker quotes are inside it, so no citation moved. Also drops a below that pointed into deleted text, updates the section 9 row in the map, and stops claiming a fixed number of review rounds.
Section 8 removes the second build call, so what it fixes after construction is which executable the object describes. It does not stop the object changing: format, save_hpp_file and expose_functions all mutate other state. The clone paragraph was the worst place to get this wrong, because it is about a clone sharing a mutable environment and exposing functions into it, which is one of the mutations the claim denied. The snapshot argument had the same claim in shorter form.
Section 7 rejects six arguments supplied beside exe_file, testing whether each was supplied rather than what it resolves to, and then recommended a NULL sentinel without saying which of the six it fits. The review read that as general advice, which is a fair reading, and the answer is that it is general. user_header looked like the exception, because user_header = NULL means compile without one today. That meaning exists only because the header persisted: resolve_user_header's supplied flag gives the argument precedence over the two cpp_options spellings and otherwise falls back to previous. Section 3 rejects both spellings and section 8 removes previous, so nothing is left for an explicit NULL to override, and omitting the argument already means no header. The capability is untouched, only the distinction goes. The constructor never passes previous, so an explicit NULL and an omitted argument already reach the same result there unless a cpp_options spelling is supplied too, and the clearing semantics belong to compile, which section 8 removes. The flag leaves resolve_user_header with the precedence chain it existed for.
The section was called Removing deferred compilation while owning the whole replacement API: the four standalone functions, the shared build implementation, and the build-info result with its schema and print method. That is the document's largest public contract, and the title named only the thing it removes. The map row gained the build-info result to match. Two lines were over the limit, one of them at 145 bytes in the paragraph the release-candidate move left behind. Worth recording what the first one turned up: the injection rule above it is 87 bytes, which passes the 88-byte gate but not the wrapper's 86, so wrapping the paragraph split a sentence two issues quote. A cited rule has to be reflowed around, not through.
The section already argued that a source-backed model on the reuse path cannot expose its functions and gets a message about a pre-compiled one. That was traced through the guard and its three assignments rather than run. Running it confirms the argument and shows the defect is worse than the issue that reported it says: cmdstan_model twice on the same file, then expose_functions, and the message comes back with the source sitting beside the model. No compile = FALSE and no dry_run, which is the corner 1245 documents. The rule above it was shortened so 1258 can quote it. At its old length it wrapped onto a second line, which the citation check does not resolve, and the same trap had already split a rule earlier in this pass.
Two claims about the world rather than about the design, both wrong when checked. The milestone paragraph said every issue in this area carries the 1.0 milestone. 1260 and 1025 do not, verified against GitHub. Fixing the sentence would leave a statement that goes stale whenever anyone milestones an issue, nothing cites it, and the tracker is where that belongs, so it goes rather than getting a qualifier. The reviewer asked for the same thing. Calling 1025 the project's existing answer to concurrency overstated it twice. It is open rather than settled, and its title is about giving unit tests independent workspaces, so it is scoped to the test suite and not to user models. The paragraph now says what it is and keeps the point it was making, that a lock and that proposal are two concurrency strategies worth deciding together.
Section 6 claimed that every directory participating in C++ include resolution is compared as a spelling, and named two members: the -I flags in cpp_options and the user header's own directory. CPATH and CPLUS_INCLUDE_PATH are a third, and no part of the document mentioned them, so the rule is narrowed to directories supplied to cmdstanr and the variables get a bullet among the things no record can fix. Measured, though the document does not need to carry it: through cmdstanr, a user header whose include is satisfied only from a CPATH directory builds with the variable set and fails without it, with nothing in the call or in any recorded field to tell the two apart. Reading the variable would establish that it is set, never that this build used it, so detecting it would attach a note to every model on a machine where someone set it once. Issue 1257 already listed both variables as untracked, so nothing there changes.
Section 5 noted that the setter's only call site is in a test built on compile = FALSE, and that it retires with that. Read as the call site retiring it was right, but read as the whole test retiring it drops coverage of a guard the design keeps: a build has to refuse a directory destination, and dir still resolves onto one whenever file.path of dir and basename names a directory. Nothing else covers that. So the sentence now separates the two, and issue 1258 carries the same correction on its setter-removal item, where whoever does the work will be reading.
Adding CPATH to the things no record can fix left section 6 saying, two subsections earlier, that two dependencies cannot be tracked in v1. That count was only ever about the two the regexes detect, so the sentence now says so instead of counting the untracked set. The tbb_path reference gave 1238-1248 where R/run.R ends at 1247 and the function ends with it. The other reference to the same function, in section 9, already had it right.
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 (with plenty of em dashes and Claude-isms). 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. 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, 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.json,that is written next to the executable. It contains:cpp_optionsandstanc_optionsthe user suppliedinclude pathsand user header pathstanc --infoTracking issue: #1258