[Mixtral] Fix loss + nits#28115
Conversation
ArthurZucker
changed the title
ConfigMixtral] default to not use windowed attentionMixtral] FIx loss + nits
younesbelkada
left a comment
younesbelkada
left a comment
There was a problem hiding this comment.
Thanks very much, the changes look great, I left one open question about the modeling file
| tie_word_embeddings=False, | ||
| rope_theta=1e6, | ||
| sliding_window=4096, | ||
| sliding_window=None, |
There was a problem hiding this comment.
should we completely remove all logic with respect to sliding_window in modeling mixtral?
There was a problem hiding this comment.
So no we can't
There was a problem hiding this comment.
In any case the mixtral team has already removed the sliding_window attribute in the config and it is already breaking as the argument is silently getting ignored in the modeling file: https://github.com/huggingface/transformers/blob/main/src/transformers/modeling_attn_mask_utils.py#L149 / https://github.com/huggingface/transformers/blob/main/src/transformers/models/mixtral/modeling_mixtral.py#L1063
There was a problem hiding this comment.
We can maybe deprecate the arg through a deprecation cycle
There was a problem hiding this comment.
As discussed offline, let's keep it for performance BC for users that have pushed a mixtral model with sliding window
Co-authored-by: Younes Belkada <[email protected]>
LysandreJik
left a comment
LysandreJik
left a comment
There was a problem hiding this comment.
Thanks for the offline explanation, LGTM
Co-authored-by: Lysandre Debut <[email protected]>
younesbelkada
left a comment
younesbelkada
left a comment
There was a problem hiding this comment.
Thanks! let's 🚢 it
4 participants
What does this PR do?
Properly compute the loss. Pushes for a uniform distribution.
fixes #28021
Fixes #28093