refac: use KServe types for InferenceServices #140
Conversation
| sigs.k8s.io/controller-runtime v0.10.2 | ||
| sigs.k8s.io/yaml v1.2.0 |
There was a problem hiding this comment.
Just FYI I was actually was preparing another PR to refresh dependencies (these and bunch of others). Fine to keep these here though.
| TargetModelState: v1beta1.ModelState(predictor.Status.TargetModelState), | ||
| } | ||
| inferenceService.Status.ModelStatus.ModelCopies = &v1beta1.ModelCopies{ | ||
| FailedCopies: predictor.Status.FailedCopies, |
There was a problem hiding this comment.
We should propagate the TotalCopies value now too, can we also add this to Predictor status?
There was a problem hiding this comment.
Yea, I planned on doing this in a separate PR since it'd require changes to the PredictorStatus spec (i.e. re-enabling LoadedCopies). Is LoadingCopies something that can just be removed from the code comments?
There was a problem hiding this comment.
Well I was still hoping to support LoadingCopies at some point but to implement in a reasonable way requires some nontrivial rework of modelmesh internals, so will stay as a "possible future addition" for now.
| inferenceService.Status.SetCondition(v1beta1.IngressReady, &apis.Condition{ | ||
| Type: v1beta1.IngressReady, | ||
| Status: corev1.ConditionTrue, | ||
| }) |
There was a problem hiding this comment.
What's the thinking behind this? We don't currently configure an ingress in the MM case right?
There was a problem hiding this comment.
Hah, this tripped me up for a while as I was initially trying to set the Ready condition to True directly. In KServe, the Ready condition for an ISvc is dependent on a list of conditions mentioned here with IngressReady being one of those conditions. So, every time I call InferenceService.Status.SetCondition, if the prerequisite conditions are True, then Ready is set to True.
But I can probably find an alternative way to set the PredictorReady and Ready conditions without going through the SetCondition function. I agree that the IngressReady condition doesn't make much sense in ModelMesh.
Yea, planned on handling ServingRuntime transitioning into a dedicated PR. |
With the InferenceService status now supporting the needed ModelMesh status fields, we can now use the type definitions from KServe instead of defining duplicates here. This commit imports the kserve go package and uses the types defined there for InferenceServices and their related fields. Signed-off-by: Paul Van Eck <[email protected]>
|
Had to update the FVT GH Actions workflow to free up disk space since we were running into space issues. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: njhill, pvaneck The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
Motivation
With the InferenceService status now supporting the needed ModelMesh status fields, we can now use the type definitions from KServe instead of defining duplicate types here.
Modifications
This commit imports the kserve go package and uses the types defined there for InferenceServices and their related fields.
Old InferenceService type definitions are removed.
Result
InferenceService types are now only defined in one place (i.e. the kserve repo)
Ref: #99
Signed-off-by: Paul Van Eck [email protected]