search

huggingface / transformers

🤗 Transformers: State-of-the-art Machine Learning for Pytorch, TensorFlow, and JAX.
https://huggingface.co/transformers
Apache License 2.0
131.47k stars 26.16k forks source link

SeamlessM4TFeatureExtractor fails with pad_to_multiple_of not being a multiple of stride #31916

Closed TechInterMezzo closed 4 weeks ago

TechInterMezzo commented 1 month ago

https://github.com/huggingface/transformers/blob/e314395277d784a34ee99526f48155d4d62cff3d/src/transformers/models/seamless_m4t/feature_extraction_seamless_m4t.py#L285-L290

I tried to study and understand the code for the SeamlessM4TFeatureExtractor. The code that I referenced above does not seem to be complete. num_frames is already the size for the second dimension, so the last two lines would not be doing anything. My guess is, that remainder should be subtracted from num_frames to get a size that is a multiple of stride. And because of the padding this was never a problem. Or is my thought process wrong?

amyeroberts commented 1 month ago

cc @ylacombe

ylacombe commented 1 month ago

Hey @TechInterMezzo, thanks for catching this, the code should indeed substract remainder from num_frames!

In the default parameters, this wasn't caught because stride == pad_to_multiple_of, but there could be some cases in which it's not the case. Would you mind opening a PR to correct this? You could add a test here to make sure there's no error when stride != pad_to_multiple_of and remainder!=0.

Let me know if this is something you're willing to do !

TechInterMezzo commented 1 month ago

Hi @ylacombe! I would like to give it a try at the end of the week. As this would be my first contribution to the transformers repo I will need to study the contribution guide first.

ylacombe commented 1 month ago

Great @TechInterMezzo, let us know if you run into any problems after reading the contribution guide!

TechInterMezzo commented 1 month ago

@ylacombe I created a pull request after following the contribution guide. I hope I did it right. The test I made is based on the existing call test with numpy inputs. It failed before the fix and worked after it.

romitjain commented 1 month ago

Thanks @TechInterMezzo for fixing this issue. @ylacombe Can we please get this merged? Would be helpful for me as well.

ylacombe commented 1 month ago

@ylacombe I created a pull request after following the contribution guide. I hope I did it right. The test I made is based on the existing call test with numpy inputs. It failed before the fix and worked after it.

You did it right, thanks ;)