[CI][Contrib] Add Vitis-AI docker installation

[anilmartha]

This PR adds the Vitis-AI PyXIR dependency for testing Vitis-AI codegen to the ci_cpu dockerfile.

[leandron]

nit:as you have it already defined, maybe mkdir "$PYXIR_HOME" is better.

[leandron]

suggestion: maybe ubuntu_install_vitis_ai.sh would be more descriptive?

[jtuyls]

@leandron Yes, we will make it more descriptive. However, in the main Vitis-AI integration PR we add another script so it wouldn't be too clear if we call this ubuntu_install_vitis_ai.sh. Also, as this script is only used for CI I think ubuntu_install_vitis_ai_packages_ci.sh would be better. What do you think?

[comaniac]

Hi could you explain the purpose of docker related files/scripts in the main integration PR? Ideally we should separate all environment related changes to this PR so that we could see the whole picture.

[anilmartha]

@comaniac The purpose of docker related files/scripts in the main integration PR is to build the docker image for Vitis-AI codegen infrastructure using Dockerfile.ci_vai and launching the docker container with necessary drivers if required. Should we add those files in this PR itself?

[leandron]

Also, as this script is only used for CI I think ubuntu_install_vitis_ai_packages_ci.sh would be better. What do you think?

I had a look on your other PR now, and I understand what you said about the other script. I think it would be beneficial to have all the CI change in one patch, if possible.

[jtuyls]

@leandron I am not sure about the clarity of the demo prefix either. Maybe we can just add it as Dockerfile.vitisai? @tqchen ?

And ok, we will move all environment changes to this PR.

[comaniac]

I agree with Leandro basically. In TVM we have two types of docker files: CI and Demo. Demo is used to provide an environment for users to quickly try out basic TVM functions. However, TVM does not provide the docker file for developments other than CI environment.

If you would like to provide an environment for TVM users that want to use Vitis-AI, the currently solution is either providing installation instructions in the tutorial or document like ACL does to help users set up the environment, or make it as a Demo image and point users to there.

Of course, ideally it would be better for TVM to have a set of docker files for developments, but it is hard to achieve for some backends due to the license issue.

cc @tqchen for more comments.

[anilmartha]

@comaniac @leandron
I have added Dockerfile.demo_vitis_ai and corresponding environment related scripts. Could you please review those?

[tmoreau89]

LGTM @leandron lemme know if you're happy with the outcome of the file renaming.

[leandron]

Thanks @tmoreau89 - yes, it looks more integrated with existing Dockerfiles now. LGTM

[zhiics]

cc @tqchen @tmoreau89

[comaniac]

Agree the comments from @leandron. Overall LGTM.

[tmoreau89]

Overall LGTM, I suggest addressing some of the comments made by @leandron

[tqchen]

This seems to be too xilinx specific. Is it possible to do it in another way ousdier docker/bash? e.g. like the runtime case in nvidia

[jtuyls]

The check is indeed Xilinx specific but there are also NVIDIA and GPU specific checks (https://github.com/apache/incubator-tvm/blob/d4bfcd4c4f8861f6716406ebae391f9936434c4e/docker/bash.sh#L54) in the bash.sh so I don't see how this differs. And this is the usual way in which we setup the docker environment with Vitis-AI. Also, the change shouldn't affect the other docker environments.

[tqchen]

OK. Please add some comments on top of the if section to describe the set of env being setup. We will need to think about cleanly organize the code as we start to add support to more devices, this could be a good starting pt.

Thanks @jtuyls

[leandron]

One option - out of scope for this specific patch: have a 1:1 function that will do "all that is special about dealing with this specific image."

In this case, bash.sh will still be self contained, but we know that some logic (as a function) is only executed if the image requires it.

enable_other_custom_image()
{
  # do all that is requested to enable this image
}

enable_vitis_ai()
{
  # do all that is requested to enable this image
}

# we only add entries here if there is something special to do about a specific image
case "${DOCKER_IMAGE_NAME}" in
  "demo_vitis_ai")
    enable_vitis_ai()
    ;;
  "other_custom_image")
    enable_other_custom_image()
    ;;
  ... 
esac 

[jtuyls]

I like the proposal of @leandron. Would improve clarity in the bash.sh for multiple devices. We could additionally standardize on variables like $VITIS_AI_FLAGS, etc to be edited in the function and to be added to the docker run command.

@leandron You specifically want to leave this out of this PR as we could easily already write our Vitis-AI changes in this format?

[leandron]

@jtuyls I feel it needs a bit more discussion so I wouldn't block this PR merge for that. Despite feeling the idea is appropriate, I think we need to collect feedback in a specific PR.

[tqchen]

Approve and i think we can merge once the comment is addressed, cc @tmoreau89 to manage this pr

[tmoreau89]

Thanks @anilmartha @jtuyls @comaniac @leandron @tqchen the PR has been merged. @leandron and @jtuyls can work together on follow-up improvements as discussed.