Enable `conversational` pipeline for `GPTSw3Tokenizer`
Author: saattrupdan
Pull request: yes

# What does this PR do?

The `ConversationalPipeline` is great for easily running dialogue models, and also enables smooth interfaces in the associated Hugging Face Hub widget. These seem to require a `_build_conversation_input_ids` method on the associated tokenizer, however, which takes a `Conversation` object and encodes it into the chat format that the model was trained on.

With this change, we can now easily use the GPT-SW3 models. Here's an example of asking a single question:

```python
from transformers import pipeline, Conversation
chatbot = pipeline(model="AI-Sweden-Models/gpt-sw3-20b-instruct")
conversation = chatbot(Conversation("Hvad hedder du?"))
output = conversation.generated_responses[-1]
print(output)
```

And here is an example with a never-ending multi-turn dialogue session:

```python
from transformers import pipeline, Conversation
chatbot = pipeline(model="AI-Sweden-Models/gpt-sw3-20b-instruct")
conversation = Conversation()
while True:
    user_input = input('> ')
    conversation.add_user_input(user_input)
    conversation = chatbot(conversation)
    output = conversation.generated_responses[-1]
    print(output)
```

## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [ ] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [x] Did you write any new necessary tests?


## Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@Narsil @ArthurZucker @YouJiacheng @ekgren 

Comments:
_The documentation is not available anymore as the PR was closed or merged._

Hi @saattrupdan, thanks for this contribution and opening this PR.

As it stands, this isn't a change that we'd accept to be merged in. A few notes on why: 
* The pipelines are a higher level abstraction than the tokenizers, and so shouldn't be imported into a tokenizer's module. 
* The job of the tokenizer is to prepare raw text inputs for the model and decode its predicted tokens. `_build_conversation_input_ids` is higher level logic that belongs outside the class in e.g. a custom script. 
* It's not necessary to add `load_in_4bit` to the pipeline - the model can be instantiated with `ModelClass.from_pretrained(checkpoint, load_in_4bit=True)` and then passed into the pipeline. We try to keep the number of arguments in our public APIs as small as possible. 
* I think there might be a conflicting configuration, auto formatting from an IDE or different package version, but the line split changes in the PR shouldn't be there.








> Hi @saattrupdan, thanks for this contribution and opening this PR.

Thanks for your review @amyeroberts! 

> * The pipelines are a higher level abstraction than the tokenizers, and so shouldn't be imported into a tokenizer's module.

I've fixed this now, via @ArthurZucker's suggestion.

> * The job of the tokenizer is to prepare raw text inputs for the model and decode its predicted tokens. `_build_conversation_input_ids` is higher level logic that belongs outside the class in e.g. a custom script.

I'm a bit confused by this, as this method already exists for 9-10 tokenizers in the package (such as GPT2, Bloom, GPT-neox and more), and is also required by the conversational pipeline [here](https://github.com/huggingface/transformers/blob/469f4d0c29275473daf0627a0b26ec05256e47d2/src/transformers/pipelines/conversational.py#L256-L257). 

> * It's not necessary to add `load_in_4bit` to the pipeline - the model can be instantiated with `ModelClass.from_pretrained(checkpoint, load_in_4bit=True)` and then passed into the pipeline. We try to keep the number of arguments in our public APIs as small as possible.

That's fair enough if that's a design goal, I've removed it now. I just liked the idea of being able to instantiate a pipeline without having to load in the model first 🙂 

> * I think there might be a conflicting configuration, auto formatting from an IDE or different package version, but the line split changes in the PR shouldn't be there.

Ah right, I just thought it was a mistake that an 88 character line limit wasn't enforced - I've reverted the changes back now I think!

@saattrupdan @ArthurZucker OK, my bad, I hadn't noticed the `_build_conversation_input_ids` before - happy for that to be added then :) 

@ArthurZucker All formatting changes have been reversed now too 🙂 

Really nice! 

A quick comment from one of the developers of GPT-SW3, and the one responsible for the tokenization pipline.

Since there's a mismatch between the huggingface tokenizer and the sentencepiece tokenizer used during training, and how they treat special tokens, I'm a bit wary of this PR as it stands right now. To better match the training-procedure, each turn should be tokenized in isolation by the underlying sp_model, and joined with <bos>-tokens. This might result in the same thing, but I'm not 100% sure :sweat_smile: 

 

Regarding the special token issue, do you have small reproducer? I can have a look if needed! Currently working on our sentencepiece compatibility issues 

> @Apsod  Since there's a mismatch between the huggingface tokenizer and the sentencepiece tokenizer used during training, and how they treat special tokens, I'm a bit wary of this PR as it stands right now. To better match the training-procedure, each turn should be tokenized in isolation by the underlying sp_model, and joined with -tokens. This might result in the same thing, but I'm not 100% sure 😅

I just did some experiments to check this. The underlying sentencepiece model cannot deal with the special tokens, since these are dealt with by the `tokens_trie`, which is used in the `tokenize` method. Here's a sanity check:

```python
>>> tokenizer.tokens_trie.data
{'<': {'p': {'a': {'d': {'>': {'': 1}}}}, 's': {'>': {'': 1}}, 'u': {'n': {'k': {'>': {'': 1}}}}, '|': {'e': {'n': {'d': {'o': {'f': {'t': {'e': {'x': {'t': {'|': {'>': {'': 1}}}}}}}}}}}}}}
```

We see that it correctly deals with `<pad>`, `
`, `<unk>` and `
` special tokens. The `encode` method uses the `encode_plus` method, which uses the `_encode_plus` method, which finally uses the `tokenize` method, so using `encode` should be fine here, I think.

Note that, in the `tokenize` method, after the special tokens have been removed using the `tokens_trie`, the underlying `_tokenize` method is used to do the actual tokenization, which is implemented in the `GPTSw3Tokenizer` as

```python
def _tokenize(self, text: str, **kwargs) -> List[str]:
    text = self.preprocess_text(text)
    return self.sp_model.encode(text, out_type=str)
```

If I replace the `self.encode` with `self.sp_model.encode` in the new function that's being added in this PR, then I end up with an incompatible tokenization:

```python
>>> tokenizer.sp_model.encode('
Hej med dig
', out_type=str)
['▁<', 's', '>', 'Hej', '▁med', '▁dig', '<', '|', 'end', 'of', 'text', '|', '>']
```

If I'm completely missing the point here, @Apsod, then please let me know 🙂 

> If I replace the `self.encode` with `self.sp_model.encode` in the new function that's being added in this PR, then I end up with an incompatible tokenization:
> 
> ```python
> >>> tokenizer.sp_model.encode('
Hej med dig
', out_type=str)
> ['▁<', 's', '>', 'Hej', '▁med', '▁dig', '<', '|', 'end', 'of', 'text', '|', '>']
> ```

This is an edge-case where the semantic discrepancy between sentencepiece and huggingface tokenization leads to different results.

If we encounter `
` in text and tokenizes this using sentencepiece (as was done during training), it would tokenize this as `<, |, end, of, text, |, >` and not as the special eos-token, since in sentencepiece, special tokens are not textual and can never be produced by tokenizing text. 

I think there's also differences in how sentencepice treats the initial token after a special token (due to whitespace-prefix-stuff), which leads to a general mismatch between the tokenizers: 

```
TEXT = """



Hej


Hoj
""".strip()
print(tokenizer.decode(tokenizer.encode(TEXT))
# will print out the following:
#  

 Hej
Hoj
```

EDIT:

A simpler example of weird interactions between whitespace  and special tokens:
```
TEXT = """    Hej   
"""

print('"', TEXT, '"', sep='')
print('"', tokenizer.decode(tokenizer.encode(TEXT)), '"', sep='')
```
Results in: 

```
"    Hej   
"
"     Hej
"
```

@Apsod Thanks for the clarification. Just tried inspecting the result of using the `encode` method, and it removes some of the newline symbols. More specifically,

```python
prompt = "

\nUser:\nJag tycker träd är fina\n
\nBot:\n"
```

is being tokenised as `[3, 2, 15088, 63458, 18, 3947, 1886, 7590, 377, 6173, 2, 22493, 63458, 18]`, which translates token-by-token to "
\<s\>User:\nJag tycker träd är fina\<s\>Bot:\n". Notably, all newlines adjacent to a BOS token have been removed when encoded with this method.

I have been chatting to Amaru from the AI Sweden team (which might be you @Apsod? User names are always confusing!), and he said that they actually used multiple different prompts, sampled stochastically during training:

```
<eos><bos>{A}User:{B}{Query}{C}<bos>{A}Bot:{B}{Response}{C}...
A ~ ["\n", ""]
B ~ ["\n", " "]
C ~ ["\n", ""]
```

With this flexibility in mind, I propose that we change the above prompt to the following:

```python
prompt = "

User: Jag tycker träd är fina
Bot: "
```

I compared the encodings of the `encode` and `sp_model.encode` methods, and they now yield equivalent tokens. Here's the code that I ran to check:

```python
all_responses_encoded = [self.sp_model.encode(response) for response in all_responses]
sp_encoded_prompt = [self.eos_token_id, self.bos_token_id]
for response in all_responses_encoded:
    sp_encoded_prompt += response + [self.bos_token_id]
sp_encoded_prompt += self.sp_model.encode("Bot: ")

prompt = (
    f"{self.eos_token}{self.bos_token}"
    + f"{self.bos_token}".join(all_responses)
    + f"{self.bos_token}Bot: "
)
hf_encoded_prompt = self.encode(text=prompt)

assert sp_encoded_prompt == hf_encoded_prompt
```

Another thing: I looked into the mysterious extra whitespace added during decoding, and found that it's all due to these two lines in the `GPTSw3Tokenizer.convert_tokens_to_string` method ([link](https://github.com/huggingface/transformers/blob/66a378429d0e085e4e72bc63a4147889a3b65a14/src/transformers/models/gpt_sw3/tokenization_gpt_sw3.py#L233-L234)):
```
if not prev_is_special:
    out_string += " "
```

Is there any reason for this, or should it just be removed to ensure that `tokenizer.decode(tokenizer.encode(doc)) == doc`?

Looks good to me! 
The only outstanding issue then is special-token-injection, but I guess that is a more general HF-issue? 

> Looks good to me! The only outstanding issue then is special-token-injection, but I guess that is a more general HF-issue?

@Apsod  Great. I've changed the prompt now. I also added a TODO comment to clarify whether [these two lines](https://github.com/huggingface/transformers/blob/66a378429d0e085e4e72bc63a4147889a3b65a14/src/transformers/models/gpt_sw3/tokenization_gpt_sw3.py#L233-L234) are needed, as they break the decode(encode(doc)) == doc consistency. But that can be dealt with in another PR, if needed.

@amyeroberts @ArthurZucker I cannot seem to merge in this PR - do any of you need to re-approve it first?

@saattrupdan Yes, the branch is protected so that only certain people can merge. It also needs an approval from a core maintainer (me in this case :) )

Merging for you now. Thanks again for this contribution! 

Also regarding why spaces before / after special tokens is eating in the slow version of transformers:
- `add_tokens` does not support changing `lstrip` and `rstrip` thus by default it will strip. A fix is on its way here #23909 
- text after special tokens is not properly handled. This leads to addition of spaces. A fix is also on its way for T5 and Llama but should be pushed to all `spm` based models. #24622