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

Replace accelerator.use_fp16 in examples #33513

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

hlky
Copy link
Member

@hlky hlky commented Sep 16, 2024

What does this PR do?

examples_torch tests are currently failing due to usage of accelerator.use_fp16, use_fp16 was removed in accelerate#3098 and transformers CI uses accelerate@main so the property is no longer available.

This PR replaces accelerator.use_fp16 with accelerator.mixed_precision != "no" which is equivalent to the removed use_fp16 property.

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?

cc @muellerzr

@muellerzr muellerzr requested a review from SunMarc September 16, 2024 21:52
Copy link
Member

@SunMarc SunMarc left a comment

Choose a reason for hiding this comment

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

Thx for fixing !

@SunMarc
Copy link
Member

SunMarc commented Sep 16, 2024

If we are using fp8, we need to pad to 16. Since these are simple examples + did not take into account fp8 from the start, I guess we can leave it like that. Otherwise, we would have to do the following

  if accelerator.mixed_precision == "fp8":
            pad_to_multiple_of = 16
        elif accelerator.mixed_precision != "no":
            pad_to_multiple_of = 8
        else:
            pad_to_multiple_of = None

@SunMarc SunMarc requested a review from amyeroberts September 16, 2024 22:49
@hlky hlky force-pushed the examples-accelerate-use_fp16 branch from 831a6e1 to 51bc1b9 Compare September 16, 2024 22:56
@hlky
Copy link
Member Author

hlky commented Sep 16, 2024

Thanks for the review, I've gone ahead and added pad_to_multiple_of = 16 for fp8.

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

You know a lot more than me about this one @SunMarc ! Juste wondering when was mixed_precision introduced? (before 1.0?)

@hlky
Copy link
Member Author

hlky commented Sep 17, 2024

Looks like mixed_precision was introduced a while ago in accelerate#247, the now removed use_fp16 property was changed to use mixed_precision at the same time.

@SunMarc
Copy link
Member

SunMarc commented Sep 17, 2024

Looks like mixed_precision was introduced a while ago in huggingface/accelerate#247, the now removed use_fp16 property was changed to use mixed_precision at the same time.

Thanks for looking into ! Looks like this was done in 2022, so with <= accelerate v.0.20. The min version of accelerate in transformers is v0.26.0.

Merging this since this should be safe to do

@SunMarc SunMarc merged commit 9f196ef into huggingface:main Sep 17, 2024
9 checks passed
@ArthurZucker
Copy link
Collaborator

Thanks both for checking!

BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
* Replace `accelerator.use_fp16` in examples

* pad_to_multiple_of=16 for fp8
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.

3 participants