Another issue noticed while working on #1235. I asked Claude to write up the report below:
cmdstan_model(exe_file = ) reads the executable's build metadata at
R/model.R:324, and that call is not guarded. If the file cannot be launched as
a process the raw processx failure reaches the user:
exe <- tempfile()
writeLines("i am not a program", exe)
Sys.chmod(exe, "0644")
cmdstan_model(exe_file = exe)
#> Error: ! Native call to `processx_exec` failed
#> Caused by error in `chain_call(...)`:
#> ! cannot start processx process '/tmp/.../file1025b6854155e'
#> (system error 13, Permission denied) @unix/processx.c:651 (processx_exec)
assert_file_exists(exe_file, access = "r") checks only that the file is
readable, so anything readable-but-not-runnable — wrong architecture, a text
file, a lost execute bit — arrives here and fails this way.
What the right behaviour probably is
Erroring is defensible: you were handed a path that is not a working executable,
and $sample() would fail moments later anyway. The problem is the message, not
the decision to stop. So this is not simply a matter of wrapping the call in
tryCatch() — it wants an error that names the file and says it could not be
run as a CmdStan executable.
Note the asymmetry with the other two call sites, which is worth resolving at the
same time. model_compile_info() is read in three places:
| Site |
Binary cannot be launched |
Binary runs, info exits nonzero |
initialize(), R/model.R:324 |
raw processx error |
silent; $cpp_options() empty |
no-op compile, R/model.R:751 |
silent (tryCatch → NULL) |
silent |
post-commit merge, R/model.R:958 |
silent (tryCatch → NULL) |
silent |
The two compile paths are deliberately quiet: an executable that cannot report
its metadata will fail loudly at $sample(), and the only cost of staying quiet
is that $cpp_options() under-reports options inherited from make/local, which
in turn produces a misleading threads_per_chain "will have no effect" warning.
That is recorded as a consequence of having no build provenance beside the
executable (#1238).
The construction path is the one that is neither quiet nor clear.
Related: tests/testthat/test-model-recompile-logic.R has a skip()ped test,
"warning when no recompile and no info", expecting a "Recompiling is recommended." warning for an executable whose metadata cannot be read. It is
parked behind #1019, so there is an existing intent to warn here that should be
settled alongside this.
Found while reviewing #1235.
Fix it in a shared adoption helper
From the #1254 design review.
The fix should live in a shared adoption helper used by every entry point that
adopts an executable, rather than being patched at the constructor alone, with
tests covering each entry point separately.
The helper only reaches a subprocess launch when provenance is unknown. Once #1238
lands, adopting an executable whose build record matches its hash hydrates from
that record without launching anything, so the helper covers the fallback path,
which is where a non-CmdStan binary shows up.
Every site that launches the model binary needs it, not only adoption
An unrunnable executable with a valid record never reaches the helper. It adopts
cleanly, and the first launch is the fit, so the raw error arrives at $sample()
instead of at construction. Measured, on a model whose executable lost its execute
bit after it was built:
Error: ! Native call to `processx_exec` failed
Caused by error in `chain_call(...)`:
! cannot start processx process './bern' (system error 13, Permission denied)
Note the relative path: R/run.R runs the binary from its own directory, so the
message names neither the model nor the file it could not start.
This is not a corner case. R's own utils::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 in exactly this state, record and .stan file
intact. A binary built for another platform on a shared drive arrives in it too, and
so does one whose recorded TBB directory is gone.
The design settles what cmdstanr does about it
(#1254 §6, "An executable that will not launch is an error, not a rebuild trigger")
and puts the remedy here rather than in a rebuild: the error names the executable, and for a model with a source it says that
force_recompile = TRUE rebuilds it. With only an executable there is nothing to
rebuild and the message says so instead.
cmdstanr launches the model binary at four sites — R/run.R:660 and :782,
run_info_cli() (R/cpp_opts.R:11), and parse_cmdstan_args() (R/model.R:2750) —
and the error belongs at all of them, not only the two that adopt.
Another issue noticed while working on #1235. I asked Claude to write up the report below:
cmdstan_model(exe_file = )reads the executable's build metadata atR/model.R:324, and that call is not guarded. If the file cannot be launched asa process the raw
processxfailure reaches the user:assert_file_exists(exe_file, access = "r")checks only that the file isreadable, so anything readable-but-not-runnable — wrong architecture, a text
file, a lost execute bit — arrives here and fails this way.
What the right behaviour probably is
Erroring is defensible: you were handed a path that is not a working executable,
and
$sample()would fail moments later anyway. The problem is the message, notthe decision to stop. So this is not simply a matter of wrapping the call in
tryCatch()— it wants an error that names the file and says it could not berun as a CmdStan executable.
Note the asymmetry with the other two call sites, which is worth resolving at the
same time.
model_compile_info()is read in three places:infoexits nonzeroinitialize(),R/model.R:324processxerror$cpp_options()emptyR/model.R:751tryCatch→NULL)R/model.R:958tryCatch→NULL)The two compile paths are deliberately quiet: an executable that cannot report
its metadata will fail loudly at
$sample(), and the only cost of staying quietis that
$cpp_options()under-reports options inherited frommake/local, whichin turn produces a misleading
threads_per_chain"will have no effect" warning.That is recorded as a consequence of having no build provenance beside the
executable (#1238).
The construction path is the one that is neither quiet nor clear.
Related:
tests/testthat/test-model-recompile-logic.Rhas askip()ped test,"warning when no recompile and no info", expecting a
"Recompiling is recommended."warning for an executable whose metadata cannot be read. It isparked behind #1019, so there is an existing intent to warn here that should be
settled alongside this.
Found while reviewing #1235.
Fix it in a shared adoption helper
From the #1254 design review.
The fix should live in a shared adoption helper used by every entry point that
adopts an executable, rather than being patched at the constructor alone, with
tests covering each entry point separately.
The helper only reaches a subprocess launch when provenance is unknown. Once #1238
lands, adopting an executable whose build record matches its hash hydrates from
that record without launching anything, so the helper covers the fallback path,
which is where a non-CmdStan binary shows up.
Every site that launches the model binary needs it, not only adoption
An unrunnable executable with a valid record never reaches the helper. It adopts
cleanly, and the first launch is the fit, so the raw error arrives at
$sample()instead of at construction. Measured, on a model whose executable lost its execute
bit after it was built:
Note the relative path:
R/run.Rruns the binary from its own directory, so themessage names neither the model nor the file it could not start.
This is not a corner case. R's own
utils::unzip()extracts an executable withoutthe execute bit where
untar()andfile.copy()keep it, so a project folder sharedas a zip and unpacked from R arrives in exactly this state, record and
.stanfileintact. A binary built for another platform on a shared drive arrives in it too, and
so does one whose recorded TBB directory is gone.
The design settles what cmdstanr does about it
(#1254 §6, "An executable that will not launch is an error, not a rebuild trigger")
and puts the remedy here rather than in a rebuild: the error names the executable, and for a model with a source it says that
force_recompile = TRUErebuilds it. With only an executable there is nothing torebuild and the message says so instead.
cmdstanr launches the model binary at four sites —
R/run.R:660and:782,run_info_cli()(R/cpp_opts.R:11), andparse_cmdstan_args()(R/model.R:2750) —and the error belongs at all of them, not only the two that adopt.