Skip to content

exe_file_ conflates the installed executable with the planned build destination #1253

Description

@jgabry

From the design review for #1238. I asked Claude to write up the report below.


exe_file_ is two different things at once: the path where the executable is,
and the path where the next build will put one. They coincide most of the
time, which is why this has held together, but they diverge whenever a
destination is configured without building — and the divergence is invisible.

The problem

Every run method resolves the binary through self$exe_file():

R/model.R:1440, 1601, 1738, 1912, 2046, 2230, 2367, 2441
    sample, optimize, variational, laplace, pathfinder,
    generate_quantities, diagnose, log_prob

resolve_exe_path() also treats it as a destination (R/model.R:2707-2709),
swapping the directory while keeping the basename:

if (length(self_exe_file) != 0) {
  self_exe_file <- file.path(dir, basename(self_exe_file))
}

and $compile() commits that path whether or not a build happened, deliberately
and with a comment saying so (R/model.R:948-950):

# These fields also describe dry runs, so update them outside the commit block.
private$cmdstan_version_ <- compiled_cmdstan_version
private$exe_file_ <- exe

So $compile(dir = elsewhere, dry_run = TRUE) retargets the field, and every run
method then looks for a binary that was never built there.

This also blocks the #1238 state model, which separates desired configuration
from the installed-artifact snapshot. A field that is both cannot sit in either.

What the design does about it

The second meaning loses its only consumer, so the field collapses instead of
splitting.
Storing "where the next build will put one" is worth doing only if
something later can act on it, and after #1256 nothing can: $compile() is gone,
so every build is a fresh cmdstan_model() call that derives its destination from
its own arguments, and compile_impl() is a plain function returning
list(path =, record =, ...) that never touches object state. The destination
becomes a local variable that either becomes an installed executable or is
discarded. No accessor has to be split, publicly or internally.

What has to be true in the new code is one sentence: the installed path is set
from a build that happened, never from one that was planned.
That is a property
of compile_impl() rather than an audit across the constructor, so it is checkable
when that function is reviewed. #1258 carries it as the condition for closing this.

Scope

Breaking, so 1.0. $exe_file() stops ever returning a planned destination, and the
path argument goes.

Retained invariant, with its test rewritten. test-model-compile.R:1526 checks
that a build refuses a directory destination, and reaches that state through
cmdstan_model(compile = FALSE), $exe_file(<directory>) and $compile() — three
pieces of API this design removes, so the test cannot stand as written. The guard
itself stays, because dir still resolves onto a directory whenever
file.path(dir, basename) names one, and the test is rewritten to reach it that
way.

A caller must be able to ask whether a usable executable exists, without an
error.
brms reads $exe_file() and calls file.exists() on the result
(brms/R/backends.R:377-383) to decide whether to rebuild a fit whose executable is
gone. That code is ours to change and is not a constraint; the capability is. The
getter stays a plain accessor that never errors (#1254 §5), so the question stays
askable.

Adoption does not always launch the executable. That flow belongs to #1238: a
valid record matching the executable's hash is hydrated from without running
anything, and <exe> info is the unknown-provenance fallback.

The $exe_file(path) setter is removed

R/model.R:365-370 assigns private$exe_file_ and returns it, with no validation,
no snapshot refresh and no provenance update:

exe_file = function(path = NULL) {
  if (!is.null(path)) {
    private$exe_file_ <- path
  }
  private$exe_file_
}

Retargeting an object this way leaves it holding a record that describes a different
binary, which is the pairing the build record exists to prevent. Constructing a new
object is the replacement, matching how the rest of the mutable-configuration
surface is handled.

Its one call site is tests/testthat/test-model-compile.R:1535, inside the
directory-destination test above. The call retires with the setter; the test is
rewritten to reach the guard through dir, which is the only route left to a
directory destination.

Choosing an executable filename goes with it. dir places the binary in any
directory, the basename comes from the .stan file's name or from
write_stan_file(basename = ) (R/file.R:61) for generated code, and what is
actually lost is naming two builds inside one directory, which a subdirectory covers
(#1254 §7, "dir replaces it and gives up nothing but the filename").

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