Skip to content
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions vllm_spyre_next/tests/test_silu_and_mul.py

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we still need this file if we reuse the upstream tests?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might still need to check for forward_oot dispatch, but other tests can go

Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
"""
Test SpyreSiluAndMul custom op correctness against a reference implementation.
"""

import pytest
import torch
import torch.nn.functional as F


def reference_silu_and_mul(x: torch.Tensor) -> torch.Tensor:
"""Golden reference: standard SiluAndMul (SwiGLU) in PyTorch.

Computes: silu(x[..., :d]) * x[..., d:] where d = x.shape[-1] // 2
"""
d = x.shape[-1] // 2
x1 = x[..., :d]
x2 = x[..., d:]
return F.silu(x1) * x2


@pytest.mark.spyre
@pytest.mark.siluandmul
@pytest.mark.parametrize("num_tokens", [1, 7, 63, 64, 65, 1024])
@pytest.mark.parametrize("d", [2, 63, 64, 65, 1024, 13824])
@pytest.mark.parametrize(
"dtype",
[
pytest.param(
torch.float16,
marks=pytest.mark.xfail(reason="Strided tensors on float16 does not work on spyre")
),
torch.float32,
],
)
def test_spyre_siluandmul_matches_reference(default_vllm_config, num_tokens, d, dtype):
"""SpyreSiluAndMul output matches golden reference.

Tests both paths:
- forward(): custom op dispatch (no-compile path via torch.ops.vllm.spyre_siluandmul)
- forward_oot(): direct Spyre device execution
"""
from vllm_spyre_next.custom_ops.silu_and_mul import SpyreSiluAndMul

torch.manual_seed(42)

# Input shape is [num_tokens, 2*d], output shape is [num_tokens, d]
x = torch.randn(num_tokens, 2 * d, dtype=dtype)
layer = SpyreSiluAndMul()

expected = reference_silu_and_mul(x)
actual = layer.forward_oot(x)

torch.testing.assert_close(actual, expected, atol=1e-2, rtol=1e-2)


@pytest.fixture
def dummy_tensor():
return torch.randn(4, 256, dtype=torch.float32) # 2*d=256, so d=128


def mock_forward_oot(x):
"""Mock: return ones with output shape (halves last dim)."""
d = x.shape[-1] // 2
return torch.ones(x.shape[:-1] + (d,), dtype=x.dtype, device=x.device)


@pytest.mark.spyre
@pytest.mark.siluandmul
def test_siluandmul_oot_dispatch(default_vllm_config, monkeypatch, dummy_tensor):
"""Verify SiluAndMul OOT registration: class swap and forward_oot routing."""
from vllm.model_executor.layers.activation import SiluAndMul
from vllm_spyre_next.custom_ops.silu_and_mul import SpyreSiluAndMul

layer = SiluAndMul()

# OOT class swap: SiluAndMul.__new__ should produce SpyreSiluAndMul
assert isinstance(layer, SpyreSiluAndMul)

# dispatch_forward should have selected forward_oot
assert layer._forward_method == layer.forward_oot

# Mock forward_oot (called by forward_oot) with a known transform
monkeypatch.setattr(layer, "forward_oot", mock_forward_oot)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes makes sense. I will update it

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bohnstingl I have updated this

out = layer.forward_oot(dummy_tensor)

# Expected: ones with shape [4, 128] (halved last dim)
expected = torch.ones(4, 128, dtype=dummy_tensor.dtype)
assert torch.allclose(out, expected)
23 changes: 12 additions & 11 deletions vllm_spyre_next/vllm_spyre_next/custom_ops/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,16 @@ def convert(tensor, device=None, dtype=None):
"""
if tensor is None:
return None
if tensor.device.type == "spyre":
# In case the tensor is on spyre, we first need to move it to cpu and then change the dtype.
if device is not None:
tensor = tensor.to(device=device)
if dtype is not None:
tensor = tensor.to(dtype=dtype)
else:
if dtype is not None:
tensor = tensor.to(dtype=dtype)
if device is not None:
tensor = tensor.to(device=device)
# In case the tensor is on spyre and a dtype change is requested

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bohnstingl I have lightly modified this function here. Earlier, if this function had received a tensor residing on spyre and the device was also passed as spyre, then this function would have failed earlier.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conversion now seems to be a little convoluted and the flow is not immediately obvious. I can't really think of a cleaner or more simplified way to do the device transfers. With your new function, the semantics changes a bit though. For example, if the tensor is on spyre and just a dtype change is requested, it will first convert the tensor to cpu, then do the conversion and then transfer it back to spyre.
What was the fail that you saw earlier?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, if the tensor is on spyre and just a dtype change is requested, it will first convert the tensor to cpu, then do the conversion and then transfer it back to spyre

Isn't this expected? I relied on this comment:

# In case the tensor is on spyre, we first need to move it to cpu and then change the dtype.

In the earlier definition, if I requested just a dtype change on a Spyre Tensor it would have failed.

>>> from vllm_spyre_next.custom_ops.utils import convert
>>> import torch
>>> x = torch.randn((2,2), dtype=torch.float32).to(device="spyre")
>>> x
tensor([[ 1.0254, -1.2343],
        [-0.9974, -0.4666]], device='spyre:0')
>>> convert(x, dtype=torch.float16)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/dev/shm/vllm-spyre/vllm_spyre_next/vllm_spyre_next/custom_ops/utils.py", line 68, in convert
    tensor = tensor.to(dtype=dtype)
             ^^^^^^^^^^^^^^^^^^^^^^
  File "/dev/shm/.venv/lib64/python3.12/site-packages/torch_spyre/_monkey_patch.py", line 63, in spyre_to
    return orig_to(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
RuntimeError: Spyre backend does not support type conversion yet during copy.

# we first need to move it to cpu and then change the dtype.
if tensor.device.type == "spyre" and dtype is not None:
tensor = tensor.to(device="cpu")
# Set the device to spyre to avoid unexpected device change of the return tensor.
if device is None:
device = "spyre"

if dtype is not None:
tensor = tensor.to(dtype=dtype)
if device is not None:
tensor = tensor.to(device=device)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I mentioned the other time, I think the logic here could be slightly improved to avoid unintended copies. Maybe something like this?

if tensor is None:
    return None

current_device = tensor.device.type

# Spyre doesn't support on-device dtype conversion
if current_device == "spyre" and dtype is not None and tensor.dtype != dtype:
    tensor = tensor.to(device="cpu")
    if device is None:
        device = "spyre"
                                                                                                                            
if dtype is not None and tensor.dtype != dtype:
    tensor = tensor.to(dtype=dtype)                                                                                         
if device is not None and tensor.device.type != device:                                                                     
    tensor = tensor.to(device=device)
                                                                                                                            
return tensor

return tensor
24 changes: 23 additions & 1 deletion vllm_spyre_next/vllm_spyre_next/testing/pytest_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,28 @@ def sort_key(item: pytest.Item) -> tuple[int, int]:
items.sort(key=sort_key)


def _convert_yaml_value(value):

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is added for future compatibility in case we want to override the torch data types

"""Convert YAML string values to Python objects where appropriate.

