Confusing error message
Author: jpiabrantes
Pull request: no
Labels: name: bug

name: Generation

https://github.com/huggingface/transformers/blob/a06a0d12636756352494b99b5b264ac9955bc735/src/transformers/generation/utils.py#L2022

The error message says:
> A decoder-only architecture is being used, but right-padding was detected! For correct generation results, please set `padding_side='left'` when initializing the tokenizer."

But the code does not check if the input `tokenizer.padding_side == "left"`. Instead the code checks if the last token id is a padding token which is often the case when people set `tokenizer.pad_token_id = tokenizer.eos_token_id`.

Comments:
Indeed! cc @gante, @zucchini-nlp 

gentle ping @gante @zucchini-nlp 

@jpiabrantes sorry for later reply. Do you mean there are cases when you want to generate from an input that ends with an `eos` token? 

The warning is mostly an advice for beginners who try to generate, since generating with right padding might result in gibberish or lower quality text.  So we point out to those who don't have much experience with generation, what are the best practices. If you already have the padding set on the correct side, you can ignore the warning

when generating in batches some generations will reach eos faster than
others, but the generation carries on.

however most importantly I think the error message should reflect what is
actually being checked - which is not the padding side.

On Wed, 11 Dec 2024 at 01:51, Raushan Turganbay ***@***.***>
wrote:

> @jpiabrantes <https://github.com/jpiabrantes> sorry for later reply. Do
> you mean there are cases when you want to generate from an input that ends
> with an eos token?
>
> The warning is mostly an advice for beginners who try to generate, since
> generating with right padding might result in gibberish or lower quality
> text. So we point out to those who don't have much experience with
> generation, what are the best practices. If you already have the padding
> set on the correct side, you can ignore the warning
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/huggingface/transformers/issues/34658#issuecomment-2535347988>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AASCMU4JDUSJATDAJMCIBFT2FADKTAVCNFSM6AAAAABRNVGP4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMZVGM2DOOJYHA>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>


@jpiabrantes yep, we could check the tokenizer's attribute directly but since tokenizer is not a compulsory kwargs when calling `generate()`, we opted to check the inputs. What I cane think now is to change the warning level in `warning.warn` to be suppressible 

>  some generations will reach eos faster than

True, but we don't try to generate from an EOS token right? When we just pass inputs to the `generate()` method, tokenizer doesn't add any EOS so it can be continued by an LLM

if 'tokenizer' in kwargs and kwargs['tokenizer'].padding_side == 'right':
  logger.log('Right padding side is being used with a decoder
only architecture')


On Wed, Dec 11, 2024 at 8:57 AM Raushan Turganbay ***@***.***>
wrote:

> @jpiabrantes <https://github.com/jpiabrantes> yep, we could check the
> tokenizer's attribute directly but since tokenizer is not a compulsory
> kwargs when calling generate(), we opted to check the inputs. What I cane
> think now is to change the warning level in warning.warn to be
> suppressible
>
> some generations will reach eos faster than
>
> True, but we don't try to generate from an EOS token right? When we just
> pass inputs to the generate() method, tokenizer doesn't add any EOS so it
> can be continued by an LLM
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/huggingface/transformers/issues/34658#issuecomment-2536551602>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AASCMUZZV2DQMCUKQVP6AGT2FBVINAVCNFSM6AAAAABRNVGP4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMZWGU2TCNRQGI>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>


Yes, that;s an option in case users decide to pass the tokenizer. The initial idea was to allow the tokenizer as arg in special cases like when we have `stop_str` or `assisted_decoding`. Would you like to open a PR for that? Feel free to tag gante and myself :)

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.