Skip to content
This repository was 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 intoapache:masterfrom
reminisce:add_non_tvm_op_pipeline
Oct 19, 2019
Merged

Add test pipeline for USE_TVM_OP=OFF on Unix#16450
reminisce merged 7 commits intoapache:masterfrom
reminisce:add_non_tvm_op_pipeline

Conversation

@reminisce
Copy link
Contributor

@reminisce 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 reminisce force-pushed the add_non_tvm_op_pipeline branch from a019b0b to 410623f Compare October 14, 2019 06:47
rondogency
rondogency reviewed Oct 14, 2019
View reviewed changes
ci/jenkins/Jenkins_steps.groovy Outdated

// 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'
Copy link
Contributor

@rondogency rondogency Oct 14, 2019

Choose a reason for hiding this comment

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

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

samskalicky
samskalicky reviewed Oct 15, 2019
View reviewed changes
ci/jenkins/Jenkins_steps.groovy
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/*'
Copy link
Contributor

@samskalicky samskalicky Oct 15, 2019

Choose a reason for hiding this comment

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

@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"

Copy link
Contributor

@mseth10 mseth10 Oct 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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 reminisce force-pushed the add_non_tvm_op_pipeline branch from 921f6fc to 7d133eb Compare October 15, 2019 18:49
mseth10
mseth10 reviewed Oct 15, 2019
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)
Copy link
Contributor

@mseth10 mseth10 Oct 15, 2019

Choose a reason for hiding this comment

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

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

mseth10
mseth10 reviewed Oct 15, 2019
View reviewed changes
ci/jenkins/Jenkins_steps.groovy Outdated
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/libsample_lib.so, build/3rdparty/dmlc-core/libdmlc.a, build/tests/mxnet_unit_tests, build/3rdparty/openmp/runtime/src/libomp.so'
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'
Copy link
Contributor

@mseth10 mseth10 Oct 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

mseth10
mseth10 reviewed Oct 15, 2019
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)
Copy link
Contributor

@mseth10 mseth10 Oct 15, 2019

Choose a reason for hiding this comment

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

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
mseth10 reviewed Oct 15, 2019
View reviewed changes
ci/jenkins/Jenkins_steps.groovy Outdated
// 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'
Copy link
Contributor

@mseth10 mseth10 Oct 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

mseth10
mseth10 suggested changes Oct 15, 2019
View reviewed changes
Copy link
Contributor

@mseth10 mseth10 left a comment

Choose a reason for hiding this comment

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

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 reminisce force-pushed the add_non_tvm_op_pipeline branch from 7d133eb to 4902c5a Compare October 15, 2019 22:54
@reminisce
Copy link
Contributor Author

reminisce commented Oct 15, 2019

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

mseth10
mseth10 approved these changes Oct 15, 2019
View reviewed changes
Copy link
Contributor

@mseth10 mseth10 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments. Looks good to me.

@reminisce
Copy link
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.

@aaronmarkham
Copy link
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 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 reminisce force-pushed the add_non_tvm_op_pipeline branch from f568b87 to 9d61d86 Compare October 19, 2019 04:31
@reminisce reminisce merged commit 9fecfbb into apache:master Oct 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@yzhliu yzhliu Awaiting requested review from yzhliu

@szha szha Awaiting requested review from szha

@aaronmarkham aaronmarkham Awaiting requested review from aaronmarkham aaronmarkham is a code owner

4 more reviewers

@samskalicky samskalicky samskalicky left review comments

@rondogency rondogency rondogency left review comments

@marcoabreu marcoabreu marcoabreu approved these changes

@mseth10 mseth10 mseth10 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@reminisce @aaronmarkham @samskalicky @rondogency @marcoabreu @mseth10