This repository has been archived by the owner on Nov 17, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[DOC] Update ubuntu install instructions from source #14534
[DOC] Update ubuntu install instructions from source #14534
Changes from all commits
3367b7d
7024cf1
beab7ab
1b8821e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What is the implication for older Ubuntu OS? Is there a version >= that must be met?
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.
For older versions the distribution's provided CMake is working.
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.
Should we clarify that in the doc?
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.
No, I guess it is fine if there is no action required.
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.
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?
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.
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.
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.
Should put something here instead of having two code blocks back to back... some description...
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.
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?
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.
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.
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.
-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.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.
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.
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.
@TaoLv happy if you can help with MKL and CMake: #14670
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.
All MKL is disabled, check the end of the line: https://github.com/apache/incubator-mxnet/blob/master/CMakeLists.txt#L31