-
Notifications
You must be signed in to change notification settings - Fork 832
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
Fixed Seldon golang package #1246
Conversation
Hi @hemantkashniyal. Thanks for your PR. I'm waiting for a SeldonIO or seldonio member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign @adriangonz |
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.
Thanks for your contribution @hemantkashniyal!
I've added a few comments on the PR, mostly around how the *.proto
are downloaded and compiled. Besides that, I think my only remaining question would be if we should keep this on the /incubating
folder or if we should move it to root folder. What do you think @cliveseldon?
@adriangonz @cliveseldon it seems Is Moreover these @adriangonz kindly review PR, have incorporated suggestions. |
Hi @adriangonz, can you please review the pull request, I have updated the changes. |
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.
Nice one! Thanks for making those changes @hemantkashniyal. I think this looks pretty well now.
Regarding the incubating/
folder, we use that for wrappers/clients which may not be ready yet for prime time and thus are not 100% supported. The idea is that as they become more mature and stable, they "graduate" towards stable ones which live on the root folder. However, you raise a good point regarding the imports.
What are your thoughts on this @cliveseldon @axsaucedo?
/ok-to-test |
Thu Dec 19 10:20:35 UTC 2019 impatient try |
Thu Dec 19 10:20:37 UTC 2019 impatient try |
Yes might be good to move to |
Got it @cliveseldon! Can you change that @hemantkashniyal? We can think of ways to reduce breaking changes when the Go wrapper is moved from I think after that this PR should be ready to go. |
Pulling main repo changes
Thu Dec 19 15:14:20 UTC 2019 impatient try |
Thu Dec 19 15:14:32 UTC 2019 impatient try |
Thu Dec 19 15:23:57 UTC 2019 impatient try |
Thu Dec 19 15:24:00 UTC 2019 impatient try |
Thu Dec 19 15:26:13 UTC 2019 impatient try |
Thu Dec 19 15:26:25 UTC 2019 impatient try |
Thu Dec 19 15:45:13 UTC 2019 impatient try |
Thu Dec 19 15:45:25 UTC 2019 impatient try |
Hi @adriangonz , moved protos to incubating folder. Kindly review PR. Following command needs to be added to CI |
/approve Awesome! Thanks for your effort and for contributing this work! I think it looks great now! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriangonz 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 |
Fri Dec 20 14:18:43 UTC 2019 impatient try |
Fri Dec 20 14:18:55 UTC 2019 impatient try |
Compiled code for tensorflow and seldon as a GO package
Issue #1156
Fixes #1245
TODO: need to include
make protos_go
target in CI/CD build @adriangonz