-
Notifications
You must be signed in to change notification settings - Fork 68
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
Mixed activity for broadcast against scalars #1482
Comments
It does support that, the issue here comes from the type instability at
|
Oh that error message is a bit misleading then 😅 . Is there any way to catch that at the higher level and attribute to type instability? The issue here is that the values are relying on union splitting: https://github.com/ODINN-SciML/Sleipnir.jl/blob/main/src/glaciers/glacier/Glacier2D.jl#L25-L26 Does Enzyme not handle union splitting the same way? |
How would you recommend improving the error? The linked docs give a fairly full explanation, including how to resolve: Mixed activity Active variables are used for immutable variables (like Float64), whereas Duplicated variables are used for mutable variables (like Vector{Float64}). Speciically, since Active variables are immutable, functions with Active inputs will return the adjoint of that variable. In contrast Duplicated variables will have their derivatives +='d in place. This error indicates that you have a type, like Tuple{Float, Vector{Float64}} that has immutable components and mutable components. Therefore neither Active nor Duplicated can be used for this type. Internally, by virtue of working at the LLVM level, most Julia types are represented as pointers, and this issue does not tend to arise within code fully differentiated by Enzyme internally. However, when a program needs to interact with Julia API's (e.g. as arguments to a custom rule, a type unstable call, or the outermost function being differentiated), Enzyme must adhere to Julia's notion of immutability and will throw this error rather than risk an incorrect result. For example, consider the following code, which has a type unstable call to myfirst, passing in a mixed type Tuple{Float64, Vector{Float64}}.
When this situation arises, it is often easiest to resolve it by adding a level of indirection to ensure the entire variable is mutable. For example, one could enclose this variable in a reference, such as Ref{Tuple{Float, Vector{Float64}}}, like as follows.
|
I think it's missing a section and the error message is missing a detail. From the error message:
it looks like everything is inferred. IIUC It seems the issue is that due to lack of inference we effectively have:
and it doesn't know what activity to place on the IIUC you can see from the Additionally, it seems like there may need to be another section in that documentation part: Connection to Type InferenceIn normal execution of Enzyme, this type T is the inferred type in the code. Thus the error can arise if the compile-time inferred type has a mixture of activities. For example, if the inferred type is Am I on the right track here? |
Maybe we can add a Mixed activity in type unstable call or something for clarity? However per your comment mixed activity and runtime activity are two completely separate things. Mixed activity is a reverse-mode specific issue when you have a data-structure which has both mutable and immutable differentiable data components. Thus updates cannot be represented by either Active (aka immutable) or Duplicated (aka mutable). The reason type inference come into play here (as well as custom rules), is the fact that this error tends to only occur when a variable needs to conform to Julia semantics. Speciically if we allocate 16 bytes to make an immutable struct, Enzyme doesn't care and it can update the derivative in place. However, if we pass it to a literal julia function (as is the case when going through the JIT, or a custom rule) for safety and correctness we throw an error rather than potentially creating behavior that a user would see as violating the julia immutable variable guarantee. This is why we needed the mutable on that one struct. In contrast runtime activity is a different (and incidentally solvable) issue. See the latter half of the previous FAQ https://enzyme.mit.edu/index.fcgi/julia/stable/faq/#Activity-of-temporary-storage starting from "However, even if we ignore the semantic guarantee provided by marking tmp as constant, another issue arises." from the FAQ: Recent versions of Enzyme will attempt to error when they detect these latter types of situations, which we will refer to as activity unstable. This term is chosen to mirror the Julia notion of type-unstable code (e.g. where a type is not known at compile time). If an expression is activity unstable, it could either be constant, or active, depending on data not known at compile time. For example, consider the following:
The returned value here could either by constant or duplicated, depending on the runtime-defined value of cond. If cond is true, Enzyme simply returns the shadow of active_var as the derivative. However, if cond is false, there is no derivative shadow for constant_var and Enzyme will throw a "Mismatched activity" error. Admittedly these definitely need better and more distinguisable names if you have any suggestions. The resolution to this latter issue is runtime activity https://enzyme.mit.edu/index.fcgi/julia/stable/api/#Enzyme.API.runtimeActivity!-Tuple{Bool} . This flag essentially uses a slightly runtime slowdown to be able to always resolve these cases (though has slightly different semantics as a result). As described in its docs: " However, in certain cases, an insufficiently aggressive activity analysis may result in derivative errors – for example by mistakenly using the primal (const) argument and mistaking it for the duplicated shadow. As a result this may result in incorrect results, or accidental updates to the primal. This flag enables runntime activity which tells all load/stores to check at runtime whether the value they are updating is indeed active (in addition to the compile-time activity analysis). This will remedy these such errors, but at a performance penalty of performing such checks. It is on the Enzyme roadmap to add a PotentiallyDuplicated style activity, in addition to the current Const and Duplicated styles that will disable the need for this, which does not require the check when a value is guaranteed active, but still supports runtime-based activity information. This function takes an argument to set the runtime activity value, true means it is on, and false means off. By default it is off. |
I see, so runtime activity is just const vs non-const, but not Active vs Duplicated like would happen in the case of Union{Float64, Vector{Float64}}? The naming is then a bit odd, runtime constant determination would make a bit more sense to me. |
yeah, though incidentally Union is actually fine, it would be Tuple{Float64, Vector{Float64}} which would present the issue [as it does here, well except struct not tuple] |
the union may be what causes julia to have a type instability, but its the |
function f1(u)
dSdx = u ./ Δx
sum(dSdx)
end
function f2(u)
dSdx .= u ./ Δx
sum(dSdx)
end
function f3(u)
dSdx = u ./ dx
sum(dSdx)
end
using Enzyme
Δx = 62.0
const dx = 62.0
u = zeros(129,138)
du = zero(u)
dSdx = zeros(129,138)
Enzyme.autodiff(Reverse, f1, Active, Duplicated(u, du)) # Error
Enzyme.autodiff(Reverse, f2, Active, Duplicated(u, du)) # Error
Enzyme.autodiff(Reverse, f3, Active, Duplicated(u, du)) # Works! highlights it, though I'm kind of surprised that the type instability is the key to fixing that? |
No that still makes sense from the reasons above? |
It's still surprising given the error doesn't mention type inference at all. |
FYI ~half of the required code for supporting this without any changes is here: #1526 |
If I read this error message correctly:
This statement means that Enzyme does not allow for broadcast of arrays (matrices) against scalar values. This would suggest a simple MWE of
A ./ a
whereA
is a matrix anda
is a scalar.The full stack trace is:
where it's referring to: https://github.com/ODINN-SciML/Huginn.jl/blob/v0.6.0/src/models/iceflow/SIA2D/SIA2D_utils.jl#L120 which is doing exactly what the suggested MWE does.
This was found as part of a larger investigation ODINN-SciML/ODINN.jl#151
The text was updated successfully, but these errors were encountered: