Skip to content

Conversation

@poussa
Copy link
Member

@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
Collaborator

@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.

@poussa
Copy link
Member Author

poussa commented Feb 11, 2025

Not needed.

@poussa poussa closed this Feb 11, 2025
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