`run_log()` and friends shouldn't fail when it can't parse a model

[seth127]

(pulled out from issue originally mentioned in #475)

@kylebaron: run_log() should gracefully if a run cannot be read; this should be retained in the output but with no data; maybe inject a note that the run could not be recovered ... maybe a time stamp; the key thing here is that I can run the log at any time and it will work and I don't need to maintain code around this.

Notes

• This should be true of config_log() and summary_log() too
• My vote would be to have it return the row, with NA for all columns (except the absolute_model_path and run), though another option would be to drop those rows from the tibble and maybe warn about them.

[seth127]

@kylebaron do you have an example of this happening? Just in a few minutes of playing with the test models, I couldn't get it to fail like you describe.

Ideally, if you could post something like a (possibly intentionally mangled) model output dir that would cause a test like this one to fail.

It's also possible that what was actually failing for you was the add_config() or add_summary() functions. In that case, you would want to make one of these tests fail:

• add_summary() test
• add_config() test

[seth127]

On further investigation, I'm pretty sure that this isn't run_log() that's failing, but actually summary_log().

The confusion likely comes from some example code that's been passed around that has run_log() %>% add_summary() at the top. This will call summary_log(), which in turn calls model_summaries(). I've recently (re)discovered that model_summaries(), though it's supposed to capture any errors from bbi and pass them back, will error out when bbi hits an actual panic (as opposed to a properly handled error that it can pass back). Unfortunately, this is prone to happen when trying to parse an incomplete .lst file, i.e. an in-progress run.

This is sort of a nuance of error handling in Go, and should definitely be fixed on the bbi side. Hopefully that will solve this issue without needing any changes in bbr.

[seth127]

This was likely fixed by https://github.com/metrumresearchgroup/bbi/pull/272 though it's a little hard to say without an example to test against.

Leaving this open for now, in case we find one where this is still happening, but we'll hope that this was fixed by bbi 3.2.0 and potentially close this at some point.

[seth127]

Moving this off of the next milestone, based on previous comment and no further reports since. I'll leave it open in the backlog, in case it comes back up.