Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Modify gRPC API with Current Request Number #1728

Merged
merged 4 commits into from
Nov 5, 2021
Merged

Modify gRPC API with Current Request Number #1728

merged 4 commits into from
Nov 5, 2021

Conversation

andreyvelich
Copy link
Member

Fixes: #1637.

I changed request_number to current_request_number to be consistent.

/hold for the review

Please take a look.

/assign @gaocegege @johnugeorge @tenzen-y @anencore94

@anencore94
Copy link
Member

Maybe it' ok to do not change the original proposal here ?

anencore94
anencore94 reviewed Nov 3, 2021
View reviewed changes
Copy link
Member

@anencore94 anencore94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM !

pkg/suggestion/v1beta1/hyperband/service.py Outdated
@@ -48,7 +48,7 @@ def GetSuggestions(self, request, context):
# Hyperband outlerloop has finished
return reply
# This is a hack to get request number.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: how about change this comment to add the word current ?

pkg/suggestion/v1beta1/skopt/base_service.py Outdated
for suggestion in skopt_suggested:
logger.info("New suggested parameters for Trial: {}".format(suggestion))
return_trial_list.append(
BaseSkoptService.convert(self.search_space, suggestion))

logger.info("GetSuggestions return {} new Trials\n\n".format(request_number))
logger.info("GetSuggestions returns {} new Trials\n\n".format(current_request_number))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it's not about this PR directly, but isn't this log should be len(skopt_suggested) or len(return_trial_list) rather than current_request_number ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

pkg/controller.v1beta1/suggestion/suggestionclient/suggestionclient.go Outdated
}

// Get new suggestions
responseSuggestion, err := rpcClientSuggestion.GetSuggestions(ctx, requestSuggestion)
if err != nil {
return err
}
logger.Info("Getting suggestions", "endpoint", endpoint, "Number of request parameters", requestNum, "Number of response parameters", len(responseSuggestion.ParameterAssignments))
if len(responseSuggestion.ParameterAssignments) != requestNum {
logger.Info("Getting suggestions", "endpoint", endpoint, "Number of request parameters", currentRequestNum, "Number of response parameters", len(responseSuggestion.ParameterAssignments))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Number of request parameters -> Number of current request parameters

gaocegege
gaocegege reviewed Nov 3, 2021
View reviewed changes
Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Thanks!

tenzen-y
tenzen-y approved these changes Nov 3, 2021
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Nov 3, 2021
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, tenzen-y

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@andreyvelich
Copy link
Member Author

Maybe it' ok to do not change the original proposal here ?

I think we decide to change API name to be consistent with total_request_number.
In original proposal we don't have it.

Maybe we can keep request_number gRPC API as deprecated for 1 release (until Katib 0.14 will be released) ?
Some Katib users might deploy their own custom Algorithm service where they are using this API.

For our Suggestions we can rename request_number to current_request_number in this PR.

WDYT @gaocegege @anencore94 @johnugeorge ?

@google-oss-prow google-oss-prow bot removed the lgtm label Nov 3, 2021
@gaocegege
Copy link
Member

Maybe we can keep request_number gRPC API as deprecated for 1 release (until Katib 0.14 will be released) ?
Some Katib users might deploy their own custom Algorithm service where they are using this API.

SGTM

@anencore94
Copy link
Member

Maybe we can keep request_number gRPC API as deprecated for 1 release (until Katib 0.14 will be released) ?
Some Katib users might deploy their own custom Algorithm service where they are using this API.

As we discussed last working group meeting, I also agree with this suggestion. What's left now is to decide the way to notify the deprecation of this API. Is it enough to announce on Katib v0.13's Release note and here

@andreyvelich
Copy link
Member Author

As we discussed, I left request_number with [deprecated=true] parameter in gRPC API. We will also leave a note in the Katib 0.13 release notes about it.
As I can see, some of the Kubernetes API also follows this: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/cri-api/pkg/apis/runtime/v1alpha2/api.proto#L298.

Please take a look.

@tenzen-y
Copy link
Member

tenzen-y commented Nov 4, 2021

I left request_number with [deprecated=true] parameter in gRPC API. We will also leave a note in the Katib 0.13 release notes about it.

SGTM

@anencore94
Copy link
Member

As we discussed, I left request_number with [deprecated=true] parameter in gRPC API. We will also leave a note in the Katib 0.13 release notes about it.
As I can see, some of the Kubernetes API also follows this: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/cri-api/pkg/apis/runtime/v1alpha2/api.proto#L298.

It make sense :) Thanks for sharing @andreyvelich
/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Nov 5, 2021
@andreyvelich
Copy link
Member Author

Thanks everyone for the review!
/hold cancel

@google-oss-robot google-oss-robot merged commit 16e0574 into kubeflow:master Nov 5, 2021
@andreyvelich andreyvelich deleted the issue-1637-rename-request-number branch November 5, 2021 01:07
@tenzen-y tenzen-y mentioned this pull request Jul 19, 2022
Closed
@tenzen-y tenzen-y mentioned this pull request Nov 2, 2022
Closed
13 tasks
@johnugeorge johnugeorge mentioned this pull request Nov 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anencore94 anencore94 anencore94 left review comments

@gaocegege gaocegege gaocegege left review comments

@tenzen-y tenzen-y tenzen-y approved these changes

@johnugeorge johnugeorge Awaiting requested review from johnugeorge

@sperlingxx sperlingxx Awaiting requested review from sperlingxx

Assignees

@johnugeorge johnugeorge

@gaocegege gaocegege

@anencore94 anencore94

@tenzen-y tenzen-y

Labels
approved lgtm size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename request_number parameter in gRPC manager
6 participants
@andreyvelich @anencore94 @gaocegege @tenzen-y @johnugeorge @google-oss-robot