client
client copied to clipboard
triton-inference-server
feat: Fix generation, update go structure, add go module
I tried out the go grpc client but found some issues, let me know if you want me to break it up further.
This pr aims to do the following fixes:
- Plugins are not supported any longer, so update protoc command: https://github.com/golang/protobuf/issues/1070
- Add protobuff option to allow for he go package generation: https://developers.google.com/protocol-buffers/docs/reference/go-generated#package
- Add
go.modfile and include package to not require everyone to generate it. - Update
README.md
Thank you!
Hmm realised that in order to export the go package we need to start using tags based upon semver, would that make sense in relation to the current release structure of the client?
@Tabrizian could you help our with this PR or maybe point me to someone that can? Thanks:)
Hi @NikeNano, thanks for your contribution. I have requested a review from experts in this area to help review this change.
@Tabrizian maybe you know someone that could take a look otherwise I will close it so I dont just leave a dangling PR, thanks.
I believe I tested these changes and they worked, and it looks like Niklas responded to all the feedback from Ryan and me. @rmccorm4, do you know if L0_simple_go_client is sufficient to test these changes? If so, have you run it or should I? Then, we can merge this PR.
Thanks for your patience, Niklas, and sorry that this took a while.
As long as we're in agreement that we won't be maintaining a public go package at this time, I think this PR can be accepted for some modernization/cleanup.
However, a few things that will need to happen:
- Yes I believe the L0_simple_go_client test passing should be sufficient @dyastremsky. I don't think it will pass as-is because the package in the go script is renamed in this PR, but the old package name is built in the server/qa test, see (2) below.
- This section of the test should probably be fixed/updated to just use the
gen_go_stubs.shscript from the client repo as a single source of truth. - Might as well update the example/README to use/test with the latest container at time of merging (ex: 22.10 instead of 22.07)
If so, have you run it or should I? Then, we can merge this PR.
@dyastremsky do you mind driving this when you have time?
I can drive this. Do you have suggestions for 2? I don't believe the client repo gets built into the QA container, so I imagine I'd have to change the build script to copy the script into the container. It might not be worth it versus imply renaming the variable.
@NikeNano, have you filled out the CLA yet? https://github.com/triton-inference-server/server/blob/main/CONTRIBUTING.md#contributor-license-agreement-cla
We usually ask before code review starts, but I'm not seeing yours filed. Would you be able to submit it, if you haven't yet?
@NikeNano, have you filled out the CLA yet? https://github.com/triton-inference-server/server/blob/main/CONTRIBUTING.md#contributor-license-agreement-cla
We usually ask before code review starts, but I'm not seeing yours filed. Would you be able to submit it, if you haven't yet?
Will fill it out, been over loaded so missed the comments here.
@NikeNano, have you filled out the CLA yet? https://github.com/triton-inference-server/server/blob/main/CONTRIBUTING.md#contributor-license-agreement-cla
We usually ask before code review starts, but I'm not seeing yours filed. Would you be able to submit it, if you haven't yet?
I sent it on the 21 of July to [email protected], I can send it again if there are some issues @dyastremsky
@matthewkotila Yes. We need to address Ryan's comment with three needed changes. Specifically, this makes L0_simple_go_client fail and therefore CI fails.
Since we've got the CLA verified now, I'll go back to working on it once I work through a few very high priority items on my plate. If you prefer to tackle this and try to get it merged, I've got some pending changes on this branch that should help with 1 and 2, though I don't recall if it got the test fully passing.
@matthewkotila Yes. We need to address Ryan's comment with three needed changes. Specifically, this makes L0_simple_go_client fail and therefore CI fails.
Since we've got the CLA verified now, I'll go back to working on it once I work through a few very high priority items on my plate. If you prefer to tackle this and try to get it merged, I've got some pending changes on this branch that should help with 1 and 2, though I don't recall if it got the test fully passing.
I will take a look on monday @dyastremsky, would be fun to pair up on the fixes, always happy to help out and especially since we would like to use this in prod.
Thank you for the generous offer to pair up, Niklas! I've updated the tests to build off your changes. They're passing locally, so now running it through our internal CI to make sure all tests pass.
Once that's done, we should be able to merge both PRs into the main branch. Expecting to do so in the next day. Would you be able to do a quick update to address this comment from Ryan? I know you submitted this PR a while back, so it's not mandatory but would be nice to merge all together.
Might as well update the example/README to use/test with the latest container at time of merging (ex: 22.10 instead of 22.07)
Might as well update the example/README to use/test with the latest container at time of merging (ex: 22.10 instead of 22.07)
Update to: nvcr.io/nvidia/tritonserver:22.11-py3 which seems to be the latest? Where is it possible to explore the images and how does the releases work, seems like it don't match between different components of triton?
Thanks, @NikeNano! 22.11 was released just today, so that's perfect.
You can find the latest image here: https://catalog.ngc.nvidia.com/orgs/nvidia/containers/tritonserver You can see the latest release notes here (22.11 ones are still in "Draft" mode): https://github.com/triton-inference-server/server/releases