docs(vllm): document current benchmark sampling flags and fix the timeout default - #13036
Merged
MatejKosec merged 2 commits intoAug 14, 2026
Conversation
…eout The Benchmarking section of the vLLM configuration reference was authored against an earlier shape of the argument parser and never re-derived when the code moved. Three consequences, all verified against components/src/dynamo/vllm/backend_args.py: - Three deprecated --benchmark-*-granularity flags were presented as current options with invented defaults (16, 6, 6). All five legacy granularity flags are registered with default=None and help text naming their replacement. - The six flags that actually control sampling today were undocumented. - --benchmark-timeout was documented as default 300 and as a hard limit that fails worker startup. It defaults to 900 and is a soft limit: the current iteration finishes, partial results are returned, and startup continues. Document the six current sampling flags with their real defaults, correct the timeout entry, and move all five granularity flags into the existing Deprecated section with their replacement mapping and the combine-raises behavior. Docs only; no behavior change. Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com>
glamr-agent
temporarily deployed
to
external_collaborator
August 11, 2026 18:53 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
glamr-agent
temporarily deployed
to
external_collaborator
August 11, 2026 18:53 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
github-actions
Bot
added
external-contribution
Contributor
Author
Automated evidence record — validation completeValidation status: complete Evidence summary: [3/3 validated] AI review assessment (advisory, not a merge authorization): sound — an automated Validation result: complete — pass. Evidence audit: complete [3/3 validated] — the evidence table is grounded in Evidence [3/3 validated]Generated from validation/registry.jsonl — do not edit by hand.
|
Contributor
Author
plan.md# Plan — DYN-3803: four user-doc inconsistencies with the shipped implementation
Route: implementation
Template: debug-investigation
Engine: vllm
No `Disposition:` line is written. Discovery found no *merged* pull request that
covers the whole request on the Base branch; the coverage that exists is partly a
different branch and partly an *open* pull request, and the planner brief is
explicit that neither qualifies.
## User intent
A Slack thread asks us to triage Linear issue DYN-3803
(`[dynamo][1.4.0][Planner][docs] Four user-doc inconsistencies with the shipped
implementation`), find the root cause of each of the four reported
documentation defects, and fix them. The four, as reported, are: (1) metric
names in the docs that the shipped implementation never emits; (2) a CA-bundle
field description whose meaning is inverted relative to the code; (3) CLI
examples that do not work against the shipped argument parser; and (4)
deprecated parameters presented alongside current ones as though they were
current.
The Base branch given in `input.md` is `main`, and the review strategy is
`unknown` with `Review requests: none recorded`, so this is an ordinary
implementation request that will open one new pull request against `main`. The
caller did not ask us to review anybody's existing change.
Note on the ticket text: the Linear issue body and comments were **not**
captured in `input.md` (`Issue body: none available`, `Issue comments: none
available`). I attempted the permitted read-only retrieval and it is not
available in this sandbox — the `agenthub-linear` binary is not on `PATH` and
`LINEAR_API_KEY` is unset. No key value was read or printed. Every one of the
four items below is therefore grounded in the repository and in GitHub
provider metadata rather than in the ticket's own wording, which turned out to
be sufficient: an in-flight pull request (#12983) ports the originating report
verbatim and enumerates all four items, and each one is independently
verifiable against source.
## Non-goals
- **Not** fixing `release/1.4.0`. Three of the four items are wrong only on the
1.4.0 release branch and are already correct on `main`; a cherry-pick that
carries them to the release branch is already open as #12983 by another
author. Our Base branch is `main`, and duplicating that port would be a
second change over the same release-branch files.
- **Not** touching the `ttft:` / `itl:` versus `ttft_ms:` / `itl_ms:` alias
usage in the Planner guides. These aliases are still accepted by
`PlannerConfig` (`validation_alias=AliasChoices(...)`, no `deprecated=True`
marker) and are used consistently across `planner-guide.md`,
`global-planner-guide.md`, `auto-deploy-with-dgdr.md`, and
`examples/deployments/dgdr/planner.yaml`. This was my earlier hypothesis for
item 4 and it is wrong; sweeping it in would balloon the diff without fixing
anything the ticket reported.
- **Not** treating `dynamo_ttft_p95` / `dynamo_ttft_p95_seconds` in
`autoscaling.md` as a defect. Those are user-chosen Prometheus-Adapter
`rules.external[].name.as` aliases and a KEDA `metricName`, layered over the
genuinely emitted `dynamo_frontend_time_to_first_token_seconds_bucket`. The
page is internally consistent. This was a false lead and is dropped.
- **Not** building a generic docs-vs-argparse parity test harness. That is a
larger change than the ticket asks for and would touch every reference page.
- **Not** regenerating any page under `pages/reference/api/` or
`pages/reference/kubernetes-api/`. The file we touch carries no
`GENERATED … do not edit` marker and is hand-authored.
- No GPU work, no runtime behavior change, no engine smoke test.
## Discovery
### What I read
- `/workflow/agents/01-agency/planner/goal.md`, the
`templates/debug-investigation.md` overlay, `validation-recipes/README.md`,
and recipes `00`, `01`, `03`, `04`, `05`, plus
`<work-item-root>/compute-env.md` (1×A100-SXM4-80GB, vLLM 0.22.0 in-sandbox,
no Docker socket, `gh` on `PATH`).
- `docs/fern/AGENTS.md` and
`docs/fern/pages/community/contributing/documentation/documentation-style-guide.md`.
- `.pre-commit-config.yaml` (the docs hooks: `check-asset-paths` on
`^docs/fern/`, `gen-llms-tables-check` on
`^docs/fern/…pages/reference/general/.*\.mdx`, `check-agent-twins`,
`codespell`, `end-of-file-fixer`, `trailing-whitespace`,
`mixed-line-ending`), plus `.github/workflows/docs-link-check.yml` and
`docs/fern/scripts/check_reference.sh`.
- `components/src/dynamo/vllm/backend_args.py`,
`components/src/dynamo/vllm/tests/test_backend_args.py`,
`components/src/dynamo/vllm/tests/test_benchmark_points.py`.
- `components/src/dynamo/planner/__main__.py`,
`components/src/dynamo/planner/config/planner_config.py`,
`components/src/dynamo/planner/monitoring/traffic_metrics.py`,
`components/src/dynamo/planner/core/base.py`,
`components/src/dynamo/planner/tests/unit/test_predicted_metric_gating.py`,
`lib/bindings/python/src/dynamo/prometheus_names.py`.
- On `main`: `docs/fern/pages/reference/backends/vllm-configuration.mdx`,
`docs/fern/pages/reference/backends/sglang-configuration.mdx` (for the
house pattern of a `## Deprecated` section),
`docs/fern/pages/reference/components/planner-configuration.mdx`,
`docs/fern/pages/reference/kubernetes-api/dynamo-graph-deployment-request.mdx`,
`docs/fern/pages/developer-guide/knowledge-base/modular-components/planner/{overview.md,planner-guide.md,planner-examples.md}`,
`docs/fern/pages/developer-guide/knowledge-base/kubernetes/kubernetes-operator/autoscaling.md`.
- On `release/1.4.0`, fetched read-only through
`gh api … -H "Accept: application/vnd.github.raw"`:
`docs/fern/components/planner/README.md`,
`docs/fern/components/planner/planner-config-reference.mdx`,
`docs/fern/components/planner/planner-examples.md`.
### Related-request survey
`gh pr list --state all --search DYN-3803` returns nothing relevant — the
factory convention keeps Linear IDs out of shipped text, and
`docs/fern/AGENTS.md` forbids JIRA/NVBug-style IDs in docs anyway. Semantic
searches over the affected symbols and subsystems found four requests that
matter:
- **#12408 — MERGED 2026-07-30** (`86f84b948`, "docs: sync Planner, Profiler,
and SPICA references"). This is what fixed the inverted CA-bundle sentence on
`main`. Verified against the current default branch: both
`planner-configuration.mdx:300-302` and
`dynamo-graph-deployment-request.mdx:467-468` now read "the bundle is used
for verification regardless of `metric_pulling_prometheus_ssl_verify`". Not
reverted, not superseded.
- **#12975 — OPEN**, base `main`, "docs(router): correct the nvext disable flag
and document four shipped surfaces". Touches router/metrics pages only. It
belongs to a different originating report (6571617) and is out of DYN-3803
scope.
- **#12979 — OPEN, unmerged** (`mergedAt: null`, `reviewDecision:
REVIEW_REQUIRED`, not a draft), base `main`, author `dagil-nvidia`, branch
`dagil-nvidia/fix-6572846-planner-docs`,
<https://github.com/ai-dynamo/dynamo/pull/12979>. Title: "docs(vllm):
document current benchmark sampling flags, demote the legacy aliases". It
touches exactly one file, `docs/fern/pages/reference/backends/vllm-configuration.mdx`
(+57/−9), and its body says: "The originating report covered four items
against `release/1.4.0`. Three are already correct on `main` and are
release-branch-only… This PR fixes the one item still wrong on `main`."
- **#12983 — OPEN**, base `release/1.4.0`, branch
`dagil-nvidia/cp-docs-6571617-6572846-140`. This is the authoritative
restatement of the originating report. Its body states plainly that "Three of
the four 6572846 items are release-branch-only. They are already correct on
`main` with no commit behind them", and its file table maps each of the four
items to a file and a correction. That table is what let me identify item 4
correctly after my first hypothesis for it was wrong.
Existing-test survey (`grep -rn` over `tests/` and the component test trees):
`components/src/dynamo/vllm/tests/test_backend_args.py` already exercises the
legacy-to-current sampling mapping — it sets
`config.benchmark_decode_length_granularity` and
`config.prefill_max_new_token_samples_explicit` and asserts the resolution and
conflict behavior — and `test_benchmark_points.py` covers the points-file path.
The behavior these documents is already tested; nothing about this work item
warrants a new test, and adding one that re-asserts the parser's own defaults
would be exactly the tautology `learnings/no-tautological-tests.md` warns
about.
### The four items, each grounded
**Item 1 — metric names that do not exist. Already correct on `main`.**
On `release/1.4.0`, `docs/fern/components/planner/README.md:263` names
`dynamo_planner_engine_prefill_requests_per_second` and
`dynamo_planner_engine_decode_requests_per_second`. Neither exists: the
Prometheus gauges are built from
`components/src/dynamo/planner/core/base.py:560,563`
(`pm.engine_prefill_capacity_requests_per_second`,
`pm.engine_decode_capacity_requests_per_second`) under the
`dynamo_planner` prefix, and
`components/src/dynamo/planner/tests/unit/test_predicted_metric_gating.py:130-131,167-168`
asserts those same names. The `main` equivalent page,
`…/modular-components/planner/overview.md:263`, already carries the correct
`…_capacity_requests_per_second` forms. Corrected values:
`dynamo_planner_engine_prefill_capacity_requests_per_second` and
`dynamo_planner_engine_decode_capacity_requests_per_second`.
Nothing to change on `main`.
**Item 2 — inverted CA-bundle description. Already correct on `main`.**
`release/1.4.0`'s `planner-config-reference.mdx:301` says the CA bundle is a
"No-op unless SSL verification is enabled." The code says the opposite.
`components/src/dynamo/planner/monitoring/traffic_metrics.py:108-130` builds
`PrometheusConnect(url=url, disable_ssl=not ssl_verify)` and then, if
`ca_bundle` is set, unconditionally assigns `self.prom._session.verify =
ca_bundle`. In `prometheus_api_client==0.6.0`, `PrometheusConnect.__init__`
sets `self._session.verify = not disable_ssl` and every request passes
`verify=self._session.verify` — so the later assignment overwrites the
`ssl_verify`-derived value in both directions. The bundle therefore applies
*regardless of* `metric_pulling_prometheus_ssl_verify`, which is what the two
`main` pages already say (fixed by merged #12408). Nothing to change on `main`.
**Item 3 — non-functional CLI examples. Already correct on `main`.**
`release/1.4.0`'s `docs/fern/components/planner/planner-examples.md` shows
container `args` lists containing `--load-predictor`,
`--load-predictor-warmup-trace`, `--load-predictor-log1p`, `--kalman-q-level`,
`--kalman-q-trend`, `--kalman-r`, `--kalman-min-points`, and
`--prophet-window-size`. `components/src/dynamo/planner/__main__.py` builds an
`argparse.ArgumentParser(description="Dynamo Planner")` with exactly one
argument — `--config`, `required=True`, "JSON string or path to a JSON/YAML
config file" — so every one of those eight flags is rejected at startup. The
corrected form is a `PlannerConfig` YAML document passed as
`python -m dynamo.planner --config planner.yaml`, which is precisely what
`main`'s `…/planner/planner-examples.md` already does (line 17 and the YAML
blocks that follow). Nothing to change on `main`.
**Item 4 — deprecated parameters presented as current. STILL WRONG on `main`.
This is the whole of the actionable scope.**
`docs/fern/pages/reference/backends/vllm-configuration.mdx`, under
`## Benchmarking`, documents three flags as current with invented defaults:
`--benchmark-prefill-granularity` (`default="16"`, line 194),
`--benchmark-decode-length-granularity` (`default="6"`, line 200), and
`--benchmark-decode-batch-granularity` (`default="6"`, line 206).
`components/src/dynamo/vllm/backend_args.py:352-392` registers those three —
plus `--benchmark-prefill-kv-read-granularity` and
`--benchmark-prefill-batch-granularity`, which the page omits entirely — in a
`legacy_sampling_flags` tuple, every one with `default=None` and help text
"Deprecated compatibility option; use {replacement}. Legacy values are
translated to the new sampling limit."
`_resolve_legacy_benchmark_sampling` (lines 506-570) maps each to its
replacement, raises if the replacement was also set explicitly, and emits
`_warn_deprecated`. The six flags that actually control sampling today are
undocumented on the page. Their real names, defaults, and line numbers in
`backend_args.py`:
| Flag | Default | Defined at |
|---|---|---|
| `--benchmark-points-file` | `null` | L244 |
| `--prefill-max-new-token-samples` | `64` | L259 |
| `--prefill-max-kv-read-token-samples` | `16` | L273 |
| `--decode-max-kv-read-token-samples` | `128` | L288 |
| `--decode-max-batch-size-samples` | `128` | L302 |
| `--prefix-max-batch-size-samples` | `3` | L316 |
The legacy-to-current mapping, from the `mappings` tuple at L510-540, is:
prefill-granularity → `--prefill-max-new-token-samples`;
prefill-kv-read-granularity → `--prefill-max-kv-read-token-samples`;
prefill-batch-granularity → `--prefix-max-batch-size-samples`;
decode-length-granularity → `--decode-max-kv-read-token-samples`;
decode-batch-granularity → `--decode-max-batch-size-samples`.
**One further drift I found that #12979 does not fix.** The same section
documents `--benchmark-timeout` with `default="300"` and describes it as a hard
limit ("Worker startup fails if this limit is exceeded"). `backend_args.py:411-419`
sets `default=900` and describes a **soft** limit: "After the limit, the
current measured iteration finishes, partial results are returned, and engine
startup continues. A bounded cleanup grace still fails closed if no result is
written." Both the number and the semantics are wrong on the page. This is the
same class of defect the ticket reports and it is not covered anywhere.
### Root cause
All four are the same failure mode, not four independent slips: the
`## Benchmarking` and Planner reference sections were hand-authored against an
earlier shape of the code and never re-derived when the code moved. The vLLM
page even claims in its own opening paragraph that "Every field, type, default,
and choice on this page comes from the `DynamoVllmArgGroup` and
`DynamoVllmConfig` definitions" — a claim nothing enforces. Unlike
`pages/reference/api/` and `pages/reference/kubernetes-api/`, which have
generators and a `--check` pre-merge gate, the backend and component reference
pages are hand-maintained with no parity check, so a rename or a default change
in `backend_args.py` leaves the page silently stale.
### Style constraints the printer must honor
From `docs/fern/AGENTS.md` and the documentation style guide, for the one
`.mdx` file we touch: keep the existing `---` frontmatter with its SPDX header
(copyright range `2025-2026`) and `title`/`subtitle`; do **not** introduce a
body `# H1` — the body starts at `##`; use Fern callout components (`<Note>`,
`<Warning>`, …) rather than `> [!NOTE]` blockquotes, because the file is
`.mdx`; keep relative links with their extensions inside `docs/`; language-tag
every code fence (`bash`, never `sh`); spell the backend **vLLM**; and put no
NVBug/JIRA/Linear ID, internal host, or `TODO` in the shipped text. Follow the
page's own established `<ParamField path="…" type="…" default="…">` shape,
including `deprecated={true}` for entries in `## Deprecated` — the pattern is
already in this repo at `sglang-configuration.mdx:143` and at
`vllm-configuration.mdx:233` for `--model-express-url`.
## Chosen approach
Correct item 4 on `main` in
`docs/fern/pages/reference/backends/vllm-configuration.mdx`, and only there.
Concretely:
1. Rewrite `## Benchmarking` so it documents the six sampling flags that exist
today, with the defaults from the table above, and describes what each one
limits (they are per-axis sample caps over a CUDA-graph-aware axis, not
"granularity" counts).
2. Fix `--benchmark-timeout` to `default="900"` and restate it as a soft limit
whose expiry finishes the current iteration and returns partial results
rather than failing startup, matching `backend_args.py:411-419`.
3. Move all **five** `--benchmark-*-granularity` flags into the existing
`## Deprecated` section with `default="null" deprecated={true}`, each naming
its replacement flag, and note that combining a legacy flag with its
replacement raises. The two flags the page never documented
(`--benchmark-prefill-kv-read-granularity`,
`--benchmark-prefill-batch-granularity`) get entries there too, so a reader
migrating off them can find the mapping.
4. Leave the `--benchmark-mode`, `--benchmark-warmup-iterations`, and
`--benchmark-output-path` entries alone; they already match source.
Items 1, 2, and 3 get **no diff**. They are already correct on `main`, and the
pull-request description must say so explicitly, with the file:line evidence
above, so a reader of the ticket can see that all four were triaged rather than
assuming three were missed. That disclosure is a required deliverable, not a
nicety.
**Overlap with the open pull request #12979, stated plainly.** #12979 is open,
unmerged, review-required, by another author, and covers steps 1, 3, and 4 of
the list above on the same file. It does *not* cover step 2 (the
`--benchmark-timeout` default and semantics), which is a genuine independent
defect. The planner brief's default counsel is to help an existing open request
land rather than open a second one over the same file, and that counsel applies
here — but the caller asked for a fix and the sandbox's only publication
pathway is one new pull request against `main`, so standing down entirely would
return nothing. The resolution: produce the full corrected section (a partial
diff that fixes only the timeout line would leave the page half wrong at HEAD),
and require the pull-request description to link #12979 by URL, state that it
covers most of the same ground, name the `--benchmark-timeout` drift as the
delta, and invite the maintainer to take whichever is more convenient — merging
#12979 and cherry-picking the timeout hunk is an explicitly acceptable outcome.
A maintainer must never have to work out on their own that two requests overlap.
For the same reason there is no `Disposition: already-resolved`: #12979 is
**open**, and the brief forbids that disposition for an open request, a
closed-unmerged request, or partial coverage. The merged request that does
exist (#12408) covers item 2 only. Partial coverage narrows scope; it does not
end the work item. Of the four reported items, exactly one — item 4 — remains
actionable on the Base branch.
## Rejected alternatives
- **Write `Disposition: already-resolved` and stand down.** Rejected: the only
merged request, #12408, covers one of the four items, and the request that
covers item 4 is unmerged. The brief is explicit that an open request is not
proof of resolution.
- **`Route: review` on #12979.** Rejected: `input.md` records `Review requests:
none recorded` and the caller asked for triage plus fixes, not for a review
of somebody else's change. Nothing in discovery changes that. The overlap is
handled by disclosure in the pull-request description instead.
- **Also port items 1–3 to `release/1.4.0`.** Rejected: the Base branch is
`main`, and #12983 already carries exactly that port. Opening a second
release-branch change over the same three files is the duplication the brief
warns against.
- **Fix only the `--benchmark-timeout` line, to minimize overlap with #12979.**
Rejected: it is the smallest diff but not the smallest *coherent* one. If
#12979 does not land, the page still presents three deprecated flags as
current with fabricated defaults — the exact defect the ticket reports.
- **Add a docs-vs-argparse parity test (parse the `.mdx` `<ParamField>`
defaults and compare to the parser).** Genuinely tempting, because it is the
only thing that would stop this recurring, and it is not tautological — it
asserts the document against the code, not the code against itself. Rejected
as scope: it needs a stable convention across every backend and component
reference page, and building that harness inside a four-item docs-correction
ticket would make the change unreviewable. Worth a follow-up issue; called
out in the pull-request `## Notes`.
- **Sweep the `ttft:` / `itl:` aliases in the Planner guides.** Rejected — see
Non-goals. They are still supported and used consistently.
- **Regenerate the page with a generator.** Rejected: this page has no
generator and no `GENERATED … do not edit` marker; it is hand-authored, and
`docs/fern/AGENTS.md`'s generated-reference rules do not apply to it.
## Validation strategy
Docs-only, one `.mdx` file, no behavior change. Hardware required: none beyond
the sandbox — no GPU, no Docker socket, no Kubernetes. The recipes below are
sized to that and no larger; recipes `07`/`08`/`13`–`16` (engine smoke),
`09` (GPU pytest), `10` (perf), `02` (Rust), `06` (Docker), `11` (Go), and
`12` (Helm) are all `N/A` because the change touches none of those surfaces and
makes no runtime or performance claim.
- **`00-dynamo-editable-install`** — mandatory idempotent preflight, because the
evidence below imports `dynamo.vllm.backend_args`. Recorded as the
short-circuit confirmation; `compute-env.md` states the editable install is
already built.
- **`01-python-lint`** — the executable core of the ladder. Section 1's
`py_compile` is `N/A` for a `.mdx` file; Section 2 is the real check:
`pre-commit run --files docs/fern/pages/reference/backends/vllm-configuration.mdx
--hook-stage manual`. This is not a formality — the docs hooks that match this
path (`check-asset-paths` on `^docs/fern/`, plus `codespell`,
`end-of-file-fixer`, `trailing-whitespace`, `mixed-line-ending`) are exactly
the deterministic subset the Dynamo Docs Bot enforces pre-merge. Pass
condition: all hooks green, exit 0.
- **`05-code-inspection`** — carries the correctness evidence, which is the
claim that actually matters here and which no linter can reach. Two things go
in it. First, an **executed** default dump rather than a hand trace: build
the parser from `dynamo.vllm.backend_args` in the validation venv and print
each benchmark flag's `default`, so every `default="…"` value written into
the page is backed by a recorded command's output rather than by my reading
of L244–L419. Second, the genuinely-inspection part: re-check #12979's
provider state at validation time (`gh pr view 12979 --json state,mergedAt`)
and confirm the disclosure in our pull-request description still matches
reality — if it merged in the interim, that is a material finding the
reviewer needs, and the correct response is to reduce our diff to the
`--benchmark-timeout` hunk rather than publish a conflicting rewrite.
No new test. `learnings/no-tautological-tests.md` applies directly:
`components/src/dynamo/vllm/tests/test_backend_args.py` already covers the
legacy-alias resolution, the explicit-conflict error, and the points-file path,
and a test asserting that argparse returns the defaults argparse declares would
prove nothing about the document. `03-python-unit-tests-mocker` is therefore
**not** nominated — there is no Python behavior change for it to guard, and
nominating it would create a recipe that can only ever be recorded as a
no-op regression run.
```validation-recipes
00-dynamo-editable-install
01-python-lint
05-code-inspection
```
## Required deliverables
The standard packets for `debug-investigation`: this `plan.md`, then
`change.md`, `change.diff`, `change-validation.md`, and `review.md`.
Specific to this work item:
- `change.diff` touches exactly one file,
`docs/fern/pages/reference/backends/vllm-configuration.mdx`. Any other path
in the diff is out of scope and should be questioned.
- `change.md` and the pull-request description must both account for **all
four** reported items — the one fixed, and the three shown to be already
correct on `main` with their file:line evidence and the note that they are
`release/1.4.0`-only (with #12983 carrying them there). A reader of DYN-3803
must not be left wondering whether three items were overlooked.
- The pull-request description must link
<https://github.com/ai-dynamo/dynamo/pull/12979>, state the overlap, and name
the `--benchmark-timeout` default (`300` → `900`) and its soft-limit
semantics as the part #12979 does not carry.
- `## Notes` should record the follow-up worth filing: these hand-maintained
reference pages have no parity gate against their source modules, which is
the root cause of all four items.
- Commits signed off (`git commit --signoff`); the pull request opened
ready for review, never as a draft, against Base branch `main` on the
Working branch `fix/dyn-3803-doc-inconsistencies--0a7dfd23f70a`. |
Contributor
Author
change.md# change.md — DYN-3803: four user-doc inconsistencies with the shipped implementation
Branch: `fix/dyn-3803-doc-inconsistencies--0a7dfd23f70a`, cut from `main` at
`9710d35fb8582863cf441fb8d02681f0d75db394`.
Commit: **`6b987089d458cbd8ce1fd18701ff324f827b981a`**
— `docs(vllm): document current benchmark sampling flags and fix the timeout`.
One commit, signed off; `%ae` and the `Signed-off-by:` trailer both read
`svc-glamr@nvidia.com` (verified with
`git log -1 --format='%ae | %(trailers:key=Signed-off-by,valueonly)'`).
Nothing was pushed anywhere. `git remote -v` shows only the clone's `origin`
(`https://github.com/ai-dynamo/dynamo`) and no command in this run wrote to it.
## Files touched
Exactly one, as the plan requires.
`docs/fern/pages/reference/backends/vllm-configuration.mdx` (+69 / −11) — the
`## Benchmarking` section now documents the six sampling flags that the shipped
parser actually exposes, `--benchmark-timeout` carries its real default and its
real (soft) semantics, and all five `--benchmark-*-granularity` flags have moved
into the existing `## Deprecated` section with `deprecated={true}` and their
replacement mapping.
`git diff main...HEAD` contains one `diff --git` header. `change.diff` was
checked with `git apply --check` against `main` and applies cleanly.
## All four ticket items, accounted for
The ticket reports four defects. Only one of them is still wrong on `main`; the
other three are `release/1.4.0`-only and were already correct on the Base branch
before this change. I re-verified each on this checkout rather than taking the
plan's word for it. A reader of DYN-3803 should not be left wondering whether
three items were missed — they were triaged, and the evidence is below.
### Item 1 — metric names the implementation never emits. Already correct on `main`. No diff.
The 1.4.0 page named `dynamo_planner_engine_prefill_requests_per_second` /
`…_decode_requests_per_second`. Those do not exist. The gauges are built at
`components/src/dynamo/planner/core/base.py:560` and `:563` from
`pm.engine_prefill_capacity_requests_per_second` and
`pm.engine_decode_capacity_requests_per_second`.
`docs/fern/pages/developer-guide/knowledge-base/modular-components/planner/overview.md:263`
on `main` already names both `…_capacity_requests_per_second` forms. Nothing to fix here.
### Item 2 — inverted CA-bundle description. Already correct on `main`. No diff.
1.4.0 said the bundle was a "no-op unless SSL verification is enabled"; the code
says the opposite. `components/src/dynamo/planner/monitoring/traffic_metrics.py:120`
constructs `PrometheusConnect(url=url, disable_ssl=not ssl_verify)`, and line 128
then assigns `self.prom._session.verify = ca_bundle` unconditionally — overwriting
the `ssl_verify`-derived value in both directions, so the bundle applies
regardless. Both `main` pages already say exactly that:
`docs/fern/pages/reference/components/planner-configuration.mdx:301` and
`docs/fern/pages/reference/kubernetes-api/dynamo-graph-deployment-request.mdx:468`
("the bundle is used for verification regardless of
`metric_pulling_prometheus_ssl_verify`"). Fixed upstream by merged PR #12408.
### Item 3 — CLI examples that do not parse. Already correct on `main`. No diff.
1.4.0's planner examples passed `--load-predictor`, `--kalman-q-level`,
`--prophet-window-size` and five more. `components/src/dynamo/planner/__main__.py:59-64`
builds `argparse.ArgumentParser(description="Dynamo Planner")` with exactly one
argument — `--config`, `required=True` — so every one of those flags is rejected
at startup. `main`'s
`docs/fern/pages/developer-guide/knowledge-base/modular-components/planner/planner-examples.md:17`
already shows the supported form, `python -m dynamo.planner --config planner.yaml`,
and a repo-wide grep for `load-predictor|kalman-q-level|prophet-window-size`
under the `main` planner docs returns nothing. Nothing to fix here.
### Item 4 — deprecated parameters presented as current. STILL WRONG on `main`. This is the diff.
Before this change, `## Benchmarking` documented three deprecated flags as
current options with fabricated defaults: `--benchmark-prefill-granularity`
(`default="16"`), `--benchmark-decode-length-granularity` (`default="6"`), and
`--benchmark-decode-batch-granularity` (`default="6"`). All three are registered
in the `legacy_sampling_flags` tuple at
`components/src/dynamo/vllm/backend_args.py:352-390` with `default=None` and help
text reading "Deprecated compatibility option; use {replacement}". Two further
legacy flags — `--benchmark-prefill-kv-read-granularity` and
`--benchmark-prefill-batch-granularity` — were not on the page at all, so a
reader migrating off them had nowhere to look. The six flags that actually
control sampling today were entirely undocumented.
I also carried the drift the plan flagged as a fourth-item extension:
`--benchmark-timeout` was documented as `default="300"` and as a hard limit
("Worker startup fails if this limit is exceeded"). `backend_args.py:411-419`
sets `default=900` and documents a *soft* limit — the measured iteration
finishes, partial results are returned, engine startup continues, and only a
bounded cleanup grace fails closed if nothing was written.
## Defaults are executed evidence, not a hand trace
The plan supplied a table of flags and defaults from reading L244–L419. I did not
write any `default="…"` from that table. I built the real parser in the
validation venv and dumped `action.default` for every benchmark-related flag,
with `DYN_BENCHMARK*` / `DYN_PREFILL*` / `DYN_DECODE*` / `DYN_PREFIX*` cleared
from the environment first so no ambient variable could shadow a default:
```
/opt/dynamo/venv/bin/python -c '...DynamoVllmArgGroup().add_arguments(parser)...'
--benchmark-mode default=None
--benchmark-points-file default=None
--prefill-max-new-token-samples default=64
--prefill-max-kv-read-token-samples default=16
--decode-max-kv-read-token-samples default=128
--decode-max-batch-size-samples default=128
--prefix-max-batch-size-samples default=3
--benchmark-prefill-granularity default=None
--benchmark-prefill-kv-read-granularity default=None
--benchmark-prefill-batch-granularity default=None
--benchmark-decode-length-granularity default=None
--benchmark-decode-batch-granularity default=None
--benchmark-warmup-iterations default=5
--benchmark-output-path default='/tmp/benchmark_results.json'
--benchmark-timeout default=900
```
**Source agreed with the plan's table on every row**, including the `900`
timeout. There is nothing to report as a source-wins override. (Note for the
validator: `DynamoVllmArgGroup.add_arguments` is an *instance* method taking
`parser`; my first attempt called it on the class and raised `TypeError`. The
recorded command instantiates the class.)
Behavioral claims in the new prose are grounded the same way, not invented:
- The 1–1024 accepted range and the `ValueError` on a value outside it:
`backend_args.py:549-552`.
- "Combining a legacy flag with its replacement raises":
`backend_args.py:555-560` — `if replacement_explicit or replacement_value !=
replacement_default: raise ValueError(f"cannot combine --{…} with --{…}")`.
The message I quote in the `<Warning>` is that f-string's output for the
decode-length pair.
- "A legacy value of `1` maps to `2` for the four uniform-sampling limits, but
is carried across unchanged for `--benchmark-prefill-batch-granularity`":
the `needs_endpoints` boolean in the `mappings` tuple (`backend_args.py:510-540`)
is `True` for four entries and `False` for `benchmark_prefill_batch_granularity`,
and line 561 applies `max(2, legacy_value)` only when it is set.
- "All five are ignored when `--benchmark-points-file` is set":
`_resolve_legacy_benchmark_sampling` returns immediately when
`self._benchmark_points is not None` (`backend_args.py:507-508`).
- "Setting `--benchmark-points-file` without `--benchmark-mode` raises":
`backend_args.py:501-502`.
- The "must be at least 2" / "must be positive" minimums:
`_validate_benchmark_sampling`, `backend_args.py:574-591`.
- The axis description in the new lead paragraph (CUDA-graph capture-size
boundaries, power-of-two KV ladders, uniform selection retaining endpoints) is
paraphrased from the `--benchmark-mode` help text at `backend_args.py:232-240`
and the per-flag help strings, not composed freehand.
## Lint — what I ran and what actually happened
Real results, including the failure.
**`pre-commit run --files docs/…/vllm-configuration.mdx` → exit 0.** Green.
`codespell`, `check for case conflicts`, `check for merge conflicts`,
`mixed line ending`, `trim trailing whitespace`, `Check docs asset paths are
ones Fern rewrites`, and `Check canonical AI Simulate docs are synced to Fern`
all passed; the Python/Rust hooks skipped with "no files to check", which is
correct for a lone `.mdx`.
**Same command with `--hook-stage manual` → exit 0.** No additional failures.
(First measurement of the plain run was piped through `tail`, so the `$?` I saw
was `tail`'s. I re-ran it redirecting to a file to read the real exit code —
worth knowing before anyone trusts a piped exit status here.)
**`bash docs/fern/scripts/check_reference.sh` → exit 1. FAILED — and it fails
identically on unmodified `main`.** Step 4/5 ("absolute reference hrefs match
the nav") dies with `StopIteration` from an inline Python snippet, and step 5/5
skips with "fern CLI not found". I confirmed this is not mine by stashing my
change and re-running on clean `main` content: same `StopIteration`, same
`CHECK FAILED`, same exit 1. Per `learnings/format-changed-only.md` a failure on
lines I did not touch is not mine to fix, so I left it. I separately confirmed
the file I edited contains no absolute (`](/…` or `href="/…`) reference links at
all, so the failing check has no bearing on this diff either way.
No formatter was run across the tree. The only file written is the one in the
diff.
## Style constraints actually inspected
Checked by grep against the edited file, not assumed: the `---` frontmatter with
its `2025-2026` SPDX header and `title`/`subtitle` is intact; there is no body
`# H1` (the only `#` lines are the two SPDX comments inside the frontmatter);
no `> [!NOTE]`-style blockquote admonitions — the new callout is a Fern
`<Warning>`; both code fences in the file remain `bash`-tagged and I added no
new fence; no `TODO`, NVBug, JIRA, or `DYN-` identifier appears in the shipped
text; the backend is spelled **vLLM** everywhere except inside identifiers
(`DYN_VLLM_*`, `dynamo.vllm`, `--use-vllm-tokenizer`), which is correct. The new
entries follow the page's own `<ParamField path="…" type="…" default="…">` shape,
with `deprecated={true}` on the five demoted flags, matching the existing
`--model-express-url` entry and `sglang-configuration.mdx:143`.
## I tried X, it didn't work, fell back to Y
**Anchor links in the deprecation entries.** I first wrote each "use `X` instead"
as a Markdown link to the replacement's on-page anchor —
`[\`--prefill-max-new-token-samples\`](#--prefill-max-new-token-samples)`. Then I
grepped for precedent and found that after my edit the *only* five `](#--`
occurrences in all of `docs/fern/pages/` were my own; every other on-page anchor
in the docs (e.g. `global-router-configuration.mdx:196`,
`metric-labels.mdx:33`) targets a heading, never a `ParamField`. I could not
confirm that Fern emits an addressable `id` for a `ParamField`, and the `fern`
CLI is not installed here so `check_reference.sh`'s broken-links step cannot tell
me. Rather than ship five links that might all be dead, I replaced them with
plain code spans. The reader still gets the exact replacement flag name, and the
page has no link that cannot be verified. Someone who knows Fern's `ParamField`
rendering could upgrade these to real anchors later.
**Scope held.** No test added: `components/src/dynamo/vllm/tests/test_backend_args.py`
already covers legacy-alias resolution and the combine-raises path, and a test
asserting that argparse returns the defaults argparse declares is the tautology
`learnings/no-tautological-tests.md` warns about. I did not touch the ttft/itl
aliases, `autoscaling.md`, any `release/1.4.0` file, or build a docs-vs-argparse
parity harness — all explicit non-goals.
## Overlap with open PR #12979 — still live, re-checked at print time
`gh pr view 12979 --repo ai-dynamo/dynamo --json state,mergedAt` returns
`state: OPEN`, `mergedAt: null` as of this run, so the plan's disclosure
requirement stands unchanged. #12979
(<https://github.com/ai-dynamo/dynamo/pull/12979>, branch
`dagil-nvidia/fix-6572846-planner-docs`) touches the same single file and covers
the same three moves — documenting the current sampling flags, demoting the
legacy aliases, leaving mode/warmup/output-path alone. **It does not carry the
`--benchmark-timeout` correction** (`300` → `900`, hard limit → soft limit),
which is an independent defect of the same class.
The pull-request description must link #12979 by URL, state the overlap plainly,
name the timeout hunk as the delta, and invite the maintainer to take whichever
is more convenient — merging #12979 and cherry-picking the timeout hunk is an
explicitly acceptable outcome. A maintainer must never have to discover on their
own that two open requests touch the same file. That is the publisher's text to
write; flagging it here so it does not get lost.
## Follow-up worth filing (for `## Notes`)
The root cause of all four items is structural, not four independent slips.
Unlike `pages/reference/api/` and `pages/reference/kubernetes-api/`, which have
generators and a `--check` pre-merge gate, the backend and component reference
pages are hand-maintained with no parity check against their source modules — so
a rename or a default change in `backend_args.py` leaves the page silently stale.
This page even opens by claiming "Every field, type, default, and choice on this
page comes from the `DynamoVllmArgGroup` and `DynamoVllmConfig` definitions",
which nothing enforces. A parity harness is the only thing that would stop this
recurring; it was rejected here as scope (it needs a stable convention across
every reference page) and is worth a follow-up issue.
MR URL: https://github.com/ai-dynamo/dynamo/pull/13036 |
Contributor
Author
change-validation.md# Validation — DYN-3803 (vLLM configuration reference: benchmark sampling flags and timeout)
Change under validation: one file, `docs/fern/pages/reference/backends/vllm-configuration.mdx`,
+69/−11, at HEAD `6b987089d458cbd8ce1fd18701ff324f827b981a`.
Hardware target: none beyond the CPU side of the sandbox. The change is documentation-only,
but the *claims* it makes are claims about a Python argument parser and a config object, so
every claim below is checked by executing that parser and that config object rather than by
reading them. The sandbox's A100 was not needed and was not used.
Recipes run: exactly the three the plan nominates — `00-dynamo-editable-install`,
`01-python-lint`, `05-code-inspection`. No others were run, nominated, or substituted.
## What the recorded logs prove
### Recipe 0 — the import path is the work-item checkout, not the dev image
`compute-env.md` states the editable install was already built pre-agency, so Recipe 0 is an
idempotent confirmation rather than a build. I recorded it anyway, and extended it with an
import-provenance print, because every later claim rests on *which* `backend_args` module the
interpreter actually loads. The log shows `dynamo`, `dynamo._core`, and
`dynamo.vllm.backend_args` all importing, with `backend_args.__file__` resolving into
`/home/sandbox/workspace/wi-20260811T165051Z-unscoped/repo/components/src/dynamo/vllm/`.
No `PYTHONPATH` substitution was used. Exit 0.
That print is load-bearing: without it, a green defaults dump would be consistent with having
introspected the dev image's pre-agency copy of the parser, which would prove nothing about
this checkout.
### Recipe 1 — lint
Section 1 (`py_compile`) is genuinely `N/A`: the diff touches no Python file. Section 2 is the
real check — `pre-commit run --files docs/fern/pages/reference/backends/vllm-configuration.mdx
--hook-stage manual`. Every applicable hook reports Passed; hooks with no matching files report
Skipped as expected for a single-`.mdx` file list. Exit 0.
### Recipe 5 — correctness evidence
This recipe carries the substance, in five recorded sections.
**Section A — executed default dump, compared row by row against the page.**
I built the real parser (`DynamoVllmArgGroup().add_arguments(parser)` — instance, not class;
the printer's `change.md` reported hitting a `TypeError` by calling it on the class, and I
avoided that) and dumped `action.default` for every benchmark-related flag. Ambient
`DYN_BENCHMARK*` / `DYN_PREFILL*` / `DYN_DECODE*` / `DYN_PREFIX*` environment variables were
cleared first so no shell-inherited value could shadow a declared default; the log records that
none were present.
**All 15 documented benchmark `<ParamField>` defaults equal the parser's own default**, with
no exceptions:
- `--benchmark-points-file` null, `--prefill-max-new-token-samples` 64,
`--prefill-max-kv-read-token-samples` 16, `--decode-max-kv-read-token-samples` 128,
`--decode-max-batch-size-samples` 128, `--prefix-max-batch-size-samples` 3
- `--benchmark-timeout` 900 (the value the ticket says the page had wrong)
- the five `--benchmark-*-granularity` legacy flags, all `None`
I checked two axes beyond what was asked, because a defaults-only check can be green on a page
that is still wrong:
- **Coverage** — every flag the parser registers appears on the page. The log reads
`none — every registered benchmark flag is documented`. This catches sins of omission, which
a per-row default comparison structurally cannot.
- **Deprecation polarity** — the page's `deprecated={true}` marks agree with the source help
text on all 15 rows. The five granularity flags are marked deprecated and their help says
"Deprecated compatibility option"; the other ten are marked neither.
Exit 0.
**Section A-negative — the same harness against the pre-change page.**
The harness takes the `.mdx` path as an argument, so I pointed it at
`git show main:docs/fern/pages/reference/backends/vllm-configuration.mdx`. It exits **1** with
15 mismatches, reproducing the reported defect exactly: `--benchmark-prefill-granularity`
doc=16 / parser=None, `--benchmark-decode-length-granularity` doc=6 / parser=None,
`--benchmark-decode-batch-granularity` doc=6 / parser=None, `--benchmark-timeout` doc=300 /
parser=900, eight parser flags entirely undocumented, and three deprecation-polarity
inversions.
This is the negative control, and it is the answer to the question
`learnings/parser-behavior-runtime-proof.md` insists on — *if I revert the change and re-run
the identical command, do I get a different result?* Yes: exit 0 against the new page, exit 1
against the old one, same harness, same parser, same invocation. The green result in Section A
is therefore discriminating rather than vacuous.
**Section B — PR #12979 provider state, re-checked at validation time.**
At `2026-08-11T18:02:33Z`: `state: OPEN`, `mergedAt: null`, `isDraft: false`,
`reviewDecision: REVIEW_REQUIRED`, author `dagil-nvidia`, branch
`dagil-nvidia/fix-6572846-planner-docs`, touching the same one file, +57/−9. It has **not**
merged in the interim. The plan's overlap disclosure therefore still stands as written, and no
reduction of this diff to the timeout hunk is warranted. Had it merged, that would have been a
material finding for the reviewer; it did not. Exit 0.
**Section C — the prose claims, executed.**
Numbers are the easy part; the diff also adds sentences asserting runtime *behavior*. I
exercised 16 such claims through the real `DynamoVllmConfig`, all of which held:
- The `<Warning>`'s quoted error string is the actual string. The page quotes
`cannot combine --benchmark-decode-length-granularity with --decode-max-kv-read-token-samples`;
the resolver raises a `ValueError` whose `str()` equals that, character for character. I
assert both that the exception is raised *and* that the quoted text appears on the page, so
the two cannot drift apart silently.
- Setting the replacement explicitly to its own default still raises — the page says "passing a
legacy flag together with its replacement", and the explicit-flag path is the non-obvious half
of that.
- The stated 1..1024 range holds at the boundaries: 0 rejected, 1025 rejected, 1024 accepted.
- The 1→2 mapping holds for all four uniform limits, and `--benchmark-prefill-batch-granularity`
carries 1 across unchanged as 1 — which is exactly the asymmetry the page calls out as "the
one legacy flag whose replacement does not sample uniformly".
- The documented legacy→replacement mapping equals the resolver's mapping. I derived the source
side **empirically** rather than reading the `mappings` tuple: set one legacy flag, run the
resolver, assert exactly one replacement attribute moved, and build the mapping from what
actually moved. Comparing the page against a hand-read table would only have proved I can read
a tuple; comparing it against observed movement proves the page matches behavior.
- A loaded points file makes the legacy flags a no-op, and a points file without
`--benchmark-mode` raises. Both are sentences the diff adds.
- The "must be at least 2" / "must be positive" minimums raise as documented, including
`--benchmark-timeout must be positive`.
Exit 0.
**Section D — the soft-timeout semantics.**
The single claim not reachable through the config object is the rewrite of `--benchmark-timeout`
from a hard limit ("Worker startup fails if this limit is exceeded") to a soft one. That is a
statement about `worker_factory` and the instrumented scheduler, so I executed the five in-tree
tests that govern it: partial-result acceptance on timeout, the bounded post-timeout grace,
stopping after saving the current point, checking the deadline before the next point, and the
synchronizer sharing the stop decision. 5 passed in 14.28s. The page's "the iteration being
measured finishes, the partial results collected so far are returned, and engine startup
continues. A bounded cleanup grace still fails closed if no result is written at all" is a fair
reading of what those tests assert. Exit 0.
**Section E — parallel pattern, scope, and house style.**
No *other* file under `docs/`, `examples/`, `recipes/`, or `README.md` still presents a
granularity flag as current or restates the old 300-second timeout — the only occurrences
anywhere are the deprecation entries on the edited page itself, so this class of staleness is
not left half-fixed elsewhere. The diff is exactly one file. Frontmatter is present with SPDX,
there is no body `# H1` (counting the body only — the SPDX lines are `#` comments *inside* the
YAML frontmatter and must not be counted), no GitHub-style blockquote admonitions in a Fern
page, no ticket IDs or TODOs left in the prose, code fences are tagged, backend casing is
correct, no unverifiable on-page flag anchors, and `<ParamField>` / `<Warning>` tags balance
27/27 and 2/2. Exit 0.
## Honest notes on the record
Two rows in the registry for `05-code-inspection` are red (exit 127 and exit 1). **Both are bugs
in my own Section E harness, not defects in the change**, and I am flagging them rather than
leaving the reviewer to infer it:
- exit 127 — I wrote Section E inline inside `bash -lc '…'` and the nested quoting broke; the
shell mangled several `grep` invocations. It also caused a stray 2-byte file `\[!"` to be
created in the checkout by an accidental redirect. I removed that file and confirmed
`git status --porcelain` is empty and HEAD is still `6b987089d`; the printer's branch and
every tracked file are untouched.
- exit 1 — after moving Section E into a script file, my H1 check counted the SPDX `#` comment
lines *inside* the YAML frontmatter as body H1s, and the stray file above was still present.
Both were harness faults. Fixed, re-recorded, exit 0.
I am leaving the failed rows in the registry rather than hiding them. The most recent run per
recipe is what customs uses, and all three are green; the redundant recorded evidence is the
point of the recorder.
Two other things I deliberately did **not** do. I did not re-run `docs/fern/scripts/check_reference.sh`
as a recipe: the printer reported it fails identically on unmodified `main` (StopIteration in
step 4/5, fern CLI absent in 5/5), and a pre-existing failure must not be attributed to this
change or added to the ladder. And I did not nominate Recipe 3 — the plan's reasoning that there
is no Python behavior change for it to guard is correct on the diff I read.
## Investigation outcome:
The ticket's item 4 was that this page documented benchmark sampling flags that no longer exist
under those names and a `--benchmark-timeout` default of 300 that the parser had already moved to
900. Section A-negative reproduces that original symptom mechanically against the pre-change
page: 15 mismatches, including the exact four defects named, plus eight flags the page never
mentioned at all.
The fix is documentation-only and the diff is confined to the one page. After it, the identical
harness reports every documented default equal to the executed parser's default, complete
coverage of the registered benchmark flags, and correct deprecation polarity — and the prose the
diff adds is not merely plausible but executed: the quoted error message is the real message, the
documented legacy→replacement mapping equals the mapping the resolver actually performs, the
stated numeric range holds at both boundaries, the 1→2 versus 1→1 asymmetry is real, and the
hard→soft timeout rewrite is backed by five passing in-tree tests.
The one item outside the sandbox's reach is the provider-side overlap: PR #12979 edits the same
file and is still OPEN and unmerged as of validation time, so the plan's disclosure remains
accurate and the eventual rebase conflict is a publisher/reviewer concern rather than a
correctness defect here. Nothing I ran contradicts any claim the change makes, and the negative
control confirms the checks would have caught it if something did.
## Verdict: pass |
Contributor
Author
review.md> 🤖 **Automated AI review — advisory.** An AI agent's judgment of
> whether this change is logically sound on the evidence; not a merge
> authorization. CI, customs, and a human reviewer hold that.
## Assessment: sound
The change is documentation-only, confined to the one file the plan required, and every
value and behavioural sentence it adds is a faithful restatement of what
`components/src/dynamo/vllm/backend_args.py` and
`components/src/dynamo/vllm/instrumented_scheduler.py` actually do in this checkout. I
verified that independently of the validator's dump, reading the source rather than
trusting the recorded log. The evidence table is complete against the plan and its rows
show real execution, including a discriminating negative control.
Two nits below. Neither is a correctness defect and neither blocks.
## What I verified independently
**Scope.** `change.diff` carries exactly one `diff --git` header, for
`docs/fern/pages/reference/backends/vllm-configuration.mdx`. In the checkout,
`git diff --name-only main...HEAD` returns that one path, `git status --porcelain` is
empty, and HEAD is `6b987089d458cbd8ce1fd18701ff324f827b981a` on branch
`fix/dyn-3803-doc-inconsistencies--0a7dfd23f70a` — matching `change.md` exactly, and not
the upstream default branch. No drive-by edits, nothing touching `release/1.4.0`, no
parity harness — the plan's non-goals hold.
**Documented values, against source.** Every `default="…"` the diff writes matches the
parser:
| Flag | Page | `backend_args.py` |
|---|---|---|
| `--benchmark-points-file` | null | L244 `default=None` |
| `--prefill-max-new-token-samples` | 64 | L259 `default=64` |
| `--prefill-max-kv-read-token-samples` | 16 | L273 `default=16` |
| `--decode-max-kv-read-token-samples` | 128 | L288 `default=128` |
| `--decode-max-batch-size-samples` | 128 | L302 `default=128` |
| `--prefix-max-batch-size-samples` | 3 | L316 `default=3` |
| `--benchmark-timeout` | 900 | L411 `default=900` |
| five `--benchmark-*-granularity` | null | L352–390, all `default=None` |
**Behavioural prose, against source.** The five legacy→replacement pairs the page states
are the `mappings` tuple's pairs, one for one. The `<Warning>`'s quoted string at
`vllm-configuration.mdx:266` is exactly what
`f"cannot combine --{legacy_name.replace('_','-')} with --{replacement_name.replace('_','-')}"`
renders for the decode-length pair. The `1..1024` range and the `ValueError` outside it are
the literal guard in `_resolve_legacy_benchmark_sampling`. The stated `1 → 2` mapping for
the four uniform limits and the `1 → 1` carry-through for
`--benchmark-prefill-batch-granularity` are `mapped_value = max(2, legacy_value) if
needs_endpoints else legacy_value`, with `needs_endpoints` false only for that one flag —
so the asymmetry the page calls out at line 281 is real, not asserted. The
"`[1, 2, 4]` when all three are legal" claim at line 229 matches
`instrumented_scheduler.py:2474`, which head-slices
`sorted(set(presets))[: prefix_max_batch_size_samples]`. The soft-limit rewrite at line 247
is a fair restatement of the L411 help text ("the current measured iteration finishes,
partial results are returned, and engine startup continues. A bounded cleanup grace still
fails closed if no result is written"). "Setting it without `--benchmark-mode` raises a
`ValueError`" matches `_load_explicit_benchmark_points`. The "must be at least 2" /
"must be positive" minimums match `_validate_benchmark_sampling`.
**Style.** Frontmatter and its SPDX header (`2025-2026`) survive; the body still opens at
`##` with no `# H1`; the new admonition is a Fern `<Warning>`, correct for `.mdx` and
consistent with the existing `<Warning>` at line 79; `deprecated={true}` follows the
in-file `--model-express-url` precedent and `sglang-configuration.mdx:143`; both code
fences are untouched and ```bash-tagged; backend spelled vLLM; no ticket ID, TODO, or
unverifiable on-page flag anchor introduced.
**Compute pathway.** The change is documentation about a Python parser, and every recorded
recipe ran locally on the CPU pathway in this sandbox against the work-item checkout. No
recipe required a GPU, and nothing in the logs reaches for SSH, Slurm, or remote compute.
That matches `compute-env.md`.
**Evidence table.** `evidence report` returns `## Evidence [3/3 validated]` and
`customs: cleared [3/3 validated]`. All three recipes the plan's `validation-recipes` block
names — `00-dynamo-editable-install`, `01-python-lint`, `05-code-inspection` — are present;
none is `missing`. Walking each cited log:
- **00** — prints `backend_args OK at
…/wi-20260811T165051Z-unscoped/repo/components/src/dynamo/vllm/backend_args.py`. That
provenance print is what makes the later introspection meaningful: it shows the
interpreter loaded this checkout's parser, not a pre-agency copy in the dev image.
- **01** — full `pre-commit` hook roster over the changed `.mdx`, `pre-commit exit: 0`,
applicable hooks Passed and non-matching hooks Skipped as expected for a one-file list.
- **05** — the final green row's log shows the Section E assertions running against the
changed page with a clean tree at `6b987089d`. The Section A-negative log is a genuine
negative control: the same harness pointed at `git show main:…` exits 1 with the three
granularity default mismatches (16/6/6 vs `None`), timeout 300 vs 900, eight
undocumented parser flags, and three deprecation-polarity inversions. Same harness, same
parser, opposite result — so the green row is discriminating, not vacuous.
The two red `05-code-inspection` rows the validator disclosed are its own harness faults,
and the logs bear that out rather than merely asserting it: the exit-127 log shows shell
mangling (`grep: ": No such file or directory`, `syntax error near unexpected token '('`)
from nested quoting inside `bash -lc`, and the exit-1 log shows an H1 miscount that counted
the two SPDX `#` comment lines inside the YAML frontmatter plus a stray untracked file from
an errant redirect. Neither red row reports anything about the change itself. The final
green run shows the stray file gone and `git status --porcelain` empty. No `validated` row
here is green-but-vacuous, so no missing `failing` disposition is owed.
The change adds no test and no new infrastructure, so there is nothing here that could
assert the implementation back at itself — the `no-tautological-tests` concern does not
arise, and the Section D evidence leans on five pre-existing in-tree tests rather than
tests written to match the prose.
**The three no-diff items.** `change.md` accounts for all four ticket items with
`file:line` evidence, three of them as already-correct-on-`main`. I spot-checked all three
rather than one: `planner/overview.md:263` already names both
`engine_prefill_capacity_requests_per_second` and its decode twin, matching
`planner/core/base.py`; `planner-configuration.mdx:301` already says the CA bundle is used
"regardless of `metric_pulling_prometheus_ssl_verify`", matching the unconditional
`self.prom._session.verify = ca_bundle` in `traffic_metrics.py`; and a repo-wide grep for
`--load-predictor`, `--kalman-q-level`, and `--prophet-window-size` across `docs/`,
`examples/`, and `recipes/` returns nothing, consistent with `planner/__main__.py`
registering only `--config`.
## Findings
**1. `docs/fern/pages/reference/backends/vllm-configuration.mdx:186` — nit.**
The new lead paragraph states without qualification that, for "the flags below", "points
are selected uniformly across the sorted axis while the endpoints are retained". That is
true of four of the five limits but not of `--prefix-max-batch-size-samples`, which
`instrumented_scheduler.py:2474` implements as a head slice —
`sorted(set(presets))[: prefix_max_batch_size_samples]` — and which therefore drops the
feasible maximum rather than retaining it. Line 229 does correct this eight rows later
("Unlike the limits above, this one keeps the first N values … rather than sampling
uniformly"), so the page is not self-contradictory, but a reader who takes the section
preamble at face value would expect `--prefix-max-batch-size-samples 3` on an axis of
`[1, 2, 4, 8, 13]` to retain `13`, when in fact it yields `[1, 2, 4]`. Naming the exception
in the preamble would close the gap.
**2. `docs/fern/pages/reference/backends/vllm-configuration.mdx:262` — nit.**
The new paragraph opens "Each is translated to its replacement at startup, with a
deprecation warning", eight lines after the section's own standing preamble at line 254
already says "Each is mapped to its replacement at startup with a deprecation warning".
The consequence is small but real: within one screen the `## Deprecated` section states the
same rule twice in near-identical words, which invites the reader to hunt for a distinction
that is not there, and leaves two places to update if the mechanism ever changes. The
sentence's genuinely new content — the `1..1024` range, the `ValueError`, and the `1 → 2`
endpoint rule — stands on its own without the restatement. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
WalkthroughThe vLLM benchmark configuration reference adds engine-derived sampling controls, explicit benchmark point files, soft-timeout behavior, and deprecated granularity aliases with validation and compatibility mappings. ChangesvLLM benchmark configuration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 264-266: Replace the <Warning> wrapper around the legacy-flag
guidance with the GitHub-style > [!WARNING] admonition syntax, preserving the
existing warning text and migration guidance unchanged.
- Line 262: Update the documentation around _resolve_legacy_benchmark_sampling
to qualify legacy flag translation, deprecation warnings, and 1–1024 validation:
these behaviors apply only when --benchmark-mode is set and no explicit points
file is selected. Clarify that without those conditions, the early return
prevents this validation and mapping.
🪄 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: a21c40e4-e494-4783-bece-a89ff548d8b2
📒 Files selected for processing (1)
docs/fern/pages/reference/backends/vllm-configuration.mdx
Contributor
Author
|
CI result: passed All required checks on this pull request reached a terminal state: 15 successful, 6 skipped, 0 failed. Combined commit status Passing checks include This head includes repair round 1 ( |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
The legacy --benchmark-*-granularity section stated that translation, deprecation warnings, and 1-1024 range validation happen unconditionally at startup. _resolve_legacy_benchmark_sampling returns early when benchmark_mode is None or an explicit points file was loaded, so none of that runs in those cases. Qualify the text accordingly. Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com>
glamr-agent
temporarily deployed
to
external_collaborator
August 11, 2026 19:10 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
Babysitter repair round 1 — head CI on the previous head Addressed
Commit: Checks run: |
dagil-nvidia
approved these changes
Aug 11, 2026
dagil-nvidia
left a comment
dagil-nvidia
left a comment
Collaborator
There was a problem hiding this comment.
Approving as a `dynamo-docs` CODEOWNER.
Structural lint is clean, and the central factual claim checks out against the PR head: `components/src/dynamo/vllm/backend_args.py:467` declares `benchmark_timeout: int = 900`, so the `300` this replaces was wrong.
On the overlap this PR flags with #12979: taking this one. It is a superset. Both document the current sampling flags and demote the five `--benchmark-*-granularity` aliases, but only this one carries the `--benchmark-timeout` correction, which is an independent defect of the same class and would otherwise need a follow-up. One PR on one file is the cleaner outcome, so #12979 will be closed rather than merged.
Thanks for calling out the collision in the description instead of leaving a reviewer to find it.
Collaborator
|
/ok to test a2be262 |
dagil-nvidia
added a commit
that referenced
this pull request
Aug 15, 2026
…rence, recipe ID fix Fix the helm-docs source template's docs link (the generated README was fixed but generate-helm-docs regenerated the old link, dirtying the operator check). Adopt main's refreshed vLLM benchmark sampling-flag reference (#13036 — same shipped flag set, code-verified prose) and the Qwen3.8 recipe model-ID correction (#13265). Triaged all main docs commits since the last snapshot point: #13192 and #11723 docs excluded (features absent from 1.4.0); release-side #13280/#13256 have no docs impact (AIC-core wheel is a Spica dependency; planner shim wording verified still correct). Signed-off-by: Dan Gil <dagil@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview:
The
## Benchmarkingsection of the vLLM configuration reference documented three--benchmark-*-granularityflags as current options with defaults (16,6,6) thatthe shipped parser does not set, while the six flags that actually control benchmark
sampling today were undocumented.
--benchmark-timeoutwas also documented with thewrong default and the wrong semantics. This updates the page to match
components/src/dynamo/vllm/backend_args.py.Documentation only — one file, no code or behaviour changes.
Details:
docs/fern/pages/reference/backends/vllm-configuration.mdx(+69 / −11):--benchmark-points-file,--prefill-max-new-token-samples(64),--prefill-max-kv-read-token-samples(16),--decode-max-kv-read-token-samples(128),--decode-max-batch-size-samples(128),and
--prefix-max-batch-size-samples(3). Every default is taken from the parser asbuilt by
DynamoVllmArgGroup.add_arguments, not transcribed by hand.each axis from the engine's own limits and that these flags are per-axis sample
limits over those derived axes — raising one measures more of the same axis rather
than changing it.
--prefix-max-batch-size-samplesis called out as keeping thefirst N values instead of sampling uniformly, matching the head slice in
instrumented_scheduler.py.--benchmark-*-granularityflags move tothe existing
## Deprecatedsection withdeprecated={true}and their replacementmapping. Two of them (
--benchmark-prefill-kv-read-granularity,--benchmark-prefill-batch-granularity) were not on the page at all, so readersmigrating off them had nowhere to look. A
<Warning>records that combining a legacyflag with its replacement raises, quoting the parser's own message.
--benchmark-timeout. Default300→900, and the description changesfrom a hard limit ("worker startup fails") to the soft limit the code implements: the
measured iteration finishes, partial results are returned, engine startup continues,
and only a bounded cleanup grace fails closed if nothing was written.
Overlap with #12979 — please read before merging
#12979 (branch
dagil-nvidia/fix-6572846-planner-docs) is open against this same file and covers threeof the same moves: documenting the current sampling flags, demoting the legacy aliases,
and leaving mode/warmup/output-path alone. It does not carry the
--benchmark-timeoutcorrection, which is an independent defect of the same class.Whichever is more convenient is fine — merging #12979 and cherry-picking the timeout
hunk from here is an entirely acceptable outcome. Flagging it so no reviewer has to
discover the collision themselves.
Not changed
Three related reference-page defects reported alongside this one were checked against
mainand are already correct there (planner capacity metric names, the CA-bundledescription, and the planner CLI examples); they were
release/1.4.0-only. Norelease/1.4.0file is touched here.Where should the reviewer start?
docs/fern/pages/reference/backends/vllm-configuration.mdxis the only file. Start withthe
## Benchmarkingsection, then the five demoted entries in## Deprecated. Thedefaults and the behavioural sentences are checkable against
components/src/dynamo/vllm/backend_args.py(flag registration around L244–L419, legacyresolution in
_resolve_legacy_benchmark_sampling) andcomponents/src/dynamo/vllm/instrumented_scheduler.py:2474for the--prefix-max-batch-size-sampleshead-slice behaviour.Related Issues
🚫 This PR is NOT linked to an issue:
Tracked internally; no public GitHub issue exists for this documentation drift.
Summary by CodeRabbit