Add warnings #2415
Add warnings #2415
Conversation
|
cc @gdalle re DI comment since I can't seem to add you as a reviewer |
|
Your PR requires formatting changes to meet the project's style guidelines. Click here to view the suggested changes.diff --git a/ext/EnzymeDifferentiationInterfaceExt.jl b/ext/EnzymeDifferentiationInterfaceExt.jl
index c79aa90..d6286a7 100644
--- a/ext/EnzymeDifferentiationInterfaceExt.jl
+++ b/ext/EnzymeDifferentiationInterfaceExt.jl
@@ -1,10 +1,10 @@
module EnzymeDifferentiationInterfaceExt
function __init__()
- @warn """You are using DifferentiationInterface!"
- DifferentiationInterface introduces interstitial wrappers that may limit the scope of input programs and add overhead."
- This can cause derivatives to be slower, or fail to differentiate with default settings when they work with Enzyme directly (e.g. Enzyme.gradient instead of DI.gradient)."
- If you find issues, please report at https://github.com/EnzymeAD/Enzyme.jl/issues/new and try Enzyme directly in the interim."""
+ return @warn """You are using DifferentiationInterface!"
+ DifferentiationInterface introduces interstitial wrappers that may limit the scope of input programs and add overhead."
+ This can cause derivatives to be slower, or fail to differentiate with default settings when they work with Enzyme directly (e.g. Enzyme.gradient instead of DI.gradient)."
+ If you find issues, please report at https://github.com/EnzymeAD/Enzyme.jl/issues/new and try Enzyme directly in the interim."""
end
end # module
diff --git a/src/init.jl b/src/init.jl
index 07357bc..4303006 100644
--- a/src/init.jl
+++ b/src/init.jl
@@ -1,9 +1,9 @@
function __init__()
- if VERSION >= v"1.11.0"
+ return if VERSION >= v"1.11.0"
@warn """You are using Julia v1.11 or later!"
- Julia 1.11 changes the default Array type to contain a triply-nested pointer, rather than a doubly nested pointer."
- This may cause programs (primal but especially derivatives) to be slower, or fail to differentiate with default settings when they previously worked on 1.10."
- If you find issues, please report at https://github.com/EnzymeAD/Enzyme.jl/issues/new and try Julia 1.10 in the interim."""
+ Julia 1.11 changes the default Array type to contain a triply-nested pointer, rather than a doubly nested pointer."
+ This may cause programs (primal but especially derivatives) to be slower, or fail to differentiate with default settings when they previously worked on 1.10."
+ If you find issues, please report at https://github.com/EnzymeAD/Enzyme.jl/issues/new and try Julia 1.10 in the interim."""
end
-end
\ No newline at end of file
+end |
There was a problem hiding this comment.
While I understand the idea, I find the present PR to be a rather agressive approach. This message will display for everyone who has Enzyme + DI loaded. Given the prevalence of DI in the ecosystem (7% of registered packages import it), this means that basically everyone who uses Enzyme in a slightly complicated setting might see it. My best guess is that it will essentially piss people off about both packages.
Once again, I'm available to work on any proven, reproducible issues or slowdowns that arise from the DI + Enzyme interaction. But this kind of vague warning may not only deter people from using DI, it may deter people from Enzyme as well. One of my main goals with DI is to help Enzyme succeed, and it still is.
|
|
||
| function __init__() | ||
| @warn """You are using DifferentiationInterface!" | ||
| DifferentiationInterface introduces interstitial wrappers that may limit the scope of input programs and add overhead." |
There was a problem hiding this comment.
I would remove "interstitial", I think most non native English speakers don't know the word. Also it isn't clear what you mean by "limit the scope"
There was a problem hiding this comment.
Yeah I just meant that programs need to be rewritten to be DI compatible (eg can’t have multiple active args, dupnoneed as arg etc). So then general programs that are supported by enzyme directly aren’t necessarily supported by DI (at least without a rewrite)
| @warn """You are using DifferentiationInterface!" | ||
| DifferentiationInterface introduces interstitial wrappers that may limit the scope of input programs and add overhead." | ||
| This can cause derivatives to be slower, or fail to differentiate with default settings when they work with Enzyme directly (e.g. Enzyme.gradient instead of DI.gradient)." | ||
| If you find issues, please report at https://github.com/EnzymeAD/Enzyme.jl/issues/new and try Enzyme directly in the interim.""" |
There was a problem hiding this comment.
In the spirit of lightening your load, wouldn't you want people to submit these issues to DI instead?
As for the recommendation to switch, most people calling Enzyme through DI will actually be doing it through a third party package like ODE or Turing. In that sense, I don't think the recommendation to switch to native Enzyme is very actionable, or useful.
Benchmark Results
Benchmark PlotsA plot of the benchmark results has been uploaded as an artifact at https://github.com/EnzymeAD/Enzyme.jl/actions/runs/15257837697/artifacts/3198498421. |
|
Warnings during init are incredibly obnoxious. I agree with @gdalle that the most likely consequences is people just not using Enzyme. The 1.11 warning we could add to the readme |
|
Can we somehow register it to run during Julia’s custom segfault / etc error handler? I agree and would’ve preferred it not run when necessary, but didn’t know a way to handle the segfault case otherwise |
|
Alternatively is there a better place to put it (this is just a copy of where Oceananigans has their 1.11 warning) |
|
My other worry is that if folks already ignore warnings in the errors we throw, they’ll definitely ignore them in a readme they won’t open |
|
Perhaps as an error hint? There is some experimental facility for that, but I haven't worked with that yet.
Those people are also the ones ignoring a global warning. |
Like the low level systems thing I would do is set up a signal handler, which iirc julia does for itself (though not gc I suppose)?
Yeah that's fair, but I really doubt people look at the readme. |
|
Another question is how do we warn people about slow perf. We won't throw an error, so we can't tack things onto that |
|
Regardless of how you want to warn people, I'd much rather want to see where this slow perf is so that DI resolves it whenever possible. Do you have examples? |
|
Also sometimes people are okay with things being slow if they are more convenient. So I am much less concerned about that. |
|
I get that, though also people may only try things once and decide that it doesn't work for them (when in reality they were using a bad configuration) ... and given the fact that I've seen so many people hit the same runtime_activity error that comes from the triple pointer + broadcasting and/or closures that equally impacts differentiability and perf (and often mistake it as a "Segfault"), I want to try to give users a way to get the best configuration from the start |
But then again there are plenty of people who are empowered to try Enzyme thanks to DI. Forcing a warning at every load of Enzyme might just convince them, along with everyone else, that Enzyme is not robust enough for their use case. Given how widely DI has been adopted, I don't think it is fair to call it "the bad configuration" anymore. What we can do, however, is improve error messages in more delicate touches ⬇️
The latest version of DI includes a rather friendly and visible error message for that one. julia> using DifferentiationInterface
julia> using Enzyme: Enzyme
julia> function g(active_var, constant_var, cond)
if cond
return active_var
else
return constant_var
end
end;
julia> function h(active_var, constant_var, cond)
return [g(active_var, constant_var, cond), g(active_var, constant_var, cond)]
end;
julia> pushforward(
h, AutoEnzyme(; mode=Enzyme.Forward), [1.0], ([1.0],), Constant([1.0]), Constant(true)
)
ERROR:
If you are using Enzyme by selecting the `AutoEnzyme` object from ADTypes, you may want to try setting the `mode` option as follows:
AutoEnzyme(; mode=Enzyme.set_runtime_activity(Enzyme.Forward))
AutoEnzyme(; mode=Enzyme.set_runtime_activity(Enzyme.Reverse))
This hint appears because DifferentiationInterface and Enzyme are both loaded. It does not necessarily imply that Enzyme is being called through DifferentiationInterface.
Constant memory is stored (or returned) to a differentiable variable.
As a result, Enzyme cannot provably ensure correctness and throws this error.
This might be due to the use of a constant variable as temporary storage for active memory (https://enzyme.mit.edu/julia/stable/faq/#Runtime-Activity).
If Enzyme should be able to prove this use non-differentable, open an issue!
To work around this issue, either:
a) rewrite this variable to not be conditionally active (fastest, but requires a code change), or
b) set the Enzyme mode to turn on runtime activity (e.g. autodiff(set_runtime_activity(Reverse), ...) ). This will maintain correctness, but may slightly reduce performance.
Mismatched activity for: %value_phi = select i1 %.not, {} addrspace(10)* %1, {} addrspace(10)* %0, !dbg !15 const val: {} addrspace(10)* %1
value=Unknown object of type Vector{Float64}
llvalue={} addrspace(10)* %1 |
|
my earlier comment was in reference to the triple pointer by 1.11 not a DI specific issue. my original comment on giving people an actionable way to debug performance in advance through a warning also had no reference to DI, but is something we generally need to figure out for potential bad configurations (be it Julia 1.11 array sadness, DI, or something else). I was not calling DI a bad configuration, but referring to any bad configuration that could cause problems as something we want to create a warning for. Also the "This hint appears because DifferentiationInterface and Enzyme are both loaded. It does not necessarily imply that Enzyme is being called through DifferentiationInterface." comment is kind of weird to have (also it might merit having the enzyme error at the start). I think we should get the backtrace version of the error in which would solve part of this PR and other issues for non-segfault errors. The perf warning, and segfault error ofc still needs a solution |
That was my take on your request to "make the error message more conservative because it will print even when DI is not used".
That's defined in Enzyme, more specifically here: Lines 166 to 194 in bd60763 At the moment, |
For what is worth, I can't say I appreciated that warning when running Oceananigans on thousands of processes. It only made log files harder to read. |
per a convo with @gbaraldi last week, it feels like adding a warning in advance is easier than telling folks word of mouth to try different setups in issues.
My hope would've been to just emit the warning when an error is thrown, but if something crashes (which to be clear should never happen) I'm not sure we have other recourse. Also it seems like folks just ignore the messages in errors like "set runtime activity" =/