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

[RFC] Raising the toolchain requirements for MXNet 2 #17968

Closed
leezu opened this issue Apr 4, 2020 · 2 comments
Closed

[RFC] Raising the toolchain requirements for MXNet 2 #17968

leezu opened this issue Apr 4, 2020 · 2 comments

Comments

@leezu
Copy link
Contributor

leezu commented Apr 4, 2020

Description

I propose to raise our toolchain requirements for the MXNet 2 development branch to require at minimum gcc7 or clang6 on Unix systems and MSVC 2019 on Windows system. All 3 have reasonable complete C++17 support and MSVC 2019 fully supports, so that we can adopt C++17 as required language standard. gcc7 and clang6 are available on Ubuntu 18.04 LTS release.

The benefits of adopting a more recent C++ standard should be obvious, giving us access to new features and abstractions that the C++ committee has worked on over the course of 6 years. The benefits of adopting a more recent toolchain should also be obvious, as newer compilers will come with more optimizations as older ones.

There are no downsides for MXNet's users, as we can continue to build binary releases of MXNet on CentOS 6 that should work on any major Linux distribution released after 2004. This is possible based on the great work by RedHat to bring new C++ toolchains to old platforms [1].

With respect to Windows: MSVC 2019 is the first MSVC that uses an 64bit toolchain by default. You may have noticed that our Windows CI was recently blocked due to the use of a 32bit toolchain and updating it to MSVC 2019 was chosen as remedy (attempts to use the 64bit version of the 2017 toolchain failed). It also appears that MSVC 2019 16.5 is the first release to make proper use of advanced instruction sets, such as AVX2 [2].

References

1: https://www.softwarecollections.org/en/scls/rhscl/devtoolset-8/
2: https://devblogs.microsoft.com/cppblog/avx2-floating-point-improvements-in-visual-studio-2019-version-16-5/

@sandeep-krishnamurthy
Copy link
Contributor

+1 Thanks for bringing this up.

leezu added a commit that referenced this issue Apr 14, 2020
As per #17968, require C++17 compatible compiler. For cuda code, use C++14 mode introduced in Cuda 9. C++17 support for Cuda will be available in Cuda 11.

Switching to C++17 requires modernizing the toolchain, which exposed a number  of technical debt issues in the codebase. All blocking issues are fixed as part of this PR. See the full list below.

This PR contains the following specific changes:

    Switch CI pipeline to use gcc7 on Ubuntu and CentOS
    Switch CD pipeline to CentOS 7 with https://www.softwarecollections.org/en/scls/rhscl/devtoolset-7/ This enables us to build with gcc7 C++17 compiler while keeping a relatively old glibc requirement for distribution.
    Simplify ARM Edge builds
        Switch to standard Ubuntu / Debian cross-compilation toolchain for ARMv7, ARMv8
        Switch to https://toolchains.bootlin.com/ toolchain for ARMv6 (the Debian ARMv6 toolchain is for ARMv4 + ARMv5 + ARMv6, but we wish to only target ARMv6 and make use of ARMv6 features)
        Remove reliance on dockcross for cross compilation.
    Simplify Jetson build
        Use standard Ubuntu / Debian cross-compilation toolchain for ARMv8
        Upgrade to Cuda 10 and Jetpack 4.3
        Simplify build setup
    Simplify QEMU ARM virtualization test setup on CI
        Remove complex "Virtual Machine in Docker" logic and run a QEMU based Docker container instead based on arm32v7/ubuntu
    Fix out of bounds vector accesses in
        SoftmaxGradOpType
        MKLDNNFCBackward
    Fix use of non-standard rand_r function (which is not available on anymore on newer Android toolchains and shouldn't be use in any case).
    Fix reproducibility of RNN with Dropout
    Fix reproducibility of DGL Graph Sampling Operators
    Update tests for Android Edge build to NDK19. The previously used standalone toolchain is obsolete.

Those Dockerfiles that required refactoring as part of the effort were refactored based on the following consideration

    Maximize the use of system dependencies provided by the distribution instead of manually installing dependencies from source or from third party vendors. This reduces the complexity of the installation process and essentially pins the dependency versions, increasing CI stability. Further, Dockerfile build speed is improved. To facilitate this, use recent distribution versions. We still ensure backwards compatibility via CentOS7 based build and test stages
    Minimize the number of layers in the Dockerfile. Don't have 5 different script files executed, each calling apt-get update and install, but just execute once. Speeds up the build and reduces image size. Keep each Dockerfile simple and tailored to a purpose, instead of running 20 scripts to install dependencies for every thinkable scenario, which is unmaintainable.

Some more small changes:

    Remove outdated references to Cuda 7 and Cuda 8 in various files.
    Remove C++03 support in mshadow
    Disable broken tests
        NumpyBooleanAssignForwardCPU #17990
        test_init.test_rsp_const_init #17988
        quantized_elemwise_mul #18034

List of squashed commits

* cpp standard

* Remove leftover files of Cuda 7 and Cuda 8 support

* thrust 1.9.8 for clang10

* compiler warnings

* Disable broken test_init.test_rsp_const_init

* Disable tests invoking NumpyBooleanAssignForwardCPU

* Fix out of bounds access in SoftmaxGradOpType

* Use CentOS 7 for staticbuilds

CentOS 7 fullfills the requirements for PEP 599 manylinux-2014 and provides a
C++17 toolchain.

* Fix MKLDNNFCBackward

* Update edge toolchain

* Support platforms without rand_r

* Cleanup random.h

* Greatly simplify qemu setup

* Remove unused functions in Jenkins_steps.groovy

* Skip quantized_elemwise_mul due QuantizedElemwiseMulOpShape bug

* Fix R package installation

#18042

* Fix centos ccache

* Fix GPU Makefile staticbuild on CentOS7

* CentOS7 NCCL

* CentOS7 staticbuild fix link with libculibos
@leezu
Copy link
Contributor Author

leezu commented Apr 14, 2020

fb73a17 implements the proposal, fixing a number of other issues that were blocking. Please see the commit message for a complete list of changes.

As a follow-up item, I suggest to remove the cpplint we currently use in favor of clang-tidy (which we also use). cpplint enforces the Google's C++ style guide, which is geared toward C++03. Instead we can target https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines with enforcement by clang-tidy (+ potentially MSVC).

AntiZpvoh pushed a commit to AntiZpvoh/incubator-mxnet that referenced this issue Jul 6, 2020
As per apache#17968, require C++17 compatible compiler. For cuda code, use C++14 mode introduced in Cuda 9. C++17 support for Cuda will be available in Cuda 11.

Switching to C++17 requires modernizing the toolchain, which exposed a number  of technical debt issues in the codebase. All blocking issues are fixed as part of this PR. See the full list below.

This PR contains the following specific changes:

    Switch CI pipeline to use gcc7 on Ubuntu and CentOS
    Switch CD pipeline to CentOS 7 with https://www.softwarecollections.org/en/scls/rhscl/devtoolset-7/ This enables us to build with gcc7 C++17 compiler while keeping a relatively old glibc requirement for distribution.
    Simplify ARM Edge builds
        Switch to standard Ubuntu / Debian cross-compilation toolchain for ARMv7, ARMv8
        Switch to https://toolchains.bootlin.com/ toolchain for ARMv6 (the Debian ARMv6 toolchain is for ARMv4 + ARMv5 + ARMv6, but we wish to only target ARMv6 and make use of ARMv6 features)
        Remove reliance on dockcross for cross compilation.
    Simplify Jetson build
        Use standard Ubuntu / Debian cross-compilation toolchain for ARMv8
        Upgrade to Cuda 10 and Jetpack 4.3
        Simplify build setup
    Simplify QEMU ARM virtualization test setup on CI
        Remove complex "Virtual Machine in Docker" logic and run a QEMU based Docker container instead based on arm32v7/ubuntu
    Fix out of bounds vector accesses in
        SoftmaxGradOpType
        MKLDNNFCBackward
    Fix use of non-standard rand_r function (which is not available on anymore on newer Android toolchains and shouldn't be use in any case).
    Fix reproducibility of RNN with Dropout
    Fix reproducibility of DGL Graph Sampling Operators
    Update tests for Android Edge build to NDK19. The previously used standalone toolchain is obsolete.

Those Dockerfiles that required refactoring as part of the effort were refactored based on the following consideration

    Maximize the use of system dependencies provided by the distribution instead of manually installing dependencies from source or from third party vendors. This reduces the complexity of the installation process and essentially pins the dependency versions, increasing CI stability. Further, Dockerfile build speed is improved. To facilitate this, use recent distribution versions. We still ensure backwards compatibility via CentOS7 based build and test stages
    Minimize the number of layers in the Dockerfile. Don't have 5 different script files executed, each calling apt-get update and install, but just execute once. Speeds up the build and reduces image size. Keep each Dockerfile simple and tailored to a purpose, instead of running 20 scripts to install dependencies for every thinkable scenario, which is unmaintainable.

Some more small changes:

    Remove outdated references to Cuda 7 and Cuda 8 in various files.
    Remove C++03 support in mshadow
    Disable broken tests
        NumpyBooleanAssignForwardCPU apache#17990
        test_init.test_rsp_const_init apache#17988
        quantized_elemwise_mul apache#18034

List of squashed commits

* cpp standard

* Remove leftover files of Cuda 7 and Cuda 8 support

* thrust 1.9.8 for clang10

* compiler warnings

* Disable broken test_init.test_rsp_const_init

* Disable tests invoking NumpyBooleanAssignForwardCPU

* Fix out of bounds access in SoftmaxGradOpType

* Use CentOS 7 for staticbuilds

CentOS 7 fullfills the requirements for PEP 599 manylinux-2014 and provides a
C++17 toolchain.

* Fix MKLDNNFCBackward

* Update edge toolchain

* Support platforms without rand_r

* Cleanup random.h

* Greatly simplify qemu setup

* Remove unused functions in Jenkins_steps.groovy

* Skip quantized_elemwise_mul due QuantizedElemwiseMulOpShape bug

* Fix R package installation

apache#18042

* Fix centos ccache

* Fix GPU Makefile staticbuild on CentOS7

* CentOS7 NCCL

* CentOS7 staticbuild fix link with libculibos
@leezu leezu closed this as completed Jul 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants