-
Notifications
You must be signed in to change notification settings - Fork 82
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
[ GPU ] Update android_test.sh to enable unittests for GPU @open sesame 11/04 09:08 #2773
Conversation
📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2773. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/. |
cibot: @EunjuYang, The last line of a text file must have a newline character. Please append a new line at the end of the line in docs/how-to-use-testcases.md. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EunjuYang, 💯 All CI checkers are successfully verified. Thanks.
48ca2f9
to
f8d520f
Compare
@@ -458,7 +458,6 @@ LOCAL_SRC_FILES := \ | |||
../unittest/layers/unittest_layers_flatten.cpp \ | |||
../unittest/layers/unittest_layers_activation.cpp \ | |||
../unittest/layers/unittest_layers_addition.cpp \ | |||
../unittest/layers/unittest_layers_addition_cl.cpp \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question) is there any reason to remove this from unittest_layers build ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ../unittest/layers/unittest_layers_addition_cl.cpp
should be added after #2752.
If not, NDK build returns error, for the addition layer for OpenCL is not implemented in the current upstream. I wanted to make upstream run as it is.
By the way... it would be better to change this PR [Wait for #2752], while removing this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see, thank you for detail explain
By the way... it would be better to change this PR [Wait for https://github.com//pull/2752], while removing this change.
Yes i would be good!! or i think you can simply rebase #2752 or cherry pick some commit's ( if there are some dependency)
rebase sample command
git pull --rebase upstream refs/pull/2752/head:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! I will rebase #2752. Thank you ☺
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DonghakPark
I tested rebasing on #2752, but the PR itself is not well rebased on the main; It makes this PR jammed.
Instead of rebasing on it, I updated the Android.mk
to leave line you mentioned as commented out. Could you please check the update?
If #2752 is merged or it resolves the rebasing issue, I will update this PR as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, it seems to be good. It may reduce inconvenience
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EunjuYang, 💯 All CI checkers are successfully verified. Thanks.
f8d520f
to
7bcc1f4
Compare
7bcc1f4
to
e00d205
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EunjuYang, 💯 All CI checkers are successfully verified. Thanks.
tools/android_test.sh
Outdated
# $ adb push res/ /data/local/tmp/nntr_android_test | ||
# meson build will unzip golden data for the unit tests | ||
cd ../../../ | ||
meson build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be nice to add some basic flags to get the proper golden data (e.g., -Denable-fp16=true
, -Denable-neon=true
, -Denable-opencl=true
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!! for more information we use below flags for build android_package
-Dplatform=android
-Dopenblas-num-threads=1
-Denable-tflite-interpreter=false
-Denable-tflite-backbone=false
-Denable-fp16=true
-Denable-neon=true
-Domp-num-threads=1
-Denable-opencl=true
-Dhgemm-experimental-kernel=false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the comments, all! It would be better to apply the same options for the android build. I will apply it 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DonghakPark I applied every option you suggested except for the android option, formeson.build
doesn't create resource for unittest with -Dplatofrm=android
option
Please refer to
Line 439 in 63f9875
warning('test is not supported in android build, test skipped') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for considering my opinion.
I think we need to ensure that testing is enabled in the future, even for cases of Android as well. (As there are increasingly more examples utilizing Android.)
I will create an issue for this matter.
Don't we need a script to make golden data as well? |
Thank you for your comment. The script for the golden data was already updated in FYI, I attach some links for the golden layer generation parts additionally added in this update : nntrainer/test/input_gen/gen_layer_tests.py Line 927 in 63f9875
nntrainer/test/input_gen/gen_layer_tests.py Line 917 in 63f9875
nntrainer/test/input_gen/gen_layer_tests.py Line 885 in 63f9875
|
e00d205
to
84c9ca9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EunjuYang, 💯 All CI checkers are successfully verified. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tools/android_test.sh
Outdated
# $ adb push res/ /data/local/tmp/nntr_android_test | ||
# meson build will unzip golden data for the unit tests | ||
cd ../../../ | ||
meson build -Dopenblas-num-threads=1 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there might be cases where the build
directory already exists. we should handle that case!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point! I will update the script. Thanks!
- This commit updates android_test.sh to enable unittests for GPU - This commit updates `how-to-use-testcases.md` file, including guide for running test cases for OpenCL on Android. - This commit remove add kernel from Android.mk in order to make it enabled (commented out) - unittest_layers.tar.gz is updated to add nnlayer golden data for GPU layers. **Self evaluation**: 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Eunju Yang <[email protected]>
84c9ca9
to
71f3a4a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EunjuYang, 💯 All CI checkers are successfully verified. Thanks.
- This commit is related to nnstreamer#2774. - The test `setPropertiesInvalid_n_gpu` was failed with LayerPropertySemantics, which tests negative case for some layers not implemented for GPU. - This test should be enabled later. Self evaluation: Build test: [X]Passed [ ]Failed [ ]Skipped Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Eunju Yang <[email protected]>
71f3a4a
to
fd95c4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EunjuYang, 💯 All CI checkers are successfully verified. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This resolves many inconveniences that I have been working on. Thanks!
android_test.sh
to enable unittests for GPUnntrainer_opencl_kernels
folderhow-to-use-testcases.md
file, including guide for running test cases for OpenCL on Android.Note
Self evaluation: