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

Updated models in the zoo to use the latest versions of various packages #143

Merged
merged 14 commits into from
Sep 27, 2019

Conversation

SudhanshuAgrawal27
Copy link
Contributor

Updated the Manifests of cppn, mnist, and cifar10

Ensured that the sample output for cppn saves to the same directory as the program file.

Ensured that the model created by mnist\conv saves to the same directory as the program file.

Replaced the given sample output of cppn with a new one.

Tried changing the optimiser from ADAM to Momentum in the cifar10 program but got a better accuracy with ADAM so reverted to it.

@SudhanshuAgrawal27 SudhanshuAgrawal27 changed the title Updated vision models to use the latest versions of various packages Updated models in the zoo to use the latest versions of various packages Jun 11, 2019
@SudhanshuAgrawal27
Copy link
Contributor Author

Commits on June 11, 2019:
Updating the 60 minute blitz

Updated Manifest and Project of the 60 minute blitz

In the code:

  1. Changed derivative() to gradient()

  2. When gradient() is used, the example given to find the second derivative doesn't work. Fixed according to the example given in the official Flux documentation.
    https://fluxml.ai/Flux.jl/stable/models/basics/#Taking-Gradients-1

  3. Changed optimiser definition from SGD(params(m), 0.01) to Descent(0.01) and updated the Flux.train!() function as well.

  4. Line 425 in the original code only works if splatting isn't done.

@shreyas-kowshik
Copy link
Contributor

Looks good to merge to me.

@SudhanshuAgrawal27
Copy link
Contributor Author

Updated Manifest of char-rnn. No code changes required to ensure compatibility.

@SudhanshuAgrawal27 SudhanshuAgrawal27 changed the title Updated models in the zoo to use the latest versions of various packages [WIP] Updated models in the zoo to use the latest versions of various packages Jun 18, 2019
@SudhanshuAgrawal27
Copy link
Contributor Author

No code changes required

@SudhanshuAgrawal27 SudhanshuAgrawal27 changed the title [WIP] Updated models in the zoo to use the latest versions of various packages Updated models in the zoo to use the latest versions of various packages Jun 18, 2019
@shreyas-kowshik
Copy link
Contributor

@SudhanshuAgrawal27 Have you tested the code locally with these versions? It throws up errors while instantiating the environments on my system. Try this :

# Go to the folder with the file to run
julia --project=Project.toml __file-name__.jl

and tell me if it works fine for all the files...

@SudhanshuAgrawal27
Copy link
Contributor Author

@shreyas-kowshik I've tested the code locally for all these programs. Which environments in particular are giving you errors? And what are these errors?

@shreyas-kowshik
Copy link
Contributor

@SudhanshuAgrawal27 Why not also consider changing the version of Flux to the latest one for 60-minute-blitz.jl? The changes seem to be broken on the current version...

@DhairyaLGandhi
Copy link
Member

@SudhanshuAgrawal27 bump
Let's get this merged, please

@SudhanshuAgrawal27
Copy link
Contributor Author

@SudhanshuAgrawal27 Why not also consider changing the version of Flux to the latest one for 60-minute-blitz.jl? The changes seem to be broken on the current version...

@shreyas-kowshik I've already included an update to the 60 minute blitz in this PR. I've tested it too as you suggested

@SudhanshuAgrawal27 Have you tested the code locally with these versions? It throws up errors while instantiating the environments on my system. Try this :

# Go to the folder with the file to run
julia --project=Project.toml __file-name__.jl

and tell me if it works fine for all the files...

and it seems to work fine.

@DhairyaLGandhi
Copy link
Member

@shreyas-kowshik can you list the errors you faced?

@DhairyaLGandhi
Copy link
Member

@SudhanshuAgrawal27 maybe also test the blitz with Flux#zygote

@shreyas-kowshik
Copy link
Contributor

Here is the stacktrace :

ERROR: LoadError: MethodError: no method matching make_makeargs(::Base.Broadcast.Broadcasted{Flux.Tracker.TrackedStyle,Nothing,typeof(identity),Tuple{Base.Broadcast.Broadcasted{Flux.Tracker.TrackedStyle,Nothing,typeof(+),Tuple{TrackedArray{…,Array{Float64,1}},TrackedArray{…,Array{Float64,1}}}}}})
Closest candidates are:
  make_makeargs(::Any, !Matched::Tuple{}) at broadcast.jl:327
  make_makeargs(::Any, !Matched::Tuple{#s4,Vararg{Any,N} where N} where #s4<:Base.Broadcast.Broadcasted) at /home/shreyas/.julia/packages/Flux/rcN9D/src/tracker/array.jl:439
  make_makeargs(::Any, !Matched::Tuple) at broadcast.jl:329

This is due to incompatibility with Float64 as input to the Dense layer which seems to be broken on Flux v0.6.8 but works in v0.8.1+.

@DhairyaLGandhi
Copy link
Member

Is this with the environments in the PR?

@shreyas-kowshik
Copy link
Contributor

Yes. With the version of the file in Project.toml in the PR.

@SudhanshuAgrawal27
Copy link
Contributor Author

@shreyas-kowshik I've just tested it again. It's working fine for me.
You said that the program is supposed to work on Flux v0.8.1+. The version in the new Manifest I've put up is v0.8.2.

@dhairyagandhi96 could you possibly test it to see if it works for you?

@DhairyaLGandhi
Copy link
Member

DhairyaLGandhi commented Aug 14, 2019

Yeah

Can you also update the cuda folders wherever relevant?
EDIT:
And maybe add one to cifar10

@SudhanshuAgrawal27
Copy link
Contributor Author

@dhairyagandhi96 Actually I couldn't ever get CuArrays to work on my system while I was at the office. I downloaded the toolkit etc, but it never built properly.

I'll give it another shot, and if it works I'll put the changes in another PR, so as not to hold up this one.

@SudhanshuAgrawal27
Copy link
Contributor Author

@dhairyagandhi96 CuArrays still don't work on my system for some reason. I'm guessing it will take me a while to fix whatever's wrong.

Have you had the chance to test that program @shreyas-kowshik and I were discussing?

@lassepe
Copy link

lassepe commented Aug 29, 2019

Yeah

Can you also update the cuda folders wherever relevant?
EDIT:
And maybe add one to cifar10

To get this to work with cuda, one will need to do more than just updating the dependencies I suppose. E.g. for the vision/mnist/mlp.jl I get (after updating the cuda environment):

KernelError: passing and using non-bitstype argument

Argument 4 to your kernel function is of type Base.Broadcast.Broadcasted{Nothing,Tuple{Base.OneTo{Int64}},typeof(==),Tuple{Base.Broadcast.Extruded{CUDAnative.CuDeviceArray{Int64,1,CUDAnative.AS.Global},Tuple{Bool},Tuple{Int64}},Base.Broadcast.Extruded{Array{Int64,1},Tuple{Bool},Tuple{Int64}}}}.
That type is not isbits, and such arguments are only allowed when they are unused by the kernel.  .args is of type Tuple{Base.Broadcast.Extruded{CUDAnative.CuDeviceArray{Int64,1,CUDAnative.AS.Global},Tuple{Bool},Tuple{Int64}},Base.Broadcast.Extruded{Array{Int64,1},Tuple{Bool},Tuple{Int64}}} which is not isbits.
    .2 is of type Base.Broadcast.Extruded{Array{Int64,1},Tuple{Bool},Tuple{Int64}} which is not isbits.
      .x is of type Array{Int64,1} which is not isbits.


Stacktrace:
 [1] check_invocation(::CUDAnative.CompilerJob, ::LLVM.Function) at /home/lassepe/.julia/packages/CUDAnative/nItlk/src/compiler/validation.jl:70
 [2] macro expansion at /home/lassepe/.julia/packages/CUDAnative/nItlk/src/compiler/driver.jl:187 [inlined]
 [3] macro expansion at /home/lassepe/.julia/packages/TimerOutputs/7zSea/src/TimerOutput.jl:216 [inlined]
 [4] #codegen#121(::Bool, ::Bool, ::Bool, ::Bool, ::Bool, ::typeof(CUDAnative.codegen), ::Symbol, ::CUDAnative.CompilerJob) at /home/lassepe/.julia/packages/CUDAnative/nItlk/src/compiler/driver.jl:186
 [5] #codegen at /home/lassepe/.julia/packages/CUDAnative/nItlk/src/compiler/driver.jl:0 [inlined]
 [6] #compile#120(::Bool, ::Bool, ::Bool, ::Bool, ::Bool, ::typeof(CUDAnative.compile), ::Symbol, ::CUDAnative.CompilerJob) at /home/lassepe/.julia/packages/CUDAnative/nItlk/src/compiler/driver.jl:47
 [7] #compile at ./none:0 [inlined]
 [8] #compile#119 at /home/lassepe/.julia/packages/CUDAnative/nItlk/src/compiler/driver.jl:28 [inlined]
 [9] #compile at ./none:0 [inlined] (repeats 2 times)
 [10] macro expansion at /home/lassepe/.julia/packages/CUDAnative/nItlk/src/execution.jl:388 [inlined]
 [11] #cufunction#161(::Nothing, ::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}, ::typeof(CUDAnative.cufunction), ::getfield(GPUArrays, Symbol("##23#24")), ::Type{Tuple{CuArrays.CuKernelState,CUDAnative.CuDeviceArray{Bool,1,CUDAnative.AS.Global},Base.Broadcast.Broadcasted{Nothing,Tuple{Base.OneTo{Int64}},typeof(==),Tuple{Base.Broadcast.Extruded{CUDAnative.CuDeviceArray{Int64,1,CUDAnative.AS.Global},Tuple{Bool},Tuple{Int64}},Base.Broadcast.Extruded{Array{Int64,1},Tuple{Bool},Tuple{Int64}}}}}}) at /home/lassepe/.julia/packages/CUDAnative/nItlk/src/execution.jl:356
 [12] cufunction(::Function, ::Type) at /home/lassepe/.julia/packages/CUDAnative/nItlk/src/execution.jl:356
 [13] macro expansion at /home/lassepe/.julia/packages/CUDAnative/nItlk/src/execution.jl:174 [inlined]
 [14] macro expansion at ./gcutils.jl:87 [inlined]
 [15] macro expansion at /home/lassepe/.julia/packages/CUDAnative/nItlk/src/execution.jl:171 [inlined]
 [16] _gpu_call(::CuArrays.CuArrayBackend, ::Function, ::CuArray{Bool,1}, ::Tuple{CuArray{Bool,1},Base.Broadcast.Broadcasted{Nothing,Tuple{Base.OneTo{Int64}},typeof(==),Tuple{Base.Broadcast.Extruded{CuArray{Int64,1},Tuple{Bool},Tuple{Int64}},Base.Broadcast.Extruded{Array{Int64,1},Tuple{Bool},Tuple{Int64}}}}}, ::Tuple{Tuple{Int64},Tuple{Int64}}) at /home/lassepe/.julia/packages/CuArrays/wXQp8/src/gpuarray_interface.jl:60
 [17] gpu_call at /home/lassepe/.julia/packages/GPUArrays/J4c3Q/src/abstract_gpu_interface.jl:151 [inlined]
 [18] gpu_call at /home/lassepe/.julia/packages/GPUArrays/J4c3Q/src/abstract_gpu_interface.jl:128 [inlined]
 [19] copyto! at /home/lassepe/.julia/packages/GPUArrays/J4c3Q/src/broadcast.jl:48 [inlined]
 [20] copyto! at ./broadcast.jl:842 [inlined]
 [21] copy(::Base.Broadcast.Broadcasted{Base.Broadcast.ArrayStyle{CuArray},Tuple{Base.OneTo{Int64}},typeof(==),Tuple{CuArray{Int64,1},Array{Int64,1}}}) at ./broadcast.jl:818
 [22] materialize(::Base.Broadcast.Broadcasted{Base.Broadcast.ArrayStyle{CuArray},Nothing,typeof(==),Tuple{CuArray{Int64,1},Array{Int64,1}}}) at ./broadcast.jl:798
 [23] accuracy(::CuArray{Float32,2}, ::Flux.OneHotMatrix{CuArray{Flux.OneHotVector,1}}) at /home/lassepe/worktree/model-zoo/vision/mnist/mlp.jl:23
 [24] top-level scope at /home/lassepe/worktree/model-zoo/vision/mnist/mlp.jl:31
 [25] include at ./boot.jl:328 [inlined]
 [26] include_relative(::Module, ::String) at ./loading.jl:1094
 [27] include(::Module, ::String) at ./Base.jl:31
 [28] include(::String) at ./client.jl:431
 [29] top-level scope at REPL[8]:1
in expression starting at /home/lassepe/worktree/model-zoo/vision/mnist/mlp.jl:31

Update:

The problem is that in the given example onecold(X) give a CuArray while onecold(Y) is a Base Array. So a naive fix would be accuracy(x, y) = mean(onecold(m(x)|>cpu) .== onecold(y))

@DhairyaLGandhi
Copy link
Member

Right, there is a PR on flux that should get rid of that particular issue I think.
I would very much like to see this merged.

@SudhanshuAgrawal27
Copy link
Contributor Author

@dhairyagandhi96
Everything except the cuda folders is ready. And like I said, I don't think I'll be able to update those anytime soon, and if I do, I'll include those changes in another PR. This PR is ready to go.
Did you get a chance to check the issue @shreyas-kowshik was having? I think that's where we last left off.

@aminya
Copy link

aminya commented Sep 27, 2019

I would appreciate it if this is merged as soon as possible. If there are problems we should create separate issues and solve those separately.

@DhairyaLGandhi
Copy link
Member

Fair enough, I think this has been a while in the doing. Thanks @SudhanshuAgrawal27 @shreyas-kowshik

cc @aminya

We still will need to look at the CUDA manifests, although those should be relatively easy to do

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 this pull request may close these issues.

None yet

5 participants