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

Expose tritonserver args in values.yaml #5582

Merged

Conversation

okyspace
Copy link
Contributor

@okyspace okyspace commented Apr 2, 2023

Current k8s_onprem chart has predefined args in deployment.yaml. We could change it from the yaml file though but would be cleaner to customise the args through values.yaml instead.

@rmccorm4
Copy link
Contributor

rmccorm4 commented Apr 4, 2023

Seems reasonable to me. CC @nealvaidya for comments

@rmccorm4 rmccorm4 self-assigned this Apr 4, 2023
@nealvaidya
Copy link
Contributor

Looks good to me

@rmccorm4 rmccorm4 changed the title Allow to customise args for tritonserver through values.yanl instead … Expose tritonserver args in values.yaml Apr 4, 2023
Copy link
Contributor

@rmccorm4 rmccorm4 left a comment

Choose a reason for hiding this comment

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

Hi @okyspace, LGTM overall, just a few things:

  1. Rename additionalArgs to serverArgs to be more clear.
  2. Can you also share an example command ... --set serverArgs=... and the output showing that server deployed successfully as a sanity check?
  3. Have you signed a CLA already? If not, please follow the instructions here and let us know when you've sent it.

deploy/k8s-onprem/templates/deployment.yaml Outdated Show resolved Hide resolved
deploy/k8s-onprem/values.yaml Outdated Show resolved Hide resolved
@okyspace
Copy link
Contributor Author

okyspace commented Apr 6, 2023

@rmccorm4 , Thanks for your reply.

  1. I have updated the naming based on your suggestion.
  2. I can work out an example. Can advise where to provide this example?
  3. I will follow up with the CLA.

Thanks.

@okyspace
Copy link
Contributor Author

@rmccorm4, as requested, I have added the examples for sanity check. Any comments?


The examples show how you can set the customised serverArgs and verify that they are taking effect.

  1. Execute the command below to generate the helm template to check that the serverArgs is being set correctly.
    helm template --set serverArgs="{'--model-repository=s3://192.168.1.110:9000/triton-models', '--repository-poll-secs=8', '--model-control-mode=poll', '--grpc-use-ssl=false'}" triton -n triton .

You should see the generated deployment.yaml. The serverArgs are populated as array args in this example.
image

  1. Run the container using the command.
    helm install --set serverArgs="{'--model-repository=s3://192.168.1.110:9000/triton-models', '--repository-poll-secs=8', '--model-control-mode=poll', '--grpc-use-ssl=false'}" triton -n triton .

Run kubectl describe pods -n triton. You should see the args in the description.
image

Once the container is successfully running, run kubectl logs <container id> -n triton.
You should see these outputs that indicates the args set for the triton server.

for e.g. model-repository, model-control-mode
image

for e.g. repository-poll-secs. You can see from the logs that "Polling model repository" happens every 8 secs.
image

@okyspace
Copy link
Contributor Author

I have also emailed my signed CLA.

@okyspace
Copy link
Contributor Author

Hi, is there anything that I need to follow up to close this PR?

@okyspace
Copy link
Contributor Author

@rmccorm4 , is there something i missed out for this PR?

rmccorm4
rmccorm4 previously approved these changes Feb 21, 2024
Copy link
Contributor

@rmccorm4 rmccorm4 left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks for your patience @okyspace 🙇 Letting the linters/pre-commit run to double check.

Copy link
Contributor

@rmccorm4 rmccorm4 left a comment

Choose a reason for hiding this comment

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

Added suggested fixes for linter errors

@okyspace okyspace requested a review from rmccorm4 February 22, 2024 14:06
@okyspace
Copy link
Contributor Author

@rmccorm4 Thanks for the reviews and suggestions.

@rmccorm4
Copy link
Contributor

rmccorm4 commented Feb 22, 2024

I have also emailed my signed CLA.

Hi @okyspace, can you either resend this email or clarify the name and company attached to it?

@okyspace
Copy link
Contributor Author

@rmccorm4
I have resend the CLA.
Name: Ong Kai Yong
Company: N/A. I am not submitting on behalf of a company.

@rmccorm4 rmccorm4 merged commit 46f87ff into triton-inference-server:main Mar 4, 2024
@rmccorm4
Copy link
Contributor

rmccorm4 commented Mar 4, 2024

Thanks for your contribution and patience @okyspace !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants