Repository navigation
Conversation
|
yuxiaoxi seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Cambricon MLU devices, including the implementation of CNCLCheckpointEngine for collective communication and updates to device utilities and engine registrations. Feedback highlights the need to offload BroadcastOperation to a background thread in CNCLCheckpointEngine to prevent blocking the Ray event loop and to ensure proper synchronization in wait_for_complete. Additionally, it was noted that get_resource_name should be updated to explicitly support the "MLU" resource name for correct Ray scheduling.
| self.socket = socket | ||
| self.topic = topic | ||
|
|
||
| self._run() |
There was a problem hiding this comment.
The BroadcastOperation class is intended to perform operations asynchronously in a separate thread, as stated in its docstring. However, self._run() is currently called directly in __init__, which executes synchronously and blocks the event loop. This is a critical issue for Ray actors as it can lead to deadlocks or significant performance degradation when waiting for ZMQ messages or CNCL collectives. It should be offloaded to a background thread to allow the event loop to remain responsive.
| self._run() | |
| import threading | |
| self.thread = threading.Thread(target=self._run, daemon=True) | |
| self.thread.start() |
References
- Avoid blocking the event loop in Ray actors, as it can lead to performance degradation or deadlocks.
| async def wait_for_complete(self) -> dict[str, TensorMeta]: | ||
| """Wait for the broadcast operation to complete. | ||
|
|
||
| Returns: | ||
| dict[str, TensorMeta]: The bucket meta after broadcast. | ||
| """ | ||
| return self.metadata |
There was a problem hiding this comment.
Since BroadcastOperation should run in a background thread, wait_for_complete must actually wait for that thread to finish. The current implementation returns immediately, which could lead to race conditions where the next operation starts before the current broadcast is complete. Using asyncio.sleep in a loop allows the event loop to yield while waiting for the thread.
| async def wait_for_complete(self) -> dict[str, TensorMeta]: | |
| """Wait for the broadcast operation to complete. | |
| Returns: | |
| dict[str, TensorMeta]: The bucket meta after broadcast. | |
| """ | |
| return self.metadata | |
| async def wait_for_complete(self) -> dict[str, TensorMeta]: | |
| """Wait for the broadcast operation to complete. | |
| Returns: | |
| dict[str, TensorMeta]: The bucket meta after broadcast. | |
| """ | |
| import asyncio | |
| while self.thread.is_alive(): | |
| await asyncio.sleep(0.01) | |
| return self.metadata |
References
- Avoid blocking the event loop in Ray actors, as it can lead to performance degradation or deadlocks.
| def get_resource_name() -> str: | ||
| """Function that return ray resource name based on the device type. | ||
| Returns: | ||
| ray resource name string, either "GPU" or "NPU". | ||
| """ | ||
| return "GPU" if is_cuda_available else "NPU" | ||
| if is_npu_available: | ||
| return "NPU" | ||
| else: | ||
| return "GPU" |
There was a problem hiding this comment.
The get_resource_name function is missing support for Cambricon MLU. When running on MLU-based clusters, Ray typically uses the "MLU" resource name for scheduling. Returning "GPU" for MLU devices may lead to scheduling failures if the cluster is configured with MLU resources. Additionally, the docstring should be updated to reflect the support for MLU.
| def get_resource_name() -> str: | |
| """Function that return ray resource name based on the device type. | |
| Returns: | |
| ray resource name string, either "GPU" or "NPU". | |
| """ | |
| return "GPU" if is_cuda_available else "NPU" | |
| if is_npu_available: | |
| return "NPU" | |
| else: | |
| return "GPU" | |
| def get_resource_name() -> str: | |
| """Function that return ray resource name based on the device type. | |
| Returns: | |
| ray resource name string, either "GPU", "NPU", or "MLU". | |
| """ | |
| if is_npu_available: | |
| return "NPU" | |
| elif is_mlu_available: | |
| return "MLU" | |
| else: | |
| return "GPU" |
|
MLU will be supported via external plugin #6086 |
|
@uqyxx All training/inference backend plugin will be located in https://github.com/verl-project/verl-hardware-plugin |
hi @wuxibin89 do you have a rough timeline about verl-hardware-plugin? if verl-hardware-plugin will not be ready to accept contributions in a short time, can we merge this pr first and continue other implementations in verl-hardware-plugin? |
|
@uqyxx #6086 will be merged after v0.8 release, we will release v0.8 in next 2 weeks. Then you can develop MLU plugin in verl-project/verl-hardware-plugin |
|
@uqyxx Hi~ Currently, we have updated the PR #6086 and verl-hardware-plugin https://github.com/verl-project/verl-hardware-plugin. You can adapt according to the instructions. If there are any issues, please contact us promptly. |
What does this PR do?
Checklist Before Starting
[{modules}] {type}: {description}(This will be checked by the CI){modules}includefsdp,megatron,veomni,sglang,vllm,rollout,trainer,ci,training_utils,recipe,hardware,deployment,ray,worker,single_controller,misc,perf,model,algo,env,tool,ckpt,doc,data,cfg,reward,fully_async,one_step_off,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 (飞书群).)recipesubmodule, please also update the reference to the submodule commit viagit submodule update --remoteorcd recipe && git pull origin main.