-
Notifications
You must be signed in to change notification settings - Fork 84
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
Makes cudnn a default executor #427
Conversation
for more information, see https://pre-commit.ci
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.
Thank you!
@parthmannan how do you feel about this from perf perspective? one concern might be the _make_cudnn_sdpa_*_graph
expense, but maybe that's 1) cheaper than i worry about, or 2) not that relevant because _cudnn_sdpa_checker
doesn't get called all that often anyway ?
Before:
After:
|
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.
would love to hear from someone smarter than me on the requirements.txt question (Jirka? Ivan?), but +1 from me
Co-authored-by: Jingyue Wu <wujingyue@gmail.com>
@tfogal I don't think we need to worry about this from a performance perspective as you pointed in 2. - I don't expect this to be called very often. Hopefully just the first iteration for static shapes if I am thinking about this correctly. What happens with dynamic shapes? We eventually plan to support that, can cuDNN create broader graphs that work for many shapes or will we need to call this everytime? |
for more information, see https://pre-commit.ci
This reverts commit b36d6c8.
@t-vi this is ready for your final review and merge. :) |
@vedaanta I think the test failures are real / caused by this patch:
I think we should hold off pending investigation; please take a look and let's double check we're not causing regressions here. |
Yeah, as @tfogal , points out, I think something is up in the tests. |
Co-authored-by: Vedaanta Agarwalla <vagarwalla@ipp2-1949.nvidia.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Jingyue Wu <wujingyue@gmail.com> Co-authored-by: Thomas Viehmann <tv@beamnet.de> Co-authored-by: Luca Antiga <luca@lightning.ai>
Before submitting
What does this PR do?
Cudnn is now a default executor.
The only operation targeted is sdpa.
The main change is a stricter checker function. Both forward and backward graph support are ensured before claiming sdpa operation.
(The checker was previously made lenient in #57)
Fixes #418.
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