Handling of NULL values passed in bbi_args

[rymarinelli]

Update: This issue is no longer considered a bug. Details in this comment below.

When working on correcting the bug #553, it was observed that NULL could be set as an element in lists, and it would be passed through functions.

  mod1 <- new_model(file.path(tempdir(), "test_path", "1"), .description = "original test-workflow-bbi model",
                        .tags = ORIG_TAGS,.bbi_args = list(parallel = FALSE, threads = NULL))

The resulting model object leaves threads blank.

> mod1

── Status ─────────────────────────────────────────────────────────────────────────────────────────────────────────────

── Finished Running ──

── Absolute Model Path ────────────────────────────────────────────────────────────────────────────────────────────────
• /tmp/Rtmp468kbm/test_path/1

── YAML & Model Files ─────────────────────────────────────────────────────────────────────────────────────────────────
• /tmp/Rtmp468kbm/test_path/1.yaml
• /tmp/Rtmp468kbm/test_path/1.ctl

── Description ────────────────────────────────────────────────────────────────────────────────────────────────────────
• original test-workflow-bbi model

── BBI Args ───────────────────────────────────────────────────────────────────────────────────────────────────────────
• parallel: FALSE
• threads: 

The yaml for mod looks like :

model_type: nonmem
description: original test-workflow-bbi model
bbi_args:
  parallel: no
  threads: ~

[rymarinelli]

I did some looking into where .bbi_args is passed through.

git grep -l .bbi_args | grep R/

``` NAMESPACE NEWS.md R/aaa.R R/bbr.R R/copy-model-from.R R/model-summaries.R R/model-summary.R R/modify-model-field.R R/new-model.R R/nm-join.R R/submit-model.R R/submit-models.R R/test-threads.R R/utils.R ```

git grep -c "(.bbi_args)" | grep R/

``` R/bbr.R:3 R/model-summaries.R:2 R/model-summary.R:2 R/modify-model-field.R:1 R/new-model.R:1 R/nm-join.R:2 R/submit-model.R:1 R/test-threads.R:1 ```

cat R/model-summaries.R | grep ".bbi_args" | grep R/

``` #' Additionally, **if you have renamed your `.ext` file, you will need to pass the new name through**, to `.bbi_args` or `.fail_flags` like so: #' `model_summaries(..., .bbi_args = list(ext_file = "whatever_you_named_it"))` #' @param .fail_flags Same as `.bbi_args` except these are used _only_ when a [model_summary()] call fails. #' In that case, flags are appended to anything in `.bbi_args` and the summary is tried again. .bbi_args = NULL, model_summaries_serial <- function(.mods, .bbi_args, .fail_flags) { model_summary(.m, .bbi_args = .bbi_args) if (!is.null(.bbi_args)) { .fail_flags <- combine_list_objects(.fail_flags, .bbi_args) .retry <- bbr::model_summary(.m, .bbi_args = .fail_flags) cmd <- bbi_exec(c("nonmem", "summary", check_bbi_args(args), paths), model_summaries_concurrent <- function(.mods, .bbi_args, .fail_flags) { summaries <- bbi_exec_model_summaries(.bbi_args, paths) if (!is.null(.bbi_args)) { args_retry <- combine_list_objects(args_retry, .bbi_args) .bbi_args = NULL, model_summaries_concurrent(.mods, .bbi_args, .fail_flags) model_summaries_serial(.mods, .bbi_args, .fail_flags) .bbi_args = NULL, .bbi_args = .bbi_args, ```

cat R/model-summary.R | grep ".bbi_args"

``` #' or they are missing for some other legitimate reason, pass the appropriate flags through the `.bbi_args` argument. #' For example, if have asked to skip the `$COV` step, you would call `model_summary(..., .bbi_args = list(no_cov_file = TRUE))`. #' `ext_file = "NEWNAME"` to `.bbi_args`. #' @param .bbi_args A named list specifying arguments to pass to bbi formatted like `list("nm_version" = "nm74gf_nmfe", "json" = T, "threads" = 4)`. #' See [print_bbi_args()] for full list of options. .bbi_args = NULL, .bbi_args = NULL, .bbi_args = .bbi_args, #' @param .bbi_args A named list specifying arguments to pass to bbi formatted like `list("nm_version" = "nm74gf_nmfe", "json" = T, "threads" = 4)`. Run [print_bbi_args()] to see valid arguments. .bbi_args = NULL, if (is.null(.bbi_args)) { .bbi_args <- list() .bbi_args <- purrr::list_modify(.bbi_args, json = TRUE) args_vec <- check_bbi_args(.bbi_args) ```

cat R/new-model.R | grep ".bbi_args"

``` #' @param .bbi_args A named list specifying arguments to pass to bbi #' 4)`. Run [print_bbi_args()] to see valid arguments. These will be written .bbi_args = NULL, if (!is.null(.bbi_args)) .mod <- replace_all_bbi_args(.mod, .bbi_args) ```

cat R/submit-model.R | grep ".bbi_args"

``` #' @param .bbi_args A named list specifying arguments to pass to bbi #' 4)`. Run [print_bbi_args()] to see valid arguments. Note that bbr does .bbi_args = NULL, .bbi_args = NULL, .bbi_args = .bbi_args, .bbi_args = NULL, .bbi_args <- parse_args_list(.bbi_args, .mod[[YAML_BBI_ARGS]]) args_vec <- check_bbi_args(.bbi_args) ```

[seth127]

After discussion with @kyleam and @rymarinelli I think we can actually leave this for now. As discussed here we are catching this invalid value at submit time (i.e. when it would first be used) so this is really a question of whether it would be better UX for us to catch it earlier (i.e. when they attempt to set it).

There was also a related issue that we ill-defined here about how a user can unset a bbi arg. I had initially stumbled into this by trying to pass .bbi_args = list(threads = NULL) to unset the threads arg and that didn't work (though in retrospect, there's no obvious reason that it should have worked).

Where to go from here

So this issue now boils down to two potential features/improvements that we can consider whether we want to implement:

• Add a clearer interface and documentation for unsetting elements of .mod$bbi_args (e.g. possibly by passing NULL to those elements in a function call).
  • To be clear, "unsetting" the arg means that it would not be present on the model object or in the associated YAML and therefore bbi would fall back to its default (the bbi.yaml or the arg default within bbi) to determine the value.
• Check for invalid .bbi_args at the time they are passed in (to the functions mentioned here) instead of waiting until we attempt to use them.