Design note: v1.0 compilation state and C++ options - #1254
Draft
jgabry wants to merge 138 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 build record) and it replaces the previous half-done C++ options work that never got finished. You can ignore all the issues that have been opened lately, they're just based off of this list and I'll close them as I go through the implementation. I'm hoping to start working on this ASAP. |
Section 9 gave the ordering but said nothing about execution. Adds the release candidate as a third constraint on the order: stages 0-4 must all be in it, because the API removal is the breaking change downstream packages need to see, while stage 5 only adds a function and can follow. The candidate period is also the real use stage 5 was already waiting on. One constraint falls out of that. The repo-wide formatting and linting work (#1153, #1172) has to land before stage 1 or after 1.0, never between stage 4 and the candidate, where a reformatting diff on top of the API removal would hide what actually broke. Adds a note on how the stages are run: one pull request each, stage 4 built as a tested pure engine before the wiring and the API removal, and only one compiling task at a time, since make/local and the precompiled headers live in the CmdStan installation rather than the checkout and separate checkouts do not separate them. Drops the joint cmdstanpy naming process. Where cmdstanpy already has a name we copy it, and otherwise we pick one and they can copy it; nothing here needs to wait on that. Also brings the issue-consolidation note up to date. It still described the work as pending and pointed at a section that has since been removed.
JSON, named <model>.cmdstanr.json beside the executable. jsonlite is already an import, so the format costs nothing; the name stays clear of .dep and .d, which make and the C++ toolchain already claim in that directory. Stage 2 no longer has to settle this, but portability and the git-ignore story are still open and still have to be answered before anything writes a file. Choosing JSON adds a third way to get the tri-state fields wrong, so the note in section 10 now says that unknown has to round-trip as distinct from both absent and false, and that this is a property to test rather than assume.
Section 6 treats an executable predating records as a rebuild trigger. Since 0.9 stays installable from GitHub, that transition could be tested in CI rather than waited for in the wild. Recorded as a possibility for stage 4 to weigh, not as a commitment; building such an executable by hand when it is needed may well be enough. Also corrects stage 4's issue references. It still said the stage closes #1252, which the consolidation already closed, and pointed at #1019 rather than the #1255 and #1256 that were opened to carry this work.
Air's one-time whole-repo format goes last, immediately before 1.0. It is whitespace-only and deterministic, so shipping it after the candidate is cheap, and by then nothing is left for it to conflict with. Its pull request review action is a separate matter and is better landed early, while stages 2 to 4 are writing the code it would otherwise reformat afterwards. Jarl is not the same kind of change. Adopting it is additive, but acting on its findings is semantic editing, and that cannot follow the candidate without 1.0 shipping code in a form nobody tested. Those are ordinary reviewed changes. The previous note offered "before stage 1" as an option. That was never really available, with #1235 and #1254 both open.
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.
Both reviewers found the same hole from different directions. GNU Make imports ordinary environment variables without -e, so a variable a build call leaves unset can be set in the environment, or in a makefile that make/local includes, and reach the build with nothing recorded moving. Measured against 2.39.0, six arrive that way: USER_HEADER, STANCFLAGS, STAN_THREADS, STAN_OPENCL, TBB_BIN and TBB_LIB. A command-line assignment beats the environment, so the exposure is only what cmdstanr does not itself supply. Two of those can change the artifact with nothing to show for it. A USER_HEADER left in a shell profile compiles a header into the binary that appears in no dependencies entry, so editing that header afterwards leaves every compared field identical. STANCFLAGS from an included makefile changes generated code while make/local's own hash never moves, which is the arrangement make/local.example:36 recommends. The header is refused rather than recorded, since recording it would reopen the second channel section 3 exists to close. That check is one branch rather than a comparison, because a supplied user_header already wins on command-line precedence, and its query must not carry the build's own flags or it reads back cmdstanr's own assignment. Section 3's enumeration is scoped to cmdstanr's own arguments now that something outside them can reach the same variable, and section 10 says which of the three queries built on that call wants the build's flags, since only tbb_dir does. The flags become a compared field holding the value get_cmdstan_flags already computes on every build. That narrows the untracked make/local-include category instead of leaving it blanket, and the claim that make/local's contribution is covered by its own hash now lives only in the table. What stays untracked is named: STAN_CPP_OPTIMS, INTEGRATED_OPENCL and TBB_INTERFACE_NEW set compiler and preprocessor flags rather than changing what code exists.
The fallback substituted the selected installation's TBB whenever the recorded directory had disappeared, which is the derivation the next rule rejects, reached by another route. For a default-layout build the recorded directory lives inside the builder tree, so a gone directory and a gone builder are one event, and substituting there recreates the case the rule was written to stop: a 2.39 binary running on 2.40's TBB. For a build that named its own TBB the substitute either lacks the library the binary imports or supplies a different build of it under the same name. Since with_path prefixes, it would also outrank a working TBB the user already has on PATH. Three passages already described the new behaviour and contradicted the old rule, so this makes the section agree with itself rather than trading one wording for another. The branches are now a table, since leaving the fallback unnamed is what let one of those passages call this the second case when it is the first. Also: tbb_path cannot be the helper, since its argument means an installation root and it appends the library subpath, and three callers in install.R depend on that meaning. The four launch sites get their own helper and nine keep tbb_path unchanged. Records the directory absolute, because the makefile uses TBB_LIB literally, and names the environment and a direct LDFLAGS_TBB override as untracked rather than claiming every route to it is compared. Corrects a stale cross reference while here: the four forms of an unusable record are section 7's, not section 6's.
The sort was defended on the grounds that nothing in stanc_options is
order-sensitive, which is a claim about how every pair of stanc flags
interacts and holds only because 2.39 was measured. The collapse the rule
advertised as the sort's benefit, list("O1") against list(O1 = TRUE),
comes from stanc_options_to_args and happens with or without sorting, so
the sort bought only immunity to reordering.
The defense of that residual benefit does not hold. A stanc that made
order significant would arrive in a CmdStan release, and the release
rebuilds the first artifact, but two later requests under that same
version still compare equal once sorted, so the wrong artifact is reused
silently.
Dropping it settles three things the document already said. The bullet
declined per-option semantics for semantic equivalence in its closing
sentence while the sort depended on them. The note on the general
diagnostic classifier forward-references this bullet as where per-option
semantics are declined. And the claim that the canonical form of an
option is what the compiler receives becomes true.
The section recommended a NULL sentinel and then said both public build functions declare the option as their default. Only the first can be built. Once a signature default has been evaluated an option-supplied TRUE and an argument-supplied TRUE are the same value, so the rule that an adopted executable rejects supplied build configuration has nothing left to read, and either every adoption by someone with the option set errors or none does. The rule is now that no signature resolves it. The shared implementation resolves it after the check, and every public function that forwards the argument declares NULL. That is three functions rather than the two the review named, because cmdstanr_example resolves the option in its own signature and hands the answer on, which would leave its rebuild reason naming an argument the caller never passed. Also corrects where missing fails. It survives dynamic dispatch and dots forwarding, so it works at the constructor; it breaks one layer down, where any wrapper default including NULL makes it FALSE, and that is exactly where the rebuild reason still needs to know the value's origin.
Rejecting unnamed entries was said to leave only plain assignment reachable, which is what makes the canonicalization rule sound. It did not. A named entry keeps its name through toupper, which leaves a trailing plus alone, so a list whose name ends in one reaches make as an append rather than an assignment. The user header shows this is not only about operators. Its name with a suffix is not equal to the reserved name, so the matcher that rejects the two cpp_options spellings passes it, and make then sets the header from it. The one channel that rule gives the user header had a second one behind it. The grammar is the one the parser already applies, where it decides only how a flag is classified and nothing consults it before the flag is handed to make. The obligation to name the owning argument in the message now covers rejections on shape generally, so it reaches this one as well as the unnamed case.
The assessment is handed the sources already resolved, and separately it skips re-resolution when the selected installation differs from the recorded builder, because that difference is a rebuild trigger by itself. Those two rules meet at a third one telling the engine to report every applicable trigger. Nothing said which of them wins. An implementer following it literally compares a recorded include list against an empty one and reports an included file as changed when nobody looked at it. Writing the branch that avoids this is a decision the design never made, so two implementations would make it differently. The observed side now says whether the dependencies were resolved, and the rule asks for every evaluable trigger. This is the shape the rule beside it already uses for a missing record: a precondition rather than an exception. Only one path reaches it, and since that path is itself a trigger the verdict never changes, only the list of reasons. No status enum, and no state for a resolution that fails. Nothing here has ever described a failing stanc info call during assessment, and a status for it would oblige three more answers nothing currently needs.
An executable-only model whose record cannot be used was said to go on running. It only does so if the binary reports a supported version; it takes the adoption path, whose second and third rows are the two outcomes, and the third refuses. The public result was said to keep the include paths because a rebuild turns on them. It does not, and the recorded value is not even what re-resolution runs with, so the field is kept for diagnosis alone. The sentence after it was leaning on the half that was wrong and moves too. Supplying a built executable was said to tell us its provenance is unknown. That is false for a record-backed adoption and for a build followed by adoption, both first-class here, so the paragraph goes; the argument above it carries the point on its own. The promise that changing what we inject rebuilds nothing is now stated with its scope. What keeps the class closed is that a correctness fix to generated code belongs to stanc, and a CmdStan upgrade already rebuilds. An injection outside that is outside the rule, not an exception to it, because a NEWS entry cannot make incorrect reuse safe. The two instantiate references also become full links. Bare numbers resolve against this repository when rendered.
The classification table excused the builder from its member counts by naming one function, but the table lists both build entry points now, so the sentence covers neither reliably. It names them as a pair instead. The recorded TBB directory claimed to be the only field recorded for neither provenance nor comparison. The format version and the untracked dependency list are both recorded for reasons that are neither, so the claim is dropped and the row says what it is recorded for. This is the same shape as a uniqueness claim corrected in the tracker this round.
The completeness test compares the live public surface against the classification table, and the live surface is twenty-seven methods and one field, measured on the class itself. The sentence named twenty-seven rows, which counts the methods and drops the field, so the test it describes would fail on the one member it was written to cover. Saying that both link branches bake an absolute rpath contradicts the paragraph added directly above it earlier this round, which establishes that the makefile uses TBB_LIB literally and that a relative one comes back unchanged. That paragraph owns the question, so the sentence below it no longer answers it a second way, and the measurement that follows still gives the default layout's absolute result.
The record stored and compared the STANCFLAGS value Make resolves from the environment, make/local and whatever make/local includes. That made it the one Make variable from outside cmdstanr that was tracked, while the untracked-dependency rule says the rest are not, so the design had an exception to its own rule and the reasons for it had to be argued in four places. Ambient Make variables and anything an included makefile sets are now uniformly untracked, and force_recompile is the remedy for all of them. A change to make/local itself is still a rebuild trigger through its hash. The one thing kept is the build-time check that --include-paths does not appear in the effective STANCFLAGS, since that guards the rule that include_paths is the only channel for include resolution; it reads Make's answer and records nothing. The stanc options are stored two ways rather than three, the field leaves the table and the public result, and the environment bullet under what no record can fix now says plainly that nothing there is compared instead of claiming reported_features handles it.
With no user_header supplied, a USER_HEADER set in the environment reaches the build and compiles a header no dependencies entry names. The design asked Make for the effective value and refused a non-empty one, which made it the second Make variable from outside cmdstanr to be treated specially. The query it relied on was also weaker than it looked: a make/local that wraps the assignment in ifdef answers differently to a flag-free query than to the build's own invocation, so the refusal could be argued past without anyone intending to. Ambient Make variables are untracked, uniformly. The environment bullet under what no record can fix states the USER_HEADER case as the limitation it is, and force_recompile is the remedy alongside the rest.
Which TBB directory goes on PATH when the model binary is launched is a launch-side rule. It reads the record, but no verdict turns on it, and it can land any time after Stage 3 has written tbb_dir. Carrying the three-row table, the four launch sites, the second helper and the gone-directory argument inside the rebuild section made that section the longest in the document for a rule the assessment never consults. What stays is what the record needs: tbb_dir is recorded, absolute, from a query that carries the build's own flags, because Windows needs it at launch and only the build can determine it, and it is not compared. The rpath mechanism stays too, since builder identity and the missing-builder rule rest on it. The rest is drafted as an issue body to be filed separately; the document names it by title until it has a number.
1.0 reads exactly the format it writes, and any other version is an artifact-side reason: rebuild with a source, unprovenanced without one, reported as unsupported_format and never refused. That was already the rule; it sat under five paragraphs telling future releases when a bump is owed, how hashing and canonicalization changes interact with one, and why refusing to replace a forward-version record was rejected. None of that binds 1.0, and a release that widens the readable set will write its own rule when it does.
The stanc_name row stays: the build bakes --name into the binary, no other compared field pins it down, and a coordinated rename would otherwise reuse a binary that stamps the old name into every CSV. The row was carried by six paragraphs arguing the CSV boundary, the mangling measurements and the limits, which is more defence than a row that meets the table's own criterion needs. One paragraph now states the reason, the raw-versus-mangled rule, and the executable-only limit.
4 tasks
The rule moved out in the previous commit is filed as #1261, so the three places that named it by title point at the number.
Nothing said what happens when the stanc info call fails while the sources are being re-resolved for an assessment. A reviewer read the gap as needing a third engine state. It does not: resolving is the caller's job and the engine never sees a failure, so the caller raises it. At construction and at every guarded method the error carries stanc's message, nothing runs and nothing rebuilds, since the build's own stanc call would fail at the same point. #1237's request that an unresolvable include fail toward rebuilding is reworded there to match.
Read literally, the guarded-method precondition asked every model for a valid record before running, and an executable adopted on its info output alone never has one, so every instantiate fit would have errored. The check for an executable-only object is now stated once: compare the executable's hash to the one the object was constructed with, and nothing else, whether or not a record was present at adoption. A replaced binary errors, an unchanged one runs, and a record that goes away after construction changes nothing.
Passages that argued against an earlier draft, a reviewer's proposal or a full review round explained how the document got here rather than what it says, and the rejected alternatives were argued at the length needed to win them the first time. Both go. Each rejected idea is now one line in an appendix, the idea and why it lost, and the section that owns the replacing rule keeps only the reason the rule needs. The longest of them, path-and-content identity for sources, keeps its worked example since it is the closest call in the document.
The table in §4 is the single statement of what is recorded and what is compared, and the paragraph above it says prose elsewhere must not restate a row. Seven passages did, each restating a row's answer and then citing the table for it. They now point and stop, and the two that were nothing but a restatement are gone.
A pass over every section for the register the document was written in: em dashes, bold-led bullets, negative listings, "deliberately" and "genuinely" doing no work, rhetorical setups, and abstraction nouns standing in for a verb. Rules, tables, measurements, code and file references are unchanged; the two remaining "earlier draft" remarks in §10 are gone with them. Stage headings use a colon instead of a dash.
A hunk-by-hunk read of the rewrite against its parent found three places where a sentence lost a claim rather than a mannerism. The validator paragraph in §1 pointed at the consumer table and now does again. The stanc_name row said the CSV header carries both the raw name and the mangled one, which is the observable the row rests on. And §7 stated outright that executable-only models are kept, which the adoption paragraph had reduced to an implication.
…d version once each Three figures the doc measured or quoted in one place were repeated as literals in two or three others. The secondary mentions now point at the owning passage, so a remeasurement or a moved line in CmdStan's example file changes one place.
Two cited a neighbouring line (cpp_options_to_compile_flags is defined at R/cpp_opts.R:131, the space-to-underscore substitution is R/model.R:273), one cited the roxygen line before the one naming $expose_functions(), and three bare :NNN references sat closer to a different file than the one they meant, so they now name it. The §4 include-path aside said $sample() calls $variables() unconditionally; R/model.R:1410 guards that call on a registered Stan file, so it now says so.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
dev-notes/compilation-state.md. No code changes.This is the current plan for v1.0, developed based on conversation with @SteveBronder and @WardBrian.
AI disclosure: the compilation-state.md file is being written iteratively via a back and forth with Claude and Codex. It is based on the list below, which is my own summary of the planned changes. This is somewhat of an experiment, and if it goes poorly I may end up writing the document myself from scratch.
New API
cmdstan_model()checks if the existing executable matches the requested Stan file (and includes and user headers) and options (see section on new build record below). If everything matches we reuse the executable, otherwise we recompile.cmdstan_model(exe_file = )stays. A pre-built executable still works, but$code(),$variables(),$check_syntax()and$format()need a Stan file, and passing build options (cpp_options,stanc_options,include_paths,user_header,force_recompile,pedantic) with only an executable is an error, since there's nothing to rebuild.cmdstan_model(stan_file = , exe_file = )together is now an error. That usage ofexe_filewas just used to indicate where to put it, anddirdoes that anyway (albeit without filename customization).compile = FALSEargument tocmdstan_modeland the$compile()method). This means we losecompilearguments likecompile_model_methodsandcompile_standalone. But$expose_functions()and$init_model_methods()already do the same job.format_stan_file()check_syntax_stan_file()stan_variables()compile_stan_file()stan_build_info()for inspecting how an executable was built$code()and$variables()refer to the Stan file used to build the executable, even if the Stan file has changed since (needs to be recompiled)cmdstan_model()again, notforce_recompile = TRUEsincecmdstan_model()will now know what to docpp_optionsare rejected, e.g.list("STAN_THREADS=TRUE")has to belist(stan_threads = TRUE)user_headeris only settable through theuser_headerargument (notcpp_options) and a new method$user_header()is added to read it back.When do we recompile
If the user sets
force_recompile = TRUEor when any of these change:include_paths(when the change means different content, not e.g. a directory rename)cmdstan_model()at a different one) or its pathcpp_optionsorstanc_optionsthe user supplied (options cmdstanr fills in itself are recorded but don't trigger a rebuild, except the model name, which we get from the file name and we do compare, so renaming your .stan file recompiles)Or when we can't trust what we recorded:
If more than one of these applies we report all of them (if possible).
One exception:
cmdstan_model(exe_file = )) can't recompile automatically since there's no Stan file to build fromThe new build record
The current plan is a file
.<exe>.cmdstanr.jsonthat is written next to the executable. It contains:cpp_optionsandstanc_optionsthe user suppliedinclude pathsand user header pathstanc --infoTracking issue: #1258