-
Notifications
You must be signed in to change notification settings - Fork 13
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
Check cl_khr_fp16 support #225
Conversation
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.
Test (commit 8f24069) failed in vega.
@nsakabe-fixstars Please check this branch. Does ClPy of this branch work on your environment? |
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.
Test (commit 8f24069) failed in titanv.
b8cada7
to
8e5154b
Compare
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.
Test (commit acc786b) failed in vega.
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.
Test (commit acc786b) failed in titanv.
8e5154b
to
bbc8af3
Compare
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.
Test (commit 7b0637e) failed in vega.
bbc8af3
to
ddb430d
Compare
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.
Test (commit 3c0be6a) failed in vega.
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.
Test (commit 7b0637e) passed in titanv.
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.
Test (commit 7b0637e) passed in titanv.
@vorj I confirmed it working on NVIDIA GPU w/o fp16 feature. Thank you. |
ddb430d
to
85fc8bb
Compare
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.
Test (commit 695f653) passed in vega.
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.
Test (commit 3c0be6a) passed in titanv.
@nsakabe-fixstars It's time to pass the baton. This PR has a remaining task, so please push this forward. You have! |
I ran the tests with 1080Ti(w/o fp16). Needed to skip: Need to skip cases:
|
2c3075b
to
bf0304f
Compare
bf0304f
to
43e8d6f
Compare
@LWisteria The CI has passed. Please review this PR. |
Of course this PR is too ad-hoc, so we should use this as temporary solution and will revert this in the future if this PR will be merged. |
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.
Skipping CI if no fp16 support is a bad idea. I won't approve of this PR.
@vorj Removing CI tests needing fp16 support looks better for me. |
@LWisteria You mean that we should remove all of codes related to |
@vorj Actually, you don't need to do that. It's just ideally better than this PR, that's why I won't approve. |
@LWisteria I didn't talk about my task, I just want to understand what you told more correctly. BTW, for my information, I want to know the usecase of skipping test, because it seems that my opinion about that is wrong. |
@vorj As you know, the primary reason for the problem is ClPy's insufficiency of fp16 support. Only passing CI must not be your goal. |
This sentence is true, then what the difference between removing the test cases and skipping them? IMHO, removing tests will solve nothing as well as this PR.
I haven't said that, I told that we should revert this in the future if we will merge this PR. I have never thought that this PR solves the core issue, but I think that broken CI should be fixed in short order. |
I meant that removing the test from CI, not from unit-tests itself, which guarantees functionalities of current and/or future ClPy. We don't have enough discussions to do that. And I won't agree with it now.
I agree. However, we must not treat symptoms, remove the bottom causes. We don't have to hurry so much. Now you waste your costs for the symptomatic treatment. |
Ah, OK, I understand. Thank you for your explanation.
Hmm, well, any user doesn't make the ticket like #239 recently, so I kind of agree. |
No, I had already wasted (it's just joke). |
After #269, this PR is obsolete. I'm closing but not deleting this branch for the further development. |
Task of #224 .
ClPy in this PR checks that the device supports
#pragma OPENCL EXTENSION cl_khr_fp16: enable
or not before first running kernel, and enable or disablecl_khr_fp16
adequately.Additionally, we also have the problem that
test_helper.py
blames DeprecationWarning withpytest>=5
. I also fixed this issue.Remaining tasks:
half
output when the device doesn't supportcl_khr_fp16