replace with torch.cuda.stream() - #33225
xinyu-intel wants to merge 5 commits into
Conversation
with torch.cuda.stream()
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase to replace the deprecated torch.cuda.stream() with the modern context manager usage on torch.Stream objects. The changes are generally correct and improve code clarity. However, I've identified a potential performance issue in one of the files where a new torch.Stream() is created in a loop, which can be inefficient. I've provided a suggestion to address this.
| @@ -1894,12 +1894,12 @@ def forward_impl( | |||
| # sync end point immediately after it is done. This is | |||
| # important to avoid excessive stream allocations by the cuda | |||
| # graph replay later. | |||
| with torch.cuda.stream(self.shared_experts_stream): | |||
| shared_experts_stream = self.shared_experts_stream or torch.Stream() | |||
There was a problem hiding this comment.
Creating a new torch.Stream() on every forward pass when self.shared_experts_stream is None is inefficient and can lead to resource exhaustion. The original code with torch.cuda.stream(None) was a no-op that ran on the current stream, which is more efficient.
To preserve the original behavior while using the new context manager syntax, you can use torch.cuda.current_stream() as a fallback. This avoids creating a new stream in a loop.
| shared_experts_stream = self.shared_experts_stream or torch.Stream() | |
| shared_experts_stream = self.shared_experts_stream or torch.cuda.current_stream() |
|
This pull request has merge conflicts that must be resolved before it can be |
e732754 to
a8a7333
Compare
|
Documentation preview: https://vllm--33225.org.readthedocs.build/en/33225/ |
|
This pull request has merge conflicts that must be resolved before it can be |
a8a7333 to
2173fc1
Compare
2173fc1 to
d3c42b6
Compare
|
Hi @xinyu-intel, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
eac6944 to
3353e7b
Compare
|
Please fix DCO |
3353e7b to
014a449
Compare
hmellor
left a comment
There was a problem hiding this comment.
You should also update the pattern in tools/pre_commit/check_torch_cuda.py to include stream
014a449 to
7d90e70
Compare
|
Hi @xinyu-intel, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
7d90e70 to
e60d07e
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
7f9a9a5 to
b316306
Compare
Signed-off-by: Xinyu Chen <xinyu1.chen@intel.com>
Signed-off-by: Xinyu Chen <xinyu1.chen@intel.com>
Signed-off-by: Xinyu Chen <xinyu1.chen@intel.com>
Signed-off-by: Xinyu Chen <xinyu1.chen@intel.com>
Signed-off-by: Xinyu Chen <xinyu1.chen@intel.com>
|
This pull request has merge conflicts that must be resolved before it can be |
Purpose
Part of #30679
In PyTorch, torch.Stream objects implement enter / exit and it should work well with context manager. So, replace
with torch.cuda.stream(astream):withwith astream:. For cases which astream can be None, use torch. accelerator.current_stream() instead.Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.