search

AutoFlowResearch / SmartPeak

Fast and Accurate CE-, GC- and LC-MS(/MS) Data Processing
MIT License
43 stars 13 forks source link

Enable OpenMP for MacOS Builds #455

Closed bertrandboudaud closed 2 years ago

bertrandboudaud commented 2 years ago

Unlike other platforms, on MacOS OpenMP was not enabled for OpenMS, as we see in this build log: 

-- OpenMP support requested: ON
-- Could NOT find OpenMP_CXX (missing: OpenMP_CXX_FLAGS OpenMP_CXX_LIB_NAMES) 
-- Could NOT find OpenMP (missing: OpenMP_CXX_FOUND CXX)

OpenMP seems to be a hard requirement as it is used, for example in their logging system to be thread-safe.

At the moment, users report crashes on MacOS, and call stacks show most of the time a crash around openMS's logger methods. we can also see sometimes interlaced log messages from openMS, just before crashing, which looks worrying.

As in this log:

While loading '/Users/distiller/SmartPeak/src/examples/data/LCMS_MRM_QCs/mzML/150601_0_BloodProject01_PLT_QC_Broth-1.mzML': Ill formed absolute or relative sourceFile path: file://.le://.

(note the double "le://." at the end, coming from a previous log message).

So it is a strong indication that at least one of the reasons for these crashes is the missing OpenMP when building OpenMS.

one side note: only to install the openmp developer lib is sufficient to include it in the build. that would explain why it works on some machines (developer machines?) and not on others. anyway, as race conditions are involved, machine model, os version, all can lead to different behaviors.

This PR adds a stress test that indeed crashes only on Mac when OpenMP is not there, using the example data. so I had to mix example data with unit tests, which may be a little bit weird depending on the plan we had about the different data usages.

bertrandboudaud commented 2 years ago

Would it be possible to simply run the examples similar to Ubuntu for MacOS instead of adding in a new unit test?

Yes, I think it's a better idea. i will make the change.

bertrandboudaud commented 2 years ago

Let's remove the line sed -i '' 's/#pragma omp parallel for//g' src/openms/source/FORMAT/HANDLERS/MzMLHandler.cpp in config.yml to reinstate full OpenMP support.

Ok, done

bertrandboudaud commented 2 years ago

Would it be possible to simply run the examples similar to Ubuntu for MacOS instead of adding in a new unit test?

It's done now. And indeed the examples fail if we don't activate OpenMP. So I removed the unit test.