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

Build and test TVM under minimal configuration #12178

Merged
merged 4 commits into from
Aug 11, 2022

Conversation

gigiblender
Copy link
Contributor

@gigiblender gigiblender commented Jul 25, 2022

This PR builds and tests TVM (running the CPP and unittests) under minimal configuration with some debug flags enabled:

  • USE_RELAY_DEBUG=ON in TVM
  • -Wp,-D_GLIBCXX_ASSERTIONS in TVM
  • -DLLVM_ENABLE_ASSERTIONS=ON in LLVM

It also adds this configuration to the CI.

tests/python/unittest/test_meta_schedule_task_scheduler.py::test_meta_schedule_task_scheduler_multiple_gradient_based results in an array OOB access and a segfault due to D_GLIBCXX_ASSERTIONS. I disable this test for now and will open an issue to solve it ASAP.

It should fix #11932 and address this discussion.

@gigiblender gigiblender marked this pull request as ready for review July 25, 2022 19:28
@gigiblender
Copy link
Contributor Author

@areusch @driazati

Copy link
Member

@driazati driazati 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 tackling this! It's been on the issue list for quiet a while, things mostly lgtm. Can you also follow up to add an ECR repo for ci_minimal as well? https://github.com/tlc-pack/ci/blob/main/terraform/vars/tvm-ci-prod.auto.tfvars#L79

@@ -199,6 +234,9 @@ stage('Test') {
{{ method_name }}()
},
{% endfor %}
'unittest: CPU MINIMAL': {
Copy link
Member

Choose a reason for hiding this comment

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

can you use the m.test_step macro here instead? That way we don't need to duplicate all the meta-logic (timeout, workspace, etc) just for this test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to do that. The test_step inlines the body while in this case, I am calling run_unittest_minimal. I could try to inline the method and use the macro, but that might not work due to the 64KB bytecode size/method limit in the JVM.

I also guess I could modify the macros.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I am adding a new macro

Copy link
Member

Choose a reason for hiding this comment

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

I meant something like https://github.com/apache/tvm/blob/main/ci/jenkins/Test.groovy.j2#L202-L218, if you do that it will automatically call the test method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I needed a different macro that did not include the step name (eg unittest: CPU MINIMAL, frontend: aarch64 2 of 2, unittest: CPU).

https://github.com/apache/tvm/pull/12178/files#diff-1a31fe4681520821e63f0bec0a963c86b8eea7af2c7130b28eba0b987a5fc21bR86

ci/jenkins/Test.groovy.j2 Outdated Show resolved Hide resolved
tests/python/unittest/test_meta_schedule_task_scheduler.py Outdated Show resolved Hide resolved
tests/scripts/task_config_build_minimal.sh Show resolved Hide resolved
@areusch
Copy link
Contributor

areusch commented Jul 26, 2022

tests/scripts/task_config_build_minimal.sh Show resolved Hide resolved
ninja crttest
popd
# crttest requires USE_MICRO to be enabled.
if grep -Fq "set(USE_MICRO ON)" ${BUILD_DIR}/config.cmake; then
Copy link
Contributor

Choose a reason for hiding this comment

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

i really wanted to be able to have you just look this up in CMakeCache.txt, but unfortunately we don't cache these. i wonder if we should be setting this with set(USE_ABC ___ CACHED) so we can inspect that. what if we modified Summary.cmake to write the options to a file via https://cmake.org/cmake/help/latest/command/file.html#append?

Copy link
Member

Choose a reason for hiding this comment

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

it might also be cleaner to split this out into task_cpp_unittest_micro.sh or something and just add it as another step everywhere that currently runs the cpp unit tests, that way its easy to run regardless of the specific build options

Copy link
Contributor Author

@gigiblender gigiblender Jul 27, 2022

Choose a reason for hiding this comment

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

I went the route of separating the script into two. Let me know your thoughts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: It seems that there are crt tests that get included in the cpp test suite when USE_MICRO is enabled.

Instead of separating those, I am following @areusch advice and writing to a file all the options used to build TVM.

driazati pushed a commit to tlc-pack/ci that referenced this pull request Jul 26, 2022
Adds an ECR repo for `ci_minimal`

Required for apache/tvm#12178
ci/jenkins/Test.groovy.j2 Show resolved Hide resolved
tests/scripts/ci.py Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Built docs for commit ea3c93e can be found here.

Copy link
Member

@driazati driazati left a comment

Choose a reason for hiding this comment

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

lgtm, waiting for approve from @areusch too before merging

tests/python/unittest/test_meta_schedule_task_scheduler.py Outdated Show resolved Hide resolved
@gigiblender gigiblender force-pushed the tvm-minimal branch 2 times, most recently from a122907 to 4911c17 Compare August 2, 2022 11:55
@areusch
Copy link
Contributor

areusch commented Aug 2, 2022

@gigiblender looks like just need to resolve the merge conflict

@gigiblender gigiblender force-pushed the tvm-minimal branch 5 times, most recently from aa0afb9 to a8f5af1 Compare August 8, 2022 19:17
@driazati
Copy link
Member

@tvm-bot merge

@driazati
Copy link
Member

@tvm-bot merge

@driazati driazati merged commit 52152e0 into apache:main Aug 11, 2022
driazati added a commit to driazati/tvm that referenced this pull request Aug 11, 2022
This got out of date after merging apache#12178
areusch pushed a commit that referenced this pull request Aug 11, 2022
This got out of date after merging #12178

Co-authored-by: driazati <driazati@users.noreply.github.com>
driazati added a commit to driazati/tvm that referenced this pull request Aug 11, 2022
This got out of date after merging apache#12178

Co-authored-by: driazati <driazati@users.noreply.github.com>
driazati added a commit to driazati/tvm that referenced this pull request Aug 11, 2022
This got out of date after merging apache#12178

Co-authored-by: driazati <driazati@users.noreply.github.com>
driazati added a commit to driazati/tvm that referenced this pull request Aug 30, 2022
This got out of date after merging apache#12178

Co-authored-by: driazati <driazati@users.noreply.github.com>
areusch pushed a commit that referenced this pull request Aug 31, 2022
* [skip ci][ci] Fix Jenkinsfile (#12387)

This got out of date after merging #12178

Co-authored-by: driazati <driazati@users.noreply.github.com>

* Address comments

Co-authored-by: driazati <driazati@users.noreply.github.com>
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
This PR builds and tests TVM (running the CPP and unittests) under minimal configuration with some debug flags enabled:
- `USE_RELAY_DEBUG=ON` in TVM
- `-Wp,-D_GLIBCXX_ASSERTIONS` in TVM
- `-DLLVM_ENABLE_ASSERTIONS=ON` in LLVM

It also adds this configuration to the CI. 

`tests/python/unittest/test_meta_schedule_task_scheduler.py::test_meta_schedule_task_scheduler_multiple_gradient_based` results in an array OOB access and a segfault due to `D_GLIBCXX_ASSERTIONS`. I disable this test for now and will open an issue to solve it ASAP.

It should fix apache#11932 and address [this discussion](https://discuss.tvm.apache.org/t/pre-rfc-new-ci-container-ci-cpu-asserts/12536/9).
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
This got out of date after merging apache#12178

Co-authored-by: driazati <driazati@users.noreply.github.com>
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
* [skip ci][ci] Fix Jenkinsfile (apache#12387)

This got out of date after merging apache#12178

Co-authored-by: driazati <driazati@users.noreply.github.com>

* Address comments

Co-authored-by: driazati <driazati@users.noreply.github.com>
mikeseven pushed a commit to mikeseven/tvm that referenced this pull request Sep 27, 2023
This PR builds and tests TVM (running the CPP and unittests) under minimal configuration with some debug flags enabled:
- `USE_RELAY_DEBUG=ON` in TVM
- `-Wp,-D_GLIBCXX_ASSERTIONS` in TVM
- `-DLLVM_ENABLE_ASSERTIONS=ON` in LLVM

It also adds this configuration to the CI. 

`tests/python/unittest/test_meta_schedule_task_scheduler.py::test_meta_schedule_task_scheduler_multiple_gradient_based` results in an array OOB access and a segfault due to `D_GLIBCXX_ASSERTIONS`. I disable this test for now and will open an issue to solve it ASAP.

It should fix apache#11932 and address [this discussion](https://discuss.tvm.apache.org/t/pre-rfc-new-ci-container-ci-cpu-asserts/12536/9).
mikeseven pushed a commit to mikeseven/tvm that referenced this pull request Sep 27, 2023
This got out of date after merging apache#12178

Co-authored-by: driazati <driazati@users.noreply.github.com>
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.

Exercise TVM under minimal configuration in CI
4 participants