Repository navigation
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 760c3512cc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| assert frames.shape[2] == HEIGHT | ||
| assert frames.shape[3] == WIDTH | ||
|
|
||
|
|
There was a problem hiding this comment.
please add @pytest.mark.advanced_model to differentiate test case levels
There was a problem hiding this comment.
I've added the marker in the latest commit.
| sequence_parallel_size=num_gpus, | ||
| ulysses_degree=ulysses_degree, | ||
| ) | ||
| m = Omni( |
There was a problem hiding this comment.
maybe you can use omni_runner and omni_runner_handler in conftest.py, like e2e/offline_inference/test_qwen3_omni.py
There was a problem hiding this comment.
I tried to follow the setup but it looks like adding test fixtures for omni_runner and omni_runner_handler is not very necessary for these simple tests. I did extract the duplicated assertion logic into a shared helper to reduce code duplication though.
hsliuustc0106
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds a well-structured Ray executor backend for distributed diffusion model execution. The implementation follows vLLM's RayDistributedExecutor patterns and includes good test coverage for the main inference path.
✅ Validated
- Design follows vLLM patterns appropriately
- Unit tests cover worker initialization and error handling
- E2E tests demonstrate single-GPU and multi-GPU Ulysses SP working
- Proper placement group management and worker lifecycle
🔧 Changes Needed
-
Merge conflicts - PR needs rebase onto main
-
Missing test for
collective_rpc()- This is public API that downstream code may use for distributed coordination. A unit test would ensure it behaves correctly.
📝 Suggestions (non-blocking)
- The
to_cpu()method inDiffusionOutputmutates in-place. Consider adding a docstring noting this behavior for clarity.
Thanks for the detailed test evidence in the PR description!
| from vllm_omni.diffusion.executor.abstract import DiffusionExecutor | ||
|
|
||
| cls = DiffusionExecutor.get_class(mock_od_config) | ||
| assert cls is RayDiffusionExecutor |
There was a problem hiding this comment.
The collective_rpc() method is public API but has no test coverage. Consider adding a unit test to verify:
- It correctly calls
execute_methodon all workers - The
unique_reply_rankparameter returns only the specified rank's response - Timeout handling raises
TimeoutError
Example:
def test_collective_rpc_returns_all_responses(self, mocker, mock_od_config):
"""collective_rpc should aggregate responses from all workers."""
# Setup mock executor with workers
# Call collective_rpc with a method name
# Verify it returns responses from all workers
def test_collective_rpc_unique_reply_rank(self, mocker, mock_od_config):
"""collective_rpc with unique_reply_rank should return single response."""
# Verify only the specified rank's response is returnedThere was a problem hiding this comment.
Thank you for the suggestions! I've included these tests in the latest commit.
Signed-off-by: Binh Tang <tangbinhna@gmail.com>
Signed-off-by: Binh Tang <tangbinhna@gmail.com>
Signed-off-by: Binh Tang <binht@netflix.com>
Signed-off-by: Binh Tang <binht@netflix.com>
lishunyang12
left a comment
There was a problem hiding this comment.
Two things on shutdown/cleanup:
-
shutdown()usesray.kill()without graceful shutdown (ray_executor.py) — the mp executor sendsSHUTDOWN_MESSAGEand doesproc.join(30)beforeproc.terminate(). Here it's straight to SIGKILL.RayDiffusionWorkerWrapper.shutdown()exists but is never called. Consider callingmeta.worker.shutdown.remote()with a timeout first. -
__del__as cleanup fallback is unreliable — GC may never call it, or call it after Ray has shut down. The mp executor usesweakref.finalize()for this.
|
This PR was accidentally closed during rebasing and cannot be reopened (my apologies). Let's continue the review in PR #2106. |
Purpose
Add a Ray-based distributed executor for diffusion models, enabling multi-node GPU execution. Resolves #796.
The existing
MultiprocDiffusionExecutor(#865) uses local multiprocessing, which limits execution to a single node. This PR addsRayDiffusionExecutoras an alternative backend that uses Ray actors for worker management, supporting both single-node multi-GPU and multi-node execution across a Ray cluster.Design follows the same patterns as vLLM's
RayDistributedExecutor(vllm/v1/executor/ray_executor.py), adapted for the diffusion executor interface.Usage:
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist