[DONOT MERGE]Cache thirdparty pacakge on CI system, TEST CI #9150

Yancey1989 · 2018-03-16T04:13:56Z

Test task on CI: https://paddleci.ngrok.io/viewType.html?buildTypeId=Paddle_DistributedUnitTestNightly&branch_Paddle=9150&tab=buildTypeStatusDiv

helinwang · 2018-03-19T22:59:19Z

Thanks for the amazing PR!

TeamCity CI is very special in the sense that nvidia-docker command is run outside of docker container. We should not let TeamCity CI run arbitrary script outside of docker, otherwise it's a big security risk.

For example, currently docker/build.sh is run inside of docker, but the command to start the container (nvidia-docker run ... docker/build.sh) is configured by admin in TeamCity. In this way, even when a PR which changes build.sh is created, the build.sh run only inside the container, causing no damage to the host build agent if it's malicious. On the other hand, if teamcity/pr_build.sh is checked in, and CI configured to run it directly, it will run on the host system directly (outside of docker).

Maybe this PR can be documented in the wiki, and configured in the TeamCity PR build script settings?

helinwang · 2018-03-19T23:06:11Z

paddle/scripts/teamcity/pr_build.sh

+    -e "WITH_COVERAGE=ON" \
+    -e "COVERALLS_UPLOAD=ON" \
+    -e "GIT_PR_ID=${PR_ID}" \
+    -e "JSON_REPO_TOKEN=JSUOs6TF6fD2i30OJ5o2S55V8XWv6euen" \


Is this token sensitive data :) ?

Sure...my mistake and removed it.

Yancey1989 · 2018-03-20T06:38:30Z

Thanks for @helinwang! It's a very important reminder. And maybe we can review this PR and then close && move the script to a wiki page after approved.

typhoonzero · 2018-03-20T06:41:54Z

paddle/scripts/teamcity/pr_build.sh

+
+# Do not build dev image for now
+
+if [[ "%WITH_UBUNTU_MIRROR%" == "ON" ]]; then


Can test if Dockerfile is not changed, then we can skip building the image.

May not, a PR job may run on any node of the CI cluster, for one PR changes the Dockerfile and merged, the next PR run another node without changing of Dockerfile, and it would use the older dev Docker image, we do not want this. For another side,docker build would use the cached image Layer if Dockerfile has no changs, would not cost too much time.

Does it make sense to tag the docker image with the hash of the Dockerfile? (and remove all other tags when a PR builds)
E.g.,
docker image:
paddlepaddle/paddle:dev-000000
paddlepaddle/paddle:dev-000001

If the hash is 000001, then use paddlepaddle/paddle:dev-000001 and remove paddlepaddle/paddle:dev-000000 to free up space.

Done, it's a very good way!

helinwang · 2018-03-21T00:44:14Z

paddle/scripts/teamcity/pr_build.sh

+# Use the cached thirdparty package if the MD5 is mathes.
+md5_content=$(cat \
+              ${PWD}/cmake/external/*.cmake \
+              ${PWD}/paddle/scripts/teamcity/pr_build.sh \


Maybe don't need this line anymore it we don't check-in pre-build.sh to our repo.

helinwang · 2018-03-21T00:44:37Z

paddle/scripts/teamcity/pr_build.sh

+
+# Use the cached thirdparty package if the MD5 is mathes.
+md5_content=$(cat \
+              ${PWD}/cmake/external/*.cmake \


helinwang · 2018-03-21T00:47:19Z

paddle/scripts/teamcity/pr_build.sh

+
+# Do not build dev image for now
+
+if [[ "%WITH_UBUNTU_MIRROR%" == "ON" ]]; then


Does it make sense to tag the docker image with the hash of the Dockerfile? (and remove all other tags when a PR builds)
E.g.,
docker image:
paddlepaddle/paddle:dev-000000
paddlepaddle/paddle:dev-000001

If the hash is 000001, then use paddlepaddle/paddle:dev-000001 and remove paddlepaddle/paddle:dev-000000 to free up space.

Done, it's a very good way!

helinwang

LGTM!!!

helinwang · 2018-03-21T17:34:54Z

paddle/fluid/operators/CMakeLists.txt

@@ -156,7 +156,8 @@ if(WITH_DISTRIBUTE)
    set_source_files_properties(recv_op.cc PROPERTIES COMPILE_FLAGS ${DISTRIBUTE_COMPILE_FLAGS})
    op_library(listen_and_serv_op DEPS ${DISTRIBUTE_DEPS})
    set_source_files_properties(listen_and_serv_op.cc PROPERTIES COMPILE_FLAGS ${DISTRIBUTE_COMPILE_FLAGS})
-    cc_test(test_send_recv SRCS send_recv_op_test.cc DEPS send_op listen_and_serv_op sum_op executor)
+    # FIXME(Yancey1989): fix unit test
+    # cc_test(test_send_recv SRCS send_recv_op_test.cc DEPS send_op listen_and_serv_op sum_op executor)


It's better to separate this to another PR next time :)

Okay, and I add some comments at #9046 (comment), there are still some bugs while building with WITH_DISTRIBUTE=ON.

helinwang · 2018-03-21T17:38:01Z

python/paddle/fluid/tests/unittests/test_recv_op.py

@@ -21,6 +21,8 @@
 import os, sys
 import time

+sys.exit(0)


Maybe add a TODO here, otherwise it's possible to forget.

helinwang · 2018-03-21T17:52:35Z

paddle/scripts/teamcity/pr_build.sh

+
+dev_image_id=$(docker images paddlepaddle/paddle-dev:${md5_dev_dockerfile} -q)
+if [[ ${dev_image_id} == "" ]]; then
+    docker rmi $(docker images paddlepaddle/paddle-dev -q) || true


Smart of using paddlepaddle/paddle-dev!

Yancey1989 · 2018-03-22T03:25:29Z

Thanks for @helinwang and @typhoonzero 's reviewing, I moved the build script to the wiki page: https://github.com/PaddlePaddle/Paddle/wiki/PaddlePaddle-CI-Build-Scripts.

helinwang · 2018-03-22T03:26:26Z

Thank you!!!

Yancey1989 added 2 commits March 16, 2018 12:13

cache thirdparty pacakge on CI system

60ae7be

refine code

1dc8b4f

helinwang reviewed Mar 19, 2018

View reviewed changes

typhoonzero reviewed Mar 20, 2018

View reviewed changes

Yancey1989 added 6 commits March 20, 2018 15:11

update

4b6c08e

remove token message

255ac1e

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into improve_ci

9dd1da7

test env

0344d8e

fix ci

05517bd

fix ci

998334f

helinwang reviewed Mar 21, 2018

View reviewed changes

Yancey1989 mentioned this pull request Mar 21, 2018

Fluid dist unit test failed #9292

Closed

Yancey1989 added 2 commits March 21, 2018 16:57

disable test_recv_op

abc84be

update

c4c2ce1

helinwang previously approved these changes Mar 21, 2018

View reviewed changes

Yancey1989 mentioned this pull request Mar 22, 2018

Fluid dist build error on CI #9309

Closed

update

bc00b41

Yancey1989 dismissed helinwang’s stale review via bc00b41 March 22, 2018 03:17

move build script to wiki

69e9267

Yancey1989 added 2 commits March 29, 2018 12:51

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into improve_ci

d4960ed

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into improve_ci

4554276

Yancey1989 changed the title ~~Cache thirdparty pacakge on CI system~~ [DONOT MERGE]Cache thirdparty pacakge on CI system, TEST CI Mar 30, 2018

Yancey1989 added 3 commits April 2, 2018 09:21

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into improve_ci

68fdc95

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into improve_ci

897f602

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into improve_ci

5231c9c

Yancey1989 closed this Apr 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DONOT MERGE]Cache thirdparty pacakge on CI system, TEST CI #9150

[DONOT MERGE]Cache thirdparty pacakge on CI system, TEST CI #9150

Yancey1989 commented Mar 16, 2018 •

edited

Loading

helinwang commented Mar 19, 2018 •

edited

Loading

helinwang Mar 19, 2018

Yancey1989 Mar 20, 2018 •

edited

Loading

Yancey1989 commented Mar 20, 2018

typhoonzero Mar 20, 2018

Yancey1989 Mar 20, 2018

helinwang Mar 21, 2018 •

edited by Yancey1989

Loading

helinwang Mar 21, 2018

Yancey1989 Mar 21, 2018

helinwang Mar 21, 2018

helinwang Mar 21, 2018 •

edited by Yancey1989

Loading

helinwang left a comment

helinwang Mar 21, 2018

Yancey1989 Mar 22, 2018

helinwang Mar 21, 2018

helinwang Mar 21, 2018

Yancey1989 commented Mar 22, 2018 •

edited

Loading

helinwang commented Mar 22, 2018


		# Do not build dev image for now

		if [[ "%WITH_UBUNTU_MIRROR%" == "ON" ]]; then

[DONOT MERGE]Cache thirdparty pacakge on CI system, TEST CI #9150

[DONOT MERGE]Cache thirdparty pacakge on CI system, TEST CI #9150

Conversation

Yancey1989 commented Mar 16, 2018 • edited Loading

helinwang commented Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Yancey1989 Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Yancey1989 commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helinwang Mar 21, 2018 • edited by Yancey1989 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

helinwang Mar 21, 2018 • edited by Yancey1989 Loading

Choose a reason for hiding this comment

helinwang 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

Yancey1989 commented Mar 22, 2018 • edited Loading

helinwang commented Mar 22, 2018

Yancey1989 commented Mar 16, 2018 •

edited

Loading

helinwang commented Mar 19, 2018 •

edited

Loading

Yancey1989 Mar 20, 2018 •

edited

Loading

helinwang Mar 21, 2018 •

edited by Yancey1989

Loading

helinwang Mar 21, 2018 •

edited by Yancey1989

Loading

Yancey1989 commented Mar 22, 2018 •

edited

Loading