restructure algorithm configuration for hyperopt_service #1161
Conversation
|
This change is |
There was a problem hiding this comment.
Hi @sperlingxx! Thank you for adding validations 👍 I put some review comments.
| __schema_dict = { | ||
| 'tpe': { | ||
| 'gamma': (lambda x: float(x), lambda x: 1 > float(x) > 0), | ||
| 'prior_weight': (lambda x: float(x), lambda x: float(x) > 0), | ||
| 'n_EI_candidates': (lambda x: int(x), | ||
| lambda x: float(x).is_integer() and int(x) > 0), | ||
| "random_state": (lambda x: int(x), lambda x: x.isdigit()), | ||
| }, | ||
| "random": { | ||
| "random_state": (lambda x: int(x), lambda x: x.isdigit()) | ||
| } | ||
| } |
There was a problem hiding this comment.
I mind that validate functions ( __schema_dict[algo_name][s.name][1] ) cannot return the message why parameters are invalid.
There was a problem hiding this comment.
And I personally feels this dict is a bit tricky. I prefer the following code:
@classmethod
def _validate_tpe_setting(algorithm_settings):
for s in algorithm_settings:
if s == 'gamma' and 0 < float(x) < 1:
return False, "gamma should be in the range of [0, 1]"
...
return True, ""
@classmethod
def validate_algorithm_spec(cls, algorithm_spec):
algo_name = algorithm_spec.algorithm_name
if algo_name == "tpe":
return cls._validate_tpe_setting(algorithm_spec.algorithm_setting)
elif algo_name == "random":
return cls._validate_random_setting(algorithm_spec.algorithm_setting)
return False, "invalid algorithm name"But I want to hear opinions of maintainers ( @andreyvelich @gaocegege ).
There was a problem hiding this comment.
I'm okay with both code styles.
There was a problem hiding this comment.
I agree with @c-bata, I think for validation we should say why settings is not correct. Like we did with enas: https://github.com/kubeflow/katib/blob/master/pkg/suggestion/v1alpha3/nas/enas_service.py#L218
There was a problem hiding this comment.
@c-bata @andreyvelich I've refactored codes of validation part.
| if not (1 > float(s.value) > 0): | ||
| return False, "gamma should be in the range of (0, 1)" | ||
| elif s.name == 'prior_weight': | ||
| if not (float(s.value) > 0): | ||
| return False, "prior_weight should be great than zero" | ||
| elif s.name == 'n_EI_candidates': | ||
| if not (int(s.value) > 0): | ||
| return False, "n_EI_candidates should be great than zero" | ||
| elif s.name == 'random_state': | ||
| if not (int(s.value) >= 0): | ||
| return False, "random_state should be great or equal than zero" |
There was a problem hiding this comment.
[nits] I guess you can remove redundant brackets. It's OK to ignore this comment.
| if not (1 > float(s.value) > 0): | |
| return False, "gamma should be in the range of (0, 1)" | |
| elif s.name == 'prior_weight': | |
| if not (float(s.value) > 0): | |
| return False, "prior_weight should be great than zero" | |
| elif s.name == 'n_EI_candidates': | |
| if not (int(s.value) > 0): | |
| return False, "n_EI_candidates should be great than zero" | |
| elif s.name == 'random_state': | |
| if not (int(s.value) >= 0): | |
| return False, "random_state should be great or equal than zero" | |
| if not 1 > float(s.value) > 0: | |
| return False, "gamma should be in the range of (0, 1)" | |
| elif s.name == 'prior_weight': | |
| if not float(s.value) > 0: | |
| return False, "prior_weight should be great than zero" | |
| elif s.name == 'n_EI_candidates': | |
| if not int(s.value) > 0: | |
| return False, "n_EI_candidates should be great than zero" | |
| elif s.name == 'random_state': | |
| if not int(s.value) >= 0: | |
| return False, "random_state should be great or equal than zero" |
There was a problem hiding this comment.
I only found this commit suggestion can be applied directly after I pushed the same commit = =!
|
/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 |
sperlingxx
force-pushed
the
add_gamma_for_tpe
branch
from
ba85959
to
55d99d5
Compare
|
@gaocegege There exists a conflict with latest merged commit. And I've solved it. |
|
/lgtm |
* restructure configuration for hyperopt_service * refine * refine schema_dict of OptimizerConfiguration * transfer code style of validation * remove redundant brackets * format print style change
Restructuring algorithm configuration related codes for hyperopt_service, which makes it easier to modify algorithm schema in future.
@andreyvelich @gaocegege @c-bata