This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Add test pipeline for USE_TVM_OP=OFF on Unix #16450

Merged

reminisce merged 7 commits into apache:master from reminisce:add_non_tvm_op_pipeline

Oct 19, 2019

Contributor

reminisce commented Oct 11, 2019

Description

Currently, USE_TVM_OP defaults toON on Unix machines in the CI. When one operator is implemented w/ and w/o TVM, the non-TVM implementation is not tested in the CI on Unix machines. This PR adds additional test pipeline for those implementations on Unix.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Feature1, tests, (and when applicable, API doc)
Feature2, tests, (and when applicable, API doc)

Comments

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

reminisce requested review from yzhliu, szha and marcoabreu

October 11, 2019 23:08

reminisce requested a review from aaronmarkham as a code owner

October 11, 2019 23:08

reminisce force-pushed the add_non_tvm_op_pipeline branch from a019b0b to 410623f Compare

October 14, 2019 06:47

rondogency reviewed

View reviewed changes

ci/jenkins/Jenkins_steps.groovy Outdated

               // Python wheels
               mx_pip = 'build/*.whl'
               // mxnet cmake libraries, in cmake builds we do not produce a libnvvm static library by default.
               mx_cmake_lib = 'build/libmxnet.so, build/libmxnet.a, build/3rdparty/tvm/libtvm_runtime.so, build/libtvmop.so, build/3rdparty/dmlc-core/libdmlc.a, build/tests/mxnet_unit_tests, build/3rdparty/openmp/runtime/src/libomp.so'
+              mx_cmake_lib_no_tvm_op = 'build/libmxnet.so, build/libmxnet.a, build/3rdparty/dmlc-core/libdmlc.a, build/tests/mxnet_unit_tests, build/3rdparty/openmp/runtime/src/libomp.so'

Contributor

rondogency Oct 14, 2019

can you add libsample_lib.so as target here like line 28?

samskalicky reviewed

View reviewed changes

ci/jenkins/Jenkins_steps.groovy

               // mxnet cmake libraries, in cmake builds we do not produce a libnvvm static library by default.
               mx_cmake_lib_debug = 'build/libmxnet.so, build/libmxnet.a, build/3rdparty/tvm/libtvm_runtime.so, build/libtvmop.so, build/libsample_lib.so, build/3rdparty/dmlc-core/libdmlc.a, build/tests/mxnet_unit_tests'
               mx_cmake_mkldnn_lib = 'build/libmxnet.so, build/libmxnet.a, build/3rdparty/tvm/libtvm_runtime.so, build/libtvmop.so, build/3rdparty/dmlc-core/libdmlc.a, build/tests/mxnet_unit_tests, build/3rdparty/openmp/runtime/src/libomp.so, build/3rdparty/mkldnn/src/libmkldnn.so.0'
               mx_mkldnn_lib = 'lib/libmxnet.so, lib/libmxnet.a, lib/libtvm_runtime.so, lib/libtvmop.so, libsample_lib.so, lib/libiomp5.so, lib/libmkldnn.so.0, lib/libmklml_intel.so, 3rdparty/dmlc-core/libdmlc.a, 3rdparty/tvm/nnvm/lib/libnnvm.a'
               mx_tensorrt_lib = 'build/libmxnet.so, build/3rdparty/tvm/libtvm_runtime.so, build/libtvmop.so, lib/libnvonnxparser_runtime.so.0, lib/libnvonnxparser.so.0, lib/libonnx_proto.so, lib/libonnx.so'
               mx_lib_cpp_examples = 'lib/libmxnet.so, lib/libmxnet.a, lib/libtvm_runtime.so, lib/libtvmop.so, libsample_lib.so, 3rdparty/dmlc-core/libdmlc.a, 3rdparty/tvm/nnvm/lib/libnnvm.a, 3rdparty/ps-lite/build/libps.a, deps/lib/libprotobuf-lite.a, deps/lib/libzmq.a, build/cpp-package/example/*, python/mxnet/_cy2/*.so, python/mxnet/_cy3/*.so'
+              mx_lib_cpp_examples_no_tvm_op = 'lib/libmxnet.so, lib/libmxnet.a, libsample_lib.so, 3rdparty/dmlc-core/libdmlc.a, 3rdparty/tvm/nnvm/lib/libnnvm.a, 3rdparty/ps-lite/build/libps.a, deps/lib/libprotobuf-lite.a, deps/lib/libzmq.a, build/cpp-package/example/*, python/mxnet/_cy2/*.so, python/mxnet/_cy3/*.so'
               mx_lib_cpp_examples_cpu = 'build/libmxnet.so, build/3rdparty/tvm/libtvm_runtime.so, build/libtvmop.so, build/cpp-package/example/*'

Contributor

samskalicky Oct 15, 2019

@mseth10 Can you take a look at all the libsample_lib.so's? Some of them have a path like "build/libsample_lib.so" and others are just "libsample_lib.so"

Contributor

mseth10 Oct 15, 2019 •

edited

Loading

I took a look and let @reminisce know offline. mx_cmake_lib_no_tvm_op is the binary set corresponding to a cmake build and hence we need to put it in a build/ directory.

reminisce force-pushed the add_non_tvm_op_pipeline branch from 921f6fc to 7d133eb Compare

October 15, 2019 18:49

mseth10 reviewed

View reviewed changes

ci/jenkins/Jenkins_steps.groovy Outdated

+                        timeout(time: max_time, unit: 'MINUTES') {
+                          utils.init_git()
+                          utils.docker_run('ubuntu_gpu_cu101', 'build_ubuntu_gpu_cmake_no_tvm_op', false)
+                          utils.pack_lib('cmake_gpu_no_tvm_op', mx_cmake_lib_cython_no_tvm_op)

Contributor

mseth10 Oct 15, 2019

We don't need to pack libraries here, as there is no test stage that uses it.

mseth10 reviewed

View reviewed changes

ci/jenkins/Jenkins_steps.groovy Outdated

		mx_cmake_lib_cython = 'build/libmxnet.so, build/libmxnet.a, build/3rdparty/tvm/libtvm_runtime.so, build/libtvmop.so, build/3rdparty/dmlc-core/libdmlc.a, build/tests/mxnet_unit_tests, build/3rdparty/openmp/runtime/src/libomp.so, python/mxnet/_cy2/.so, python/mxnet/_cy3/.so'
		mx_cmake_lib_cython_no_tvm_op = 'build/libmxnet.so, build/libmxnet.a, build/3rdparty/dmlc-core/libdmlc.a, build/tests/mxnet_unit_tests, build/3rdparty/openmp/runtime/src/libomp.so, python/mxnet/_cy2/.so, python/mxnet/_cy3/.so'

Contributor

mseth10 Oct 15, 2019 •

edited

Loading

We do not need this library set, as a result of above comment:
https://github.com/apache/incubator-mxnet/pull/16450/files#r335187907

mseth10 reviewed

View reviewed changes

ci/jenkins/Jenkins_steps.groovy Outdated

+                    node(NODE_LINUX_GPU) {
+                      ws('workspace/ut-python3-gpu-no-tvm-op') {
+                        try {
+                          utils.unpack_and_init('gpu_no_tvm_op', mx_lib_cython_no_tvm_op)

Contributor

mseth10 Oct 15, 2019

The build corresponding to this test packs library set mx_lib_cpp_examples_no_tvm_op
https://github.com/apache/incubator-mxnet/pull/16450/files#diff-bb61a49bf10098c4c42879f1632fdb40R273

We should use the same library set while unpacking. A new library set is not required.
Suggested action: Replace mx_lib_cython_no_tvm_op with mx_lib_cpp_examples_no_tvm_op

mseth10 reviewed

View reviewed changes

ci/jenkins/Jenkins_steps.groovy Outdated

@@ @@ -25,19 +25,23 @@ utils = load('ci/Jenkinsfile_utils.groovy') @@
               // mxnet libraries
               mx_lib = 'lib/libmxnet.so, lib/libmxnet.a, lib/libtvm_runtime.so, lib/libtvmop.so, libsample_lib.so, 3rdparty/dmlc-core/libdmlc.a, 3rdparty/tvm/nnvm/lib/libnnvm.a'
               mx_lib_cython = 'lib/libmxnet.so, lib/libmxnet.a, lib/libtvm_runtime.so, lib/libtvmop.so, libsample_lib.so, 3rdparty/dmlc-core/libdmlc.a, 3rdparty/tvm/nnvm/lib/libnnvm.a, python/mxnet/_cy2/*.so, python/mxnet/_cy3/*.so'
+              mx_lib_cython_no_tvm_op = 'lib/libmxnet.so, lib/libmxnet.a, libsample_lib.so, 3rdparty/dmlc-core/libdmlc.a, 3rdparty/tvm/nnvm/lib/libnnvm.a, python/mxnet/_cy2/*.so, python/mxnet/_cy3/*.so'

Contributor

mseth10 Oct 15, 2019 •

edited

Loading

We do not need this library set, as a result of above comment:
https://github.com/apache/incubator-mxnet/pull/16450/files#r335190763

mseth10 suggested changes

View reviewed changes

Contributor

mseth10 left a comment

Hi @reminisce I have left a few comments on reusing the library sets in test stages. We need to cache a library set in build stage only to be used later by a test stage, so we don't need to create separate library sets for both stages. Please let me know what you think.

reminisce force-pushed the add_non_tvm_op_pipeline branch from 7d133eb to 4902c5a Compare

October 15, 2019 22:54

Contributor Author

reminisce commented Oct 15, 2019

@mseth10 I have made changes as you suggested. Could you review again? Thanks.

mseth10 approved these changes

View reviewed changes

Contributor

mseth10 left a comment

Thanks for addressing the comments. Looks good to me.

marcoabreu approved these changes

View reviewed changes

Contributor Author

reminisce commented Oct 16, 2019

dist-kvstore tests GPU failure has shown up in quite a few PRs recently. Wonder what the root cause is.

Contributor

aaronmarkham commented Oct 16, 2019

dist-kvstore tests GPU failure has shown up in quite a few PRs recently. Wonder what the root cause is.

I noticed the same thing.

reminisce force-pushed the add_non_tvm_op_pipeline branch 2 times, most recently from 9fbeb2b to f568b87 Compare

October 18, 2019 23:07

reminisce added 7 commits

October 18, 2019 21:30


          Add no tvm op build and test pipelines

9d31218


          Add steps

4a6a74f


          Modify display

494a6f4


          Fix ci

97e3eeb


          Try to fix one unit test

e62882b

Fix

f13ac55


          Address cr

9d61d86

reminisce force-pushed the add_non_tvm_op_pipeline branch from f568b87 to 9d61d86 Compare

October 19, 2019 04:31

reminisce merged commit 9fecfbb into apache:master

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

samskalicky samskalicky left review comments

rondogency rondogency left review comments

marcoabreu marcoabreu approved these changes

mseth10 mseth10 approved these changes

yzhliu Awaiting requested review from yzhliu

szha Awaiting requested review from szha

aaronmarkham Awaiting requested review from aaronmarkham aaronmarkham is a code owner

Labels

None yet