Add integration tests for Bijectors #2037
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2037 +/- ##
==========================================
+ Coverage 67.50% 69.92% +2.41%
==========================================
Files 31 44 +13
Lines 12668 15660 +2992
==========================================
+ Hits 8552 10950 +2398
- Misses 4116 4710 +594 ☔ View full report in Codecov by Sentry. |
|
It is surprising to see so many broken tests, given that:
It's good to have this integration test in Enzyme. However, more comprehensive Enzyme unit tests should have caught the issues (even without the Bijectors integration test in this PR) so the community could have confidence in Enzyme. |
|
Sure happy to have this, but the linked errors need to be reduced to more minimal examples first |
|
Had to mark one more test as broken, CI now passes. As Hong said, I won't have time to minimise the issues I opened in the near future. Would you be happy to merge as is? Should be better than nothing, and we can add in more tests as we close the individual issues. |
|
I think the segfaults need further reductions at least, lest we not be able to mark them as broken |
|
I added back in a bunch of tests that were unblocked by the recent fixes. Now the only one marked with |
|
There's now a new seg fault I'm unable to reproduce locally. @wsmoses (or anyone else), would you have a Linux machine to try it on? I'm on Mac, CI runs on Ubuntu 22.04. |
| (3, 3), | ||
| ), | ||
| # TODO(mhauru) Both modes broken because of | ||
| # https://github.com/EnzymeAD/Enzyme.jl/issues/2035 |
There was a problem hiding this comment.
So this issue is essentially due to Enzyme 0.13 not being supported by the custom rule on bijectors on all uses of roots.jl to define rules.
x/ref: TuringLang/Bijectors.jl#339
I think this needs to be fixed in bijectors.jl before we can take this in here
There was a problem hiding this comment.
I would propose merging this as is while we fix this, to get the vast majority of the tests in this PR into CI. Bijectors is pinned to a specific version, so once the necessary changes have been made in Bijectors we can release a new version of IT, and then make a PR that marks this unbroken as it bumps the Bijectors version of these tests.
|
Tests pass and nothing is marked as |
There have been some regressions that cause various crashes and errors with Bijectors.jl and Enzyme v0.13. To guard against this in the future, I think it would be useful to introduce some integration tests for Bijectors, like we are doing in #1813 and #1819 for Turing.jl and Distributions.jl.
I've added a basic check of running forward and reverse mode on all bijector types with at least one argument, plus some extra ones that have caused trouble in the past. I've marked all failing ones, and reported them as separate issues. Here's a list:
cc @yebai