Add Enzyme testing as a separate CI action#950
Conversation
9e9d2e0 to
96c7eaa
Compare
Benchmark Report for Commit 1f1e5e8Computer InformationBenchmark 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. |
mhauru
left a comment
There was a problem hiding this comment.
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).