mwpennell / geiger-v2

A suite of methods and models for studying evolutionary radiations
22 stars 17 forks source link

Build is broken #5

Closed richfitz closed 10 years ago

richfitz commented 10 years ago

The build has been broken since 79e79d9: https://travis-ci.org/mwpennell/geiger-v2

Looks like the examples have fallen behind the code. Y'all should be getting emails when it fails.

mwpennell commented 10 years ago

yes, we are working on it. been unable to replicate the error on our machines (it is a multicore issue). package works fine but fails a new check implemented by CRAN. think we should have it sorted out.

richfitz commented 10 years ago

OK, I think I can see how to fix it.

richfitz commented 10 years ago

This is Jon, I presume: https://github.com/mwpennell/geiger-v2/blob/master/R/utilities-misc.R#L58-L72

mwpennell commented 10 years ago

yes, i don't understand how this all works. CRAN only allows 2 cores to be opened automatically (which Is arbitrary and unnecessary IMO) so @uyedaj and i have just been trying to launch two cores for the purposes of the examples rather than change the structure of how multicore is called. open to any ideas

richfitz commented 10 years ago

Two options:

  1. I think we're going to have to change that linked function and do the same thing that mclapply does and take use getOption("mc.cores", 2L) rather than detectCores.
  2. Manually pass in ncores everywhere that parallel might be used and have this break everytime someone uses a new call to fitContinuous.

Personally, I think that 1 is the right call. Using all cores on the machine is potentially rude. This is why the options changed between multicore and parallel. It's simple enough to change by setting the mc.cores option if you want to use more (e.g., add option(mc.cores=detectCores()) in your .Rprofile.

richfitz commented 10 years ago

But it might also be worth taking the time and working out if that particular piece of code is doing what makes sense.

richfitz commented 10 years ago

The other reason why 1 is the Right Thing is then you can take out all of the ncores arguments that are proliferating around and replace them with parallel which takes a logical argument. If parallel=TRUE. Then replace Jon's function with this:

.get.parallel <- function(..., parallel=FALSE) {
  if (parallel) {
    parallel::mclapply
  } else {
    lapply
  }
}

Then things like this: https://github.com/mwpennell/geiger-v2/blob/master/R/medusa.R#L3-L11

Use the code by saying:

