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

White list for builtin functions and stdlib #5

Closed
Roger-luo opened this issue Feb 19, 2020 · 1 comment
Closed

White list for builtin functions and stdlib #5

Roger-luo opened this issue Feb 19, 2020 · 1 comment

Comments

@Roger-luo
Copy link
Collaborator

Glad to see this is implemented with Cassette, I never had time to actually finish it.

I'm trying this with Zygote on my own machine learning models and hit some error, e.g

julia> avoid_allocations(record, back, rand(10))
ERROR: BoundsError
Stacktrace:
 [1] copyto! at /Users/roger/.julia/packages/Cassette/7OymZ/src/context.jl:450 [inlined]
 [2] copyto! at ./array.jl:303 [inlined]
 [3] overdub(::Cassette.Context{nametype(ReplayCtx),AutoPreallocation.AllocationReplay{Array{Array,1}},Nothing,Cassette.var"##PassType#404",Nothing,Cassette.DisableHooks}, ::typeof(copyto!), ::Array{Any,1}, ::Array{Any,1}) at /Users/roger/.julia/packages/Cassette/7OymZ/src/overdub.jl:0
 [4] _collect_indices at ./array.jl:578 [inlined]
 [5] overdub(::Cassette.Context{nametype(ReplayCtx),AutoPreallocation.AllocationReplay{Array{Array,1}},Nothing,Cassette.var"##PassType#404",Nothing,Cassette.DisableHooks}, ::typeof(Base._collect_indices), ::Tuple{Base.OneTo{Int64}}, ::Array{Any,1}) at /Users/roger/.julia/packages/Cassette/7OymZ/src/overdub.jl:0
 [6] collect at ./array.jl:562 [inlined]
 [7] _totuple at ./tuple.jl:248 [inlined]
 [8] Tuple at ./tuple.jl:220 [inlined]
 [9] back at /Users/roger/Documents/TNFilters.jl/src/layers/mps.jl:144 [inlined]
 [10] overdub(::Cassette.Context{nametype(ReplayCtx),AutoPreallocation.AllocationReplay{Array{Array,1}},Nothing,Cassette.var"##PassType#404",Nothing,Cassette.DisableHooks}, ::TNFilters.var"#back#21"{Context,MPS{10,2,4,Float64,Array{Float64,3}},Array{Int64,1},Array{Any,1},TNFilters.var"#54#back#7"{TNFilters.var"#5#6"{Array{Float64,3}}}}, ::Array{Float64,1}) at /Users/roger/.julia/packages/Cassette/7OymZ/src/overdub.jl:0
 [11] #75#back at /Users/roger/.julia/packages/ZygoteRules/6nssF/src/adjoint.jl:49 [inlined]

This is because there is a part of the backward rules make use of Tuple(::Array) to convert gradient of a Tuple back, which looks like

function back(Δ)
        _, Δ = tr_back(Δ)
        grads = []
        for i in length(op):-1:2
            _, Δ, grad_tensor = stack[i-1](Δ)
            accum_param(__context__, op.tensors[i][configs[i]], grad_tensor)
            push!(grads, grad_tensor)
        end
        accum_param(__context__, op.tensors[1][configs[1]], Δ)
        push!(grads, Δ)
        return (;tensors=Tuple(grads)), nothing
    end

I know this could be workaround by not using Tuple, but I guess in most cases this is not a bottleneck and it will can be generated by other things (since it's just a type conversion) so #4 might not be helpful in such case. I think in practice, we could white list some functions that will use dynamic realloc but usually not performance/memory bottleneck. I think we did this in Alloc.jl before: https://github.com/FluxML/Alloc.jl/pull/1/files#diff-c678e3660b372f14911e30988c8632ceR45

This will also partly workaround #2 I think and for copy and similar I found it could be worth to handle them explicitly since there might be some performance issue (might be related to #2 ).

@oxinabox
Copy link
Owner

Seems legit yes.

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

2 participants