docs(vllm): document current benchmark sampling flags, demote the legacy aliases - #12979
docs(vllm): document current benchmark sampling flags, demote the legacy aliases#12979dagil-nvidia wants to merge 1 commit into
Conversation
…acy aliases The Benchmarking section listed five --benchmark-*-granularity flags as current parameters with invented defaults. Those flags are deprecated compatibility aliases (components/src/dynamo/vllm/backend_args.py, legacy_sampling_flags), each registered with default=None and help text pointing at a replacement. The six current parameters were absent entirely, so a reader configuring a sweep reached for the deprecated path and saw defaults with no source in the code. - Document --benchmark-points-file and the five *-samples sampling limits with their real defaults and selection semantics - Move the five --benchmark-*-granularity aliases into the existing Deprecated section, each naming its replacement Signed-off-by: Dan Gil <dagil@nvidia.com>
github-actions
Bot
added
docs
documentation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
WalkthroughChangesvLLM benchmark configuration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/fern/pages/reference/backends/vllm-configuration.mdx`:
- Around line 194-198: Update the documentation for the --benchmark-points-file
parameter to explicitly state that --benchmark-mode must also be provided;
retain the existing description of how the file affects benchmark sampling and
rank forwarding.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c8cabb60-a3b6-4d21-951b-0169844a1cce
📒 Files selected for processing (1)
docs/fern/pages/reference/backends/vllm-configuration.mdx
| <ParamField path="--benchmark-points-file" type="string" default="null"> | ||
| JSON file of explicit pure prefill/decode benchmark points, applied uniformly to every data-parallel rank. The file completely replaces generated grid sampling for the phases selected by `--benchmark-mode`, so the sampling limits below are ignored when it is set. It is read and normalized once before vLLM workers start, then the same contents are forwarded to every rank. | ||
|
|
||
| Environment variable: `DYN_BENCHMARK_PREFILL_GRANULARITY` | ||
| Environment variable: `DYN_BENCHMARK_POINTS_FILE` | ||
| </ParamField> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the --benchmark-mode prerequisite.
--benchmark-points-file fails at startup when --benchmark-mode is absent. State this requirement explicitly. The loader enforces it in components/src/dynamo/vllm/backend_args.py lines 497-504.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/fern/pages/reference/backends/vllm-configuration.mdx` around lines 194 -
198, Update the documentation for the --benchmark-points-file parameter to
explicitly state that --benchmark-mode must also be provided; retain the
existing description of how the file affects benchmark sampling and rank
forwarding.
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32604fc7d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| <ParamField path="--benchmark-decode-length-granularity" type="integer" default="6"> | ||
| Number of context length sample points for the decode sweep. | ||
| <ParamField path="--prefill-max-new-token-samples" type="integer" default="64"> | ||
| Maximum number of iteration-total prefill new-token samples. If the CUDA-graph-aware axis has more points, points are selected uniformly across the sorted axis while always retaining its minimum and maximum. Must be at least 2. |
There was a problem hiding this comment.
When a CUDA-graph-aware axis has an eager tail comprising at most 20% of its candidates, _limit_cudagraph_axis preserves every eager-tail point and uniformly samples only the graph-covered prefix; it does not sample uniformly across the full sorted axis as stated here. This affects both this prefill flag and the analogous decode batch-size description, and can mislead users predicting which operating points a constrained benchmark sweep will retain.
Useful? React with 👍 / 👎.
| <ParamField path="--benchmark-points-file" type="string" default="null"> | ||
| JSON file of explicit pure prefill/decode benchmark points, applied uniformly to every data-parallel rank. The file completely replaces generated grid sampling for the phases selected by `--benchmark-mode`, so the sampling limits below are ignored when it is set. It is read and normalized once before vLLM workers start, then the same contents are forwarded to every rank. |
There was a problem hiding this comment.
Using --benchmark-points-file without explicitly setting --benchmark-mode does not merely leave the phase unspecified: DynamoVllmConfig._load_explicit_benchmark_points raises ValueError("--benchmark-points-file requires --benchmark-mode"), so the worker fails during argument validation. The new flag description should identify this required companion option so a command assembled from the reference can start successfully.
Useful? React with 👍 / 👎.
| <ParamField path="--benchmark-prefill-granularity" type="integer" default="16"> | ||
| Number of ISL sample points for the prefill sweep. | ||
| <ParamField path="--benchmark-points-file" type="string" default="null"> | ||
| JSON file of explicit pure prefill/decode benchmark points, applied uniformly to every data-parallel rank. The file completely replaces generated grid sampling for the phases selected by `--benchmark-mode`, so the sampling limits below are ignored when it is set. It is read and normalized once before vLLM workers start, then the same contents are forwarded to every rank. |
There was a problem hiding this comment.
This newly documented flag is not usable from the reference because it never specifies or links to the strict manifest schema. A repo-wide search finds no other user-facing schema or example, while BenchmarkPoints requires schema_version, prefill, and decode, with different required fields for prefill and decode entries and forbidden extra fields; users therefore cannot construct a valid file without reading Python source or tests.
Useful? React with 👍 / 👎.
| </ParamField> | ||
|
|
||
| <ParamField path="--benchmark-prefill-granularity" type="integer" default="null" deprecated={true}> | ||
| **Deprecated** — use `--prefill-max-new-token-samples`. Legacy values are translated to the new sampling limit. |
There was a problem hiding this comment.
Legacy values are translated only when a benchmark mode is active and no explicit points file was loaded: _resolve_legacy_benchmark_sampling returns before processing these mappings when benchmark_mode is None or _benchmark_points is set. In particular, combining this alias with --benchmark-points-file silently ignores it without the stated translation or deprecation warning, so qualify this behavior as applying only to generated-grid benchmarks; the same issue affects the other four aliases added below.
Useful? React with 👍 / 👎.
|
Closing in favor of #13036, which covers everything here plus one defect this PR missed. Both document the current benchmark sampling flags and demote the five `--benchmark-*-granularity` aliases to Deprecated. #13036 additionally corrects `--benchmark-timeout`: the page documented `default="300"` with hard-limit semantics ("worker startup fails if this limit is exceeded"), while `components/src/dynamo/vllm/backend_args.py:467` declares `benchmark_timeout: int = 900` and the limit is soft, since the measured iteration finishes and partial results are returned before startup continues. It also qualifies when the legacy flags are actually read, which is only with `--benchmark-mode` set and no points file. One PR on one file beats a merge plus a follow-up, so this is the redundant half. The release-branch cherry-pick #12983 has been updated to carry those same three corrections, so 1.4.0 does not ship without them. NVBug 6572846 stays tracked against #13036 and #12983. |
1 participant
Summary
The vLLM Benchmarking reference listed five
--benchmark-*-granularityflags as current parameters, with defaults that have no source in the code. Those flags are deprecated compatibility aliases:components/src/dynamo/vllm/backend_args.pyregisters them inlegacy_sampling_flagswithdefault=Noneand help text that names a replacement for each.The six current parameters were not documented at all, so a reader configuring a self-benchmark sweep reached for the deprecated path and tuned against defaults that do not exist.
Changes
Documented as current, with the real defaults and selection semantics from the source:
--benchmark-points-file--prefill-max-new-token-samples--prefill-max-kv-read-token-samples--prefix-max-batch-size-samples--decode-max-kv-read-token-samples--decode-max-batch-size-samplesMoved to the existing Deprecated section, each naming its replacement:
--benchmark-prefill-granularity,--benchmark-prefill-kv-read-granularity,--benchmark-prefill-batch-granularity,--benchmark-decode-length-granularity,--benchmark-decode-batch-granularity.Scope
The originating report covered four items against
release/1.4.0. Three are already correct onmainand are release-branch-only: the planner engine-capacity metric name, the Prometheus CA-bundle field description, and the planner CLI examples. This PR fixes the one item still wrong onmain; the cherry-pick carries the full set to the release branch.Documentation only, no behavior change.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation