Support BatchNorm in Hubert pos_conv_emb as in fairseq
Author: gallilmaimon
Pull request: yes
Labels: name: run-slow

# What does this PR do?
This issue adds support for BatchNorm instead of weight norm in the HubertModel as in https://github.com/facebookresearch/fairseq/commit/4db264940f281a6f47558d17387b1455d4abd8d9

The conversion file was also adapted to support the conversion from fairseq to HF, and was used to convert the widely used Hubert-base-25hz introduced in https://arxiv.org/abs/2305.13009

Fixes #34229

We already uploaded the converted weights to the hub at - https://huggingface.co/slprl/mhubert-base-25hz , which allows to assert the conversion worked correctly (compared to the original publication in textlesslib as follows):
```py
# Asserting that results are identical to textless original
from transformers import HubertModel
from textless.data.speech_encoder import SpeechEncoder
import torchaudio

model = SpeechEncoder.by_name(dense_model_name='mhubert-base-25hz', quantizer_model_name='kmeans', vocab_size=500, deduplicate=False, need_f0=False)
hf_model = HubertModel.from_pretrained('slprl/mhubert-base-25hz')

wav = torchaudio.load(<WAV_PATH>)[0]

torch.allclose(model(wav)['dense'], hf_model(wav, output_hidden_states=True).hidden_states[11])
``` 

@ylacombe - would love your review and specifically there were several open questions I was wondering about:
1) This means that HubertPositionalConvEmbedding is no longer a copy of transformers.models.wav2vec2.modeling_wav2vec2 - I addressed this but removing the comment, would you prefer I also change wav2vec?
2) The conversion script convert_hubert_original_pytorch_checkpoint_to_pytorch.py didn't work (before the change) for the regular hubert-base-ls960h model because of layernorm naming changes, as discussed in #26796. I didn't fix this because this felt out of scope.
3) I would love some guidance or help with deepspeed because I wasn't sure if any changes were needed to support this.
4) I also got some error when running `make fixup` which has to do with a file I haven't changed - `src/transformers/models/glm/modeling_glm.py` and I didn't manage to understand why. This also happened when running make fixup on a clean branch with no changes at all so would appreciate any help.

## 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#create-a-pull-request),
      Pull Request section?
- [x] 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.
- [x] 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.
@ylacombe 
@eustlb

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_34389). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

> The integration test you did looks good. Let's make sure to add this in the integration tests of `test_modeling_hubert.py` !

Okay, do you prefer that I do it with the textlesslib dependency in the test itself? or save the output and then just compare the output of HubertModel to the saved results from textlesslib?

> 2. This is a correct observation. Sorry that I've missed your comment about it! Would you like to open another quick PR to correct this ?

Okay, I will open a new one about this separately.

> 4. you might want to rebase your branch on the main `transformers`. If it doesn't work, you can share some logs here, so that I can help you!

I did do the rebase I think, but will try again and let you know


> Okay, do you prefer that I do it with the textlesslib dependency in the test itself? or save the output and then just compare the output of HubertModel to the saved results from textlesslib?

You can take a look at the [test modeling file](https://github.com/huggingface/transformers/blob/1d063793318b20654ebb850f48f43e0a247ab7bb/tests/models/hubert/test_modeling_hubert.py#L752) to get inspiration, we usually compute a few stats from the expected outputs, as well as a small sequence extracted from the expected outputs


@ylacombe Hey, I think I addressed all of your comments. Let me know if anything else is needed :)

@ylacombe Hey again, just a gentle reminder about this as I would be happy to integrate as soon as possible. Thanks again for your time and feedback!

@ylacombe 
sorry to bother about this. But I would really love for this change to be added.

@ylacombe - Hey again, just wondering if you had a chance to go over this so we can integrate this addition. There are several projects I know of which would build on this fix. Thanks again!

Let's push again the empty commit: git commit --allow-empty -m "[run-slow] hubert" ! I don't think it has run yet

cc @ArthurZucker , could you review when you have time?

@gallilmaimon, thanks again for the work on this PR! Excited to try the new checkpoint on downstream tasks. Have you been able to run some benchmarks against other Hubert checkpoints?


> @gallilmaimon, thanks again for the work on this PR! Excited to try the new checkpoint on downstream tasks. Have you been able to run some benchmarks against other Hubert checkpoints?

@ylacombe - My main usage is for discretising the representations and using them to train SpeechLMs and the results there seem good as expected (similar to the TWIST paper and notably better than the 50 hz), I will open-source this all hopefully very soon once it is ready!

But might also be interesting to try it for other downstream usages:)

Hey @ArthurZucker any chance you had an opportunity to review this PR? Would really love to integrate this :) Thanks!

Requesting @Rocketknight1's because @ArthurZucker has limited responsibility for a few days

My only request is to use official checkpoint path for the test, otherwise good job and sorry for being late on this review!

> My only request is to use official checkpoint path for the test, otherwise good job and sorry for being late on this review!

Hey @ArthurZucker, thanks for the review! I am not sure I understand what you mean by "official checkpoint" as this was only released as part of Fairseq and not HF, thus we preformed the conversion (as part of our academic lab - SLPRL) for community use. We validated that the results are identical as shown by the test. We of course give full reference and credit in the model card. I am happy to put the weights anywhere needed and merge the PR!

Hey @ArthurZucker, the model hasn't been officially released and can only be found by digging deep into the fairseq repositories. Since the model card is quite clean, gives full reference, and is hosted in the organisation of an academic research lab, I believe it should be OK to keep it like this, WDYT?  

@gallilmaimon, could you add the license (MIT I think?) to the model card metadata in the meantime? I've opened [a PR](https://huggingface.co/slprl/mhubert-base-25hz/discussions/1) to do this, if it's indeed MIT-licensed 

Hey @ylacombe, I approved your PR as this is in fact MIT licensed and I also have a link to the original license in fairseq GitHub!

I've asked some of the people on the research paper author lists if it would be possible to transfer the checkpoint to the Meta organization. In the meantime, let's merge, thanks for your excellent work!