v1alpha2 api server implementation #456
Conversation
Signed-off-by: YujiOshima <yuji.oshima0x3fd@gmail.com>
|
I will add some tests. |
cmd/manager/v1alpha2/main.go
Outdated
| if in.AlgorithmName == "" { | ||
| return &api_pb.GetSuggestionsReply{Trials: []*api_pb.Trial{}}, errors.New("No algorithm name is specified") | ||
| } | ||
| conn, err := grpc.Dial("vizier-suggestion-"+in.AlgorithmName+":6789", grpc.WithInsecure()) |
There was a problem hiding this comment.
One question: should we still use vizier-* names internally or should we start renaming them to katib? See #136.
There was a problem hiding this comment.
i think, we should start renaming to katib. i had renamed studyjobcontroller to katib-controller
There was a problem hiding this comment.
agree. I will change them.
| string start_time = 2; ///The start of the time range. RFC3339 format | ||
| string end_time = 3; ///The end of the time range. RFC3339 format | ||
| string filter = 4; | ||
| string filter = 2; |
There was a problem hiding this comment.
what does filter look like? I don't find it is used in GetTrialList()
There was a problem hiding this comment.
Sorry, I forgot it. I will add it to GetTrialList.
There was a problem hiding this comment.
Since the filter is used to filter trial name, can we rename it to filter_by_name? Also in db function, please handle filter == "" case.
There was a problem hiding this comment.
Thanks! updated.
cmd/manager/v1alpha2/main.go
Outdated
| if in.AlgorithmName == "" { | ||
| return &api_pb.ValidateAlgorithmSettingsReply{}, errors.New("No algorithm name is specified") | ||
| } | ||
| conn, err := grpc.Dial("vizier-suggestion-"+in.AlgorithmName+":6789", grpc.WithInsecure()) |
There was a problem hiding this comment.
we can add a function like getSuggestionServiceUrl(name String) string or even a function like getSuggestionServiceConnection(name String)
Signed-off-by: YujiOshima <yuji.oshima0x3fd@gmail.com>
YujiOshima
changed the title
|
I applied comments and added tests. |
|
cc @anchovYu PTAL, too |
Signed-off-by: YujiOshima <yuji.oshima0x3fd@gmail.com>
|
/lgtm |
cmd/manager/v1alpha2/main.go
Outdated
| return &resp, fmt.Errorf("grpc.health.v1.Health can only be accepted if you specify service name.") | ||
| } | ||
|
|
||
| // Check if connection to vizier-db is okay since otherwise manager could not serve most of its methods. |
There was a problem hiding this comment.
Update this comment as well (vizier -> katib)
pkg/api/v1alpha2/api.proto
Outdated
| string start_time = 2; ///The start of the time range. RFC3339 format | ||
| string end_time = 3; ///The end of the time range. RFC3339 format | ||
| string filter = 4; | ||
| string filter_by_name = 2; |
There was a problem hiding this comment.
I actually prefer "filter", because semantically it allows us to do things like:
- filter:name=foo
- filter:start_time>x
- filter:end_time<=y
We probably will only support filtering by name for now, but I can see a use case for the others. What do you think?
There was a problem hiding this comment.
Make sense. A general filter will really help. So we need use d.db.Query("SELECT * FROM trials WHERE experiment_name = ? AND ?, experimentName, filter) instead, not limited in name filed filter. So when update, please comment what filter looks like.
There was a problem hiding this comment.
@richardsliu @hougangliu Thanks. I revert to filter and add comments.
| @@ -29,7 +29,6 @@ echo "Generating v1alpha1 VizierDBInterface..." | |||
| mockgen -package mock -destination pkg/mock/v1alpha1/db/db.go github.com/kubeflow/katib/pkg/db/v1alpha1 VizierDBInterface | |||
Signed-off-by: YujiOshima <yuji.oshima0x3fd@gmail.com>
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hougangliu 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 |
Signed-off-by: YujiOshima yuji.oshima0x3fd@gmail.com
This change is