[GitHub] [incubator-tvm] leandron commented on a change in pull request #6342: [CI][Contrib] Add Vitis-AI docker installation
leandron commented on a change in pull request #6342:
URL: https://github.com/apache/incubator-tvm/pull/6342#discussion_r481134889
##
File path: docker/bash.sh
##
@@ -75,6 +75,27 @@ else
CI_PY_ENV=""
fi
+if [[ "${DOCKER_IMAGE_NAME}" == *"demo_vitis_ai"* && -d "/dev/shm" && -d
"/opt/xilinx/dsa" && -d "/opt/xilinx/overlaybins" ]]; then
+WORKSPACE_VOLUMES="-v /dev/shm:/dev/shm -v /opt/xilinx/dsa:/opt/xilinx/dsa
-v /opt/xilinx/overlaybins:/opt/xilinx/overlaybins"
Review comment:
@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.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-tvm] leandron commented on a change in pull request #6342: [CI][Contrib] Add Vitis-AI docker installation
leandron commented on a change in pull request #6342:
URL: https://github.com/apache/incubator-tvm/pull/6342#discussion_r480982502
##
File path: docker/bash.sh
##
@@ -75,6 +75,27 @@ else
CI_PY_ENV=""
fi
+if [[ "${DOCKER_IMAGE_NAME}" == *"demo_vitis_ai"* && -d "/dev/shm" && -d
"/opt/xilinx/dsa" && -d "/opt/xilinx/overlaybins" ]]; then
+WORKSPACE_VOLUMES="-v /dev/shm:/dev/shm -v /opt/xilinx/dsa:/opt/xilinx/dsa
-v /opt/xilinx/overlaybins:/opt/xilinx/overlaybins"
Review comment:
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
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-tvm] leandron commented on a change in pull request #6342: [CI][Contrib] Add Vitis-AI docker installation
leandron commented on a change in pull request #6342: URL: https://github.com/apache/incubator-tvm/pull/6342#discussion_r480959853 ## File path: docker/Dockerfile.ci_cpu ## @@ -83,3 +83,7 @@ RUN bash /install/ubuntu_install_caffe.sh # Github Arm(R) Ethos(TM)-N NPU driver COPY install/ubuntu_install_ethosn_driver_stack.sh /install/ubuntu_install_ethosn_driver_stack.sh RUN bash /install/ubuntu_install_ethosn_driver_stack.sh + +# Vitis-AI PyXIR CI deps +COPY install/ubuntu_install_vai_packages.sh /install/ubuntu_install_vai_packages.sh +RUN bash /install/ubuntu_install_vai_packages.sh Review comment: Thanks @tmoreau89 - yes, it looks more integrated with existing Dockerfiles now. LGTM 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [incubator-tvm] leandron commented on a change in pull request #6342: [CI][Contrib] Add Vitis-AI docker installation
leandron commented on a change in pull request #6342: URL: https://github.com/apache/incubator-tvm/pull/6342#discussion_r478288536 ## File path: docker/Dockerfile.ci_cpu ## @@ -83,3 +83,7 @@ RUN bash /install/ubuntu_install_caffe.sh # Github Arm(R) Ethos(TM)-N NPU driver COPY install/ubuntu_install_ethosn_driver_stack.sh /install/ubuntu_install_ethosn_driver_stack.sh RUN bash /install/ubuntu_install_ethosn_driver_stack.sh + +# Vitis-AI PyXIR CI deps +COPY install/ubuntu_install_vai_packages.sh /install/ubuntu_install_vai_packages.sh +RUN bash /install/ubuntu_install_vai_packages.sh Review comment: > Should we rename the ci_vai to make it more clear that it's our main docker environment and it's not really for CI at the moment? I think it would be good to name it in a way that is obvious to people on what that Dockerfile is intended for. Looking at existing Dockerfiles (https://github.com/apache/incubator-tvm/tree/master/docker) I see there are three prefixes: `ci_*`, `conda_*` and `demo_`. Maybe from the repository point of view it fits more for a `demo` than `ci`? I'm not very sure about what the right answer here, maybe @tqchen will know better. > Should we add it to this PR even though it's meant to be used for CI? Even not being strictly related to CI, as @comaniac pointed out, it is related to environment changes (for example the change/fix on the `ubuntu_install_python.sh` that might affect other Dockerfiles). So I think it would be good to have them all in one PR. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [incubator-tvm] leandron commented on a change in pull request #6342: [CI][Contrib] Add Vitis-AI docker installation
leandron commented on a change in pull request #6342: URL: https://github.com/apache/incubator-tvm/pull/6342#discussion_r478273962 ## File path: docker/Dockerfile.ci_cpu ## @@ -83,3 +83,7 @@ RUN bash /install/ubuntu_install_caffe.sh # Github Arm(R) Ethos(TM)-N NPU driver COPY install/ubuntu_install_ethosn_driver_stack.sh /install/ubuntu_install_ethosn_driver_stack.sh RUN bash /install/ubuntu_install_ethosn_driver_stack.sh + +# Vitis-AI PyXIR CI deps +COPY install/ubuntu_install_vai_packages.sh /install/ubuntu_install_vai_packages.sh +RUN bash /install/ubuntu_install_vai_packages.sh Review comment: > 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. ## File path: docker/Dockerfile.ci_cpu ## @@ -83,3 +83,7 @@ RUN bash /install/ubuntu_install_caffe.sh # Github Arm(R) Ethos(TM)-N NPU driver COPY install/ubuntu_install_ethosn_driver_stack.sh /install/ubuntu_install_ethosn_driver_stack.sh RUN bash /install/ubuntu_install_ethosn_driver_stack.sh + +# Vitis-AI PyXIR CI deps +COPY install/ubuntu_install_vai_packages.sh /install/ubuntu_install_vai_packages.sh +RUN bash /install/ubuntu_install_vai_packages.sh Review comment: > 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. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [incubator-tvm] leandron commented on a change in pull request #6342: [CI][Contrib] Add Vitis-AI docker installation
leandron commented on a change in pull request #6342: URL: https://github.com/apache/incubator-tvm/pull/6342#discussion_r477270729 ## File path: docker/install/ubuntu_install_vai_packages.sh ## @@ -0,0 +1,29 @@ +#!/bin/bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +set -e +set -u +set -o pipefail + +export PYXIR_HOME=/opt/pyxir +mkdir /opt/pyxir Review comment: nit:as you have it already defined, maybe `mkdir "$PYXIR_HOME"` is better. ## File path: docker/Dockerfile.ci_cpu ## @@ -83,3 +83,7 @@ RUN bash /install/ubuntu_install_caffe.sh # Github Arm(R) Ethos(TM)-N NPU driver COPY install/ubuntu_install_ethosn_driver_stack.sh /install/ubuntu_install_ethosn_driver_stack.sh RUN bash /install/ubuntu_install_ethosn_driver_stack.sh + +# Vitis-AI PyXIR CI deps +COPY install/ubuntu_install_vai_packages.sh /install/ubuntu_install_vai_packages.sh +RUN bash /install/ubuntu_install_vai_packages.sh Review comment: suggestion: maybe `ubuntu_install_vitis_ai.sh` would be more descriptive? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
