feat(experiment): Delete dup trials #647
Conversation
Signed-off-by: Ce Gao <[email protected]>
There was a problem hiding this comment.
/assign @johnugeorge
There are some bugs without the PR
Sometimes we will create dup trials, then unit test will fail.
|
/assign @hougangliu |
| return err | ||
| } | ||
| actualDeleteCount := int(deleteCount) | ||
| if int32(len(trials.Items)) < deleteCount { |
There was a problem hiding this comment.
when do this condition happen?
There was a problem hiding this comment.
No idea, just to make sure the program won't crash.
There was a problem hiding this comment.
I am not sure if it is needed. deleteCount is updated on the results of List operation. https://github.com/kubeflow/katib/blob/master/pkg/controller/v1alpha2/experiment/experiment_controller.go#L280
One question is if List operation returns stale entries. But this is not solved here too
There was a problem hiding this comment.
Because we do not use the trials from https://github.com/kubeflow/katib/blob/master/pkg/controller/v1alpha2/experiment/experiment_controller.go#L280 . Thus the result may be not consistent, IMO.
There was a problem hiding this comment.
This just to avoid IndexOutOfBoundsException
There was a problem hiding this comment.
What if you use the earlier trial.Items results than a new List operation?
There was a problem hiding this comment.
It works for me. I am working on it now.
Signed-off-by: Ce Gao <[email protected]>
|
Comments are addressed, please take a look. |
Signed-off-by: Ce Gao <[email protected]>
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johnugeorge 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 |
Signed-off-by: Ce Gao [email protected]
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Release note:
This change is