Skip to content

Refactor tests and make NNPACK optional #154

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Dec 24, 2019
Merged

Refactor tests and make NNPACK optional #154

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f5cb955
refactor tests
Dec 23, 2019
b8fb492
rm throwing fn
Dec 23, 2019
daa862a
obscuring conv_nnpack for CI
Dec 23, 2019
36010e4
add return types annotations
Dec 24, 2019
1b4ce08
reverting the type annotations
Dec 24, 2019
dc8d734
switch NNPACK off
Dec 24, 2019
ba8e771
let the user decide
Dec 24, 2019
cc06137
fixes
Dec 24, 2019
836889d
make NNPACK conditional
Dec 24, 2019
cd789e8
check_deps from deps.jl
Dec 24, 2019
db59d8e
fallback return false
Dec 24, 2019
20e8e47
write deps on build everytime
Dec 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/NNlib.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,14 @@ using Requires
include("dim_helpers.jl")

# NNPACK support
if Sys.islinux() || Sys.isapple()
include("nnpack/NNPACK.jl")
if get(ENV, "NNLIB_USE_NNPACK", "false") == "true"
if Sys.islinux() || Sys.isapple()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is then something that needs to be set during precompile time. And if you want to change it, how do you cause the package to be recompiled for it to take affect?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, maybe I should move this to an __init__ function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That has some other negatives in the sense that the NNPack code will no longer be preconpilable. One way would probably be to not use dispatch to select the backend code but instead have something like

function conv(...)
    if get(ENV(...))
         _NNPack_conv(...)
    else
        generic_conv(...)
    end
end

But that would require some restructuring of code that might be a bit annoying.

The easiest is probably to move to use a build step. The build file looks at the ENV variable and creates a file that includes the NNPack depending on the env variable. If one wants to change this option, one sets the nrw env var and the runs build on the package.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean doing this in the build.jl file and write to deps.jl?

Copy link
Member Author

@DhairyaLGandhi DhairyaLGandhi Dec 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made it so.

include("nnpack/NNPACK.jl")
else
is_nnpack_available() = false
end
else
is_nnpack_available() = false
is_nnpack_available() = false
end

include("activation.jl")
Expand Down
30 changes: 18 additions & 12 deletions test/conv.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,9 +274,13 @@ conv_answer_dict = Dict(
# A "drop channels and batch dimension" helper
ddims(x) = dropdims(x, dims=(rank+1, rank+2))

for conv in (NNlib.conv, NNlib.conv_im2col, NNlib.conv_direct, NNlib.conv_nnpack)
if conv == NNlib.conv_nnpack && !NNlib.nnpack_supported_operation(DenseConvDims(x, w))
continue
convs = [NNlib.conv, NNlib.conv_im2col, NNlib.conv_direct,]
NNlib.is_nnpack_available() && push!(convs, NNlib.conv_nnpack)
for conv in convs
if NNlib.is_nnpack_available()
if conv == NNlib.conv_nnpack && !NNlib.nnpack_supported_operation(DenseConvDims(x, w))
continue
end
end
@testset "$(conv)" begin
cdims = DenseConvDims(x, w)
Expand Down Expand Up @@ -352,12 +356,11 @@ conv_answer_dict = Dict(
end
end
end
end

if get(ENV,"NNLIB_TEST_FUZZING","false") == "true"
# @info("Skipping Convolutional fuzzing tests, set NNLIB_TEST_FUZZING=true to run them")
@testset "fuzzing" begin
if get(ENV,"NNLIB_TEST_FUZZING","false") != "true"
@info("Skipping Convolutional fuzzing tests, set NNLIB_TEST_FUZZING=true to run them")
return
end
@info("Starting Convolutional fuzzing tests; this can take a few minutes...")
# Now that we're fairly certain things are working, let's fuzz things a little bit:
for x_size in (
Expand Down Expand Up @@ -441,9 +444,10 @@ conv_answer_dict = Dict(
end
println()
end
else
@info "Skipping Convolutional fuzzing tests, set NNLIB_TEST_FUZZING=true to run them"
end


@testset "Depthwise Convolution" begin
# Start with some easy-to-debug cases that we have worked through and _know_ work
for rank in (1,) #2,3)
Expand Down Expand Up @@ -552,12 +556,11 @@ end
end
end
end
end


if get(ENV,"NNLIB_TEST_FUZZING","false") == "true"
@testset "fuzzing" begin
if get(ENV,"NNLIB_TEST_FUZZING","false") != "true"
@info("Skipping Depthwise Convolutional fuzzing tests, set NNLIB_TEST_FUZZING=true to run them")
return
end
@info("Starting Depthwise Convolutional fuzzing tests; this can take a few minutes...")
# Now that we're fairly certain things are working, let's fuzz things a little bit:
for x_size in (
Expand Down Expand Up @@ -641,8 +644,11 @@ end
end
println()
end
else
@info "Skipping Depthwise Convolutional fuzzing tests, set NNLIB_TEST_FUZZING=true to run them"
end


@testset "conv_wrapper" begin
x = rand(10, 10, 3, 10)
w = rand(2, 2, 3, 16)
Expand Down
5 changes: 2 additions & 3 deletions test/inference.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using NNlib, Test
using NNlib: conv_direct, conv_im2col
import NNlib: conv_direct, conv_im2col

@testset "Conv Inference" begin
x = rand(10, 10, 3, 2)
Expand All @@ -9,6 +8,6 @@ using NNlib: conv_direct, conv_im2col
NNlib.is_nnpack_available() && push!(impl, NNlib.conv_nnpack)

for T in impl
@inferred T(x, w, DenseConvDims(x, w))
@test T(x, w, DenseConvDims(x, w)) isa AbstractArray{K,4} where K
end
end
14 changes: 8 additions & 6 deletions test/pooling.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using NNlib, Test
#using NNlib, Test

maxpool_answer_dict = Dict(
1 => Dict(
Expand Down Expand Up @@ -298,11 +298,13 @@ for rank in (1, 2, 3)
end
end

x = rand(10, 10, 3, 10)
@test size(maxpool(x, (2, 2))) == (5, 5, 3, 10)
@test size(maxpool(x, (2, 2); pad = (1, 1), stride = (2, 2))) == (6, 6, 3, 10)
@test size(meanpool(x, (2, 2))) == (5, 5, 3, 10)
@test size(meanpool(x, (2, 2); pad = (1, 1), stride = (2, 2))) == (6, 6, 3, 10)
@testset "Pooling - Check Sizes" begin
x = rand(10, 10, 3, 10)
@test size(maxpool(x, (2, 2))) == (5, 5, 3, 10)
@test size(maxpool(x, (2, 2); pad = (1, 1), stride = (2, 2))) == (6, 6, 3, 10)
@test size(meanpool(x, (2, 2))) == (5, 5, 3, 10)
@test size(meanpool(x, (2, 2); pad = (1, 1), stride = (2, 2))) == (6, 6, 3, 10)
end

# Add another test for 2d maxpool that uses an odd-length size:
@testset "Issue #133" begin
Expand Down