As a result, bbi_config.json isn't written. As discussed at metrumresearchgroup/bbr#685, from bbr's perspective it would be useful to have bbi_config.json consistently written as an indication that a model run has completed (even if unsuccessfully).
When considering this change, we should go through local's Cleanup and decide which parts should run in case of a failure.
Here are three related issues mentioned in the bbr thread. (At this point I'll track them here, but if they don't end up getting handled by the same series, they should be split off into dedicated issues.)
if bbi aborts early due to the data file not existing, it doesn't write the config file or create the output directory. From a consistency perspective, it might be good if bbi_config.json was created. I'm not sure where to draw the line between a pure bbi early non-zero exit (e.g., unknown argument given) and a model run error. But if the data file doesn't exist, that's something nmfe would fail on, and bbi is just catching it early. So it seems reasonable for it to behave consistently with other nmfe failures.
There are perhaps other cases that behave similarly to the non-existent data path.
if --overwrite is passed but bbi aborts to a non-existent data path (again, there may be other cases), the directory is not removed. That's confusing from bbr's perspective because it looks at bbi_config.json and will think it's from its latest model submission.
bbr is interested in knowing whether a model is currently running. To help, bbi could do the combination of 1) adding a file before launching nmfe and removing it on cleanup and 2) ensuring cleanup still fires when the nmfe subprocess exits with a non-zero status
[ related to metrumresearchgroup/bbr#685, +cc @seth127 @barrettk ]
For the various errors where
RecordConcurrentErroris invoked (including a non-zero exit of thenmfe*subprocess)https://github.com/metrumresearchgroup/bbi/blob/835953efed10fff2d7024ac1c3dd88949b268c05/cmd/local.go#L185-L191
...
trueis sent thecancelchannelhttps://github.com/metrumresearchgroup/bbi/blob/835953efed10fff2d7024ac1c3dd88949b268c05/cmd/root.go#L177-L178
... which stops the execution before the turnstile cleanup phase.
As a result,
bbi_config.jsonisn't written. As discussed at metrumresearchgroup/bbr#685, from bbr's perspective it would be useful to havebbi_config.jsonconsistently written as an indication that a model run has completed (even if unsuccessfully).When considering this change, we should go through local's
Cleanupand decide which parts should run in case of a failure.Here are three related issues mentioned in the bbr thread. (At this point I'll track them here, but if they don't end up getting handled by the same series, they should be split off into dedicated issues.)
if bbi aborts early due to the data file not existing, it doesn't write the config file or create the output directory. From a consistency perspective, it might be good if
bbi_config.jsonwas created. I'm not sure where to draw the line between a pure bbi early non-zero exit (e.g., unknown argument given) and a model run error. But if the data file doesn't exist, that's something nmfe would fail on, and bbi is just catching it early. So it seems reasonable for it to behave consistently with other nmfe failures.There are perhaps other cases that behave similarly to the non-existent data path.
if
--overwriteis passed but bbi aborts to a non-existent data path (again, there may be other cases), the directory is not removed. That's confusing from bbr's perspective because it looks atbbi_config.jsonand will think it's from its latest model submission.bbr is interested in knowing whether a model is currently running. To help, bbi could do the combination of 1) adding a file before launching nmfe and removing it on cleanup and 2) ensuring cleanup still fires when the nmfe subprocess exits with a non-zero status