-
Notifications
You must be signed in to change notification settings - Fork 218
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
Test Enzyme and reexport ADTypes.AutoEnzyme
#1887
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 10678833580Details
💛 - Coveralls |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1887 +/- ##
==========================================
- Coverage 86.79% 86.17% -0.63%
==========================================
Files 24 24
Lines 1598 1598
==========================================
- Hits 1387 1377 -10
- Misses 211 221 +10 ☔ View full report in Codecov by Sentry. |
Also if you want to disable the warnings you can set it like so (https://github.com/EnzymeAD/Enzyme.jl/blob/c29e6119c7963ddb22f1363726f762455748e193/src/api.jl#L414
|
You also may want to set the version to 0.11.2 since your CI currently is running at 0.11.0 ( |
@devmotion this PR (EnzymeAD/Enzyme.jl#914) should fix the immediate issues you see on CI if you want to try. |
CI running now 👍 |
@torfjelde looks like one of the 1.7 test fails. If you can open a MWE I can try to quickly fix |
Neato, though I think someone else will have to look into this as I don't have much time to spend on TuringLang-related stuff these days (and the limited time I do have, I need to spend on some other PRs). |
It looks like we obtained an incorrect gradient for a test case on Julia 1.7, which causes HMC to give poor results. |
Open an issue with an MWE!
…On Wed, Jul 31, 2024 at 3:33 PM Hong Ge ***@***.***> wrote:
It looks like we obtained an incorrect gradient for a test case on Julia
1.7, which causes HMC to give poor results.
—
Reply to this email directly, view it on GitHub
<#1887 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJTUXGFC2UMUPHXLHFT66DZPE3XTAVCNFSM6AAAAAAQXUOLRSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRRGI4DCNZUGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
cc @sunxd3 or @yebai for opening since I think @mhauru is out of office atm and @torfjelde is unavailable, for opening a 1.7 MWE. In interim you can probalby just merge the 1.10 support with 1.7 as marked broken. Once you have an MWE for us, I can investigate and fix |
cross posting my comment: what happens if you set Enzyme.API.runtimeActivity!(true) as the error message says? Turing already does this automatically so this isn't the cause of the issue at hand. |
bumping this @devmotion could we merge this with 1.10 support and @yebai and co can open an issue for 1.7 when they find the time for a follow up pr? that way we can make sure enzyme+turing's 1.10 suport doesn't regress |
Sorry, missed the latest updates while I was on vacation. I updated the PR (re-enabled tests with other AD backends) and bumped the version number. Given that most tests seem to pass with Enzyme now, I think we are more confident to re-export |
ADTypes.AutoEnzyme
I have a relatively strong view about adding more unit and integration tests to Enzyme instead of replying to packages like Turing to catch Enzyme failures. If this PR was merged, Turing CI might frequently be broken due to enzyme issues, which could be a big distraction. |
Marked as draft since this depends on |
I wonder if we should run Enzyme tests in a separate GH action, similar to e.g. separate nightly tests, that could be allowed to fail and maybe make it easier to see that everything else still passes? |
Totally up to you guys how you want to do testing, but I'd recommend you put it wherever you currently test AD packages (which I think is here?) and nevertheless these tests were useful historically and it would be good to ensure they run to catch regressions. And @yebai we've added relevant minimized tests to Enzyme for each o reelvant functinoality which caused issues in the past (we now have thousands of distinct tests in our suites). Happy to add more integration tests of course, if you have a PR, or want to add this repo as a downstream CI like here: https://github.com/EnzymeAD/Enzyme.jl/pull/1675/files I also don't think this PR depends on those PR's (as these tests pass regardless). Of course adding the tests in those repos is good too (and currently waiting action from the Turing side for MWE's), but I don't think a PR adding useful tests should be blocked on an unrelated PR which also adds other useful tests. |
Yes, it is a good idea to separate Enzyme tests into a separate CI task. We should consider doing the same for all tested autodiff backends. In addition, we can reduce CI time by testing gradient correctness only (see #2307). Also, testing a small, carefully chosen set of Turing models on Enzyme's CI suite is very helpful, too. |
bumping this. having tests is generally a good thing (and will prevent breakages in the future), and this PR does nothing different from how existing AD packages function. Totally up to you if you want to refactor how AD backend testing works, but that can be a follow up PR. |
@mhauru, can you adapt the following distributions and Turing test suite to help create an integration test PR for Enzyme?
These integration tests could supersede TuringLang/DistributionsAD.jl#254. |
A PR for adding Turing integration tests to Enzyme: EnzymeAD/Enzyme.jl#1813 Related issues that came up while making that: |
I think that PR requires this to be landed first. Integration testing
requires the downstream package to also include relevant CI to ensure any
API changes it makes doesn't introduce breakage, so we can make sure that
any changes on our end don't break you (rather than your package had an
issue). In other words both packages working together to prevent breakage :)
Relatedly we're about to drop before 1.10 so this PR should likely be
modified to only test 1.10+
Once this lands I'll review the downstream CI one on Enzyme.jl
…On Thu, Sep 12, 2024, 7:22 AM Markus Hauru ***@***.***> wrote:
A PR for adding Turing integration tests to Enzyme:
EnzymeAD/Enzyme.jl#1813 <EnzymeAD/Enzyme.jl#1813>
Related issues that came up while making that:
- EnzymeAD/Enzyme.jl#1812
<EnzymeAD/Enzyme.jl#1812>
- EnzymeAD/Enzyme.jl#1811
<EnzymeAD/Enzyme.jl#1811>
- EnzymeAD/Enzyme.jl#1807
<EnzymeAD/Enzyme.jl#1807>
—
Reply to this email directly, view it on GitHub
<#1887 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJTUXFO2N56HPETSRLWNCLZWF2N3AVCNFSM6AAAAAAQXUOLRSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBWGAZDEMZUGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Merging this before/at the same time as Enzyme's Turing CI makes sense. However, I think we need EnzymeAD/Enzyme.jl#1811 and EnzymeAD/Enzyme.jl#1812 fixed before merging this. I'm surprised that the Turing test suite didn't catch them, need to investigate why, maybe add some tests here. |
Note: This does not work yet
I opened this PR to make it easier to debug (and possibly fix) issues with Enzyme.
Currently, the following example does
notwork (note that the snippet does not require the PR which solely reexportsAutoEnzyme
at this point):With Enzyme#main my Julia (1.8.1) segfaults. An incomplete (it filled my whole terminal) output: https://gist.github.com/devmotion/1352197f2354c6fecddd7b778ec4bcf7#file-log-txtThe example works (latest releases of Turing, Enzyme, and ADTypes on Julia 1.10.0) but the following warnings show up: