fix(vllm): align vLLM-Omni pin with bundled vLLM 0.24.0 - #11345
fix(vllm): align vLLM-Omni pin with bundled vLLM 0.24.0#11345tanmayv25 wants to merge 6 commits into
Conversation
…match The vllm-runtime image bundles vLLM 0.24.0 but vllm_omni_ref was still pinned to v0.23.0rc1. vLLM-Omni imports internal vLLM symbols (e.g. split_routed_experts) that change across releases, so a major/minor skew crashes at import time with an opaque ImportError. The unified-omni worker then never registers and the Frontend's /v1/models stays empty. - Bump vllm_omni_ref to v0.24.0 (aligned with bundled vLLM 0.24.0). - Add a fail-fast guard (check_vllm_omni_compatibility) that raises an actionable OmniVersionMismatchError on a major/minor mismatch. - Add regression tests for the guard. - Refresh the stale version reference in protected_packages.txt. Signed-off-by: tanmayv25 <tanmayv@nvidia.com>
This comment has been minimized.
This comment has been minimized.
WalkthroughAdds a version_check.py module implementing a fail-fast compatibility check between installed vllm and vllm-omni package versions, wires it into module import in omni/__init__.py, adds corresponding unit tests, and bumps the bundled vllm-omni reference version in container configuration files. ChangesVersion compatibility guard
Estimated code review effort: 2 (Simple) | ~12 minutes Suggested labels: vllm, tests, container Suggested reviewers: dynamo vLLM maintainers 🐰 A version check hops in with care, 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/src/dynamo/vllm/tests/omni/test_omni_version_check.py (1)
66-72: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSimulate the real exception type for the missing-metadata case.
This test raises a generic
Exceptionto simulate missing metadata, butimportlib.metadata.version()actually raisesimportlib.metadata.PackageNotFoundErrorfor a missing package. If the broadexcept Exceptioninversion_check.pyis narrowed to catchPackageNotFoundError(recommended in that file's review), this test would start failing with an unhandled exception instead of exercising the skip path.🔧 Proposed fix
def test_missing_metadata_is_skipped(): def _raise(name): - raise Exception("no metadata") + raise importlib.metadata.PackageNotFoundError(name) with patch("importlib.metadata.version", _raise): # Should not raise; downstream import handles the missing package. check_vllm_omni_compatibility()(requires
import importlib.metadataat the top of this test module)🤖 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/tests/omni/test_omni_version_check.py` around lines 66 - 72, The missing-metadata test is using a generic Exception, which won’t match the real failure mode from importlib.metadata.version. Update test_missing_metadata_is_skipped to patch importlib.metadata.version so it raises importlib.metadata.PackageNotFoundError instead, and add the needed importlib.metadata import at the top of the test module. This keeps check_vllm_omni_compatibility exercised through the intended skip path and aligns the test with version_check.py behavior.
🤖 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/omni/version_check.py`:
- Around line 33-41: Move the importlib.metadata dependency in version_check.py
to module scope and access it via importlib.metadata.version inside the version
check logic, rather than importing version inside the function. Narrow the
exception handling in the version check helper to only catch
importlib.metadata.PackageNotFoundError instead of a bare Exception, keeping the
existing debug log and early return for missing packages. Preserve test
patchability by continuing to resolve version through the module attribute
rather than binding a local imported function.
In `@components/src/dynamo/vllm/tests/omni/test_omni_version_check.py`:
- Around line 16-23: The spacing before the top-level helper `_fake_version` is
incorrect for `black`; add one more blank line after the `pytestmark` list so
there are two blank lines before the function definition, matching the
formatting expected by `black`.
---
Nitpick comments:
In `@components/src/dynamo/vllm/tests/omni/test_omni_version_check.py`:
- Around line 66-72: The missing-metadata test is using a generic Exception,
which won’t match the real failure mode from importlib.metadata.version. Update
test_missing_metadata_is_skipped to patch importlib.metadata.version so it
raises importlib.metadata.PackageNotFoundError instead, and add the needed
importlib.metadata import at the top of the test module. This keeps
check_vllm_omni_compatibility exercised through the intended skip path and
aligns the test with version_check.py behavior.
🪄 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: 12f873f1-125d-42b2-b89c-d3ccb0bdce07
📒 Files selected for processing (5)
components/src/dynamo/vllm/omni/__init__.pycomponents/src/dynamo/vllm/omni/version_check.pycomponents/src/dynamo/vllm/tests/omni/test_omni_version_check.pycontainer/context.yamlcontainer/deps/vllm/protected_packages.txt
- Move importlib.metadata to module scope and access version via the module attribute (keeps test patchability of importlib.metadata.version). - Narrow the caught exception to PackageNotFoundError instead of a bare Exception so unexpected errors propagate. - Update the missing-metadata test to raise PackageNotFoundError. Signed-off-by: tanmayv25 <tanmayv@nvidia.com>
Add the second blank line after the pytestmark list so there are two blank lines before the top-level function, as black requires. Signed-off-by: tanmayv25 <tanmayv@nvidia.com>
Signed-off-by: tanmayv25 <tanmayv@nvidia.com>
|
I wouldn't force bumping in cadence. vllm comes out ahead of vllm-omni so we can't gate on it. |
vLLM-Omni releases can lag vLLM, so the refs can't always be bumped in lockstep; remove the comment implying they must match. Signed-off-by: tanmayv25 <tanmayv@nvidia.com>
vLLM-Omni releases can lag vLLM, so a strict major/minor lockstep guard is too restrictive. Remove version_check.py, its invocation in omni/__init__.py, and the associated test. Signed-off-by: tanmayv25 <tanmayv@nvidia.com>
|
Duplicate: #11248 |
@tanmayv25 you can keep one and close other, your PR missing other required changes. |
Overview
The
vllm-runtimeimage bundles vLLM 0.24.0 butcontainer/context.yamlpinnedvllm_omni_refto v0.23.0rc1. vLLM-Omni is built against a specific vLLM release line and imports internal vLLM symbols (e.g.split_routed_experts) that are added/renamed/removed across releases. A major/minor skew therefore crashes at import time with an opaqueImportError, before the unified-omni worker can register — so the Frontend's/v1/modelsstays permanently empty and no image/video/audio request is ever served.This is the same class of failure originally reported against
1.3.0-rc.2(omni 0.21.x vs vLLM 0.23.0); onmainit re-appeared as omni 0.23.x vs vLLM 0.24.0.Fix
vllm_omni_reftov0.24.0(the vLLM-Omni build aligned with upstream vLLM 0.24.0) incontainer/context.yaml.container/deps/vllm/protected_packages.txt.Note: vLLM-Omni releases can lag vLLM, so the refs are aligned manually per release rather than enforced in lockstep.
Test
Rebuilt the
vllm-runtimeimage (x86_64) with the aligned pin and ran the aggregated omni worker:from dynamo.vllm.unified_omni import VllmOmniEngineimports cleanly (previously crashed onsplit_routed_experts)./v1/modelsis populated.POST /v1/videoswithWan-AI/Wan2.1-T2V-1.3B-Diffusersreturnsstatus: "completed"and writes a valid H.264 MP4.A rebuild of the
vllm-runtimeimage is required for the pin change to take effect.