Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[MXNET-1224]: improve scala maven jni build. #13493

Merged
merged 1 commit into from
Dec 11, 2018
Merged

[MXNET-1224]: improve scala maven jni build. #13493

merged 1 commit into from
Dec 11, 2018

Conversation

frankfliu
Copy link
Contributor

@frankfliu frankfliu commented Nov 30, 2018 •
edited
Loading

Description

Scala jni shared library requires compile flags inherited from make. But there is no way to enforce those flags to be the same as libmxnet.so was built. And current Scala package is built is manually. This is error prone can has caused issues in past.

This fix is trying make scala jni library not depends on any mxnet build flags, and dynamically link libmxnet.so directly. libmxnet-scala.so file will be compiled dynamically link to libmxnet.so, and allows load libmxnet.so from the same directory. Scala code is also updated to extract libmxnet.so file into temp folder and load it temp folder.

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

@nswamy
Copy link
Member

nswamy commented Nov 30, 2018

@frankfliu could you please explain what you intend to do.

@vandanavk
Copy link
Contributor

@mxnet-label-bot add [Scala, pr-work-in-progress]

@marcoabreu marcoabreu added pr-work-in-progress PR is still work in progress Scala labels Nov 30, 2018
@lanking520 lanking520 self-requested a review December 3, 2018 18:23
@lanking520
Copy link
Member

@nswamy this PR is intended to drop the requirement to have USE_XXX flags applied in the scala build process. Through using libmxnet.so and not libmxnet.a, it would help to improve the build process to be simpler.

@frankfliu could you please check if you have the link for ps-lite?

@frankfliu frankfliu changed the title [WIP]: improve scala maven jni build and packing. [MXNET-1224]: improve scala maven jni build. Dec 4, 2018
nswamy
nswamy reviewed Dec 4, 2018
View reviewed changes
scala-package/init-native/linux-x86_64/pom.xml
<compilerEndOption>-I${project.basedir}/../../../include</compilerEndOption>
<compilerEndOption>${all_includes}</compilerEndOption>
<compilerEndOption>${cflags}</compilerEndOption>
<compilerEndOption>-I${MXNET_DIR}/include</compilerEndOption>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the C_FLAGS and LD_FLAGS defined and passed from the Makefile

andrewfayres
andrewfayres approved these changes Dec 5, 2018
View reviewed changes
Copy link
Contributor

@andrewfayres andrewfayres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I really like these changes!

zachgk
zachgk approved these changes Dec 6, 2018
View reviewed changes
scala-package/pom.xml Outdated
@@ -39,6 +39,8 @@
<scala.version>2.11.8</scala.version>
<scala.binary.version>2.11</scala.binary.version>
<build.platform />
<cxx>g++</cxx>
<dollor>$</dollor>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be dollar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, will fix this in next PR with maven build improvement.

piyushghai
piyushghai approved these changes Dec 6, 2018
View reviewed changes
Copy link
Contributor

@piyushghai piyushghai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contributions @frankfliu !

lanking520
lanking520 reviewed Dec 9, 2018
View reviewed changes
Copy link
Member

@lanking520 lanking520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @frankfliu I am still not confident to merge this PR, could you please provide a short README in the Scala package explains the backend build procedure:

    1. the reason you set these dependencies and why we limited to only dlmc, mshadow, dlpack? If I want to build with MKLDNN what will happen?
    1. Structure of the result jar file

scala-package/init-native/osx-x86_64/pom.xml
<compilerEndOption>-I${MXNET_DIR}/3rdparty/dlpack/include</compilerEndOption>
<compilerEndOption>-I${MXNET_DIR}/3rdparty/tvm/nnvm/include</compilerEndOption>
<compilerEndOption>-DMSHADOW_USE_MKL=0 -DMSHADOW_USE_CUDA=0</compilerEndOption>
<compilerEndOption>-g -O0 -fPIC -msse3 -mf16c</compilerEndOption>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we are using these build options? What are these for?

scala-package/init-native/linux-x86_64/pom.xml
<compilerEndOption>-I${MXNET_DIR}/3rdparty/mshadow</compilerEndOption>
<compilerEndOption>-I${MXNET_DIR}/3rdparty/dlpack/include</compilerEndOption>
<compilerEndOption>-I${MXNET_DIR}/3rdparty/tvm/nnvm/include</compilerEndOption>
<compilerEndOption>-DMSHADOW_USE_MKL=0 -DMSHADOW_USE_CUDA=0</compilerEndOption>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we make them =0, if we build with GPU what will happened?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a README.md file to describe compiler and linker flags.

@lanking520
Copy link
Member

@marcoabreu looks like the CI passed however not reflect on this PR?

lanking520
lanking520 approved these changes Dec 11, 2018
View reviewed changes
Copy link
Member

@lanking520 lanking520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the readme and that make things clear! LGTM!

piyushghai
piyushghai approved these changes Dec 11, 2018
View reviewed changes
Copy link
Contributor

@piyushghai piyushghai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The readme is super useful!
Thanks @frankfliu

@lanking520 lanking520 merged commit b242b0c into apache:master Dec 11, 2018
zhaoyao73 added a commit to zhaoyao73/incubator-mxnet that referenced this pull request Dec 13, 2018
@zhaoyao73
Merge remote-tracking branch 'upstream/master'
76e901a 
* upstream/master: (38 commits)
  Feature/mkldnn static (apache#13628)
  Fix the bug of BidirectionalCell (apache#13575)
  Set install path for libmxnet.so dynamic lib on Mac OS (apache#13629)
  add batch norm test (apache#13625)
  Scripts for building dependency libraries of MXNet (apache#13282)
  fix quantize pass error when the quantization supported Op are excluded in the model (apache#13596)
  Optimize C++ API (apache#13496)
  Fix warning in waitall doc (apache#13618)
  [MXNET-1225] Always use config.mk in make install instructions (apache#13364)
  [MXNET-1224]: improve scala maven jni build and packing. (apache#13493)
  [MXNET-1155] Add scala packageTest utility (apache#13046)
  fix the Float not showing correctly problem (apache#13617)
  apache#13385 [Clojure] - Turn examples into integration tests (apache#13554)
  Add Intel MKL blas to Jenkins (apache#13607)
  Revert "[MXNET-1198] MXNet Java API (apache#13162)"
  Reducing the length of setup tutorial (apache#13306)
  [MXNET-1182] Predictor example (apache#13237)
  [MXNET-1187] Added Java SSD Inference Tutorial for website (apache#13201)
  add defaults and clean up the tests (apache#13295)
  [MXNET-1181] Added command line alternative to IntelliJ in install instructions (apache#13267)
  ...
lanking520 pushed a commit to lanking520/incubator-mxnet that referenced this pull request Jan 3, 2019
@frankfliu @lanking520
[MXNET-1224]: improve scala maven jni build and packing. (apache#13493)
059723e 
Major JNI feature changes. Please find more info here: https://cwiki.apache.org/confluence/display/MXNET/Scala+maven+build+improvement
@frankfliu frankfliu deleted the jni branch January 9, 2019 17:08
@zachgk zachgk mentioned this pull request Jan 12, 2019
Closed
lanking520 pushed a commit to lanking520/incubator-mxnet that referenced this pull request Apr 26, 2019
@frankfliu @lanking520
[MXNET-1224]: improve scala maven jni build and packing. (apache#13493)
647c7ae 
Major JNI feature changes. Please find more info here: https://cwiki.apache.org/confluence/display/MXNET/Scala+maven+build+improvement
lanking520 pushed a commit to lanking520/incubator-mxnet that referenced this pull request Apr 26, 2019
@frankfliu @lanking520
[MXNET-1224]: improve scala maven jni build and packing. (apache#13493)
2e226ac 
Major JNI feature changes. Please find more info here: https://cwiki.apache.org/confluence/display/MXNET/Scala+maven+build+improvement
lanking520 pushed a commit to lanking520/incubator-mxnet that referenced this pull request Apr 30, 2019
@frankfliu @lanking520
[MXNET-1224]: improve scala maven jni build and packing. (apache#13493)
9d22e4a 
Major JNI feature changes. Please find more info here: https://cwiki.apache.org/confluence/display/MXNET/Scala+maven+build+improvement
zachgk pushed a commit to zachgk/incubator-mxnet that referenced this pull request May 16, 2019
@frankfliu @zachgk
[MXNET-1224]: improve scala maven jni build and packing. (apache#13493)
79d8915 
Major JNI feature changes. Please find more info here: https://cwiki.apache.org/confluence/display/MXNET/Scala+maven+build+improvement
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nswamy nswamy nswamy left review comments

@zachgk zachgk zachgk approved these changes

@piyushghai piyushghai piyushghai approved these changes

@lanking520 lanking520 lanking520 approved these changes

@andrewfayres andrewfayres andrewfayres approved these changes

@yzhliu yzhliu Awaiting requested review from yzhliu yzhliu is a code owner

Assignees
No one assigned
Labels
pr-work-in-progress PR is still work in progress Scala
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@frankfliu @nswamy @vandanavk @lanking520 @zachgk @piyushghai @andrewfayres @marcoabreu