Skip to content

[TVM EP] Integrate tests for TVM EP into public onnxruntime CI - #10505

Merged
Xavier Dupré (xadupre) merged 21 commits into
microsoft:masterfrom
octoml:agladyshev/dev/ci/integration
Feb 24, 2022
Merged

Xavier Dupré (xadupre) merged 21 commits into
microsoft:masterfrom
octoml:agladyshev/dev/ci/integration

Conversation

@KJlaccHoeUM9l

@KJlaccHoeUM9l Alexey Gladyshev (KJlaccHoeUM9l) commented Feb 9, 2022

Copy link
Copy Markdown
Contributor

This PR is waiting for PR#10260 to be merged.

TVM EP is not currently being tested in public CI. Because of this, any changes made by third party contributors could potentially result in the TVM EP not working. To avoid this we have created this PR which is to add testing of TVM EP in public CI.

Notes:

  1. Added support for bool types. A similar input tensor is used in this test;
  2. Cleared post build actions for TVM EP;
  3. Moved _ld_preload.py conditional extension from setup.py to CMake level;
  4. Added support for TVM EP to run onnxruntime_test_python.py with build.py;
  5. Added Dockerfile and workflow config to integrate TVM EP tests into azure pipeline;
  6. Replaced TVM logging with ONNX RT logging. Additionally, implemented custom logger for TVM.

If there are things that were not taken into account for integrating TVM EP into public CI, please let us know.

@KJlaccHoeUM9l Alexey Gladyshev (KJlaccHoeUM9l) changed the title [TVM EP][WIP] Integrate tests for TVM EP into public onnxruntime CI [TVM EP] Integrate tests for TVM EP into public onnxruntime CI Feb 10, 2022
Comment thread tools/ci_build/github/linux/docker/Dockerfile.ubuntu_tvm Outdated
@KJlaccHoeUM9l

Copy link
Copy Markdown
Contributor Author

Hello Edward Chen (@edgchen1)!
PR#10260 has been merged. I rebase to the current master so this PR can be reviewed.
Could you recheck our updates?

@tmoreau89

Copy link
Copy Markdown

Thank you Alexey Gladyshev (@KJlaccHoeUM9l) ! CC Xavier Dupré (@xadupre) - FYI; this PR adds CI for TVM EP; it's ready for review.

Comment thread onnxruntime/core/providers/tvm/custom_logging.cc Outdated
Comment thread cmake/CMakeLists.txt Outdated
endif()

add_definitions(-DTVM_LOG_CUSTOMIZE=1)
add_library(custom_logger STATIC ${ONNXRUNTIME_ROOT}/core/providers/tvm/custom_logging.cc)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can it be called tvm_custom_logger instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread cmake/CMakeLists.txt Outdated
set(USE_CUDA ON CACHE BOOL "Only defined for TVM" FORCE)
endif()

add_definitions(-DTVM_LOG_CUSTOMIZE=1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
add_definitions(-DTVM_LOG_CUSTOMIZE=1)
add_compile_definitions(TVM_LOG_CUSTOMIZE=1)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

displayName: 'Install python deps'
inputs:
script: |
python3 -m pip install decorator scipy

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what are these used by? I only see a docker run later that does anything substantial. these dependencies won't be in the actual docker image right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

see comment below

@@ -0,0 +1,62 @@
jobs:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

has this pipeline been tested out yet?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

see comment below

@edgchen1

Copy link
Copy Markdown
Contributor

Created a build definition on our end to test this out.
https://dev.azure.com/onnxruntime/onnxruntime/_build/results?buildId=585915&view=results
Will need to use an existing agent pool.

FYI, I created a branch that mirrors this one that we can use to test the builds. I don't plan to update it outside of additional merges from this branch.
https://github.com/microsoft/onnxruntime/tree/edgchen1/tvm_ep_ci

@KJlaccHoeUM9l

Copy link
Copy Markdown
Contributor Author

Edward Chen (@edgchen1), about using Dockerfile for TVM and azure pipeline.
Yes, you are absolutely right. The Python dependencies will be installed before the docker image is launched. Therefore, the dependencies will not be installed there.
Also you are right that this azure pipeline has not been tested yet. This testing involves running this pipeline in onnxruntime CI. Therefore, this is a "black box" for us, we cannot check the behavior locally.
In view of this, I reset the commit in which I deleted the Dockerfile.ubuntu_tvm. It seems that other EPs also install Python dependencies inside their Dockerfiles. Additionally, now the initial integration of TVM EP into CI. We use already existing tests. And to run them, we need to additionally deliver only two dependencies. However, in the future, when expanding the test suite, the list of these dependencies may grow, so the specified Dockerfile will grow and the difference will not be just one line.

Alexey Gladyshev (@KJlaccHoeUM9l) Regarding the separate Dockerfile, there is some infrastructure that caches CI build images in a container registry when we use get-docker-image-steps.yml. If the image is detected to be different (based on things like content of Dockerfile, Docker context directory), a new image is stored. This takes up additional space in the container registry. So if possible (if installing the dependencies at runtime is feasible), I would prefer to reuse the existing Dockerfile. If this changes in the future, we can switch to a separate Dockerfile then.

Something like this might work, if installation can be done without admin privileges:

docker run ... \
  onnxruntimecpubuild \
    /bin/bash -c \
      "install_tvm_test_dependencies.sh && \
      /opt/python/cp38-cp38/bin/python3 /onnxruntime_src/tools/ci_build/build.py ..."

Edward Chen (@edgchen1), thank you for the explanation! Resolved.

@KJlaccHoeUM9l

Copy link
Copy Markdown
Contributor Author

/azp run Linux TVM CI Pipeline

@azure-pipelines

Copy link
Copy Markdown
Commenter does not have sufficient privileges for PR 10505 in repo microsoft/onnxruntime

@KJlaccHoeUM9l

Copy link
Copy Markdown
Contributor Author

Edward Chen (@edgchen1), could you restart the pipeline?

@xadupre

Copy link
Copy Markdown
Member

/azp run Linux TVM CI Pipeline

@azure-pipelines

Copy link
Copy Markdown
No pipelines are associated with this pull request.

@xadupre

Copy link
Copy Markdown
Member

/azp Linux CPU CI Pipeline

@azure-pipelines

Copy link
Copy Markdown
Command 'Linux' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@xadupre

Copy link
Copy Markdown
Member

/azp run Linux CPU CI Pipeline

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@edgchen1

Copy link
Copy Markdown
Contributor

Move install_tvm_test_dependencies.sh out of Docker context dir, update build definition.
@KJlaccHoeUM9l

Copy link
Copy Markdown
Contributor Author

Hello Edward Chen (@edgchen1)!
Thank you very much for your help! I have merged your changes. It seems that now we can run all pipelines?

@KJlaccHoeUM9l

Copy link
Copy Markdown
Contributor Author

Hello Xavier Dupré (@xadupre)!
Could you run CI pipelines?

@xadupre

Copy link
Copy Markdown
Member

/azp run Linux CPU CI Pipeline

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@xadupre

Copy link
Copy Markdown
Member

/azp run Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux Nuphar CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 8 pipeline(s).

@xadupre

Copy link
Copy Markdown
Member

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows WebAssembly CI Pipeline, orttraining-amd-gpu-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, onnxruntime-python-checks-ci-pipeline

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 8 pipeline(s).

@edgchen1

Copy link
Copy Markdown
Contributor

/azp run Linux TVM CI Pipeline

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@edgchen1 Edward Chen (edgchen1) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CI pipeline related changes look good to me

@KJlaccHoeUM9l

Copy link
Copy Markdown
Contributor Author

Hello Xavier Dupré (@xadupre)!
Looks like we can merge this PR?

@xadupre
Xavier Dupré (xadupre) merged commit 7dc7529 into microsoft:master Feb 24, 2022
Peter Salas (petersalas) pushed a commit to octoml/onnxruntime that referenced this pull request Nov 7, 2022
…soft#10505)

* add support for bool type

* add TVM EP support for tests

* include TVM EP in python test pool

* fix pylint

* moved technical imports to a separate file

* clean up post build actions & move _ld_preload.py extension to CMake level

* add files for include TVM EP into CI

* implement custom logger for TVM

* replace TVM logging with ONNX RT logging

* update link for TVM EP tutorial

* clean up TVM EP cmake

* add pybind auto enabling for TVM EP

* fix blank spaces

* code review fixes

* replace print with comment

* add list of EP without TVM EP

* enable onnx tests

* disable contrib ops and ml ops

* reuse Dockerfile.ubuntu

* Move install_tvm_test_dependencies.sh out of Docker context dir, update build definition.

Co-authored-by: Edward Chen <18449977+edgchen1@users.noreply.github.com>
(cherry picked from commit 7dc7529)
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.

5 participants