Change in-place operations to out-of-place in LogitsProcessors #29680
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
gante
left a comment
gante
left a comment
There was a problem hiding this comment.
Looking good 👍
Missing: a test regarding that confirms that logits == scores when no processors are used in generate, and logits != scores otherwise.
|
|
||
| scores.scatter_(1, input_ids, score) | ||
| scores = scores.scatter(1, input_ids, score) | ||
| return scores |
There was a problem hiding this comment.
nit: we should do return scores_processed or return scores in ALL processors, for the sake of keeping a consistent pattern
gante
left a comment
gante
left a comment
There was a problem hiding this comment.
The changes look good 👍
One big question though: no doctests should have changed in the process. Do you know what is causing the change?
This reverts commit 4772768.
amyeroberts
left a comment
amyeroberts
left a comment
There was a problem hiding this comment.
Very nice 🔥 Thanks for working on this!
Quite a few of the changes don't seem to be necessary, but I'm assuming it's for consistency of having scores_processed returned.
| scores_processed = scores / self.temperature | ||
| return scores_processed |
There was a problem hiding this comment.
I find it surprising that this causes an inplace modification
In [8]: import torch
In [9]: x = torch.Tensor([1, 2, 3, 4])
In [10]: x
Out[10]: tensor([1., 2., 3., 4.])
In [11]: id(x)
Out[11]: 4339929136
In [12]: x /= 2
In [13]: id(x)
Out[13]: 4339929136
In [14]: x = x / 5
In [15]: id(x)
Out[15]: 10943842160There was a problem hiding this comment.
No, it was not causing any modifications on scores itself. That naming is for consistency only :)
| indices_to_remove = sorted_indices_to_remove.scatter(1, sorted_indices, sorted_indices_to_remove) | ||
| scores = scores.masked_fill(indices_to_remove, self.filter_value) | ||
| return scores | ||
| scores_processed = scores.masked_fill(indices_to_remove, self.filter_value) |
There was a problem hiding this comment.
Same for all the masked_fill calls here
There was a problem hiding this comment.
yep, naming consistency purposes again
| scores_processed = torch.full_like(scores, -math.inf) | ||
| scores_processed[:, self.bos_token_id] = 0 | ||
| return scores_processed |
Co-authored-by: amyeroberts <[email protected]>
Co-authored-by: amyeroberts <[email protected]>
4 participants
What does this PR do?
Fixes #29551 . In fact we do not really fix it. As @gante already explained why the logits and scores for contrastive decoding are same in that setting, because we did not apply any logits processors.
This PR changes all in-place operation on
scoresto out-of-place, so that thelogitsandscoresare actually different when logits processors are used. Actually, this is mostly copy-paste from PR forcompilecompatibility, we also has to get rid of in-place operations there.Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@gante