-
Notifications
You must be signed in to change notification settings - Fork 35
Add Enzyme testing as a separate CI action #950
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
Conversation
9e9d2e0
to
96c7eaa
Compare
Benchmark Report for Commit 1f1e5e8Computer Information
Benchmark Results
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #950 +/- ##
=======================================
Coverage 82.92% 82.92%
=======================================
Files 36 36
Lines 3964 3964
=======================================
Hits 3287 3287
Misses 677 677 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
96c7eaa
to
705bb68
Compare
DynamicPPL.jl documentation for PR #950 is available at: |
Thanks @penelopeysm -- good work. I'd separate the Enzyme test block into a script and put it under the test folder. Would you do the same for Bijectors and AdvancedVI, please? |
I'd love to do it for Bijectors! I know we talked about it a few times now, it's just never been at the top of my priority list 😬 But I figured we should get it right the first time here. |
I can probably stick the script in the integration test folder. Will do so, though will also wait for anything Markus wants to add on. |
EnzymeAD/Enzyme.jl#1813 should be merged together with this PR for mutual compatibility. |
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.
I like the idea of having it as script so I can run it locally too. Nothing else to add though, looks great to me.
Okay, should now be locally runnable with:
|
Pull Request Test Coverage Report for Build 15427231062Details
💛 - Coveralls |
Adds a separate CI workflow that loops over the demo models and tests Enzyme on them.
Closes #860
This gives us some guarantees that changes to DPPL retain compatibility with the current version of Enzyme, or at the very least tells us if it is broken. A lot of work on both sides has already gone into bringing it to this point and not testing it feels rather like a waste of that effort.
For example, accumulators broke Enzyme compatibility (#947) and I only found that out because I handled a merge commit wrongly, which broke ReverseDiff, which led me to check all other backends including Enzyme. I would really have appreciated being automatically informed about this, even if I wasn't going to go and fix it right away.
Rationale for adding it as a separate workflow rather than just adding AutoEnzyme into
test/ad.jl
(as was done in previous PRs like #813):ad.jl
).