[Hardware][XPU] Register batch-invariant kernels for XPU - #41934
Conversation
…s for XPU Co-authored-by: GitHub Copilot Signed-off-by: tzielinski-habana <tomasz.zielinski@intel.com>
|
Documentation preview: https://vllm--41934.org.readthedocs.build/en/41934/ |
There was a problem hiding this comment.
Code Review
This pull request introduces experimental support for batch invariance on Intel XPU devices, covering operations such as bmm, log_softmax, softmax, and mean. It includes updated documentation, a new test suite for verifying correctness and invariance on XPU, and the necessary dispatch overrides. The reviewer identified that allow_override=True must be added to the torch.library.Library.impl calls in both the implementation and the test fixtures to prevent potential RuntimeError exceptions caused by conflicting registrations.
Signed-off-by: tzielinski-habana <tomasz.zielinski@intel.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Tomasz Zielinski <85164140+tzielinski-habana@users.noreply.github.com>
Signed-off-by: tzielinski-habana <tomasz.zielinski@intel.com>
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: tzielinski-habana <tomasz.zielinski@intel.com>
Signed-off-by: Tomasz Zielinski <85164140+tzielinski-habana@users.noreply.github.com>
Signed-off-by: tzielinski-habana <tomasz.zielinski@intel.com>
Signed-off-by: tzielinski-habana <tomasz.zielinski@intel.com>
Signed-off-by: tzielinski-habana <tomasz.zielinski@intel.com>
Signed-off-by: tzielinski-habana <tomasz.zielinski@intel.com>
Signed-off-by: tzielinski-habana <tomasz.zielinski@intel.com>
Signed-off-by: tzielinski-habana <tomasz.zielinski@intel.com>
yewentao256
left a comment
There was a problem hiding this comment.
Thanks for the work!
Could you add more context for this PR? Is there a lot of demands from users to realize this in XPU?
There was a problem hiding this comment.
Could we try to reuse current unit test instead creating new one?
There was a problem hiding this comment.
I was thinking about it before I marked the PR as ready for review. There are a couple of ways we can go and I wasn't sure which one would be most welcome by the community. Reusing existing tests would introduce some changes around the "skip_unsupported" decorator. I'd have to modify it to include checks for XPU and introduce separate checks for XPU ("skip_if_not_xpu"?) and CUDA ("skip_if_not_cuda"?). The name "skip_if_not_cuda" itself isn't very good, because it checks the condition CUDA and >= Ampere (SM80). All in all, I decided not to go this way, but if you like I can prepare a commit that refactors tests and you can decide which way works better.
There was a problem hiding this comment.
Also, regarding your question about more context and the demand - the context is, we need batch-invariance for reinforcement learning and this PR is basically the first step to enable it. I don't know about the demand for RL, but I asked the team and I'll let you know when I get the answer.
There was a problem hiding this comment.
@yewentao256 please see a preview of what the test refactoring would look like and let me know what you think:
tzielinski-habana#1
There was a problem hiding this comment.
I have same comment as @yewentao256 , please not creating another tests/v1/determinism/test_xpu_batch_invariant.py
There was a problem hiding this comment.
update to "skip_unsupported" decorator sounds reasonable to me. @wendyliu235 as well, Her team is working on migrate more cuda to run on XPU, might help to provide suggestion for the generalization work
There was a problem hiding this comment.
Ok, I'll merge my PR with testing improvements: tzielinski-habana#1
| def _register_matmul_overrides(lib, key: str): | ||
| """Register matmul overrides for batch invariance.""" | ||
| lib.impl("aten::mm", mm_batch_invariant, key) | ||
| lib.impl("aten::addmm", addmm_batch_invariant, key) | ||
| lib.impl("aten::matmul", matmul_batch_invariant, key) | ||
| lib.impl("aten::linear", linear_batch_invariant, key) | ||
|
|
||
|
|
||
| def _register_common_overrides(lib, key: str): | ||
| """Register batch-invariant overrides shared across CUDA and XPU.""" | ||
| lib.impl("aten::_log_softmax", _log_softmax_batch_invariant, key) | ||
| lib.impl("aten::softmax", softmax_batch_invariant, key) | ||
| lib.impl("aten::_softmax", softmax_batch_invariant, key) | ||
| lib.impl("aten::mean.dim", mean_batch_invariant, key) | ||
| # torch 2.12+ registers a built-in Triton bmm kernel for CUDA | ||
| # (torch._native.ops.bmm_outer_product), so we need allow_override | ||
| # to replace it at the dispatcher level. | ||
| lib.impl("aten::bmm", bmm_batch_invariant, key, allow_override=True) |
There was a problem hiding this comment.
Right, I assume you're asking this, because I put it with "common overrides" and not "matmul overrides". There are two reasons for this:
- bmm_batch_invariant has a dedicated Triton kernel (bmm_kernel) while the ops from matmul overrides all use matmul_kernel_persistent (with a small caveat - matmul_batch_invariant uses matmul_kernel_persistent in 2/3 if/else branches and bmm_kernel in the one remaining branch).
- more imporantly, the common overrides are unconditional for both CUDA and XPU, while matmul overrides are only registered for cuda if this condition holds: if current_platform.is_device_capability_family(80). So we need to keep these groups separate. If you prefer some other names for these functions, can you give me some suggestions?
There was a problem hiding this comment.
I don't like the refactor here, let's keep the change minimal in this file. we may use key = "cuda" or "xpu" outside, then
_batch_invariant_LIB.impl("aten::softmax", softmax_batch_invariant, key) directly instead of this function warpper
There was a problem hiding this comment.
I significantly reduced the scope of this PR. Got rid of the refactor and removed the common ops test. I'm also skipping matmul and linear registrations for now, because they require more work. I plan to add them in another PR later.
Signed-off-by: tzielinski-habana <tomasz.zielinski@intel.com>
|
Hi @tzielinski-habana, 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, |
| XPU_BACKENDS: list[str] = [ | ||
| "TRITON_ATTN", | ||
| ] | ||
|
|
||
|
|
||
| def skip_unsupported_xpu_backends(backend: str): | ||
| if current_platform.is_xpu() and backend not in XPU_BACKENDS: | ||
| pytest.skip(f"Backend {backend} not verified for batch invariance on XPU") |
There was a problem hiding this comment.
Currently all devices and backends are quite complicated, please organize them all together, design with a cleaner version.
There was a problem hiding this comment.
Ok, will do
| def enable_batch_invariant_mode(): | ||
| if not (current_platform.is_cuda() or current_platform.is_xpu()): | ||
| raise NotImplementedError( | ||
| "Batch invariance is only supported on CUDA and XPU platforms, " | ||
| f"got {current_platform.device_name}" |
There was a problem hiding this comment.
Please check if Rocm is enabled
There was a problem hiding this comment.
I don't have access to ROCm to check. I will remove this condition
| # Not all batch-invariant kernels are registered on XPU yet | ||
| # (e.g. attention, custom ops), so e2e determinism is not guaranteed. | ||
| if current_platform.is_xpu(): | ||
| pytest.xfail("Not all batch-invariant kernels registered on XPU yet") |
There was a problem hiding this comment.
e2e determinism is not guaranteed
We want it is guaranteed.
There was a problem hiding this comment.
Right, but that means the PR needs to be much larger. I wanted to register a subset of the kernels first and add the other ones later.
Signed-off-by: tzielinski-habana <tomasz.zielinski@intel.com>
yewentao256
left a comment
There was a problem hiding this comment.
Nice work! Looks much better, thanks!
Signed-off-by: tzielinski-habana <tomasz.zielinski@intel.com>
yewentao256
left a comment
There was a problem hiding this comment.
All E2E tests are xfail
# Not all batch-invariant kernels are registered on XPU yet
# (e.g. attention, custom ops), so e2e determinism is not guaranteed.
if current_platform.is_xpu():
pytest.xfail("Not all batch-invariant kernels registered on XPU yet")Are we sure this is what we want? I hope we can make sure at least one dense e2e like Qwen 3 could pass
@yewentao256 e2e tests are xfailed, because this PR is only the first step, out of two, to enable batch invariance on XPU. It's missing matmul and linear kernels registrations, because they are more problematic and require more code and a custom matmul kernel with tensor descriptors. I'm sure there will be quite a few review comments for the new code. I wanted to break it down for two reasons:
I had to rebase the branch a lot of times to make CI pass, because it is extremely unstable for some reason - every other time I got some interrupts/infrastructure issues. Now I see CI has finally passed, so if you're ok with that, I would love to merge this PR and then I'm going to submit the missing kernels in the next one. If not, I can expand this PR to cover end to end scenarios, but I think the more granular approach is better and we can deliver part of the functionality sooner. |
yewentao256
left a comment
There was a problem hiding this comment.
LGTM, thanks for the work!
Purpose
This pull request adds partial support for batch invariance on Intel XPU devices. This is a requirement for reinforcement learning on XPU.
Test Plan
Unit tests are modified to cover the registered kernels on XPU.
Test Result
All newly added unit tests pass.
Note
The code was co-developed with GitHub Copilot.