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

Fix inactive rule to only mark constructor of one type as inactive, n… #412

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

wsmoses
Copy link
Collaborator

@wsmoses wsmoses commented Jul 23, 2023

…ot the constructor for all types

The custom rule below would mark all constructors as being inactive, not just the constructor of the individual function.

This bug was found by the following code

using Test
using Tracker
using Enzyme

Enzyme.API.printall!(true)
Enzyme.API.printactivity!(true)

struct MyBroadcasted{Style, F, Args<:Tuple}
    f::F
    args::Args
end

@noinline MyBroadcasted(f::F, args::Args) where {F, Args<:Tuple} =
    MyBroadcasted{Int64, F, Args}(f, args)

function cons_oop(v)
    bc2 = MyBroadcasted(identity, (v,))
    return bc2.args[1]
end

jac = autodiff(Forward, cons_oop, DuplicatedNoNeed, Duplicated([5.0], [1.0]))
@show jac

Eliminating the using tracker would eliminate the bug (presumably because tracker itself imported KA).

This was eventually found by looking at the method tables, which clearly should not have this:

julia> mt = ccall(:jl_method_table_for, Any, (Any,), sig)
# 30 methods for generic function "inactive":
[1] inactive(::typeof(thisind), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:84
[2] inactive(::typeof(Base.CoreLogging.current_logger_for_env), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:12
[3] inactive(::typeof(randn), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:75
[4] inactive(::typeof(Base.print_to_string), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:45
[5] inactive(::typeof(Base.CoreLogging.shouldlog), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:6
[6] inactive(::typeof(Base.Threads.nthreads), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:51
[7] inactive(::typeof(Base.Threads.threadid), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:48
[8] inactive(::typeof(flush), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:36
[9] inactive(::typeof(prevfloat), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:60
[10] inactive(::typeof(println), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:27
[11] inactive(::typeof(Core.kwfunc), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:66
[12] inactive(::typeof(eps), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:54
[13] inactive(::typeof(Random.rand!), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:72
[14] inactive(::typeof(Base.CoreLogging.current_logger), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:9
[15] inactive(::typeof(string), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:39
[16] inactive(::typeof(repr), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:42
[17] inactive(::typeof(Base.fixup_stdlib_path), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:15
[18] inactive(::typeof(Random.default_rng), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:78
[19] inactive(::typeof(Base.CoreLogging.logging_error), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:21
[20] inactive(::typeof(Random.seed!), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:81
[21] inactive(::typeof(Base.CoreLogging.handle_message), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:18
[22] inactive(::typeof(Base.to_tuple_type), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:24
[23] inactive(::typeof(print), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:30
[24] inactive(::typeof(show), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:33
[25] inactive(::typeof(rand), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:69
[26] inactive(::typeof(Base.CoreLogging.logmsg_code), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:3
[27] inactive(::typeof(nextind), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:87
[28] inactive(::typeof(nextfloat), args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:57
[29] inactive(::Type{Val}, args...) in Enzyme at /Users/wmoses/git/Enzyme.jl/src/internal_rules.jl:63
[30] inactive(::UnionAll, x...) in KernelAbstractions.EnzymeExt at /Users/wmoses/.julia/packages/KernelAbstractions/QFxLx/ext/EnzymeExt.jl:11

@vchuravy I'm not sure how we would successfully have a test for this, since this is a broken rule in a package and a test on Enzyme itself would not run with rule (and thus not fail). Does this mean that rule-makers should also run Enzyme tests on their extension (and if so we can add the above)?

This error is responsible for (and minimized from): SciML/Optimization.jl#565 cc @Vaibhavdixit02

I would also hypothesize it is responsible for SciML/SciMLSensitivity.jl#847 cc @ChrisRackauckas

@wsmoses
Copy link
Collaborator Author

wsmoses commented Jul 23, 2023

separately if it's possible to somehow make it illegal for someone to register a rule on UnionAll, that would be amazing.

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.

2 participants