You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/04/20 20:09:45 UTC
[GitHub] [incubator-mxnet] leezu opened a new pull request #18115: CI: Consolidate Dockerfiles
leezu opened a new pull request #18115:
URL: https://github.com/apache/incubator-mxnet/pull/18115
## Description ##
This PR shows how to get rid of duplicate Dockerfiles and the `install/X.sh` code deduplication strategy on the example of CentOS7 Dockerfiles. Instead, we use a single templated Dockerfile with a multiple targets for different scenarios. The template arguments and respective targets are declared in `docker-compose.yml`.
This has multiple benefits:
1) Having a single Dockerfile instead of a number of scripts is easier to understand and maintain
2) Copying scripts into the Dockerfile as done in the previous approach prevents usage of the https://hub.docker.com/u/mxnetci/ cache on developer machines, as the `COPY` instruction invalidates the cache if the local repository does not match the repository used on CI (eg. development branch instead of master branch). If the cache is invalidated, developers must wait for long docker build time when attempting to reproduce certain CI builds (such as website build, cc @aaronmarkham) or using the docker image to perform the staticbuild.
3) Having fewer steps in the Dockerfile improves the build speed.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18115: CI: Consolidate Dockerfiles
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18115:
URL: https://github.com/apache/incubator-mxnet/pull/18115#discussion_r412545271
##########
File path: ci/docker/runtime_functions.sh
##########
@@ -983,6 +983,7 @@ sanity_check() {
# $2 -> python_cmd: The python command to use to execute the tests, python or python3
cd_unittest_ubuntu() {
set -ex
+ source /opt/rh/rh-python35/enable
Review comment:
CentOS7 ships with Python 2 by default. 3.5 is opt in. This is the opt-in. You can find more info https://www.softwarecollections.org/en/
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha commented on a change in pull request #18115: CI: Consolidate Dockerfiles
Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #18115:
URL: https://github.com/apache/incubator-mxnet/pull/18115#discussion_r412544049
##########
File path: ci/docker/runtime_functions.sh
##########
@@ -983,6 +983,7 @@ sanity_check() {
# $2 -> python_cmd: The python command to use to execute the tests, python or python3
cd_unittest_ubuntu() {
set -ex
+ source /opt/rh/rh-python35/enable
Review comment:
ok this seems to switch the python alias
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha commented on a change in pull request #18115: CI: Consolidate Dockerfiles
Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #18115:
URL: https://github.com/apache/incubator-mxnet/pull/18115#discussion_r412543587
##########
File path: ci/docker/runtime_functions.sh
##########
@@ -983,6 +983,7 @@ sanity_check() {
# $2 -> python_cmd: The python command to use to execute the tests, python or python3
cd_unittest_ubuntu() {
set -ex
+ source /opt/rh/rh-python35/enable
Review comment:
what does this do?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18115: CI: Consolidate Dockerfiles
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18115:
URL: https://github.com/apache/incubator-mxnet/pull/18115#discussion_r412546798
##########
File path: ci/docker/runtime_functions.sh
##########
@@ -983,6 +983,7 @@ sanity_check() {
# $2 -> python_cmd: The python command to use to execute the tests, python or python3
cd_unittest_ubuntu() {
set -ex
+ source /opt/rh/rh-python35/enable
Review comment:
Due to the way the CI runs stuff in the Dockerfile, we can't permanently opt-in when creating the Docker image. We can fix that later. For now need to opt in inside the runtime functions.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18115: CI: Consolidate Dockerfiles
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18115:
URL: https://github.com/apache/incubator-mxnet/pull/18115#discussion_r412545549
##########
File path: cd/mxnet_lib/mxnet_lib_pipeline.groovy
##########
@@ -75,9 +75,9 @@ def get_stash(mxnet_variant) {
// The environment corresponds to the docker files in the 'docker' directory
def get_environment(mxnet_variant) {
if (mxnet_variant.startsWith("cu")) {
- return "publish.centos7_gpu_${mxnet_variant}"
+ return "centos7_gpu_${mxnet_variant}"
Review comment:
Could be removed, but better to keep for consistency with the existing naming scheme. The name used here must match the name in the docker-compose.yml
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on issue #18115: CI: Consolidate Dockerfiles
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on issue #18115:
URL: https://github.com/apache/incubator-mxnet/pull/18115#issuecomment-616781425
Hey @leezu , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:
- To trigger all jobs: @mxnet-bot run ci [all]
- To trigger specific jobs: @mxnet-bot run ci [job1, job2]
***
**CI supported jobs**: [windows-gpu, windows-cpu, miscellaneous, website, centos-gpu, unix-gpu, edge, sanity, centos-cpu, clang, unix-cpu]
***
_Note_:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] marcoabreu commented on issue #18115: CI: Consolidate Dockerfiles
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #18115:
URL: https://github.com/apache/incubator-mxnet/pull/18115#issuecomment-616786077
That looks really neat!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on issue #18115: CI: Consolidate Dockerfiles
Posted by GitBox <gi...@apache.org>.
leezu commented on issue #18115:
URL: https://github.com/apache/incubator-mxnet/pull/18115#issuecomment-616845435
@mxnet-bot run ci [miscellaneous, website, centos-gpu, unix-gpu, edge, sanity, centos-cpu, clang, unix-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha commented on a change in pull request #18115: CI: Consolidate Dockerfiles
Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #18115:
URL: https://github.com/apache/incubator-mxnet/pull/18115#discussion_r412545223
##########
File path: cd/mxnet_lib/mxnet_lib_pipeline.groovy
##########
@@ -75,9 +75,9 @@ def get_stash(mxnet_variant) {
// The environment corresponds to the docker files in the 'docker' directory
def get_environment(mxnet_variant) {
if (mxnet_variant.startsWith("cu")) {
- return "publish.centos7_gpu_${mxnet_variant}"
+ return "centos7_gpu_${mxnet_variant}"
Review comment:
the `_gpu` infix doesn't seem necessary either
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18115: CI: Consolidate Dockerfiles
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18115:
URL: https://github.com/apache/incubator-mxnet/pull/18115#discussion_r412545549
##########
File path: cd/mxnet_lib/mxnet_lib_pipeline.groovy
##########
@@ -75,9 +75,9 @@ def get_stash(mxnet_variant) {
// The environment corresponds to the docker files in the 'docker' directory
def get_environment(mxnet_variant) {
if (mxnet_variant.startsWith("cu")) {
- return "publish.centos7_gpu_${mxnet_variant}"
+ return "centos7_gpu_${mxnet_variant}"
Review comment:
It's for consistency with the existing naming scheme. The name used here must match the name in the docker-compose.yml
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on issue #18115: CI: Consolidate Dockerfiles
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on issue #18115:
URL: https://github.com/apache/incubator-mxnet/pull/18115#issuecomment-616845507
Jenkins CI successfully triggered : [miscellaneous, website, centos-gpu, centos-cpu, unix-gpu, clang, edge, unix-cpu, sanity]
----------------------------------------------------------------
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:
users@infra.apache.org