You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/11/09 20:44:18 UTC

[GitHub] [tvm] driazati opened a new pull request, #13335: [ci] Split out C++ unittests

driazati opened a new pull request, #13335:
URL: https://github.com/apache/tvm/pull/13335

   This makes C++ unittests follow the normal flow of build -> upload
   artifacts -> download and run tests. To simplify the changes there is a
   new utility for interacting with S3.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] driazati commented on a diff in pull request #13335: [ci] Split out C++ unittests

Posted by GitBox <gi...@apache.org>.
driazati commented on code in PR #13335:
URL: https://github.com/apache/tvm/pull/13335#discussion_r1023570932


##########
tests/scripts/task_microtvm_cpp_tests.sh:
##########
@@ -0,0 +1,44 @@
+#!/usr/bin/env bash

Review Comment:
   ah forgot to add calls to these in the relevant jobs after their C++ testing, fixed



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on a diff in pull request #13335: [ci] Split out C++ unittests

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #13335:
URL: https://github.com/apache/tvm/pull/13335#discussion_r1023418780


##########
tests/scripts/task_microtvm_cpp_tests.sh:
##########
@@ -0,0 +1,44 @@
+#!/usr/bin/env bash

Review Comment:
   is this used anywhere?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] alanmacd commented on a diff in pull request #13335: [ci] Split out C++ unittests

Posted by GitBox <gi...@apache.org>.
alanmacd commented on code in PR #13335:
URL: https://github.com/apache/tvm/pull/13335#discussion_r1019589504


##########
ci/jenkins/Build.groovy.j2:
##########
@@ -208,7 +231,9 @@ stage('Build') {
       script: "${docker_run} ${ci_hexagon} ./tests/scripts/task_config_build_hexagon.sh build",
       label: 'Create Hexagon cmake config',
     )
-    make(ci_hexagon, 'build', '-j2')
+    cmake_build(ci_hexagon, 'build', '-j2')
+    make_standalone_crt(ci_hexagon, 'build')

Review Comment:
   We would want to remove make_standalone_crt() here for the hexagon build.
   
   cc: @mehrdadh 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] driazati commented on a diff in pull request #13335: [ci] Split out C++ unittests

Posted by GitBox <gi...@apache.org>.
driazati commented on code in PR #13335:
URL: https://github.com/apache/tvm/pull/13335#discussion_r1023416998


##########
ci/jenkins/Test.groovy.j2:
##########
@@ -14,11 +21,19 @@
   test_method_names=test_method_names,
 ) %}
   {% if shard_index == 1 %}
-  {{ m.download_artifacts(tag='gpu2', filenames=tvm_multilib) }}
-  cpp_unittest(ci_gpu)
+  {{ m.download_artifacts(tag='gpu2', filenames=tvm_multilib + crttest + standalone_crt) }}
+  sh "${docker_run} --no-gpu ${ci_gpu} ./tests/scripts/task_config_build_gpu_other.sh build"
+  // These require a GPU to finish the build (i.e. CUDA needs to be load-able)
+  make_standalone_crt(ci_gpu, 'build')
+  // make_cpp_tests(ci_gpu, 'build')
+  // cpp_unittest(ci_gpu)

Review Comment:
   These aren't run at all right now and are currently broken, so this leaves them commented out. The previous code uploaded the same binaries for both `gpu` and `gpu2` since there was a fixed reference to the `build` directory. This resolves the issue which causes this to fail but doesn't fix the build failure itself, which we should leave for a follow up



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on a diff in pull request #13335: [ci] Split out C++ unittests

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #13335:
URL: https://github.com/apache/tvm/pull/13335#discussion_r1023422856


##########
tests/scripts/task_cpp_unittest.sh:
##########
@@ -28,38 +28,15 @@ else
     BUILD_DIR=build
 fi
 
-# Python is required by apps/bundle_deploy
-source tests/scripts/setup-pytest-env.sh
 
-export LD_LIBRARY_PATH="lib:${LD_LIBRARY_PATH:-}"
 # NOTE: important to use abspath, when VTA is enabled.
-export VTA_HW_PATH=`pwd`/3rdparty/vta-hw
+VTA_HW_PATH=$(pwd)/3rdparty/vta-hw
+export VTA_HW_PATH
 
 # to avoid CI thread throttling.
 export TVM_BIND_THREADS=0
 export OMP_NUM_THREADS=1
 
-# Build cpptest suite
-python3 tests/scripts/task_build.py \
-    --sccache-bucket tvm-sccache-prod \
-    --cmake-target cpptest \
-    --build-dir "${BUILD_DIR}"
-
-# crttest requries USE_MICRO to be enabled.
-if grep -Fq "USE_MICRO ON" ${BUILD_DIR}/TVMBuildOptions.txt; then
-  pushd "${BUILD_DIR}"
-  ninja crttest
-  popd
-fi
-
 pushd "${BUILD_DIR}"

Review Comment:
   I'm not sure what happened here. shouldn't `ctest` also move to wherever `crttest` was built?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] driazati commented on a diff in pull request #13335: [ci] Split out C++ unittests

Posted by GitBox <gi...@apache.org>.
driazati commented on code in PR #13335:
URL: https://github.com/apache/tvm/pull/13335#discussion_r1023571421


##########
tests/scripts/task_cpp_unittest.sh:
##########
@@ -28,38 +28,15 @@ else
     BUILD_DIR=build
 fi
 
-# Python is required by apps/bundle_deploy
-source tests/scripts/setup-pytest-env.sh
 
-export LD_LIBRARY_PATH="lib:${LD_LIBRARY_PATH:-}"
 # NOTE: important to use abspath, when VTA is enabled.
-export VTA_HW_PATH=`pwd`/3rdparty/vta-hw
+VTA_HW_PATH=$(pwd)/3rdparty/vta-hw
+export VTA_HW_PATH
 
 # to avoid CI thread throttling.
 export TVM_BIND_THREADS=0
 export OMP_NUM_THREADS=1
 
-# Build cpptest suite
-python3 tests/scripts/task_build.py \
-    --sccache-bucket tvm-sccache-prod \
-    --cmake-target cpptest \
-    --build-dir "${BUILD_DIR}"
-
-# crttest requries USE_MICRO to be enabled.
-if grep -Fq "USE_MICRO ON" ${BUILD_DIR}/TVMBuildOptions.txt; then
-  pushd "${BUILD_DIR}"
-  ninja crttest
-  popd
-fi
-
 pushd "${BUILD_DIR}"

Review Comment:
   The build stuff should all be moved into the corresponding build stage and downloaded in the relevant test step to actually be run, so this removes all the building and just runs ctest (the other stuff behind the flags is also moved to the other test script below)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] tvm-bot commented on pull request #13335: [ci] Split out C++ unittests

Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13335:
URL: https://github.com/apache/tvm/pull/13335#issuecomment-1309353850

   <!---bot-comment-->
   
   Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
   
   
   
   <sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] driazati commented on pull request #13335: [ci] Split out C++ unittests

Posted by GitBox <gi...@apache.org>.
driazati commented on PR #13335:
URL: https://github.com/apache/tvm/pull/13335#issuecomment-1317474198

   Certain GitHub forks are configured in Jenkins to be trusted (e.g. those for people on the oss team), so the extra step to push to apache/tvm isn't necessary. You can see the info here: https://ci.tlcpack.ai/job/tvm/indexing/events, when a job comes from an untrusted source it'll say something like
   
   ```
   Checking pull request [#13335](https://github.com/apache/tvm/pull/13335)
       (not from a trusted source)
         ‘Jenkinsfile’ found
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on pull request #13335: [ci] Split out C++ unittests

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #13335:
URL: https://github.com/apache/tvm/pull/13335#issuecomment-1317335521

   @driazati can you push your branch to a TVM branch to test the jenkins changes?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh merged pull request #13335: [ci] Split out C++ unittests

Posted by GitBox <gi...@apache.org>.
mehrdadh merged PR #13335:
URL: https://github.com/apache/tvm/pull/13335


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org