-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Fix and clean up Ubuntu build from source instructions #17229
Conversation
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.
Changes look reasonable and instructions work.
Is the verification of this instruction not automated? I thought we had automatic verification for it which wouldn't allow it to be broken |
There were multiple separate instructions. One of them are the CI scripts and those were verified. Other's were not verified and one of them was broken ( However, the CI scripts contain CI specific code and shouldn't be used on user systems. Thus they are removed with this PR and replaced by a simplified 3 step process. |
@szha I'm not aware of such automation, I think there's something for notebooks / tutorials, not for installation instructions. I could be wrong. |
Thanks a lot for cleaning up this instruction. The current one is so confusing and not working as well. One more thing is that I found current CMake still not working perfectly. I have a feature request here: #17180 |
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
@ptrendx This PR may be nice to backport to 1.6. What do you think? |
Website lives kind of on its own anyway, so there is no need to include this change in 1.6 package for it to apply to 1.6 release. I would be against including things that are not essential as it is very easy to just get into this loop of adding new shiny things one after another and never actually releasing the thing. |
@ptrendx isn't the website built from 1.6 branch? I agree it's not necessary to include the change in the release, but could be good to include for the website. |
|
||
```bash | ||
rm -rf build | ||
mkdir -p build && cd build | ||
cmake -GNinja \ | ||
~/.local/bin/cmake -GNinja \ |
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.
Somehow I found using this absolute path causes inconvenience as my 'cmake' is not installed on this path, nor is the default one on AWS DLAMI. So I can't just copy and paste this command to build mxnet. Do you have a better idea to not using a fixed path?
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 would be better to just use CMake and specify what's the minimum required version
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.
@larroy some users may not know how to understand cmake.
@apeforest DLAMI encourages users to use conda, which causes problems for above installation steps.
I'll open a PR to improve the experience shortly
Description
Fix and clean up Ubuntu build from source instructions.
Fixes #17226
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments
Justification for respective removals
ci/docker/install/ubuntu_{core,python}.sh
"quick" install: 1) Compile from Source instructions re-use CI install scripts #17226 2) Only supports Ubuntu 16.04./install_mxnet_ubuntu_python.sh
: Does not exist anymoredev_menu.py build
: Thedev_menu.py build
does things differently compared to the simple build instructions in this tutorial. For example, it uses virtualenv and contains other unrelated features such as building docker containers. I think it adds confusion for new users if we include it in the setup guide. (CC @larroy)