BLOOM
Author: younesbelkada
Pull request: yes

# What does this PR do?

Integrating BigScience converted models into HuggingFace library! 

Original PR: https://github.com/thomwolf/transformers/pull/2 that I directly moved here

- [x] add a generation test with a small model pushed on the hub
- [x] slow tests needs to be modified accordingly
- [ ] add final credits to all reviewers

cc @thomasw21 @thomwolf @sgugger @stas00 

EDIT: PR moved at https://github.com/huggingface/transformers/pull/17474

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

Hi @stas00 !

Thank you very much for your comment !!
1-
Yes it will be changed to BLOOM most probably in the next commit 

2-
I like the idea of making it modulable (enable/disable alibi and embed norm) so that both models would work. But in this case 13b-en could be understood as a smaller version of BLOOM which is not the case. What do you think?

3-
I managed to convert the small models using this arch therefore should not be a problem to use this arch for them. There is still the question of the naming (should they still be named BLOOM?)  

> 1- Yes it will be changed to BLOOM most probably in the next commit

Perfect!

> 2- I like the idea of making it modulable (enable/disable alibi and embed norm) so that both models would work. But in this case 13b-en could be understood as a smaller version of BLOOM which is not the case. What do you think?

That's a good point. It's a sort of Pre-BLOOM :)

> 3- I managed to convert the small models using this arch therefore should not be a problem to use this arch for them. There is still the question of the naming (should they still be named BLOOM?)

OK, let's discuss the last 2 on slack



2. Maybe something I don't understand is why make it modulable? It feels like having another GPT2 no? 13B should more of less fit inside GPT-2 no? 

> 13B should more of less fit inside GPT-2 no?

Well, we have gone through this 6 months ago, you can definitely re-read the discussions. There were 3 things that needed to be changed in HF's GPT2, which wasn't producing the same output under fp16.

Yeah I read it, `layernorm` is fixed by torch, the other two can be implemented there the same way they are implemented here (you just need to make it modular as well no?).

Unfortunately the `transformers`'s current policy is against making things modular. So we can't add anything to gpt2 

I thought that perhaps Bloom could be an exception but I won't be surprised if this will not be allowed.

Okay actually the `badbmm` was implemented there as well. So really we're missing only one activation which is jitted `gelu_fast`. Well why would we make this one modular to the point of really being REALLLY close to `gpt-2` then? If so the other course of action is building a new skeleton (which seems overkill for a change in activation).

yes already tried pushing for it - and it wasn't approved.

@sgugger thank you very much for your comments!
For the tokenizer since the models has not been pushed yet on the hub, I had to "hotfix" this by explicitly giving the path to the [bigscience tokenizer](https://huggingface.co/bigscience/tokenizer). Do you think it is a good idea to push this tokenizer to the [debug model's hub](https://huggingface.co/bigscience/bigscience-small-testing) ?
Also I have notived that Bloom is the only model on HF that does not have a slow tokenizer *and* has a fast tokenizer (usually it is either both or only the slow tokenizer). 

5 more small tests and we should be good!!

1 test left!

All tests finally passed!! I'll refactor the code with the suggested final changes and may ping you for a new review

I will need to modify the slow tests to add our custom ones

Just a small note on a test. Due to some stochasticity (since we are taking a random slice) [this test](https://github.com/younesbelkada/transformers/blob/cdf41e8f309a6744f4e1488bffc7be76503ccd6d/tests/models/bloom/test_modeling_bloom.py#L215) does not always pass with `atol=4e-2` (sometimes it passes with `5e-2` or `6e-2`). Therefore I've put `atol=1e-1` to be sure it passes. How accurate are we expecting this test to be? I may be wrong but I think the operations could not match at 100% (due to tensor slicing for example)

EDIT: Slow tests seems to work fine on the GPU, but batched generation seem to not work, I have to investigate that!

All tests are passing! Let me know if you need any more modification @LysandreJik @sgugger 

Thanks @thomasw21 for the comments!
Agreed with you, regarding the alibi positional embeddings. What I'll do I think is to create the positional embeddings on-the-fly on the forward pass (since you have access to the input sequence length there). I was just worried about the computational cost of it (re-computing alibi at each inference step is more costly than computing it once) - but for the reward we get (making the model agnostic to the sequence length) I think that it's worth it
 

>What I'll do I think is to create the positional embeddings on-the-fly on the forward pass 

I wonder if this may break deepspeed zero-3. All params should be created when the model is created. But if it's not a param it probably should be fine.

See this issue: https://github.com/microsoft/DeepSpeed/issues/1757

If by param you mean torch.nn.Parameter then the alibi tensor is not a param. Since these embeddings are not learned you can just use them as a non param tensor. I think that it should be fine and will not break deepspeed zero-3

Looks good so far! Think we have to revisit the `dtype` config param here though - I'm against adding it to the config IMO the user should define it at runtime by passing `torch_type` to the model and then the layers relevant logic should not be:

```py
if config.dtype == ...
```
but rather:
```py
if inputs_embeds.dtype == ...
```

cc @sgugger @stas00 

> Looks good so far! Think we have to revisit the `dtype` config param here though - I'm against adding it to the config IMO the user should define it at runtime by passing `torch_type` to the model and then the layers relevant logic should not be:
> 
> ```python
> if config.dtype == ...
> ```
> 
> but rather:
> 
> ```python
> if inputs_embeds.dtype == ...
> ```
> 
> cc @sgugger @stas00

Thank you for the comments! I agree with the fact that we should stay in line with what is done currently and should not add any extra logic. I have applied your suggested changes, there is no more explicit initialization with the dtype from the config + there should not be any logic such as `if config.dtype == ...` 
But I would still keep the `torch_dtype` param in the config file because in Megatron-DS this parameter is quite important, it helps explicitly keeping track on the precision that has been used during training.


> But I would still keep the torch_dtype param in the config file because in Megatron-DS this parameter is quite important, it helps explicitly keeping track on the precision that has been used during training.

FYI, `torch_dtype` gets automatically added to all saved models' config files since that feature was added, so you don't need to do anything special about it. Via `save_pretrained` that is.

> > But I would still keep the torch_dtype param in the config file because in Megatron-DS this parameter is quite important, it helps explicitly keeping track on the precision that has been used during training.
> 
> FYI, `torch_dtype` gets automatically added to all saved models' config files since that feature was added, so you don't need to do anything special about it. Via `save_pretrained` that is.

Yes but for bloom models I save the models + config files using the `convert_bloom_to_pytorch.py`script that does not use `save_pretrained`, that is also why I want to keep them in the config file

Ah, yes, in that case - yes, please, to adding manually the correct `torch_dtype` - thank you, @younesbelkada!

Added some final changes + suggestions from @thomasw21 ! Thanks to alibi shifting the tests pass with a much lower tolerance.
LGTM now, let me know if you see any other changes

Yeah let me close it and create a new PR!

Moved the PR at #17474

> The git commit history seems to be messed up - should we maybe open a new PR here?

It appears to be due to a broken merge commit here: https://github.com/huggingface/transformers/pull/17202/commits/06d98db76f9a958bd68d8158411674ba62856de1 Didn't really need a new PR, but rolling back the bad commit. But oh well it's done.