Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: multilingual midel convert to tflite get wrong token #32079

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

Ayaa17
Copy link
Contributor

@Ayaa17 Ayaa17 commented Jul 19, 2024

What does this PR do?

when using multilingual whisper model and covert to tflite for end device use, always get wrong token.
Like this: this sample is voice transcription to chinese(zh)

ds = ds: {'audio': {'path': '/root/.cache/huggingface/datasets/downloads/extracted/81aac54528c46189de3348c82aaf37cfa33d9b1d3110a
f647d92b9e97280889d/zh-TW_dev_0/common_voice_zh-TW_17383559.mp3', 'sampling_rate': 16000}, 'sentence': '二話不說拉住她的手往回走',  'locale': 'zh-TW'}

before convert that  is work!
Tensorflow Transcription: 而話不說拉住他的手往回憶住

but after convert to tflite get wrong->
tflite transcription: : !!!

get token : generated_ids: [[50258 50260     0     0     0 50257 50257 ...... 50257 50257 ]]

It seem like TFForceTokensLogitsProcessor _force_token this function may happen overflow when converted

            new_scores = tf.ones_like(scores, dtype=scores.dtype) * -float("inf")

So change the way that new_scores retains the most negative values to avoid it

            new_scores = tf.zeros_like(scores, dtype=scores.dtype) + tf.constant([scores.dtype.min])

result:

# after fixed result
tflite transcription: 而話不說拉住她的手往回頭

convert model sample code can ref this

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? 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, and
    here are tips on formatting docstrings.
  • 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.
@gante @Rocketknight1

@amyeroberts
Copy link
Collaborator

cc @Rocketknight1 @gante

@Ayaa17
Copy link
Contributor Author

Ayaa17 commented Jul 29, 2024

Just a friendly reminder about PR review. If you have time, could you please take a look and provide any feedback, or if everything looks good, approve it? If you have any questions or need to make changes, please let me know.

Thank you so much!

Copy link
Member

@gante gante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, LGTM 👍 Thank you for fixing!

@gante
Copy link
Member

gante commented Jul 29, 2024

@Ayaa17 Ci is red because a test needs to be updated accordingly: tests/generation/test_tf_logits_process.py::TFLogitsProcessorTest::test_force_tokens_logits_processor

In a nutshell, instead of checking that the intended values are -inf, we now need to check that they are a very large negative value :)

@Ayaa17
Copy link
Contributor Author

Ayaa17 commented Jul 30, 2024

@gante I've understood the reason for the CI failure. Is there anything I need to help or should I wait for test_force_tokens_logits_processor to be modified the checking value?

thx a lot!

Copy link

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@gante
Copy link
Member

gante commented Aug 23, 2024

@Ayaa17 please go ahead and modify test_force_tokens_logits_processor with the new values (the values that are the result of your change) 🤗

@Ayaa17 Ayaa17 force-pushed the fix_TFLogitsProcessor branch 2 times, most recently from 33ca5d6 to dd77717 Compare August 25, 2024 05:38
@Ayaa17
Copy link
Contributor Author

Ayaa17 commented Aug 25, 2024

@gante , I have resolved the CI issue. Could you please review the changes again?
Thank you very much!

@Ayaa17 Ayaa17 requested a review from gante August 26, 2024 12:58
Copy link
Member

@gante gante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thank you for iterating 💛

@gante gante requested a review from LysandreJik August 26, 2024 14:50
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for your PR @Ayaa17

@LysandreJik LysandreJik merged commit 7562366 into huggingface:main Aug 27, 2024
20 checks passed
zucchini-nlp pushed a commit to zucchini-nlp/transformers that referenced this pull request Aug 30, 2024
…e#32079)

* fix: multilingual midel convert to tflite get wrong token

* fix: modify test_force_tokens_logits_processor the checking value as scores.dtype.min

---------

Co-authored-by: kent.sc.hung <kent.sc.hung@benq.com>
Co-authored-by: Aya <[kent831217@gmail.com]>
zucchini-nlp pushed a commit to zucchini-nlp/transformers that referenced this pull request Aug 30, 2024
…e#32079)

* fix: multilingual midel convert to tflite get wrong token

* fix: modify test_force_tokens_logits_processor the checking value as scores.dtype.min

---------

Co-authored-by: kent.sc.hung <kent.sc.hung@benq.com>
Co-authored-by: Aya <[kent831217@gmail.com]>
itazap pushed a commit to NielsRogge/transformers that referenced this pull request Sep 20, 2024
…e#32079)

* fix: multilingual midel convert to tflite get wrong token

* fix: modify test_force_tokens_logits_processor the checking value as scores.dtype.min

---------

Co-authored-by: kent.sc.hung <kent.sc.hung@benq.com>
Co-authored-by: Aya <[kent831217@gmail.com]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants