Skip to content
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 tests #254

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Add Enzyme tests #254

wants to merge 12 commits into from

Conversation

devmotion
Copy link
Member

I had tried to run Enzyme tests, and it seems currently quite a few tests fail/error.

@yebai
Copy link
Member

yebai commented Jul 17, 2023

The fact that it doesn't segfault in so many tests is a good sign.

Related: TuringLang/Turing.jl#1887

@wsmoses
Copy link
Collaborator

wsmoses commented Aug 1, 2023

@devmotion can you rerun on latest Enzyme main?

@wsmoses
Copy link
Collaborator

wsmoses commented Aug 5, 2023

fyi the test failures appear to be due to currently unimplemented derivatives of lapack functions. probably should disable those tests.

@wsmoses
Copy link
Collaborator

wsmoses commented Aug 7, 2023

@devmotion @yebai at this point, everything in this PR works except:

  • differentiating through Lapack functions is not yet supported by Enzyme
  • your test code uses an active return for a function which returns a vector (this must be duplicated).

Moreover all the errors are relatively nice.

at this point there aren't Enzyme bugs here and merging this is now on your end (presumably marking the code using lapack as unsupported atm, and fixing your test handler to not use an active vector for that input)

@devmotion
Copy link
Member Author

Thank you, I'll try to have a look at and fix the remaining issues this week.

@devmotion
Copy link
Member Author

The latest commit added a way to mark some Enzyme tests as broken (and I also introduced a bug in the test setup 😄) but it also reveals that there are many more issues - so far only a tiny initial testset was run but no tests of the log densities of the uni-, multi- and matrixvariate distributions we're actually interested in.

@devmotion
Copy link
Member Author

@wsmoses
Copy link
Collaborator

wsmoses commented Aug 16, 2023 via email

@yebai
Copy link
Member

yebai commented Aug 16, 2023

One slightly annoying thing I found about the Enzyme error message is printing a large amount of LLVM code when compilation fails (see here for an example). This is useful when developing Enzyme (at least in the early stages) but not very helpful for users of Enzyme. Very few people can read such LLVM code and figure out why Enzyme couldn't compile. They also add clutter to CI logs. I suggest disabling such LLVM messages by default but providing an API for developers to activate them when needed.

@wsmoses
Copy link
Collaborator

wsmoses commented Sep 14, 2023

@devmotion did you have a chance to isolate that 1.6 error that crashes Julia (so we can ensure it does not)?

@wsmoses
Copy link
Collaborator

wsmoses commented Sep 22, 2023

@devmotion can you repush (its not letting me rerun tests for some reason)

@wsmoses
Copy link
Collaborator

wsmoses commented Oct 11, 2023

@devmotion hm I still can't retrigger this, if you can have the tests run again.

@yebai
Copy link
Member

yebai commented Oct 22, 2023

@wsmoses you should have the priveledges to trigger CI (re-)runs -- can you try again?

@devmotion devmotion closed this Oct 22, 2023
@devmotion devmotion reopened this Oct 22, 2023
@devmotion devmotion closed this Oct 22, 2023
@devmotion devmotion reopened this Oct 22, 2023
@wsmoses wsmoses closed this Dec 6, 2023
@wsmoses wsmoses reopened this Dec 6, 2023
@yebai yebai closed this Mar 4, 2024
@yebai yebai reopened this Mar 4, 2024
@yebai yebai closed this May 25, 2024
@yebai yebai reopened this May 25, 2024
test/Project.toml Outdated Show resolved Hide resolved
@mhauru
Copy link

mhauru commented Jun 26, 2024

I reran CI against the latest Enzyme.jl v0.12.19. They are now failing with either (mac os)

Assertion failed: (vec.back() == currentBlock), function addReverseBlock, file /workspace/srcdir/Enzyme/enzyme/Enzyme/GradientUtils.cpp, line 8952.

or (ubuntu)

 julia: /workspace/srcdir/Enzyme/enzyme/Enzyme/GradientUtils.cpp:8952: llvm::BasicBlock* GradientUtils::addReverseBlock(llvm::BasicBlock*, const llvm::Twine&, bool, bool): Assertion `vec.back() == currentBlock' failed.

See the failed CI runs for details.

@wsmoses, is this an issue you are aware of?

@wsmoses
Copy link
Collaborator

wsmoses commented Jun 26, 2024

Nope not aware of please open MWE

@mhauru
Copy link

mhauru commented Jul 2, 2024

@wsmoses
Copy link
Collaborator

wsmoses commented Jul 9, 2024

@yebai Looks like the issues here are that Enzyme actually passes tests it was marked as failing?

@yebai
Copy link
Member

yebai commented Jul 9, 2024

yes, most tests are passing with most recent Enzyme.

@yebai
Copy link
Member

yebai commented Jul 9, 2024

@devmotion do you know why we are testing Enzyme only on Others tests?

test/ad/others.jl Outdated Show resolved Hide resolved
test/ad/others.jl Outdated Show resolved Hide resolved
test/ad/others.jl Outdated Show resolved Hide resolved
test/ad/others.jl Outdated Show resolved Hide resolved
test/ad/others.jl Outdated Show resolved Hide resolved
test/ad/utils.jl Outdated
Comment on lines 11 to 14
# Disable Enzyme warnings
Enzyme.API.typeWarning!(false)
# Enable runtime activity (workaround)
Enzyme.API.runtimeActivity!(true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Disable Enzyme warnings
Enzyme.API.typeWarning!(false)
# Enable runtime activity (workaround)
Enzyme.API.runtimeActivity!(true)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typewarning shouldnt need to be set [it is presently the default]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this commit is for removing them

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you may still need the runtimeactivity one for now [and looks like tests concur]

@yebai yebai marked this pull request as ready for review July 9, 2024 21:13
@yebai
Copy link
Member

yebai commented Jul 9, 2024

I think Enzyme might still struggle with a few MvNomal tests, which was revealed after 8b9fa77

@wsmoses
Copy link
Collaborator

wsmoses commented Jul 10, 2024

open issues and we'll get those sorted!

@wsmoses
Copy link
Collaborator

wsmoses commented Jul 10, 2024

Oh yeah runtime activity isn't supported yet for forward mode blas, so my recommendation here is to turn on runtime activity more selectively for which tests need it

@mhauru
Copy link

mhauru commented Jul 10, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants