Skip to content

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
Contributor

@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
Contributor

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
@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.

2 participants