Add option to choose between OMP implementations #15808

larroy · 2019-08-08T20:15:24Z

Description

Adds a build switch to choose OpenMP implementations:

Workaround for #15790

@karan6181

larroy · 2019-08-08T20:18:01Z

@mxnet-label-bot add [Build, CMake]

larroy · 2019-08-08T20:18:55Z

@mxnet-label-bot add [Breaking]

ChaiBapchya

LGTM!

CMakeLists.txt

apeforest

LGTM. But I am not an expert in CMake so I can not really decide whether this is the most proper way. @marcoabreu @lebeg for review.

lebeg · 2019-08-13T12:10:14Z

LGTM. But I am not an expert in CMake so I can not really decide whether this is the most proper way. @marcoabreu @lebeg for review.

I think what you were proposing would be a better way to handle this. Instead of ON/OFF I would define an order in which the Omp versions would be preferred. Like a list OMP_SYSTEM, OMP_BUNDLED, OMP_OFF.

For a case like OMP_SYSTEM if it's not found it would be an error.

larroy · 2019-08-13T17:10:55Z

@lebeg you might be right, but getting this working is urgent as we can't debug. I would propose doing it incrementally or getting your contribution in this direction.

apeforest · 2019-08-13T17:43:05Z

@larroy What's the effort required to implement the multi-selection as you earlier proposed? If this only blocks debugging using CMake and not regular users or developers using Make, I am more leaning towards @lebeg suggestion.

larroy · 2019-08-13T22:34:00Z

CI is blocked now, and it's going to be blocked for a few days. There's problems with Windows and other things. If I make changes I'm afraid this won't be merged for a while. Up to you guys to decide. I think this is urgent enough to have it merged and then refine it. I would say this is a critical fix for being able to debug, as other people need to debug issues related to other tasks. (Make in debug requires updating the linker which is something I would prefer to avoid in my workstation).

The effort to implement your suggestion should be around 30 minutes.

marcoabreu · 2019-08-13T22:37:42Z

I'd also prefer the multi selection.

larroy · 2019-08-23T17:12:56Z

Can you guys review again? I made the suggested changes.

CMakeLists.txt

apeforest

@larroy I thought the suggested change is to use multi-selection for USE_OPENMP not adding additional variable:

It would look like:

USE_OPENMP:  "Configure Openmp support"    OMP_SYSTEM |  | OMP_BUNDLED | OMP_OFF)

@lebeg Please let me know if my understanding is correct. Thanks

larroy · 2019-08-24T01:40:48Z

@apeforest I think what you propose is not homogeneous with what we have. I think is better to leave an intitial boolean switch and a fine grained selector. It also keeps backwards compatibility. Hence my proposal in this PR.

lebeg · 2019-08-26T09:05:13Z

@larroy I thought the suggested change is to use multi-selection for USE_OPENMP not adding additional variable:

It would look like:
USE_OPENMP:  "Configure Openmp support"    OMP_SYSTEM |  | OMP_BUNDLED | OMP_OFF)
@lebeg Please let me know if my understanding is correct. Thanks

Yes, this is more or less what I had in mind. For the sake of backwards compatibility (and maybe simplicity) keeping the option ON would default to OMP_SYSTEM.

lebeg · 2019-08-30T09:25:00Z

There seem to be some unintended changes in this PR, otherwise it looks good

larroy · 2019-08-30T17:10:49Z

They are for CI, a change in dmlc is needed first:

dmlc/dmlc-core#558

larroy · 2019-09-06T20:46:37Z

@lebeg please approve then

apeforest · 2019-09-06T22:08:05Z

CMakeLists.txt

@@ -23,7 +23,7 @@ mxnet_option(USE_CUDA             "Build with CUDA support"   ON)
 mxnet_option(USE_OLDCMAKECUDA     "Build with old cmake cuda" OFF)
 mxnet_option(USE_NCCL             "Use NVidia NCCL with CUDA" OFF)
 mxnet_option(USE_OPENCV           "Build with OpenCV support" ON)
-mxnet_option(USE_OPENMP           "Build with Openmp support" ON)
+mxnet_option(USE_OPENMP           "Build with Openmp support" ON) # OFF | ON | PLATFORM | BUNDLED


What's the difference between ON and the rest? Can you add some comments here?

apeforest · 2019-09-06T22:08:25Z

CMakeLists.txt

    if(OPENMP_FOUND)
      set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${OpenMP_C_FLAGS}")
      set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}")
      set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${OpenMP_EXE_LINKER_FLAGS}")
      set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${OpenMP_EXE_LINKER_FLAGS}")
      add_definitions(-DMXNET_USE_OPENMP=1)
    endif()
+  else()
+    message(FATAL_ERROR "USE_OPENMP takes values [PLATFORM, BUNDLED, OFF]")


"USE_OPENMP takes values [PLATFORM, BUNDLED, OFF, ON]")?

apeforest · 2019-09-06T22:09:35Z

They are for CI, a change in dmlc is needed first:

dmlc/dmlc-core#558

Why is it needed for this PR?

marcoabreu

