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

Zygote in Julia 1.10+ not reading rrules for default constructors #1502

Open
mrazomej opened this issue Feb 20, 2024 · 8 comments
Open

Zygote in Julia 1.10+ not reading rrules for default constructors #1502

mrazomej opened this issue Feb 20, 2024 · 8 comments

Comments

@mrazomej
Copy link

Zygote fails to use rrules defined by other packages when run with Julia 1.10+

I noticed this when looking at compatibility with TaylorDiff.jl.

In Julia 1.9.4:

import Zygote
import TaylorDiff

TaylorDiff.derivative(x -> sum(x .^ 2), [1.0, 2.0, 3.0], [0.0, 1.0, 0.0], :1) # works

Zygote.withgradient([1.0, 2.0, 3.0]) do x
    TaylorDiff.derivative(x -> sum(x .^ 2), x, [0.0, 1.0, 0.0], :1)
end # works, returning (val = 4.0, grad = ([0.0, 2.0, 0.0],))

In Julia 1.10+:

import Zygote
import TaylorDiff

TaylorDiff.derivative(x -> sum(x .^ 2), [1.0, 2.0, 3.0], [0.0, 1.0, 0.0], :1) # works

Zygote.withgradient([1.0, 2.0, 3.0]) do x
    TaylorDiff.derivative(x -> sum(x .^ 2), x, [0.0, 1.0, 0.0], :1)
end # doesn't work

The last line gives the following error:

ERROR: Need an adjoint for constructor TaylorDiff.TaylorScalar{Float64, 2}. Gradient is of type TaylorDiff.TaylorScalar{Float64, 2}
Stacktrace:
  [1] error(s::String)
    @ Base ./error.jl:35
  [2] (::Zygote.Jnew{TaylorDiff.TaylorScalar{Float64, 2}, Nothing, false})(Δ::TaylorDiff.TaylorScalar{Float64, 2})
    @ Zygote ~/.julia/packages/Zygote/jxHJc/src/lib/lib.jl:330
  [3] (::Zygote.var"#2210#back#313"{Zygote.Jnew{}})(Δ::TaylorDiff.TaylorScalar{Float64, 2})
    @ Zygote ~/.julia/packages/ZygoteRules/M4xmc/src/adjoint.jl:72
  [4] TaylorScalar
    @ ~/.julia/packages/TaylorDiff/zNnz2/src/scalar.jl:17 [inlined]
  [5] (::Zygote.Pullback{Tuple{…}, Tuple{…}})(Δ::TaylorDiff.TaylorScalar{Float64, 2})
    @ Zygote ~/.julia/packages/Zygote/jxHJc/src/compiler/interface2.jl:0
  [6] TaylorScalar
    @ ~/.julia/packages/TaylorDiff/zNnz2/src/scalar.jl:22 [inlined]
  [7] macro expansion
    @ ~/.julia/packages/TaylorDiff/zNnz2/src/primitive.jl:143 [inlined]
  [8] ^
    @ ~/.julia/packages/TaylorDiff/zNnz2/src/primitive.jl:128 [inlined]
  [9] (::Zygote.Pullback{Tuple{…}, Tuple{…}})(Δ::TaylorDiff.TaylorScalar{Float64, 2})
    @ Zygote ~/.julia/packages/Zygote/jxHJc/src/compiler/interface2.jl:0
 [10] literal_pow
    @ ./intfuncs.jl:351 [inlined]
 [11] (::Zygote.var"#1368#1374")(::Tuple{…}, ȳ₁::TaylorDiff.TaylorScalar{…})
    @ Zygote ~/.julia/packages/Zygote/jxHJc/src/lib/broadcast.jl:218
 [12] #4
    @ ./generator.jl:36 [inlined]
 [13] iterate(g::Base.Generator, s::Vararg{Any})
    @ Base ./generator.jl:47 [inlined]
 [14] collect(itr::Base.Generator{Base.Iterators.Zip{Tuple{…}}, Base.var"#4#5"{Zygote.var"#1368#1374"}})
    @ Base ./array.jl:834
 [15] map
    @ ./abstractarray.jl:3406 [inlined]
 [16] (::Zygote.var"#∇broadcasted#1373"{})(ȳ::FillArrays.Fill{…})
    @ Zygote ~/.julia/packages/Zygote/jxHJc/src/lib/broadcast.jl:218
 [17] #4117#back
    @ ~/.julia/packages/ZygoteRules/M4xmc/src/adjoint.jl:72 [inlined]
 [18] #291
    @ ~/.julia/packages/Zygote/jxHJc/src/lib/lib.jl:206 [inlined]
 [19] #2169#back
    @ ~/.julia/packages/ZygoteRules/M4xmc/src/adjoint.jl:72 [inlined]
 [20] broadcasted
    @ ./broadcast.jl:1347 [inlined]
 [21] #8
    @ ./REPL[4]:2 [inlined]
 [22] (::Zygote.Pullback{Tuple{…}, Tuple{…}})(Δ::TaylorDiff.TaylorScalar{Float64, 2})
    @ Zygote ~/.julia/packages/Zygote/jxHJc/src/compiler/interface2.jl:0
 [23] derivative
    @ ~/.julia/packages/TaylorDiff/zNnz2/src/derivative.jl:37 [inlined]
 [24] derivative
    @ ~/.julia/packages/TaylorDiff/zNnz2/src/derivative.jl:23 [inlined]
 [25] #7
    @ ./REPL[4]:2 [inlined]
 [26] (::Zygote.Pullback{Tuple{…}, Tuple{…}})(Δ::Float64)
    @ Zygote ~/.julia/packages/Zygote/jxHJc/src/compiler/interface2.jl:0
 [27] (::Zygote.var"#75#76"{Zygote.Pullback{Tuple{}, Tuple{}}})(Δ::Float64)
    @ Zygote ~/.julia/packages/Zygote/jxHJc/src/compiler/interface.jl:91
 [28] withgradient(f::Function, args::Vector{Float64})
    @ Zygote ~/.julia/packages/Zygote/jxHJc/src/compiler/interface.jl:0
 [29] top-level scope
    @ REPL[4]:1

But those @adjoints are definitely defined in TaylorDiff.jl

@ToucheSir
Copy link
Member

Are you using TaylorDiff v0.2.1 (the latest released version) but looking at the code of the TaylorDiff.jl main branch? Note that there seem to have been a lot of changes between the two.

As for the behaviour being different between versions, I would not suspect rules not being picked up first. It's possible that either TaylorDiff.jl or Julia Base changed some implementation details between 1.9 and 1.10, so that code used to take a path that was differentiable but now is not. Again though, we need to know what versions of Zygote and TaylorDiff you're using, as well as if they're the same versions across 1.9 and 1.10.

@mrazomej
Copy link
Author

Thank you so much for your quick response. I was going to add the versions of the packages, and I completely forgot.

For both, 1.9.4 and 1.10+ I am using the latest versions of the packages.

[e88e6eb3] Zygote v0.6.69
[b36ab563] TaylorDiff v0.2.1

@ToucheSir
Copy link
Member

Can you try adding TaylorDiff#main and seeing if that changes things?

@mrazomej
Copy link
Author

Just did it.

pkg> add TaylorDiff#main
Cloning git-repo `https://github.com/JuliaDiff/TaylorDiff.jl.git`
Updating git-repo `https://github.com/JuliaDiff/TaylorDiff.jl.git`

same error

@ToucheSir
Copy link
Member

It's possible that either TaylorDiff.jl or Julia Base changed some implementation details between 1.9 and 1.10, so that code used to take a path that was differentiable but now is not.

Can confirm it goes even deeper than this to the compiler. To demonstrate:

1.9:

julia> @code_warntype TaylorScalar((1,))
MethodInstance for TaylorScalar(::Tuple{Int64})
  from TaylorScalar(value::Tuple{Vararg{T, N}}) where {T, N} @ TaylorDiff ~/.julia/packages/TaylorDiff/SMDgC/src/scalar.jl:17
Static Parameters
  T = Int64
  N = 1
Arguments
  #self#::Type{TaylorScalar}
  value::Tuple{Int64}
Body::TaylorScalar{Int64, 1}
1 ─ %1 = Core.apply_type(TaylorDiff.TaylorScalar, $(Expr(:static_parameter, 1)), $(Expr(:static_parameter, 2)))::Core.Const(TaylorScalar{Int64, 1})
│   %2 = (%1)(value)::TaylorScalar{Int64, 1}
└──      return %2

1.10:

julia> @code_warntype TaylorScalar((1,))
MethodInstance for TaylorScalar(::Tuple{Int64})
  from TaylorScalar(value::Tuple{Vararg{T, N}}) where {T, N} @ TaylorDiff ~/.julia/packages/TaylorDiff/SMDgC/src/scalar.jl:17
Static Parameters
  T = Int64
  N = 1
Arguments
  #self#::Type{TaylorScalar}
  value::Tuple{Int64}
Body::TaylorScalar{Int64, 1}
1 ─ %1 = Core.apply_type(TaylorDiff.TaylorScalar, $(Expr(:static_parameter, 1)), $(Expr(:static_parameter, 2)))::Core.Const(TaylorScalar{Int64, 1})
│   %2 = %new(%1, value)::TaylorScalar{Int64, 1}
└──      return %2

Essentially, the compiler can skip the intermediate step of calling the Type constructor when constructing new values. This means that rules like https://github.com/JuliaDiff/TaylorDiff.jl/blob/4f7b477b188ef15f6a4368285bb2c40319089a48/src/chainrules.jl#L9-L12 may not be picked up because Zygote has a fallback path for Expr(:new) it uses unconditionally. Evidently something needs to change to workaround this change in internals, but it's not clear who should make that change yet.

@oxinabox oxinabox changed the title Zygote in Julia 1.10+ not reading rrules defined by other packages Zygote in Julia 1.10+ not reading rrules for default constructors Feb 23, 2024
@mBarreau
Copy link

mBarreau commented Mar 6, 2024

@ToucheSir any news on this issue?

@ToucheSir
Copy link
Member

Unfortunately, no. I've not heard anything from the Base Julia side about possible solutions and I wouldn't know how to tackle this on the Zygote side. If somebody has news on the former or a concrete proposal for the latter, please chime in.

@tansongchen
Copy link

This should be fine now as of JuliaDiff/TaylorDiff.jl@c11dd72

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

No branches or pull requests

4 participants