-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix broken links and reorganize build from source page #12962
Conversation
@aaronmarkham Thanks for the contribution! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one nit
docs/install/build_from_source.md
Outdated
|
||
These might be optional, but they're typically desirable as the extend or enhance MXNet's functionality. | ||
|
||
* [OpenCV](http://opencv.org/) - Image Loading and Augmentation. Each operation system has different packages and build from source options for OpenCV. Refer to your OS's link in the [Build Instructions by Operating System](#build-instructions-by-operating-system) section for further instructions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: operating system
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @roywei - fixed.
387c51a
to
168dc0a
Compare
@lebeg - Are the cmake flags in the examples alright? Should there be some others? |
|
||
```bash | ||
make USE_OPENCV=0 | ||
cmake -j USE_CUDA=1 USE_CUDA_PATH=/usr/local/cuda USE_CUDNN=1 USE_MKLDNN=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we usually give these default build options for user to use while building from source. Should we have a section describing different build options from here - https://github.com/apache/incubator-mxnet/blob/master/make/config.mk explaining what each of those do so that the user can make an informed personalized choice for himself? Provide the default options, but also add this section. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's interesting that we tend to promote using MKLDNN for better performance and in the CMAKE instructions, they imply that it is turned on by default.
However the config.mk has it turned off.
I thought it was supplied as a submodule and it is on by default. Is the config.mk out of date?
And yes, it should be mentioned what happens when you just run make
.
Seems like right now you get:
opencv, openmp (that doesn't work on mac out the box), atlas || apple
Looking this config.mk file actually makes things even more unclear...
@anirudhacharya Can you propose the logic that is followed and the recommended flags per general situation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a conversation here - https://lists.apache.org/thread.html/ca3c858a712a66f4b3443dfb395d84e8fdf2c86e297bcd28a3a36ebd@%3Cdev.mxnet.apache.org%3E on including mkl-dnn in mxnet default build.
I think the recommended flags seem okay for now, the way it is. As and when it is decided to include MKL-DNN in the default build, then the recommended flags can also be updated.
What I was suggesting was to have a section in the documentation that describes the flags already provided in the config.mk file( each environment variable has a comment describing what it does). Please let me know if I need to help in any way here, and I would be glad to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flags are good
I am wondering this if the build instructions are tested in CI to make sure we are aware of any breakage and users aren't surprised and frustrated. |
can you please share a screenshot on how the install selection will look like? |
@nswamy I added a preview link to the main description. I have a proposal for automated testing of installation instructions. That's all out of scope for this update though. I was just trying to make an incremental improvement to this page's info and add some examples for CMAKE since we didn't have any. The part I'm still worried about is this |
@nswamy ping! |
@aaronmarkham have you tested the cmake flags in the doc? |
I pulled the flags from the cmake PR. I haven't tested the variations. It would be great if the community provided tested/recommended cmake flags for the doc. |
|
||
```bash | ||
make USE_OPENCV=0 | ||
cmake -j USE_CUDA=1 USE_CUDA_PATH=/usr/local/cuda USE_CUDNN=1 USE_MKLDNN=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flags are good
* [non-Intel CPUs](#recommended-for-Systems-with-non-Intel-CPUs) | ||
2. [Install the language API binding(s)](#installing-mxnet-language-bindings) you would like to use for MXNet. | ||
MXNet's newest and most popular API is Gluon. Gluon is built into the Python binding. If Python isn't your preference, you still have more options. MXNet supports several other language APIs: | ||
- [Python (includes Gluon)](../api/python/index.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flow does not seem right. We are taking about Installing API bindings and we redirect the users to API page, we should redirect them to the language install page instead.
I had offline discussion with @aaronmarkham and expressed a concern, I am noting here that we suggest to use For example Scala still uses |
@lebeg is that the command we run in CI? |
@nswamy we had already discussions about this and moving to cmake exclusively is the way forward. At the time we are running both make and cmake in CI: runtime_functions.sh |
Here is a link to an old discussion: #8702 |
Discussion on @dev |
Thanks for sharing the links. I agree we should move to |
Description
This PR fixes broken links and updates the build from source page.
Preview
http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-12962/2/install/build_from_source.html
Comments
The examples now use
cmake
. Reviewers please verify that the switches' names didn't change for things like opencv and cuda and cudnn between cmake and make.