Fix Failed Loaded Modules being considered Ready #763
Conversation
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
0533fa5 to
37a3037
Compare
| @@ -118,7 +118,11 @@ def model(self) -> ModuleBase: | |||
| self.wait() | |||
| return self._model | |||
|
|
|||
| def loaded(self) -> bool: | |||
| def loaded(self, require_instance: bool = False) -> bool: | |||
There was a problem hiding this comment.
Why is the default False? I guess really, why make this an argument at all? It seems like the old logic was just wrong? Did you trace history to figure out why or self._caikit_model_future.done() was added?
There was a problem hiding this comment.
I didn't trace history but logically it seems loaded used to see if the ModelFuture was generally completed not just successfully completed. When I checked caikit general it doesn't seem like we use loaded outside the servicer so assumed it was used in a subclass/other implementation.
After looking through the history it seems like it is only used in the info servicer. I'm game to change this to the default
Co-authored-by: Gabe Goodhart <gabe.l.hart@gmail.com> Signed-off-by: Michael Honaker <37811263+HonakerM@users.noreply.github.com>
Co-authored-by: Gabe Goodhart <gabe.l.hart@gmail.com> Signed-off-by: Michael Honaker <37811263+HonakerM@users.noreply.github.com>
There was a problem hiding this comment.
Ok, yeah, I looked through history and it looks like I added this all in one line, so that second clause was not added for a specific reason. The rest of the code seems to clearly indicate that self._model is only ever set to self._caikit_model_future.result(), so I would say let's just remove that second clause and not have this argument.
Also, linting and passing tests!
Sounds good!
I just committed your changes 👀 |
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
🤦 This is why I still don't really trust suggestions in GH. You can't see how long the lines are! |
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
|
@gabe-l-hart it looks like a test is failing because it was using |
Hmmm. Yeah, that sounds like the right option. I'd suggest calling it |
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
What this PR does / why we need it:
There is a bug in Caikit when a model failed to load but its
LoadedModelinstance is still marked as "loaded" because theloaded()check only considers if the future finished not if the model is actually loaded. This PR fixes this by adding an additional parameterrequire_instancewhich ensures that a local instance is actually available.Special notes for your reviewer:
If applicable: