You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/06/21 22:35:34 UTC

[GitHub] [tvm] mehrdadh opened a new pull request, #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

mehrdadh opened a new pull request, #11809:
URL: https://github.com/apache/tvm/pull/11809

   This would prevent rebuilding multiple image on each update on CMSIS, ethosu, etc.
   cc @areusch @Mousius @leandron


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] ashutosh-arm commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
ashutosh-arm commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1193780818

   > Thanks! looking good.
   > 
   > It would be great to capture the steps discussed and agreed in the issue (as per @areusch) and link to the PR.
   > 
   > @ashutosh-arm could you take a look as well from cmsis tests, whether they would be all run in ci_qemu now ?
   
   afaik CMSIS-NN tests do not run on ci_qemu at present. In this code change, I don't see CMSIS-NN tests moved to tests/scripts/task_python_microtvm.sh. I am referring to https://github.com/apache/tvm/blob/6eb3a1fc3688bede69efd7f14b313b35497ccf02/tests/scripts/task_python_integration.sh#L64. Am I missing something @mehrdadh / @manupa-arm ?
   
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1206634013

   @manupa-arm friendly reminder about this PR. thanks!


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] manupa-arm commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
manupa-arm commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1194315234

   @mehrdadh test_ethosn (unlike test_ethosu) is not related to microTVM.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] manupa-arm commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
manupa-arm commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1168451827

   cc : @leandron 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1203151499

   just rebased with main, timeout issue should be resolved.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] manupa-arm commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
manupa-arm commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1172185176

   Thanks all for the discussion!.
   
   > we can avoid dependency drift by being careful in reviewing Docker image changes.
   
   If we are solely relying on the reviewers, we need would need better written guidance in an OSS project to aid both reviewers (to cite) and authors (to follow), on what could be added to : ci_base (if we are taking this approach), ci_micro and ci_cpu. 
   NOTE : Im scoping this discussion to the specific issue being discussed here.
   
   > but even with that people might add any script to any of the docker files. So as a reviewer we need to keep an eye on changes on those files.
   
   I suppose how ci_base would help here is that we can make the above guidance to be simplified to say all the layers in the image added by ci_micro Dockerfile has to be micro specific.
   
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1167973847

   Adding to Andrew's point, I think we can remove `USE_MICRO` since it's almost always ON. Also, I prefer to keep ci_qemu separate and also call it ci_microtvm. I believe this image could grow larger in future and if we combine it with ci_cpu it becomes similar to ci_cpu. 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] manupa-arm commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
manupa-arm commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1193786595

   I see. 
   
   Yes none of the contrib tests are run (that is how cmsis tests are run today in ci_cpu).
   We need to add that as well then.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] u99127 commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
u99127 commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1194578355

   > just musing about naming--previously we called this image `ci_qemu` because it contained qemu. it seems like with this effort + riscv32 + the hexagon effort, we are gravitating more towards purpose-driven CI images (we discussed this before). there is already a ci_arm--shall we consider a container rename such as:
   > 
   > * `ci_arm` -> `ci_arm_aarch64`
   > * `ci_qemu` -> `ci_arm_cortexm`
   
   I'd suggest that ci_arm is renamed to ci_aarch64  . 
   
   Ramana


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] areusch commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
areusch commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1203151363

   @manupa-arm please take another look when you have a minute


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on a diff in pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #11809:
URL: https://github.com/apache/tvm/pull/11809#discussion_r924844530


##########
docker/Dockerfile.ci_cpu:
##########
@@ -124,15 +124,6 @@ RUN bash /install/ubuntu_install_androidsdk.sh
 ENV ANDROID_HOME=/opt/android-sdk-linux/
 ENV ANDROID_NDK_HOME=/opt/android-sdk-linux/ndk/21.3.6528147/
 
-# Install CMSIS_NN
-COPY install/ubuntu_install_cmsis.sh /install/ubuntu_install_cmsis.sh
-RUN bash /install/ubuntu_install_cmsis.sh /opt/arm/ethosu/cmsis
-ENV CMSIS_PATH=/opt/arm/ethosu/cmsis/
-
-# Arm(R) Ethos(TM)-U NPU driver
-COPY install/ubuntu_install_ethosu_driver_stack.sh /install/ubuntu_install_ethosu_driver_stack.sh

