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

bump Enzyme version in v0.2 #132

Merged
merged 3 commits into from
Oct 24, 2024
Merged

bump Enzyme version in v0.2 #132

merged 3 commits into from
Oct 24, 2024

Conversation

Red-Portal
Copy link
Member

This PR bumps Enzyme's compat bound to v0.13 so that AdvancedVI v0.2 doesn't hold the Turing ecosystem behind.

@yebai
Copy link
Member

yebai commented Oct 21, 2024

@Red-Portal Enzyme is still fragile, as observed in other parts of the TuringLang ecosystem. I don't think it is worth supporting it officially from AdvancedVI.

Maybe move all Enzyme tests into a separate CI task and allow them to fail during PR checks.

@Red-Portal
Copy link
Member Author

@yebai Yeah I was planning to do that in #129

@Red-Portal
Copy link
Member Author

Red-Portal commented Oct 22, 2024

@mhauru @yebai The Turing integration test is failing, but due issues downstream. Should I just merge anyways?

@mhauru
Copy link
Member

mhauru commented Oct 22, 2024

Thanks @Red-Portal, this is very helpful!

The tests on Turing's master are indeed broken because of 1.11. @penelopeysm, do you think either TuringLang/Turing.jl#2341 or TuringLang/Turing.jl#2368 is close enough to fixing the test breakage that it makes sense to hold off with this one for a moment?

@penelopeysm
Copy link
Member

@mhauru I suspect #2341 might be merged by today or tomorrow!

@wsmoses
Copy link
Contributor

wsmoses commented Oct 22, 2024

@Red-Portal Enzyme is still fragile, as observed in other parts of the TuringLang ecosystem. I don't think it is worth supporting it officially from AdvancedVI.

Maybe move all Enzyme tests into a separate CI task and allow them to fail during PR checks.

@yebai Do you have a reference to anything which is fragile in other parts of turing? From my understanding TuringLang/Turing.jl#1887 has been passing all tests for the past four months without issue (though certainly there's more to do like the additional ones distributions tests @mhauru opened)

@Red-Portal
Copy link
Member Author

Red-Portal commented Oct 22, 2024

@yebai Do you have a reference to anything which is fragile in other parts of turing? From my understanding TuringLang/Turing.jl#1887 has been passing all tests for the past four months without issue (though certainly there's more to do like the additional ones distributions tests @mhauru opened)

My impression is that Distributions.jl is full of demons and dragons such that someone will need to write a comprehensive set of tests to really see how much works and how much doesn't.

@wsmoses
Copy link
Contributor

wsmoses commented Oct 23, 2024

So @mhauru has been working on this recently here: EnzymeAD/Enzyme.jl#1819

as far as I can tell, its got a few more failures than the other tools, but they're all determinstic in throwing julia errors for currently unsupported distributions (and tend to be moreso forward mode, with reverse mode generally being more fine)

@Red-Portal
Copy link
Member Author

Oh I wasn't aware of that. I'll keep an eye!

@yebai yebai merged commit e5e26d0 into v0.2-backport Oct 24, 2024
7 of 8 checks passed
@yebai yebai deleted the v0.2-backport-enzyme-bump branch October 24, 2024 10:28
Copy link
Member

@mhauru mhauru left a comment

Choose a reason for hiding this comment

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

The code looks good to me, and the Turing integration tests now pass (thanks @penelopeysm!). The buildkite thing is failing (is this GPU tests?) but it seems to have been failing for the previous v2 release too, so maybe that's fine? Happy to merge @Red-Portal?

@yebai
Copy link
Member

yebai commented Oct 24, 2024

Merged!

@mhauru
Copy link
Member

mhauru commented Oct 24, 2024

I see I arrived late by seconds :)

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.

5 participants