[fix] Fix positive masking in GIST losses with gather_across_devices#3817
Merged
tomaarsen merged 2 commits intoJun 12, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
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
GISTEmbedLossto protectoffset + rowrather than the local diagonal. - Update positive-protection masking in
CachedGISTEmbedLossto protectoffset + begin + rowper 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When
gather_across_devices=True,GISTEmbedLossandCachedGISTEmbedLossbuild thepositive-protection mask without the cross-device
offset, so on rank > 0 the wrong columns areprotected 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
beginfor its minibatch loop):but the positive mask did not:
On rank 1 with
world_size=2andbatch_size=3, the CE target for local rowris columnoffset + r(i.e.[3, 4, 5]), but the mask protects[0, 1, 2]. Withmargin > 0the truepositive itself exceeds
positive_sim - margin, so the absolute-margin false-negative suppressionsets the target logit to
-infand cross-entropy then returns+inf. This can be hidden in rank-0or single-process runs because
offset = 0.gather_across_deviceswas added to these losses in #3442, and #3453 patched the offset alignmentfor the in-batch-negative losses (
MultipleNegativesRankingLossand the cached / symmetricvariants), which select their identity mask by global index (
identity[local_batch]withlocal_indices = arange(offset, ...)). The two GIST losses were not part of that patch and kept theoffset-unaware mask.
This builds the mask with the same global index as the CE targets:
When
offset == 0this is identical to the previous mask, so rank 0 / single-GPU behavior isunchanged.
Tests
Added
tests/sentence_transformer/losses/test_gist_embed.pyandtests/sentence_transformer/losses/test_cached_gist_embed.py. They simulate rank 1 / world 2 in asingle process (monkeypatching the gather and rank helpers; the cached test parametrizes
mini_batch_sizesobegin = 0andbegin > 0are both covered) and assert that withmargin > 0the 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.