medusa <- function (phy, richness = NULL, criterion = c("aicc", "aic"), partitions = NA, model = c("mixed", "bd", "yule"),
    cut = c("both", "stem", "node"), stepBack = TRUE, init = c(r = 0.05, epsilon = 0.5), parallel=FALSE, verbose = FALSE, ...) {

    ## CHECK ARGUMENTS
    initialE <- init[["epsilon"]];
    initialR <- init[["r"]];
    sp = c(initialR, initialE);

    fx <- .get.parallel(parallel);

But for the love of the FSM drop the semicolons.

josephwb commented 10 years ago

Grr. Updated to 10.9.2, everything breaks.

I can build, but check/install fails:

* installing *source* package ‘geiger’ ...
** libs
gcc -arch x86_64 -std=gnu99 -I/Library/Frameworks/R.framework/Resources/include -DNDEBUG  -I/usr/local/include -I"/Library/Frameworks/R.framework/Versions/3.0/Resources/library/Rcpp/include"   -fPIC  -mtune=core2 -g -O2  -c APE_reorder_phylo.c -o APE_reorder_phylo.o
g++ -arch x86_64 -I/Library/Frameworks/R.framework/Resources/include -DNDEBUG  -I/usr/local/include -I"/Library/Frameworks/R.framework/Versions/3.0/Resources/library/Rcpp/include"  `/Library/Frameworks/R.framework/Resources/bin/Rscript -e "Rcpp:::CxxFlags()"` -fPIC  -mtune=core2 -g -O2  -c branches.cpp -o branches.o
gcc -arch x86_64 -std=gnu99 -I/Library/Frameworks/R.framework/Resources/include -DNDEBUG  -I/usr/local/include -I"/Library/Frameworks/R.framework/Versions/3.0/Resources/library/Rcpp/include"   -fPIC  -mtune=core2 -g -O2  -c mkn.c -o mkn.o
gcc -arch x86_64 -std=gnu99 -I/Library/Frameworks/R.framework/Resources/include -DNDEBUG  -I/usr/local/include -I"/Library/Frameworks/R.framework/Versions/3.0/Resources/library/Rcpp/include"   -fPIC  -mtune=core2 -g -O2  -c pic.c -o pic.o
gcc -arch x86_64 -std=gnu99 -I/Library/Frameworks/R.framework/Resources/include -DNDEBUG  -I/usr/local/include -I"/Library/Frameworks/R.framework/Versions/3.0/Resources/library/Rcpp/include"   -fPIC  -mtune=core2 -g -O2  -c util.c -o util.o
g++ -arch x86_64 -I/Library/Frameworks/R.framework/Resources/include -DNDEBUG  -I/usr/local/include -I"/Library/Frameworks/R.framework/Versions/3.0/Resources/library/Rcpp/include"  `/Library/Frameworks/R.framework/Resources/bin/Rscript -e "Rcpp:::CxxFlags()"` -fPIC  -mtune=core2 -g -O2  -c utilities.cpp -o utilities.o
g++ -arch x86_64 -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_lookup -single_module -multiply_defined suppress -L/usr/local/lib -L/usr/local/lib -o geiger.so APE_reorder_phylo.o branches.o mkn.o pic.o util.o utilities.o -L/Library/Frameworks/R.framework/Resources/lib -lRblas -L/usr/local/lib/gcc/i686-apple-darwin8/4.2.3/x86_64 -L/usr/local/lib/x86_64 -L/usr/local/lib/gcc/i686-apple-darwin8/4.2.3 -lgfortran -L/Library/Frameworks/R.framework/Resources/lib -F/Library/Frameworks/R.framework/.. -framework R -Wl,-framework -Wl,CoreFoundation
ld: warning: directory not found for option '-L/usr/local/lib/gcc/i686-apple-darwin8/4.2.3/x86_64'
ld: warning: directory not found for option '-L/usr/local/lib/x86_64'
ld: warning: directory not found for option '-L/usr/local/lib/gcc/i686-apple-darwin8/4.2.3'
ld: library not found for -lgfortran
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [geiger.so] Error 1
ERROR: compilation failed for package ‘geiger’
* removing ‘/Users/josephwb/Desktop/GEIGER/geiger-v2/geiger.Rcheck/geiger’

Freakin' clang... Easy enough to install/symlink alternative/decent gcc, but not so easy for Joe-Shmoe.

BTW, I don't believe in the FSM, but I do believe fervently in semicolons.

richfitz commented 10 years ago

On my 10.9.2 machines, .R/Makevars contains:

CXX=clang++
CC=clang
SHLIB_CXXLD=clang++
FCFLAGS = -g -O2 -mtune=native -fbounds-check
FFLAGS = -g -O2 -mtune=native -fbounds-check

which should let you link properly. I believe that the "g++" included in OSX is actually aliased to clang. FWIW, having got used to clang's error messages I like it. But the bunfight between the FSF and Apple is causing me a few grey hairs.

Do you use semicolons in Python too?

josephwb commented 10 years ago

I; use; semicolons; everywhere;

On 26 March 2014 23:55, Rich FitzJohn notifications@github.com wrote:

On my 10.9.2 machines, .R/Makevars contains:

CXX=clang++CC=clangSHLIB_CXXLD=clang++FCFLAGS = -g -O2 -mtune=native -fbounds-checkFFLAGS = -g -O2 -mtune=native -fbounds-check

which should let you link properly. I believe that the "g++" included in OSX is actually aliased to clang. FWIW, having got used to clang's error messages I like it. But the bunfight between the FSF and Apple is causing me a few grey hairs.

Do you use semicolons in Python too?

Reply to this email directly or view it on GitHubhttps://github.com/mwpennell/geiger-v2/issues/5#issuecomment-38766874 .

josephwb commented 10 years ago

@richfitz The reason I stay away from clang is that it does not support openmp, and so is essentially useless for a whole wack of stuff.

richfitz commented 10 years ago

Well you know what they say:

A programmer had a problem. They thought to themself, "I know, I'll solve it with threads!". has Now problems. two they

It sounds like openmp support is in the works, but with the way apple moves on compilers, it could be years before anything makes it into the Xcode-distributed versions...

josephwb commented 10 years ago

True enough. But if you are aware of such problems, they are easy to avoid/fix.

lukejharmon commented 10 years ago

I fixed this using Fitzjohn method (2) above. If someone wants to go through and implement (1) - which I agree is better - then go for it. Until then, geiger will eat all your cores unless you tell it not to.

cboettig commented 10 years ago

Hey awesome geiger folks,

Just a note that since the recent CRAN update, my travis checks started failing where I had called geiger's fitContinuous and not set the number of cores explicitly. (Setting ncores=1 fixed the issue).

The same checks without setting ncores explicitly ran just fine on my local machine. Seemed like it might be related to whatever changes in multicore handling happened here, and made me wonder if it might be a problem with the way multicore works in virtual/cloud machines? (Or maybe travis and my local copy just have different versions of something?) Anyway, just a heads up in case this rings a bell.

lukejharmon commented 10 years ago

Yes we know about this issue - I just went through and fixed all of the geiger code to set ncores=2 on all of the examples. It might be worth just setting ncores=1 as the default in all of the relevant geiger functions.

On May 23, 2014, at 2:02 PM, Carl Boettiger notifications@github.com wrote:

Hey awesome geiger folks,

Just a note that since the recent CRAN update, my travis checks started failing where I had called geiger's fitContinuous and not set the number of cores explicitly. (Setting ncores=1 fixed the issue).

The same checks without setting ncores explicitly ran just fine on my local machine. Seemed like it might be related to whatever changes in multicore handling happened here, and made me wonder if it might be a problem with the way multicore works in virtual/cloud machines? (Or maybe travis and my local copy just have different versions of something?) Anyway, just a heads up in case this rings a bell.

— Reply to this email directly or view it on GitHub.