get-path-from-object: non-existent output directory fixes

kyleam commented 1 year ago

This series fixes two errors triggered by a missing output directory:

model_summaries() (due to a build_path_from_model call underneath) failed if a model's output didn't exist.

This is likely the error described by @barrettk at https://github.com/metrumresearchgroup/bbr/issues/569#issuecomment-1400485335. The main wrinkle needed to get @seth127's reproducer to trigger the issue is using a bbi version of at least v3.2.0, so that the model_summaries_concurrent() code path is used.
get_config_path(..., .check_exists = FALSE) failed when the model's output directory didn't exist.

This came up when working on a bbr.bayes PR.

Code-wise, the source of these errors are independent, but I'm putting them in the PR as conceptually the same (mostly to avoid validation ID conflicts :/).

kyleam commented 1 year ago

I was going to comment that we should test this case on summary_log() [...]

Yeah, as you say, I think testing the lower layers is probably sufficient here, but I can also see the point of making a test trigger what was initially reported. I've added one with the last push. Here's how it fails on top of main without the other commits in this PR (in particular e8a8fdf78ea5f6547edc685b7b94943714ad0098):

Error ('test-summary-log.R:115'): summary_log() doesn't fail on missing model output directory [BBR-SMLG-012]
<purrr_error_indexed/rlang_error/error/condition>
Error in `map_chr(.mods, ~fs::path_rel(build_path_from_model(.x, ".lst")))`: ℹ In index: 1.
Caused by error in `get_output_dir_nonmem()`:
! Assertion on '.path' failed: Directory '/tmp/RtmpYPUth3/file32323e1009a4/mod' does not exist.
Backtrace:
  1. withr::with_tempdir(...)
       at test-summary-log.R:115:2
  9. bbr::summary_log(".")
       at rlang/R/eval.R:96:2
 10. bbr:::summary_log_impl(mod_list, ...)
       at bbr/R/summary-log.R:44:2
 12. bbr:::model_summaries.list(.mods, ...)
       at bbr/R/model-summaries.R:39:2
 13. bbr:::model_summaries_concurrent(.mods, .bbi_args, .fail_flags)
       at bbr/R/model-summaries.R:179:4
     ...
 29. bbr:::get_output_dir_nonmem(.bbi_object, .check_exists)
       at bbr/R/get-path-from-object.R:65:2
 30. checkmate::assert_directory_exists(.path)
       at bbr/R/get-path-from-object.R:247:4
 31. checkmate::makeAssertion(x, res, .var.name, add)
       at tmp/IXEVAIAXHXSW/checkmate:1:130
 32. checkmate:::mstop(...)
       at checkmate/R/makeAssertion.R:44:6
 33. base::stop(simpleError(sprintf(msg, ...), call.))
       at checkmate/R/helper.R:2:2
────────────────────────────────────────────────────────────────────────────────

══ Results ═════════════════════════════════════════════════════════════════════
Duration: 7.5 s

[ FAIL 1 | WARN 0 | SKIP 0 | PASS 82 ]

kyleam commented 1 year ago

Merging, but of course let me know if you think the new test should be adjusted in any way (or dropped entirely).

metrumresearchgroup / bbr

get-path-from-object: non-existent output directory fixes #573