Refactor tests and make NNPACK optional #154
Conversation
|
As a test, I have added a dummy function in there to throw and make sure that CI reports a failure. |
|
Things look as expected on Travis, AppVeyor needs a bit of hand holding to build |
Codecov Report
@@ Coverage Diff @@
## master #154 +/- ##
==========================================
- Coverage 74.41% 66.18% -8.24%
==========================================
Files 24 24
Lines 774 757 -17
==========================================
- Hits 576 501 -75
- Misses 198 256 +58
Continue to review full report at Codecov.
|
|
Added return type annotations to the NNPACK codebase to help inference along. Earlier it was inferring |
|
The inference works, but that breaks mixed precision |
|
So this also introduces a switch to turn off NNPACK, which is unset by default. Good for backwards compatibility and fleshing out the implementation. |
src/NNlib.jl
Outdated
| if Sys.islinux() || Sys.isapple() | ||
| include("nnpack/NNPACK.jl") | ||
| if get(ENV, "NNLIB_USE_NNPACK", "false") == "true" | ||
| if Sys.islinux() || Sys.isapple() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Good catch, maybe I should move this to an __init__ function.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You mean doing this in the build.jl file and write to deps.jl?
There was a problem hiding this comment.
I've made it so.
DhairyaLGandhi
changed the title
|
Merging this now, to get the ecosystem up and running. Hopefully it doesn't complicate debugging and testing too much @staticfloat ? |
This is aimed towards the multiple failures noted in NNlib due to the testing scheme failing to catch broken cases, even when errors were thrown
Relates to #153 and #152
Additionally the inference tests with the
@inferredmacro have been replaced with checking types from the output, this remains as a TODO to fix inference, and find what is causing inference to fail.For now, this refactor ensures that tests that have errored (esp. on CI), show up correctly. The difference comes up as
tests/conv.jlhas the skips in place for checking fuzzing tests, returns and further tests aren't marked with failurescc @staticfloat @ChrisRackauckas @MikeInnes