Note: While working on #1235 I was reminded of several longstanding issues. I asked Claude to turn them into reports.
The up-to-date check reads the mtimes of the top-level Stan program and the user
header. Files reached by #include are not checked at any depth — including
one level down, which is worth stating because "nested includes" suggests the
direct case works. It does not:
# include dir holds params.stan declaring `alpha`
mod <- cmdstan_model(stan_file, include_paths = inc, force_recompile = TRUE)
# edit inc/params.stan to declare `beta` instead
mod$compile()
#> no recompilation
mod$variables()$parameters
#> beta <- but the executable still has alpha
The object then validates data and initial values against a program it is not
running, which is the failure class of #1228. force_recompile = TRUE is the
escape hatch and is now documented as such, but the check should eventually
notice on its own.
Preferred design. During a successful compile, when stanc is running anyway
and the cost is invisible, record which files the program actually pulled in.
The later up-to-date check just stats those recorded paths. Editing a recorded
include is caught directly; adding a directive to the top-level file or to an
already-recorded include changes a file that is already checked. Fast-path cost
stays in the same order as today.
Known gap in that design. Shadowing is caught by neither this nor the
include-path check added in #1235: with include_paths = c(A, B) and
foo.stan resolving from B, creating A/foo.stan changes which file the
program uses while the path vector is unchanged and no recorded file's mtime
moves. Deletion is the mirror image, and is recoverable by treating a recorded
file that no longer exists as dirty. Recording and statting the include
directories as well would close both, since create/delete bumps a directory's
mtime — but compile artifacts (the executable, the generated .hpp) are written
into dirname(stan_file), which is the include path CmdStanR infers when none is
given, so that trade-off needs thought.
Alternatives considered. Hashing the generated C++ is complete — it moves
with source edits, include edits, include paths, stanc version and options — but
makes every no-op $compile() spawn stanc, which is the one path that is
currently just a few stat() calls. Statting whole include trees runs into the
same artifact-directory problem above.
Getting the scan wrong degrades the heuristic rather than corrupting a build, so
it should fail toward rebuilding whenever a directive cannot be resolved.
How the include list is obtained
Settled in the #1254 design review.
The list comes from stanc --info's included_files, transitively resolved to
absolute paths. Verified present and identical on CmdStan 2.35.0, 2.36.0 and
2.39.0, so it works at the current minimum supported version.
Two constraints:
--info requires the model to parse, which is fine because the record is only
written after a successful build.
- The list must be captured on the build path, not by calling
$variables()
later, or it describes a different moment than the artifact it claims to
describe.
This adds a stanc invocation rather than reusing an existing one.
model_variables() is reached only from $variables() and the fit methods, and
the commit block nulls variables_, so compilation does not currently run
stanc --info at all. Measured at 29.9 ms against a compile measured at 6.7 s for bernoulli.stan
with precompiled headers enabled and 13.8 s without — a floor rather than a
typical figure, since a program with many user functions or ODE solves is
substantially slower. So the cost is acceptable — but it is a real addition, not free.
Content hashes, not mtimes, for each recorded include. mtime's failure mode is
a false negative — a silently stale executable, the bug class this effort exists
to remove — while hashing's is a wasted rebuild. The suite already contains an
observed mtime-granularity miss, worked around rather than fixed, in
test-model-compile-user_header.R: "On GHA Windows/R 4.1 files created close
together sometimes compared equal and skipped the mocked recompile."
Do not mix. Hashing includes while the Stan program and user header stay on
mtime keeps the granularity miss on the files most likely to change. Hash
everything in the up-to-date decision, or nothing.
Shadowing is detected, not documented away. An earlier position was that a
newly created file shadowing a resolved include at higher priority could not be
caught without disproportionate effort. Re-running stanc --info during validation
resolves it directly, and at 29.9 ms there is no performance argument for
reimplementing stanc's resolution rules in R instead.
Sequencing and behaviour change
This ships inside #1238, not after it. The tempting alternative — publish the
record format first and let this settle the hash policy as its first consumer —
does not work, because the policy has to be settled up front regardless.
touch stops forcing a rebuild. force_recompile = TRUE covers it, but it is
a real behaviour change and needs a NEWS entry.
Note: While working on #1235 I was reminded of several longstanding issues. I asked Claude to turn them into reports.
The up-to-date check reads the mtimes of the top-level Stan program and the user
header. Files reached by
#includeare not checked at any depth — includingone level down, which is worth stating because "nested includes" suggests the
direct case works. It does not:
The object then validates data and initial values against a program it is not
running, which is the failure class of #1228.
force_recompile = TRUEis theescape hatch and is now documented as such, but the check should eventually
notice on its own.
Preferred design. During a successful compile, when stanc is running anyway
and the cost is invisible, record which files the program actually pulled in.
The later up-to-date check just stats those recorded paths. Editing a recorded
include is caught directly; adding a directive to the top-level file or to an
already-recorded include changes a file that is already checked. Fast-path cost
stays in the same order as today.
Known gap in that design. Shadowing is caught by neither this nor the
include-path check added in #1235: with
include_paths = c(A, B)andfoo.stanresolving fromB, creatingA/foo.stanchanges which file theprogram uses while the path vector is unchanged and no recorded file's mtime
moves. Deletion is the mirror image, and is recoverable by treating a recorded
file that no longer exists as dirty. Recording and statting the include
directories as well would close both, since create/delete bumps a directory's
mtime — but compile artifacts (the executable, the generated
.hpp) are writteninto
dirname(stan_file), which is the include path CmdStanR infers when none isgiven, so that trade-off needs thought.
Alternatives considered. Hashing the generated C++ is complete — it moves
with source edits, include edits, include paths, stanc version and options — but
makes every no-op
$compile()spawn stanc, which is the one path that iscurrently just a few
stat()calls. Statting whole include trees runs into thesame artifact-directory problem above.
Getting the scan wrong degrades the heuristic rather than corrupting a build, so
it should fail toward rebuilding whenever a directive cannot be resolved.
How the include list is obtained
Settled in the #1254 design review.
The list comes from
stanc --info'sincluded_files, transitively resolved toabsolute paths. Verified present and identical on CmdStan 2.35.0, 2.36.0 and
2.39.0, so it works at the current minimum supported version.
Two constraints:
--inforequires the model to parse, which is fine because the record is onlywritten after a successful build.
$variables()later, or it describes a different moment than the artifact it claims to
describe.
This adds a
stancinvocation rather than reusing an existing one.model_variables()is reached only from$variables()and the fit methods, andthe commit block nulls
variables_, so compilation does not currently runstanc --infoat all. Measured at 29.9 ms against a compile measured at 6.7 s forbernoulli.stanwith precompiled headers enabled and 13.8 s without — a floor rather than a
typical figure, since a program with many user functions or ODE solves is
substantially slower. So the cost is acceptable — but it is a real addition, not free.
Content hashes, not mtimes, for each recorded include. mtime's failure mode is
a false negative — a silently stale executable, the bug class this effort exists
to remove — while hashing's is a wasted rebuild. The suite already contains an
observed mtime-granularity miss, worked around rather than fixed, in
test-model-compile-user_header.R: "On GHA Windows/R 4.1 files created closetogether sometimes compared equal and skipped the mocked recompile."
Do not mix. Hashing includes while the Stan program and user header stay on
mtime keeps the granularity miss on the files most likely to change. Hash
everything in the up-to-date decision, or nothing.
Shadowing is detected, not documented away. An earlier position was that a
newly created file shadowing a resolved include at higher priority could not be
caught without disproportionate effort. Re-running
stanc --infoduring validationresolves it directly, and at 29.9 ms there is no performance argument for
reimplementing stanc's resolution rules in R instead.
Sequencing and behaviour change
This ships inside #1238, not after it. The tempting alternative — publish the
record format first and let this settle the hash policy as its first consumer —
does not work, because the policy has to be settled up front regardless.
touchstops forcing a rebuild.force_recompile = TRUEcovers it, but it isa real behaviour change and needs a NEWS entry.