Fix how we compute the final non-padding token for ForSequenceClassification models#35911
Conversation
4620592 to
8ccde63
Compare
|
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. |
|
cc @ArthurZucker for core maintainer review - but if you have too much to do, let me know and I'll find another reviewer! |
|
cc @Cyrilvallez actually, as core-maintainer-in-training |
Cyrilvallez
left a comment
Cyrilvallez
left a comment
There was a problem hiding this comment.
Hey! Nicely done, indeed much simpler than before! Just added some small comments! Let me know what you think!
| last_non_pad_token = -1 | ||
| logger.warning_once( | ||
| f"{self.__class__.__name__} will not detect padding tokens in `inputs_embeds`. Results may be " | ||
| "unexpected if using padding tokens in conjunction with `inputs_embeds.`" | ||
| ) |
There was a problem hiding this comment.
Instead of adding the warning everywhere, maybe we should do it the other way around and actually remove it from the few classes where it is present? It is a fairly niche case, and people using it should be aware of the caveat IMO. It's nice to have less warnings in general, WDYT? (It would allow to simplify branching as well)
There was a problem hiding this comment.
Hmm - I think a lot of classes actually support inputs_embeds as well as input_ids, so we still need this on most classes I think! It just only fires when users actually pass inputs_embeds and not input_ids.
There was a problem hiding this comment.
Be aware that it breaks torch.compile as well, would anyone want to use it! But once again, it is quite a niche case, and it's already here for some models, so I'll let you judge -- we can keep it if you think that removing it would bring confusion/error for users 🤗
There was a problem hiding this comment.
I think it's better to leave the warning, because the results will generally be wrong in that case. In future, we might consider raising an error and asking users to supply an attention_mask in that case!
Cyrilvallez
left a comment
Cyrilvallez
left a comment
There was a problem hiding this comment.
All right, LGTM! Thank you very much!
…ication models (#35911) * Fix how we compute the final non-padding token for Gemma (and probably other models) * .size() -> .shape[] * Propagating changes to other models * Propagating changes to other models * Change it for all ForSequenceClassification models * Fix batch dim * More TF fixes * Copy the TF fix around as well * Correct layer name for TFCTRL * Cleaner .to() * Clean up the nested if-else * Use argmax() instead of .max().values
…ication models (huggingface#35911) * Fix how we compute the final non-padding token for Gemma (and probably other models) * .size() -> .shape[] * Propagating changes to other models * Propagating changes to other models * Change it for all ForSequenceClassification models * Fix batch dim * More TF fixes * Copy the TF fix around as well * Correct layer name for TFCTRL * Cleaner .to() * Clean up the nested if-else * Use argmax() instead of .max().values
…ication models (huggingface#35911) * Fix how we compute the final non-padding token for Gemma (and probably other models) * .size() -> .shape[] * Propagating changes to other models * Propagating changes to other models * Change it for all ForSequenceClassification models * Fix batch dim * More TF fixes * Copy the TF fix around as well * Correct layer name for TFCTRL * Cleaner .to() * Clean up the nested if-else * Use argmax() instead of .max().values
BioGptForSequenceClassification was the last decoder classification head still using the legacy pooling index (input_ids != pad).sum(-1) - 1, which is the last real token only under right padding. With left padding the real tokens are shifted to the end, so the head pooled an earlier token and returned silently wrong logits and loss. Use the same left/right-padding safe selection as the other decoders (from huggingface#35911). Signed-off-by: Ting Sun <[email protected]>
…huggingface#46782) Fix left-padding token selection in BioGptForSequenceClassification BioGptForSequenceClassification was the last decoder classification head still using the legacy pooling index (input_ids != pad).sum(-1) - 1, which is the last real token only under right padding. With left padding the real tokens are shifted to the end, so the head pooled an earlier token and returned silently wrong logits and loss. Use the same left/right-padding safe selection as the other decoders (from huggingface#35911). Signed-off-by: Ting Sun <[email protected]>
3 participants
We have a lot of CLM models with
ForSequenceClassificationheads. These models are supposed to use the hidden state at the final non-padding token as the input to the classification head. However, the way they compute it is a bit weird - they get the index of the leftmost token that is equal topad_token_idand subtract 1 from it. This has a few issues:pad_token_idis absent, that need workaroundsargmax(), specifically that when multiple indices have the same maximum value, it always returns the smallest oneThis PR replaces that logic with simpler logic that actually searches for what we want, the rightmost non-padding token, not the token next to the leftmost padding token. This means the same logic works with left-padding, right-padding, no-padding, or even padding on both sides (I don't think any models do that, but we're ready if they do!)
Fixes #30004
Fixes #35352
Fixes #35909