Skip to content

[fix] Fix positive masking in GIST losses with gather_across_devices#3817

Merged
tomaarsen merged 2 commits into
huggingface:mainfrom
Incheonkirin:fix-gist-gather-positive-mask-offset
Jun 12, 2026
Merged

[fix] Fix positive masking in GIST losses with gather_across_devices#3817
tomaarsen merged 2 commits into
huggingface:mainfrom
Incheonkirin:fix-gist-gather-positive-mask-offset

Conversation

@Incheonkirin

Copy link
Copy Markdown
Contributor

Summary

When gather_across_devices=True, GISTEmbedLoss and CachedGISTEmbedLoss build the
positive-protection mask without the cross-device offset, so on rank > 0 the wrong columns are
protected and true positives can be suppressed.

In both losses the CE targets and the guided-similarity diagonal already use the rank offset (the
cached loss adds begin for its minibatch loop):

range_labels = torch.arange(offset, offset + batch_size, ...)        # CE targets
guided_sim = guided_ap_sim.diagonal(offset=offset).view(-1, 1)       # positive threshold (GISTEmbedLoss)
guided_sim = guided_ap_sim.diagonal(offset=offset + begin).view(-1, 1)  # CachedGISTEmbedLoss

but the positive mask did not:

# GISTEmbedLoss
positive_mask = torch.eye(*guided_ap_sim.shape, ...)                 # protects columns [0 .. batch-1]
# CachedGISTEmbedLoss
positive_mask = torch.eye(*guided_ap_sim.shape, ...).roll(begin)

On rank 1 with world_size=2 and batch_size=3, the CE target for local row r is column
offset + r (i.e. [3, 4, 5]), but the mask protects [0, 1, 2]. With margin > 0 the true
positive itself exceeds positive_sim - margin, so the absolute-margin false-negative suppression
sets the target logit to -inf and cross-entropy then returns +inf. This can be hidden in rank-0
or single-process runs because offset = 0.

gather_across_devices was added to these losses in #3442, and #3453 patched the offset alignment
for the in-batch-negative losses (MultipleNegativesRankingLoss and the cached / symmetric
variants), which select their identity mask by global index (identity[local_batch] with
local_indices = arange(offset, ...)). The two GIST losses were not part of that patch and kept the
offset-unaware mask.

This builds the mask with the same global index as the CE targets:

positive_mask = torch.zeros_like(guided_ap_sim, dtype=torch.bool)
rows = torch.arange(guided_ap_sim.size(0), ...)
positive_mask[rows, offset + rows] = True            # + begin in the cached minibatch loop

When offset == 0 this is identical to the previous mask, so rank 0 / single-GPU behavior is
unchanged.

Tests

Added tests/sentence_transformer/losses/test_gist_embed.py and
tests/sentence_transformer/losses/test_cached_gist_embed.py. They simulate rank 1 / world 2 in a
single process (monkeypatching the gather and rank helpers; the cached test parametrizes
mini_batch_size so begin = 0 and begin > 0 are both covered) and assert that with margin > 0
the loss stays finite and that the protected column equals the CE target. Both regression tests fail
on the current code and pass with the fix.

pytest tests/sentence_transformer/losses/test_gist_embed.py tests/sentence_transformer/losses/test_cached_gist_embed.py

Copilot AI left a comment

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.

Pull request overview

Fixes an offset/alignment bug in GISTEmbedLoss and CachedGISTEmbedLoss when gather_across_devices=True, where the “protect true positives” mask did not account for the cross-rank column offset (and minibatch begin in the cached variant), potentially masking out the true positive and producing +inf loss on rank > 0 with margin > 0.

Changes:

  • Update positive-protection masking in GISTEmbedLoss to protect offset + row rather than the local diagonal.
  • Update positive-protection masking in CachedGISTEmbedLoss to protect offset + begin + row per minibatch chunk.
  • Add regression tests simulating rank=1/world=2 to ensure the loss stays finite and the protected column matches the CE target.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
sentence_transformers/sentence_transformer/losses/gist_embed.py Fix positive mask to align with gathered column indices (rank offset).
sentence_transformers/sentence_transformer/losses/cached_gist_embed.py Fix positive mask to align with gathered column indices (rank offset + minibatch begin).
tests/sentence_transformer/losses/test_gist_embed.py Add regression/structural tests for rank>0 masking behavior in GISTEmbedLoss.
tests/sentence_transformer/losses/test_cached_gist_embed.py Add regression/structural tests for rank>0 masking behavior in CachedGISTEmbedLoss (incl. begin>0).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/sentence_transformer/losses/test_gist_embed.py
Comment thread tests/sentence_transformer/losses/test_cached_gist_embed.py

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@tomaarsen
tomaarsen merged commit bfba988 into huggingface:main Jun 12, 2026
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants