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 float out of range in owlvit and owlv2 when using FP16 or lower precision #31657

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

aliencaocao
Copy link
Contributor

@aliencaocao aliencaocao commented Jun 27, 2024

What does this PR do?

As discussed in #31342 (comment)

The hard coded -1e6 value used to mask logits is not in range for FP16 which causes a value cannot be converted to type at::Half without overflow error. We replace that with the min value of whichever dtype the logits are in.

Did a search and only the owlvit and owlv2 models are using this, however, we should take note if any future model implementation hardcodes a similar value. There exists a couple more models using -1e7, -1e8, -1e9 and -1e10, but most of them are TensorFlow models, and the rest are either deprecated or language models. Since there has been no issue reports on this so far, I think it is properly handled somewhere so I won't change them.

Tests on FP32 showed no numerical difference in logits up to 1e-4 (what the test uses).

The reason of not using float('-inf') is that it can be 20% (RTX 3080Ti) to 3x (H100 SXM5) slower than using a predefined value for some reason.

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?

@amyeroberts

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks for fixing and for investigating across the models in the library ❤️

@aliencaocao
Copy link
Contributor Author

@amyeroberts Anything else to do before it can be merged? It would fix the last failing test for #31342

@amyeroberts
Copy link
Collaborator

Nope! I'll merge now. Thanks for pinging and reminding me, and sorry for the delay!

@amyeroberts amyeroberts merged commit e44b878 into huggingface:main Jun 27, 2024
18 checks passed
@aliencaocao aliencaocao deleted the fix-owlvit-OOB-float branch June 27, 2024 17:10
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.

None yet

2 participants