config_log error when model run with multiple NONMEM versions about bbr

[kyleam]

@callistosp Thanks for the feedback.

@kyleam totally agree on bbi-1 as a solution. If this is done, I think bbi-2 would not be necessary.

Yes, I agree it'd not be necessary.

The appeal I see in it is that this info is captured in bbi_config.json rather than left to custom logic in bbr to determine. bbi already has to determine what nm version to use, so it could record that rather than make bbr try to figure it out. But, as you say, with bbi-1 in place it really doesn't matter, so perhaps not worth the effort and config change at this point.

For bbr-2 this internal function shouldn't encounter a json with multiple defaults if bbi-1 is implemented correctly (actually, I think bbr-1 also shouldn't be an issue if bbi-1 is implemented).

Yes, that's right. bbi-1 is the only strictly necessary one. The main reason I like the idea of bbr-2 is because it's possible that a newer bbr with such a change will encounter a bbi_config.json on disk that was generated with an older bbi (even in cases where the latest bbi is currently installed). Those may be very rare, and it doesn't matter too much because it's just replacing one error with a more appropriate one, but I think it could be worth doing given that it's easy.

[kyleam]

[ Capturing some details from offline discussion ]

The root cause was the bbi.yaml having more than one NONMEM version marked as the default, leading
resolve_nonmem_version extracting multiple versions.

So, the error is right that there is something invalid going on here that we should be catching.

Some possible adjustments (all aimed at getting a better error)

On bbr's end:

1. When submit_model() is called, we could catch the issue and abort. (That probably involves bbi.yaml parsing that we're not currently doing though.)

2. Adjust resolve_nonmem_version() to abort if more than one default is found.

On bbi's end:

1. it should abort if more than one default is found

2. it'd be good for it to record the default version that it actually discovered and used in bbi_config.json.

My initial thinking is that we should do bbr item 2 and bbi item 1, with a maybe for bbi item 2, but I'd like to hear what @callistosp, @seth127, and others think.

[callistosp]

@kyleam totally agree on bbi-1 as a solution. If this is done, I think bbi-2 would not be necessary.

For bbr-2 this internal function shouldn't encounter a json with multiple defaults if bbi-1 is implemented correctly (actually, I think bbr-1 also shouldn't be an issue if bbi-1 is implemented). I might be missing an edge case though that you have in mind.

[callistosp]

ahh good point about potentially different versions of bbr/bbi, that makes sense why you would want to implement in both.

[seth127]

Thanks for this discussion @callistosp and @kyleam. My take on it is that all four of those things sound potentially worth doing. I would prioritize them like:

• bbi-1
  • This seems obviously necessary. We can't ever run with two versions of NONMEM, so it has to pick one. Much better to alert the user and make them pick one, than to have bbi mysteriously and silently pick underneath.
  • Related: maybe I missed it, but did we test what actually happens in this case currently? My understanding was that these models (that started this issue) were never actually run with this "2 versions" configuration, but that it was an artifact of someone modifying the json after the fact. Am I wrong about that?
• bbr-2
  • Maybe this is the intention, but as part of this, I think the desired behavior would be that config_log() itself doesn't error, but returns NA or something for the nm_version column on the offending models, but that we raise a warning or something to let the user know.
  • I'm open to discussion on that, but I am generally not in favor of the log functions actually erroring out and not returning, unless there's a really good reason to.
• bbi-2
  • You both note that this may not be necessary, if we do bbi-1, which is true. However...
  • Kyle's point here seems like a good one: "bbi already has to determine what nm version to use, so it could record that rather than make bbr try to figure it out"
  • It just seems obvious to me that bbi should capture and record the version of NONMEM that was actually used to run the model, instead of just passing through a config that needs to be re-parsed downstream to figure it out. I think this should be unambiguous in the bbi_config.json. Thoughts on that?
• bbr-1
  • I like the idea here, but I think it's lowest priority because it mostly becomes unnecessary once we do bbi-1
  • As noted though, it may still be worth doing because of differing versions floating around.