-
Notifications
You must be signed in to change notification settings - Fork 232
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
Add Enzyme Forward mode custom rule #1869
Conversation
Putting this in |
Yeah it should be an extension package, but I was having difficulties getting that set up. Help is welcome! |
Don't have experience with that. |
Okay, I set CUDAEnzymeCoreExt up as an extension that pre-v1.9 is conditionally loaded using Requires. I'll leave it to others to decide if its better to make EnzymeCore a full dependency pre-v1.9 instead. |
need to rerun CI though now that Enzyme 11.8 is out which has mutual GPUcompiler compatibility |
Most of the functionality in the extension package is Enzyme-specific, so wouldn't it make more sense to make this a CUDA extension package inside Enzyme.jl? |
I think it makes more sense here since it just uses the lightweight interface-only package EnzymeCore, whereas it does need to know about some CUDA internals (and a lot more for reversemode). |
The Manifest wasn't updated, and there were typo's in the extension package that prevented it from parsing... But even with those fixed, CI doesn't pass:
Once this is fixed, it should be good to go, as I'm told other Enzyme rules live in their respective packages as well (instead of adding extension packages to Enzyme itself). |
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.
Couple of nits, and tests needs to be fixed.
Thanks to @vchuravy this is now much simplified, and tests pass locally. |
OK. I rebased the PR and re-triggered CI. |
Fails again during Enzyme tests. |
This is a different error than last time and I can reproduce this on |
@wsmoses is something blocking this? |
Tim saw that it (at least several Enzyme versions ago) had a nondetermism in CI, which I have been unable to reproduce. I've also not had time to investigate it more. But probalby rebasing/rerunning/giving it some love would be warranted if you have cycles. |
Fails to compile now, because of embedding a CuArray in a kernel. Missing Adapt rule for Duplicated?
|
Hm should be defined here https://github.com/EnzymeAD/Enzyme.jl/blob/5baf187dd2086aa47954e6e564b1bb244307a696/lib/EnzymeCore/ext/AdaptExt.jl#L7 edit: EnzymeAD/Enzyme.jl#1232 (comment) @maleadt was your test on 1.8? |
We should also standardize the extension name #2281 |
Disabling <=1.8 CUDA+Enzyme test for now. Does 1.9+ look good o you? |
Disabling the Enzyme test on 1.8 is fine, I guess. There's other Enzyme-related test failures though, in some of the special tests we only run if the essential ones pass. |
@maleadt the test failures here seem unrelated |
dc7ba69
to
dd64c38
Compare
Co-authored-by: Seth Axen <[email protected]> Co-authored-by: "William S. Moses" <[email protected]>
CI failure seems related again. |
Yeah rebase error, lets see if this is happy now |
@maleadt tests now look reasonable to me? |
There's still an Enzyme failure, now in the unified memory tests:
There's also a lot of log spam that I would like to see fixed:
and
These are all generated by the Enzyme tests:
|
@maleadt the unified memory tests now pass. LGTM? |
Yep, thanks! |
Wasn't sure how to integrate the sample test file into your infrastructure.