Repository navigation
[Frontend][RL] Track HTTP weight operation outcomes and concurrency - #55781
Conversation
|
Scope correction pushed in The earlier mainline synchronization accidentally included unrelated changes from a local tracking reference that did not match the PR's actual upstream base. Those changes have now been removed from the final PR diff:
I verified the five files against the immutable upstream merge-base This corrects the earlier scope-validation claim. No new GPU execution results are claimed. |
Signed-off-by: Ronald1995 <ronaldautomobile@163.com>
Signed-off-by: Ronald1995 <ronaldautomobile@163.com>
777dcb0 to
46a0704
Compare
Signed-off-by: Ronald1995 <ronaldautomobile@163.com>
Signed-off-by: Ronald1995 <ronaldautomobile@163.com>
dbf7e83 to
1f0262b
Compare
Signed-off-by: Ronald1995 <ronaldautomobile@163.com>
1f0262b to
54ce10e
Compare
Add bounded-label telemetry around dispatched frontend weight operations: init, start, start_draft, update, finish and set_version. Counters report success/error, histograms measure one dispatched operation, and an in-flight gauge tracks concurrency without one request clearing another. Input validation stays outside the recorder. Both frontends expose the same names and labels: the Python RL HTTP routes use an independently owned `WeightOperationMetrics` created on first use on `get_prometheus_registry()`, and the Rust HTTP weight-transfer routes use a drop-safe RAII recorder. `finish` and the weight-version handshake are recorded as separate operations so that a version failure is not reported as a failed finish; the Python route now calls `finish_weight_update()` and `update_weight_version()` separately to match the Rust frontend, and `/update_weight_version` is instrumented on both. Tests: recorder outcome/overlap tests, route-level in-flight and rejected-input tests, a registry-binding regression test, and Rust recorder plus HTTP lifecycle and rejection metric assertions. Signed-off-by: Ronald1995 <ronaldautomobile@163.com>
Scope splitThe two changes that were bundled here are now separated, so this PR keeps only the RL weight-metric feature and the shared logger-ownership fix can be reviewed by its own owners: Both are currently based on What #11 changes relative to this branch
Test evidence on this branchH20 cluster (
Review logistics@Ronald1995 — since these branches sit on top of this PR's branch, the intended sequence is: review/land #10 (shared fix, separate owners) and #11 (RL metric, RL maintainers), then this branch can be retired or fast-forwarded. Nothing here needs to be re-litigated; the RL-specific design decisions are unchanged apart from the |
The per-collector ownership tracking (`PrometheusStatLoggerRegistry`,
`unregister_prometheus_stat_logger_metrics`) and the connector /
speculative-decoding / perf metric-class plumbing belong with the shared
`PrometheusStatLogger` lifetime fix, not with the RL weight-operation metrics:
the same bug also silently drops the tool-call parser metrics, the WebSocket
connection metrics and the prometheus-fastapi-instrumentator HTTP metrics.
- Restore `vllm/v1/metrics/{loggers,prometheus,ray_wrappers,perf}.py`,
`vllm/v1/spec_decode/metrics.py` and the KV/EC connector metric modules to
their state on the PR base; this PR no longer touches them.
- Drop `tests/v1/metrics/test_prometheus_logger_ownership.py`, which tests that
ownership behaviour and moves with it.
- Keep everything that is specific to this PR: the recorders in both frontends,
the `finish` / `set_version` split, the lazy binding of the Python recorder to
`get_prometheus_registry()`, and all the tests for them.
Known limitation after this revert: in single-process deployments
(`api_server_count == 1`, the default) a `PrometheusStatLogger` rebuild removes
every `vllm:*` collector from the default registry, so these frontend metrics
stop being exported from `/metrics` until the logger-ownership fix lands. The
recorder object stays alive and keeps counting; only the export is affected.
This is the pre-existing behaviour for all frontend-owned metrics.
Tested after the revert with the real modules: 11/11 weight-operation tests pass
(recorder outcomes, overlap, lazy registry binding, route in-flight/rejection,
finish-succeeds/version-fails, unversioned finish, update_weight_version).
Signed-off-by: Ronald1995 <ronaldautomobile@163.com>
Scope narrowed: the logger-ownership change is reverted out of this PRPushed Reverted to the PR base (this PR no longer touches them):
Kept, because it is specific to this PR: both recorders, the Known limitation (please read before reviewing)In single-process deployments ( This is the pre-existing behaviour for every frontend-owned metric in the tree (the tool-call parser metrics and the WebSocket connection metrics have the same exposure problem today), and it is fixed separately in a follow-up PR rather than here, so that this PR stays focused on the RL weight metrics. Test status after the revertRe-ran the weight-operation tests against the real modules: 11/11 pass (recorder success/error/cancellation, overlapping requests, lazy registry binding, recorder-created-once, route in-flight accounting, rejected input, The Rust route-level assertions ( |
…emantics
Rename and re-scope the frontend weight-operation telemetry so that the metrics
describe logical frontend weight operations, not HTTP requests:
- `vllm:rl_weight_update_operations_total` (was `..._requests_total`)
- `vllm:rl_weight_update_operation_duration_seconds` (was `..._request_...`)
- `vllm:rl_weight_update_operations_in_flight` (was `..._requests_in_flight`)
One HTTP request maps to zero or more operations: input validation happens
outside the recorder, and `/finish_weight_update` with a `weight_version`
records `finish` for `engine.finish_weight_update()` plus `set_version` for
`engine.update_weight_version()`. `finish{success}` together with
`set_version{error}` (HTTP 500 overall) is a valid outcome and is what makes the
failure attributable; `/update_weight_version` records `set_version` as well.
Help text, docstrings and comments no longer describe these as HTTP requests.
Metric creation no longer targets the scrape registry. The collector is created
on the process default registry with no `registry=` argument, which is the
`prometheus_client` contract for multiprocess mode: the values become
mmap-backed and the `/metrics` scrape registry aggregates them through
`MultiProcessCollector`. The lazy singleton stays - its purpose is that metric
objects exist only after multiprocess Prometheus setup has run - but it is no
longer described as "binding to the registry returned to /metrics". Tests may
still pass a private registry for isolation.
Tests:
- new `test_rl_weight_operation_multiprocess.py` uses real child processes and a
real `PROMETHEUS_MULTIPROC_DIR`: two processes aggregating into
`operations_total == 2`, `operation_duration_seconds_count == 2` and
`operations_in_flight == 2`; per-operation series staying separate; and
`mark_process_dead` clearing a dead process's stale `in_flight` while leaving a
live process untouched (skipped on Windows, which locks the mmap files);
- `test_rl_weight_operation_routes.py` now parametrizes every operation
(`init`, `start`, `start_draft`, `update`, `finish`, `set_version`) for both the
success path (gauge 1 during the call, `operations_total{success}` and duration
count +1, gauge back to 0) and the engine-exception path (`{error}` +1, no
`{success}`, gauge released), plus rejected input touching no series at all;
- the recorder unit tests cover success/error/cancellation, overlapping
operations, the `finish` + `set_version` split, and lazy creation.
Rust mirrors the same names, labels, buckets and boundaries; the counter is
registered without the `_total` suffix because the crate adds it when encoding.
Signed-off-by: Ronald1995 <ronaldautomobile@163.com>
Semantics revision pushed:
|
| Before | After |
|---|---|
vllm:rl_weight_update_requests_total |
vllm:rl_weight_update_operations_total |
vllm:rl_weight_update_request_duration_seconds |
vllm:rl_weight_update_operation_duration_seconds |
vllm:rl_weight_update_requests_in_flight |
vllm:rl_weight_update_operations_in_flight |
The names now say operations because one HTTP request maps to zero or more of them (/finish_weight_update with a version records finish and set_version). The help text states this, and the PR body documents that summing across the operation label is not meaningful: update is per weight chunk while finish/set_version/init/start/start_draft are lifecycle events. So sum(..._operations_total) is not "number of weight updates" - query per operation instead.
Registry usage
The recorder is no longer created with get_prometheus_registry() as its creation registry. In multiprocess mode that returns a CollectorRegistry whose purpose is scrape/aggregation (MultiProcessCollector), not metric creation. The collector is now created on the process default registry with no registry= argument, which is what prometheus_client expects: the values become mmap-backed and the /metrics scrape registry aggregates them. The lazy singleton stays, but its stated purpose is now "metric objects are instantiated after multiprocess Prometheus setup has occurred" rather than "bind to the registry returned to /metrics".
New real multiprocess regression test
tests/entrypoints/unit_tests/test_rl_weight_operation_multiprocess.py uses real child processes and a real PROMETHEUS_MULTIPROC_DIR (skipped on Windows, where the mmap files are locked):
- two processes each completing one operation:
operations_total == 2,operation_duration_seconds_count == 2; - two processes each holding one operation open:
operations_in_flight == 2, which is what actually validatesmultiprocess_mode="livesum"; - different
operationlabels stay separate across processes; mark_process_deadclears a dead process's stale in-flight gauge while a live process is unaffected;- a subprocess asserts the module import creates no collectors and that the counter/histogram/livesum mmap files appear only after first use.
The old test that mocked get_prometheus_registry() and asserted registration into the mock registry is gone - it did not test multiprocess behaviour, and test_metrics_are_created_lazily now covers the lazy contract directly.
Test evidence (H20)
29 passed, 1 warning in 29.18s
That is the recorder tests, the route wiring tests parametrized over all six operations (success and engine-exception paths, plus rejected input touching no series), and the four multiprocess tests. cargo test -p vllm-metrics on the same cluster: 3 passed; 0 failed.
cargo test -p vllm-server --lib weight_transfer is still blocked by the missing OpenSSL development headers on that image (no sudo/apt; the CUDA devel container has no libssl-dev either). The Rust route assertions use the new metric names and will be confirmed on a host with pkg-config libssl-dev before this leaves draft.
Known limitation, unchanged and still handled separately: in single-process deployments a PrometheusStatLogger rebuild calls unregister_vllm_metrics(), which removes these collectors from the default registry, so they stop being exported until the logger-ownership fix (pr-55781-ownership-fix) lands. That ownership change is deliberately not part of this PR.
Three follow-up commits were needed to get the multiprocess harness honest - a child stderr pipe that deadlocked on vLLM import warnings, a double increment of the held gauge, and children that never completed an operation so no counter was written. All three were test-harness bugs, not product bugs; the product code is unchanged apart from the rename and the registry change above.
|
/ci run |
|
❌ This PR is 1 commit behind upstream |
Signed-off-by: Ronald1995 <ronaldautomobile@163.com>
|
/ci run |
|
✅ Triggered Buildkite CI #92011 for commit |
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Ronald1995 <ronaldautomobile@163.com>
|
/ci run |
|
❌ This PR is 3 commits behind upstream |
Signed-off-by: Ronald1995 <ronaldautomobile@163.com>
|
/ci run |
|
✅ Triggered Buildkite CI #92045 for commit |
Signed-off-by: Ronald1995 <ronaldautomobile@163.com>
|
/ci run |
|
✅ Triggered Buildkite CI #92064 for commit |
Signed-off-by: Ronald1995 <ronaldautomobile@163.com>
|
/ci run |
|
❌ This PR is 2 commits behind upstream |
|
/ci run |
|
✅ Triggered Buildkite CI #92072 for commit |
| } | ||
|
|
||
|
|
||
| def test_weight_sync_metrics_aggregate_across_api_servers(tmp_path): |
There was a problem hiding this comment.
I think this test is flaky, it's causing a failure on #59007: https://buildkite.com/vllm/ci/builds/92162/list?jid=01a0f2e0-2154-474b-bc1a-7e655cc0878f&tab=output
There was a problem hiding this comment.
Thanks for the help, will avoid it next time.
|
@aoshen02 this is still flaky. Please review this one: |
Purpose
Weight sync is on the critical path of every RL step, but the frontend exposes no signal for how long each weight-transfer operation takes or whether one is stuck. The generic HTTP metrics already count every weight route by status (
http_requests_total{handler,status}), but their per-handler histogram tops out at 1 s, and nothing is recorded until a request completes, so a hung NCCL transfer is invisible.This PR adds two metrics, with the same names, labels and buckets in the Python and Rust frontends:
vllm:rl_weight_update_operation_duration_secondsoperationvllm:rl_weight_update_operations_in_flightlivesumin multiprocess mode)operationoperation/init_weight_transfer_engineinit/start_weight_updatestart/start_draft_weight_updatestart_draft/update_weightsupdate(one per chunk when packed)/finish_weight_updatefinish(includes the optional version update)http_requests_totalalready covers counts and status. This matches the sleep-mode metrics in [Frontend][RL] Align sleep-mode API responses and operation metrics #52864.Test Plan
vllm serve --api-server-count 2with the IPC weight-transfer backend. An HF trainer runs eight full syncs over HTTP./metricsmust report the exact summed duration counts per operation and zero in-flight, and the multiprocess files must show both API servers recorded, so the counts are cross-process sums./metricsserves (get_prometheus_registry()) reports in-flight 2, then 0 and two duration samples after both finish. Fails iflivesumis dropped.Exception.update/finishduration counts and in-flight; invalid payloads record no duration.Test Result
vllm-metrics1 passed,vllm-serverweight_transfer routes 3 passed.AI assistance (Claude Code) was used for this change; the submitter reviewed every changed line and ran the tests above.
🤖 Generated with Claude Code