[Misc] Warn When vLLM / vLLM-Omni Have Mismatched Versions#2691
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Nice utility. Two things:
-
No test for the warning logic — consider adding a quick unit test with mocked
vllm.__version__covering the three cases (match, mismatch, dev/0.0). Low effort, high value for a safety check that runs on every import. -
The import order change in
__init__.py(version before patch) — the comment explains why, but have you verified this doesn't break any existing import path whereversion.pymight depend on monkeypatched vLLM? Looks safe from the diff sinceversion.pyonly importsvllm.__version__, but worth confirming on a clean install.
|
Clean utility. One suggestion: consider adding a test that verifies the warning fires when versions mismatch (mock |
|
Thank you for the reviews @lishunyang12 @hsliuustc0106 🙂 Added some small tests for when the warnings will fire. Also yes, moving the version check should be fine since the versioning of vllm omni / vllm are independent and shouldn't depend on any monkey patching. For testing against a new install, you can do something like this as a quick check. uv pip install vllm==0.18.0
uv pip install -e .
python3 -c "import vllm_omni" |
|
maybe we can look this pr #1941 |
18b77aa to
fb0d874
Compare
- Warn if mismatched versions - Remove redundant check - Add tests for version compat - Add more tests for local identifiers Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: lengrongfu <lenronfu@gmail.com>
fb0d874 to
0e8ba1e
Compare
|
Hi @hsliuustc0106, @lengrongfu and I chatted and have decided to move forward with this PR. We have added some of the additional tests from the other PR that were not covered explicitly in this one, and went ahead and squashed it to a coauthored commit. It's ready for another look when you have time, thanks 🙂 |
lgtm, I also met this problem before, thanks anyway |
…ect#2691) Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: lengrongfu <lenronfu@gmail.com>
…ect#2691) Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: lengrongfu <lenronfu@gmail.com>
…ect#2691) Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: lengrongfu <lenronfu@gmail.com>
Purpose
Mismatched vLLM Omni / vLLM versions are a very common source of errors for users. This PR adds a warning if the major / minor version for vLLM Omni & vLLM aren't the fallback for
_versionnot being created (0.0) and are mismatched. This runs on import of the version module, which is now done before running monkeypatching on vLLM, because the imports in monkey patch will frequently break with incompatible versions.Example (with my current omni build and vLLM 0.18 instead of 0.19), where it will warn before crashing due to patch issues:
This is a joint contribution with @lengrongfu