-
Notifications
You must be signed in to change notification settings - Fork 638
Aflowers/bump vllm 0.11.0 debug #3569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: alec-flowers <[email protected]>
Signed-off-by: alec-flowers <[email protected]>
Signed-off-by: alec-flowers <[email protected]>
Signed-off-by: alec-flowers <[email protected]>
WalkthroughBumps VLLM and FlashInfer versions in container build and install scripts, updates a PromptLogprobs import path in an example utility, and adds a long sleep in pytest configuration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as pytest Runner
participant Conftest as tests/conftest.py
Runner->>Conftest: import and run pytest_configure()
Note over Conftest: Introduced delay
Conftest->>Conftest: time.sleep(4 hours)
Conftest-->>Runner: return
Runner->>Runner: collect and execute tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
container/deps/vllm/install_vllm.sh (1)
201-201
: Update stale version reference in precompiled wheel URL.The
VLLM_PRECOMPILED_WHEEL_LOCATION
still references version0.10.2
instead of0.11.0
. This will cause the script to download the wrong wheel when building from source on AMD64.Apply this diff to update the version reference:
- export VLLM_PRECOMPILED_WHEEL_LOCATION="https://vllm-wheels.s3.us-west-2.amazonaws.com/${VLLM_REF}/vllm-0.10.2-cp38-abi3-manylinux1_x86_64.whl" + export VLLM_PRECOMPILED_WHEEL_LOCATION="https://vllm-wheels.s3.us-west-2.amazonaws.com/${VLLM_REF}/vllm-0.11.0-cp38-abi3-manylinux1_x86_64.whl"Alternatively, verify the exact wheel filename by running:
#!/bin/bash # Description: Verify the correct wheel filename for VLLM 0.11.0 aws s3 ls s3://vllm-wheels/v0.11.0/ --region us-west-2 --no-sign-request | grep -E 'cp38-abi3-manylinux.*x86_64.whl'
🧹 Nitpick comments (1)
examples/multimodal/utils/protocol.py (1)
26-26
: Remove unusednoqa
directive.Static analysis reports that the
# noqa: F401
directive on this line is no longer needed.As per static analysis hints
Apply this diff:
-from vllm.multimodal.inputs import MultiModalUUIDDict # noqa: F401 +from vllm.multimodal.inputs import MultiModalUUIDDict
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
container/Dockerfile.vllm
(1 hunks)container/deps/vllm/install_vllm.sh
(4 hunks)examples/multimodal/utils/protocol.py
(1 hunks)tests/conftest.py
(2 hunks)
🧰 Additional context used
🪛 Ruff (0.13.3)
examples/multimodal/utils/protocol.py
26-26: Unused noqa
directive (non-enabled: F401
)
Remove unused noqa
directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (4)
examples/multimodal/utils/protocol.py (1)
25-25
: LGTM! Import path aligns with VLLM 0.11.0.The import source change for
PromptLogprobs
fromvllm.sequence
tovllm.logprobs
correctly reflects the module restructuring in VLLM 0.11.0.container/Dockerfile.vllm (1)
18-20
: LGTM! Version bumps are consistent.The VLLM (v0.10.2 → v0.11.0) and FlashInfer (v0.3.0 → v0.3.1) version updates align with the PR objectives and are consistent with changes in
container/deps/vllm/install_vllm.sh
.container/deps/vllm/install_vllm.sh (2)
16-16
: LGTM! Version bumps are consistent.The VLLM (v0.10.2 → v0.11.0) and FlashInfer (v0.3.0 → v0.3.1) version updates align with the PR objectives and are consistent with changes in
container/Dockerfile.vllm
.Also applies to: 32-32
134-135
: LGTM! Cherry-pick block appropriately replaced with placeholder.The removal of the historical git cherry-pick and replacement with a TODO comment is appropriate for future maintenance.
import os | ||
import shutil | ||
import tempfile | ||
import time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove debug sleep blocking test execution.
The 4-hour delay in pytest_configure
will block all tests from running. This appears to be a debug artifact left in the code.
Apply this diff to remove the debug sleep:
-import time
-
import pytest
from tests.utils.constants import TEST_MODELS
def pytest_configure(config):
- time.sleep(60 * 60 * 4)
# Defining model morker to avoid `'model' not found in `markers` configuration option`
# error when pyproject.toml is not available in the container
config.addinivalue_line("markers", "model: model id used by a test or parameter")
Also applies to: 28-29
🤖 Prompt for AI Agents
In tests/conftest.py around line 20 (and also lines 28-29), there is a debug
time.sleep call that blocks test execution for hours; remove the time.sleep(...)
calls so pytest_configure no longer sleeps, and if the only use of the time
module was for these sleeps, also remove the import time line to avoid an unused
import.
Overview:
Do Not Merge
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit