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

Generated functions for types resolve returned bindings in the wrong module #57417

Closed
Keno opened this issue Feb 14, 2025 · 1 comment · Fixed by #57419
Closed

Generated functions for types resolve returned bindings in the wrong module #57417

Keno opened this issue Feb 14, 2025 · 1 comment · Fixed by #57419

Comments

@Keno
Copy link
Member

Keno commented Feb 14, 2025

The Zygote error is basically (need to comment out the precompile.jl file in Zygote to get it to load):

julia> using Zygote

# 1.12
julia> code_warntype(Zygote._pullback, Tuple{Zygote.Context{false}, typeof(>), Int64, Int64})
MethodInstance for ZygoteRules._pullback(::Zygote.Context{false}, ::typeof(>), ::Int64, ::Int64)
  from _pullback(ctx::ZygoteRules.AContext, f, args...) @ Zygote ~/PkgEvalAnalysis/JuliaPkgs/Zygote.jl/src/compiler/interface2.jl:101
Arguments
  methodinstance::Core.Const(ZygoteRules._pullback)
  ctx::Zygote.Context{false}
  f::Core.Const(>)
  args::Tuple{Int64, Int64}
Body::Any
1%1 = Base.chain_rrule::Any # <----------------------------- Base module
...

# 1.11
julia> code_warntype(Zygote._pullback, Tuple{Zygote.Context{false}, typeof(>), Int64, Int64})
MethodInstance for ZygoteRules._pullback(::Zygote.Context{false}, ::typeof(>), ::Int64, ::Int64)
...
Body::Tuple{Bool, Zygote.ZBack{Returns{Tuple{ChainRulesCore.NoTangent, ChainRulesCore.NoTangent, ChainRulesCore.NoTangent}}}}
1%1 = Zygote.chain_rrule::Core.Const(Zygote.chain_rrule) # <---------------------------- Zygote module
...

where there is a disagreement over what module the chain_rrule function refers to (Base vs Zygote). The chain_rrule symbol gets inserted in an expression at

https://github.com/FluxML/Zygote.jl/blob/12cd77d82546674e4951b900e6c8076d94e397b4/src/compiler/interface2.jl#L15-L25

which is then used as input into the Core.GeneratedFunctionStub at:

https://github.com/FluxML/Zygote.jl/blob/12cd77d82546674e4951b900e6c8076d94e397b4/src/compiler/interface2.jl#L77-L89

I don't really know what determines what module this should be "expanded" into (probably something with the source input (which has a different type on 1.11 vs 1.12).

Originally posted by @KristofferC in #57328

@Keno
Copy link
Member Author

Keno commented Feb 14, 2025

So this particular issue is somewhat specific to that use of Core.GeneratedFunctionStub - it doesn't affect the syntax form, but we do use it as an example in the Base tests, so we should just fix it in Base.

Keno added a commit that referenced this issue Feb 14, 2025
This addresses post-commit review #57230 (comment).
This change was left-over from before I decided to also change the
type of the `source` argument (at which point `source.module` was
unavailable in the function). This module was supposed to be the
same, but it turns out that both the julia tests and several packages
use this code manually and use different modules for the two places.
Use the same one we used before (which is probably more correct anyway)
to fix #57417
Keno added a commit that referenced this issue Feb 15, 2025
This addresses post-commit review
#57230 (comment).
This change was left-over from before I decided to also change the type
of the `source` argument (at which point `source.module` was unavailable
in the function). This module was supposed to be the same, but it turns
out that both the julia tests and several packages use this code
manually and use different modules for the two places. Use the same one
we used before (which is probably more correct anyway) to fix #57417
KristofferC pushed a commit that referenced this issue Feb 17, 2025
This addresses post-commit review
#57230 (comment).
This change was left-over from before I decided to also change the type
of the `source` argument (at which point `source.module` was unavailable
in the function). This module was supposed to be the same, but it turns
out that both the julia tests and several packages use this code
manually and use different modules for the two places. Use the same one
we used before (which is probably more correct anyway) to fix #57417

(cherry picked from commit 0c5372f)
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 a pull request may close this issue.

1 participant