-
Notifications
You must be signed in to change notification settings - Fork 448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for grid search algorithm in Optuna Suggestion Service #2060
Support for grid search algorithm in Optuna Suggestion Service #2060
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tenzen-y 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 |
04b7d02
to
31b2d82
Compare
6ba7852
to
9990aca
Compare
/hold for the review /assign @andreyvelich @johnugeorge @anencore94 |
assert code == grpc.StatusCode.INVALID_ARGUMENT | ||
|
||
# [RANDOM] Invalid random_state | ||
@pytest.mark.parametrize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, we would be good to do refactoring unit tests using @pytest.mark.parametrize
in other suggestion services, same as this.
We can follow up with other PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this feature of PyTest, thanks!
if max_trial_count > num_combinations: | ||
return False, "Max Trial Count: {max_trial} > all possible search combinations: {combinations}".\ | ||
format(max_trial=max_trial_count, combinations=num_combinations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with optuna, Is it ok to set max_trial_count < num_combinations
? since with the documentation GridSampler looks like always trying to search all possible combinations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right about optuna behavior. Although optuna suggestion-service creates trials using already used parameters after all combinations used.
katib/pkg/suggestion/v1beta1/optuna/base_service.py
Lines 58 to 65 in db72ce1
for _ in range(current_request_number): | |
optuna_trial = self.study.ask(fixed_distributions=self._get_optuna_search_space()) | |
assignments = [Assignment(k, v) for k, v in optuna_trial.params.items()] | |
list_of_assignments.append(assignments) | |
assignments_key = self._get_assignments_key(assignments) | |
self.assignments_to_optuna_number[assignments_key].append(optuna_trial.number) |
This does not mean the optuna suggestion service can remove duplicated suggestions in the above section since the suggestionclient faces the error in the following:
katib/pkg/controller.v1beta1/suggestion/suggestionclient/suggestionclient.go
Lines 123 to 128 in db72ce1
logger.Info("Getting suggestions", "endpoint", endpoint, "Number of current request parameters", currentRequestNum, "Number of response parameters", len(responseSuggestion.ParameterAssignments)) | |
if len(responseSuggestion.ParameterAssignments) != currentRequestNum { | |
err := fmt.Errorf("The response contains unexpected trials") | |
logger.Error(err, "The response contains unexpected trials") | |
return err | |
} |
Also, if users want to run experiments using all combinations, users can skip setting maxTrialCount
in the Experiment.
For these reasons, it would be good to provide that validation.
@anencore94 Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap I see now. Thanks for your kind reply :)
2f94b40
to
f661f59
Compare
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this effort @tenzen-y!
We should also update the docs:
combinations[parameter.name] = range(int(parameter.min), int(parameter.max)+1, int(parameter.step)) | ||
elif parameter.type == DOUBLE: | ||
if parameter.step == "" or parameter.step is None: | ||
raise Exception("Param {} step is nil".format(parameter.name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also say that: "For discrete search space all parameters must include step" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. It helps to debug parameters
.
assert code == grpc.StatusCode.INVALID_ARGUMENT | ||
|
||
# [RANDOM] Invalid random_state | ||
@pytest.mark.parametrize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this feature of PyTest, thanks!
e7ac972
to
2cb4d86
Compare
@andreyvelich Thanks for the review and pointing out sections we need to update the documentation! |
raise Exception( | ||
"Param {} step is nil; For discrete search space, all parameters must include step". | ||
format(parameter.name) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreyvelich I updated this error message.
@andreyvelich I addressed your comments; please take another look. |
Thank you @tenzen-y! We are going to remove Chocolate references in the following PRs, right ? |
Yes, I will create another PR to remove all codes for Chocolate after this PR is merged. |
Blocked by: #2070 |
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
2cb4d86
to
339e67f
Compare
@johnugeorge @anencore94 I rebased to take #2070. PTAL. |
/lgtm Thanks @tenzen-y |
Thanks for all reviews and comments. |
Signed-off-by: Yuki Iwai yuki.iwai.tz@gmail.com
What this PR does / why we need it:
I added a mechanism to support grid search in Optuna Suggestion Service since we will remove Chocolate Suggestion Service as mentioned in #2058.
Also, I switched the default Suggestion Service for grid search to Optuna Suggestion Service in katib-config.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Part of #2058
Checklist: