-
Notifications
You must be signed in to change notification settings - Fork 441
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
#2412
base: master
Are you sure you want to change the base?
[GSOC] hyperopt
suggestion service logic update
#2412
Conversation
Signed-off-by: Shashank Mittal <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/area gsoc |
Signed-off-by: Shashank Mittal <[email protected]>
Signed-off-by: Shashank Mittal <[email protected]>
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 @shashank-iitbhu!
I left a few comments
- "file-metrics-collector,pytorchjob-mnist" | ||
- "median-stop-with-json-format,file-metrics-collector-with-json-format" | ||
- "median-stop-with-json-format,file-metrics-collector-with-json-format" |
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.
Please add new line.
pkg/apis/manager/v1beta1/api.proto
Outdated
NORMAL = 2; | ||
LOG_NORMAL = 3; | ||
DISTRIBUTION_UNKNOWN = 4; | ||
DISTRIBUTION_UNKNOWN = 0; |
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.
Please keep the same name as for parameter_type
DISTRIBUTION_UNKNOWN = 0; | |
UNKNOWN_DISTRIBUTION = 0; |
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.
DISTRIBUTION_UNKNOWN = 0; | |
DISTRIBUTION _UNSPECIFIED = 0; |
I would like to select the UNSPECIFIED
suffix here.
Please see: https://google.aip.dev/126
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.
Make sense, @tenzen-y should we rename other gRPC parameters to UNSPECIFIED ?
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.
Changing released gRPC, it indicates losing backward compatibility.
So, I would like to keep using the existing API for released protocolbuffers API, @andreyvelich WDYT?
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.
Since these gRPC APIs are not exposed to the end-users, do you still think that we should not change the existing APIs ?
It only affects users who build their own 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.
Since these gRPC APIs are not exposed to the end-users, do you still think that we should not change the existing APIs ?
Almost correct. Additionally, when users keep using the removed Suggestion Services like the Chocolate Suggestion, users face the same problem.
So, can we collect feedback on the dedicated issue outside of here?
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.
Sure, let's followup on this in the issue, and rename it after few months if we don't get any feedback.
@shashank-iitbhu Please can you create an issue to track it ?
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.
SGTM
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.
Sure, let's followup on this in the issue, and rename it after few months if we don't get any feedback. @shashank-iitbhu Please can you create an issue to track it ?
Sure, I will create a separate issue to track the renaming of other gRPC parameters to UNSPECIFIED.
@@ -533,14 +533,6 @@ func convertParameterType(typ experimentsv1beta1.ParameterType) suggestionapi.Pa | |||
|
|||
func convertFeasibleSpace(fs experimentsv1beta1.FeasibleSpace) *suggestionapi.FeasibleSpace { | |||
distribution := convertDistribution(fs.Distribution) |
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.
Since convertDistribution
doesn't return error, I think you can simple add this line in return statement:
return &suggestionapi.FeasibleSpace{
Max: fs.Max,
Min: fs.Min,
List: fs.List,
Step: fs.Step,
Distribution: convertDistribution(fs.Distribution),
}
name="param-5", | ||
parameter_type=api_pb2.DOUBLE, | ||
feasible_space=api_pb2.FeasibleSpace( | ||
max="5", min="1", list=[], step="0.5", distribution=api_pb2.LOG_UNIFORM) |
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.
Please add more tests cases for other hyperopt distributions.
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.
done
) | ||
elif param.type == DOUBLE: | ||
hyperopt_search_space[param.name] = hyperopt.hp.uniform( | ||
hyperopt_search_space[param.name] = hyperopt.hp.uniformint( |
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.
If parameter is int, why we can't support other distributions like lognormal ?
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.
Distributions like uniform
quniform
loguniform
normal
etc return float values. They are designed to sample from a range of values that can take any real number (float), which might not make sense if we're looking for an integer value.
Although we can definitely add support for these distributions when parameter is int
also. Should we do this?
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.
@tenzen-y @kubeflow/wg-training-leads @shashank-iitbhu Should we round this float
value to int
if user wants to use this distribution and int parameter type ?
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.
@tenzen-y @kubeflow/wg-training-leads @shashank-iitbhu Should we round this float value to int if user wants to use this distribution and int parameter type ?
SGTM
Users can specify the double parameter type if they want to compute more exactly.
But, documentation of this restriction for int parameter type would be better.
else: | ||
hyperopt_search_space[param.name] = hyperopt.hp.uniform( | ||
param.name, float(param.min), float(param.max) | ||
) |
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 think, we can simplify this if statement by checking if distribution==UNIFORM or UNKNOWN
and step is null
, we use hyperopt.hp.uniform()
.
1a7a831
to
fddb763
Compare
Signed-off-by: Shashank Mittal <[email protected]> validation fix add e2e tests for hyperopt added e2e test to workflow
Signed-off-by: Shashank Mittal <[email protected]>
Signed-off-by: Shashank Mittal <[email protected]>
Signed-off-by: Shashank Mittal <[email protected]>
Signed-off-by: Shashank Mittal <[email protected]>
Signed-off-by: Shashank Mittal <[email protected]>
Signed-off-by: Shashank Mittal <[email protected]> sigma calculation fixed fix parse new arguments to mnist.py
fddb763
to
282f81d
Compare
) | ||
elif param.distribution == api_pb2.NORMAL: | ||
mu = (float(param.min) + float(param.max)) / 2 | ||
sigma = (float(param.max) - float(param.min)) / 6 |
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
from min
and max
.
cc @tenzen-y @andreyvelich
Signed-off-by: Shashank Mittal <[email protected]>
) | ||
elif param.distribution == api_pb2.NORMAL: | ||
mu = (float(param.min) + float(param.max)) / 2 | ||
sigma = (float(param.max) - float(param.min)) / 6 |
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.
sigma = (float(param.max) - float(param.min)) / 6 | |
// We consider the normal distribution based on the range of ±3 sigma. | |
sigma = (float(param.max) - float(param.min)) / 6 |
@tenzen-y I have added two new parameters, |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #2374
Checklist: