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

Make the NNlib tests more robust #152

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ChrisRackauckas
Copy link
Member

The NNlib tests were found to not be catching many issues because they committed a few sins:

  1. The tests leaked between each other, so definitions in one can be effected by the other. This is fixed by using SafeTestsets.
  2. The tests did not have good coverage of potential use cases (i.e. no integration testing), and so they missed true failures. This is fixed by at least adding a downstream test on Flux.jl. It should probably hook into any GPU-based tests as well.
  3. It used a Manifest when testing, so errors that would be seen on user setups were not caught in the tests.

This should now correctly give red tests.

@ChrisRackauckas
Copy link
Member Author

Locally I see:

(NNlib) pkg> test NNlib
   Testing NNlib
 Resolving package versions...
Test Summary:        | Pass  Total
Activation Functions |  257    257
[ Info: Skipping Convolutional fuzzing tests, set NNLIB_TEST_FUZZING=true to run them
∇depthwiseconv_filter_direct/∇depthwiseconv_data_direct: Test Failed at C:\Users\accou\.julia\dev\NNlib\test\conv.jl:540
  Expression: ddims(∇conv_filter(x, dy, cdims)) == dw_pad
   Evaluated: [NaN, 150.0] == [135.0, 150.0]
Stacktrace:
 [1] top-level scope at C:\Users\accou\.julia\dev\NNlib\test\conv.jl:540
 [2] top-level scope at D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.3\Test\src\Test.jl:1107
 [3] top-level scope at C:\Users\accou\.julia\dev\NNlib\test\conv.jl:513
 [4] top-level scope at D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.3\Test\src\Test.jl:1107
 [5] top-level scope at C:\Users\accou\.julia\dev\NNlib\test\conv.jl:449
 [6] top-level scope at D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.3\Test\src\Test.jl:1107
 [7] top-level scope at C:\Users\accou\.julia\dev\NNlib\test\conv.jl:446
[ Info: Skipping Depthwise Convolutional fuzzing tests, set NNLIB_TEST_FUZZING=true to run them
 58.176720 seconds (143.73 M allocations: 7.287 GiB, 3.04% gc time)
   Testing NNlib tests passed

The testsets in the conv part seem to be nested in such a way that when they fail they don't trigger a failure?

@ChrisRackauckas
Copy link
Member Author

The current test failure traces back to:

https://travis-ci.org/FluxML/NNlib.jl/jobs/619051416#L203-L238

meaning that #142 is the culprit. Before that, the library had other hidden test failures:

https://travis-ci.org/FluxML/NNlib.jl/jobs/612356942#L207-L259

So I think the current issues may trace back to that PR? @staticfloat

@ChrisRackauckas
Copy link
Member Author

The downstream tests reproduce the expected failures:

https://travis-ci.org/FluxML/NNlib.jl/jobs/628608690#L488-L517

@ChrisRackauckas
Copy link
Member Author

Also, this sets Travis to -1 instead of -1.1 so it stays up to date!

@DhairyaLGandhi
Copy link
Member

I think testing should happen on multiple Julia versions regardless. Adding testing on 1.1 forces us to not break code on earlier versions too

@ChrisRackauckas
Copy link
Member Author

Added tests for older versions

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.

2 participants