Review comments. Also is the submodule update necessary

marcoabreu · 2019-09-06T22:23:39Z

CMakeLists.txt

-     AND NOT MSVC
-     AND NOT CMAKE_CROSSCOMPILING)
-
+  if(USE_OPENMP STREQUAL "BUNDLED" AND EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/3rdparty/openmp/CMakeLists.txt


The first check and the others should be separate to throw appropriate error messages

Thanks for your review. Can you explain what you mean exactly? A concrete request would help, as this is a followup on previous review requests that might be conflicting.

If someone uses the option "bundled" but the "exists" check fails, they'd receive an error message that they selected an invalid mode.

marcoabreu · 2019-09-06T22:23:58Z

CMakeLists.txt

@@ -452,14 +451,18 @@ if(USE_OPENMP)
    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${OpenMP_C_FLAGS}")
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}")
    add_definitions(-DMXNET_USE_OPENMP=1)
-  else()
+  elseif(USE_OPENMP STREQUAL "PLATFORM" OR USE_OPENMP STREQUAL "ON")


What's the difference between on and platform

They are the same, for backwards compatibility, meaning if someone had USE_OPENMP=ON will continue working with the OMP implementation which has been shown to be most stable.

cjolivier01

dangerous

szha · 2019-09-08T03:21:22Z

@cjolivier01 if you'd like your change request to be considered a veto, please provide a technical reason. You may refer to the Apache Voting Process for details.

Reopening given that @larroy didn't indicate that he'd like to abandon this change.

larroy · 2019-09-08T03:26:49Z

Submodule update is necessary as build was failing, there's a fix in the submodule that should be merged soon. You can see the error message by pulling the PR in your local and trying to build. It's related to throw specification, for details see the line changed in the submodule.

cjolivier01 · 2019-09-08T03:28:06Z

this is a repeat of a previously vetoed proposal by Pedro and another by someone he tried to sneak a similar change in, just under a different guise. I really don’t have the energy to fight openly discuss this anymore after two years.

cjolivier01 · 2019-09-08T03:29:38Z

in addition, on a fundamental level, flippantly switching between omp libraries while taking nothing else into considerations is extraordinarily hazardous (ie what’s mkl bringing in?).

larroy · 2019-09-08T03:31:22Z

This has repeatedly caused crashes reported by several users which didn't happen when we used the platform OMP which is also the one shipped with binary releases. You are doing a disservice to the community and the project with this attitude, the users are free to choose the OMP implementation that they want. We need to reproduce the builds which use the builtin omp with the CMake build, and this change enables exactly this behaviour, You are free to compile with whatever OMP version you like, see the github issues for reports of crashes and problems with other OMP implementations. Intel already said they will remove the bundled iomp from MKL as it causes some crashes using multiprocessing which doesn't see to be caused by our code.

szha · 2019-09-08T03:32:25Z

@cjolivier01 I didn't realize that. If this is indeed a repetition of a change proposal already vetoed, would you mind linking to it?

I only recall a case where @lebeg attempted to remove the bundled omp, which is different from what @larroy is proposing here. If this is what you're referring to, given the difference, would you mind providing a technical reason for rejecting this change?

cjolivier01 · 2019-09-08T03:42:50Z

There is plenty of info out there already on this issue including discussions on dev. I will not be forever forced to re-argue this thing every time Pedro wants to sneak basically the exact same change in every few months for two years+ while never actually root-causing the issue (he’s been told what the issue was — not the OMP library’s fault) — it’s a big in mxnet — see the issue he is locked out of), but ignores repeatedly.

larroy · 2019-09-08T07:52:11Z

@cjolivier01 stop holding this project hostage. This will be reported to the relevant decision makers in Apache. I hope your powers are revoked due to your track record of destructive actions.

marcoabreu · 2019-09-08T09:04:54Z

If the submodule update fixes the build, please put it in a separate PR as already stated.

marcoabreu · 2019-09-08T09:17:04Z

While I try to not engage in the discussion above whether this is a match to the previously vetoed PR, I agree that I see some similarities.

With regards to #15790: This PR doesn't actually solve the problem (which appears to be the same as previously) but gives users a different OMP library choice. That effectively means that the default method we use is still broken and users would have to know that they have to use a different one. I might be no expert in that field, but I agree that to this day we only read something about some assertion being hit and nothing further. From what I can see, there has been no root causing (to the actual root) but a lot based on assumptions. This change seems quite massive (introducing a new OMP Backend choice) while not being based on real data. So i understand the frustration from both sides here: Chris' request for root causing is constantly being denied, while the root cause for pedro seems apparent. But since we are a community and we would like to make a well educated decision as a group, I'd agree with the general request for more root causing in order to create transparency.

Thinking about it, I agree that this is the same change in a different costume that already had been vetoed with proper technical reasons - but I understand that Chris is getting frustrated by constantly having to repeat his request.

So yeah, I agree with his veto that I'd like to see a deep dive. That should be collected somewhere and if we are at a crossroads, we should bring the topic up for a broader vote on dev@.

marcoabreu · 2019-09-08T09:20:30Z

