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 extension #3327

Merged
merged 16 commits into from
Oct 16, 2023
Merged

Add Enzyme extension #3327

merged 16 commits into from
Oct 16, 2023

Conversation

wsmoses
Copy link
Collaborator

@wsmoses wsmoses commented Oct 10, 2023

This PR adds an extension package to Oceananigans to describing the custom derivative behavior of Oceananigans types and functions.

@wsmoses
Copy link
Collaborator Author

wsmoses commented Oct 10, 2023

On Julia < 1.9 you need requires [which is standard practice by extension packages], if Oceananigans enforces 1.9+, it's not necessary.

Wasn't sure so added both here, and you can remove the requires dependency if desired.

@navidcy
Copy link
Collaborator

navidcy commented Oct 10, 2023

@navidcy
Copy link
Collaborator

navidcy commented Oct 10, 2023

Please add a small description of this PR in the top comment.

Question: is Oceananigans the right place for the extension to be or an extension for Oceananigans in Enzyme?

(I'm not arguing for one or the other; I'm just wondering.)

@wsmoses
Copy link
Collaborator Author

wsmoses commented Oct 10, 2023

Please add a small description of this PR in the top comment.

Done

Question: is Oceananigans the right place for the extension to be or an extension for Oceananigans in Enzyme?

(I'm not arguing for one or the other; I'm just wondering.)

So I think here is the right place, since code has a dependency on Oceananigans itself and EnzymeCore (which is dependency free).

It also is how other packages (like NNlib, LinearSolve, SciMLSensitivity, etc) work with Enzyme and other AD tools.

@navidcy
Copy link
Collaborator

navidcy commented Oct 10, 2023

OK. How are these methods going to be tested and or maintained?

@wsmoses
Copy link
Collaborator Author

wsmoses commented Oct 11, 2023

So I added a unit test showing that checks that should test its functionality.

Once we get more of Oceananigans differentiated we can add some bigger integration tests.

@wsmoses wsmoses force-pushed the enzymeext branch 2 times, most recently from c4ea879 to 8ba01ee Compare October 12, 2023 06:59
@glwagner
Copy link
Member

@wsmoses should we add a new buildkite run dedicated solely to the enzyme extension? It might help for debugging / interpreting failures. (If that makes sense, I can shuffle things around to make that work.)

@wsmoses
Copy link
Collaborator Author

wsmoses commented Oct 12, 2023

What's happening presently is KA is forcing an older version of Enzyme to be used in the tests, that doesn't have the inactive_type used by the testing infra.

This PR JuliaGPU/KernelAbstractions.jl#426 added relevant parts to KA, which currently is waiting for the julia registry to release a new patch release: JuliaRegistries/General#93272

Once that lands, this should succeed.

@glwagner
Copy link
Member

What's happening presently is KA is forcing an older version of Enzyme to be used in the tests, that doesn't have the inactive_type used by the testing infra.

This PR JuliaGPU/KernelAbstractions.jl#426 added relevant parts to KA, which currently is waiting for the julia registry to release a new patch release: JuliaRegistries/General#93272

Once that lands, this should succeed.

Ok great!

Also let me know if its ok with you if I set up a new buildkite step for the enzyme extension.

@wsmoses
Copy link
Collaborator Author

wsmoses commented Oct 12, 2023

@glwagner the prerequisites have landed with releases cut. Hopefully this is happy (and finds the correct versions of KA/Enzyme), but it was just tagged a few hours ago, so we'll see

@glwagner
Copy link
Member

Might need to update the Manifest

@glwagner glwagner merged commit 7af4aad into CliMA:main Oct 16, 2023
45 checks passed
@wsmoses wsmoses deleted the enzymeext branch October 16, 2023 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants