Repository navigation
fix(vllm): add env-gated CPython GC freeze policy for FPM self-benchmark - #12177
Conversation
WalkthroughAdds an environment-controlled FPM garbage-collection freeze policy, worker extension controls, benchmark lifecycle hooks, engine configuration validation, and tests covering startup, maintenance, shutdown, restart, and interval parsing. ChangesFPM GC policy
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/args.py`:
- Around line 362-382: Update the DYN_FPM_GC_POLICY handling in the engine
configuration path to inject or validate FpmGcWorkerExtension only when the
environment value enables the policy’s freeze behavior. Treat values such as
“off” as disabled, preserving custom worker extensions and avoiding injection
when freeze is not enabled; keep the existing extension validation for enabled
freeze mode.
In `@components/src/dynamo/vllm/gc_policy.py`:
- Around line 85-95: The _freeze_loop function broadly catches and suppresses
gc.freeze() failures, allowing the process to continue with the policy disabled.
Catch only a specifically expected exception, or after logging an unexpected
failure re-raise it and transition the policy through its cleanup path so normal
GC thresholds are restored.
- Around line 61-68: Update _interval_seconds to reject non-finite parsed
values, including positive or negative infinity and NaN, alongside invalid
strings. Treat them like other invalid inputs by logging the existing warning
and returning the 60.0-second fallback; retain the existing minimum clamp for
finite values.
- Around line 171-177: Keep the import-time start_gc_policy() behavior in
components/src/dynamo/vllm/gc_policy.py:171-177 unchanged for worker-extension
loading. In InstrumentedScheduler._bench_init at
components/src/dynamo/vllm/instrumented_scheduler.py:1628-1630, start the
engine-core GC policy only after benchmark configuration validation and
successful initialization; ensure failures roll back any already-started policy.
Update the related cleanup or lifecycle logic at
components/src/dynamo/vllm/instrumented_scheduler.py:2834-2838 and adjust
expectations in components/src/dynamo/vllm/tests/test_gc_policy.py:18-25 to
verify startup does not persist after failed initialization.
In `@components/src/dynamo/vllm/tests/test_gc_policy.py`:
- Around line 46-57: Update test_stop_is_idempotent_and_allows_restart so the
complete GC policy start/stop and restart sequence is enclosed in a try/finally
block, with gc_policy.stop_gc_policy() in finally. Preserve the existing
assertions while ensuring cleanup runs after any assertion failure.
🪄 Autofix (Beta)
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: 658f3bb5-0fce-4ceb-9a79-34474affb880
📒 Files selected for processing (4)
components/src/dynamo/vllm/args.pycomponents/src/dynamo/vllm/gc_policy.pycomponents/src/dynamo/vllm/instrumented_scheduler.pycomponents/src/dynamo/vllm/tests/test_gc_policy.py
This comment has been minimized.
This comment has been minimized.
e443d89 to
6129fd2
Compare
tedzhouhk
left a comment
There was a problem hiding this comment.
Blocking on post-benchmark GC equivalence for real serving. _bench_deactivate() restores only the engine-core process; workers auto-start the policy on extension import and no call site invokes fpm_gc_stop, so they continue serving with automatic gen2 collection disabled and the freeze daemon active. I reopened the existing worker-lifecycle thread with the required normal-completion and abort coverage.
08a5576 to
5234ba3
Compare
5234ba3 to
caddcde
Compare
|
Question on the completion path: if |
CPython gen2 (full) collections walk every tracked object and trigger on the 25% long-lived-growth rule, so their pauses grow with the heap: measured 0.4s -> 9s over a 30k-point sweep, inside the engine processes. Under TP/DP lockstep a single worker's pause stalls the whole forward pass and lands in the measured wall_time as a 3-200x outlier at geometrically spaced, run-independent iteration indices. - New dynamo/vllm/gc_policy.py: disable automatic gen2, gc.freeze() the engine-init heap once, then a daemon thread runs a bare gc.freeze() per tick (O(1) splice, no traversal). gc_maintain() reclaims cycles from untimed windows (unfreeze -> collect -> freeze); stop_gc_policy() restores thresholds and reclaims the frozen heap. - Env-gated and off by default (DYN_FPM_GC_POLICY=freeze): zero behavior change unless explicitly enabled. - Workers are covered via vLLM's worker_extension_cls, auto-injected in benchmark mode when the env var is set and no extension is configured; the engine-core process starts the policy from _bench_init (benchmark-gated) and stops it on deactivate. Split from the combined noise-elimination PR; the eager-shape warmup fix lands separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: YZLi <yuanli@nvidia.com>
- Gate worker-extension injection on the normalized freeze value: any other DYN_FPM_GC_POLICY value neither injects an inert extension nor rejects a custom one (the policy itself only activates on 'freeze'). - Start the engine-core policy at the END of _bench_init so a config validation error never leaves the process with auto-gen2 disabled. - Serialize gc_maintain() with the periodic tick through the module lock: a freeze fired between unfreeze() and collect() would re-freeze the garbage the maintenance pass is meant to reclaim. - Reject non-finite freeze intervals (inf reaches Event.wait and kills the daemon thread with an overflow while gen2 stays disabled). - Fail fast if a tick ever raises instead of looping silently, and always stop the policy in test cleanup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: YZLi <yuanli@nvidia.com>
Reloading with DYN_FPM_GC_POLICY=freeze ran the module-tail autostart twice when a freeze test executed in isolation, leaking the first daemon thread and leaving gen2 disabled after stop. Reload with the policy unset and let each test start it explicitly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: YZLi <yuanli@nvidia.com>
…chmark
Workers auto-start the freeze policy when worker_extension_cls resolves,
but only the engine-core process restored GC on deactivate: workers kept
serving with automatic gen2 collection disabled and the freeze daemon
alive, so cyclic garbage was never reclaimed. The launcher now awaits
collective_rpc("fpm_gc_stop") right after the benchmark-wait completes
- both normal completion and abort funnel through that call site - so
serving resumes only once every worker is GC-equivalent to one that
never benchmarked. Lifecycle tests cover threshold/enabled/freeze-thread
state and frozen-cycle reclamation for both paths, plus the RPC wiring
(issued, gated off, and failure propagation).
Signed-off-by: YZLi <yuanli@nvidia.com>
… benchmark exit path stop_gc_policy() unfroze the whole heap on the way out, which also stripped the baseline freeze vLLM deliberately installs for serving (freeze_gc_heap() after EngineCore startup and after each GPU worker's warmup) so gen2 never scans model weights, KV caches, or CUDA graphs. A post-benchmark process therefore paid larger gen2 pauses than one that never benchmarked. Reclaim the benchmark garbage, then re-freeze the surviving heap under the policy lock to re-establish that baseline. The launcher's worker stop only ran when the benchmark wait returned; _bench_abort publishes status=failed artifacts, so an aborted benchmark raised during validation and skipped the RPC entirely. Route both call sites through a wrapper that stops the workers on success and failure, preserves the original error, fails closed when a success-path stop fails, and time-boxes the RPC so a dead engine cannot hang the launcher on the very path meant to surface the error. Lifecycle coverage: a pre-frozen-heap restoration test that proves the baseline object is back in the permanent generation (mutation-verified against a stop without the re-freeze), a fresh-interpreter worker test asserting thresholds, enabled state, dead freeze thread, restored serving freeze, and reclaimable cycles, a real status=failed artifact driving the production validation failure into the worker stop, time-box tests for both paths, call-site guards on the prefill and decode startup paths, and a stop-before-registration ordering test. Signed-off-by: YZLi <yuanli@nvidia.com>
caddcde to
73bae73
Compare
|
@tianhaox Good catch — that path could survive into serving on the prefill worker. The stop now sits on both exit paths (see |
tedzhouhk
left a comment
There was a problem hiding this comment.
Re-reviewed at 73bae73. The benchmark lifecycle now restores the vLLM serving freeze, stops worker GC policy on both success and failure before model registration, preserves the primary error, and has process-level lifecycle coverage. Targeted GC-policy tests and current CI pass.
Overview:
Single-sample self-benchmark
wall_timemeasurements are intermittently poisoned by3–200x outliers. Root cause: CPython gen2 (full) collections walk every tracked
object, so their pauses grow with the heap — measured 0.4s → 9s over a 30k-point
sweep — and under TP/DP lockstep a single process's pause stalls the whole forward
pass. Triggers follow CPython's 25% long-lived-growth rule, producing outliers at
geometrically spaced, run-independent iteration indices (ratio ≈ 1.23, identical
across nodes/days/topologies) at a fleet-constant ~0.08%/sample rate.
This PR adds an opt-in, benchmark-scoped GC policy that keeps those pauses out
of measured windows. It is env-gated and off by default — zero behavior change
unless explicitly enabled, engine-core activation is benchmark-gated, and normal
GC is fully restored when the benchmark deactivates.
Details:
dynamo/vllm/gc_policy.py: on activation, disable automatic gen2(
gc.set_threshold(t0, t1, 1<<30)),gc.freeze()the engine-init heap once,then a daemon thread runs a bare
gc.freeze()per tick (O(1) generation-listsplice: no traversal, no pause). Refcounting and gen0/gen1 stay fully active, so
only cyclic garbage defers; young-gen scans stay bounded by allocation rate
instead of total heap size.
_bench_initonly when a benchmark is configured, and_bench_deactivatecallsstop_gc_policy()— stops the freeze loop, restores the saved gen2 threshold,and unfreezes + collects the deferred garbage.
gc_maintain()(unfreeze → collect → freeze) is available for explicit reclamation from untimed
windows in long runs.
worker_extension_cls(
FpmGcWorkerExtension), auto-injected in benchmark mode whenDYN_FPM_GC_POLICYis set and the user configured no extension; a conflictingextension raises rather than silently leaving workers unpoliced. No vLLM changes.
measurable stalls: periodic
gc.collect()re-introduces growing full-heap walks(outlier rate 0.082% → 0.712%), and per-tick
gc.get_freeze_count()walks thewhole permanent generation (0.4–4s once millions of objects are frozen).
Validation (GLM-5.2-NVFP4, 4×B200 attention-DP=4, vLLM 0.25.1)
A/B on identical 30,480-measurement sweeps (same node, same GPUs, same point list;
only the GC policy toggled):
End-to-end: fresh single-sample collections with this policy reach median-of-5
database quality (LOO prefill 4.74%/max 125% vs reference 4.17%/129%; holdout
2.24%/max 12.2% vs 1.96%/13.5%) at ~1/3.5 the collection cost, with zero
curve-neighbor poisoned rows. No OOM across four full sweeps with gen2 disabled
(worker RSS ≈ +1–2 GB/h, decelerating); no change to the non-outlier latency
distribution.
Where should the reviewer start?
components/src/dynamo/vllm/gc_policy.py— the policy, including designrationale and rejected variants in the module docstring
components/src/dynamo/vllm/instrumented_scheduler.py— the twobenchmark-gated hooks (
_bench_initstart,_bench_deactivatestop); 9 linestotal
components/src/dynamo/vllm/args.py— benchmark-modeworker_extension_clsinjection and collision handling
components/src/dynamo/vllm/tests/test_gc_policy.py— lifecycle, idempotency,threshold restore, and frozen-cycle reclamation tests
Related Issues
🚫 This PR is NOT linked to an issue:
Summary by CodeRabbit
New Features
DYN_FPM_GC_POLICY=freeze, memory cleanup is managed during benchmark runs to reduce garbage-collection pauses and improve measurement consistency.Bug Fixes