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.

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

Implement validation for early stopping #1709

Conversation

tenzen-y
Copy link
Member

@tenzen-y tenzen-y commented Oct 10, 2021

What this PR does / why we need it:

Users can not find the wrong algorithm name until to start trial Pod since there are no validating webhooks for early stopping. So, implement validating webhooks for spec.earlyStopping.algorithmName.
Also, I implement gRPC API to verify parameters for early stopping so that it can verify parameters.
In addition, I fix some documentations.

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 #1680

/assign
/assign @andreyvelich @gaocegege @johnugeorge

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thank you so much for driving this @tenzen-y!
I left my questions.

README.md Show resolved Hide resolved
manifests/v1beta1/components/controller/katib-config.yaml Outdated Show resolved Hide resolved
@tenzen-y tenzen-y changed the title Implement validating webhooks for early stopping [WIP] Implement validation for early stopping Oct 26, 2021
@tenzen-y tenzen-y force-pushed the issue-1680-implement-validating-webhook-for-early-stopping branch from cfc5f62 to d98467c Compare October 27, 2021 18:42
@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tenzen-y
To complete the pull request process, please ask for approval from andreyvelich after the PR has been reviewed.

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

@tenzen-y tenzen-y force-pushed the issue-1680-implement-validating-webhook-for-early-stopping branch 3 times, most recently from 28324da to 171d37e Compare October 27, 2021 18:50
@coveralls
Copy link

coveralls commented Oct 27, 2021

Coverage Status

Coverage increased (+0.5%) to 74.214% when pulling 6f231e8 on tenzen-y:issue-1680-implement-validating-webhook-for-early-stopping into cffab07 on kubeflow:master.

@google-oss-prow google-oss-prow bot added size/XXL and removed size/L labels Nov 8, 2021
@tenzen-y tenzen-y force-pushed the issue-1680-implement-validating-webhook-for-early-stopping branch 3 times, most recently from 9eb4ab0 to c04704c Compare November 8, 2021 09:58
@tenzen-y tenzen-y force-pushed the issue-1680-implement-validating-webhook-for-early-stopping branch from c04704c to 0e35f07 Compare November 8, 2021 10:14
@tenzen-y tenzen-y changed the title [WIP] Implement validation for early stopping Implement validation for early stopping Nov 8, 2021
@tenzen-y
Copy link
Member Author

tenzen-y commented Nov 9, 2021

Can you comment with @googlebot I consent., please? @andreyvelich

@google-cla
Copy link

google-cla bot commented Nov 9, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Co-authored-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
@google-cla
Copy link

google-cla bot commented Nov 9, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Nov 9, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@andreyvelich
Copy link
Member

@googlebot I consent.

@tenzen-y tenzen-y force-pushed the issue-1680-implement-validating-webhook-for-early-stopping branch 2 times, most recently from 564e93a to 8ca5b26 Compare November 9, 2021 22:24
@tenzen-y tenzen-y force-pushed the issue-1680-implement-validating-webhook-for-early-stopping branch from 8ca5b26 to cae15c6 Compare November 9, 2021 22:26
@tenzen-y
Copy link
Member Author

tenzen-y commented Nov 9, 2021

I addressed all your suggestions.
Can you re-check the changes? @andreyvelich

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thank you for the updates @tenzen-y!
/lgtm
/assign @gaocegege @johnugeorge

@google-oss-prow google-oss-prow bot added the lgtm label Nov 10, 2021
@tenzen-y
Copy link
Member Author

Thanks for the review! @andreyvelich
According to this documentation, I'll create PR to update API documentation in kubeflow/website repository.

@andreyvelich
Copy link
Member

Thanks for the review! @andreyvelich According to this documentation, I'll create PR to update API documentation in kubeflow/website repository.

I think API Reference page is outdated.
@tenzen-y @gaocegege @johnugeorge Do we want to maintain this API doc on the Kubeflow website ?
Since we have similar API doc here, maybe we can just add the link to this doc in Katib Interfaces section.

@tenzen-y
Copy link
Member Author

Thanks for the review! @andreyvelich According to this documentation, I'll create PR to update API documentation in kubeflow/website repository.

I think API Reference page is outdated. @tenzen-y @gaocegege @johnugeorge Do we want to maintain this API doc on the Kubeflow website ? Since we have similar API doc here, maybe we can just add the link to this doc in Katib Interfaces section.

Sounds good to me.

@gaocegege
Copy link
Member

SGTM!

Thanks for the review! @andreyvelich According to this documentation, I'll create PR to update API documentation in kubeflow/website repository.

I think API Reference page is outdated. @tenzen-y @gaocegege @johnugeorge Do we want to maintain this API doc on the Kubeflow website ? Since we have similar API doc here, maybe we can just add the link to this doc in Katib Interfaces section.

@google-oss-prow google-oss-prow bot removed the lgtm label Nov 11, 2021
@tenzen-y
Copy link
Member Author

Removed description about updating gRPC API docs in kubeflow website. @andreyvelich

@andreyvelich
Copy link
Member

Thanks @tenzen-y!
/lgtm
/assign @gaocegege @johnugeorge

@google-oss-prow google-oss-prow bot added the lgtm label Nov 11, 2021
@tenzen-y
Copy link
Member Author

Could you review this PR? @gaocegege @johnugeorge

@johnugeorge
Copy link
Member

Sorry for delay
/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnugeorge, 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

@google-oss-prow google-oss-prow bot merged commit 10051dc into kubeflow:master Nov 26, 2021
@tenzen-y tenzen-y deleted the issue-1680-implement-validating-webhook-for-early-stopping branch November 26, 2021 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add validating webhooks for early stopping experiment
6 participants