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

change mxnet_option behavior #14743

Merged
merged 3 commits into from
Apr 26, 2019
Merged

change mxnet_option behavior #14743

merged 3 commits into from
Apr 26, 2019

Conversation

yajiedesign
Copy link
Contributor

@yajiedesign yajiedesign commented Apr 19, 2019 •
edited
Loading

Description

change mxnet_option behavior

In the past, when the condition in mxnet_option was false, the option would be deleted completely instead of set to OFF. This is not the expected behavior. Now, change it to set to OFF when the condition is false.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • 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 http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Comments

@yajiedesign yajiedesign requested a review from szha as a code owner April 19, 2019 08:01
@TaoLv
Copy link
Member

TaoLv commented Apr 19, 2019

Thank you for the fix @yajiedesign .The OMP issue is also fixed in #14740 @ciyongch @yinghu5

@yajiedesign
Copy link
Contributor Author

@TaoLv ok.

@yajiedesign yajiedesign mentioned this pull request Apr 19, 2019
Closed
@Roshrini Roshrini added the pr-awaiting-review PR is waiting for code review label Apr 19, 2019
@TaoLv
Copy link
Member

TaoLv commented Apr 21, 2019

#14740 is merged now. Can you rebase? @yajiedesign

@yajiedesign
change mxnet_option behavior
da61046 
fix omp with msvc with mkldnn
@pengzhao-intel pengzhao-intel mentioned this pull request Apr 23, 2019
Closed
@yajiedesign yajiedesign mentioned this pull request Apr 23, 2019
Closed
pengzhao-intel
pengzhao-intel reviewed Apr 23, 2019
View reviewed changes
CMakeLists.txt
@@ -29,7 +29,7 @@ mxnet_option(USE_SSE "Build with x86 SSE instruction support" ON IF
mxnet_option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
mxnet_option(USE_LAPACK "Build with lapack support" ON)
mxnet_option(USE_MKL_IF_AVAILABLE "Use MKL if found" ON)
mxnet_option(USE_MKLML_MKL "Use MKLDNN variant of MKL (if MKL found)" ON IF USE_MKL_IF_AVAILABLE AND (NOT APPLE))
mxnet_option(USE_MKLML_MKL "Use MKLDNN variant of MKL (if MKL found)" ON IF USE_MKL_IF_AVAILABLE AND (NOT APPLE) AND (NOT MSVC) )
Copy link
Contributor

Choose a reason for hiding this comment

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

MKLML only rely on MKLDNN rather than MKL so we need to switch USE_MKL_IF_AVAILABLE to USE_MKLDNN. @TaoLv

Copy link
Member

Choose a reason for hiding this comment

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

Not sure what's the USE_MKLML_MKL used for. But I think it's out of the scope of this PR. I hope this PR can be merged soon then we can have @yinghu5's changes for cmake including adjustments for these redundant options. @yajiedesign @szha

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree :)

pengzhao-intel
pengzhao-intel reviewed Apr 23, 2019
View reviewed changes
CMakeLists.txt
@@ -29,7 +29,7 @@ mxnet_option(USE_SSE "Build with x86 SSE instruction support" ON IF
mxnet_option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
mxnet_option(USE_LAPACK "Build with lapack support" ON)
mxnet_option(USE_MKL_IF_AVAILABLE "Use MKL if found" ON)
mxnet_option(USE_MKLML_MKL "Use MKLDNN variant of MKL (if MKL found)" ON IF USE_MKL_IF_AVAILABLE AND (NOT APPLE))
mxnet_option(USE_MKLML_MKL "Use MKLDNN variant of MKL (if MKL found)" ON IF USE_MKL_IF_AVAILABLE AND (NOT APPLE) AND (NOT MSVC) )
mxnet_option(USE_MKLDNN "Use MKLDNN variant of MKL (if MKL found)" ON IF USE_MKL_IF_AVAILABLE AND (NOT APPLE) AND (NOT MSVC) AND (CMAKE_HOST_SYSTEM_PROCESSOR STREQUAL "x86_64") AND (NOT CMAKE_CROSSCOMPILING))
Copy link
Contributor

Choose a reason for hiding this comment

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

MKLDNN doesn't rely on USE_MKL_IF_AVAILABLE

pengzhao-intel
pengzhao-intel approved these changes Apr 25, 2019
View reviewed changes
Copy link
Contributor

@pengzhao-intel pengzhao-intel left a comment

Choose a reason for hiding this comment

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

LGTM

Please rebase the code, one of Julia issue is fixed now.

larroy
larroy approved these changes Apr 25, 2019
View reviewed changes
Copy link
Contributor

@larroy larroy left a comment

Choose a reason for hiding this comment

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

LGTM

@yinghu5
Copy link
Contributor

yinghu5 commented Apr 26, 2019 •
edited
Loading

@yajiedesign, @TaoLv @pengzhao-intel @larroy thank you for the fix . It also fixes the issue #14729 Build with CMake under windows can't trigger USE_MKLDNN in CI .
While the user defined variable like -DUSE_MKL=1 should have higher priority than the variable in mxnet_option ( even when the condition in mxnet_option was false.) Thanks

@pengzhao-intel
Copy link
Contributor

pengzhao-intel commented Apr 26, 2019 •
edited
Loading

Thanks all of your effort and Intel TCE team (@yinghu5 @NeoZhangJianyu) will continuously work on CMake improvement :)

Merging now.

@pengzhao-intel pengzhao-intel merged commit 6aeb97e into apache:master Apr 26, 2019
access2rohit pushed a commit to access2rohit/incubator-mxnet that referenced this pull request May 14, 2019
@yajiedesign
change mxnet_option behavior (apache#14743)
ce37c4e 
fix omp with msvc with mkldnn
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@yajiedesign
change mxnet_option behavior (apache#14743)
2662177 
fix omp with msvc with mkldnn
@leezu leezu mentioned this pull request Dec 9, 2019
Merged
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TaoLv TaoLv TaoLv left review comments

@pengzhao-intel pengzhao-intel pengzhao-intel approved these changes

@larroy larroy larroy approved these changes

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

Assignees
No one assigned
Labels
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.
6 participants
@yajiedesign @TaoLv @yinghu5 @pengzhao-intel @larroy @Roshrini