[MXNET-57] Added support android64 by larroy · Pull Request #11055 · apache/mxnet

larroy · 2018-05-24T15:50:33Z

Description

Fixes for Android64
Added Android64 to CI

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://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

larroy · 2018-06-06T22:55:07Z

@marcoabreu @szha @piiswrong please merge

marcoabreu · 2018-06-06T23:02:19Z

+        -DUSE_OPENCV=OFF\
+        -DUSE_OPENMP=OFF\
+        -DUSE_SIGNAL_HANDLER=ON\
+        -DCMAKE_BUILD_TYPE=RelWithDebInfo\


To reduce the size, I'd propose we build without debug info for now

I want symbols to diagnose initial runs and crashes, there's a reason for this.

I'd propose that you set that flag locally then. We're referencing these instructions in our guides and to our customers, means we have to provide a ready-made solution. We had the same case previously when a customer complained about the size and in fact it was only the RelWithDebInfo.

I don't see how the points you are making about an experimental build which is not official nor tested are of any relevance at this stage of Android support. This is a development build and I want debug symbols to diagnose crashes, this should be reason enough to have it as it is, because size of build doesn't really matter right now, and debugability is more important.

We already spoke about this offline and I said that I'm fine with moving forward. Before you explaining it to me, I was under the impression that this would be the final stage.

marcoabreu · 2018-06-06T23:02:42Z

+        -DUSE_SSE=OFF\
+        -DUSE_LAPACK=OFF\
+        -DUSE_OPENCV=OFF\
+        -DUSE_OPENMP=OFF\


No open mp? Android phones usually have around 4 to 8 cores, so we should have it enabled

first we make a baseline version run, don't propose to overcomplicate things. Feel free to work on making openmp work in android to use all your cores if you have time.

Well we want to make a good first impression if people use it. If everything is slow the first time they test it, they most likely will not come back. OpenMP is very important, especially since we don't have any other hardware acceleration. Until that works, we can't really tell any customer to use it or we will lose creditability.

This is an initial build, what you are saying doesn't apply. Nobody should test this.

marcoabreu · 2018-06-06T23:03:13Z

+    export MXNET_LIBRARY_PATH=`pwd`/libmxnet.so
+    #cd /work/mxnet/python
+    #python setup.py bdist_wheel --universal
+    #cp dist/*.whl /work/build


Remove commented code

there will be more work to build a binary so these comments will go away, having another run of CI will take many hours and with flaky tests potentially days. I would suggest to merge this as it is.

Removed commented out code and added comments on flags that need further investigation

"CI will take many hours and with flaky tests potentially days" is not a valid reason.

Thank you.

szha · 2018-06-07T05:21:42Z

 mxnet_option(USE_CUDNN            "Build with cudnn support"  ON) # one could set CUDNN_ROOT for search path
-mxnet_option(USE_SSE              "Build with x86 SSE instruction support" ON)
+mxnet_option(USE_SSE              "Build with x86 SSE instruction support" ON IF NOT ARM)
 mxnet_option(USE_F16C             "Build with x86 F16C instruction support" ON) # autodetects support if ON


does ARM/android support F16C instruction?

AFAIK F16C is x86, so maybe only in android x86 but most android is ARM, and this is an ARM build ATM. We should add x86 android builds in the future, good point.

We should do an automatic scan in the future if the feature is present on the system and the compiler like for F16 in this PR.

Does this work also for cross compilation?

Can you give thumbs up or down to the review? Any more comments?

Sure, here you go)

szha · 2018-06-07T05:22:15Z

-mxnet_option(USE_SSE              "Build with x86 SSE instruction support" ON)
+mxnet_option(USE_SSE              "Build with x86 SSE instruction support" ON IF NOT ARM)
 mxnet_option(USE_F16C             "Build with x86 F16C instruction support" ON) # autodetects support if ON
-mxnet_option(USE_LAPACK           "Build with lapack support" ON IF NOT MSVC)


why is this relevant to android64?

this logic is buggy, so tries to always use LAPACK, and if I recall correctly is not available atm in android. See also: #11094

lebeg · 2018-06-11T09:15:11Z

 mxnet_option(USE_CUDNN            "Build with cudnn support"  ON) # one could set CUDNN_ROOT for search path
-mxnet_option(USE_SSE              "Build with x86 SSE instruction support" ON)
+mxnet_option(USE_SSE              "Build with x86 SSE instruction support" ON IF NOT ARM)
 mxnet_option(USE_F16C             "Build with x86 F16C instruction support" ON) # autodetects support if ON


Does this work also for cross compilation?

lebeg · 2018-06-11T09:16:50Z

+#COPY install/ubuntu_ccache.sh /work/
+#RUN /work/ubuntu_ccache.sh

 FROM dockcross/base:latest


Why not use dockcross/android-arm64 ?

Happy to do so in a subsequent PR, now this one passed. I would very much like to get it merged so at least we have this build as a gatekeeping mechanism.

larroy · 2018-06-12T10:17:10Z

@marcoabreu @piiswrong @szha please merge

marcoabreu · 2018-06-12T16:16:23Z

+#COPY install/ubuntu_core.sh /work/
+#RUN /work/ubuntu_core.sh
+#COPY install/ubuntu_ccache.sh /work/
+#RUN /work/ubuntu_ccache.sh


Could you elaborate?

Let's get the ARMv7 PR merged first and then I ammend this one and re-enable the ccache logic.

marcoabreu

LGTM besides the ccache removal

* Fixes sources for android64 Add initial android64 build logic. Remove pthread when linking MXNet in Android. Bionic provides built-in support for threads. * Simplify and fix android64 build

larroy · 2018-06-13T06:45:15Z

CI is giving problems when downloading CUDA?

http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-11055/23/pipeline

This is an adversarial environment to get code merged.

* Fixes sources for android64 Add initial android64 build logic. Remove pthread when linking MXNet in Android. Bionic provides built-in support for threads. * Simplify and fix android64 build

larroy requested a review from szha as a code owner May 24, 2018 15:50

larroy force-pushed the android branch from e61d32b to 717cd0a Compare May 24, 2018 16:13

larroy requested a review from marcoabreu as a code owner May 24, 2018 16:13

larroy changed the title ~~[WIP] Fixes for android64~~ [WIP] Added support android64 May 24, 2018

larroy force-pushed the android branch 3 times, most recently from 9f6e8f9 to 4dfd6d4 Compare May 24, 2018 23:11

szha requested review from yzhliu and removed request for szha May 25, 2018 00:32

larroy force-pushed the android branch from 4dfd6d4 to 5eac41f Compare May 25, 2018 11:04

larroy force-pushed the android branch 2 times, most recently from c8d943f to 05ef1df Compare June 6, 2018 18:09

larroy changed the title ~~[WIP] Added support android64~~ Added support android64 Jun 6, 2018

marcoabreu reviewed Jun 6, 2018

View reviewed changes

szha reviewed Jun 7, 2018

View reviewed changes

larroy force-pushed the android branch 5 times, most recently from 10a6af5 to a74fb07 Compare June 10, 2018 18:52

larroy changed the title ~~Added support android64~~ [MXNET-57] Added support android64 Jun 10, 2018

larroy force-pushed the android branch from 28b15f7 to 3ada6df Compare June 11, 2018 09:09

lebeg reviewed Jun 11, 2018

View reviewed changes

larroy force-pushed the android branch from 3ada6df to 8e3ce5f Compare June 11, 2018 23:16

marcoabreu reviewed Jun 12, 2018

View reviewed changes

marcoabreu approved these changes Jun 12, 2018

View reviewed changes

larroy changed the title ~~[MXNET-57] Added support android64~~ [WIP][MXNET-57] Added support android64 Jun 12, 2018

larroy force-pushed the android branch from 8e3ce5f to fd06ae5 Compare June 12, 2018 18:50

[MXNET-57] Add android64 build (apache#11188)

6446950

* Fixes sources for android64 Add initial android64 build logic. Remove pthread when linking MXNet in Android. Bionic provides built-in support for threads. * Simplify and fix android64 build

larroy force-pushed the android branch from fd06ae5 to 6446950 Compare June 12, 2018 18:52

larroy changed the title ~~[WIP][MXNET-57] Added support android64~~ [MXNET-57] Added support android64 Jun 12, 2018

marcoabreu merged commit ebf3777 into apache:master Jun 15, 2018

larroy deleted the android branch June 19, 2018 19:25

Conversation

larroy commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

Uh oh!

larroy commented Jun 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoabreu Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoabreu Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larroy commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoabreu left a comment

Choose a reason for hiding this comment

Uh oh!

larroy commented Jun 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

larroy commented May 24, 2018 •

edited

Loading

marcoabreu Jun 13, 2018 •

edited

Loading

marcoabreu Jun 7, 2018 •

edited

Loading

larroy commented Jun 12, 2018 •

edited

Loading