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

Replace gRPC code generation tool from Znly/protoc to Buf #2344

Merged
merged 12 commits into from
Jun 15, 2024

Conversation

forsaken628
Copy link
Contributor

What this PR does / why we need it:
Replace gRPC code generation tool from Znly/protoc to Buf , and some fix

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

Checklist:

  • Docs included if any changes are user facing

Signed-off-by: forsaken628 <[email protected]>
Signed-off-by: forsaken628 <[email protected]>
Signed-off-by: forsaken628 <[email protected]>
Signed-off-by: forsaken628 <[email protected]>
Signed-off-by: forsaken628 <[email protected]>
Signed-off-by: forsaken628 <[email protected]>
@forsaken628
Copy link
Contributor Author

medianstop doesn't have any unit tests, and it doesn't look like there's any reason it has to be implemented in python. Can I re-implement it in go later and add unit tests?
@andreyvelich

Signed-off-by: forsaken628 <[email protected]>
@andreyvelich
Copy link
Member

medianstop doesn't have any unit tests, and it doesn't look like there's any reason it has to be implemented in python. Can I re-implement it in go later and add unit tests?

Actually, we have unit tests here: https://github.com/kubeflow/katib/blob/master/test/unit/v1beta1/earlystopping/test_medianstop_service.py

Any specific reason do you want to implement in Go, ?
We are trying to keep our optimization algorithms in Python since Data Science community are more familiar with it.

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 this great contribution @forsaken628!
I left a few comments.
@tenzen-y @johnugeorge Should we try to merge it before the release to unblock: #2346 ?

hack/update-codegen.sh Outdated Show resolved Hide resolved
hack/update-proto.sh Show resolved Hide resolved
buf.gen.yaml Outdated
@@ -0,0 +1,12 @@
version: v2
plugins:
- remote: buf.build/protocolbuffers/go:v1.33.0
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to keep buf config for Go packages outside of /apis/manager/v1beta1 directory ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm prefer global buf.gen.yaml, but there will be a lot of changes for python. Well, that's fine.

Comment on lines +4 to +12
root = os.path.join(os.path.dirname(__file__), "..")
path.extend(
[
os.path.join(root, "pkg/apis/manager/v1beta1/python"),
os.path.join(root, "pkg/apis/manager/health/python"),
os.path.join(root, "pkg/metricscollector/v1beta1/common"),
os.path.join(root, "pkg/metricscollector/v1beta1/tfevent-metricscollector"),
]
)
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for doing this!

@tenzen-y
Copy link
Member

Thank you for this great contribution @forsaken628! I left a few comments. @tenzen-y @johnugeorge Should we try to merge it before the release to unblock: #2346 ?

SGTM

Signed-off-by: forsaken628 <[email protected]>
Signed-off-by: forsaken628 <[email protected]>
@tenzen-y
Copy link
Member

@forsaken628 As I mentioned here, could you open a PR to implement the custom readiness check to resolve flake E2E tests?

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.

Thank you for this great contribution!
/approve

I leave lgtm on @andreyvelich

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

@forsaken628
Copy link
Contributor Author

@forsaken628 As I mentioned here, could you open a PR to implement the custom readiness check to resolve flake E2E tests?

if err := mgr.AddReadyzCheck("readyz", hookServer.StartedChecker()); err != nil {

There's already a webhook ready check added before manger start, and I'm wondering why it's still getting the
connection refused

@tenzen-y
Copy link
Member

tenzen-y commented Jun 15, 2024

@forsaken628 As I mentioned here, could you open a PR to implement the custom readiness check to resolve flake E2E tests?

if err := mgr.AddReadyzCheck("readyz", hookServer.StartedChecker()); err != nil {

There's already a webhook ready check added before manger start, and I'm wondering why it's still getting the connection refused

As I described in the comment, the default checker can not consider if the certs are ready.

https://github.com/kubeflow/training-operator/blob/9a9edecf146bb0c2cccaeb3bec912ab2dfa5d8c5/cmd/training-operator.v1/main.go#L230-L236

@andreyvelich
Copy link
Member

Thank you for doing this @forsaken628!
Please can you cherry-pick this PR to the release branch: release-0.17 ?

@andreyvelich
Copy link
Member

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Jun 15, 2024
@google-oss-prow google-oss-prow bot merged commit 0d190b9 into kubeflow:master Jun 15, 2024
60 checks passed
tariq-hasan pushed a commit to tariq-hasan/katib that referenced this pull request Jun 15, 2024
)

* Replace gRPC code generation tool from Znly/protoc to Buf

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* del build.sh

Signed-off-by: forsaken628 <[email protected]>

* cleanup

Signed-off-by: forsaken628 <[email protected]>

* fix test

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* refine

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* rm outter yaml

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

---------

Signed-off-by: forsaken628 <[email protected]>
@forsaken628 forsaken628 deleted the pb branch June 16, 2024 03:41
andreyvelich pushed a commit to andreyvelich/katib that referenced this pull request Jun 18, 2024
)

