Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
8 changes: 5 additions & 3 deletions deepspeed/runtime/sequence_parallel/ulysses_sp.py
Original file line number Diff line number Diff line change
Expand Up @@ -1145,10 +1145,12 @@ def forward(
bs, seqlen = x.shape[:2]

# flatten bs+seqlen to avoid having stride issues when narrowing into seqlen w/ bs>1
x = x.view(-1, *x.shape[2:])
y = y.view(-1, *y.shape[2:])
# reshape rather than view: a caller may pass a non-contiguous x, y or mask (a transposed or
# channel-sliced activation), for which view cannot produce the flattened shape
x = x.reshape(-1, *x.shape[2:])
y = y.reshape(-1, *y.shape[2:])
if mask is not None:
mask = mask.view(-1)
mask = mask.reshape(-1)
incoming_grad = torch.tensor(1.0, dtype=x.dtype, device=x.device)

# we are faking the incoming gradient, and since we perform a reduction outside of `autograd.backward` below we need to pre-adjust the incoming gradient. in the case of "sum" the gradient is 1.0, in the case of "mean" it's 1.0/num_elements, which in this case is 1/shards.
Expand Down
48 changes: 48 additions & 0 deletions tests/unit/ulysses_alst/test_tiled_compute.py
Original file line number Diff line number Diff line change
Expand Up @@ -401,3 +401,51 @@ def loss_fn(self, x, y):

# restore
MyModel.forward = MyModel.forward_orig


@pytest.mark.parametrize("shards", [2, 4])
class TestTiledFusedLogitsLossInputLayout:
"""
Same caller contract as TestTiledMLPInputLayout, on the loss. TiledFusedLogitsLoss flattens batch and
sequence into one axis before sharding, and a transposed activation cannot be flattened by a view.
"""

def make_model(self, hidden_dim, vocab_size, dtype):
model = Linear(hidden_dim, vocab_size, bias=False, dtype=dtype)
torch.nn.init.normal_(model.weight, std=0.02)
return model

@staticmethod
def loss_fn(model, x_shard, y_shard):
return torch.nn.functional.cross_entropy(model(x_shard), y_shard, reduction="sum")

def test_transposed_input_matches_a_contiguous_copy(self,
shards,
batch_size=2,
seqlen=12,
hidden_dim=16,
vocab_size=32):
dtype = torch.float32
torch.manual_seed(0)
# [bs, hidden, seqlen] transposed into [bs, seqlen, hidden] keeps the original strides
source = torch.rand((batch_size, hidden_dim, seqlen), dtype=dtype)
strided = source.transpose(1, 2).detach().requires_grad_(True)
assert not strided.is_contiguous(), "input is contiguous, so it does not exercise the flattening"
contiguous = strided.detach().clone().contiguous().requires_grad_(True)
y = torch.randint(0, vocab_size, (batch_size, seqlen))

model = self.make_model(hidden_dim, vocab_size, dtype)
losses, param_grads = [], []
for x in (strided, contiguous):
model.zero_grad()
loss = TiledFusedLogitsLoss.apply(self.loss_fn, model, x, y, None, shards, list(model.parameters()), "sum")
# The backward scatters into a zeros_like of the same flattened activation, so the
# gradient path runs through the flatten under test as well as the forward.
loss.backward()
losses.append(loss)
param_grads.append([p.grad.detach().clone() for p in model.parameters()])

torch_assert_close(losses[0], losses[1])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new test compares the two losses but never the gradient, and the flatten you changed feeds the gradient path as well: x_grad is a zeros_like of the flattened x, it is scattered per shard through x_grad.narrow(0, shard_offset, shard_step).view_as(x_shard) at ulysses_sp.py:1190, and unflattened at :1208. TestTiledMLPInputLayout, which your docstring names as the same caller contract, does assert x_tiled.grad against the reference.

The test also never calls backward(), so TiledFusedLogitsLoss.backward and that saved x_grad are not exercised at all.

Adding both passes at 307ff15, in a clean python:3.12-slim container with torch 2.14.0+cpu:

shards=2  loss_equal=True  x_grad_equal=True  max|dx|=0.000e+00
shards=4  loss_equal=True  x_grad_equal=True  max|dx|=0.000e+00

Concretely that is loss.backward() inside the loop, then comparing strided.grad with contiguous.grad after it. The parameter gradients match too, if you want the third assertion the MLP test makes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Both added in ac714ffloss.backward() in the loop, strided.grad against contiguous.grad, and the parameter gradients as the third comparison, matching TestTiledMLPInputLayout.

Same numbers you measured, on torch 2.13.0:

shards=2  loss_equal=True  x_grad_equal=True  max|dx|=0.000e+00  max|dW|=0.000e+00
shards=4  loss_equal=True  x_grad_equal=True  max|dx|=0.000e+00  max|dW|=0.000e+00

One thing I checked while doing this, since it changes what the assertion is worth: reverting the reshape and running the strengthened test fails at the forward flatten (ulysses_sp.py:1150, view size is not compatible ...) before backward is ever reached. So the gradient assertion is not what catches this particular bug — the forward already did.

What it does cover is the part your comment named that the forward does not. reshape on a non-contiguous input returns a copy, so x_grad is a zeros_like of that copy rather than of the tensor the caller handed in, and the shard scatter at :1190 and the unflatten at :1208 have to put those values back on the original layout. Nothing was checking that they did. max|dx| = 0.000e+00 against the contiguous reference is what says the copy does not scramble it, and grad.abs().sum() > 0 says it is not passing by both sides being empty.

Thanks — the test was asserting the shallower half of its own docstring.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch on the revert check, and you are right that it changes what the assertion is worth. The forward flatten fails first, so the gradient comparison is not the guard against this particular regression and I should not have implied it was.

What it does pin is the part nothing was covering: the scatter at :1190 and the unflatten at :1208 putting values back on the caller's layout after reshape hands back a copy. grad.abs().sum() > 0 alongside it is the right guard against both sides being empty.

torch_assert_close(strided.grad, contiguous.grad)
for grad_a, grad_b in zip(*param_grads):
torch_assert_close(grad_a, grad_b)
Loading