Open callistosp opened 3 months ago
[ 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:
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.)
Adjust resolve_nonmem_version()
to abort if more than one default is found.
On bbi's end:
it should abort if more than one default is found
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.
@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 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.
ahh good point about potentially different versions of bbr/bbi, that makes sense why you would want to implement in both.
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
mysteriously and silently pick underneath.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.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?
I have received the dreaded message:
What I have surmised is that because the 6 models were all run in both NM 7.4 and NM 7.5, when it tries to create the config log it sees duplicate rows based on the key only being defined by the absolute model path. I'm not sure how both versions of run results are showing up because the run_log itself only has one row per model.