-
Notifications
You must be signed in to change notification settings - Fork 27.7k
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 bf16 issue in text classification pipeline #30996
Conversation
@amyeroberts Hi, this is the new PR I mentioned in #30518 |
There was a problem hiding this 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 adding tests!
Some of the CI tests failing are unrelated, and I've triggered re-runs which should hopefully resolve these.
Some are related to this PR. For the quality checks, running make fixup
should resolve. For the others, specifically:
FAILED tests/test_pipeline_mixin.py::TextClassificationPipelineTests::test_accepts_torch_bf16 - TypeError: '<' not supported between instances of 'torch.dtype' and 'int'
FAILED tests/test_pipeline_mixin.py::TextClassificationPipelineTests::test_accepts_torch_fp16 - TypeError: '<' not supported between instances of 'torch.dtype' and 'int'
you'll need to debug :)
@amyeroberts Hello, I have fixed the bugs. The failing CI tests seem about tf or flax, but I guess they are unrelated to this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding!
For the failing tests - could you rebase on main to include the most recent upstream changes?
Hi, I synced with the upstream changes, but the failed tests remain. |
@amyeroberts Hi, could you help check the PR? I think this PR is quite simple. It can also facilitate many use cases as the bf16-based classifiers are becoming popular. |
Hi @chujiezheng, the PR itself all looks good, but there's some outstanding tests which are still failing which need to be resolved before we can merge. When you're merging in |
Yes, I did it just a few hours ago. You can notice the sync in the commits. |
@chujiezheng It's very odd. The tests are failing are ones which were failing about two weeks ago and have been resolved. I can see that there's merge commits in the history, although from this I'm not sure if it's possible to tell from which commit in Could you try a rebase:
|
The issue still exists... |
@chujiezheng Hmm, how odd 🤔 Thanks for trying the rebase. OK, as these tests aren't related to the pipeline, I'm going to forcibly merge. Thanks for your patience and for adding this! |
What does this PR do?
Fixes # (issue)
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
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.