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

Replace mxnet_option macro with standard CMAKE_DEPENDENT_OPTION #17018

Merged
merged 2 commits into from
Dec 10, 2019
Merged

Replace mxnet_option macro with standard CMAKE_DEPENDENT_OPTION #17018

merged 2 commits into from
Dec 10, 2019

Conversation

leezu
Copy link
Contributor

@leezu leezu commented Dec 9, 2019 •
edited
Loading

Description

Replace mxnet_option macro with standard CMAKE_DEPENDENT_OPTION.
Using standard language constructs improves maintainability and eases reading the code.

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

  • Replace mxnet_option macro with CMAKE_DEPENDENT_OPTION part of CMake "standard library"

Comments

@cjolivier01 please confirm the change in FindMKL.cmake is correct. Previously, based on #14743, if the condition was false, the option was set to OFF.

@leezu leezu requested a review from szha as a code owner December 9, 2019 07:02
@szha szha requested a review from yajiedesign December 9, 2019 07:04
@szha
Copy link
Member

szha commented Dec 9, 2019

cc @junrushao1994 who may want to offer suggestions

@leezu leezu force-pushed the cmakedependentoption branch 2 times, most recently from 90110f0 to be66bc5 Compare December 9, 2019 07:39
@junrushao
Copy link
Member

I think I am a very beginner in using cmake. Looks like CMAKE_DEPENDENT_OPTION provides a native way to express cmake options, which is great :-)

@leezu leezu added the pr-awaiting-review PR is waiting for code review label Dec 9, 2019
yajiedesign
yajiedesign suggested changes Dec 10, 2019
View reviewed changes
CMakeLists.txt Show resolved Hide resolved
@leezu leezu mentioned this pull request Dec 10, 2019
Merged
5 tasks
yajiedesign
yajiedesign reviewed Dec 10, 2019
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
yajiedesign
yajiedesign reviewed Dec 10, 2019
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@leezu leezu added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-review PR is waiting for code review labels Dec 10, 2019
@leezu leezu merged commit 7895f93 into apache:master Dec 10, 2019
yajiedesign pushed a commit to yajiedesign/mxnet that referenced this pull request Jan 5, 2020
@leezu @yajiedesign
Backport apache#16980 apache#17031 apache#17018 apache#17019 to 1.6 b…
e64788b 
…ranch

Fix CUDNN detection for CMake build (apache#17019)

Replace mxnet_option macro with standard CMAKE_DEPENDENT_OPTION (apache#17018)

Switch to modern CMake CUDA handling (apache#17031)

Introduce unified MXNET_CUDA_ARCH option to specify cuda architectures.
Previously cuda architecture setting was partially broken and different options
were applied to different parts of the build (CUDA_ARCH_NAME CUDA_ARCH_BIN
CUDA_ARCH_PTX and CUDA_ARCH_LIST).

Include FindCUDAToolkit from CMake 3.17, which replaces the deprecated FindCUDA
functionality for finding the cuda toolkit include directories and libraries.

Workaround for DLL size limitation on Windows (apache#16980)

* change windows build system.
add gen_warp cpp version
add add_custom_command to run warp_gen
add download cmake
add option
change option
add dynamic read mxnet dll
ptrendx pushed a commit that referenced this pull request Jan 5, 2020
@yajiedesign @leezu @ptrendx
Backport #16980 #17031 #17018 #17019 to 1.6 branch (#17213)
0015fc3 
Fix CUDNN detection for CMake build (#17019)

Replace mxnet_option macro with standard CMAKE_DEPENDENT_OPTION (#17018)

Switch to modern CMake CUDA handling (#17031)

Introduce unified MXNET_CUDA_ARCH option to specify cuda architectures.
Previously cuda architecture setting was partially broken and different options
were applied to different parts of the build (CUDA_ARCH_NAME CUDA_ARCH_BIN
CUDA_ARCH_PTX and CUDA_ARCH_LIST).

Include FindCUDAToolkit from CMake 3.17, which replaces the deprecated FindCUDA
functionality for finding the cuda toolkit include directories and libraries.

Workaround for DLL size limitation on Windows (#16980)

* change windows build system.
add gen_warp cpp version
add add_custom_command to run warp_gen
add download cmake
add option
change option
add dynamic read mxnet dll

Co-authored-by: Leonard Lausen <leonard@lausen.nl>
@leezu leezu deleted the cmakedependentoption branch September 28, 2020 18:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@yajiedesign yajiedesign yajiedesign approved these changes

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

@cjolivier01 cjolivier01 Awaiting requested review from cjolivier01

Assignees
No one assigned
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@leezu @szha @junrushao @yajiedesign