* Replace gRPC code generation tool from Znly/protoc to Buf

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* del build.sh

Signed-off-by: forsaken628 <[email protected]>

* cleanup

Signed-off-by: forsaken628 <[email protected]>

* fix test

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* refine

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* rm outter yaml

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

---------

Signed-off-by: forsaken628 <[email protected]>
andreyvelich pushed a commit to andreyvelich/katib that referenced this pull request Jun 18, 2024
)

* Replace gRPC code generation tool from Znly/protoc to Buf

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* del build.sh

Signed-off-by: forsaken628 <[email protected]>

* cleanup

Signed-off-by: forsaken628 <[email protected]>

* fix test

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* refine

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* rm outter yaml

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

---------

Signed-off-by: forsaken628 <[email protected]>
Signed-off-by: Andrey Velichkevich <[email protected]>
google-oss-prow bot pushed a commit that referenced this pull request Jun 18, 2024
…branch (#2362)

* Fix TestReconcileBatchJob (#2350)

* update

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* update

Signed-off-by: forsaken628 <[email protected]>

* update

Signed-off-by: forsaken628 <[email protected]>

* update

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* cleanup

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* update

Signed-off-by: forsaken628 <[email protected]>

* use gomock

Signed-off-by: forsaken628 <[email protected]>

---------

Signed-off-by: forsaken628 <[email protected]>
Signed-off-by: Andrey Velichkevich <[email protected]>

* Use cache-dependency-path in actions/setup-go for CI workflow (#2355)

Signed-off-by: forsaken628 <[email protected]>
Signed-off-by: Andrey Velichkevich <[email protected]>

* Replace already closed github.com/golang/mock with go.uber.org/mock (#2357)

* replace gomock

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* revert

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

---------

Signed-off-by: forsaken628 <[email protected]>
Signed-off-by: Andrey Velichkevich <[email protected]>

* Replace gRPC code generation tool from Znly/protoc to Buf  (#2344)

* Replace gRPC code generation tool from Znly/protoc to Buf

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* del build.sh

Signed-off-by: forsaken628 <[email protected]>

* cleanup

Signed-off-by: forsaken628 <[email protected]>

* fix test

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* refine

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* rm outter yaml

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

---------

Signed-off-by: forsaken628 <[email protected]>
Signed-off-by: Andrey Velichkevich <[email protected]>

* Upgrade the protobuf version to >=4.21.12,<5 (#2358)

Signed-off-by: Yuki Iwai <[email protected]>
Signed-off-by: Andrey Velichkevich <[email protected]>

* [SDK] Fix empty list for env variables and numpy version (#2360)

* [SDK] Fix empty list for env variables

Signed-off-by: Andrey Velichkevich <[email protected]>

* Fix numpy version in tests

Signed-off-by: Andrey Velichkevich <[email protected]>

---------

Signed-off-by: Andrey Velichkevich <[email protected]>

---------

Signed-off-by: forsaken628 <[email protected]>
Signed-off-by: Andrey Velichkevich <[email protected]>
Signed-off-by: Yuki Iwai <[email protected]>
Co-authored-by: coldWater <[email protected]>
Co-authored-by: Yuki Iwai <[email protected]>
shashank-iitbhu pushed a commit to shashank-iitbhu/katib that referenced this pull request Jun 19, 2024
)

* Replace gRPC code generation tool from Znly/protoc to Buf

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* del build.sh

Signed-off-by: forsaken628 <[email protected]>

* cleanup

Signed-off-by: forsaken628 <[email protected]>

* fix test

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* refine

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* rm outter yaml

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

---------

Signed-off-by: forsaken628 <[email protected]>
shashank-iitbhu pushed a commit to shashank-iitbhu/katib that referenced this pull request Jun 30, 2024
)

* Replace gRPC code generation tool from Znly/protoc to Buf

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* del build.sh

Signed-off-by: forsaken628 <[email protected]>

* cleanup

Signed-off-by: forsaken628 <[email protected]>

* fix test

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* refine

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* rm outter yaml

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

---------

Signed-off-by: forsaken628 <[email protected]>
shashank-iitbhu pushed a commit to shashank-iitbhu/katib that referenced this pull request Jul 25, 2024
)

* Replace gRPC code generation tool from Znly/protoc to Buf

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* del build.sh

Signed-off-by: forsaken628 <[email protected]>

* cleanup

Signed-off-by: forsaken628 <[email protected]>

* fix test

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* refine

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

* rm outter yaml

Signed-off-by: forsaken628 <[email protected]>

* fix

Signed-off-by: forsaken628 <[email protected]>

---------

Signed-off-by: forsaken628 <[email protected]>
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.

Replace the tool of code generator for the gRPC API
3 participants