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

Choosing between Active and Duplicated #1948

Closed
gdalle opened this issue Oct 9, 2024 · 12 comments · Fixed by #1955
Closed

Choosing between Active and Duplicated #1948

gdalle opened this issue Oct 9, 2024 · 12 comments · Fixed by #1955

Comments

@gdalle
Copy link
Contributor

gdalle commented Oct 9, 2024

To compute gradients with Enzyme on a non-mutable array (like SArray), I need to annotate it as Active. Otherwise, if I do this

using StaticArrays, Enzyme
s = SVector(1.0, 2.0)
ds = zero(s)
Enzyme.autodiff(Reverse, sum, Active, Enzyme.Duplicated(s, ds))

there is no way to recover the derivative because ds doesn't change.

My default setting in DI is to mark arrays as Duplicated. Is there a user-facing function in Enzyme that I could call to pick a better-suited annotation?

Ping @ExpandingMan

@ExpandingMan
Copy link
Contributor

For whatever reason this seems to be a much bigger problem for reverse mode. Forward mode usually seems to "just work" and I don't always understand why. I think more extensive documentation about all the various mode arguments would be useful, as well as more examples.

@wsmoses
Copy link
Member

wsmoses commented Oct 9, 2024

yeah we should probably add better docs on, offhand probably see https://enzyme.mit.edu/getting_started/CallingConvention/#types and https://enzyme.mit.edu/index.fcgi/julia/stable/faq/#Mixed-activity

Forward mode doesn't have active so there's never a choice needed

@gdalle
Copy link
Contributor Author

gdalle commented Oct 9, 2024

The typical problem is for the generic pullback inside DI. In this line, I pick a return activity according to a very simple rule of thumb: Number gets Active, anything else is Duplicated.

https://github.com/gdalle/DifferentiationInterface.jl/blob/3470e1424d667ffe79112c3b8429c9c0ca0e81e5/DifferentiationInterface/ext/DifferentiationInterfaceEnzymeExt/reverse_onearg.jl#L74

But it is wrong for SArrays, and for mixed activity. I presume Enzyme has some automatic activity guesser inside?

@wsmoses
Copy link
Member

wsmoses commented Oct 9, 2024

Yup, an internal function Enzyme.Compiler.active_reg_inner. But note that it is definitionally type unstable (though it tries its best to be)

@gdalle
Copy link
Contributor Author

gdalle commented Oct 9, 2024

Would you be open to documenting it so that it can be used by DI? Type instability is less of an issue thanks to the preparation mechanism, which we only use at the beginning.

@wsmoses
Copy link
Member

wsmoses commented Oct 9, 2024

sure to docs, but it definitely should remain an internal function since it takes a recursive descent state that might need to be modified as we build out more activity analysis infra and I don't want to force a breaking change for that.

@gdalle
Copy link
Contributor Author

gdalle commented Oct 9, 2024

If it is used by DI it will have to be part of the public API. But just being part of the public API doesn't necessarily mean that its behavior has to stay exactly the same. I would be fine with a public function whose docstring says something like

    Enzyme.Compiler.active_reg_inner

Pick the most appropriate activity annotation for a given object and AD mode.
This function is subject to change as Enzyme's activity analysis progresses, but its goal will remain the same.

@wsmoses
Copy link
Member

wsmoses commented Oct 9, 2024

I mean if you're willing to keep up with any changes to the ABI in patch releases (the threshold which I would refer to something as an internal function), I'm happy to put a docstring on it.

I don't expect that to happen often (or frankly much at all), but it's definitely not something that should force an Enzyme breaking change since as we just saw that often requires a whole ecosystem sweep.

@gdalle
Copy link
Contributor Author

gdalle commented Oct 9, 2024

What would be the consequences of such ABI changes on the activity analysis? I still don't fully understand what the ABI does ^^ #1345

@danielwe
Copy link
Contributor

danielwe commented Oct 9, 2024

There's the higher-level Enzyme.guess_activity(::Type, ::Enzyme.Mode) that seems like could be made public without much trouble?

Enzyme.jl/src/compiler.jl

Lines 859 to 860 in 4d4c546

@inline Enzyme.guess_activity(::Type{T}, mode::Enzyme.Mode) where {T} =
guess_activity(T, convert(API.CDerivativeMode, mode))

@wsmoses
Copy link
Member

wsmoses commented Oct 9, 2024

Oh right I forgot that existed, yeah that one is fine to make public, since we can still change any of the args to active reg inner with ease

@gdalle
Copy link
Contributor Author

gdalle commented Oct 10, 2024

Opened #1955 with a teeny tiny docstring

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 a pull request may close this issue.

4 participants