Beta testing feedback

[mattwarkentin]

Submit your issues and feedback here.

Please wrap code in code fences (i.e., 3 back-ticks) to make it easier to read.

[johnhutchinson99]

Most of these are just feature ideas and suggestions, so feel free to ignore anything you don't like.

1. When trying to modify a readonly workset, it doesn't tell you that its locked. It just sends a generic 400 bad request response. This is minor, but it would be nice to have a check and a warning that it's locked.
2. It would be convenient if create_scenario also returned the scenario object like load_scenario does. This would remove the need to create a scenario and then manually load it.
3. (I may be missing something in this one) It would be nice to be able to retrieve the file path of a parameter CSV file in a workset directory. I've been using this paste(modelToUse,".set.",worksetName,"/set.",worksetName,"/",paramName,".csv",sep="") which works though.

[mattwarkentin]

Thanks for the feedback.

1. Yes, I can add a check for this and a better error message. Thanks.
2. The original version of create_scenario() did indeed return the scenario object but I then reverted this functionality. I'm not sure which approach I like better. When the function primarily has a non-local side-effect like creating an object in the API, I tend to not like that it also returns the object for the reason that you only create_scenario() once, but you load_scenario() every time you work on the project. So for the time being I have separated these functions, but I will continue to think this over and we can discuss further.
3. Why are you trying to retrieve the CSV path? If you can provide me some further context or your use-case I can think about how to update the user experience. Thanks.

[mattwarkentin]

For posterity, the field scenario$WorksetDir exposes the path to the workset on disk.

[YibingRuan]

Hi Matt, Is it possible to add the option of dealing with NA in the get_table function, for example, na = c("", "null") ? When I loaded the output table with empty or null values, the numeric variable expr_value will be coerced as character. Itself isn't a big issue but it creates error when loading several tables with expr_value as either numeric or character. Thanks.

[mattwarkentin]

Great observation. I also ran into this annoying issue.

You're right, it generally isn't a huge problem when loading a single table from a single run, but when you are trying to load multiple tables (such as with a ModelRunSet) then it becomes a problem because the function tries to stack the tables and it can't stack character and numeric columns. I do want to come up with a better solution.

I think it is safe to assume expr_value should ALWAYS be numeric. If this is true, then I can force/coerce that column to numeric type when reading the CSV data. By forcing it to numeric, anything that doesn't look like a number (NA, "", null) should be coerced to NA, which is fine.

You are the expert. Do you know if ANY of the output tables would return an expr_value that isn't numeric?

[mattwarkentin]

@YibingRuan I have pushed changes so that expr_value is always coerced to numeric silently. This means that tables should always be stack-able for a ModelRunSet. Let me know if you have any further issues or if you don't think this is a good change.

[YibingRuan]

You are the expert. Do you know if ANY of the output tables would return an expr_value that isn't numeric?

All output tables should have numeric expr_value. Thank you for pushing the changes!

[johnhutchinson99]

Modifying many parameters in a targets pipeline can result in an error

! Error running targets::tar_make() Error messages: targets::tar_meta(fields = error, complete_only = TRUE) Debugging guide: https://books.ropensci.org/targets/debugging.html How to ask for help: https://books.ropensci.org/targets/help.html Last error: _store_ a character vector argument expected

This seems to only happen when running several worksets in a pipeline, making it very difficult to reproduce using a simple example. My best guess is that there are too many API requests being sent in a short time. Putting Sys.sleep to delay runs does not seem to help however

[YibingRuan]

Hi Matt, On the Cloud interface, there is a toggle switch for showing and not showing hidden parameters: Occasionally, we need to modify hidden parameters for our scenarios. For example, Mariet recently needs to change the test performance of the HPV DNA testing in the cervical model under a hidden parameter table ‘HpvDnaTestSensitivitySpecificity’. I noticed that for now we could only copy parameters that are not hidden. Just wonder if there could be a feature in the future to copy and change hidden parameters? Thanks, Yibing

[mattwarkentin]

@johnhutchinson99 I will investigate when I return.

@YibingRuan Thanks for bringing this up. I didn't know this distinction between visible and hidden parameters. I have sent an email to the admin to figure out how we can copy hidden parameters using the API.

[mattwarkentin]

@YibingRuan I have been told by the OpenM++ team that the visible/hidden toggle is only a GUI thing and shouldn't prohibit copying parameters. Have you tried copying a "hidden" parameter?

[YibingRuan]

It didn't work in my previous attempt. I just tried again and it worked. My previous attempt might be during the time they updated the model. Thanks.

[YibingRuan]

Hi Matt, What is the syntax to use set_base_digest? I tried with new_scenario$set_base_digest(base = baserun_digest)in which baserun_digestwas the model digest of the default workset. But I got the following error message: " Error in path.expand(path) : argument "file" is missing, with no default " Thanks, Yibing

[mattwarkentin]

Sounds like a bug introduced in recent changes...will work to fix this asap.

[mattwarkentin]

@YibingRuan I am not able to reproduce this error. Can you please make sure you've installed the most recent version and try again? Let me know if the error persists.

[YibingRuan]

Hi Matt, I have the most recent version. I found this error only occurs if I try to run scenarios previously created from the UI. It does not occur if the scenario is created de novo from R. So probably no big deal if everything is done in R.

[mattwarkentin]

Note to self...

• Maybe rethink whether tables and parameters should be top-level fields or should be within a sub-environment for better organization
• Embed a parameter object inside the ModelRun since parameters belong to a run (a permanent copy is created) and it helps to know which parameters were associated with a given model run
• Update and improve documentation and add logo/favicon

[zsun2023]

Hi Matt, as mentioned in the meeting, please see the issues I have when using digest numbers. This is my first time reporting a bug and I'm still a R beginner so not too sure how to best describe the issue. I just pasted the codes and error messages I got. Thanks! Issue: Ideally, we want to use digest number "f3611d0b5add6342fcb0f258ac4b38da" for the lung model. However, current oncosimx package seems to have a bug if using digest number. After I change to use 'OncoSimX-lung' instead, it works.

Codes not working

modelToUse <- "f3611d0b5add6342fcb0f258ac4b38da" scenarioNameRandom <-"test" model= load_model(modelToUse) baserun_digest = model$ModelRuns$RunDigest[[1]] scenarioToUse <- create_scenario(modelToUse,scenarioNameRandom,baserun_digest) Error in httr2::req_perform(): ! HTTP 400 Bad Request. Run rlang::last_trace() to see where the error occurred.

Works if not using digest number modelToUse <- 'OncoSimX-lung'

model= load_model(modelToUse) baserun_digest = model$ModelRuns$RunDigest[[1]] scenarioToUse <- create_scenario(modelToUse,scenarioNameRandom,baserun_digest)

The scenario was created successfully.

[mattwarkentin]

Good catch, @zsun2023! This was indeed a bug that is now fixed with 1cce1b5c34642d9b5443458b743bfcfb5b06b4d6. Please install the latest version.

[mattwarkentin]

• New and better errors thrown if you try to update or query a param that hasn't been copied
• Make sure to have a function for every API endpoint and make sure they work right (e.g., microdata)
• Add handlers for registering local or remote API connections

[mattwarkentin]

Probably makes sense to update this function to check that a parameter belongs to a workset: https://github.com/oncology-outcomes/oncosimx/blob/4ddedac0c7cd36d7a1550b2d17b43e6436840b9f/R/R6ClassWorkset.R#L311-L319

[mattwarkentin]

# zzz.R
.onLoad <- function(libname, pkgname) {
  # << code goes here for instantiating a connection when package is loaded >>
}

[mattwarkentin]

I think this is wrong, for example... https://github.com/oncology-outcomes/oncosimx/blob/4ddedac0c7cd36d7a1550b2d17b43e6436840b9f/R/Microdata.R#L25-L33

[barnzilla]

Hey Matt,

I was wondering if an argument could be added to opts_run() so that the Mpi list could be excluded if/when users are running a non-MPI model? Something like the following;

opts_run <- function(
  SimulationCases = '5000',
  SubValues = '12',
  RunStamp = NULL,
  Tables = list(),
  Mpi = TRUE
  ...
) {
  if (rlang::is_null(RunStamp)) {
    RunStamp <- sub('.' , '_', fixed = TRUE, format(Sys.time(),"%Y_%m_%d_%H_%M_%OS3"))
  }

  SimulationCases <- format(SimulationCases, scientific = FALSE)

  opts_run <- structure(
    list(
      RunStamp = RunStamp,
      Opts = list(
        Parameter.SimulationCases = SimulationCases,
        OpenM.SubValues = as.character(SubValues),
        OpenM.RunStamp = RunStamp,
        OpenM.Threads = '4',
        OpenM.LogToConsole = 'true'
      ),
      Tables = Tables
    ),
    class = c('OpenMppRunOpts', 'list')
  )

  if(isTRUE(Mpi)) {
    opts_run$Mpi <- list(
      Np = 4,
      IsNotOnRoot = TRUE
    )
  }

  return(opts_run)

}

[mattwarkentin]

@barnzilla Great suggestion! I am happy to work this kind of update into the code base. Thanks for the feedback.

[barnzilla]

Sounds great, Matt! And thanks so much for all the work you've done on the package. It's great!

Take care, Joel

[barnzilla]

Hey Matt,

In create_scenario(), I think you're enforcing unique scenario names, which is nice (see bolded portion):

function (model, name, base = NULL) 
{
    existing_scenarios <- get_worksets(model)$Name
    if (name %in% existing_scenarios) {
        rlang::abort("Scenario name already in use, choose a different name.")
    }
    is_model_name <- model %in% get_models()$Name
    body <- list(ModelName = if (is_model_name) model else get_digest_model(model), 
        ModelDigest = if (!is_model_name) model else get_model_digests(model)[[1]], 
        Name = name)
    if (!rlang::is_null(base)) 
        body$BaseRunDigest <- base
    create_workset(body)
    invisible()
}

I don't think the same is happening in run(). It's possible to have two model runs with the same name (although, of course, the RunDigest values will differ). Wondering if you think it's a good idea to enforce unique model run names?

Take care, Joel

[mattwarkentin]

@barnzilla I agree, I think it would be a good quality-of-life addition to ensure unique run names also. Happy to make this change.

[barnzilla]

Thanks, Matt!

[mattwarkentin]

Fixed https://github.com/oncology-outcomes/openmpp/issues/3#issuecomment-1949541233 in 9e8fef0c288b44c9caaee7d17399d0c6378d0d4c

[mattwarkentin]

@barnzilla Regarding your comment in https://github.com/oncology-outcomes/openmpp/issues/3#issuecomment-1946526055, does it throw an error when trying to run a model with MPI options for a non-MPI system? I just wanted to clarify the issue further before implementing a fix.

[barnzilla]

Hey @mattwarkentin,

Yes, the following error is thrown:

Error in `resp_abort()`:
! HTTP 400 Bad Request.
Run `rlang::last_trace()` to see where the error occurred.

Here's the code I ran to get this error:

# Load dependencies
library(openmpp) # Deployment: https://github.com/oncology-outcomes/openmpp/commit/1b6c3a8844952fa91b81a36f8fc39dc5840fbb09

# Set environment variable
do.call(
  what = Sys.setenv,
  args = setNames(
    object = paste0("http://localhost:", 61285),
    nm = "OPENMPP_LOCAL_URL"
  ) %>%
    as.list
)

# Register local connection
use_OpenMpp_local()

# Set model name
model_name <- get_models()$Name[1]

# Load model
pohem <- load_model(model_name)

# Load default scenario
pohem_default <- load_scenario(model_name, "Default")

# Load baserun
baserun_digest <- pohem$ModelRuns$RunDigest[[1]]

# Create new scenario
scenario_name <- "scenario_011"
create_scenario(model_name, scenario_name, baserun_digest)

# Load new scenario
scenario <- load_scenario(model_name, scenario_name)

# Set scenario to read-only
scenario$ReadOnly <- TRUE

# Run model
scenario$run("model_run_007", wait = TRUE, progress = TRUE)

[mattwarkentin]

Great, thanks for the reprex.

[barnzilla]

Here's the backtrace on the error (should have included previously):

Backtrace:
    x
 1. \-scenario$run("model_run_007", wait = TRUE, progress = TRUE)
 2.   \-openmpp::run_model(opts)
 3.     +-httr2::resp_body_json(...)
 4.     | \-httr2:::check_response(resp)
 5.     |   \-httr2:::is_response(req)
 6.     \-httr2::req_perform(...)
 7.       \-httr2:::resp_abort(resp, error_body(req, resp))
Run rlang::last_trace(drop = FALSE) to see 1 hidden frame.

[mattwarkentin]

I think the most recent commit should fix this issue. Please give it a try when you have a chance and let me know. After some discussion with the OpenM++ team, it is very clear that run configuration options are very system and model dependent and too many "defaults" can create issues. So I have simplified opts_run() to be more flexible but less prescriptive.

Fixed https://github.com/oncology-outcomes/openmpp/issues/3#issuecomment-1955040226 with 717cba5ec5489bfad970054ffaa8c0614f584f30

[barnzilla]

Awesome, thanks @mattwarkentin! Just updated my version of the package and it works!

Totally get it re: too many default options.

Thanks again!

[mattwarkentin]

@barnzilla No problem, thanks for the feedback!

I am going to close this issue since I think it has outgrown its origins and the package is stabilizing. If any new issues arise, please open a separate issue. Thanks.