Handles torch dtype strings like "torch.half" -> torch.half.
"""
if isinstance(value, str):
import torch

dtype_map = {
"torch.half": torch.half,
"torch.float16": torch.float16,
"torch.bfloat16": torch.bfloat16,
"torch.float32": torch.float32,
"torch.float": torch.float,
"torch.float64": torch.float64,
"torch.double": torch.double,
}
if value in dtype_map:
return dtype_map[value]
return value


@pytest.hookimpl(tryfirst=True)
def pytest_generate_tests(metafunc: pytest.Metafunc) -> None:
"""Apply parameter overrides from YAML config."""
Expand Down Expand Up @@ -554,7 +576,7 @@ def pytest_generate_tests(metafunc: pytest.Metafunc) -> None:
for i, marker in enumerate(metafunc.definition.own_markers):
if marker.name == "parametrize" and marker.args[0] == po.param_name:
metafunc.definition.own_markers[i] = pytest.mark.parametrize(
po.param_name, list(po.values)
po.param_name, [_convert_yaml_value(v) for v in po.values]
).mark
break

Expand Down
15 changes: 15 additions & 0 deletions vllm_spyre_next/vllm_spyre_next/testing/upstream_tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,21 @@ tests:
# hidden_size: [64]
# block_list:
# - test: "test_fused_rms_norm_quant"
# Currently the tests fail because upstream rtol and atol are 0
# - rel_path: tests/kernels/core/test_activation.py
# allow_list:
# - test: "test_act_and_mul"
# mode: mandatory_pass
# tags: [siluandmul, upstream]
# params:
# allow:
# activation: [silu_and_mul]
# override:
# num_tokens: [1, 7, 63, 64, 65, 83, 1024]
# # Odd values fail op check. Not required for current list of models we plan to support
# d: [8, 64, 512, 13824]
# dtype: [torch.float32]
# device: [cpu]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still plan to use this eventually? To me it would also make sense to look more closely into how the changed vLLM IR does the testing? For example, the PR that recently landed in upstream vLLM introduces some new tests: https://github.com/vllm-project/vllm/pull/33825/changes#diff-510a5c10fb13605e7270f8b0647ed83a724b2658f0352af189ea1155c831ad3fR1.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still plan to use this eventually?

Upstream tests are too strict (rtol==atol==0). Locally, I tested the upstream test_act_and_mul test by editing the test to rtol==atol==1e-2 and they were passing. I don't think we would be able to use this test without parameterizing (atol, rtol) in the upstream test. I will raise a PR for that and see if upstream agrees to merge it. If that lands, we would be able to re-use it.

To me it would also make sense to look more closely into how the changed vLLM IR does the testing?

Agreed! But I believe we would also some changes on our end to implement new vLLM IR compatible ops and to enabled those tests. I am following your PR #877 and will go through the attached PR in more detail and see how we can adapt them.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be addressed with #884 or can we extend #884 to help with that?
cc @joerunde

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe!

It looks like this isn't currently set up to be easily patchable: https://github.com/vllm-project/vllm/blob/e69a265135ef48312d78130f64b7bfce4cd81a37/tests/kernels/core/test_activation.py#L106

    else:
        # The SiluAndMul, MulAndSilu, GELU and FatReLU implementations are
        # equivalent to the native PyTorch implementations, so we can do exact
        # comparison.
        torch.testing.assert_close(out, ref_out, atol=0.0, rtol=0.0)

We could set up something like this that edits assert_close to override cases where 0.0 is passed:

@pytest.fixture
def patch_tolerances(monkeypatch):

    original_assert_close = torch.testing.assert_close

    def patched_assert_close(*args, atol, rtol):
        if atol == 0.0 and rtol == 0.0:
            return original_assert_close(*args, atol=0.5, rtol=rtol)
        return original_assert_close(*args, atol=atol, rtol=rtol)

    monkeypatch.setattr(torch.testing, "assert_close", patched_assert_close)

    yield

but we'd definitely still want to update upstream to make those parameterizable, or have them pull from a common method we can override

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bohnstingl @joerunde
I have tried to solve this by adding a fixture to monkeypatch torch.testing.assert_close

Please see: https://github.com/vllm-project/vllm-spyre/pull/863/changes#diff-035ed2d7ff0ff23d9dfc96c0f483503dbb4c16529e7ea88192e717621f4da830R637

With this, we are able to enable the upstream SiluAndMul test, but please note that it will only pass for fp32 right now, since fp16 implementation is broken in Spyre.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FP16 failure is related to this internal Slack thread.

In compile mode, if we send FP32 data to the SpyreSiluAndMul layer, internally the layer does this

x1 = x[..., :d]
x2 = x[..., d:]

and then converts the tensors to FP16. The dtype conversion makes x1, x2 contiguous. And then we transfer the tensor to the Spyre device.

But if we send FP16 data to the SpyreSiluAndMul layer, conversion to FP16 is a no op. When we transfer the tensor to the Spyre device, x2 is exactly the same as x1.

This is an issue in torch-spyre, and as I understand from the comments, strided view copies are not yet reliable in torch-spyre.

- rel_path: tests/models/language/generation/test_common.py
allow_list:
- test: "test_models"
Expand Down
Loading