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
36 changes: 36 additions & 0 deletions tests/ops/test_chunk_loss.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
from types import SimpleNamespace

import torch

import veomni.ops.kernels.cross_entropy.chunk_loss as chunk_loss_module


def test_chunk_loss_reuses_valid_token_denominator(monkeypatch):
monkeypatch.setattr(chunk_loss_module, "get_parallel_state", lambda: SimpleNamespace(sp_enabled=False))

original_sum = torch.Tensor.sum
denominator_sum_calls = 0

def counting_sum(self, *args, **kwargs):
nonlocal denominator_sum_calls
if self.dtype == torch.bool and self.shape == (1, 5):
denominator_sum_calls += 1
return original_sum(self, *args, **kwargs)

monkeypatch.setattr(torch.Tensor, "sum", counting_sum)

hidden_states = torch.randn(1, 6, 4, requires_grad=True)
weights = torch.randn(8, 4, requires_grad=True)
labels = torch.tensor([[1, 2, -100, 3, 4, 5]])

loss, _ = chunk_loss_module.chunk_loss_function(
hidden_states,
weights,
labels,
chunk_size=2,
)
loss.backward()

assert denominator_sum_calls == 1
assert hidden_states.grad is not None
assert weights.grad is not None
3 changes: 2 additions & 1 deletion veomni/ops/kernels/cross_entropy/chunk_loss.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,10 @@ def ce_loss_func(hidden_states, weight, bias, labels, num_items_in_batch, ignore
return loss, logits

chunk_labels = torch.split(labels, chunk_size, dim=1)
num_items_in_batch = (labels != ignore_index).sum()

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.

high

The chunk_loss_function accepts an optional num_items_in_batch argument. Unconditionally overwriting it with (labels != ignore_index).sum() ignores any user-provided value for num_items_in_batch. This leads to incorrect loss scaling when a custom or global denominator is passed (e.g., in distributed training). Using a conditional assignment preserves the passed argument when it is not None.

Suggested change
num_items_in_batch = (labels != ignore_index).sum()
if num_items_in_batch is None:
num_items_in_batch = (labels != ignore_index).sum()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reusing the name num_items_in_batch rebinds chunk_loss_function's own parameter (line 95, num_items_in_batch: Optional[int] = None). The behavior is unchanged — main already discarded the caller's value by inlining the same expression — but this line now makes the parameter look like it is being consumed when it is in fact being overwritten.

That matters because the value can actually arrive: _chunk_loss_dispatch forwards *args, **kwargs verbatim, and num_items_in_batch is the 4th positional slot in the loss-function contract (see _LOSS_CALL_POSITIONAL_KEYS in veomni/trainer/callbacks/channel_loss_callback.py:54), so a caller doing gradient-accumulation-correct normalization would have its denominator silently dropped.

Two options, either is fine:

  • Cosmetic: name the local something like valid_token_count so the dead parameter stays visibly dead.
  • Or actually honor it: denominator = num_items_in_batch if num_items_in_batch is not None else (labels != ignore_index).sum().

If the intent is that chunk_loss_function deliberately owns its own denominator, a one-line comment saying so would prevent the next reader from filing this as a bug.


loss_kwargs_chunks = [
{"labels": chunk_labels[i], "ignore_index": ignore_index, "num_items_in_batch": (labels != ignore_index).sum()}
{"labels": chunk_labels[i], "ignore_index": ignore_index, "num_items_in_batch": num_items_in_batch}
for i in range(len(chunk_labels))
]

Expand Down
Loading