-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable OpenMP for MacOS Builds #455
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
It's done now. And indeed the examples fail if we don't activate OpenMP. So I removed the unit test. |
Unlike other platforms, on MacOS OpenMP was not enabled for OpenMS, as we see in this build log:
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:
(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.