Reconcile semantics for Suggestion Algorithms #1633
Conversation
|
/retest |
|
/test |
|
@johnugeorge: The
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test kubeflow-katib-presubmit |
1 similar comment
|
/test kubeflow-katib-presubmit |
|
/hold cancel |
| logger.Info("Getting suggestions", "endpoint", endpoint, "response", len(responseSuggestion.ParameterAssignments), | ||
| "requestNum", requestNum) |
There was a problem hiding this comment.
Make it more clear here ?
| logger.Info("Getting suggestions", "endpoint", endpoint, "response", len(responseSuggestion.ParameterAssignments), | |
| "requestNum", requestNum) | |
| logger.Info("Getting suggestions", "endpoint", endpoint, "number of response parameters", len(responseSuggestion.ParameterAssignments), | |
| "number of request parameters", requestNum) |
| if len(responseSuggestion.ParameterAssignments) != requestNum { | ||
| err := fmt.Errorf("The response contains unexpected trials") | ||
| logger.Error(err, "The response contains unexpected trials", "requestNum", requestNum, "response", responseSuggestion) | ||
| logger.Error(err, "The response contains unexpected trials", "requestNum", requestNum, "response", len(responseSuggestion.ParameterAssignments)) |
There was a problem hiding this comment.
| logger.Error(err, "The response contains unexpected trials", "requestNum", requestNum, "response", len(responseSuggestion.ParameterAssignments)) | |
| logger.Error(err, "The response contains unexpected trials", "number of request parameters", requestNum, "number of response parameters", len(responseSuggestion.ParameterAssignments)) |
| @@ -6,10 +6,11 @@ package mock | |||
|
|
|||
| import ( | |||
| context "context" | |||
| reflect "reflect" | |||
There was a problem hiding this comment.
We might need to define specific version for mockgen to have same generation files.
Do we need these changes in the PR ?
| new_actual_requested_no = total_request_number - len(self.created_trials) | ||
| prev_generated_no = request_number - new_actual_requested_no | ||
| logger.info("In this call, New {} Trials will be generated, {} Trials will be reused from previously generated".format(new_actual_requested_no, prev_generated_no)) |
There was a problem hiding this comment.
In the first call, new_actual_requested_no = 0 and prev_generated_no = 3, when request_number = 3.
Is that correct ?
There was a problem hiding this comment.
It is the other way around.
In the normal case,
total_request_number == len(self.created_trials) + request_number where self.created_trials correspond to the number of previously created trials. So, prev_generated_no will be 0 in this case
When there is a difference, it means that some of the suggestions in self.created_trials(same as in DB) are not recorded in K8s Suggestions resource. So, prev_generated_no will be greater than 0 in this case
| if total_request_number != len(self.created_trials) + request_number: | ||
| logger.info("Mismatch in generated trials with k8s suggestions trials") |
|
/hold for the testing |
|
/lgtm |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich, 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 |
|
/retest |
|
/lgtm |
* Reuse suggestions * Fix tests
Currently, GetSuggestions call does not follow Kubernetes reconcile semantics. eg: If suggestion controller cannot update the suggestions returned from GetSuggestions call(from suggestion algorithm service), new suggestions are created again during the next try. This causes few suggestions to be leaked out.
In this PR, new variable is passed in the GetSuggestions Call which indicates the total Suggestions requested till date. If there are more trials in DB which are not recorded, it reuses the missed suggestions from DB while remaining required number is generated. So, GetSuggestions will ensure that missed suggestions are reused first before generating new ones.
Fixes #1534
/hold