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

[helm] don't use vllm service name #612

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

Conversation

poussa
Copy link
Collaborator

@poussa poussa commented Nov 26, 2024

Description

If vllm k8s service name is vllm bad things happen and the server fails to comeup with error message:

ERROR 11-26 09:29:11 engine.py:366]     lambda: int(os.getenv('VLLM_PORT', '0'))
ERROR 11-26 09:29:11 engine.py:366] ValueError: invalid literal for int() with base 10: 'tcp://10.100.120.175:80'

See vllm note about the service naming

Issues

na see above.

Type of change

List the type of change like below. Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would break existing design and interface)

Dependencies

na

Tests

Manually tested.

Copy link
Contributor

@eero-t eero-t left a comment

Choose a reason for hiding this comment

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

Service fullname is always prefixed with the Helm parent chart name. Only if you helm install vLLM component directly, it's just vllm.

Agent component already relies on that service name being $release-vllm, and there are couple of not-yet-merged PRs that also rely on it.

=> You need to add check that updates fullname only when it actually equals vllm. And a comment with link to the doc (https://github.com/vllm-project/vllm/blob/main/docs/source/serving/env_vars.rst) would also be nice.

@yongfengdu
Copy link
Collaborator

@yongfengdu
Copy link
Collaborator

If so, the testpod will have to do this check too.
Do you know a better way to reference the service name in test pod? currently we use {{ include "vllm.fullname" . }}, the same as used in service name.

=> You need to add check that updates fullname only when it actually equals vllm. And a comment with link to the doc (https://github.com/vllm-project/vllm/blob/main/docs/source/serving/env_vars.rst) would also be nice.

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.

3 participants