[MXNET-1093] Add python3 Docker images for each MXNet release #12791

mbaijal · 2018-10-11T03:47:10Z

Description

@KellenSunderland @marcoabreu @gautamkmr
This PR updates the docker/docker-python folder to add 8 new dockerfiles for python3. It also updates the automated docker release script to build and update the python 3 docker images similar to python 2.
Python version used is 3.5 which is the default version on ubuntu 16.04.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http:https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Added 8 new dockerfiles for python 3 - cpu, cu80, cu90, cu92 with and without mkl.
Updated the build script to also build, test and upload these new dockerfiles
Updated the readme.

KellenSunderland · 2018-10-11T08:12:03Z

docker/docker-python/Dockerfile.mxnet.python3.cpu

+RUN apt-get update
+RUN apt-get install -y wget python3 gcc
+RUN wget https://bootstrap.pypa.io/get-pip.py
+RUN python3 get-pip.py


Looks good. Super nit: I'd combine smaller commands like this into a single layer.

RUN apt-get update && \ apt-get install -y wget python3 gcc && \ ...

+1 to Kellen's ask: I think it is NOT a nit-pick but an important requirement because each RUN corresponds to a filesystem layer for Docker. See this: https://stackoverflow.com/questions/31222377/what-are-docker-image-layers

@KellenSunderland @bhavinthaker Thank you, I agree. I will update the PR.

marcoabreu · 2018-10-11T11:14:38Z

docker/docker-python/Dockerfile.mxnet.python3.cpu

+RUN wget https://bootstrap.pypa.io/get-pip.py
+RUN python3 get-pip.py
+
+RUN pip3 install mxnet


Version has to be specifyable

Do you mean parameterize the MXNet version in the dockerfiles?

Yes. We have to be able to create images for the different versions in a reproducible fashion.

marcoabreu · 2018-10-11T11:16:08Z

docker/docker-python/README.md

@@ -12,13 +12,21 @@ It uses the appropriate pip binaries to build different docker images as -
 * gpu_cu80_mkl
 * gpu_cu92 
 * gpu_cu92_mkl
+* cpu_py3


I'd prefer if python3 was the default.

I would not want to change the docker images already uploaded for MXNet 1.3 since this might cause something to break for a user. This change to make python3 the default docker version cannot be immediate and should be discussed on the dev list. If agreed upon, this should take effect from the next release onwards. Wdyt?

Very good points. Could you start the discussion?

This has been discussed on dev-list and the decision is to switch to python3 images as default. This will be done in the next release to avoid disruption to the current users.

docker/docker-python/README.md

marcoabreu · 2018-10-11T11:18:58Z

docker/docker-python/build_python_dockerfile.sh

@@ -117,10 +131,40 @@ docker_build_image "${mxnet_version}_gpu_cu92_mkl" "Dockerfile.mxnet.python.gpu.
 docker_test_image_gpu "${mxnet_version}_gpu_cu92_mkl"


+
+# Build and Test Python3 dockerfiles - CPU
+docker_build_image "${mxnet_version}_cpu_python3" "Dockerfile.mxnet.python3.cpu"


I'd prefer if the builds and CPU tests ran parallelized to reduce the critical path

I'd be a little concerned about memory usage if we were to parallelize the builds. I could see that this might make building the images difficult on a laptop or even a middle-tier cloud instance.

I built 15 images for our docker cache in parallel on my laptop and didn't encounter any problems. I don't think it will be an issue. Also, this script is targeted for CD execution - individual compilation could be an additional mode, but we should optimise for maximum resource utilization here IMO

mbaijal · 2018-10-12T20:01:40Z

Yea.

marcoabreu

Small things, but overall looks pretty good to me

docker/docker-python/Dockerfile.mxnet.python.cpu

marcoabreu · 2018-10-16T22:40:50Z

docker/docker-python/build_python_dockerfile.sh

+docker_generate_image_gpu "${mxnet_version}_gpu_cu92_mkl" "Dockerfile.mxnet.python.gpu.cu92.mkl" "python"
+
+echo "Waiting for MXNet Python2 Docker Images to Build"
+wait


What exactly does this do?

It waits for all the processes spawned as background processes (build, test) to complete before moving on to the next command.

Awesome! Does it also properly process error codes of the underlying processes?

marcoabreu · 2018-10-16T22:41:12Z

docker/docker-python/build_python_dockerfile.sh


+#Build and Test dockerfiles - GPU
+docker_generate_image_gpu "${mxnet_version}_gpu_cu90" "Dockerfile.mxnet.python.gpu.cu90" "python" &


Does this actually run all the commands in parallel?

Yes I verified using ps -ef that builds/tests are running in parallel. However, I do also want to record the total runtime with and without parallelization.

Excellent! Did you also test error propagation in that case?

The following are runtimes on a p2.16x instance to build and test all docker images (python2 and python3)
Runtime with parallelization: 13mins
Runtime without parallelization: 29mins

Awesome! What happens if there is an error in one of the builds (you can simulate that by just returning a non zero exit code in one of the RUN clauses)? Does it get propagated?

I checked that by hardcoding an incorrect mxnet version in one of the dockerfiles. While a non-zero error message is printed the logs, the script does not exit and completes with "success".
I will fix this.

Great, thanks

Fixed in the last commit by writing each build log to a file and then parsing these logs for a possible error message. If error found, script fails. The error message makes it easy to find out which tag failed.

ankkhedia · 2018-10-29T22:37:21Z

@marcoabreu It seems that your review comments has been addressed. Could you please take a look?

mbaijal · 2018-10-30T19:07:18Z

@marcoabreu @sandeep-krishnamurthy Could either of you please merge this PR. Thanks!

marcoabreu · 2018-10-31T09:36:54Z

Please wait with merging. I'm still discussing some things internally

larroy

We can't merge this code as it is.

larroy · 2018-11-01T14:11:17Z

docker/docker-python/build_python_dockerfile.sh


+# Functions
 docker_test_image_cpu(){


We can't have docker running like this because it leaves containers hanging around even when the parent process terminates. We introduce logic to avoid this kinds of problems in CI #12381

My suggestion is to use one of this two options:

Use https://github.com/apache/incubator-mxnet/blob/master/ci/build.py as it's done in https://github.com/apache/incubator-mxnet/blob/master/ci/docker_cache.py to run the tests safely, build the images in parallel. This is probably the fastest route that is safe but introduces some coupling between CI and CD.

Abstract the cleanup logic in build.py in a module which is then used from CI and CD with the specific commands.

Duplicate the logic from build.py and docker_cache.py for this task (copy & paste) and adjust for the specific case. A downside is that introduces similar logic to be maintained , the upside is that the logic wouldn't be coupled and can evolve in parallel.

If on doubt, we can choose option 1 and go from there.

larroy · 2018-11-01T14:16:29Z

Thanks for this effort. I have some reservations about the implementation, see my previous comment about zombie containers and resource disposal in the code.

Also, can we use Python instead of shell to automate these steps? It's more robust, debuggable, flexible and easier to mantain, we don't want to introduce complex logic in shell scripts.

All the automation on the project is done in Python. The code that is forking process to parallelize the build should be done as it's done in https://github.com/apache/incubator-mxnet/blob/master/ci/docker_cache.py or with a similar Python construct of your choice. Even if it's not critical to parallelize building of the images, if the code is well factored and building the container it's on its own function is very easy to parallelize.

reconsidered internally

ankkhedia · 2018-11-01T16:51:38Z

@mbaijal Could you please look into comments provided by @larroy and @marcoabreu

kalyc · 2018-11-12T23:22:54Z

Thanks for your contributions @mbaijal
@mxnet-label-bot update [pr-awaiting-review]

@larroy , @marcoabreu could you please help with reviewing this PR?

marcoabreu · 2018-11-12T23:24:08Z

@mxnet-label-bot update [pr-awaiting-response]

marcoabreu · 2018-11-12T23:24:30Z

@mxnet-label-bot add [ci,docker]

marcoabreu · 2018-11-12T23:25:02Z

We are waiting for a response.

vandanavk · 2018-11-26T21:37:21Z

@mbaijal could you check the CI build failure and add a comment explaining the changes you made/are making in response to Pedro and Marco's feedback?

@mxnet-label-bot update [CI, Docker, pr-awaiting-testing]

larroy · 2018-12-07T12:06:57Z

Given that we are taking over this script, shall we close this?

larroy · 2018-12-07T12:07:07Z

@perdasilva

perdasilva · 2018-12-12T09:10:48Z

@larroy - let's close it. I'll open a new PR after my RFC goes through

larroy · 2018-12-16T23:39:44Z

@perdasilva it's meghnas PR, @mbaijal can you close? Thanks.

mbaijal · 2018-12-20T21:34:43Z

I don't mind closing this but half of the code for this change (python2 and initial automation script) has already been merged in a previous PR and this PR contains some updates and fixes to it. This updated script was also used for the last two docker releases. In my opinion it might be better to merge this PR to complete those changes and then deprecate it when a new python script is available.

@larroy Wdyt?

perdasilva · 2018-12-21T06:07:30Z

@mbaijal imo, in this case, we should probably go fwd and merge. @larroy?

Roshrini · 2019-01-02T19:55:11Z

@larroy Is it okay if we merge this PR?
@nswamy @marcoabreu

anirudhacharya · 2019-01-14T22:54:22Z

@mxnet-label-bot update [pr-awaiting-review]

sandeep-krishnamurthy · 2019-01-28T22:44:46Z

@larroy / @marcoabreu - I think we should take this PR forward. Can you please help take a look again?

marcoabreu · 2019-02-05T19:07:36Z

Please hold on with this PR since we are currently refactoring that pipeline.

@perdasilva for further communication regarding this topic

larroy · 2019-02-06T21:35:13Z

up to @perdasilva and @mbaijal please decide best path forward.

perdasilva · 2019-02-08T07:08:34Z

@marcoabreu lets talk about this issue today and come up with the solution. If the PR does what someone needs right now, I don't see any reason to hold this back. We can always refactor it later if need be...

perdasilva · 2019-02-19T13:19:16Z

@mbaijal @sandeep-krishnamurthy @srochel please merge this - thank you

I don't know if this is tha main merge issue the PR is complaining about on mxnet, but maybe it will help

Fixes merge issues

mbaijal · 2019-02-26T22:25:54Z

Thanks @perdasilva !

…#12791) * Add dockerfiles for python3 * Fix * Fix the dockerfile names * Update the README with python3 images * Parameterize the mxnet version * Fix typo * Reduce the number of docker layers * Combine build and Test, run builds in parallel. Use variable names instead of numbers * minor fix * Update README for build command * Bug Fix: Script should fail if any background process returns non-zero code * Changes needed for Benchmarking, 4 new tags, change python to python-dev * Minor typos * Minor Rearrangement - sometimes tagging fails * Move error checking to a method * Push BAI images to dockerhub * Create ~/temp dir * Bug Fix * Fix merge issues I don't know if this is tha main merge issue the PR is complaining about on mxnet, but maybe it will help

* Add dockerfiles for python3 * Fix * Fix the dockerfile names * Update the README with python3 images * Parameterize the mxnet version * Fix typo * Reduce the number of docker layers * Combine build and Test, run builds in parallel. Use variable names instead of numbers * minor fix * Update README for build command * Bug Fix: Script should fail if any background process returns non-zero code * Changes needed for Benchmarking, 4 new tags, change python to python-dev * Minor typos * Minor Rearrangement - sometimes tagging fails * Move error checking to a method * Push BAI images to dockerhub * Create ~/temp dir * Bug Fix * Fix merge issues I don't know if this is tha main merge issue the PR is complaining about on mxnet, but maybe it will help

…#12791) * Add dockerfiles for python3 * Fix * Fix the dockerfile names * Update the README with python3 images * Parameterize the mxnet version * Fix typo * Reduce the number of docker layers * Combine build and Test, run builds in parallel. Use variable names instead of numbers * minor fix * Update README for build command * Bug Fix: Script should fail if any background process returns non-zero code * Changes needed for Benchmarking, 4 new tags, change python to python-dev * Minor typos * Minor Rearrangement - sometimes tagging fails * Move error checking to a method * Push BAI images to dockerhub * Create ~/temp dir * Bug Fix * Fix merge issues I don't know if this is tha main merge issue the PR is complaining about on mxnet, but maybe it will help

KellenSunderland reviewed Oct 11, 2018

View reviewed changes

marcoabreu suggested changes Oct 11, 2018

View reviewed changes

sandeep-krishnamurthy added CI Docker pr-work-in-progress PR is still work in progress labels Oct 11, 2018

mbaijal changed the title ~~[MXNET-1093] Add python3 Docker images for each MXNet release~~ [WIP][MXNET-1093] Add python3 Docker images for each MXNet release Oct 12, 2018

marcoabreu reviewed Oct 16, 2018

View reviewed changes

mbaijal changed the title ~~[WIP][MXNET-1093] Add python3 Docker images for each MXNet release~~ [MXNET-1093] Add python3 Docker images for each MXNet release Oct 23, 2018

marcoabreu previously approved these changes Oct 30, 2018

View reviewed changes

larroy suggested changes Nov 1, 2018

View reviewed changes

marcoabreu added pr-awaiting-response PR is reviewed and waiting for contributor to respond and removed CI Docker pr-work-in-progress PR is still work in progress labels Nov 12, 2018

marcoabreu added CI Docker labels Nov 12, 2018

nswamy added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Dec 8, 2018

mbaijal closed this Dec 20, 2018

mbaijal reopened this Dec 20, 2018

marcoabreu added pr-awaiting-review PR is waiting for code review and removed CI Docker pr-awaiting-merge Review and CI is complete. Ready to Merge labels Jan 14, 2019

marcoabreu added pr-work-in-progress PR is still work in progress and removed pr-awaiting-review PR is waiting for code review labels Feb 5, 2019

Fix merge issues

0f135cc

I don't know if this is tha main merge issue the PR is complaining about on mxnet, but maybe it will help

perdasilva mentioned this pull request Feb 19, 2019

Fixes merge issues mbaijal/incubator-mxnet#17

Merged

mbaijal added 2 commits February 26, 2019 14:20

Merge pull request #17 from perdasilva/patch-1

25e75c4

Fixes merge issues

Merge branch 'master' into dockerpython3

5780b1b

marcoabreu merged commit ab0ca86 into apache:master Mar 12, 2019


		#Build and Test dockerfiles - GPU
		docker_generate_image_gpu "${mxnet_version}_gpu_cu90" "Dockerfile.mxnet.python.gpu.cu90" "python" &

[MXNET-1093] Add python3 Docker images for each MXNet release #12791

[MXNET-1093] Add python3 Docker images for each MXNet release #12791

Conversation

mbaijal commented Oct 11, 2018 • edited Loading

Description

Checklist

Essentials

Changes

KellenSunderland Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbaijal Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbaijal commented Oct 12, 2018

marcoabreu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankkhedia commented Oct 29, 2018

mbaijal commented Oct 30, 2018

marcoabreu commented Oct 31, 2018

larroy left a comment

Choose a reason for hiding this comment

larroy Nov 1, 2018 • edited Loading

Choose a reason for hiding this comment

larroy commented Nov 1, 2018

ankkhedia commented Nov 1, 2018 • edited Loading

kalyc commented Nov 12, 2018

marcoabreu commented Nov 12, 2018

marcoabreu commented Nov 12, 2018

marcoabreu commented Nov 12, 2018

vandanavk commented Nov 26, 2018

larroy commented Dec 7, 2018

larroy commented Dec 7, 2018

perdasilva commented Dec 12, 2018

larroy commented Dec 16, 2018

mbaijal commented Dec 20, 2018 • edited Loading

perdasilva commented Dec 21, 2018

Roshrini commented Jan 2, 2019

anirudhacharya commented Jan 14, 2019

sandeep-krishnamurthy commented Jan 28, 2019

marcoabreu commented Feb 5, 2019

larroy commented Feb 6, 2019 • edited Loading

perdasilva commented Feb 8, 2019

perdasilva commented Feb 19, 2019

mbaijal commented Feb 26, 2019

mbaijal commented Oct 11, 2018 •

edited

Loading

KellenSunderland Oct 11, 2018 •

edited

Loading

mbaijal Oct 18, 2018 •

edited

Loading

larroy Nov 1, 2018 •

edited

Loading

ankkhedia commented Nov 1, 2018 •

edited

Loading

mbaijal commented Dec 20, 2018 •

edited

Loading

larroy commented Feb 6, 2019 •

edited

Loading