Review Comment:
   I can update this PR by adding `Arm(R) Ethos(TM)-U NPU driver` to the qemu, we merge this. Then we will update the qemu image and then send another PR to remove `test_ethosu` and move it to qemu.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1194306324

   @ashutosh-arm thanks for catching this. I moved test_cmsisnn tests to qemu, there's also test_ethosn which we should move, but we need to update qemu image for that.
   I think I need to send another PR for that along with sharding the qemu tests since it has timeout issue right now


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1194350086

   @manupa-arm thanks for clarification! Then I think this PR is ready, I just need to figure out the timeout issue.
   cc @areusch @driazati 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] Mousius commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
Mousius commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1168372615

   > Adding to Andrew's point, I think we can remove `USE_MICRO` since it's almost always ON. Also, I prefer to keep ci_qemu separate and also call it ci_microtvm. I believe this image could grow larger in future and if we combine it with ci_cpu it becomes similar to ci_cpu.
   
   I support this approach as well, having two smaller containers with a specific responsibility rather than one monolithic container should make it easier to manage in the long term as the needs evolve. The only downside is the typical container hopping for development, though I think we can figure out how to get that to work also.
   
   Minor note, I think `ci_micro` rather than `ci_microtvm` as it's already identified as a `tvm` image?


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] areusch commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
areusch commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1193577278

   @manupa-arm can you take a look again?


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on a diff in pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #11809:
URL: https://github.com/apache/tvm/pull/11809#discussion_r905469964


##########
docker/Dockerfile.ci_cpu:
##########
@@ -110,10 +110,6 @@ RUN bash /install/ubuntu_install_boost.sh
 COPY install/ubuntu_install_caffe.sh /install/ubuntu_install_caffe.sh
 RUN bash /install/ubuntu_install_caffe.sh
 
-# Github Arm(R) Ethos(TM)-N NPU driver

Review Comment:
   If my understanding is correct and this is used for running this command:
   ```
   run_pytest ctypes ${TVM_INTEGRATION_TESTSUITE_NAME}-contrib-test_ethosu tests/python/contrib/test_ethosu -n auto
   ```
   I suggest we also move this to ci_qemu to have all microtvm tests in one place.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] areusch commented on a diff in pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #11809:
URL: https://github.com/apache/tvm/pull/11809#discussion_r905457625


##########
docker/Dockerfile.ci_cpu:
##########
@@ -110,10 +110,6 @@ RUN bash /install/ubuntu_install_boost.sh
 COPY install/ubuntu_install_caffe.sh /install/ubuntu_install_caffe.sh
 RUN bash /install/ubuntu_install_caffe.sh
 
-# Github Arm(R) Ethos(TM)-N NPU driver

Review Comment:
   i think this one should stay in ci_cpu, it's unrelated to microTVM



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on a diff in pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #11809:
URL: https://github.com/apache/tvm/pull/11809#discussion_r924844530


##########
docker/Dockerfile.ci_cpu:
##########
@@ -124,15 +124,6 @@ RUN bash /install/ubuntu_install_androidsdk.sh
 ENV ANDROID_HOME=/opt/android-sdk-linux/
 ENV ANDROID_NDK_HOME=/opt/android-sdk-linux/ndk/21.3.6528147/
 
-# Install CMSIS_NN
-COPY install/ubuntu_install_cmsis.sh /install/ubuntu_install_cmsis.sh
-RUN bash /install/ubuntu_install_cmsis.sh /opt/arm/ethosu/cmsis
-ENV CMSIS_PATH=/opt/arm/ethosu/cmsis/
-
-# Arm(R) Ethos(TM)-U NPU driver
-COPY install/ubuntu_install_ethosu_driver_stack.sh /install/ubuntu_install_ethosu_driver_stack.sh

Review Comment:
   I can update this PR by adding `Arm(R) Ethos(TM)-U NPU driver` to the qemu and merge this. Then we will update the qemu image and send another PR to remove `test_ethosu` and move it to qemu.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on a diff in pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #11809:
URL: https://github.com/apache/tvm/pull/11809#discussion_r924843090


##########
docker/Dockerfile.ci_cpu:
##########
@@ -124,15 +124,6 @@ RUN bash /install/ubuntu_install_androidsdk.sh
 ENV ANDROID_HOME=/opt/android-sdk-linux/
 ENV ANDROID_NDK_HOME=/opt/android-sdk-linux/ndk/21.3.6528147/
 
-# Install CMSIS_NN
-COPY install/ubuntu_install_cmsis.sh /install/ubuntu_install_cmsis.sh
-RUN bash /install/ubuntu_install_cmsis.sh /opt/arm/ethosu/cmsis
-ENV CMSIS_PATH=/opt/arm/ethosu/cmsis/
-
-# Arm(R) Ethos(TM)-U NPU driver
-COPY install/ubuntu_install_ethosu_driver_stack.sh /install/ubuntu_install_ethosu_driver_stack.sh

Review Comment:
   we should move that after we updated the qemu image with the dependencies for this test



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] leandron commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
leandron commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1168539081

   Looking at the alternatives being proposed until now, I feel that for *our current type of usage* is makes more sense to unify `ci_qemu` into `ci_cpu`, for two reasons:
   
   1. [According to our Jenkinsfile](https://github.com/apache/tvm/blob/ce77d20c4615c655214d0bd38307813e539ba9a2/Jenkinsfile#L503-L511), both jobs that require `ci_cpu` and `ci_qemu` will run on the same target machines, so the expectation is that our Jenkins nodes will more commonly pull `ci_cpu + ci_qemu` in terms of storage
   2. Creating a base image with common dependencies, would also generate a side problem, which is a _time dependency_ to rebuild our images (we can't build all of them at the same time), and the need to create a generic label e.g. `latest` or `last-successful` for this base image to be picked up in the dependant images. I'd like to avoid this if possible.
   
   Given our current Docker update practices that don't require updating all images at once, I feel we better to consolidate the dependencies in one single image until we can devise better rules for Docker updating.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] manupa-arm commented on a diff in pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
manupa-arm commented on code in PR #11809:
URL: https://github.com/apache/tvm/pull/11809#discussion_r920197615


##########
docker/Dockerfile.ci_cpu:
##########
@@ -124,15 +124,6 @@ RUN bash /install/ubuntu_install_androidsdk.sh
 ENV ANDROID_HOME=/opt/android-sdk-linux/
 ENV ANDROID_NDK_HOME=/opt/android-sdk-linux/ndk/21.3.6528147/
 
-# Install CMSIS_NN
-COPY install/ubuntu_install_cmsis.sh /install/ubuntu_install_cmsis.sh
-RUN bash /install/ubuntu_install_cmsis.sh /opt/arm/ethosu/cmsis
-ENV CMSIS_PATH=/opt/arm/ethosu/cmsis/
-
-# Arm(R) Ethos(TM)-U NPU driver
-COPY install/ubuntu_install_ethosu_driver_stack.sh /install/ubuntu_install_ethosu_driver_stack.sh

Review Comment:
   run_pytest ctypes ${TVM_INTEGRATION_TESTSUITE_NAME}-contrib-test_ethosu tests/python/contrib/test_ethosu -n auto
   
   we need this line moved alongside as well.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on a diff in pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #11809:
URL: https://github.com/apache/tvm/pull/11809#discussion_r924864417


##########
docker/Dockerfile.ci_cpu:
##########
@@ -124,15 +124,6 @@ RUN bash /install/ubuntu_install_androidsdk.sh
 ENV ANDROID_HOME=/opt/android-sdk-linux/
 ENV ANDROID_NDK_HOME=/opt/android-sdk-linux/ndk/21.3.6528147/
 
-# Install CMSIS_NN
-COPY install/ubuntu_install_cmsis.sh /install/ubuntu_install_cmsis.sh
-RUN bash /install/ubuntu_install_cmsis.sh /opt/arm/ethosu/cmsis
-ENV CMSIS_PATH=/opt/arm/ethosu/cmsis/
-
-# Arm(R) Ethos(TM)-U NPU driver
-COPY install/ubuntu_install_ethosu_driver_stack.sh /install/ubuntu_install_ethosu_driver_stack.sh

Review Comment:
   I tested it in qemu, looks fine. So I moved it



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] areusch commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
areusch commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1194441129

   just musing about naming--previously we called this image `ci_qemu` because it contained qemu. it seems like with this effort + riscv32 + the hexagon effort, we are gravitating more towards purpose-driven CI images (we discussed this before). there is already a ci_arm--shall we consider a container rename such as:
   - `ci_arm` -> `ci_arm_aarch64`
   - `ci_qemu` -> `ci_arm_cortexm`


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] Mousius commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
Mousius commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1169685951

   > [According to our Jenkinsfile](https://github.com/apache/tvm/blob/ce77d20c4615c655214d0bd38307813e539ba9a2/Jenkinsfile#L503-L511), both jobs that require ci_cpu and ci_qemu will run on the same target machines, so the expectation is that our Jenkins nodes will more commonly pull ci_cpu + ci_qemu in terms of storage
   
   Provided the images have the same initial layers (base image or copy paste both should work), the storage cost should be the same as it'll re-use the layers when pulling the containers - if a node already had `ci_cpu` it should have a bunch of layers it can re-use for `ci_micro`.
   
   > Creating a base image with common dependencies, would also generate a side problem, which is a time dependency to rebuild our images (we can't build all of them at the same time), and the need to create a generic label e.g. latest or last-successful for this base image to be picked up in the dependant images. I'd like to avoid this if possible.
   
   As far as I can tell we'd build `ci_base`, tag it with the same tag as the rest of the images and use that in the `FROM` of the subsequent images:
   
   ```
   ARG TAG="latest"
   FROM tlcpackstaging/ci-base:${TAG}
   ```
   
   Therefore, apart from a small network up/down to propagate the image, it should be a similar time to build as we have now? Sorry if I missed something here @leandron?


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] driazati commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
driazati commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1170089123

   I also support keeping the images focused on their actual use instead of everything under the sun. With #11768 it should be less common that we're using different tags for the images (everything should be built and updated at once if we follow that process, though it is possible still to do manual updates) and we can avoid dependency drift by being careful in reviewing Docker image changes.
   
   Docker image size for us is a problem (more than storage) that makes CI slower and harder to reproduce, so they should contain only what they need to run their specific tests.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] manupa-arm commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
manupa-arm commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1170044876

   > another way that we could get into trouble with consolidating ci_cpu and ci_qemu is two different docker/install scripts could add conflicting packages.
   
   Actually, I was arguing that we should avoid the need of conflicting packages as micro is another backend of TVM (only allow exclusive dependencies in either). Such a divergence is worrying for a compiler that is aiming to support both micro and non-micro compilation flows.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] manupa-arm commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
manupa-arm commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1165421724

   In order to reason about this, it would be great to understand why do we maintain two seperate containers for microTVM and TVM, especially when QEMU is a software that runs on the cpu. (Sorry I might've missed the discussion why went on to create to two seperate images)
   
   In an alternate world, we could consolidate all testing dependencies to ci_cpu and keep TVM and microTVM aligned in-terms of testing environment, unless we need conflicting dependencies in the testing for TVM vs microTVM *not just additive ones*.
   
   
   
   
   
   
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] areusch commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
areusch commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1167930956

   hmm yeah so here's what i know about the history here:
   1. originally when we added QEMU to the TVM CI, `USE_MICRO` was an optional CMake setting. having `ci_qemu` separate from `ci_cpu` provided some diversity in testing the various config settings. MLF accidentally made `USE_MICRO` required; however, at this time, we had already completed a refactor that substantially reduced the code footprint of `USE_MICRO` so i haven't exactly prioritized fixing it.
      - the folks who might care about `USE_MICRO=OFF` still are those who use `libtvm_runtime.so` without using Python. i'm absolutely happy to prioritize a fix (or review one) if the runtime impact turns out to be significant for anyone
   2. QEMU was pretty large and we do want, operationally, to avoid requiring any one user to download a huge docker image just to repro the CI. `ci_gpu` is 25GB right now, but I don't want that to become the norm. keeping Zephyr installed separately was a way to avoid that.
   3. i don't think there is a reason why the ci_cpu deps are incompatible with anything in ci_qemu, but obviously this could be a reason to separate them if so.
   
   `ci_cpu` is 7GB now, so it's not small but it has a ways to go before it's the long pole. i could go either way here, though if folks want to continue adding micro toolchains those could become somewhat sizeable.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on a diff in pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #11809:
URL: https://github.com/apache/tvm/pull/11809#discussion_r924848507


##########
docker/Dockerfile.ci_cpu:
##########
@@ -124,15 +124,6 @@ RUN bash /install/ubuntu_install_androidsdk.sh
 ENV ANDROID_HOME=/opt/android-sdk-linux/
 ENV ANDROID_NDK_HOME=/opt/android-sdk-linux/ndk/21.3.6528147/
 
-# Install CMSIS_NN
-COPY install/ubuntu_install_cmsis.sh /install/ubuntu_install_cmsis.sh
-RUN bash /install/ubuntu_install_cmsis.sh /opt/arm/ethosu/cmsis
-ENV CMSIS_PATH=/opt/arm/ethosu/cmsis/
-
-# Arm(R) Ethos(TM)-U NPU driver
-COPY install/ubuntu_install_ethosu_driver_stack.sh /install/ubuntu_install_ethosu_driver_stack.sh

Review Comment:
   sorry we already have that. I meant `Github Arm(R) Ethos(TM)-N NPU driver`, I believe that is required, correct me if I'm wrong.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh merged pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
mehrdadh merged PR #11809:
URL: https://github.com/apache/tvm/pull/11809


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] areusch commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
areusch commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1179106048

   i think most of the discussion here has aligned around purpose-built CI images. @leandron are your concerns assuaged? 
   
   regarding `ci_base`: I'm somewhat supportive of this, but currently unsure whether this would move the needle very much in practice and there are some operational concerns. we currently build images mostly automatically around the same time via the nightly rebuild and also in CI. the common deps we're talking about here are mostly a mix of `apt` packages plus some things built from sources downloaded from (i think) version-pinned URLs. there are some other things in here, to be sure--rust packages, for instance. i do agree that `ci_base` would enforce that we don't diverge, but just stating that i don't know, given the above, that i expect to see a huge effect in creating that. i agree with @Mousius that the layers would ideally be reused; i wouldn't underestimate the network transfer time, however.
   
   I do want to note that we also mitigate drift in other was e.g. via common install scripts and (soon) via locked python dependencies (this more addresses the example of tensorflow divergence). however, i acknowledge the drawback that this is merely convention and dependent on reviewers.
   
   for verticals like microTVM, i think a separate image makes sense. for e.g. backends, it's less clear--what if you want to test cuda with paddlepaddle? there is no such paddlepaddle dep for arm64, so it can't go in the base image. does this mean base images need to be arch-specific--if so, where's the line (e.g. what if it's available for 20.04 but not 18.04)?
   
   so far as action items, i think we should capture the need of either a common base image or documentation into a GH issue. we should also update our docs accordingly. would love to hear more thoughts @Mousius @manupa-arm @leandron @driazati 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] areusch commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
areusch commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1194863969

   @u99127 i'm fine with that, should we then `ci_cortexm` instead? 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1194482213

   @areusch I like to change the name, but I think ci_arm_cortexm is too specific. We could have riscv support with Zephyr in future which I assume the testing is gonna be part of this image. Then it would be confusing. I think ci_micro works better here. wdyt?


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] areusch commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
areusch commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1169368758

   i think while i'm sympathetic to reducing the overall image disk size, in practice:
   - DockerHub stores these for us and we don't have a concern there
   - we have the problem of docker image bloat hogging disk on our CI nodes anyway due to image revisions, and have already solved that.
   
   so i don't think we need to make that a priority in this decision. i think then that it comes down to what's operationally easier, and i think that's the POV @Mousius and @manupa-arm were advocating. i tend to agree here--another way that we could get into trouble with consolidating `ci_cpu` and `ci_qemu` is two different `docker/install` scripts could add conflicting packages. we can mitigate that by keeping microTVM separated, and I'd vote for continuing that with renaming `ci_qemu` to `ci_micro`.
   
   cc @driazati @konturn in case they have other thoughts from a CI perspective.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] manupa-arm commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
manupa-arm commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1168441784

   I agree that having modular (smaller) container aids the ease of reproducability from a developer PoV.
   
   I'm worried with this approach is that this creates a room for set of diveregent dependencies to be introduced between microTVM vs TVM (the deps that are not specific to micro), especially when they are not seperate projects. Right now, only thing that keeps the common deps align is on the basis that we use the same helper scripts (ubuntu_install*.sh) to construct the image.
   
   E.g. : This could open up the possibility to advance the version of tensorflow supported by TVM project while microTVM lacking behind because there will now be tested in two environments.
   
   Maybe we could create a base container with such common deps to ensure such divergence are harder to do ? (and extend that image into ci_cpu and ci_qemu)


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on pull request #11809: [microTVM][ARM] Keep microtvm testing only in QEMU Image

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #11809:
URL: https://github.com/apache/tvm/pull/11809#issuecomment-1172554440

   @manupa-arm I agree with you. Both better documentation and having a ci_base image which has common dependencies are great ideas.
   It would be great if we can make an action items list based on the discussion here.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org