Skip to content
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

Feature/openmp off on macos #170

Merged
merged 4 commits into from Aug 28, 2017
Merged

Feature/openmp off on macos #170

merged 4 commits into from Aug 28, 2017

Conversation

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Aug 28, 2017

@kevinushey : @coatless already test drove this and said it worked on his macOS box. Could you possibly give it a spin too, maybe even on two different macOS variants (hoping / thinking you may have access to more than one).

@eddelbuettel eddelbuettel requested a review from kevinushey Aug 28, 2017
@binxiangni
Copy link
Contributor

@binxiangni binxiangni commented Aug 28, 2017

FYI, it works on my macOS(10.12.6).

@eddelbuettel
Copy link
Member Author

@eddelbuettel eddelbuettel commented Aug 28, 2017

Thanks, @binxiangni. That makes it 2:0.

Are you running the same version as @coatless, assuming the newest (ElCapitan, if I have that right).

@binxiangni
Copy link
Contributor

@binxiangni binxiangni commented Aug 28, 2017

10.12.6 is Sierra, the newest system.

@coatless
Copy link
Contributor

@coatless coatless commented Aug 28, 2017

@eddelbuettel: the macOS version of R is built on El Capitan. As I'm on Sierra, gfortran encourages the use of 6.3.0 instead of R's 6.1.0. My override for not using gfortran 6.1.0 was wiped as I was testing a new toolchain installer "Rtools for macOS" that installs everything together.

@eddelbuettel eddelbuettel merged commit 998c30d into master Aug 28, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@eddelbuettel eddelbuettel deleted the feature/openmp_off_on_macos branch Aug 28, 2017
@danfulop
Copy link

@danfulop danfulop commented Oct 12, 2017

Will RcppArmadillo support OpenMP on macOS again in the future? I followed @coatless' directions for making R & OpenMP play nice, but RcppArmadillo still has OpenMP turned off. I'm running macOS 10.12 & R 3.4.1.

A related question. Will all R packages that depend on the Rcpp suite be lacking OpenMP support on macOS for the time being, or would that only be the case sometimes? Please excuse my lack of knowledge with regards to Rcpp and RcppArmadillo.

@coatless
Copy link
Contributor

@coatless coatless commented Oct 12, 2017

@danfulop: This is only the case for RcppArmadillo. Using OpenMP on macOS with Rcpp is possible via // [[Rcpp::plugins(openmp)]]. Though, you need to be wary of the dragons that exist since R is single threaded.

You can manually regain OpenMP by modifying the local install of RcppArmadillo's inline.R to include -fopenmp and removing the #define ARMA_DONT_USE_OPENMP 1 in inst/include/RcppArmadilloConfigGenerated.h

@yaccos
Copy link

@yaccos yaccos commented Apr 19, 2018

Would it not be better if the configure script detected directly if the compiler supported openmp?

@eddelbuettel
Copy link
Member Author

@eddelbuettel eddelbuettel commented Apr 19, 2018

Yes, of course it would be. In an ideal world R would do this reliably for us as it does for most other things on most platforms. @coatless and I looked into this repeatedly, but it still difficult to impossible given the current setup particularly on macOS. While things work pretty well on Linux, it is a sadder story on macOS.

@coatless
Copy link
Contributor

@coatless coatless commented Apr 19, 2018

@yaccos I've recently looked into this again except using OpenMP detection routine used by R in its make routine.

c.f.
https://github.com/wch/r-source/blob/a71d8f00c109a339b40f9ac42d2e9116d986279f/m4/openmp.m4#L34-L100

Results can be found here:

coatless@3f1aed6#diff-67e997bcfdac55191033d57a16d1408a

Notice we still fall victim to not detecting OpenMP using the clang4 setup...

devtools::install_github('coatless/rcpparmadillo', ref = 'feature/macos-openmp-detection')

checking for /usr/local/clang4/bin/clang++ option to support OpenMP... configure: WARNING: Some complex-valued LAPACK functions may not be available
unsupported

@kthohr
Copy link
Contributor

@kthohr kthohr commented Apr 19, 2018

@eddelbuettel - What about a check based on

apple_compiler=$($CXX --version 2>&1 | grep -i -c -e 'apple llvm')

Works well on my macOS system.

@coatless
Copy link
Contributor

@coatless coatless commented Apr 19, 2018

Actually, @eddelbuettel the use of the m4 routine would work in terms of allowing other compilers to work. We just haven't solved the issue of the clang4 detection...

@eddelbuettel
Copy link
Member Author

@eddelbuettel eddelbuettel commented Apr 19, 2018

@kthohr That line we could do. But ... then what? @coatless has been trying to get me to make this better for years now yet it seems that a combination of moving targets and incomplete support on the R / macOS side do not allow it.

Now, I would be all for an opt-in solution for macOS power users. Maybe you guys can set an env var, or some other handshake, and then RcppArmadillo checks and sets things up.

I just fear that "normal" macOS users just want something to work...

@kthohr
Copy link
Contributor

@kthohr kthohr commented Apr 19, 2018

@eddelbuettel A little spaghetti code :-) See line 65 onwards:

https://github.com/kthohr/RcppArmadillo/blob/macOS-openMP/configure.ac

I then modified the existing macOS check (line 135 in that file). I've tested this using regular (Apple) clang++, and clang 6.0 and gcc 7.0 from MacPorts.

@eddelbuettel
Copy link
Member Author

@eddelbuettel eddelbuettel commented Apr 19, 2018

Well I trust you @kthohr and @coatless. If you can cook something up, and someone else validates, I'd be open. I just don;t have a macOS box ...

@kthohr
Copy link
Contributor

@kthohr kthohr commented Apr 19, 2018

Sure - if @coatless could provide any feedback using his build (i.e., clang4), that would be great

@danfulop
Copy link

@danfulop danfulop commented Apr 19, 2018

I volunteer as an additional Mac tester

@kthohr
Copy link
Contributor

@kthohr kthohr commented Apr 19, 2018

@danfulop - Can you try to install using:

devtools:::install_github("kthohr/RcppArmadillo",ref="macOS-openMP")
@danfulop
Copy link

@danfulop danfulop commented Apr 19, 2018

it worked perfectly!

@binxiangni
Copy link
Contributor

@binxiangni binxiangni commented Apr 19, 2018

It also worked on my side(macOS 10.13.4)

@yaccos
Copy link

@yaccos yaccos commented Apr 19, 2018

Works for me too (macOS 10.13.4 and clang version 5.0.1). Does someone know if there are other R packages which could need the same fix? Life with macOS and openmp would be a lot easier if the default apple llvm supported openmp.

@eddelbuettel
Copy link
Member Author

@eddelbuettel eddelbuettel commented Apr 20, 2018

@coatless and I discussed this a little. In principle, all this is fine, and I can probably be persuaded to use the @kthohr patch. I won't be able to test it though.

And one general issue that with the 450+ reverse depends we have some responsibility to CRAN to no change this too much / require too many uploads. But we can certainly think about folding improvements in at the so-far bi-monthly cycle.

@kthohr
Copy link
Contributor

@kthohr kthohr commented Apr 20, 2018

I'll move this to a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.