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

Fix and clean up Ubuntu build from source instructions #17229

Merged
merged 2 commits into from
Jan 7, 2020
Merged

Fix and clean up Ubuntu build from source instructions #17229

merged 2 commits into from
Jan 7, 2020

Conversation

leezu
Copy link
Contributor

@leezu leezu commented Jan 6, 2020 •
edited
Loading

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 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 https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Simplify and fix Ubuntu build from source instructions

Comments

Justification for respective removals

  • Binary Python install instructions: 1) The instructions were outdated and incomplete 2) This is a "Build From Source Guide". Why include binary package install instructions? 3) There are better instructions for the binary python install at the Get Started Page. Thus link to that page instead.
  • 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 anymore
  • dev_menu.py build: The dev_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)

@leezu leezu added Doc Installation pr-awaiting-review PR is waiting for code review labels Jan 6, 2020
szha
szha approved these changes Jan 7, 2020
View reviewed changes
Copy link
Member

@szha szha left a 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.

@szha
Copy link
Member

szha commented Jan 7, 2020

Is the verification of this instruction not automated? I thought we had automatic verification for it which wouldn't allow it to be broken

@leezu
Copy link
Contributor Author

leezu commented Jan 7, 2020 •
edited
Loading

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 (./install_mxnet_ubuntu_python.sh doesn't exist anymore).

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.

@larroy
Copy link
Contributor

larroy commented Jan 7, 2020

@szha I'm not aware of such automation, I think there's something for notebooks / tutorials, not for installation instructions. I could be wrong.

@apeforest
Copy link
Contributor

apeforest commented Jan 7, 2020 •
edited
Loading

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.
e.g. if I add flag -DUSE_NCCL, it does not work. I have to use make if I want to use NCCL with CUDA.
Issue: #17239
Also, it's not very user-friendly to type all different flags of cmake. It would be nice to have a setup.py like pytorch.

I have a feature request here: #17180

apeforest
apeforest approved these changes Jan 7, 2020
View reviewed changes
Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

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

LGTM

@leezu leezu merged commit ba376af into apache:master Jan 7, 2020
@leezu
Copy link
Contributor Author

leezu commented Jan 7, 2020

@ptrendx This PR may be nice to backport to 1.6. What do you think?

@ptrendx
Copy link
Member

ptrendx commented Jan 7, 2020

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.

@leezu
Copy link
Contributor Author

leezu commented Jan 7, 2020

@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.

apeforest
apeforest reviewed Jan 10, 2020
View reviewed changes
docs/static_site/src/pages/get_started/ubuntu_setup.md

```bash
rm -rf build
mkdir -p build && cd build
cmake -GNinja \
~/.local/bin/cmake -GNinja \
Copy link
Contributor

@apeforest apeforest Jan 10, 2020 •
edited
Loading

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?

Copy link
Contributor

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

Copy link
Contributor Author

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

@leezu leezu mentioned this pull request Jan 11, 2020
Merged
@leezu leezu deleted the fixsourcebuildinstructions branch September 28, 2020 18:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@larroy larroy larroy left review comments

@apeforest apeforest apeforest approved these changes

@szha szha szha approved these changes

@aaronmarkham aaronmarkham Awaiting requested review from aaronmarkham aaronmarkham is a code owner

Assignees
No one assigned
Labels
Doc Installation pr-awaiting-review PR is waiting for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compile from Source instructions re-use CI install scripts
5 participants
@leezu @szha @larroy @apeforest @ptrendx