[MXNET-244] Fixed armv7 wheel #11053

lebeg · 2018-05-24T15:34:33Z

Description

Fixes a path issue in the created by cross compilation python wheel for armv7 (RPi).

Checklist

Essentials

This PR starts with MXNET-244
Changes are complete
Build tests are already in place

Changes

Updated OpenBLAS build to align with other arm builds
Extracted a build_wheel function

larroy · 2018-05-24T15:59:40Z

LGTM

marcoabreu · 2018-05-24T21:14:54Z

ci/docker/runtime_functions.sh

@@ -31,31 +31,50 @@ clean_repo() {
    git submodule update --init --recursive
 }

+build_wheel() {


Great idea to have it centralized!

marcoabreu · 2018-05-24T21:16:32Z

ci/docker/runtime_functions.sh

-        -DCMAKE_BUILD_TYPE=RelWithDebInfo\
-        -DUSE_MKL_IF_AVAILABLE=OFF\
+
+    # Lapack functionality will be included and statically linked to openblas.


This sounds a bit like a hack to me. Could we have proper handling for that? Otherwise, there might be other things that depend on the USE_LAPACK flag that are not getting triggered because it's expected to be absent while it's actually there.

Sure, I already tried that and this will not be easy. I suggest this will be part of a separate PR.

I'd prefer to have this within this PR as otherwise would leave the two branches in a semi-consistent state in terms of the USE_LAPACK flag until you follow up with the second PR.

As I've said already, it is not a trivial task and can take up significant time and effort with functionality change impacting multiple builds. This PR is about fixing the ARMv7 build, for now without improvements. It is a big comment and only a few lines to keep in sync, so that it can be considered as tolerated evil. There is already a ticket to track the work on the problem: MXNET-115

Alright. Since I'm not very familiar with the impact of the LAPACK-Flag (or it being absent), maybe @szha @eric-haibin-lin @piiswrong could help out here?

Static linking goes with openblas, but fortran needs to be linked dynamically. Comment about -static-libgfortran not working properly

Anyway we will look into the mentioned issue above, no doubt. Do you guys see it as possible to introduce the improvement in a follow up PR?

@larroy I was talking about this code: https://github.com/apache/incubator-mxnet/blob/c4a76da62fbc5e3e7272fd89294fba6b22868bba/src/operator/c_lapack_api.h#L218

It seems like @lebeg and @larroy are both working on the USE_LAPACK flag. While this PR assumes the "wrong" behaviour of our build system to basically ignore the flag and set it to true(as described in the ticket), #11094 attempts to correct this. I think we should first get #11094 merged properly. After that, we actually have a USE_LAPACK behaviour that behaves like expected. This would allows us to remove the hacks we have in this PR (USE_LAPACK=0) and thus reach a clean state.

If we merge this PR first and #11094 gets merged afterwards, the LAPACK build is broken again. I'd prefer a clean solution over multiple PRs about the same issue.

As discussed, I'm working on a fix that will be introduced as a separate PR

marcoabreu · 2018-05-28T18:52:28Z

ci/docker/Dockerfile.build.arm64

@@ -27,6 +27,10 @@ ENV FC /usr/bin/${CROSS_TRIPLE}-gfortran
 ENV HOSTCC gcc
 ENV TARGET ARMV8

+RUN apt-get update && \
+    apt-get install -y unzip && \
+    rm -rf /var/lib/apt/lists/*


rm -rf for apt lists is deprecated

Thanks, fixed

marcoabreu · 2018-05-28T18:52:44Z

ci/docker/Dockerfile.build.armv6

@@ -25,6 +25,10 @@ ENV FC=/usr/bin/${CROSS_TRIPLE}-gfortran
 ENV HOSTCC gcc
 ENV TARGET ARMV6

+RUN apt-get update && \
+    apt-get install -y unzip && \
+    rm -rf /var/lib/apt/lists/*


rm -rf for apt lists is deprecated

marcoabreu · 2018-05-28T18:53:00Z

ci/docker/Dockerfile.build.jetson

@@ -31,6 +31,10 @@ ENV FC /usr/bin/${CROSS_TRIPLE}-gfortran
 ENV HOSTCC gcc
 ENV TARGET ARMV8

+RUN apt-get update && \
+    apt-get install -y unzip && \
+    rm -rf /var/lib/apt/lists/*


rm -rf for apt lists is deprecated

lebeg · 2018-05-29T07:33:48Z

@szha @marcoabreu can you have another look?

marcoabreu · 2018-05-31T16:06:30Z

ci/docker/Dockerfile.build.arm64

@@ -27,6 +27,8 @@ ENV FC /usr/bin/${CROSS_TRIPLE}-gfortran
 ENV HOSTCC gcc
 ENV TARGET ARMV8

+RUN apt update && apt install -y unzip


Could we move this part (as well as the OpenBLAS compilation) for all ARM related Dockerfiles into separate scripts so that they are centralized in the install folder? As far as I can tell, they are all the same for the dependency installation (only unzip in this case) as well as for the OpenBLAS compilation.

marcoabreu · 2018-06-03T14:54:29Z

ci/docker/install/install_openblas.sh

@@ -0,0 +1,30 @@
+#!/usr/bin/env bash


Please rename file to match the style of our filenames. "install_platform_action"

Platform is universal, and none of the file names have a install prefix. That gives "action" as the only needed suffix. What exactly the name you are suggesting in this case?

Sorry, it was supposed to be "install/platform_action".

I'd propose arm_openblas.sh

It does not contain anything related to arm, even if we use it only for such cases.

Well I would have proposed something related to cross-compilation, but that might be confusing. You're right that it's not arm specific, but neither is it general :/

I would prefer to keep it that way, would that be alright?

I would prefer a rename since this script is not general purpose. It's only applicable for crosscompilation. Your proposed naming would indicate that it's usable for everything.

Additionally, we only use it for crosscompilation on ARM as of now, so we should mark it accordingly. If we expand the scope later on, we're free to rename it again. For now, it's limited to the crosscompilation scope of ARM and should thus be properly named.

What do you see in this script as not general purpose?

PREFIX=${CROSS_ROOT} make install

Ok, I see thanks

marcoabreu · 2018-06-03T14:55:12Z

ci/docker/install/ubuntu_arm.sh

@@ -0,0 +1,24 @@
+#!/usr/bin/env bash


Please rename file to match our filenaming style.

What exactly the name you are suggesting in this case?

Nevermind, this is fine :)

marcoabreu · 2018-06-03T14:56:25Z

ci/docker/runtime_functions.sh

-        -DCMAKE_BUILD_TYPE=RelWithDebInfo\
-        -DUSE_MKL_IF_AVAILABLE=OFF\
+
+    # Lapack functionality will be included and statically linked to openblas.


larroy · 2018-06-04T12:32:36Z

LGTM. CI is failing though. I retriggered it.

marcoabreu · 2018-06-04T14:31:44Z

Please rename the install-openblas-file, then we're good to go

larroy · 2018-06-06T11:07:27Z

CMakeLists.txt

+  if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/3rdparty/openmp/CMakeLists.txt
+     AND SYSTEM_ARCHITECTURE STREQUAL "x86_64"
+     AND NOT MSVC
+     AND NOT CMAKE_CROSSCOMPILING)


How is CMAKE_CROSSCOMPILING enabled/governed? For android I noticed we are not in "cross compiling" mode or whatever that is supposed to be in cmake.

Ah, saw it below.

larroy · 2018-06-06T11:08:07Z

ci/docker/Dockerfile.build.arm64

-    cd OpenBLAS && \
-    make -j$(nproc) && \
-    PREFIX=${CROSS_ROOT} make install
+COPY install/ubuntu_arm.sh /work/


do we need to copy the scripts vs running them directly from the mount volume?

Yes we have to copy them because the volumes are not mounted during build time of the container

larroy · 2018-06-06T11:08:44Z

ci/docker/runtime_functions.sh

+
+    cmake \
+        -DCMAKE_TOOLCHAIN_FILE=${CMAKE_TOOLCHAIN_FILE} \
+        -DCMAKE_CROSSCOMPILING=ON \


larroy · 2018-06-06T11:09:28Z

I would like to see this merged. What else is missing?

marcoabreu · 2018-06-06T11:49:24Z

"Please rename the install-openblas-file, then we're good to go"

lebeg · 2018-06-06T14:03:00Z

I'd prefer to keep it that way, would that be alright?

marcoabreu · 2018-06-06T17:59:29Z

Huh, how come we're adding Makefile configuration as well now? I think one buildsystem is enough, right? Also, it's not tested so I'd prefer if you remove it again

lebeg · 2018-06-07T08:23:22Z

I have tested it to try the failure on the device. I confirm that the build works with make and cmake.

marcoabreu · 2018-06-07T10:12:46Z

I think we should stick to one. So either remove cmake or make. The other one will be untested otherwise

lebeg · 2018-06-07T10:19:50Z

It is there because we needed to build with make instead of cmake to be sure that the problems we have are not related to the build system. I suggest to remove the added file along with the others when the build will be ported to cmake completely. At the moment we have 2 working solutions for 2 of our working build systems.

marcoabreu · 2018-06-07T12:19:23Z

Sorry I might be missing something, but where exactly are we using that new make-config? For ARMv7, I can only see cmake being used.

lebeg · 2018-06-07T12:22:59Z

See here

marcoabreu · 2018-06-07T12:35:00Z

Yeah, but it's commented. Thus, it's not tested and it should be removed.

Please elaborate in which case we need two different ways to achieve the same thing.

lebeg · 2018-06-08T12:49:44Z

I have removed the file

larroy · 2018-06-08T12:55:48Z

Let's merge later from dev-arm. It includes your changes. Shall we close this PR?

lebeg · 2018-06-14T07:59:03Z

#11245

lebeg mentioned this pull request May 24, 2018

[MXNET-244] Fixed armv7 wheel (1.2.0 release) #11054

Merged

6 tasks

marcoabreu reviewed May 24, 2018

View reviewed changes

lebeg changed the title ~~Fixed armv7 wheel~~ [MXNET-244] Fixed armv7 wheel May 25, 2018

lebeg force-pushed the armv7_fix branch from 8dd6810 to 758a277 Compare May 25, 2018 17:13

marcoabreu reviewed May 28, 2018

View reviewed changes

lebeg force-pushed the armv7_fix branch from 758a277 to bbff93d Compare May 29, 2018 07:32

marcoabreu reviewed May 31, 2018

View reviewed changes

marcoabreu suggested changes Jun 3, 2018

View reviewed changes

lebeg force-pushed the armv7_fix branch from f56ee47 to b2c8704 Compare June 5, 2018 16:37

larroy reviewed Jun 6, 2018

View reviewed changes

lebeg added 7 commits June 8, 2018 12:55

Fixed armv7 wheel

e85ee93

Fixed bash default value substitution

a6f2e8b

Fixed whitespace errors

a92e45d

Added unzip installation to crosscompile docker images

dce641a

Removed deprecated apt lists removal

6aa6840

Moved common parts to scripts for docker builds

be5441b

Disabled bundled OpenMP for cross compilation

94a8f4f

lebeg added 3 commits June 8, 2018 12:56

Added make based build for armv7 as option

b55e867

Renamed install_openblas.sh -> arm_openblas.sh

7324772

Removed makefile build for armv7

6e0b64b

lebeg force-pushed the armv7_fix branch from c1ad974 to 6e0b64b Compare June 8, 2018 12:57

lebeg closed this Jun 8, 2018

[MXNET-244] Fixed armv7 wheel #11053

[MXNET-244] Fixed armv7 wheel #11053

Conversation

lebeg commented May 24, 2018 • edited Loading

Description

Checklist

Essentials

Changes

larroy commented May 24, 2018

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

marcoabreu May 31, 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

lebeg commented May 29, 2018

marcoabreu May 31, 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

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

larroy commented Jun 4, 2018 • edited Loading

marcoabreu commented Jun 4, 2018

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

larroy commented Jun 6, 2018

marcoabreu commented Jun 6, 2018

lebeg commented Jun 6, 2018

marcoabreu commented Jun 6, 2018 • edited Loading

lebeg commented Jun 7, 2018

marcoabreu commented Jun 7, 2018

lebeg commented Jun 7, 2018

marcoabreu commented Jun 7, 2018 • edited Loading

lebeg commented Jun 7, 2018

marcoabreu commented Jun 7, 2018

lebeg commented Jun 8, 2018

larroy commented Jun 8, 2018

lebeg commented Jun 14, 2018

lebeg commented May 24, 2018 •

edited

Loading

marcoabreu May 31, 2018 •

edited

Loading

marcoabreu May 31, 2018 •

edited

Loading

larroy commented Jun 4, 2018 •

edited

Loading

marcoabreu commented Jun 6, 2018 •

edited

Loading

marcoabreu commented Jun 7, 2018 •

edited

Loading