Validate possible operations for Grid suggestion #1205
Conversation
|
This change is |
|
/retest |
|
Hello @andreyvelich, Me and @StefanoFioravanzo have been tinkering with Katib for some time now and have a different proposal on tackling #1122. We will describe it in detail as a comment to that issue by the end of the week. Would you mind holding this PR until then and decide afterwards whether to merge this or not? Thanks in advance! |
|
Hi @elikatsis, sure. |
| @@ -4,12 +4,15 @@ | |||
| from pkg.apis.manager.v1beta1.python import api_pb2 | |||
| from pkg.apis.manager.v1beta1.python import api_pb2_grpc | |||
|
|
|||
| from pkg.suggestion.v1beta1.internal.constant import DOUBLE | |||
| from pkg.suggestion.v1beta1.internal.constant import (INTEGER, DOUBLE, CATEGORICAL, DISCRETE) | |||
There was a problem hiding this comment.
I am not good at py, why do we need () for it?
There was a problem hiding this comment.
I think we can remove it from here, it uses when we need to use more than 1 line for importing
I think so, would you mind opening an issue for it? @sperlingxx |
In skopt we use log-uniform distribution: https://github.com/kubeflow/katib/blob/master/pkg/suggestion/v1beta1/skopt/base_service.py#L44. |
Other packages used by katib also support multiple distributions:
So, maybe is it better to add an option to configure distribution of continuous search space in |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gaocegege 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 |
|
/hold |
|
I believe @elikatsis wanted to hold this PR until end of this week. |
|
Oh SGTM. Thanks for the hold in time. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
/lifecycle frozen |
|
@andreyvelich: The 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. |
Validate db exhausted for chocolate
andreyvelich
force-pushed
the
issue-1122-validate-chocolate-exhausted
branch
from
33a188e
to
9c89dcb
Compare
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich, gaocegege, sperlingxx 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 |
|
I think we can merge these changes for Grid suggestion. |
|
/retest |
|
SGTM! |
There was a problem hiding this comment.
moving call_validate function to utils looks better! Thanks :) @andreyvelich
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@andreyvelich: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/retest |
|
LGTM |
Fixes: #1122.
I added validation for Chocolate Grid suggestion to check if
max_trial_count>= all possible search space combination.Also, I moved
call_validate()test function to utils to use it in Chocolate also./assign @gaocegege @johnugeorge
/cc @sperlingxx