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

MethodError if configured rrule is ambiguous #1358

Merged
merged 2 commits into from
Jan 17, 2023
Merged

MethodError if configured rrule is ambiguous #1358

merged 2 commits into from
Jan 17, 2023

Conversation

oxinabox
Copy link
Member

This resolves #1342
which is a special case of ambiguity where the ambiguity is for a configured rule and it's configured opt-out.
But it applies without an opt-out also.

This in particular implements Sol0
Which is to give an useful MethodError rather than a confusing error about no field, as follows:

Before

julia> pullback(h_ambig, 1, 2)
ERROR: type Nothing has no field method
Stacktrace:
 [1] getproperty(x::Nothing, f::Symbol)
   @ Base ./Base.jl:33
 [2] has_chain_rrule(T::Type)
   @ Zygote ~/.julia/packages/Zygote/AS0Go/src/compiler/chainrules.jl:21
 [3] #s2941#1099
   @ ~/.julia/packages/Zygote/AS0Go/src/compiler/interface2.jl:20 [inlined]
 [4] var"#s2941#1099"(::Any, ctx::Any, f::Any, args::Any)
   @ Zygote ./none:0
 [5] (::Core.GeneratedFunctionStub)(::Any, ::Vararg{Any, N} where N)
   @ Core ./boot.jl:571
 [6] pullback(::Function, ::Zygote.Context{false}, ::Int64, ::Vararg{Int64, N} where N)
   @ Zygote ~/.julia/packages/Zygote/AS0Go/src/compiler/interface.jl:44
 [7] pullback(::Function, ::Int64, ::Int64)
   @ Zygote ~/.julia/packages/Zygote/AS0Go/src/compiler/interface.jl:42
 [8] top-level scope
   @ REPL[24]:1

Now:

julia> pullback(h_ambig, 1, 2)
ERROR: MethodError: rrule(::ZygoteRuleConfig{Zygote.Context{false}}, ::typeof(h_ambig), ::Int64, ::Int64) is ambiguous. Candidates:
  rrule(::RuleConfig, ::typeof(h_ambig), x::Int64, y::Int64) in Main at /home/oxinabox/JuliaEnvs/Zygote.jl/test/chainrules.jl:291
  rrule(::ZygoteRuleConfig, ::typeof(h_ambig), x, y) in Main at /home/oxinabox/JuliaEnvs/Zygote.jl/test/chainrules.jl:290
Possible fix, define
  rrule(::ZygoteRuleConfig, ::typeof(h_ambig), ::Int64, ::Int64)
Stacktrace:
 [1] chain_rrule
   @ ~/JuliaEnvs/Zygote.jl/src/compiler/chainrules.jl:223 [inlined]
 [2] macro expansion
   @ ~/JuliaEnvs/Zygote.jl/src/compiler/interface2.jl:0 [inlined]
 [3] _pullback(::Zygote.Context{false}, ::typeof(h_ambig), ::Int64, ::Int64)
   @ Zygote ~/JuliaEnvs/Zygote.jl/src/compiler/interface2.jl:9
 [4] pullback(::Function, ::Zygote.Context{false}, ::Int64, ::Vararg{Int64})
   @ Zygote ~/JuliaEnvs/Zygote.jl/src/compiler/interface.jl:44
 [5] pullback(::Function, ::Int64, ::Int64)
   @ Zygote ~/JuliaEnvs/Zygote.jl/src/compiler/interface.jl:42
 [6] top-level scope
   @ ~/JuliaEnvs/Zygote.jl/test/chainrules.jl:293

This behavior is consistent with our behavour for non-configured rules as implemented in #1342

Sol1 (to just ignore rules) would be easily implemented for both configured and unconfigured cases by changing the final if to is_ambig || match(...) rather than `!is_ambig && match(...)
but should be a separate PR.

PR Checklist

  • Tests are added

@oxinabox oxinabox added bug Something isn't working ChainRules adjoint -> rrule, and further integration labels Jan 13, 2023
@oxinabox oxinabox requested a review from ToucheSir January 13, 2023 18:17
Copy link
Member

@ToucheSir ToucheSir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, merge when ready :)

@oxinabox oxinabox merged commit e2f0b5f into master Jan 17, 2023
@oxinabox oxinabox deleted the ox/cr_ambib branch January 17, 2023 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ChainRules adjoint -> rrule, and further integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not able to @opt_out rules with RuleConfig
2 participants