Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[GSOC]
hyperopt
suggestion service logic update #2412base: master
Are you sure you want to change the base?
[GSOC]
hyperopt
suggestion service logic update #2412Changes from all commits
f615e3f
a8bc887
a67f373
365c2f5
caa2422
0f38a51
ae9fa34
910a46c
08b01ac
16dc030
282f81d
b7d09a6
58ab1ac
2b1932e
2f1c355
8391c29
23fd30b
7f6deb5
b85b4bf
dc36303
658daaf
5198ad1
262912d
81f5526
748e4ba
14f30a5
4f35663
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
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 set the uniform distribution as a default one ?
We can mutate the Uniform distribution in the Experiment defaulter: https://github.com/kubeflow/katib/blob/master/pkg/apis/controller/experiments/v1beta1/experiment_defaults.go
For example, in Optuna the Uniform distribution will be removed in favour of floatDistribution: https://optuna.readthedocs.io/en/stable/reference/generated/optuna.distributions.UniformDistribution.html
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 followed this article to determine the value of
sigma
frommin
andmax
.cc @tenzen-y @andreyvelich
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.
Maybe we should add this article to the comments. WDYT @tenzen-y @johnugeorge ?
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 do not want to depend on the individual article. Instead of that, it would be better to add an actual mathematical description here as a comment.
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.
Shouldn't we use the fixed value when the min and max are scalers the same as Nevergrad, right?
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.
Yeah we can, but that was an edge case when
min
andmax
are not defined in case of nevergrad.For example,
The above parameter will be sampled out from this graph:
where
u=3.8123
andsigma=0.3465
are calculated by puttingmin=32
andmax=64
in our code. andE(X)
represents the mean which is48
in our case.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.
That makes sense.
In that case, could you address the cases where min or max is not specified, as well as nevergrad?
https://github.com/facebookresearch/nevergrad/blob/a2006e50b068fe598e0f3d7dab9c9bcf6cf97e00/nevergrad/optimization/externalbo.py#L61-L64
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.
@shashank-iitbhu This is still pending.
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.
katib/pkg/webhook/v1beta1/experiment/validator/validator.go
Lines 287 to 290 in 867c40a
The webhook validator requires
feasibleSpace.max
orfeasibleSpace.min
to be specified.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.
But when either min or max is empty, this validation does not reject the request, right?
So, shouldn't we implement the special case in the Suggestion Service?
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, the validation webhook does not reject the request when either min or max is empty. But I created an example where:
For this, the experiment is being created but the suggestion service is not sampling out any value hence the trials are not running, though handled this case (when either min or max are not specified) in
pkg/suggestion/v1beta1/hyperopt/base_service.py
.Do we need to check
experiment_defaults.go
file?https://github.com/kubeflow/katib/blob/867c40a1b0669446c774cd6e770a5b7bbf1eb2f1/pkg/apis/controller/experiments/v1beta1/experiment_defaults.go