Repository navigation
[Core] Default uses_xdrope_dim so the Omni runners work with newer vLLM - #7635
MrlixiangWE wants to merge 1 commit into
Conversation
|
This PR appears to belong to: docs/design/module/ar_runtime.md. Module owners: @tzhouam @fake0fan @Gaohan123 Routing: @tzhouam via module of the changed files, semantic router, CODEOWNERS; @fake0fan via module of the changed files, semantic router; @Gaohan123 via module of the changed files, semantic router @MrlixiangWE, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
b834253 to
bbeec58
Compare
|
Self-reviewed against current main; all fixed in bbeec58. #6923 now guards four of the six read sites with The two NPU read sites #6923 did not touch are what this still buys over main. They reach the default by inheritance — One case now drops the attribute after the parent constructor returns, so the pinned version covers the removal as well: 15 passed on 0.29.0 and on |
MrlixiangWE
left a comment
There was a problem hiding this comment.
Self-review pass over this head. The first item asks for a smaller diff; the rest are the test file.
| if self.uses_mrope: | ||
| positions = self.mrope_positions.gpu[:, :num_input_tokens] | ||
| elif getattr(self, "uses_xdrope_dim", 0) > 0: | ||
| elif self.uses_xdrope_dim > 0: |
There was a problem hiding this comment.
Keep #6923's four getattr guards and ship only the constructor default. This repo guards attributes precisely because tests build runners with object.__new__ — gpu_ar_model_runner.py:283 says so in a comment, and tests/worker/test_omni_gpu_model_runner.py builds OmniGPUModelRunner that way and drives _preprocess, staying green only because the fixture hand-sets uses_xdrope_dim = 0. Reverting the guards makes that hand-set load-bearing for every such fixture, and the newer-vLLM fix holds without those four hunks, so they are scope this PR does not need.
| # XD-RoPE path stays for versions that still provide it. Defaulting it | ||
| # here is the single place the six read sites depend on, including the | ||
| # two NPU runners that inherit this constructor. | ||
| self.uses_xdrope_dim = getattr(self, "uses_xdrope_dim", 0) |
There was a problem hiding this comment.
On a vLLM that no longer sets the attribute this default is always 0, so _init_xdrope_positions never runs and _preprocess falls through to the 1-D self.positions. That is correct there — uses_xdrope_dim is gone from ModelConfig and mrope_num_dims replaces it, and an xdrope_section config reports uses_mrope True on that version, which the target-version run exercises — but nothing says so at runtime. Log once when the parent did not provide the flag, so a version that folds XD-RoPE differently is visible instead of silently linear.
| output = scheduled(new=[new]) | ||
| # Request admission reads the flag at gpu_model_runner.py:696. Startup | ||
| # profiling reads it earlier still, in `_dummy_run`, which this CPU fixture | ||
| # does not reach. |
There was a problem hiding this comment.
The comment points at :696, which is the _init_mrope_positions call; the flag is read at :699. The read site that executes first in a real start is _dummy_run (gpu_model_runner.py:1173, gpu_generation_model_runner.py:815), during startup profiling, and this suite reaches neither — nor the two NPU sites. Fix the line number and say which of the six sites the suite covers.
| expected = torch.arange(4) | ||
| buffer = None | ||
| else: | ||
| if runner.uses_mrope: |
There was a problem hiding this comment.
Assert the layout in this branch too. The else asserts layout == "xdrope", but this branch asserts nothing, so a version that reports uses_mrope True for the xdrope_section config would run the three xdrope cells through the M-RoPE assertions and pass while uses_xdrope_dim > 0 is never taken. One assert layout == "mrope" closes it.
| assert runner.model.calls == [(route, [1, 2, 3, 4], mm_features, grid)] | ||
| else: | ||
| assert runner.model.calls == [(route, [1, 2, 3, 4], mm_features)] | ||
| assert buffer.cpu.shape == (runner.model.positions.shape[0], runner.max_num_tokens + 1) |
There was a problem hiding this comment.
This compares the fixture with itself: runner.model.positions.shape[0] is the row count PositionModel(...) was constructed with. The number that matters is the runner's buffer width, a literal 3 in the pinned version's __init__ and exactly what vllm-project/vllm#56078 changes. Compare against len(mrope_section) for M-RoPE and runner.uses_xdrope_dim for XD-RoPE, so a width change fails here with a readable reason.
|
|
||
| import vllm_omni.worker.gpu_model_runner as omni | ||
|
|
||
| for name in ("empty", "zeros", "ones", "full", "tensor"): |
There was a problem hiding this comment.
Patch PIN_MEMORY instead of replacing five global torch factories for the whole file. It is a module-level import in vllm.v1.worker.gpu_model_runner and drives all the pinned allocations in that constructor, so patching it covers the runner without routing the test's own torch.tensor and assert_close through a wrapper. The current set is also partial — arange, as_tensor and from_numpy are unwrapped — so an upstream __init__ that pins through any of them turns the shared CPU lane red for an unrelated reason.
| grid = [[1, 2, 2]] if input_kind == "image" else [] | ||
| assert runner.model.calls == [(route, [1, 2, 3, 4], mm_features, grid)] | ||
| else: | ||
| assert runner.model.calls == [(route, [1, 2, 3, 4], mm_features)] |
There was a problem hiding this comment.
input_kind is a dead axis on the xdrope layout: mm_features here is the same list object passed to NewRequestData, so the comparison short-circuits on identity and the text, image and cached cells assert the same thing. The grid extraction the axis exists to vary lives in _init_mrope_positions only. Parametrize input_kind for the mrope layout, or assert something the XD-RoPE path derived from the request.
vLLM 719284f (vllm-project/vllm#56078) unified XD-RoPE into M-RoPE and removed `uses_xdrope_dim` from the model runner, so on a vLLM containing that commit the Omni runners raise AttributeError where they read it. Read the flag with a default in `OmniGPUModelRunner.__init__`. vllm-project#6923 guards four of the six read sites for runners built with `object.__new__`, which this repo does in tests; those guards stay, and this default is what covers the paths they do not, including the two NPU runners that inherit this constructor. The pinned version's value is preserved, and a vLLM that never sets the flag says so once in the log rather than silently serving 1-D positions. The test constructs both GPU runners through the real parent constructor and the installed vLLM's config classes, admits a request through `_update_states`, and checks the position buffers and `_preprocess` output. One case drops the attribute after the parent constructor returns, so the pinned version also covers what the removal does. Signed-off-by: MrlixiangWE <mrdanaer@gmail.com> Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com>
bbeec58 to
17ea395
Compare
|
All fixed in 17ea395. #6923's four guards stay: this repo builds runners with 11 passed in |
|
@amy-why-3459 @linyueqian @yenuo26 Could one of you take a look at this when you have time, and add |
|
[P3] The Propose section is stale relative to the diff. It says "the six read sites read |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Reviewed at 17ea395 — approve.
The fix is sound: the default in OmniGPUModelRunner.__init__ preserves the parent's value when vLLM still sets the flag and supplies 0 where #56078 removed it, and every runner subclass (including the two NPU runners, which define no __init__ of their own) inherits that constructor, so all six read sites are covered. Tree-wide grep at this head confirms the read-site census; the gpu_ar_model_runner.py:462 frame in the body's trace is the super()._update_states() call site, not an additional read. The getattr guards kept at the four GPU sites are justified by the object.__new__-built runners in test_omni_gpu_model_runner.py, and info_once is available on the pinned vLLM's logger.
The new CPU compat test directly simulates the upstream removal and the author's evidence shows it passing on both 0.29.0 and the pre-removal-adjacent dev build, with full lanes green on both sides. One non-blocking note posted separately: the Propose section of the body still describes an all-plain-read revision that the diff superseded.
|
Closing this. The vLLM 0.30 rebase removed every |
Purpose
vLLM
719284f(vllm-project/vllm#56078) unified XD-RoPE into M-RoPE and removeduses_xdrope_dimfrom the model runner, so on a vLLM containing that commit the Omni runners raiseAttributeErrorwhere they read it.#6923 has since guarded four of the six read sites with
getattr. Put the default inOmniGPUModelRunner.__init__and read the flag plainly everywhere, so one line carries the invariant for all six sites, including the two NPU runners that inherit this constructor and that #6923 did not cover. The pinned version's value is preserved. Part of #7452.Propose
OmniGPUModelRunner.__init__reads the flag with a default of 0.self.uses_xdrope_dimdirectly —worker/gpu_model_runner.py699, 1173, 1710,worker/gpu_generation_model_runner.py815,platforms/npu/worker/npu_model_runner.py336,platforms/npu/worker/npu_generation_model_runner.py856._update_states, and checks the position buffers and_preprocessoutput for plain, image and cached input, following whichever layout the installed version selects. One case drops the attribute after the parent constructor returns, so the pinned version covers the removal too.Test Plan
vLLM Version: 0.29.0 (
98dff2a) and0.28.1rc1.dev633+g442d36031, which contains vllm-project/vllm#56078. PyTorch 2.13.0+cu130.vLLM-Omni Commit:
bbeec58, base2ab5d17.Test Result
The same test file on both versions, this branch against main:
g442d36031Every failure on main is
AttributeError: '<Runner>' object has no attribute 'uses_xdrope_dim', raised attest_runner_rope_compat.py170 and 178 because #6923's guards keep_update_statesand_preprocessworking. Dropping only the default from this branch, plain reads kept, raises in the request path instead:tests/workerNo lane fails on either side; the added tests account for the difference in the Other lane.
The NPU read sites are covered by inheritance —
OmniNPUModelRunnerdefines no__init__, both NPU subclasses callsuper().__init__first — and are not executed here.mrope_num_dims, the replacement the newer vLLM introduced, is not read in vllm_omni. Changed-file hooks pass.