Make vLLM patches fail loudly and detect drift on a vLLM bump
- Dominant language
- Python
- Stars
- 2k
- Forks
- 562
- Avg merge
- 4d 1h
- Merged PRs (30d)
- 150
Description
## Problem
We patch vLLM in several ways — rewriting its source by literal string match, and monkey-patching
its objects at runtime. Today a vLLM bump can silently invalidate any of them, and we find out from
a downstream symptom rather than from the patch itself.
Two real incidents motivate this, both from Nemotron work:
- **A string-replacement patch critical to the VLM path silently failed to apply.** The anchor no
longer matched; the patch logged and continued.
- **Two places in NeMo-RL both patched vLLM and conflicted** — the second silently overwrote the
first (seen on the MXFP8 path).
### Four gaps
**1. Patches are not all in one place.** `nemo_rl/models/generation/vllm/patches.py` holds the
string-rewriting ones, but runtime workarounds for vLLM behaviour live elsewhere — for example
`_alias_moe_quantizer_amax_buffers` in
`nemo_rl/modelopt/models/generation/vllm_quant_backend.py`, which exists only because vLLM 0.26's
`RoutedExperts.load_weights` resolves target names with a single-level `getattr`. A vLLM upgrade has
no single place to check.
**2. More than one place executes patches, with an overlapping set.** `patches.py` has two entry
points:
| entry point | patches it runs |
|---|---|
| `ensure_vllm_source_compat()` | tool-parser, radio, GLM |
| `_apply_vllm_patches()` | those same three, plus init-workers-ray, EAGLE3, TCPStore, shm-broadcast, MoE-capture |
Three patches therefore execute from two places. That is safe only while every one of them is
idempotent, which is not uniformly tested — and it is exactly the shape of the MXFP8 incident.
**3. Most patches only warn when they no longer apply.** Of the 8 patch functions, 7 call
`logger.warning` and continue when their anchor text is not found:
| patch | behaviour on drift |
|---|---|
| `_patch_vllm_init_workers_ray` | warns |
| `_patch_vllm_llama_eagle3_own_lm_head` | warns |
| `_patch_vllm_tool_parser_namespace_tool` | warns |
| `_patch_vllm_ray_executor_v2_tcpstore_port` | warns |
| `_patch_vllm_shm_broadcast_bind_retry` | warns |
| `_patch_vllm_radio_layerscale_loader` | warns |
| `_patch_vllm_glm_decoder_sequence_parallel_moe` | warns |
| `_patch_vllm_moe_routed_experts_capture` | **raises when `required=True`** |
A warning in a worker log is not a signal anyone acts on. The failure then shows up as an unrelated
runtime error — a wrong EAGLE3 draft head, a Ray port collision, a GLM decode difference — on paths
CI does not exercise. This is incident 1 above.
**4. Nothing checks the vLLM version.** Seven patch messages say *"The vLLM version may have
changed"*, but no code compares against a pinned expected version. Drift is only ever detected by
anchor-string matching, and only for the patches that bother to check.
## What we want
- **One home.** Every vLLM workaround, string-based or runtime, lives in (or is registered through)
`patches.py`.
- **One executor.** Exactly one place *applies* patches; everywhere else *registers* them. That
removes the overwrite class of bug outright, rather than relying on every patch being idempotent.
- **Loud on drift, at build time and run time.** A patch whose target has changed should fail, not
log and continue. Checking at image-build time as well as at worker start means we learn on a
vLLM bump rather than mid-run on a GPU node.
- **A version signal.** Record the vLLM version each patch was written against, so a bump reports
*which* patches need re-checking.
- **Safe to apply twice.** Re-applying must not error, so a re-entered refit or re-initialised
worker is fine.
- **As few patches as possible.** Every one is a standing cost at each upgrade. Prefer upstreaming
or a supported extension point; when we must patch, say in the docstring why no supported hook
exists.
Net effect: after a vLLM upgrade, one command tells us which patches still apply and which need
rewriting, before anything runs on a GPU.
## Scope
Not asking anyone to rewrite all 7 existing patches at once. Suggested order:
- [ ] Add the version-pinning / drift-report mechanism to `patches.py`, checked at build time and
at worker start
- [ ] Collapse to a single executor; convert the other entry point to registration
- [ ] Convert existing patches to fail loudly, one at a time
- [ ] Move or register runtime workarounds that currently live outside `patches.py`
(`_alias_moe_quantizer_amax_buffers` is the known one)
- [ ] Add an idempotency test per patch, matching the pattern in
`tests/unit/models/generation/test_vllm_patches.py`
- [ ] Audit the existing 8 and drop any that upstream has since fixed
## Context
Came out of review of #3566, which bumps vLLM 0.25.1 → 0.26.0. All 8 existing anchors were checked
by hand against 0.26.0 and still match — but that check was manual, and it is the check we want the
code to do for us next time. The patch added in that PR,
`_patch_vllm_moe_routed_experts_capture`, is the closest to the target shape: it raises on drift
when `required=True`, has anchor and idempotency tests, and runs from a single executor. It is
missing only the version check.
Contributor guide
Assessment
This issue has not been assessed yet.