Another proposal to avoid the difficult personal situation is to hand this issues over to somebody else. Maybe another engineer at Amazon or somebody at Intel or Nvidia would like to be of assistance here. I'm certain that they are also quite capable of debugging concurrency issues. Wdyt?

larroy requested a review from szha as a code owner August 8, 2019 20:15

marcoabreu added Build CMake CMake related bugs/issues/improvements labels Aug 8, 2019

marcoabreu added the Breaking label Aug 8, 2019

ChaiBapchya approved these changes Aug 10, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

apeforest reviewed Aug 12, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

apeforest approved these changes Aug 13, 2019

View reviewed changes

larroy force-pushed the omp_chooser branch from 99c1398 to 455f722 Compare August 21, 2019 21:13

apeforest reviewed Aug 23, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

apeforest reviewed Aug 23, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

apeforest suggested changes Aug 23, 2019

View reviewed changes

larroy force-pushed the omp_chooser branch 2 times, most recently from e293bdb to c378374 Compare August 27, 2019 00:29

apeforest mentioned this pull request Aug 27, 2019

Revert "Refactor LibraryInitializer so it's thread safe. Fixes random sporadical concurrency crashes." #16023

Closed

larroy force-pushed the omp_chooser branch 2 times, most recently from 9dff7d8 to 1f68252 Compare August 29, 2019 22:07

larroy mentioned this pull request Aug 29, 2019

Revert accidental change to CMakelists #16040

Merged

7 tasks

apeforest requested a review from anirudh2290 as a code owner September 6, 2019 19:56

apeforest requested review from gigasquid, marcoabreu, nswamy and yzhliu as code owners September 6, 2019 19:56

larroy force-pushed the omp_chooser branch from a12d36e to 98ae435 Compare September 6, 2019 20:45

larroy added 2 commits September 6, 2019 13:50

Add option to choose between OMP implementations

583a2bb

Switch submodule for CI

c7fd3a8

larroy force-pushed the omp_chooser branch from 98ae435 to c7fd3a8 Compare September 6, 2019 20:51

larroy added 2 commits September 6, 2019 13:51

remove ws

80d2216

retrigger ci

27a2a12

apeforest reviewed Sep 6, 2019

View reviewed changes

marcoabreu reviewed Sep 6, 2019

View reviewed changes

cjolivier01 suggested changes Sep 8, 2019

View reviewed changes

cjolivier01 closed this Sep 8, 2019

szha reopened this Sep 8, 2019

cjolivier01 closed this Sep 8, 2019

Add option to choose between OMP implementations #15808

Add option to choose between OMP implementations #15808

Conversation

larroy commented Aug 8, 2019

Description

larroy commented Aug 8, 2019

larroy commented Aug 8, 2019

ChaiBapchya left a comment

Choose a reason for hiding this comment

apeforest left a comment

Choose a reason for hiding this comment

lebeg commented Aug 13, 2019

larroy commented Aug 13, 2019

apeforest commented Aug 13, 2019

larroy commented Aug 13, 2019

marcoabreu commented Aug 13, 2019

larroy commented Aug 23, 2019 • edited Loading

apeforest left a comment • edited Loading

Choose a reason for hiding this comment

larroy commented Aug 24, 2019 • edited Loading

lebeg commented Aug 26, 2019

lebeg commented Aug 30, 2019

larroy commented Aug 30, 2019

larroy commented Sep 6, 2019

apeforest Sep 6, 2019

Choose a reason for hiding this comment

apeforest Sep 6, 2019

Choose a reason for hiding this comment

apeforest commented Sep 6, 2019

marcoabreu left a comment

Choose a reason for hiding this comment

marcoabreu Sep 6, 2019

Choose a reason for hiding this comment

larroy Sep 8, 2019 • edited Loading

Choose a reason for hiding this comment

marcoabreu Sep 8, 2019

Choose a reason for hiding this comment

marcoabreu Sep 6, 2019

Choose a reason for hiding this comment

larroy Sep 8, 2019 • edited Loading

Choose a reason for hiding this comment

cjolivier01 left a comment

Choose a reason for hiding this comment

szha commented Sep 8, 2019

larroy commented Sep 8, 2019 • edited Loading

cjolivier01 commented Sep 8, 2019

cjolivier01 commented Sep 8, 2019

larroy commented Sep 8, 2019 • edited Loading

szha commented Sep 8, 2019

cjolivier01 commented Sep 8, 2019 • edited Loading

larroy commented Sep 8, 2019

marcoabreu commented Sep 8, 2019

marcoabreu commented Sep 8, 2019

marcoabreu commented Sep 8, 2019

larroy commented Aug 23, 2019 •

edited

Loading

apeforest left a comment •

edited

Loading

larroy commented Aug 24, 2019 •

edited

Loading

larroy Sep 8, 2019 •

edited

Loading

larroy Sep 8, 2019 •

edited

Loading

larroy commented Sep 8, 2019 •

edited

Loading

larroy commented Sep 8, 2019 •

edited

Loading

cjolivier01 commented Sep 8, 2019 •

edited

Loading