-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
[R] fix OpenMP detection on macOS #8684
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.
Thank you for the fix! Small question inlined in the comments.
I don't have a macos device, @hcho3 please help look into this. ;-)
R-package/configure
Outdated
@@ -2101,8 +2101,6 @@ CXX14STD=`"${R_HOME}/bin/R" CMD config CXX14STD` | |||
CXX="${CXX14} ${CXX14STD}" | |||
CXXFLAGS=`"${R_HOME}/bin/R" CMD config CXXFLAGS` | |||
|
|||
CC=`"${R_HOME}/bin/R" CMD config CC` |
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.
We might need the CC flag here due to function registration in R-package/src/init.c
.
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.
Yes, let's retain this line.
R-package/configure
Outdated
@@ -2101,8 +2101,6 @@ CXX14STD=`"${R_HOME}/bin/R" CMD config CXX14STD` | |||
CXX="${CXX14} ${CXX14STD}" | |||
CXXFLAGS=`"${R_HOME}/bin/R" CMD config CXXFLAGS` | |||
|
|||
CC=`"${R_HOME}/bin/R" CMD config CC` | |||
CFLAGS=`"${R_HOME}/bin/R" CMD config CFLAGS` |
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 retain this line too.
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.
LGTM. But please keep the two lines with CC
and CFLAGS
.
@trivialfis I think we should include this in our CRAN submission, if you plan to re-submit. |
Sure, sorry about that! Just pushed 2f59118 adding the C compiler back. And regenerated autoconf \
--output configure \
./configure.ac |
That's a really good suggestion. Will open an issue for this. |
@jameslamb I've been strugglebus with xgboost for months on MacOS, see issue here. Minor update to that is that XGBoost in python can fully use CPU with Mac's M1 line of processors, but no evidence that the R version can yet. I ran the following install conditions you noted with the latest build:
But trying out xgboost in R, I got the following error:
I disabled makevars to make this a vanilla test. It's possible this is still specific to Mac's ARM architecture, but wanted to note the problem in this thread. |
Just like that error says, you probably have multiple versions of OpenMP installed on your Mac. You could find them like this: find /usr -name 'libomp.dylib' -type f
find ${HOME} -name 'libomp.dylib' -type f One common cause of this is the use of For example, on my mac I see the following running those commands
If you're using R from If not, and you want to be sure R processes always find the Homebrew one, you could run something like the following. export LD_LIBRARY_PATH="/usr/local/opt/libomp/lib:${LD_LIBRARY_PATH}" Or achieve similar in Please also note that I'm not a maintainer here. But I suspect that maintainers would prefer you search the open issues for similar error messages (like #5496 which you commented on a few years ago), and then if you don't find what you're looking for, open a new issue (instead of commenting on a closed pull request) with questions like this. And ideally providing a minimal, reproducible example showing exactly what you did and describing your environment. |
@jameslamb Thanks for the very helpful feedback, but apologies if the original message went astray, just wanted to report the problem I ran into following the installation instructions here. I think you've correctly identified the problem on my setup, currently working to resolve that. |
Co-authored-by: James Lamb <[email protected]>
@trivialfis @jameslamb It is still broken, at least with GCC, and gives a wrong recommendation (
P. S. Perhaps “should” should not be used with an optional package manager. |
Detection of OpenMP on macOS in the R package's
configure
script is currently not working.This PR proposes switching from a C compiler to a C++ compiler in
R-package/configure
to fix that.Why using a C compiler breaks this check
This code in
configure.ac
...xgboost/R-package/configure.ac
Line 23 in e7d612d
xgboost/R-package/configure.ac
Line 57 in e7d612d
... results in
autoconf
generating a fileconftest.cpp
xgboost/R-package/configure
Line 2745 in e7d612d
xgboost/R-package/configure
Line 2849 in e7d612d
... but then XGBoost's
configure
file is looking instead for a file namedconftest.c
xgboost/R-package/configure.ac
Line 58 in e7d612d
How I tested this
On my mac tonight (macOS 12.2.1, R 4.2.2, compiling with
clang++
13.0.0), I ran the following onmaster
And saw the following in logs
@trivialfis noted the same thing on recent R Hub checks, #8669 (comment)
Repeating that on this branch, the R package compiles, links to OpenMP, and its unit tests succeed.
cd tests Rscript testthat.R
Notes for Reviewers
Sorry for not noticing this while reviewing #8669. It wasn't until today that I had time to look closely and test.
In LightGBM, we try to catch issues like this by
grep
-ing the build logs: https://github.com/microsoft/LightGBM/blob/5df7d516f3e64e8431aecfea892a850afeb787bb/.ci/test_r_package.sh#L250-L267. I'm not proposing that here, but you might want to consider it in the future.Thanks for your time and consideration.