stan-dev / rstan

RStan, the R interface to Stan
https://mc-stan.org
1.02k stars 264 forks source link

rstan fails to build with newer versions of TBB #1041

Closed kevinushey closed 1 month ago

kevinushey commented 1 year ago

I saw this error in trying to build rstan on a recent submission of RcppParallel, which included an updated build of TBB:

clang++ -arch arm64 -std=gnu++14 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I"../inst/include" -I"../inst/include/boost_not_in_BH" -I"." -DBOOST_DISABLE_ASSERTS -DBOOST_PHOENIX_NO_VARIADIC_EXPRESSION -DBOOST_NO_AUTO_PTR -D_REENTRANT -DSTAN_THREADS  -I/opt/homebrew/Cellar/tbb/2021.8.0/include -DTBB_INTERFACE_NEW -I'/Users/kevin/Library/R/arm64/4.2/library/Rcpp/include' -I'/Users/kevin/Library/R/arm64/4.2/library/RcppEigen/include' -I'/Users/kevin/Library/R/arm64/4.2/library/BH/include' -I'/Users/kevin/Library/R/arm64/4.2/library/StanHeaders/include' -I'/Users/kevin/Library/R/arm64/4.2/library/RcppParallel/include' -I/opt/R/arm64/include -Wno-unknown-pragmas -Wno-unused-variable   -fPIC  -falign-functions=64 -Wall -g -O2  -c stan_fit.cpp -o stan_fit.o
In file included from stan_fit.cpp:33:
In file included from ./stan/services/diagnose/diagnose.hpp:10:
In file included from ./stan/model/test_gradients.hpp:7:
In file included from ./stan/model/log_prob_grad.hpp:4:
In file included from /Users/kevin/Library/R/arm64/4.2/library/StanHeaders/include/stan/math/rev/mat.hpp:12:
In file included from /Users/kevin/Library/R/arm64/4.2/library/StanHeaders/include/stan/math/prim/mat.hpp:6:
In file included from /Users/kevin/Library/R/arm64/4.2/library/StanHeaders/include/stan/math/prim/core.hpp:4:
/Users/kevin/Library/R/arm64/4.2/library/StanHeaders/include/stan/math/prim/core/init_threadpool_tbb.hpp:9:10: fatal error: 'tbb/task_scheduler_init.h' file not found
#include <tbb/task_scheduler_init.h>
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

There is a migration guide included here:

https://oneapi-src.github.io/oneTBB/main/tbb_userguide/Migration_Guide/Task_Scheduler_Init.html

Would it be possible to update StanHeaders to remain compatible with the newer releases of TBB, for a future RcppParallel update?

kevinushey commented 1 year ago

For reference, here's the very lazy way this is done in RcppParallel:

https://github.com/RcppCore/RcppParallel/blob/c6a0036d16700471dc4e486814310701ee53ddd7/src/options.cpp#L11-L19

bgoodri commented 1 year ago

For the next StanHeaders release to CRAN (which is being blocked by a few packages), it seems we already have the following lines in init_threadpool_tbb.hpp:

#ifdef TBB_INTERFACE_NEW
#include <tbb/global_control.h>
#include <tbb/task_arena.h>
#else
#include <tbb/task_scheduler_init.h>
#endif

If I change that to just

#include <tbb/global_control.h>
#include <tbb/task_arena.h>

and make oneTBB a SystemRequirement of StanHeaders, then should I expect all the packages to pass their R CMD check on CRAN at least?

kevinushey commented 1 year ago

Hmm... this is what we have in RcppParallel:

https://github.com/RcppCore/RcppParallel/blob/6f817167216e8954670ce8aba1c4fc732ff0da82/R/tbb.R#L62-L65

Which I think is our wonky way of seeing whether we're trying to use a "newer" version of TBB?

bgoodri commented 1 year ago

So, if I understand correctly, RcppParallel/src/tbb/src/tbb/tbb_version.h is in the RcppParallel_5.1.7 that you just uploaded to CRAN, but wasn't in 5.1.6. So, if I were to upload a StanHeaders with that ifdef TBB_INTERFACE_NEW or just include tbb/global_control.h and tbb/task_arena.h unconditionally, then everything should build against oneTBB instead of oldTBB?

kevinushey commented 1 year ago

I think you're right... at least, I hope so!

Enchufa2 commented 1 month ago

rstan (and packages using StanHeaders) do not build anymore in Fedora 40 and rawhide, where we have tbb 2021. The build failure is:

/usr/local/lib/R/library/StanHeaders/include/stan/math/prim/core/init_threadpool_tbb.hpp:9:10: fatal error: tbb/tbb_stddef.h: No such file or directory
    9 | #include <tbb/tbb_stddef.h>
      |          ^~~~~~~~~~~~~~~~~~

which is not present in tbb 2021. See e.g. this build log.

Enchufa2 commented 1 month ago

Same issue: #924

andrjohns commented 1 month ago

@Enchufa2 if you want to use a newer version of the TBB than 2020, you need to add -DTBB_INTERFACE_NEW to the compilation flags

Enchufa2 commented 1 month ago

Isn't this supposed to be added by this?

https://github.com/stan-dev/rstan/blob/50c3c6f4e44a849b17af7e9e2b8656cd93238a18/StanHeaders/R/Flags.R#L1-L12

andrjohns commented 1 month ago

Has the build set the TBB_INC environment variable? I can't see it in the logs.

StanHeaders is checking the TBB headers in the directory specified by TBB_INC to determine whether the set the flag, if you haven't set TBB_INC then it defaults to the bundled RcppParallel version

Enchufa2 commented 1 month ago

RcppParallel autodetects the system tbb, and:

> RcppParallel:::CxxFlags()
-I/usr/include/tbb
andrjohns commented 1 month ago

RcppParallel autodetects the system tbb, and:

Which is because you're setting TBB_INC in the RcppParallel build: the most recent log

Enchufa2 commented 1 month ago

No, that's an old workaround. Here's a ~fresh log~

andrjohns commented 1 month ago

That log is building the bundled TBB though?

** package ‘RcppParallel’ successfully unpacked and MD5 sums checked
** using staged installation
** preparing to configure package 'RcppParallel' ...
*** configured file: 'R/tbb-autodetected.R.in' => 'R/tbb-autodetected.R'
*** configured file: 'src/Makevars.in' => 'src/Makevars'
*** configured file: 'src/install.libs.R.in' => 'src/install.libs.R'
** finished configure for package 'RcppParallel'
** libs
using C++ compiler: ‘g++ (GCC) 14.1.1 20240507 (Red Hat 14.1.1-1)’
(tbb) Building TBB using bundled sources ...
make[1]: Entering directory '/builddir/build/BUILD/RcppParallel/RcppParallel/src/tbb/src'
Enchufa2 commented 1 month ago

Oh, it's not working! Something broke it then since that PR. :(

Enchufa2 commented 1 month ago

@kevinushey I opened a new issue for this https://github.com/RcppCore/RcppParallel/issues/216

@andrjohns Anyway, one way or another, RcppParallel was built with system tbb. It would be great that StanHeaders worked too without adding special flags.

andrjohns commented 1 month ago

@andrjohns Anyway, one way or another, RcppParallel was built with system tbb. It would be great that StanHeaders worked too without adding special flags.

Have you got a log where it was built with the system TBB?

Enchufa2 commented 1 month ago

The first one that you linked.

andrjohns commented 1 month ago

Right, that log is setting TBB_INC and TBB_LIB though?

Enchufa2 commented 1 month ago

That particular log it is, yes. So?

Enchufa2 commented 1 month ago

Let me rephrase because I'm not sure we are on the same page:

Enchufa2 commented 1 month ago

As it turns out, when autodetection is restored in RcppParallel, rstan builds fine, because RcppParallel::CxxFlags() adds -DTBB_INTERFACE_NEW properly.

So this can be closed. Apologies for the noise.