[GitHub] larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds
larroy commented on a change in pull request #11096: [MXNET-472] ccache for
docker builds
URL: https://github.com/apache/incubator-mxnet/pull/11096#discussion_r192722449
##
File path: ci/docker/runtime_functions.sh
##
@@ -178,6 +225,8 @@ build_centos7_cpu() {
build_centos7_mkldnn() {
set -ex
cd /work/mxnet
+export CC="ccache gcc"
+export CXX="ccache g++"
Review comment:
Is not needed for CPU builds, I added a comment about this somewhere in my
original changes.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds
larroy commented on a change in pull request #11096: [MXNET-472] ccache for
docker builds
URL: https://github.com/apache/incubator-mxnet/pull/11096#discussion_r192722216
##
File path: ci/docker/runtime_functions.sh
##
@@ -31,14 +31,49 @@ clean_repo() {
git submodule update --init --recursive
}
+# wrap compiler calls with ccache
+build_ccache_wrappers() {
+set -ex
+
+rm -f cc
+rm -f cxx
+
+touch cc
+touch cxx
+
+if [ -z ${CC+x} ]; then
+echo "No \$CC set, defaulting to gcc";
+export CC=gcc
+fi
+
+if [ -z ${CXX+x} ]; then
+ echo "No \$CXX set, defaulting to g++";
+ export CXX=g++
+fi
+
+# this function is nessesary for cuda enabled make based builds, since
nvcc needs just an executable for -ccbin
+
+echo -e "#!/bin/sh\n/usr/local/bin/ccache ${CC} \"\$@\"\n" >> cc
+echo -e "#!/bin/sh\n/usr/local/bin/ccache ${CXX} \"\$@\"\n" >> cxx
+
+chmod +x cc
+chmod +x cxx
+
+export CC=`pwd`/cc
+export CXX=`pwd`/cxx
+}
# Build commands: Every platform in docker/Dockerfile.build. should
have a corresponding
# function here with the same suffix:
build_jetson() {
set -ex
pushd .
-mv make/crosscompile.jetson.mk make/config.mk
+
+build_ccache_wrappers
+
+cp -f make/crosscompile.jetson.mk ./config.mk
Review comment:
This is fine, check the Makefile of mxnet.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds
larroy commented on a change in pull request #11096: [MXNET-472] ccache for
docker builds
URL: https://github.com/apache/incubator-mxnet/pull/11096#discussion_r192721946
##
File path: ci/docker/runtime_functions.sh
##
@@ -31,14 +31,49 @@ clean_repo() {
git submodule update --init --recursive
}
+# wrap compiler calls with ccache
+build_ccache_wrappers() {
+set -ex
+
+rm -f cc
+rm -f cxx
+
+touch cc
+touch cxx
+
+if [ -z ${CC+x} ]; then
+echo "No \$CC set, defaulting to gcc";
+export CC=gcc
+fi
+
+if [ -z ${CXX+x} ]; then
+ echo "No \$CXX set, defaulting to g++";
+ export CXX=g++
+fi
+
+# this function is nessesary for cuda enabled make based builds, since
nvcc needs just an executable for -ccbin
+
+echo -e "#!/bin/sh\n/usr/local/bin/ccache ${CC} \"\$@\"\n" >> cc
Review comment:
Agree, I think we should use > doesn't make sense to append in case the
file already exist does it?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds
larroy commented on a change in pull request #11096: [MXNET-472] ccache for
docker builds
URL: https://github.com/apache/incubator-mxnet/pull/11096#discussion_r192721558
##
File path: ci/docker/install/centos7_install_ccache.sh
##
@@ -0,0 +1,53 @@
+#!/bin/bash
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Script to build ccache for centos7 based images
+
+set -ex
+
+pushd .
+
+yum -y install epel-release
+yum -y install git
+yum -y install autoconf
+yum -y install wget
+yum -y install make
+yum -y install google-perftools
+yum -y install asciidoc
+yum -y install gcc-c++-4.8.*
+
+mkdir -p /work/deps
+cd /work/deps
+
+git clone --recursive -b v3.4.2 https://github.com/ccache/ccache.git
+
+cd ccache
+
+./autogen.sh
+./configure
+make -j$(nproc)
+make install
+
+cd /work/deps
+rm -rf /work/deps/ccache
+
+popd
+
+export CCACHE_MAXSIZE=${CCACHE_MAXSIZE:=10G}
+export CCACHE_DIR=${CCACHE_DIR:=/work/ccache}
Review comment:
Agree with @marcoabreu on this one, at least that's my experience.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds
larroy commented on a change in pull request #11096: [MXNET-472] ccache for
docker builds
URL: https://github.com/apache/incubator-mxnet/pull/11096#discussion_r192721481
##
File path: ci/docker/install/centos7_install_ccache.sh
##
@@ -0,0 +1,53 @@
+#!/bin/bash
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Script to build ccache for centos7 based images
+
+set -ex
+
+pushd .
+
+yum -y install epel-release
+yum -y install git
+yum -y install autoconf
+yum -y install wget
+yum -y install make
+yum -y install google-perftools
+yum -y install asciidoc
+yum -y install gcc-c++-4.8.*
+
+mkdir -p /work/deps
+cd /work/deps
+
+git clone --recursive -b v3.4.2 https://github.com/ccache/ccache.git
+
+cd ccache
+
+./autogen.sh
+./configure
+make -j$(nproc)
+make install
+
+cd /work/deps
+rm -rf /work/deps/ccache
+
+popd
+
+export CCACHE_MAXSIZE=${CCACHE_MAXSIZE:=10G}
Review comment:
I don't think this variable gets exported outside of this script. So it
won't be used. Am I wrong?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds
larroy commented on a change in pull request #11096: [MXNET-472] ccache for
docker builds
URL: https://github.com/apache/incubator-mxnet/pull/11096#discussion_r192721106
##
File path: ci/docker/Dockerfile.build.jetson
##
@@ -39,6 +47,9 @@ RUN git clone --recursive -b v0.2.20
https://github.com/xianyi/OpenBLAS.git && \
make -j$(nproc) && \
PREFIX=${CROSS_ROOT} make install
+ENV OpenBLAS_HOME=${CROSS_ROOT}
+ENV OpenBLAS_DIR=${CROSS_ROOT}
Review comment:
I also don't understand this one, can we add a comment on what do those
variables do?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds
larroy commented on a change in pull request #11096: [MXNET-472] ccache for
docker builds
URL: https://github.com/apache/incubator-mxnet/pull/11096#discussion_r192720757
##
File path: ci/build.py
##
@@ -127,12 +140,16 @@ def container_run(platform: str,
local_build_folder = buildir()
# We need to create it first, otherwise it will be created by the docker
daemon with root only permissions
os.makedirs(local_build_folder, exist_ok=True)
+os.makedirs(local_ccache_dir, exist_ok=True)
+logging.info("Using ccache directory: %s", local_ccache_dir)
runlist = [docker_binary, 'run', '--rm', '-t',
-'--shm-size={}'.format(shared_memory_size),
-'-v', "{}:/work/mxnet".format(mx_root), # mount mxnet root
-'-v', "{}:/work/build".format(local_build_folder), # mount mxnet/build
for storing build artifacts
-'-u', '{}:{}'.format(os.getuid(), os.getgid()),
-tag]
+ '--shm-size={}'.format(shared_memory_size),
+ '-v', "{}:/work/mxnet".format(mx_root), # mount mxnet root
+ '-v', "{}:/work/build".format(local_build_folder), # mount
mxnet/build for storing build artifacts
+ '-v', "{}:/work/ccache".format(local_ccache_dir),
+ '-u', '{}:{}'.format(os.getuid(), os.getgid()),
+ '-e', "CCACHE_DIR=/work/ccache", # this path is inside the
container as /work/ccache is mounted
Review comment:
default is 5G I think that's ok for now. We can tune it later.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds
larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds URL: https://github.com/apache/incubator-mxnet/pull/11096#discussion_r191979340 ## File path: ci/docker/install/ubuntu_install_ccache.sh ## @@ -0,0 +1,59 @@ +#!/bin/bash + +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Script to build ccache for ubuntu based images + +set -ex + +pushd . + +apt update +apt install -y --no-install-recommends \ +git \ Review comment: same comment above about "core" This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds
larroy commented on a change in pull request #11096: [MXNET-472] ccache for
docker builds
URL: https://github.com/apache/incubator-mxnet/pull/11096#discussion_r191979420
##
File path: ci/docker/runtime_functions.sh
##
@@ -31,14 +31,49 @@ clean_repo() {
git submodule update --init --recursive
}
+# wrap compiler calls with ccache
+build_ccache_wrappers() {
Review comment:
do we need wrappers if we compile the latest ccache ourselves? it should
have the fix for nvcc right?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds
larroy commented on a change in pull request #11096: [MXNET-472] ccache for
docker builds
URL: https://github.com/apache/incubator-mxnet/pull/11096#discussion_r191979310
##
File path: ci/docker/install/centos7_install_ccache.sh
##
@@ -0,0 +1,53 @@
+#!/bin/bash
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Script to build ccache for centos7 based images
+
+set -ex
+
+pushd .
+
+yum -y install epel-release
+yum -y install git
+yum -y install autoconf
+yum -y install wget
+yum -y install make
+yum -y install google-perftools
+yum -y install asciidoc
+yum -y install gcc-c++-4.8.*
+
+mkdir -p /work/deps
+cd /work/deps
+
+git clone --recursive -b v3.4.2 https://github.com/ccache/ccache.git
+
+cd ccache
+
+./autogen.sh
+./configure
+make -j$(nproc)
+make install
+
+cd /work/deps
+rm -rf /work/deps/ccache
+
+popd
+
+export CCACHE_MAXSIZE=${CCACHE_MAXSIZE:=10G}
Review comment:
who will pick up these variables? Shouldn't it be in the dockerfile or in
the python script?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds
larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds URL: https://github.com/apache/incubator-mxnet/pull/11096#discussion_r191979245 ## File path: ci/docker/install/centos7_install_ccache.sh ## @@ -0,0 +1,53 @@ +#!/bin/bash + +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Script to build ccache for centos7 based images + +set -ex + +pushd . + +yum -y install epel-release +yum -y install git +yum -y install autoconf +yum -y install wget +yum -y install make +yum -y install google-perftools +yum -y install asciidoc +yum -y install gcc-c++-4.8.* Review comment: isn't this part of centos7_core.sh . ? we should not duplicate it I think This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds
larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds URL: https://github.com/apache/incubator-mxnet/pull/11096#discussion_r191979201 ## File path: ci/docker/install/centos7_install_ccache.sh ## @@ -0,0 +1,53 @@ +#!/bin/bash + +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Script to build ccache for centos7 based images + +set -ex + +pushd . + +yum -y install epel-release +yum -y install git +yum -y install autoconf +yum -y install wget +yum -y install make +yum -y install google-perftools +yum -y install asciidoc +yum -y install gcc-c++-4.8.* + +mkdir -p /work/deps +cd /work/deps + +git clone --recursive -b v3.4.2 https://github.com/ccache/ccache.git Review comment: nice! This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds
larroy commented on a change in pull request #11096: [MXNET-472] ccache for docker builds URL: https://github.com/apache/incubator-mxnet/pull/11096#discussion_r191979046 ## File path: ci/docker/Dockerfile.build.android_arm64 ## @@ -18,9 +18,16 @@ # # Dockerfile to build MXNet for Android ARM64/ARMv8 +FROM ccache/build.ubuntu as ccachebuilder + +COPY install/ubuntu_install_ccache.sh /work/ +RUN /work/ubuntu_install_ccache.sh + FROM dockcross/base:latest MAINTAINER Pedro Larroy "pllar...@amazon.com" +COPY --from=ccachebuilder /usr/local/bin/ccache /usr/local/bin/ccache Review comment: what does this do? can we add a comment? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
