CLIP pooling is not compatible with adding new tokens
Author: okaris
Pull request: no

### System Info

Feature request (Duplicate of #21029)

For textual inversion in diffusers, we are adding tokens that have a higher token id than the eos token. So when we get clip embeddings for textual inv tokens, we need to change the pooling so it gets the eos token and not the arg max token.

Motivation

This is an issue that should be fixed as the clip embeddings won't work once we add more tokens to the tokenizer. This hasn't been a huge issue so far because most models use the hidden layers directly but [the new paper on SDXL](https://github.com/Stability-AI/generative-models/blob/main/assets/sdxl_report.pdf) also mentions using the pooled output now. 

@ArthurZucker @younesbelkada 

### Who can help?

_No response_

### Information

- [ ] The official example scripts
- [ ] My own modified scripts

### Tasks

- [ ] An officially supported task in the `examples` folder (such as GLUE/SQuAD, ...)
- [ ] My own task or dataset (give details below)

### Reproduction

1. Add new token to tokenizer
2. Encode tokens with CLIPTextModel
3. Get pooled output

### Expected behavior

Pooled output considers the added token ids vs eos id instead of argmax

Comments:
Are you taking this on @ydshieh ? 😉 

Not yet started, but self-assign so I won't forget. 

@okaris

If I understand correctly, the goal is to use the (fixed) eos id, rather than using `argmax`. Is this right?

@patrickvonplaten We need your experiste on `diffusers` for this issue 🙏 Thank you.

@ydshieh correct, because newly added tokens to the tokenizer take ids bigger than the eos id. The tokenizer config has the correct information but might not be readily available in the text model

Thanks @okaris 

Yes, the (text) model config file has `eos_token_id` being `2` which is even worse situation.  We can probably try to use `vocab_size - 1`, but I want to have some discussion with the team first to take action.

Oh yes, the text encoder has the wrong one. Length - 1 might not work if you are adding more tokens. The easy solution is to expose the eos id in the encoder model so it can be changed from the outside

This is indeed a problem here - @patil-suraj can you take a look here? 

Once we add more tokens (so `vocab_size` will change), I think one way to replace the `argmax` is to use `vocab_size - config.num_extra_tokens - 1`, where `num_extra_tokens` is a new attribute added to the config (default value would be `0`).

Happy to see if there are better/clean solution ideas.

when adding a token, the input embeddings are (and must be) resized. that length could also be used

I might be wrong, but isn't the new length of the (resized) embedding layer just the new `vocab_size`?

Thanks a lot for the issue @okaris !

IMO, updating the `eos_token_id` in the config would be better than adding a new `config` attribute. As far as I can tell, this should not break anything because `config` is never really used for tokenization, the `config` is used to get the `eos_token_id` if we are doing generation, but the CLIP model is not used for generation and also the current `config.eos_token_id` is incorrect, so updating the `eos_token_id` should be safe.  We can send a mass PR on the hub to update this (cc @patrickvonplaten ) 

What do you think @ydshieh @patrickvonplaten ?

> the CLIP model is not used for generation --> not break anything 

Sound correct! Let's have some word from the core maintainers (@amyeroberts and @sgugger) however.



Even if we don't use the `eos_token_id` from the config doesn't mean nobody else does! 

That being said, as @patil-suraj points out, the `eos_token_id` is wrong. I don't think it could be meaningfully or correctly used anywhere so happy for it to be updated. 

It makes me think we should add some tests to make sure the model and tokenizer mappings are aligned when added to the library - at least as an integration check for an example checkpoint. 

FYI: the inconsistency between config and the tokenier/processor is (one of) the main reason we **had** trouble in pipeline testing (using tiny models). I had make some extra work to avoid this problem (in the context of creating tiny models)

Fixed by #24777 

@okaris 

Let me know if this works well in your case, thank you!

Thanks @ydshieh looks like it will work for me as well.