model(X, labels=Y, return_dict=True).loss is wrong

[jettjaniak]

it should be X, labels=X
ideally we would force it to do what it was supposed to, instead of shifting tokens on it's own
but if we can't we need to adjust the design of tokenization script to produce sequences of seq_len (512) instead of seq_len+1 (513)

[jettjaniak]

we need some performance test to catch issues like this in the future

[jaidhyani]

Don't we still want to pass it seq_len+1? If it's converting it to internally we still get 512 positions on inputs of length 513, right?

https://github.com/huggingface/transformers/blob/main/src/transformers/models/llama/modeling_llama.py#L1233

[SrGonao]

I agree with jai, we don't need to change the tokenizer. @jaidhyani could you make a PR to fix this loss?

[jaidhyani]

Already merged it a few days ago. Still need to add performance tests though

…

On Mon, Apr 29, 2024, 5:23 AM Goncalo Paulo ***@***.***> wrote: I agree with jai, we don't need to change the tokenizer. @jaidhyani <https://github.com/jaidhyani> could you make a PR to fix this loss? — Reply to this email directly, view it on GitHub <#133 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC5BYY5CGNHHIGJZV2XEF3Y7Y3VZAVCNFSM6AAAAABG4GQMJOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBSGU4TGMZZHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jettjaniak]

Don't we still want to pass it seq_len+1? If it's converting it to internally we still get 512 positions on inputs of length 513, right?

https://github.com/huggingface/transformers/blob/main/src/transformers/models/llama/modeling_llama.py#L1233

I'm not sure what you're linking to, probably line numer shifted with some new commits on main. I believe it'll compute logits for all input ids, and then discard logits for last position when computing loss.

https://github.com/huggingface/transformers/blob/ccdabc5642bf84849af93f591e207dc625c8e1e1/src/transformers/models/llama/modeling_llama.py#L1213-L1214

It's not a big deal, but considering this I think inputs should be seq_len. LMK if you have strong takes