FluxML / IRTools.jl

Mike's Little Intermediate Representation
MIT License
111 stars 35 forks source link

Fixes for julia v1.10 #109

Closed IanButterworth closed 1 year ago

IanButterworth commented 1 year ago

Proposed fix for #108

Includes the fix from #112 to try and get CI green

@vtjnash

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 36.36% and project coverage change: -0.34 :warning:

Comparison is base (62d5f42) 74.98% compared to head (68b9fa5) 74.64%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #109 +/- ## ========================================== - Coverage 74.98% 74.64% -0.34% ========================================== Files 15 15 Lines 1419 1428 +9 ========================================== + Hits 1064 1066 +2 - Misses 355 362 +7 ``` | [Impacted Files](https://codecov.io/gh/FluxML/IRTools.jl/pull/109?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flux) | Coverage Δ | | |---|---|---| | [src/reflection/utils.jl](https://codecov.io/gh/FluxML/IRTools.jl/pull/109?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flux#diff-c3JjL3JlZmxlY3Rpb24vdXRpbHMuamw=) | `90.43% <28.57%> (-4.01%)` | :arrow_down: | | [src/eval.jl](https://codecov.io/gh/FluxML/IRTools.jl/pull/109?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flux#diff-c3JjL2V2YWwuamw=) | `88.88% <50.00%> (-5.23%)` | :arrow_down: | | [src/reflection/reflection.jl](https://codecov.io/gh/FluxML/IRTools.jl/pull/109?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flux#diff-c3JjL3JlZmxlY3Rpb24vcmVmbGVjdGlvbi5qbA==) | `83.33% <50.00%> (-1.42%)` | :arrow_down: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

IanButterworth commented 1 year ago

I think I've done all the obvious stuff here. Seems like it needs someone with more IRTools knowledge

IanButterworth commented 1 year ago

@aviatesk @vtjnash any idea what's going on here? I'm not familiar with IRTools at all

vtjnash commented 1 year ago

Oh, it is using APIs that Base doesn't have anymore and need to be converted to the new API

maleadt commented 1 year ago

Here's with those APIs fixed: https://github.com/FluxML/IRTools.jl/compare/master...maleadt:IRTools.jl:replace_code_newstyle_fix @IanButterworth Can you merge that here?

Only a couple of test failures remain.

IanButterworth commented 1 year ago
Compiler: Test Failed at /home/runner/work/IRTools.jl/IRTools.jl/test/compiler.jl:93
  Expression: mullify(prod, [5, 10]) == 15
   Evaluated: 50 == 15

Is because https://github.com/IanButterworth/IRTools.jl/blob/668530fb38ca20678e0934efb3b74d93d8711925/test/compiler.jl#L80 is returning as nothing because https://github.com/IanButterworth/IRTools.jl/blob/6666ab00735fd52b73161c7fdf8755b8c4501a9c/src/ir/wrap.jl#L157-L158 is returning nothing.

Might this be related to Jameson's comment here https://github.com/FluxML/IRTools.jl/commit/f401a9596921a1f30c5f4a15e77cad4d2e8e1ae1#r111498985 @maleadt ?

Also, Zygote still fails to load with this in its current form, no method matching iterate(::Nothing) so appears to suffer a similar issue

IanButterworth commented 1 year ago

Appears to be a world age issue

julia> @dynamo function mullify(a...)
         @show ir = IR(a...)
         ir == nothing && return
         ir = MacroTools.prewalk(ir) do x
           x isa GlobalRef && x.name == :(*) && return GlobalRef(Base, :+)
           return x
         end
         for (x, st) in ir
           isexpr(st.expr, :call) || continue
           ir[x] = xcall(Main, :mullify, st.expr.args...)
         end
         return ir
       end

julia> mullify(prod, [5, 10])
Tuple{Ts...} = Tuple{typeof(prod), Vector{Int64}}
m = IRTools.meta(Tuple{Ts...}) = nothing
ir = IR(a...) = nothing

julia> IRTools.meta(Tuple{typeof(prod), Vector{Int64}})
Metadata for prod(a::AbstractArray; dims, kw...) @ Base reducedim.jl:994
maleadt commented 1 year ago

Might this be related to Jameson's comment here f401a95#r111498985 @maleadt ?

No, that's unrelated.

The problem is that @dynamo is fundamentally broken. Or at least the cases where the dynamo looks up Julia IR, which now needs to be done with world ages in mind (i.e. passing the age to reflection functions and setting edges on the returned CodeInfo), like the mullify one above. Sadly, this also seems like the way most packages use @dynamo.

maleadt commented 1 year ago

Updated my fork; it passes tests now.

IanButterworth commented 1 year ago

Great! Although Zygote still fails to precompile on https://github.com/FluxML/Zygote.jl/blob/2287a86d1692c0dfc568739a80ca909199f6adfe/src/precompile.jl#LL17C20-L17C20

But perhaps that shouldn't stop this being merged and released?

ERROR: LoadError: MethodError: no method matching iterate(::Nothing)

Closest candidates are:
  iterate(::Union{LinRange, StepRangeLen})
   @ Base range.jl:887
  iterate(::Union{LinRange, StepRangeLen}, ::Integer)
   @ Base range.jl:887
  iterate(::T) where T<:Union{Base.KeySet{<:Any, <:Dict}, Base.ValueIterator{<:Dict}}
   @ Base dict.jl:728
  ...

Stacktrace:
  [1] indexed_iterate(I::Nothing, i::Int64)
    @ Base ./tuple.jl:95
  [2] chain_rrule(::Zygote.ZygoteRuleConfig{Zygote.Context{false}}, ::typeof(Zygote.pow), ::Int64, ::Int64)
    @ Zygote ~/.julia/packages/Zygote/SuKWp/src/compiler/chainrules.jl:223 [inlined]
  [3] macro expansion
    @ Zygote ~/.julia/packages/Zygote/SuKWp/src/compiler/interface2.jl:0 [inlined]
  [4] _pullback(::Zygote.Context{false}, ::typeof(Zygote.pow), ::Int64, ::Int64)
    @ Zygote ~/.julia/packages/Zygote/SuKWp/src/compiler/interface2.jl:9
  [5] pullback(::Function, ::Zygote.Context{false}, ::Int64, ::Vararg{Int64})
    @ Zygote ~/.julia/packages/Zygote/SuKWp/src/compiler/interface.jl:44
  [6] pullback(::Function, ::Int64, ::Int64)
    @ Zygote ~/.julia/packages/Zygote/SuKWp/src/compiler/interface.jl:42
  [7] gradient(::Function, ::Int64, ::Vararg{Int64})
    @ Zygote ~/.julia/packages/Zygote/SuKWp/src/compiler/interface.jl:96
  [8] top-level scope
    @ ~/.julia/packages/Zygote/SuKWp/src/precompile.jl:17
maleadt commented 1 year ago

I pushed another fix to my branch.

Some initial fixes for Zygote.jl are in https://github.com/maleadt/Zygote.jl/tree/dev, depending on this PR. This makes Zygote precompile and makes a smoke test gradient succeed, but there's more instances where the generated world age isn't propagated to reflection methods (resulting in nothing results, or in code reflection cannot be used from generated functions errors). Those should be relatively easy to fix though, with the core fixes in place. EDIT: actually, it was only a single call to which. I've pushed a commit.

IanButterworth commented 1 year ago

It would be good to get this released

maleadt commented 1 year ago

@IanButterworth Can you merge the latest version of my branch?

IanButterworth commented 1 year ago

I believe this branch is up to date with yours?

maleadt commented 1 year ago

Ah yes, the GitHub UI is confusing here.

Can we get this (squash-)merged and tagged then? @CarloLucibello @ToucheSir