Support InferenceComponent CRD. #260
Conversation
suryans-commit
requested review from
surajkota and
ryansteakley
as code owners
|
/hold |
ack-prow
bot
added
the
do-not-merge/hold
|
/retest |
|
/unhold |
ack-prow
bot
removed
the
do-not-merge/hold
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
| assert deleted | ||
|
|
||
|
|
||
| @pytest.fixture(scope="module") |
There was a problem hiding this comment.
why is this a module wide fixture?
There was a problem hiding this comment.
We want to reuse the endpoint within the inference component test and thus it has a module wide scope, similar to all other components.
| var lastSpecForUpdateString *string = nil | ||
| // get last endpoint config name used for update from annotations | ||
| annotations := desired.ko.ObjectMeta.GetAnnotations() | ||
| for k, v := range annotations { |
There was a problem hiding this comment.
Why is a for loop needed for this?
There was a problem hiding this comment.
Do you mean, why loop instead of directly retrieving the value? I can make that change in the next release since functionally there is no change, and this release is time sensitive. Let me know if you think otherwise.
| // Update is blocked in the following cases: | ||
| // 1. while InferenceComponentStatus != InService (handled by requeueUntilCanModify method). | ||
| // 2. InferenceComponentStatus == Failed. | ||
| // 3. A previous update to the InferenceComponent with same spec failed. |
There was a problem hiding this comment.
Bit curious about this, if this isnt checked will ack keep performing reconciliation loops as desired != latest?
There was a problem hiding this comment.
Yes, that is the reason for checking this condition. Since InferenceComponent is a Blue/Green deployment if the Update fails it will rollback to last successful state with Status = InService. So ACK will re-trigger the update. After discussion with product it was decided to move the resource to terminal state if an update fails.
| // 2. desiredSpec == lastSpecForUpdate only tells us an update was tried with lastSpecForUpdate | ||
| // but does not tell us anything if the update was successful or not in the past because | ||
| // it is set if updateInferenceComponent returns 200 (async operation). | ||
| // 3. Now, sdkUpdate can execute because of change in any field in Spec (like tags/deploymentConfig in future) |
There was a problem hiding this comment.
nit: Based off line 65/105 tags are ignored in the spec comparison.
There was a problem hiding this comment.
Yes, since tags are ignored, that is why SDK tags will trigger an update.
| } | ||
|
|
||
| // EqualInferenceComponentSpec checks if two InferenceComponentSpec instances are equal | ||
| func EqualInferenceComponentSpec(desiredSpec *svcapitypes.InferenceComponentSpec, |
There was a problem hiding this comment.
Is the metadata like annotations also compared here or is it just the Spec's API shape?
There was a problem hiding this comment.
No metadata is not part of the InferenceComponentSpec struct.
| template_path: common/sdk_delete_pre_build_request.go.tpl | ||
| sdk_delete_post_request: | ||
| template_path: common/sdk_delete_post_request.go.tpl | ||
| fields: |
There was a problem hiding this comment.
I see that theres a CurrentCopyCount(https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/sagemaker/client/describe_inference_component.html), does that need to be included here as well.
There was a problem hiding this comment.
No, since that is not a read only field and can be updated by the customer.
|
looks good at a high level |
|
/retest |
| func EqualInferenceComponentSpec(desiredSpec *svcapitypes.InferenceComponentSpec, | ||
| lastSpec *svcapitypes.InferenceComponentSpec) bool { | ||
| if desiredSpec == nil || lastSpec == nil { | ||
| return desiredSpec == lastSpec |
There was a problem hiding this comment.
is this because .DeepEqual cannot handle nil?
There was a problem hiding this comment.
Yes, Reflect package does not handle nil pointers gracefully.
The reason for the nil check before using reflect.DeepEqual is to prevent a potential panic caused by attempting to dereference a nil pointer. If either desiredSpec or lastSpec is nil, attempting to pass them to reflect.DeepEqual directly would result in a runtime panic because reflect.DeepEqual expects both of its arguments to be non-nil.
|
overall looks good. Have you verified the status of the IC prints in the console when using kubectl? |
Yes, have verified creating IC via kubectl and the corresponding prints in the console. |
There was a problem hiding this comment.
Thank you @suryans-commit !
lgtm-ing as implementation is very similar to the existing resources.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly, suryans-commit 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 |
Description of changes:
InferenceComponentCRDBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.