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/02/15 19:28:16 UTC
[GitHub] [tvm] driazati opened a new pull request #10257: [ci] Move pip dependencies to docker image
driazati opened a new pull request #10257:
URL: https://github.com/apache/tvm/pull/10257
Following on from #10246, this moves the `pip install`-at-runtime deps to the docker image install so they are baked in.
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
--
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 a change in pull request #10257: [ci] Move pip dependencies to docker image
Posted by GitBox <gi...@apache.org>.
Mousius commented on a change in pull request #10257:
URL: https://github.com/apache/tvm/pull/10257#discussion_r810977923
##########
File path: docker/install/ubuntu_install_python_package.sh
##########
@@ -32,10 +32,12 @@ pip3 install \
packaging \
Pillow \
pytest \
+ tlcpack-sphinx-addon==0.2.1 \
pytest-profiling \
pytest-xdist \
requests \
scipy \
synr==0.6.0 \
six \
+ ninja \
Review comment:
`apt` has better security and stability vs `pip`, especially given we install all these dependencies blindly - I'd generally favour using the OSes package manager until we actually need to work around 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 #10257: [ci] Move pip dependencies to docker images, add ninja / shellcheck
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #10257:
URL: https://github.com/apache/tvm/pull/10257#issuecomment-1063495481
i think maybe we should change task_ci_setup.sh to not install those packages if they already exist. i can't remember exactly what happened here, but there was a CI problem before where a disagreement between the pinned version of the package in task_ci_setup.sh and the pinned version in the docker/install scripts caused CI to fail. i think maybe this was due to not specifying -U in pip install? anyway, it would be great to ensure that either we use task_ci_setup.sh or we use new docker 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 #10257: [ci] Move pip dependencies to docker images
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #10257:
URL: https://github.com/apache/tvm/pull/10257#issuecomment-1061061255
@driazati what's the plan for removing task_ci_setup.sh tho?
--
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 #10257: [ci] Move pip dependencies to docker images, add ninja / shellcheck
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #10257:
URL: https://github.com/apache/tvm/pull/10257#issuecomment-1068508267
@Mousius can you take another look 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
[GitHub] [tvm] driazati commented on a change in pull request #10257: [ci] Move pip dependencies to docker image
Posted by GitBox <gi...@apache.org>.
driazati commented on a change in pull request #10257:
URL: https://github.com/apache/tvm/pull/10257#discussion_r808352208
##########
File path: docker/install/ubuntu_install_python_package.sh
##########
@@ -32,10 +32,12 @@ pip3 install \
packaging \
Pillow \
pytest \
+ tlcpack-sphinx-addon==0.2.1 \
pytest-profiling \
pytest-xdist \
requests \
scipy \
synr==0.6.0 \
six \
+ ninja \
Review comment:
See https://github.com/apache/tvm/pull/10246#discussion_r807238655, is there any advantage to sticking with `apt`?
--
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 #10257: [ci] Move pip dependencies to docker images
Posted by GitBox <gi...@apache.org>.
driazati commented on pull request #10257:
URL: https://github.com/apache/tvm/pull/10257#issuecomment-1061068680
> @driazati what's the plan for removing task_ci_setup.sh tho?
once this lands and we update the docker images we can remove the `pip install`s from there
> actually let's discuss that first before merging, as changes between the upstream index and what we pin can cause task_ci_setup.sh to start failing
the version pins are the same so the installs in`task_ci_setup.sh` should be a no-op once this is added
--
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 change in pull request #10257: [ci] Move pip dependencies to docker images
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #10257:
URL: https://github.com/apache/tvm/pull/10257#discussion_r821038566
##########
File path: docker/install/ubuntu_install_python_package.sh
##########
@@ -32,10 +32,12 @@ pip3 install \
packaging \
Pillow \
pytest \
+ tlcpack-sphinx-addon==0.2.1 \
pytest-profiling \
pytest-xdist \
requests \
scipy \
synr==0.6.0 \
six \
+ ninja \
Review comment:
i'm okay with sticking with `apt` for OS packages. for Python packages, i prefer we move to `pip` once we have properly recorded sha256 for those packages we want to install.
--
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 merged pull request #10257: [ci] Move pip dependencies to docker images, add ninja / shellcheck
Posted by GitBox <gi...@apache.org>.
Mousius merged pull request #10257:
URL: https://github.com/apache/tvm/pull/10257
--
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 a change in pull request #10257: [ci] Move pip dependencies to docker images
Posted by GitBox <gi...@apache.org>.
driazati commented on a change in pull request #10257:
URL: https://github.com/apache/tvm/pull/10257#discussion_r821044587
##########
File path: docker/install/ubuntu_install_python_package.sh
##########
@@ -32,10 +32,12 @@ pip3 install \
packaging \
Pillow \
pytest \
+ tlcpack-sphinx-addon==0.2.1 \
pytest-profiling \
pytest-xdist \
requests \
scipy \
synr==0.6.0 \
six \
+ ninja \
Review comment:
the pip `ninja` is just a thin wrapper to distribute the binary so in this case `apt` makes sense
--
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 a change in pull request #10257: [ci] Move pip dependencies to docker image
Posted by GitBox <gi...@apache.org>.
Mousius commented on a change in pull request #10257:
URL: https://github.com/apache/tvm/pull/10257#discussion_r807863992
##########
File path: docker/install/ubuntu_install_python_package.sh
##########
@@ -32,10 +32,12 @@ pip3 install \
packaging \
Pillow \
pytest \
+ tlcpack-sphinx-addon==0.2.1 \
pytest-profiling \
pytest-xdist \
requests \
scipy \
synr==0.6.0 \
six \
+ ninja \
Review comment:
Can we `apt-get install ninja-build` rather than pip?
--
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 change in pull request #10257: [ci] Move pip dependencies to docker images, add ninja / shellcheck
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #10257:
URL: https://github.com/apache/tvm/pull/10257#discussion_r827451783
##########
File path: docker/install/ubuntu_install_python_package.sh
##########
@@ -32,10 +32,12 @@ pip3 install \
packaging \
Pillow \
pytest \
+ tlcpack-sphinx-addon==0.2.1 \
pytest-profiling \
pytest-xdist \
requests \
scipy \
synr==0.6.0 \
six \
+ ninja \
Review comment:
going to resolve this one as i think we've addressed 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 edited a comment on pull request #10257: [ci] Move pip dependencies to docker images, add ninja / shellcheck
Posted by GitBox <gi...@apache.org>.
areusch edited a comment on pull request #10257:
URL: https://github.com/apache/tvm/pull/10257#issuecomment-1068508267
@Mousius can you take another look here and merge if you are good with 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