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

update docs to list cmake required for build from source page #12592

Merged
merged 2 commits into from
Sep 21, 2018

Conversation

azai91
Copy link
Contributor

@azai91 azai91 commented Sep 18, 2018

Description

https://mxnet.incubator.apache.org/install/build_from_source.html

currently our docs state that cmake is recommended but not required. in the process to migrate fully to cmake (and because it is required by some of our builds already) we will state that cmake is a prerequisite for building from source.

there were already two public disussions to remove cmake (one vote and one on github) so this transition is happening:
https://lists.apache.org/[email protected]:2018-6:cmake
#8702

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

Changes

  • move cmake to required section in build from source page.

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@azai91 azai91 requested a review from szha as a code owner September 18, 2018 18:59
@@ -16,6 +16,8 @@ This document explains how to build MXNet from source code. Building MXNet from

You need C++ build tools and a BLAS library to build the MXNet shared library. If you want to run MXNet with GPUs, you will need to install [NVDIA CUDA and cuDNN](https://developer.nvidia.com/cuda-downloads) first.

You may use [GNU Make](https://www.gnu.org/software/make/) to build the library but [cmake](https://cmake.org/) is required
Copy link
Member

Choose a reason for hiding this comment

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

can you elaborate why cmake is required? I am building from source just fine with config.mk and make

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when building with make with the MKLDNN, cmake it required: https://github.com/apache/incubator-mxnet/blob/master/mkldnn.mk#L40.

additionally there two discussions already to remove the makefile and make cmake default

https://lists.apache.org/[email protected]:2018-6:cmake
#8702

Copy link
Member

Choose a reason for hiding this comment

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

for GNU make, we can continue stay with MKLDNN=0 regardless of final discussion result. Anyways, I think it should help some users to skip those compiling tools in certain circumstances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the purpose of the vote was to eliminate the Makefile. if you think we should keep the Makefile, raise it in the email list. otherwise the direction we are heading is removing it completely and becoming dependent on cmake.

@stu1130
Copy link
Contributor

stu1130 commented Sep 19, 2018

Thanks for your contributions @azai91
@mxnet-label-bot[pr-awaiting-response]

@marcoabreu marcoabreu added the pr-awaiting-response PR is reviewed and waiting for contributor to respond label Sep 19, 2018
@azai91
Copy link
Contributor Author

azai91 commented Sep 19, 2018

@zhreshold updated the docs to specify that cmake is only necessary for MKLDNN.

Copy link
Contributor

@lupesko lupesko left a comment

Choose a reason for hiding this comment

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

Looks good.

@eric-haibin-lin eric-haibin-lin merged commit f73c1c7 into apache:master Sep 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-response PR is reviewed and waiting for contributor to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants