[DOC] Update ubuntu install instructions from source by larroy · Pull Request #14534 · apache/mxnet

larroy · 2019-03-26T19:55:04Z

Description

This PR updates ubuntu install instructions by using CMake and adding steps needed to update CMake on ubuntu 18.04 otherwise developers get nontrivial error messages.

@aaronmarkham

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

abhinavs95 · 2019-03-26T21:33:04Z

@mxnet-label-bot add [Doc, Installation, pr-awaiting-review]

TaoLv · 2019-03-27T15:14:32Z

docs/install/ubuntu_setup.md

+        -DCMAKE_C_COMPILER_LAUNCHER=ccache \
+        -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \
+    ..
+    ninja


-DUSE_MKLDNN will be turned on by default in this cmake line? Seems we need change the description in the next section and document the default behavior somewhere.

MKL install requires additional steps in Ubuntu that are not part of the installation, so it's disabled. by -DUSE_MKL_IF_AVAILABLE=OFF even if any other MKL variables are on.

@TaoLv happy if you can help with MKL and CMake: #14670

All MKL is disabled, check the end of the line: https://github.com/apache/incubator-mxnet/blob/master/CMakeLists.txt#L31

TaoLv · 2019-03-27T15:16:02Z

docs/install/ubuntu_setup.md

 ```bash
-export LD_LIBRARY_PATH=$PWD/lib
+export LD_LIBRARY_PATH=`realpath build`
+export MXNET_LIBRARY_PATH=`realpath build/libmxnet.so`


When will this env variable be used?

MXNET_LIBRARY_PATH when loading the library from python.

But why do we need this? It seems to work fine without having to set this env variable earlier.

It was there in the instructions already. I agree is not necessary.

It was there in the instructions already. I agree is not necessary.
I could not find it in the instructions before this PR. If not necessary, can we not add it to the doc?

Sure. I'm indifferent. While it can help debugging we can remove to keep the instructions lean. As you can see in the patch it was there already.

aaronmarkham

Some minor suggestions.

docs/install/ubuntu_setup.md

aaronmarkham · 2019-04-03T18:23:15Z

docs/install/ubuntu_setup.md

+    sudo apt-get install -y build-essential git ninja-build ccache
 ```

+**For Ubuntu 18.04 and CUDA builds you need to update CMake**


What is the implication for older Ubuntu OS? Is there a version >= that must be met?

For older versions the distribution's provided CMake is working.

Should we clarify that in the doc?

No, I guess it is fine if there is no action required.

aaronmarkham · 2019-04-03T18:24:31Z

docs/install/ubuntu_setup.md

+sudo apt remove --purge --auto-remove cmake
+
+# Update CMAKE for correct cuda autotedetection: https://github.com/clab/dynet/issues/1457
+version=3.14


Putting in version number in the docs makes them go stale pretty fast. Is there a way to consolidate versions in a file so it can be managed there instead of sprinkled throughout code blocks in the docs?

I think this is fine. This is the minimum version that works with ubuntu 18.04 I think is ok to put it there, and people can update it. If you put an indirect reference becomes more messy to maintain.

aaronmarkham · 2019-04-03T18:26:31Z

docs/install/ubuntu_setup.md

-    echo "USE_BLAS = openblas" >> ./config.mk
-    make -j $(nproc)
+```
+


Should put something here instead of having two code blocks back to back... some description...

piyushghai · 2019-04-09T00:52:47Z

@larroy Is this PR good to merge now ?

docs/install/ubuntu_setup.md

apeforest · 2019-04-09T06:13:36Z

docs/install/ubuntu_setup.md

+    cmake -GNinja \
+        -DUSE_CUDA=OFF \
+        -DUSE_MKL_IF_AVAILABLE=OFF \
+        -DCMAKE_CUDA_COMPILER_LAUNCHER=ccache \


I am not an expert in cmake but is ccache a de facto build option for cmake build system? Can we leave this option to user?

needs to be enabled explicitly with this flag, is not default. I think we should enable it in our instructions to speed up development process. Otherwise it becomes an obscure trick.

larroy · 2019-04-11T00:14:52Z

@piyushghai up to reviewers to approve or follow up on the changes / made additional requests. @apeforest @aaronmarkham

aaronmarkham

I'm ok with the current changes here.

apeforest

Looks fine to me. Just one small comment added. Thanks!

Co-Authored-By: larroy <pedro.larroy.lists@gmail.com>

aaronmarkham · 2019-04-19T19:51:17Z

Restarted CI (Julia test failed for some reason... let's see if gets though this time...)

larroy · 2019-04-24T19:19:17Z

@aaronmarkham @apeforest are we good to merge?

* Update ubuntu install instructions from source * Update docs/install/ubuntu_setup.md Co-Authored-By: larroy <pedro.larroy.lists@gmail.com> * Address CR comments * Address CR comments

larroy requested a review from szha as a code owner March 26, 2019 19:55

larroy force-pushed the ubuntu_doc branch from e3a92c2 to 8e85178 Compare March 26, 2019 20:23

larroy changed the title ~~Update ubuntu install instructions from source~~ [DOC] Update ubuntu install instructions from source Mar 26, 2019

marcoabreu added Doc Installation pr-awaiting-review PR is waiting for code review labels Mar 26, 2019

TaoLv reviewed Mar 27, 2019

View reviewed changes

larroy force-pushed the ubuntu_doc branch from 8e85178 to 12eed6d Compare April 2, 2019 07:05

aaronmarkham reviewed Apr 3, 2019

View reviewed changes

apeforest reviewed Apr 9, 2019

View reviewed changes

docs/install/ubuntu_setup.md Show resolved Hide resolved

apeforest reviewed Apr 9, 2019

View reviewed changes

docs/install/ubuntu_setup.md Outdated Show resolved Hide resolved

apeforest reviewed Apr 9, 2019

View reviewed changes

larroy force-pushed the ubuntu_doc branch from afbae5e to af037d6 Compare April 11, 2019 00:14

aaronmarkham approved these changes Apr 15, 2019

View reviewed changes

aaronmarkham requested a review from apeforest April 15, 2019 17:20

apeforest reviewed Apr 15, 2019

View reviewed changes

larroy and others added 4 commits April 15, 2019 15:25

Update ubuntu install instructions from source

3367b7d

Update docs/install/ubuntu_setup.md

7024cf1

Co-Authored-By: larroy <pedro.larroy.lists@gmail.com>

Address CR comments

beab7ab

Address CR comments

1b8821e

larroy force-pushed the ubuntu_doc branch from af037d6 to 1b8821e Compare April 15, 2019 22:41

aaronmarkham merged commit 014ca13 into apache:master Apr 24, 2019

Conversation

larroy commented Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

Uh oh!

abhinavs95 commented Mar 26, 2019

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 Apr 15, 2019 • 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!

aaronmarkham left a comment

Choose a reason for hiding this comment

Uh oh!

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!

piyushghai commented Apr 9, 2019

Uh oh!

Uh oh!

Uh oh!

apeforest Apr 9, 2019 • 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!

larroy Apr 11, 2019 • 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!

larroy commented Apr 11, 2019

Uh oh!

aaronmarkham left a comment

Choose a reason for hiding this comment

Uh oh!

apeforest left a comment

Choose a reason for hiding this comment

Uh oh!

aaronmarkham commented Apr 19, 2019

Uh oh!

larroy commented Apr 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

larroy commented Mar 26, 2019 •

edited

Loading

larroy Apr 15, 2019 •

edited

Loading

apeforest Apr 9, 2019 •

edited

Loading

larroy Apr 11, 2019 •

edited

Loading