[GitHub] [incubator-tvm] jtuyls commented on a change in pull request #6342: [CI][Contrib] Add Vitis-AI docker installation
jtuyls commented on a change in pull request #6342:
URL: https://github.com/apache/incubator-tvm/pull/6342#discussion_r481052359
##
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:
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?
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] jtuyls commented on a change in pull request #6342: [CI][Contrib] Add Vitis-AI docker installation
jtuyls commented on a change in pull request #6342:
URL: https://github.com/apache/incubator-tvm/pull/6342#discussion_r480217616
##
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:
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.
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] jtuyls commented on a change in pull request #6342: [CI][Contrib] Add Vitis-AI docker installation
jtuyls commented on a change in pull request #6342: URL: https://github.com/apache/incubator-tvm/pull/6342#discussion_r478304888 ## 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: @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. 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] jtuyls commented on a change in pull request #6342: [CI][Contrib] Add Vitis-AI docker installation
jtuyls commented on a change in pull request #6342: URL: https://github.com/apache/incubator-tvm/pull/6342#discussion_r478283171 ## 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: @leandron The ci_vai docker we added in the other PR is actually not meant to be used for CI at the moment. It's the main docker we use in our flow as you can see in our documentation: https://github.com/apache/incubator-tvm/blob/8903b1a3251370ee1013fc2f9f3ef6004fa0e4b2/docs/deploy/vitis_ai.rst and builds on top of the general Vitis-AI docker (https://github.com/Xilinx/Vitis-AI) containing the necessary Vitis-AI tools for quantization, compilation, etc. Two questions: - 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? - Should we add it to this PR even though it's not meant to be used for CI? 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] jtuyls commented on a change in pull request #6342: [CI][Contrib] Add Vitis-AI docker installation
jtuyls commented on a change in pull request #6342: URL: https://github.com/apache/incubator-tvm/pull/6342#discussion_r478283171 ## 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: @leandron The ci_vai docker we added in the other PR is actually not meant to be used for CI at the moment. It's the main docker we use in our flow as you can see in our documentation: https://github.com/apache/incubator-tvm/blob/8903b1a3251370ee1013fc2f9f3ef6004fa0e4b2/docs/deploy/vitis_ai.rst and builds on top of the general Vitis-AI docker (https://github.com/Xilinx/Vitis-AI) containing the necessary Vitis-AI tools for quantization, compilation, etc. Two questions: - 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? - Should we add it to this PR even though it's meant to be used for CI? 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] jtuyls commented on a change in pull request #6342: [CI][Contrib] Add Vitis-AI docker installation
jtuyls commented on a change in pull request #6342: URL: https://github.com/apache/incubator-tvm/pull/6342#discussion_r477572385 ## 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: @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? 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
