-
-
Notifications
You must be signed in to change notification settings - Fork 11.7k
[Bugfix] Fix host and port join for ipv6 in bench serve #28679
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
Conversation
Signed-off-by: Scott Zhang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly fixes an issue with IPv6 address formatting when constructing the benchmark server URL by using the join_host_port utility. This is a good improvement. While reviewing, I noticed a related potential issue in the URL construction logic that could also lead to invalid URLs if the endpoint path is not formatted with a leading slash. I've added a suggestion to make the URL construction more robust.
| api_url = f"http://{host_port}{args.endpoint}" | ||
| base_url = f"http://{host_port}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current string concatenation for api_url is brittle. If a user provides an endpoint without a leading slash (e.g., v1/completions instead of the default /v1/completions), it will result in an invalid URL like http://localhost:8000v1/completions. It's better to construct the URL more robustly to ensure there is always a single slash between the base URL and the endpoint path. My suggestion also reuses the base_url variable, making the code slightly more DRY.
| api_url = f"http://{host_port}{args.endpoint}" | |
| base_url = f"http://{host_port}" | |
| base_url = f"http://{host_port}" | |
| api_url = f"{base_url}/{args.endpoint.lstrip('/')}" |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
heheda12345
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…#28679) Signed-off-by: Scott Zhang <[email protected]> Co-authored-by: Scott Zhang <[email protected]> Signed-off-by: Bram Wasti <[email protected]>
…#28679) Signed-off-by: Scott Zhang <[email protected]> Co-authored-by: Scott Zhang <[email protected]>
…#28679) Signed-off-by: Scott Zhang <[email protected]> Co-authored-by: Scott Zhang <[email protected]>
…#28679) Signed-off-by: Scott Zhang <[email protected]> Co-authored-by: Scott Zhang <[email protected]>
Purpose
Fix IPv6 address formatting in the benchmark client (
vllm bench serve).Previously, when using
--host ::or other IPv6 addresses, the benchmark client would construct invalid URLs likehttp://:::8000/v1/completions, causingaiohttpto fail withValueError: Invalid URL: port can't be converted to integer.This PR fixes the issue by using the existing
join_host_port()utility fromvllm.utils.network_utils, which properly wraps IPv6 addresses in brackets per RFC 3986 (e.g.,http://[::]:8000/v1/completions).Test Plan
Manual testing with different host formats: