Repository navigation
fix(vllm): distinguish undeclared RL weight versions - #13041
Merged
MatejKosec merged 20 commits intoSep 19, 2026
Merged
MatejKosec merged 20 commits into
MatejKosec merged 20 commits into
Conversation
glamr-agent
temporarily deployed
to
external_collaborator
August 11, 2026 19:07 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
glamr-agent
temporarily deployed
to
external_collaborator
August 11, 2026 19:07 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
github-actions
Bot
added
fix
external-contribution
Contributor
Author
Automated evidence record — validation completeValidation status: complete Evidence summary: [3/3 validated] AI review assessment (advisory only — not a human review): sound. An automated Validation result: pass — all three planned recipes ran green in-sandbox; the regression Evidence audit: complete [3/3 validated] — the evidence table is grounded in recorded runs. Evidence [3/3 validated]Generated from validation/registry.jsonl — do not edit by hand.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
Author
plan.md# Plan — DYN-3771: RL weight-version surface stays at `"initial"` when the update bypasses `/engine`
Route: implementation
Template: debug-investigation
Engine: vllm
There is no `Disposition:` line: discovery found no merged change that resolves this, and the
behavior is present on both `main` and `release/1.4.0` today (evidence below). There is no
`Review request:` line either — `input.md` records none, and the prior-art survey found no open
PR that is attempting this fix, so there is nothing to defer to or extend.
## User intent
The Slack thread behind this work item asks for three things: triage DYN-3771, decide whether
the fix belongs to Dynamo at all (as opposed to upstream vLLM or the RL framework that drives
the rollout loop), and propose the fix if it does. The issue title states the defect precisely:
weight-version tracking assumes every weight update travels through Dynamo's `/engine` admin
API, so an RL path that loads weights by another route leaves the version surface reporting
`"initial"` forever. Three candidate fixes were offered with the assignment — (A) update the
version where the engine actually loads weights, so tracking is path-independent; (B) provide a
version-declaration endpoint decoupled from the update path; (C) distinguish "never updated"
from "updated to a value that happens to be the initial one".
I could not read the Linear ticket. `agenthub-linear` is not on PATH in this sandbox
(`agenthub-linear: command not found`), and `input.md` records "Issue body: none available;
Issue comments: none available". Everything below is grounded in code I actually read in the
checkout at `/home/sandbox/workspace/wi-20260811T170521Z-unscoped/repo`, plus GitHub queries via
`gh`. I have not invented ticket content.
## Non-goals
This work item does not change how weights are actually loaded, does not add a new weight
transport (IPC, tensor, checkpoint-engine), and does not implement
`update_weights_from_tensor`, which remains an explicit `not implemented` error at
`components/src/dynamo/vllm/handlers.py:1848-1860`. It does not touch the SGLang backend: the
engine for this item is vLLM, and SGLang's parallel surface
(`components/src/dynamo/sglang/request_handlers/handler_base.py:846-871`) is referenced only as
internal precedent. It does not attempt to police or intercept direct
`AsyncLLM.collective_rpc(...)` calls made by an embedding RL framework — see the ownership
analysis for why that is out of reach. It does not renumber, rename, or repurpose the existing
`version` field returned on the happy path; preserving that is the negative control.
## Discovery
**The version surface, end to end.** I read `components/src/dynamo/vllm/handlers.py` in full
around the RL admin block. The entire weight-version concept is a single Python attribute on
Dynamo's own handler: `self._weight_version: str = "initial"` is set in
`BaseWorkerHandler.__init__` at line 1145. It is read in exactly one place, the admin route
`get_weight_version` at lines 1725-1734, which returns
`{"status": "ok", "version": getattr(self, "_weight_version", "initial")}`. It is written in
exactly two places: `update_weights_from_disk` (lines 1736-1782, assignment after the
`collective_rpc` and `reset_prefix_cache` succeed) and `update_weights_from_distributed`
(lines 1784-1846, assignment at line 1836). I grepped the whole component for other writers and
found none. So the read surface reports a value that only two Dynamo code paths can ever move.
**How the surface is published.** `components/src/dynamo/vllm/worker_factory.py:1560-1617`
builds the `rl_routes` dict — `get_weight_version`, `update_weights_from_disk`,
`update_weights_from_distributed`, `init_weights_update_group`, `destroy_weights_update_group`,
`pause_generation`, `resume_generation`, `liveness_probe` — and hands it to
`register_rl_routes(runtime, handler.rl_route_registry, rl_routes, enable_dispatch=...)` in
`components/src/dynamo/common/rl/admin.py`. That helper registers each name as a worker-local
`/engine/<name>` route and records it in the `RLRouteRegistry`, which is what
`GET /v1/rl/workers` advertises as the worker's `routes` list. A new route is therefore a
two-line addition that automatically becomes discoverable — no new plumbing.
**Whether the bypass path exists in Dynamo.** It does not, and this is the load-bearing finding.
The issue names `update_weights_from_ipc`. That symbol exists only in Dynamo's SGLang backend
(`handler_base.py:846`, registered as `control/update_weights_from_ipc`); there is no such
method anywhere in the vLLM component. The vLLM analogue of "bypass" is simply a caller holding
the `AsyncLLM` object and invoking `collective_rpc(...)` itself. And even inside Dynamo's own
update handlers, the method that actually loads the weights is named by the *caller* at call
time: `rpc = body.get("engine_rpc", "reload_weights")` in `update_weights_from_disk` and
`rpc = body.get("engine_rpc", "update_weights_from_path")` in `update_weights_from_distributed`.
The method itself lives in a caller-supplied `--worker-extension-cls`; the repo's own example of
one is `tests/rl/weight_update_worker.py`, whose `DynamoRLTestWorkerExtension` implements
`update_weights_from_path`, `init_broadcaster`, `destroy_broadcaster`, and `liveness_probe`.
Dynamo does not own, wrap, or even know the name of the code that puts weights into GPU memory.
**Upstream vLLM has no such concept.** I resolved the installed tree with
`python3 -c "import vllm, os; print(os.path.dirname(vllm.__file__))"` →
`/usr/local/lib/python3.12/dist-packages/vllm`, then
`grep -rln "weight_version" .../vllm/ | wc -l` → `0` against vLLM 0.22.0. There is nothing
upstream to fix; the abstraction is Dynamo's invention.
**Prior art.** `gh pr list --state all --search "DYN-3771"` returns `[]`. Semantic searches on
`weight_version`, `get_weight_version`, and `update_weights_from` surfaced only non-overlapping
work: #11289 (OPEN, prime-rl vLLM generate path) touches `handlers.py` but its only
`_weight_version` hit is `handler._weight_version = "initial"` added to the `_make_handler()`
fixture in `test_vllm_sleep_wake_handlers.py` — a test-double change, with the production lines
appearing unchanged as diff context; #12856 (OPEN, Slime example) and #8553/#6094/#10094 are
SGLang-flavored; #11701 (OPEN) is ModelExpress docs; #9131, #9382, #8630, #6436, #9214 are
closed prime-rl attempts. Blame on the vLLM component
(`git log --oneline -12 -S "_weight_version" -- components/src/dynamo/vllm/`) gives
`81f7f89c9 feat(RL): add RL worker admin routes (#9680)` as the origin, with `337b55fe1` (#11692)
only bounding the init-group timeout.
**The behavior is live on both branches.** The issue is tagged `release/1.4.0`, so I checked
whether `main` had already drifted. It has not: `main` shows the code quoted above, and reading
`components/src/dynamo/vllm/handlers.py` at `release/1.4.0` (`62dfee5b04e9…`) through `gh api`
shows the identical shape — `self._weight_version: str = "initial"` at line 1035,
`get_weight_version` at 1606-1615, and the two write sites at 1654 and 1717. No
`Disposition: already-resolved`.
**Existing tests.**
`components/src/dynamo/vllm/tests/test_vllm_worker_handler.py` — **RUN.** Marked
`pre_merge, unit, vllm, gpu_0, multimodal`, no GPU required. It already carries the RL admin
block: `test_admin_rejects_non_dict_body`, `test_distributed_update_can_match_async_rl_semantics`
(asserting `resp == {"status": "ok", "version": 7}`), `test_distributed_update_rejects_unpaused_cache_reset`,
`test_distributed_update_preserves_safe_defaults`, and five `init_weights_update_group` cases.
This is both the home for the new regression test and the source of the negative control.
`components/src/dynamo/common/tests/test_rl_admin.py` — **RUN.** Registry add/describe/dispatch;
cheap, and it guards the route-advertisement path a new route flows through.
`tests/rl/test_worker_discovery.py` — **IGNORE for in-sandbox execution, but must not be
broken.** It is `post_merge, e2e, vllm, gpu_1`, loads Qwen3-0.6B, and runs with a 900 s timeout;
it asserts the advertised `routes` superset and that after a real
`update_weights_from_disk` with `"weight_version": "smoke_v1"` the read returns
`version.get("version", version.get("weight_version")) == "smoke_v1"`. That assertion is the
e2e statement of the negative control.
`components/src/dynamo/sglang/tests/test_engine_routes.py` — **IGNORE.** Out of engine scope.
## Reproducible symptom
Start a vLLM worker with `--enable-rl` and a `--worker-extension-cls`. Without issuing any
update, `POST /engine/get_weight_version` on the worker's system port returns
`{"status": "ok", "version": "initial"}`. Now have the RL framework load new weights by any path
that does not traverse Dynamo's two update handlers — a direct `collective_rpc` on the engine it
already holds, or a checkpoint-engine style transport. The weights in GPU memory change; the
same `get_weight_version` call still returns `{"status": "ok", "version": "initial"}`. A reader
of that surface — the orchestration layer deciding whether a rollout worker is on the current
policy — cannot distinguish "this worker has never been updated" from "this worker was updated
and the tracker missed it", and cannot even distinguish either of those from "this worker was
legitimately updated to a version tag whose value is the string `initial`", because the update
handlers accept whatever string arrives in `body["weight_version"]` and write it verbatim.
## Ranked hypotheses
| # | Hypothesis | Smallest distinguishing observation | Verdict |
|---|---|---|---|
| 1 | The version surface is Dynamo-owned state that only Dynamo's `/engine` update handlers ever write, so any other load path silently desynchronizes it. | Grep the vLLM component for writers of `_weight_version`: exactly two, both in `handlers.py` (1782-region and 1836), both inside `/engine` handlers. | **Confirmed — root cause.** |
| 2 | The sentinel `"initial"` is a plain string in the same value space as caller-supplied tags, so "never updated" is not a distinguishable state even in principle. | `get_weight_version` (1725-1734) returns only `version`; the update handlers do `version = body.get("weight_version", "unknown")` with no validation, so a caller may legitimately declare `"initial"`. | **Confirmed — the part that is fixable in-process.** |
| 3 | Dynamo could hook the point where weights actually land and update the version there (candidate A). | The loading method is named by the caller (`body.get("engine_rpc", ...)`) and defined in a caller-supplied `--worker-extension-cls` (see `tests/rl/weight_update_worker.py`); a framework holding `AsyncLLM` can call `collective_rpc` with no Dynamo frame on the stack. | **Refuted — no hook point exists inside Dynamo.** |
| 4 | This is an upstream vLLM gap. | `grep -rln "weight_version"` across installed vLLM 0.22.0 → `0` files. | **Refuted — vLLM has no version concept to fix.** |
| 5 | The named bypass `update_weights_from_ipc` is a vLLM-path route that forgot to update the version. | That symbol appears only in `sglang/request_handlers/handler_base.py:846`; the vLLM component has no such method. | **Refuted — the issue's example is SGLang-flavored; the vLLM bypass is direct `collective_rpc`.** |
| 6 | A fix already landed and the issue is stale against `main`. | `gh pr list --state all --search "DYN-3771"` → `[]`; `handlers.py` at both `main` and `release/1.4.0` shows the same three sites. | **Refuted — not already resolved.** |
## Ownership verdict
**The defect is Dynamo's, and the fixable part of it is narrower than the title suggests.**
The weight-version abstraction exists nowhere but in Dynamo: Dynamo declares the state, Dynamo
writes it, Dynamo publishes the reader route, and Dynamo advertises that route in
`/v1/rl/workers` as a capability RL orchestrators are invited to rely on. Upstream vLLM has no
`weight_version` at all, so there is no upstream fix to request. The RL caller is not at fault
either for calling `collective_rpc` directly — Dynamo never promised to intercept that, and
cannot.
What Dynamo *is* answerable for is that it publishes a version surface whose contract is
unstated and whose failure mode is silent. Today the surface asserts a version with the same
confidence whether or not it has any basis for the assertion. That is the Dynamo-side bug, and
it is fixable without any cooperation from the caller.
## Chosen approach
**Option (C) as the primary fix, with option (B) as its necessary complement. Option (A) is
rejected as not implementable inside Dynamo.**
(C) makes the surface honest. `get_weight_version` should distinguish a worker whose version has
never been declared from one that has — an explicit never-updated signal carried alongside the
version, rather than a sentinel string smuggled into the same field as caller data. Concretely:
initialize the tracked version to a value that is *not* a legal caller value (i.e. absent), and
have `get_weight_version` report both the version and whether it was ever set, while continuing
to emit `version` on the happy path exactly as it does now. This is what turns a silent wrong
answer into a legible "unknown", which is the only thing an orchestrator can actually act on.
(B) is the complement, because (C) alone leaves a bypassing caller with no supported way to
become correct — it can only observe that Dynamo doesn't know. A declaration route
(`set_weight_version`-shaped: accept a version string, write it, return it) closes the loop: a
caller that loads weights outside Dynamo declares what it loaded, and the surface becomes right
again. This is not a new invention in this repo — Dynamo's SGLang backend already ships exactly
this as `control/update_weight_version`
(`handler_base.py`, writing `tokenizer_manager.server_args.weight_version`). Adding the vLLM
counterpart is an internal-consistency argument, and it registers through the same
`register_rl_routes` call in `worker_factory.py:1560-1617`, so it appears in `/v1/rl/workers`
`routes` automatically and is discoverable by exactly the mechanism RL clients already use.
Scope of the change: `components/src/dynamo/vllm/handlers.py` (initialization, `get_weight_version`,
the new declaration handler, and the shared assignment used by both existing update handlers so
all three writers go through one place); `components/src/dynamo/vllm/worker_factory.py` (register
and advertise the new route); the new and updated tests; and
`docs/fern/pages/use-cases/reinforcement-learning/implementation-guide.md`, which documents the
pause/update/resume curl cycle, the `/v1/rl/workers` response shape, and the route list — any
change to the version surface or the route inventory must be reflected there per `docs/AGENTS.md`.
## Rejected alternatives
**(A) Update the version where the engine actually loads weights.** This is the right shape in
the abstract and is exactly what the issue title implies, but there is no such point inside
Dynamo. The load executes in a worker-extension method whose name is supplied by the caller in
the request body and whose implementation is supplied by the caller's own class; and the bypass
the issue is about does not enter Dynamo at all. Implementing (A) would require either monkey-
patching a caller-supplied class or wrapping `AsyncLLM.collective_rpc` to sniff method names —
both fragile, both silently wrong for any method name Dynamo hasn't heard of, and both an
overreach into the engine boundary that Dynamo deliberately sits above. Rejected on
implementability, not on desirability.
**Do nothing / escalate upstream.** Refuted by hypothesis 4: vLLM has no `weight_version`
concept, so there is no upstream owner.
**A no-change incident report.** The template is explicit that the workflow publishes a real
diff and that an incident report is not an implementation work item. Even granting that the
bypass itself is outside Dynamo's reach, (C)+(B) is a real, small, defensible Dynamo-side change,
so this route is not taken.
**Validating anything on GPU.** The defect is a state-machine honesty bug in a Python handler.
Loading Qwen3-0.6B onto the A100 to watch a string change would cost minutes and add no
evidentiary value over a unit test that constructs the handler directly. See the recipe
rationale.
## Validation strategy
**The regression test that fails for the original behavior.** Add a case to
`components/src/dynamo/vllm/tests/test_vllm_worker_handler.py`, next to the existing RL admin
block, asserting the behavioral distinction rather than any internal attribute name: a freshly
constructed handler's `get_weight_version({})` reports *never declared*; after a successful
`update_weights_from_distributed` (or `update_weights_from_disk`) declaring a version, the same
call reports that version; and — the sharp edge — a handler that has legitimately been updated
to the literal version string `"initial"` is distinguishable from one that was never updated at
all. Against today's code, the first and third assertions fail: both states return
`{"status": "ok", "version": "initial"}` and are indistinguishable to any caller. That failure
is the proof the test is not tautological — it discriminates the pre-fix behavior from the
post-fix behavior on the public response, not on a private field.
A second, smaller case covers (B): calling the new declaration route with a version string makes
`get_weight_version` report that version and flip the never-declared signal, without touching
the engine — asserted by the engine mock recording no `collective_rpc`. Against today's code this
fails at import/dispatch, because the route does not exist.
**The negative control that must remain unchanged.** `test_distributed_update_can_match_async_rl_semantics`
asserts `resp == {"status": "ok", "version": 7}` for a normal declared update. That assertion
must survive the change untouched — the happy-path `version` key keeps its exact meaning and
shape, and the never-declared signal is additive. The e2e statement of the same control is
`tests/rl/test_worker_discovery.py`, which after a real `update_weights_from_disk` asserts
`version.get("version", version.get("weight_version")) == "smoke_v1"` and asserts the advertised
`routes` list is a *superset* of the known names (so adding a route cannot break it). That file
is `post_merge`/`gpu_1`/900 s and is not run in this sandbox; it is named here so the printer
knows not to perturb its contract, and so the reviewer can check by inspection that the response
shape it depends on is preserved. `test_distributed_update_rejects_unpaused_cache_reset` and
`test_distributed_update_preserves_safe_defaults` are secondary controls: the pause/reserved-key
guards around the update handlers must be unaffected by refactoring the three writers onto one
assignment path.
**Recipes.** `00-dynamo-editable-install` is the precondition — the change is in the Python
component, and the unit tests import `dynamo.vllm.handlers` directly, so the editable install
must succeed before anything else is meaningful. `01-python-lint` covers the pre-commit surface
for a pure-Python diff plus the docs edit. `03-python-unit-tests-mocker` is the one that actually
proves the fix: it executes the new regression cases and the negative controls, all `gpu_0`, all
`pre_merge`, in seconds. A recipe that runs beats one that reads, and this change is fully
provable by execution.
Deliberately not nominated: `02-rust-cargo-check` (no Rust in the diff — the version surface is
entirely Python; `lib/rl` carries no weight-version state); `04-python-runtime-lint` (no runtime-
binding change); `05-code-inspection` (the sandbox can execute the proof, so inspection would be
a weaker substitute for evidence already available); `06-dockerfile-build` (**forbidden — no
Docker socket in this environment**, per `compute-env.md`); `07-agg-smoke`, `08-disagg-pair-smoke`,
`09-gpu-pytest`, `10-perf-benchmark` (GPU time for a handler-state change buys no evidence the
unit tests don't already give, and there is no performance dimension); `11-go-operator-tests` and
`12-helm-chart-render` (no Go, no chart changes; envtest may be blocked here anyway);
`13`–`16` (SGLang and TensorRT-LLM smokes — wrong engine).
```validation-recipes
00-dynamo-editable-install
01-python-lint
03-python-unit-tests-mocker
```
## Required deliverables
The printer produces `change.md` and `change.diff` on branch
`fix/weight-version-tracking-rl-bypass--80e17fc79aac` off `main`, containing: the (C) change to
initialization and `get_weight_version` in `components/src/dynamo/vllm/handlers.py`; the (B)
declaration route in the same file with its registration in
`components/src/dynamo/vllm/worker_factory.py`; the consolidation of the three version writers
onto a single assignment path; the new regression cases in
`components/src/dynamo/vllm/tests/test_vllm_worker_handler.py` with the existing RL admin tests
left untouched as negative controls; and the corresponding update to
`docs/fern/pages/use-cases/reinforcement-learning/implementation-guide.md` following
`docs/AGENTS.md` (SPDX header preserved, Fern frontmatter, no body `# H1`, backend casing
"vLLM"). Commits are DCO-signed; the PR title follows Conventional Commits, e.g.
`fix(vllm): distinguish undeclared weight version from a declared one`, with `Summary` and
`Validation` sections.
The validator produces `change-validation.md` with a recorded run for each of the three recipes
above, and must record the regression test failing against the pre-change tree — empirically, by
running it — as the demonstration that it discriminates. The reviewer produces `review.md`,
checking in particular that the happy-path `version` field's shape is unchanged (so the e2e
discovery test's contract holds), that the new route is advertised through `RLRouteRegistry` and
therefore visible in `/v1/rl/workers`, and that the documented route list matches the registered
one. |
Contributor
Author
change.md# change.md — DYN-3771: RL weight-version surface stays at `"initial"` when the update bypasses `/engine`
Branch: `fix/weight-version-tracking-rl-bypass--80e17fc79aac`, created off `main` at
`3aa5a199b fix(snapshot): bound CUDA helper cancellation (#12936)`.
Commit: **`656d0de3b694a23ac4197baa7eff4c56f292fcb0`** —
`fix(vllm): distinguish undeclared weight version from a declared one`.
Single commit, DCO-signed. Nothing pushed anywhere.
Implements the plan's chosen approach: **(C) as the primary fix with (B) as its complement**,
with (A) rejected as not implementable inside Dynamo. 4 files, +186/-5.
## Reproduction before the change (empirical, not asserted)
I wrote the regression tests first and ran them against the unmodified tree, before touching any
production code. All five failed:
```
FAILED ...::test_get_weight_version_reports_undeclared_before_any_update
FAILED ...::test_get_weight_version_reports_declared_version_after_update
FAILED ...::test_declared_initial_is_distinguishable_from_never_declared
FAILED ...::test_set_weight_version_declares_without_touching_the_engine
FAILED ...::test_set_weight_version_rejects_a_missing_version
5 failed, 74 deselected in 7.81s
```
The failure output is the symptom itself. For the "never declared" case pytest printed
`undeclared_resp = {'status': 'ok', 'version': 'initial'}` — the pre-change surface asserting a
version with no basis for it, and the *same* response a handler legitimately updated to the tag
`"initial"` produces. The two `set_weight_version` cases failed with
`AttributeError: 'DecodeWorkerHandler' object has no attribute 'set_weight_version'. Did you mean:
'get_weight_version'?`, i.e. the (B) route genuinely did not exist.
This is the evidence that the tests discriminate pre-fix from post-fix behavior rather than
asserting the new implementation. They assert only on the public response dict from
`get_weight_version` / `set_weight_version`; none of them reads `_weight_version` or any other
private attribute, so a revert of the production change makes them fail again (and did).
## Per-file summary
**`components/src/dynamo/vllm/handlers.py`** — the substance of the change.
- Added module-level `_WEIGHT_VERSION_UNDECLARED: Final = object()` next to the existing
`_DISTRIBUTED_WEIGHT_UPDATE_RESERVED_KEYS`. It is a bare object, not a string, specifically so
it cannot collide with anything a caller can put in `body["weight_version"]` — that collision
is the bug. The comment above it records why.
- `BaseWorkerHandler.__init__`: `self._weight_version: str = "initial"` becomes
`self._weight_version: Any = _WEIGHT_VERSION_UNDECLARED`. The annotation had to widen from `str`
because the sentinel is not a `str` — and note the field was never really `str`-typed in
practice anyway, since the update handlers write `body.get("weight_version", ...)` verbatim and
the existing negative-control test declares the integer `7`.
- `get_weight_version` now returns `{"status": "ok", "version": <v-or-None>, "version_declared":
<bool>}`. The happy-path `version` key keeps its exact meaning and shape; `version_declared` is
strictly additive, and `version` is `None` only in the never-declared state, which previously
could not be expressed at all.
- New `set_weight_version` route (option B): validates the body is a dict, requires
`weight_version` to be present, records it, returns `{"status": "ok", "version": version}`. It
deliberately touches neither the engine nor the pause lock nor the prefix cache — it declares,
it does not load. Modeled on SGLang's `control/update_weight_version` in
`components/src/dynamo/sglang/request_handlers/handler_base.py:858`, but keeping vLLM's
`{"status": ...}` response convention rather than SGLang's `{"success": ...}`.
- New private `_declare_weight_version(version)` is now the single assignment path. The two
existing writers in `update_weights_from_disk` and `update_weights_from_distributed` call it
instead of assigning `self._weight_version` directly, as does the new route. This is the plan's
consolidation item; I kept it to a one-line body so it does not change any ordering relative to
`collective_rpc` / `reset_prefix_cache` — both call sites still assign at exactly the point they
did before, still inside the `try` and still under `_pause_lock`.
**`components/src/dynamo/vllm/worker_factory.py`** — one line: `"set_weight_version":
handler.set_weight_version` added to the `rl_routes` dict. I read
`components/src/dynamo/common/rl/admin.py` to confirm nothing else is needed:
`register_rl_routes` iterates the dict, calls `runtime.register_engine_route(name, handler)` for
each, and (when `enable_dispatch`) calls `registry.add_route(name, handler)`. So the new route is
both callable at `/engine/set_weight_version` and advertised in the `routes` list that
`GET /v1/rl/workers` returns, with no new plumbing. The existing `logger.info` there prints
`", ".join(sorted(rl_routes))`, so it picks the new name up for free.
**`components/src/dynamo/vllm/tests/test_vllm_worker_handler.py`** — five new cases appended
inside the existing `TestRLAdminRouteHardening` class, placed after
`test_distributed_update_preserves_safe_defaults` and before the `init_weights_update_group`
block, plus two small `@staticmethod` helpers (`_make_rl_handler`, `_declare_via_update`) that
follow the setup the neighbouring tests already do by hand. The cases:
never-declared reports `version_declared is False` and `version is None`; a declared version is
reported back; a handler declared to the literal `"initial"` is `!=` a never-declared one (the
sharp edge); `set_weight_version` moves the reported version and the engine mock records no
`collective_rpc` and no `reset_prefix_cache`; and a `set_weight_version` with no `weight_version`
is rejected *and* leaves `version_declared` false, so a rejected declaration cannot half-apply.
**`docs/fern/pages/use-cases/reinforcement-learning/implementation-guide.md`** — added a
`### Read and Declare the Weight Version` subsection under "Call vLLM Engine Routes Directly",
covering the `get_weight_version` response shape, the instruction to branch on `version_declared`
rather than comparing `version` against a placeholder string, a `> [!WARNING]` admonition stating
plainly that the worker reports the last *declared* version rather than what is in GPU memory
(the honest statement of this issue's limitation), and the `set_weight_version` request/response.
Also added `"set_weight_version"` to the example `routes` array in the `/v1/rl/workers` response
so the documented route list matches the registered one. SPDX header and Fern frontmatter
untouched, no body `# H1`, GitHub-style admonition matching the two already in the file, "vLLM"
casing preserved.
## Negative controls — left untouched, verified still passing
I did not modify a single existing test. `test_distributed_update_can_match_async_rl_semantics`
still asserts `resp == {"status": "ok", "version": 7}` verbatim and passes, which is the
statement that the happy-path response shape is unchanged.
`test_distributed_update_rejects_unpaused_cache_reset` and
`test_distributed_update_preserves_safe_defaults` guard the pause/reserved-key behavior around the
writers I consolidated; both pass.
`tests/rl/test_worker_discovery.py` is `post_merge`/`gpu_1`/900 s and was **not** run here (per
the plan). I checked its contract by inspection instead: at line 245 it asserts the advertised
route set is a `.issubset(routes)` *superset* check, so adding `set_weight_version` cannot break
it; at line 281 it asserts `version.get("version", version.get("weight_version")) == "smoke_v1"`
after a real `update_weights_from_disk`, and since `version` is still present and still carries
the declared tag on that path, that assertion still holds. The only way I could have broken it is
by making `version` absent or renaming it, and I did neither.
## What I actually ran
| Command | Result |
|---|---|
| `pytest components/src/dynamo/vllm/tests/test_vllm_worker_handler.py` (pre-change, new tests only) | **5 failed** — the reproduction, quoted above |
| `pytest components/src/dynamo/vllm/tests/test_vllm_worker_handler.py -q` | **74 passed, 5 skipped** (5 skips are pre-existing, marked "Need to revisit tests" at the top of the file) |
| `pytest components/src/dynamo/common/tests/test_rl_admin.py -q` | **8 passed** |
| `pytest components/src/dynamo/vllm/tests/ -q` (whole component) | **838 passed, 8 failed, 6 skipped** |
| `pre-commit run --files <4 changed files>` | **all hooks passed** (isort, black, flake8, codespell, ruff, the Fern docs hooks, pytest-marker report; clang-format and the JSON/TOML/YAML hooks skipped as no matching files) |
| `git apply --check change.diff` against `main` | applies cleanly |
All Python invoked as `/opt/dynamo/venv/bin/python`, never system python.
**On the 8 component-wide failures:** they are pre-existing and unrelated. I verified rather than
assumed — `git stash`, re-ran the same command on the untouched tree, got the *same 8 failures*
(833 passed baseline vs 838 with my 5 new tests, so my diff adds 5 passes and 0 failures), then
`git stash pop`. They live in `test_realtime_handler.py`, `test_vllm_kv_events_api.py`, and
`test_vllm_renderer_api.py` — realtime audio, KV-event serialization format, and an engine-core
struct contract. None touches the RL admin surface. Per `learnings/format-changed-only.md` they
are not mine to fix and I left them alone.
## Safety constraints I actually inspected
- **Lock and ordering discipline.** Both existing writers assign the version inside the `try`
after `collective_rpc` (and `reset_prefix_cache` where applicable) succeeds, while still holding
`_pause_lock`. Replacing the assignment with a one-line method call preserves that exactly — the
version is still only recorded on success, so a failed update still cannot advance the reported
version. `set_weight_version` deliberately takes no lock at all, because it performs no engine
operation and races with nothing but itself.
- **`getattr` fallback in the reader.** The original read used
`getattr(self, "_weight_version", "initial")`, tolerating a handler whose `__init__` was bypassed
— which is precisely what the test fixture `_make_handler()` does (`patch.object(...__init__,
return_value=None)`). I kept the `getattr` shape and changed only the default to the sentinel, so
a bypassed-init handler now reads as *undeclared* rather than as falsely "initial". That is what
makes the new tests work against the existing fixture without me having to modify the fixture.
- **Type annotation.** Widening `str` → `Any` is honest rather than lax: the field genuinely holds
arbitrary caller JSON today (the negative control declares the int `7`), so `str` was already
wrong. flake8 and ruff pass.
- **No behavior change for existing callers.** The `version` key is present with identical value
and type on every path that previously produced one. The only observable difference for a
pre-existing caller is the extra `version_declared` key and the fact that a never-updated worker
now says `null` instead of `"initial"` — which is the entire point of the fix, and is the
behavior the issue asks for under option (C).
## Things I considered and did not do
- **Validating `set_weight_version`'s payload type** (rejecting non-string versions). I did not,
deliberately: the two existing update routes accept whatever `body["weight_version"]` holds and
write it verbatim — the negative control depends on an integer `7` round-tripping. Adding
stricter validation on the new route than the old ones would make the three writers inconsistent
for no stated benefit, and tightening the old ones is outside this plan's scope. I do require the
key to be *present*, since a declaration with nothing to declare is meaningless.
- **A `weight_version` alias key in the read response.** `tests/rl/test_worker_discovery.py` reads
`version.get("version", version.get("weight_version"))`, so an alias would be tolerated — but
`version` is present, nothing needs the fallback, and adding a second name for one value is
scope creep.
- **Touching SGLang.** Out of engine scope per the plan's non-goals; referenced only as precedent.
- **`update_weights_from_tensor`.** Still the explicit `not implemented` error it was; untouched.
- **GPU validation.** Not attempted, per the plan — this is a state-machine honesty bug in a Python
handler and the unit tests prove it directly. No GPU was used by this run.
## Honest notes
- **The duplicate DCO trailer, and why the SHA moved.** My first commit was `38cc80020`. I had
written `Signed-off-by: svc-glamr <svc-glamr@nvidia.com>` into the message body *and* passed
`--signoff`; the repo's `prepare-commit-msg` hook then appended its own trailer derived from
`git var GIT_AUTHOR_IDENT`, leaving two `Signed-off-by:` lines with different display names. I
amended to drop my hand-written one, keeping the hook's, which yielded `656d0de3b`. This amends
only my own commit from this iteration — no planner or prior-printer commit was rewritten. Final
state verified: `git log -1 --format='%ae | %(trailers:key=Signed-off-by,valueonly)'` gives
`svc-glamr@nvidia.com | svc-glamr@nvidia.com <svc-glamr@nvidia.com>`, both columns matching.
- **mypy was not run.** I grepped `.pre-commit-config.yaml` for `mypy` and found no hook, so there
is no configured type-check gate on this surface to run. flake8 and ruff both pass on the changed
files.
- **I could not read the Linear ticket**, same as the planner: `agenthub-linear` is not on PATH
here and `input.md` records the issue body and comments as unavailable. I implemented against the
plan's reading of the three options, which is grounded in code, not against ticket prose I never
saw.
- **On option (A).** I agree with the plan's rejection after reading the code myself: the loading
RPC is named by the caller (`body.get("engine_rpc", ...)`) and implemented in a caller-supplied
`--worker-extension-cls`, so there is no in-Dynamo point where "weights landed" is observable.
The `> [!WARNING]` I added to the docs states that limitation to users plainly rather than
papering over it — the fix makes the surface honest about what it can and cannot know, which is
the most Dynamo can do here without overreaching the engine boundary.
## Artifacts
- `change.diff` — `git diff main`, 278 lines, verified with `git apply --check` against `main`.
- Commit `656d0de3b694a23ac4197baa7eff4c56f292fcb0` on
`fix/weight-version-tracking-rl-bypass--80e17fc79aac`. Working tree clean. Not pushed.
MR URL: https://github.com/ai-dynamo/dynamo/pull/13041 |
Contributor
Author
change-validation.md# change-validation.md — DYN-3771: RL weight-version surface stays at `"initial"` when the update bypasses `/engine`
Branch `fix/weight-version-tracking-rl-bypass--80e17fc79aac` at commit
`656d0de3b694a23ac4197baa7eff4c56f292fcb0`, off `main` at `3aa5a199b`. 4 files, +186/-5.
## Hardware target
None. The plan deliberately nominated no GPU recipe and I did not use one, which I agree
with: the defect is a state-machine honesty bug in a pure-Python handler whose entire
subject matter is one attribute and the shape of one response dict. `compute-env.md`
reports a single A100-SXM4-80GB and vLLM 0.22.0 in-sandbox, so a GPU pathway was available
— it simply would have proved nothing the unit tests do not prove more sharply. Everything
below ran locally in the sandbox on the validation venv `/opt/dynamo/venv`, never system
Python, never a remote host.
The three recipes the plan named — `00-dynamo-editable-install`, `01-python-lint`,
`03-python-unit-tests-mocker` — all ran and all resolved green. Recipe 0 ran first, since
the tests import `dynamo.vllm.handlers` directly; it short-circuited as `compute-env.md`
predicted (`dynamo validation env already prepared`) and confirmed
`dynamo._core` resolves to the compiled extension **inside this work-item checkout**
(`.../wi-20260811T170521Z-unscoped/repo/lib/bindings/python/src/dynamo/_core.abi3.so`),
not some other tree. That path check is the part of Recipe 0 that matters here: it is what
makes the later pytest runs evidence about *this* branch rather than about the image.
## The discrimination proof — run, not taken on trust
The plan's sharpest requirement was that I demonstrate empirically, by running it, that the
new regression suite fails against the pre-change tree. The printer reports having done this
before implementing. I did not take that report as evidence; I reproduced it myself.
Method: with the new tests held at HEAD, I reverted **only the two production files** —
`components/src/dynamo/vllm/handlers.py` and `components/src/dynamo/vllm/worker_factory.py`
— to `main`, confirmed the pre-change source was actually in place
(`self._weight_version: str = "initial"` at line 1145; zero occurrences of
`set_weight_version` in either file), ran the five new cases, and restored under an `EXIT`
trap so the tree could not be left perturbed by a failure or a timeout.
All five failed, and the failure text is the bug itself rather than a mere absence:
- `test_get_weight_version_reports_undeclared_before_any_update` — `KeyError:
'version_declared'`, with pytest printing `resp = {'status': 'ok', 'version': 'initial'}`.
That is the pre-change surface asserting a version it has no basis for.
- `test_declared_initial_is_distinguishable_from_never_declared` — the load-bearing one:
`AssertionError: assert {'status': 'ok', 'version': 'initial'} != {'status': 'ok',
'version': 'initial'}`. A handler that was never updated and a handler legitimately
updated to the tag `"initial"` produced *byte-identical* responses. This is the defect
stated as an executable observation: not "the version is wrong" but "two distinct states
are one response, so no caller can tell them apart even in principle."
- `test_get_weight_version_reports_declared_version_after_update` — `KeyError:
'version_declared'` after a successful declared update, i.e. the additive signal is absent
on the happy path too.
- Both `set_weight_version` cases — `AttributeError: 'DecodeWorkerHandler' object has no
attribute 'set_weight_version'. Did you mean: 'get_weight_version'?`. The option-(B)
declaration route genuinely did not exist.
This reproduces the printer's reported reproduction exactly, including the response dicts
and the `AttributeError` wording. Restoring and re-running the same five cases plus the
three named negative controls: **8 passed**.
What this proves, and it is the whole point of the exercise: the tests discriminate the
pre-fix behavior from the post-fix behavior rather than mirroring the implementation. They
assert only on the public response dict returned by `get_weight_version` /
`set_weight_version`; not one of them reads `_weight_version`, `_WEIGHT_VERSION_UNDECLARED`,
or any other private name. Revert the production change and they fail again — I did revert
it, and they did. Per `learnings/no-tautological-tests.md` and the recipe-03 before/after
protocol, both runs are recorded under the same recipe id, so the failing before-proof
stays visible in the evidence trail while the passing after-run is what the recipe resolves
to.
## Negative controls
The three controls the plan named all pass on the changed tree, unmodified:
- `test_distributed_update_can_match_async_rl_semantics` — still asserts
`resp == {"status": "ok", "version": 7}` **verbatim, as an equality on the whole dict**.
This is the strongest single piece of evidence that the happy-path response shape is
untouched: had the change added `version_declared` to the *update* route's response, or
altered `version`'s value or type, a whole-dict equality would have broken. It did not.
Note also that the control declares the integer `7`, which independently justifies the
`str` → `Any` annotation widening — the field never really held only `str`.
- `test_distributed_update_rejects_unpaused_cache_reset` and
`test_distributed_update_preserves_safe_defaults` — the pause-lock and reserved-key guards
around the two writers that were consolidated onto `_declare_weight_version`. Both pass,
so the refactor of three assignment sites onto one path did not disturb the ordering or
the locking discipline they encode.
Full file: `test_vllm_worker_handler.py` + `test_rl_admin.py` → **82 passed, 5 skipped**
(the 5 skips are pre-existing, flagged "Need to revisit tests" at the top of the file).
`test_rl_admin.py` matters here because it guards the registry path the new route flows
through to reach `/v1/rl/workers`.
I did not run `tests/rl/test_worker_discovery.py` — `post_merge` / `gpu_1` / 900 s, out of
scope per the plan. Its contract is the e2e statement of the same negative control, and the
in-sandbox equality assertion above is a proxy for it, not a substitute. A reviewer should
still check by inspection that `version` remains present and unrenamed; I confirm it does
(`handlers.py:1757-1763`).
## The 8 pre-existing failures — verified, not assumed
The printer reports 8 failures elsewhere in the vLLM component and claims they are
pre-existing. Recipe 03's scope reaches them, so I checked rather than relayed the claim.
- Changed tree: **8 failed, 838 passed, 6 skipped**.
- Untouched tree (all four changed files reverted to `main`, same command, same session):
**8 failed, 833 passed, 6 skipped** — the *same eight test ids*.
The delta is exactly +5 passed and +0 failed, which is the five new cases and nothing else.
The eight are genuinely pre-existing and unrelated to this diff: three in
`test_realtime_handler.py` (two `ModuleNotFoundError: No module named 'scipy'` at
`realtime/handler.py:151`, one `asyncio` `TimeoutError`), four in `test_vllm_kv_events_api.py`
(KV-event serialization format assertions), and one in `test_vllm_renderer_api.py`
(`EngineCoreOutput fields changed!`). Two of those are environmental (`scipy` absent from
this image) and the rest are contract assertions against surfaces this change never touches
— no RL admin route, no weight-version state. Per `learnings/format-changed-only.md` they
are not this work item's to fix. I record them here so nobody later reads the component-wide
red as a regression from this branch; it is not, and the paired baseline run is on file.
## Lint
Recipe 01 ran in two recorded parts. `py_compile` on the three changed Python files: exit 0,
silent. `pre-commit run --files <all four changed files> --hook-stage manual`: every
applicable hook `Passed` — isort, black, flake8, codespell, ruff, the case/merge-conflict/
line-ending/whitespace hooks, the pytest-marker report, and the Fern docs hooks that fired
on the guide (`Check canonical AI Simulate docs are synced to Fern`, `Check docs asset paths
are ones Fern rewrites`). The rest reported "no files to check". This covers the docs edit as
well as the code, which matters because `docs/AGENTS.md` compliance is a merge gate on this
repo and the diff touches a Fern page.
## Section 3 — behavioral coverage
Each new test traces through changed production lines rather than asserting a default:
- `test_get_weight_version_reports_undeclared_before_any_update` → `handlers.py:141`
(`_WEIGHT_VERSION_UNDECLARED` sentinel), `handlers.py:1757-1763` (the `declared` branch in
`get_weight_version`), via `handlers.py:1153` (initialization).
- `test_declared_initial_is_distinguishable_from_never_declared` → the same lines plus
`handlers.py:1886` (`_declare_weight_version` at the distributed-update write site). This
is the case that fails on pre-change code by *dict equality*, so it cannot pass vacuously.
- `test_get_weight_version_reports_declared_version_after_update` → `handlers.py:1886` and
`handlers.py:1733-1739` (the consolidated single writer).
- `test_set_weight_version_declares_without_touching_the_engine` → `handlers.py:1765-1786`
(the new route), and asserts the engine mock recorded no `collective_rpc` and no
`reset_prefix_cache` — i.e. it proves the route *declares* without *loading*, which is the
entire semantic distinction option (B) rests on.
- `test_set_weight_version_rejects_a_missing_version` → `handlers.py:1780-1781` (the
missing-key rejection), and additionally asserts `version_declared` is still `False`
afterwards, so a rejected declaration cannot half-apply.
New test infrastructure under both polarities: the diff adds two `@staticmethod` helpers,
`_make_rl_handler` and `_declare_via_update`. They are not merely registered — they are
exercised in both directions and recorded. Positive: the eight-test run where every case
built on them passes. Negative: the same helpers, driving the same assertions against
pre-change production code, produced five failures. That pairing is what shows the helpers
carry real discriminating power rather than constructing a handler that satisfies any
assertion. Separately, the new `set_weight_version` route itself is exercised under both
polarities — an input it must accept (`{"weight_version": "policy-43"}` → `{"status": "ok",
"version": "policy-43"}`) and one it must reject (`{}` → `{"status": "error", ...}`, with the
surface still reporting undeclared).
## Investigation outcome: confirmed and fixed to the extent Dynamo can fix it
The root cause is confirmed empirically, not just by reading. The pre-change
`get_weight_version` returns a value from the same string space callers write into
`body["weight_version"]`, so "never declared" and "declared to the tag `initial`" were the
same response — I watched pytest print the two identical dicts side by side. The plan's
hypotheses 1 and 2 are the confirmed root cause; the run reproduces exactly the symptom
`plan.md` describes.
The fix is honest about its own limits, and I want to be precise about what the recorded
runs do and do not establish. They establish that the surface can now express "I do not
know" (`version: null`, `version_declared: false`), that the state is reachable and distinct,
that a caller which loads weights outside Dynamo has a supported way to make the surface
correct again (`set_weight_version`, which declares without touching the engine), and that
the existing happy-path contract is byte-identical. They do **not** establish that a real RL
framework performing a real bypass load will be correctly reported — nothing can establish
that, because the bypass by construction never enters Dynamo. The plan's hypothesis 3 is
refuted: there is no in-Dynamo hook point, the loading RPC being named by the caller
(`body.get("engine_rpc", ...)`) and implemented in a caller-supplied `--worker-extension-cls`.
So the change converts a silent wrong answer into a legible unknown plus a way for the
caller to fix it. That is the whole of what Dynamo can do here without overreaching the
engine boundary, and the `> [!WARNING]` added to the RL implementation guide states that
limitation to users plainly rather than papering over it. I consider that the correct
disposition rather than a shortfall — but a reviewer should weigh it as a scoping judgement,
since it means the issue's literal title ("tracking assumes updates travel through
`/engine`") remains true of the world after this change; what changes is that the surface no
longer lies about it.
Two things I checked that the recorded runs do not by themselves show. The route
registration is one line in `worker_factory.py:1591` inside the `rl_routes` dict that
`register_rl_routes` iterates, so `/engine/set_weight_version` is both callable and
advertised in `/v1/rl/workers` with no new plumbing — but no recorded run of mine actually
issues an HTTP call to that route or reads the advertised route list, because doing so needs
a live worker. That is a genuine gap in my evidence, narrowed but not closed by
`test_rl_admin.py` passing (it covers registry add/describe/dispatch generically) and by the
e2e discovery test's route assertion being a `.issubset` superset check that an added route
cannot break. I flag it for the reviewer as an inspection item rather than claiming it as
proven. Second, the docs' `routes` example array now lists `set_weight_version`, matching the
registered dict — checked by reading both, not by execution.
## Checkout state
I left the checkout exactly as I found it. Both temporary manipulations (the production-file
revert for the before-proof, the four-file revert for the baseline comparison) ran under an
`EXIT` trap that restores from HEAD unconditionally, and both printed their post-restore
state into the recorded log. Verified after each and again at the end:
`git rev-parse HEAD` → `656d0de3b694a23ac4197baa7eff4c56f292fcb0`, branch
`fix/weight-version-tracking-rl-bypass--80e17fc79aac`, `git status --short` empty,
`git diff --stat main` unchanged at 4 files / +186 / -5. Nothing was pushed; no commit was
created, amended, or rewritten.
## Verdict: pass — all three planned recipes ran green in-sandbox; the regression suite was run and shown to fail on the pre-change tree and pass on the fixed one, with the negative controls intact |
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
DYN-3771: the vLLM RL weight-version surface reported `"initial"` for a worker whose
weights were loaded by any path that does not traverse Dynamo's `/engine` update handlers.
The change makes the surface able to say "I do not know" (option C) and gives a bypassing
caller a supported way to become correct again (option B). I audited the merged result at
`656d0de3b` on `fix/weight-version-tracking-rl-bypass--80e17fc79aac`, working tree clean,
4 files / +186 / -5, base `3aa5a199b` which is the tip of `main` in this checkout — the
base is not stale and there is nothing staleness-related to report.
The change is logically coherent, the regression tests discriminate rather than mirror,
the three planned recipes all ran and all show real execution against the changed tree,
and the diff stays inside the plan's non-goals. Two nits below; neither blocks.
### The plan's three named reviewer checks
**1. The happy-path `version` field's shape is unchanged — confirmed.**
Both update routes still return a two-key dict: `handlers.py:1827`
(`update_weights_from_disk`) and `handlers.py:1891` (`update_weights_from_distributed`)
each `return {"status": "ok", "version": version}`, byte-identical to `main`. The diff
touches neither return. The `version_declared` key is added only to `get_weight_version`'s
response (`handlers.py:1759-1763`), and `version` is still present there and still carries
the declared tag verbatim. The e2e contract at `tests/rl/test_worker_discovery.py:281` —
`version.get("version", version.get("weight_version")) == "smoke_v1"` — is read *after* a
real `update_weights_from_disk`, so `declared` is true at `handlers.py:1758` and `version`
resolves to `"smoke_v1"`. The assertion holds. The whole-dict equality control
`test_distributed_update_can_match_async_rl_semantics` (`resp == {"status": "ok",
"version": 7}`) is untouched and passing, which is the strongest available in-sandbox
statement of the same thing: had `version_declared` leaked into the update route's
response, or had `version`'s value or type moved, a whole-dict equality would have broken.
**2. The new route is advertised through `RLRouteRegistry` — confirmed.**
`worker_factory.py:1591` adds `"set_weight_version": handler.set_weight_version` to the
`rl_routes` dict, which is passed wholesale to `register_rl_routes` at
`worker_factory.py:1605-1610`. `admin.py:163-166` iterates that mapping with no per-route
conditional, calling `runtime.register_engine_route(name, handler)` for every entry and
`registry.add_route(name, handler)` when `enable_dispatch`; `admin.py:108-112` builds the
advertised list as `sorted(self.routes)` over exactly that registry. So the route is both
callable at `/engine/set_weight_version` and present in the `routes` array that
`GET /v1/rl/workers` returns. Both call sites that reach `register_engine_routes`
(`worker_factory.py:1159` in `_run_decode_worker`, `worker_factory.py:1421` in
`_create_prefill_worker`) construct `BaseWorkerHandler` subclasses, so `handler.
set_weight_version` resolves on both; the embedding path (`worker_factory.py:866`) never
calls `register_engine_routes` at all, so the non-`BaseWorkerHandler`
`EmbeddingWorkerHandler` at `handlers.py:3795` is not reached by this dict.
**3. The documented route list matches the registered one — confirmed, with a note.**
`docs/.../implementation-guide.md:299-306` now lists `set_weight_version` alongside the
five names it already carried, in sorted position. That array was, and remains, an
abridged illustration of the twelve routes registered at `worker_factory.py:1579-1592`: it
omits `flush_cache`, `abort_request`, `update_weights_from_distributed`,
`update_weights_from_tensor`, `init_weights_update_group`, and
`destroy_weights_update_group`. That abridgement predates this change and the change does
not widen it; the surrounding prose already tells the reader to treat `routes` as the live
capability list rather than a fixed inventory. Not a finding, but a human may wish to
decide whether the example should be complete.
### Evidence audit
The customs table shows **3/3 validated**, matching the plan's ```validation-recipes```
block exactly — `00-dynamo-editable-install`, `01-python-lint`,
`03-python-unit-tests-mocker`. No named recipe is `missing`; no unnamed recipe was
substituted. I walked each `validated` row's cited log:
- **00** (`...18-02-31.939Z-bash-1704.log`) — exit 0. Shows `_core OK at
/home/sandbox/workspace/wi-20260811T170521Z-unscoped/repo/lib/bindings/python/src/dynamo/
_core.abi3.so`, i.e. the extension resolves inside *this* work-item checkout. That is what
makes the later pytest runs evidence about this branch rather than about the image.
- **01** (`...18-03-20.765Z-bash-e529.log`) — exit 0. `pre-commit run --files` over all four
changed files, `--hook-stage manual`: isort, black, flake8, codespell, ruff, whitespace/
line-ending hooks, the pytest-marker report, and the two Fern docs hooks that fired on the
guide all report `Passed`. The rest report "no files to check", which is correct for this
file set. The docs edit is covered, which matters because `docs/AGENTS.md` compliance is a
merge gate.
- **03** (`...18-08-19.581Z-bash-87d7.log`) — exit 0. Header prints
`HEAD=656d0de3b694a23ac4197baa7eff4c56f292fcb0 branch=fix/weight-version-tracking-rl-bypass
--80e17fc79aac status=[]`, then `82 passed, 5 skipped`. Real execution, on the committed
tree, unfiltered over both changed-surface suites. Not vacuous.
No `validated` row is contradicted by its own log, so there is no missing `failing`
disposition to catch. The registry additionally carries the before-proof run at exit 1
(`...18-04-44.417Z-bash-ff5a.log`) under the same recipe id, which is the correct shape for
the recipe-03 before/after protocol: the failing before-proof stays visible while the
passing after-run is what the recipe resolves to.
The validator's verdict is `## Verdict: pass`, and it is unqualified on substance, not
merely on the word: every recipe ran, none was skipped for a toolchain gap, and the verdict
sentence describes what was actually executed. No remote compute, SSH, or Slurm was used;
`compute-env.md` reports one A100 available but the plan nominated no GPU recipe and none
was run, which is the correct call for a state-machine bug in a Python handler.
### Test quality — the tests discriminate, they do not mirror
I read the five new cases at `test_vllm_worker_handler.py:1786-1858`. Every assertion is
against the public response dict from `get_weight_version` or `set_weight_version`; grepping
the test file for `_weight_version` returns only occurrences inside the public method names
`get_weight_version` / `set_weight_version` and the test names themselves. Not one test
reads `_weight_version` or `_WEIGHT_VERSION_UNDECLARED`. That is the difference between
asserting behavior and asserting the implementation.
The load-bearing case is
`test_declared_initial_is_distinguishable_from_never_declared`
(`test_vllm_worker_handler.py:1813`), and the validator's before-proof log is worth
quoting because it states the defect as an executable observation rather than a claim:
```
AssertionError: assert {'status': 'ok', 'version': 'initial'} != {'status': 'ok', 'version': 'initial'}
```
Two genuinely distinct worker states — never updated, and updated to the caller-supplied tag
`"initial"` — producing byte-identical responses. No caller could tell them apart even in
principle. That failure is not the absence of a new key; it is the bug. The other four fail
with `KeyError: 'version_declared'` (printing `resp = {'status': 'ok', 'version':
'initial'}`) and `AttributeError: ... has no attribute 'set_weight_version'`. Revert the
production change and they fail again — the validator did revert it, independently of the
printer's report, and they did.
The two new `@staticmethod` helpers (`_make_rl_handler` at line 1765, `_declare_via_update`
at line 1775) are new test infrastructure, and they were exercised under both polarities:
the same helpers driving the same assertions produced five failures against pre-change
production code and eight passes against the fixed tree. That pairing is exercise, not
registration — a helper that constructed a handler satisfying any assertion could not have
produced the pre-change failures.
### The validator's flagged evidence gap — I judge it sufficient
The validator honestly recorded that no run of its own issues an HTTP call to
`/engine/set_weight_version` or reads the advertised route list, since both need a live
worker, and offered it as an inspection item rather than a proven claim. That candour is
correct and I want to answer it plainly rather than leave it hanging.
The gap is not material here, and the reason is structural: there is no per-route logic
anywhere on the path. `admin.py:163-166` is an unconditional loop over the mapping, and
`admin.py:108-112` is `sorted(self.routes)` over what that loop filled. The new entry is one
key in a dict literal at `worker_factory.py:1591` alongside eleven siblings that the e2e test
already proves reach the wire. For the registration to fail while the eleven succeed, the
loop would have to discriminate on the name, and it does not. `test_rl_admin.py` passing
covers add/describe/dispatch generically, the handler method itself is unit-tested under both
its accept and reject polarities, and the e2e route assertion at
`test_worker_discovery.py:239-249` is an `.issubset` superset check that an added route
cannot break. I am satisfied by inspection and would not hold the change for a live-worker
run of this one line.
What the recorded runs do **not** establish — and the validator says so, correctly — is that
a real RL framework performing a real bypass load will be reported correctly. Nothing can
establish that, because the bypass by construction never enters Dynamo. The change converts
a silent wrong answer into a legible unknown plus a supported way for the caller to repair
it. The `> [!WARNING]` at `implementation-guide.md:383-384` states that limit to users
plainly instead of papering over it. I agree with the validator that this is the correct
disposition rather than a shortfall, and I agree with the plan's rejection of option (A):
the loading RPC is named by the caller (`body.get("engine_rpc", ...)` at `handlers.py:1811`
and `handlers.py:1874`) and implemented in a caller-supplied `--worker-extension-cls`, so
there is no in-Dynamo point at which "weights landed" is observable. A human should still
weigh this as a scoping judgement: the issue's literal title stays true of the world after
this change; what changes is that the surface no longer lies about it.
### Non-goals audit — clean
`git diff --name-only main` returns exactly the four files the plan names. Against the
plan's Non-goals: no change to how weights are loaded (both `collective_rpc` call sites are
untouched at `handlers.py:1818` and `handlers.py:1881`); no new weight transport;
`update_weights_from_tensor` remains the explicit not-implemented error at
`handlers.py:1898` and the handlers diff contains zero hits for it; no SGLang edit (the only
SGLang reference is precedent in prose); no attempt to police direct
`AsyncLLM.collective_rpc` calls; and the `version` field on the happy path is neither
renumbered, renamed, nor repurposed. No out-of-scope edit.
The one deliberate behavior change on the read route — a never-declared worker now returns
`version: null` instead of `version: "initial"` — is the fix itself under option (C), not a
non-goal violation: the happy path (a worker that has had a version declared) is
byte-identical. I found no in-repo consumer that breaks: grepping the whole tree for
`get_weight_version` and `weight_version` across Python, Rust, and Go turns up only
`handlers.py`, `worker_factory.py`, the two test files, and the docs page. External RL
orchestrators that string-compare `version` against `"initial"` will observe the change,
which is what the newly added guidance at `implementation-guide.md:381` addresses by telling
callers to branch on `version_declared` instead. Flagging for the human as an API-visibility
consideration on a published docs page, not as a finding.
### Docs
`implementation-guide.md` keeps its SPDX header and Fern frontmatter inside the `---` block
(lines 1-6), adds no body `# H1`, uses a GitHub-style `> [!WARNING]` matching the two already
in the file at lines 47 and 217, nests the new `### Read and Declare the Weight Version` at
line 367 correctly under `## Call vLLM Engine Routes Directly` at line 326, and preserves
"vLLM" casing throughout. The Fern pre-commit hooks fired on this file and passed.
## Findings
| # | Severity | Location | Claim | Evidence |
|---|---|---|---|---|
| 1 | nit | `components/src/dynamo/vllm/handlers.py:1779` | `set_weight_version` gates on key *presence*, so `{"weight_version": null}` is accepted as a declaration and produces `{"version": null, "version_declared": true}` — a response the change's own documentation defines as the undeclared marker, so a caller reading `version` alone cannot tell the two apart. | `handlers.py:1779` tests `if "weight_version" not in body`, then `handlers.py:1781-1782` records `body["weight_version"]` verbatim; `get_weight_version` at `handlers.py:1761` then emits that `None` as `version`. `implementation-guide.md:381` states "`version_declared` is `false`, with `version` set to `null`, until something declares one", and the e2e reader at `tests/rl/test_worker_discovery.py:281` consults `version` alone. A caller has to pass an explicit null to reach this, and the two pre-existing update routes are equally permissive (`handlers.py:1810`, `handlers.py:1873`), which is why the printer declined to validate the payload type — a defensible consistency argument. Rejecting a `None` version would close the last corner of the ambiguity this change exists to remove. |
| 2 | nit | `components/src/dynamo/vllm/handlers.py:1782` | `set_weight_version` writes the tracked version outside `_pause_lock`, so a declaration issued while a Dynamo-mediated update is in flight is silently overwritten by that update's later write — the surface then reports the *earlier*-declared version. The change's own reasoning ("races with nothing but itself") understates this. | `handlers.py:1782` calls `_declare_weight_version` with no lock held, whereas `update_weights_from_disk` and `update_weights_from_distributed` write at `handlers.py:1823` and `handlers.py:1886` while holding `_pause_lock` (acquired at `handlers.py:1797` and `handlers.py:1863`) and only *after* awaiting `collective_rpc`. A `set_weight_version` landing during that await wins momentarily and is then clobbered when the update resumes. Low impact — an orchestrator interleaving both paths is already in an ambiguous state, and the route is new so nothing regresses — but `change.md`'s claim that the route "races with nothing but itself" is not accurate, and the docs at `implementation-guide.md:398` warn only about pause and prefix-cache, not about ordering against an in-flight update. |
Two further observations that are **not** findings, recorded so they are not mistaken for
oversights: the docs `routes` example at `implementation-guide.md:299-306` remains an
abridged six-of-twelve subset (pre-existing, not widened by this change); and no unit test executes the
initialization at `handlers.py:1153`, because `_make_handler` patches
`BaseWorkerHandler.__init__` out at `test_vllm_worker_handler.py:83` — the tests reach the
sentinel through the `getattr` default at `handlers.py:1757` instead. The two paths are
behaviorally identical by construction, so nothing is untested in substance;
`change-validation.md`'s coverage note citing line 1153 for the undeclared case is a small
imprecision in the prose, not a gap in the evidence. The printer documented the fixture
behavior accurately in `change.md`. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. WalkthroughvLLM workers now distinguish undeclared weight versions from declared values. They expose a direct declaration route, preserve declarations when updates omit a version, register the route, add tests, and document the response and validation contracts. ChangesWeight-version tracking
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The version metadata behavior matches the documented contract, with no remaining issue requiring changes before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 42.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 3 files. (1 skipped: 1 unsupported.)
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
🧹 Nitpick comments (1)
components/src/dynamo/vllm/handlers.py (1)
1765-1784: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse lazy logging for the declared version.
Line 1783 formats
versionbefore the logger checks the log level. Pass the format string and value separately.Proposed fix
- logger.info(f"[RL] Weight version declared (version={version})") + logger.info("[RL] Weight version declared (version=%s)", version)As per coding guidelines, pass log format strings and arguments separately. As per path instructions, avoid eagerly formatted logging.
🤖 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 `@components/src/dynamo/vllm/handlers.py` around lines 1765 - 1784, Update the logger.info call in set_weight_version to use lazy logging by passing the format string and version as separate arguments instead of eagerly constructing the message with an f-string.Sources: Coding guidelines, Path instructions
🤖 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 `@components/src/dynamo/vllm/handlers.py`:
- Around line 1741-1763: Update get_weight_version to access
self._weight_version directly instead of using getattr with
_WEIGHT_VERSION_UNDECLARED. If any tests bypass BaseWorkerHandler.__init__,
initialize _weight_version in those test fixtures.
In `@docs/fern/pages/use-cases/reinforcement-learning/implementation-guide.md`:
- Around line 367-384: Update the “Read and Declare the Weight Version”
description around get_weight_version to say it reports the last weight version
declared to the worker, not the version currently being served. Keep the
existing explanation about direct engine loads leaving the reported value stale
consistent with this wording.
---
Nitpick comments:
In `@components/src/dynamo/vllm/handlers.py`:
- Around line 1765-1784: Update the logger.info call in set_weight_version to
use lazy logging by passing the format string and version as separate arguments
instead of eagerly constructing the message with an f-string.
🪄 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: 3bf46854-31d2-40ae-984a-efa93d248430
📒 Files selected for processing (4)
components/src/dynamo/vllm/handlers.pycomponents/src/dynamo/vllm/tests/test_vllm_worker_handler.pycomponents/src/dynamo/vllm/worker_factory.pydocs/fern/pages/use-cases/reinforcement-learning/implementation-guide.md
|
🔄 Datadog auto-retried 2 jobs - 2 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: 034482a | Docs | Datadog PR Page | Give us feedback! |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
Contributor
Author
CI result: passedObserved terminal at 2026-08-11T19:57Z on head
What changedThe two failures on the previous head The five All three review threads raised on this PR were addressed in round 1 and are resolved. |
glamr-agent
temporarily deployed
to
external_collaborator
August 11, 2026 19:42 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
Automated repair round 1 — Review findings addressed (3 of 3, all verified against current code first):
New regression test CI failure — The drift is not from this PR, and the evidence is worth stating since the remedy still lands here:
Checks run:
Generation was run in a clean worktree, not the dev checkout, whose build artifacts ( GPU and Rust suites remain |
Contributor
|
/ok-to-test 366d879 |
Contributor
Author
|
nursery: /devin review |
Contributor
|
🧠 Learnings used✅ Action performedFull review finished. |
Contributor
|
/ok to test c2f3452 |
Contributor
Author
|
nursery: @dynamo-ops please run full CI for c2f3452 |
Contributor
|
/ok to test c2f3452 |
MatejKosec
enabled auto-merge (squash)
September 18, 2026 22:31
This was referenced Sep 18, 2026
MatejKosec
disabled auto-merge
September 18, 2026 22:33
Signed-off-by: Matej Kosec <mkosec@nvidia.com>
Contributor
|
/ok to test c2b68e9 |
MatejKosec
enabled auto-merge (squash)
September 18, 2026 22:37
Contributor
Author
|
nursery: @coderabbitai full review |
Contributor
Author
|
nursery: /devin review |
Contributor
|
🧠 Learnings used✅ Action performedFull review finished. |
dmitry-tokarev-nv
approved these changes
Sep 18, 2026
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
Contributor
There was a problem hiding this comment.
Re-review at c2b68e97c9d
Read as a new change, not as a check against my earlier findings. Zero P0, zero P1, two P3. Approving. This approval replaces my earlier ones at 91d258aefe and e9c68749e2, which pointed at trees that nobody had read.
What moved, and what I verified on this head
Two commits arrived after e9c68749e2. c2f34522ab cut the diff back to four files. c2b68e97c9 made the declaration log a constant message, deleted a setup comment, and dropped the integer case from two parametrized tests.
The push is append-only. The last force-push was on 2026-09-10, before my last review. All 19 commits carry distinct git patch-id --stable values, the compare API reports 2 ahead and 0 behind from e9c68749e2, and both earlier approval commits are ancestors of this head on a clone proven not shallow.
The six files that left the diff are byte-identical to the merge base 66411bc9, blob by blob. The diff is now four files.
The base moved 230 commits, and 8 of them touch these four files. I merged the current base tip into this head myself. The merge is clean and the merged tree still contributes exactly the same four files, so neither P3 depends on the drift.
The new tests run. Under the real pre-merge expression pre_merge and vllm and gpu_0 and not e2e the handler test file collects 104 tests, a sibling collects 87, and a wrong-backend expression collects 0 of 104.
Five mutations, all proven by sha256 with a green unmutated baseline first: restoring "initial" in __init__ fails 1 test, treating "initial" as undeclared fails 2, dropping the key-presence guard fails 6. The coverage that the deleted test_declared_initial_is_distinguishable_from_never_declared used to give is therefore still held by the remaining matrix.
Five hypotheses I killed before posting
A typed model in lib/rl would break on version: null. It does not exist. lib/rl/src/lib.rs carries no weight-version model, and lib/runtime/src/system_status_server.rs:687 returns the handler JSON verbatim.
A second registration site would miss the new route. There is one caller of register_rl_routes under components/src, and RLRouteRegistry.describe() sorts its names, which matches the example at integration-reference.md:121.
The vLLM mocker would need the route. lib/mocker/servers/vllm/src/server.rs:410 is a gRPC stub that returns unimplemented, and it is not on this path.
The two new curl examples omit a status check. The two examples already in this file omit it too, so they follow the file's own style. The checked style belongs to weight-updates.md.
set_weight_version takes no _pause_lock while the update routes do. Last writer wins on both paths, and I could not build an interleaving that is not a caller sequencing error.
Signed-off-by: GLAMR <svc-glamr@nvidia.com>
auto-merge was automatically disabled
September 18, 2026 23:41
Head branch was pushed to by a user without write access
Contributor
Author
|
nursery: @dynamo-ops please run full CI for 755039c |
Contributor
Author
|
nursery: @coderabbitai full review |
Contributor
Author
|
nursery: /devin review |
Contributor
|
/ok to test 755039c |
Contributor
|
🧠 Learnings used✅ Action performedFull review finished. |
ishandhanani
approved these changes
Sep 19, 2026
1 task done
This branch was successfully deployed
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
7 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
Summary
Python vLLM workers now distinguish an undeclared weight version from caller-supplied JSON values, including the literal
"initial". The newset_weight_versionroute lets a loader declare metadata after updating weights outside Dynamo's/engineroutes.Details
get_weight_versionreportsversion: nullandversion_declared: falseuntil a version is declared. Explicitnullcounts as a declaration.weight_versionis present. Omission preserves the prior declaration and retainsversion: "unknown"in the update response. Successful update replies includeversion_declared, indicating whether that update supplied a version; this distinguishes omission from an explicit"unknown"declaration.set_weight_versionrecords metadata without loading weights, pausing generation, or resetting the prefix cache.Where should the reviewer start?
Start with the weight-version routes in
components/src/dynamo/vllm/handlers.py, thenTestRLAdminRouteHardeningincomponents/src/dynamo/vllm/tests/test_vllm_worker_handler.py. The route is registered inworker_factory.py; the contract is documented in the RL integration reference.Validation
PYTHON_CPU_COUNT=1to accommodate sandbox multiprocessing restrictions.755039c69be3281e411ca5c97a2e4e6dd4d68871.Related Issues
This PR is linked to an issue:
/engine.Summary by CodeRabbit
New Features
null, numbers, strings, and"initial".Documentation