-
Notifications
You must be signed in to change notification settings - Fork 2.4k
[vllm] fix: use VLLM_SLEEP_LEVEL=1 on ASCEND NPU #3355
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
Conversation
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.
Code Review
This pull request introduces a fix to set VLLM_SLEEP_LEVEL to 1 when running on Ascend NPU devices. This is achieved by adding a check for NPU availability before potentially setting the sleep level to 2. The change is localized to the vLLM initialization logic and correctly uses a utility function to detect NPU presence. The implementation appears correct and effectively addresses the issue described in the pull request title.
verl/third_party/vllm/__init__.py
Outdated
| elif vs.parse(package_version) >= vs.parse("0.7.0"): | ||
| vllm_version = package_version | ||
| if vs.parse(package_version) >= vs.parse("0.8.5"): | ||
| if vs.parse(package_version) >= vs.parse("0.8.5") and not is_npu_available: |
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.
suggest writting like this:
if package_version is None:
xxx
elif is_npu_available:
VLLM_SLEEP_MODEL = 1 # sleep_mode=2 is not supported on vllm-ascend for now, will remove this restriction when this ability is ready.1e4af49 to
4631b57
Compare
What does this PR do?
Since the current vllm-ascend does not yet support sleep level = 2 - causing rewards to always be -1 - please set sleep level to 1 when using the NPU.
Checklist Before Starting
[{modules}] {type}: {description}(This will be checked by the CI){modules}includefsdp,megatron,sglang,vllm,rollout,trainer,ci,training_utils,recipe,hardware,deployment,ray,worker,single_controller,misc,perf,model,algo,env,tool,ckpt,doc,data,like[megatron, fsdp, doc]{type}is infeat,fix,refactor,chore,test[BREAKING]to the beginning of the title.[BREAKING][fsdp, megatron] feat: dynamic batchingTest
API and Usage Example
# Add code snippet or script demonstrating how to use thisDesign & Code Changes
Checklist Before Submitting
Important
Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=alwaysci-requestchannel in theverlSlack workspace. (If not accessible, please try the Feishu group (飞书群).)