-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
TST/MAINT: special: test with CuPy, make some CUDA fixes #20701
base: main
Are you sure you want to change the base?
Conversation
This is great! Very useful, glad you went ahead and did it. At some point, we may want to hook this into the normal tests. Perhaps there is a way to create a mock array API special extension or something. In any case, great. |
Unfortunately, these tests are truly deserving of the I also want to point out to everyone other than @izaid, that the name |
Thanks @steppi I only saw the email notice for this one late yesterday evening. I will take a look one evening this week if I have bandwidth, o/w it may need to wait for next weekend for me to take a look. |
0715140
to
c009b4d
Compare
@fancidev, I've made |
Thank you very much @steppi ! Are the CUDA tests added by this PR going to be included in the CI workflow? If included, it could be helpful for people without CUDA (like me) to fix the code errors by checking the error messages in the CI logs. Btw, if you split the code into a separate C++ library for the wider audience, I expect some C++ folks will frown upon changing things in the |
Not at the moment, the tests are 1) very slow, 2) require a machine with an NVIDIA GPU, and 3) SciPy's test suites already take very long to finish. We've discussed adding GPU CI after splitting the library off though.
You're preaching to the choir about injecting things into
When compiling for CUDA, everything is marked as |
Yeah, I'm not really bothered by this. Yes, theoretically, it's undefined behaviour. In practice, I've not seen it be an issue. I think we should try to make things be as easy for people developing the library as possible, and if that means injecting things into To be clear, we are simply putting things into
Yes, that is what |
Yeah, that's an important point I should have mentioned. When compiling for CUDA, there is no |
I took a look today. I was able to run the tests which all pass but the tests for cupy are skipped unless run on a gpu device (as described already in the thread). I was going to try to setup a gpu device to run the tests but I'm not able to access the account with gpu devices @steppi and I've been sharing) Albert are you able to access? If so that means it is something on my end I can work to resolve. |
I'm able to access the account without issue. I can dm you on the scipy community slack so we can troubleshoot. |
I think I figured it out this morning, sent the details in response to dm. Thanks! When I get some time this week I'll try to test it in the cuda. |
I setup a clean env using the DL ami in aws and i'm not able to get the test module here What I've done:
Is there an env var that maybe need to be set for the I'm also not sure how to confirm that the cuda kernel's are getting properly compiled with the |
Ah spoke/wrote too soon, I think I figured it out, @steppi I sent you a dm with the particulars, lmk if that looks correct to you and if so I can post the specifics. |
What does this implement/fix?
This PR makes a handful of fixes that are needed for some C++ special functions in
scipy/special/special/
to work in CuPy. I've also added a framework for testing that special functions work in CuPy; it autogenerates CuPyElementwiseKernel
s. It is incomplete in that it cannot handle ufuncs with signatures likedd*dd->*i
for those which return multiple arguments, and it cannot handle gufuncs. I have only added tests for a handful of functions, but plan to follow up with more tests for more functions (and likely more CUDA fixes).Additional information
@rlucas7, I whipped this up because you had mentioned you have some bandwidth to work on
special
this weekend, and I currently have no outstanding PRs. @izaid, you may be interested in this as well, but I know you are very busy at the moment.