Skip to content

edits to included files are not detected #1237

Description

@jgabry

edits to included files are not detected

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.

The design is settled below, in the section added from the #1254 review. In
short: during a successful build, when stanc is running anyway, record which files
the program pulled in; compare each by content hash rather than mtime; and
re-run stanc --info during validation so that which file an include resolves to is
answered by stanc rather than by reimplementing its rules in R. That last part is
what makes the shadowing case below tractable.

When an include cannot be resolved, the check errors with stanc's own message rather
than rebuilding: the build's own stanc call would fail on the same include, so a
rebuild is a doomed compile in place of the message (#1254 §5, "A re-resolution that
fails is an error, not a verdict").


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 a free one.

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. 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. 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 content moves, so
neither a recorded-path check nor the include-path check added in #1235 sees it.
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. Deletion is the mirror image and falls out of the same re-resolution.

Sequencing and behaviour change

This closes with #1255, in Stage 4 of #1258. The record (#1238) stores the
resolved include list with content hashes, and the assessment engine (#1255) is
what compares it. The hash policy is part of the record format, so it is settled
with #1238 rather than left to the first consumer.

touch stops forcing a rebuild. force_recompile = TRUE covers it, but it is
a real behaviour change and needs a NEWS entry.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions