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

deploy: add initial vllm worker chart #37

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

whoisj
Copy link
Collaborator

@whoisj whoisj commented Jan 17, 2025

This change adds the initial version of the vLLM worker Helm chart.

This is NOT the final values schema.

Includes schema validation and embedded chart scripting to enable default values.

No tests included because we have not yet agreed on the best mechanism to validate Helm charts.

DLIS-7810

@whoisj
Copy link
Collaborator Author

whoisj commented Jan 17, 2025

This PR makes me realize that we need to agree on a mechanism for Helm chart validation / testing.

If we can agree to allowing the test to be run using pwsh, then I have an easy way to achieve this. Other wise, we'll need to come up with something reasonable.

@whoisj whoisj force-pushed the jwyman/helm-chart-1 branch from fa264bd to d497e15 Compare January 21, 2025 18:31
@whoisj whoisj force-pushed the jwyman/helm-chart-1 branch from d497e15 to d897370 Compare January 21, 2025 20:39
@whoisj whoisj force-pushed the jwyman/helm-chart-1 branch from d897370 to 1050df6 Compare January 21, 2025 23:15
@whoisj whoisj requested a review from indrajit96 January 21, 2025 23:18
@whoisj whoisj force-pushed the jwyman/helm-chart-1 branch from 1050df6 to 2daf46e Compare January 21, 2025 23:24
@whoisj whoisj force-pushed the jwyman/helm-chart-1 branch from 2daf46e to 7ab11ef Compare January 21, 2025 23:32
@whoisj whoisj force-pushed the jwyman/helm-chart-1 branch from 7ab11ef to cbdbe86 Compare January 23, 2025 21:15
@whoisj whoisj force-pushed the jwyman/helm-chart-1 branch from cbdbe86 to ed22f12 Compare January 24, 2025 17:55
@whoisj
Copy link
Collaborator Author

whoisj commented Jan 27, 2025

@alec-flowers I've updated the documentation inside values.yaml per your recommendation. Please let me know if it is sufficient now. Thanks.

{{- end }}
{{- if false }}
# What are these and are they needed? HELP!
- VLLM_ATTENTION_BACKEND: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/triton-inference-server/triton_distributed/blob/main/examples/python/llm/vllm/deploy/deploy_llama_8b_disaggregated.sh#L5

We have similar variables here. The VLLM prefix part is used to configure vLLM engine, NATS are of request plane.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes I see that. What should the value be by default and what are the allowed values?

This change adds the initial version of the vLLM worker Helm chart.

This is NOT the final values schema.

Includes schema validation and embedded chart scripting to enable default values.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants