Add Pop2Piano
Author: susnato
Pull request: yes

# What does this PR do?

<!--
Congratulations! You've made it this far! You're not quite done yet though.

Once merged, your PR is going to appear in the release notes with the title you set, so make sure it's a great title that fully reflects the extent of your awesome contribution.

Then, please replace this with a description of the change and which issue is fixed (if applicable). Please also include relevant motivation and context. List any dependencies (if any) that are required for this change.

Once you're done, someone will review your PR shortly (see the section "Who can review?" below to tag some potential reviewers). They may suggest changes to make the code even better. If no one reviewed your PR after a week has passed, don't hesitate to post a new comment @-mentioning the same persons---sometimes notifications get lost.
-->

<!-- Remove if not applicable -->

Adds Pop2Piano model to HuggingFace.

Fixes #20126


## 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?
- [x] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a [link](https://github.com/huggingface/transformers/issues/20126)
      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).
- [ ] 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.

@ArthurZucker 
<!-- Your PR will be replied to more quickly if you can figure out the right person to tag with @

 If you know how to use git blame, that is the easiest way, otherwise, here is a rough guide of **who to tag**.
 Please tag fewer than 3 people.

Models:

- text models: @ArthurZucker and @younesbelkada
- vision models: @amyeroberts
- speech models: @sanchit-gandhi
- graph models: @clefourrier

Library:

- flax: @sanchit-gandhi
- generate: @gante
- pipelines: @Narsil
- tensorflow: @gante and @Rocketknight1
- tokenizers: @ArthurZucker
- trainer: @sgugger

Integrations:

- deepspeed: HF Trainer: @stas00, Accelerate: @pacman100
- ray/raytune: @richardliaw, @amogkam

Documentation: @sgugger, @stevhliu and @MKhalusova

HF projects:

- accelerate: [different repo](https://github.com/huggingface/accelerate)
- datasets: [different repo](https://github.com/huggingface/datasets)
- diffusers: [different repo](https://github.com/huggingface/diffusers)
- rust tokenizers: [different repo](https://github.com/huggingface/tokenizers)

Maintained examples (not research project or legacy):

- Flax: @sanchit-gandhi
- PyTorch: @sgugger
- TensorFlow: @Rocketknight1

 -->


Comments:
Hi @ArthurZucker the implementation is almost ready(also tests) but I feel that the way I implemented this model is not a descent level, thats why I want you to take a look at the `Pop2PianoModel` structure. 
Just to be clear with the feature extractor, the `Pop2PianoFeatureExtractor` takes raw_audio as input and generates variable length output(`10, 50000`, `15, 62200`), even if I pad the raw_audio at start, it will still produce different results for different audio files, so I used lists to stack them and then wrapped them through `BatchFeature`. 

Please don't mind about docs I will change them afterwards 

**EDIT : Please ignore this** 

*(Here is the author of pop2piano)* 
Thank you for doing this PR. It seems that this was implemented by understanding the original code better than me! Please feel free to ask me if there is anything I can check or do.

@sweetcocoa Thanks for you comments, HF team has helped me a lot in this integration.

For solving the import issues, you have to create a `require_xxx` with the name of the package. Look for example at the [`require_accelerate`](https://github.com/ArthurZucker/transformers/blob/c3a10a5dace55657a639789ad41fb4ded80e96fe/src/transformers/testing_utils.py#L259) in the `testing_utils.py`! 😉 


Hi @ArthurZucker thanks for you comment!
But I have already created `require_xxx` in `testing_utils.py` regarding `essentia` and `pretty_midi` and also I have used them in `transformers/src/transformers/models/pop2piano/__init__.py`. 

The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_21785). All of your documentation changes will be reflected on that endpoint.

Hi @ArthurZucker sorry for the delay but the tests are green now! please review it.

Hi @ArthurZucker , sorry for the huge delay, I have made most of the changes that you asked. Also there are some changes that I didn't do these are below - 
1. I managed to remove dependency on soundfile and torchaudio but not librosa, since raw_audio is used 2 times first time in `extract_rhythm` which takes audio with original sampling_rate and the second time in `single_preprocess` which first upscales/downscales raw_audio to sampling_rate of 22050 and then uses it. And preloading raw_audio with sampling_rate of 22050(not with native sampling_rate) was giving very bad results! I tried to use scipy.resample but since it uses fft it is relatively slow and less accurate.
2. As you suggested to pad the feature_extractor outputs with silence, I tried to do that but I found that different audio files with same length have input_features of different shapes! For example one was having shape of [7, 38, 512] and another one of [6, 42, 512], both were 10s audios. I could pad them and use them in a batch but then I need to keep track of their shapes which would introduce another variable, what do you suggest? Should I leave them or try to pad and keep track of shapes?   

The [licensing page](https://essentia.upf.edu/licensing_information.html) of `essentia` says - "Essentia is available under an open license, [Affero GPLv3](http://www.gnu.org/licenses/agpl.html), for non-commercial applications, thus it is possible to test the library before deciding to licence it under a comercial licence." I don't know much about licensing so I will leave it upto you to decide what to change in the headings.  


Also please forgive me if I missed something, I will change them in the future commit.

Hey! Will try to have a look asap

Pinging @sanchit-gandhi for a review too! 

Hi @ArthurZucker In have made the changes you requested except the resample one (I have mentioned the reason [here](https://github.com/huggingface/transformers/pull/21785#discussion_r1180702087)), let me know if more changes are needed or not.

Hi @sanchit-gandhi thanks for your comments! and sorry for the late reply, the batching is not working for these reasons  - 

1. **feature_extractor** -  The output of the feature extractor varies from music to music! For example - music1.mp3(10 seconds long) can have a feature extractor output of shape - [7, 38, 512] and music2.mp3(also 10 seconds long) can have a feature extractor output of shape - [6, 42, 512] . So if a user tries to process multiple music files in batch it will be very hard to batch them! 
- `truncation` - Truncating both of them to say [5, 25, 512] gives pretty bad results! 
 - `padding` - One way we can overcome this is, we can take the maximum dimensions on each axis and pad them, but then we must unpad them or get their original shapes back, otherwise the the tokenizer won't work! So we can make a variable just to  record the shapes of the tensors before padding. We can do this approach if you want.
 
I tried other approaches such as `torch.nested.nested_tensor` but there the user wont be able to use `.to("cuda") or .to("cpu")` on feature_extractor outputs because they are not supported!

2. **model** - The model.generate can take inputs of (dim1, dim2, dim3) but as we have (dim1, dim2, dim3) shapes for each input we may need to use a for loop  if we are to support batching!   

Also should I make a new PR regarding the assert for T5?

Hi @sanchit-gandhi, I pushed the modifications, tests which are failing are mostly due to internal errors(Connection to HF hub, TF installation etc).
Please ignore the all checkpoints as those are temporary. I will change all of them just before the merge.
Also please tell me if any more modifications are needed or not and also if I have missed any or not.

And in meantime I will make a PR regarding the change of T5 assert to except, I think maybe we should wait until that gets merged and then we will change the blocks to except here too? 

**EDIT** : pushed the change with the T5 modification.

pushed the change with the `T5` modification.

Also requesting review from @hollance!

Hi @hollance, I have made those changes you requested. And Hi @sanchit-gandhi, please review the batching part(except the checkpoint part as we discussed in slack if want to move them to a separate org or not), let me know if more code changes are required or not.
btw I was automatically removed from slack channel as it says `susnatodhar10@gmail.com’s Workspace’s free trial of Slack Pro has ended. Any channels with people outside your company — such as #pop2piano-integration — have been disconnected.`  
If anymore work is needed such as transferring the files to organization checkpoint, updating the HF Space for Pop2Piano ... please let me know I would be happy to do that!  

@susnato 

> btw I was automatically removed from slack channel as it says `susnatodhar10@gmail.com’s Workspace’s free trial of Slack Pro has ended. Any channels with people outside your company — such as #pop2piano-integration — have been disconnected.`

I added you back as a guest to the pop2piano-integration channel. You should be able to use this using regular Slack (not Pro). Let me know if that doesn't work.


Hi @sanchit-gandhi, I have pushed the new changes.

Alright nice! And you've verified that the outputs are the same with/without padding? Requesting review from @ArthurZucker and @hollance to kick-off the last round of reviews :)

yes I did check for 3 types - 1. single audio + no_attention_mask, 2.single audio + attention_mask, 3. 2 audios + attention_mask and the outputs were same. Since you just said, I will still add a test for that in the next commit(after last round of reviews). 

reviewing right now

Hi @ArthurZucker I have pushed the comments. Let me know if any more changes are needed or not.  

I'll @sanchit-gandhi review now, before pinging a core maintainer 

I have transferred the necessary files to `sweetcocoa/pop2piano` and updated the checkpoints. @sanchit-gandhi 

Thanks for updating again, @sanchit-gandhi has his hands full, I'll review this weekend! Sorry for the delay and great work! 🔥 

@ArthurZucker thanks for the quick reply, please don't worry about the delay, and thanks to the HF team for launching very intuitive audio course!  :hugs: 

Sorry, I missed the review request the first time! Rest assured it's on my list - I haven't forgotten @susnato! Aiming to have you a review either on Sunday or Monday afternoon latest 🙌

Hi @sanchit-gandhi, I have pushed the changes that you requested. Also answered the questions in the threads.

Cool thanks for the explanations @susnato, all good with me. Could you click "resolve" on all the threads that have been completed?

Hi @sanchit-gandhi, just resolved all other threads.

Sorry @susnato - could you resolve **all** the threads for this PR if they're completed? There are bunch still open from previous reviews. This will help the last person to review pin-point which bits are still pending (if any). Thanks!

Hi @sanchit-gandhi, sorry I misunderstood what you said at the first time, but now I have resolved all threads except the two you asked to leave.  

Perfect - thanks @susnato! This PR is now ready for final review 🤗

Hi @amyeroberts thanks for your comments! and I am so sorry that I took so long to do the changes and for resolving the discussions without any message(I will try to not do this from the next time), but now I have reopened those discussions which you commented on and replied those answers. Also I have pushed the changes that you requested except [this one](https://github.com/huggingface/transformers/pull/21785#discussion_r1264735735) (I have mentioned the reason in the thread).

Please let me know if more changes are needed or not and if I have missed any? I will surely pushed those in the next commit.   

Hi @amyeroberts, I added `Pop2PianoProcessor` along with its tests, addressed the comments that were due from the last review and pushed the changes that you requested, 

To answer your question -

> At the moment, most of the logic in the feature extractor and tokenizer are written to take a batch which means they're all having to implement their own looping logic. 

its because how `Pop2Piano` processes the audio files, just to give you an example -  suppose we have a single audio file, pop2piano feature extractor will convert the file into array of multiple batches(eg. `(70, 8, 512)`) even though there is only 1 file. That is why even to process a single file for all functions that is applied after `self.extract_rhythm` we need to implement the batching logic for each of them. I tried to change the logic in past but couldn't since the `self.extract_rhythm` always outputs arrays of this shape. I hope this explanation helps.

Let me know if this need more changes or not.

Hi @amyeroberts , I have pushed the changes you requested :)

Let me know if it needs more changes or not.

All checks are green now.

Hi @amyeroberts  I have made some major changes to the `tokenizer` - 
- I have implemented the `__call__` method which has the same design(accepts standard arguments as most other tokenizers) as other tokenizers in the library and converts notes(which is equivalent to tokens here) to ids.
- Renamed the previous call method as `batch_decode`.
- Aligned the processors's call method to both tokenizers and feature extractors, which compiles with the library and created a seperate `processor.batch_decode` method which calls the tokenizers `batch_decode` method.
- Worked on the comments you gave in the previous review.

It was my fault that I confused the tokenizer's `batch_decode` as the `__call__` this whole time, I am so sorry for this mistake.

If I am not wrong, now this tokenizer has almost the same design as others ~~except the fact that it does not have a vocabulary, I am currently working on that(as you suggested [here](https://github.com/huggingface/transformers/pull/21785#discussion_r1273406611))~~, (this tokenizer has a vocab now).


Please review it and let me know if the changes make sense or not and what changes are needed now. :) 

I have added a vocab(in fact there are 4 vocab files for 4 different token types "TIME", "NOTE", "VELOCITY" and "SPECIAL" and 1 with vocab params).

Please let me know if this complies with the library or should I squeeze them into one single nested vocab in the checkpoint and load a single file instead of five.

Hey @susnato huge effort, will review today / tomorrow, I need to dive a bit in all the review comments! 🤗 Almost there! 

Hi @ArthurZucker I have pushed the changes you requested except [this one](https://github.com/huggingface/transformers/pull/21785#discussion_r1282874305)(I have asked for some help about that on the respective thread).

BTW the failed `circle-ci` test is unrelated to this PR.

Hi @ArthurZucker I have added the `convert_pop2piano_weights_to_hf.py` file, added a single vocab and removed the complex logic of `_convert_token_to_id` and `_convert_id_to_token`. Also we don't need those extra properties anymore.

Please let me know if any more changes are needed to the tokenizer or not.

cc @sanchit-gandhi for a final review!

Hi @sanchit-gandhi I have pushed the changes. 

Hey @susnato! Thanks for pushing the latest round of changes - would you mind resolving any comment threads that you've addressed, so that the reviewer knows what's been implemented and what's still pending? Thanks!

Hi @sanchit-gandhi, done.

Hi @amyeroberts I have pushed the changes you asked and added more tokenizer tests.

Hi @amyeroberts I have pushed the change.

The checks are green! 

> Because of the structure of the model - it was a lot more complex than the average model PR to integrate this into the library. 

Yes it was relatively hard but I learned a lot about this amazing library from this integration! 
Also I will try to maintain the library structure and docs in future PRs! :D