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

rstan fails to build with newer versions of TBB #1041

Closed
kevinushey opened this issue Feb 27, 2023 · 22 comments
Closed

rstan fails to build with newer versions of TBB #1041

kevinushey opened this issue Feb 27, 2023 · 22 comments

Comments

@kevinushey
Copy link

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
Copy link
Author

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
Copy link
Contributor

bgoodri commented Feb 27, 2023

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
Copy link
Author

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
Copy link
Contributor

bgoodri commented Feb 28, 2023

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
Copy link
Author

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

@Enchufa2
Copy link

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
Copy link

Same issue: #924

@andrjohns
Copy link
Contributor

@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
Copy link

Isn't this supposed to be added by this?

CxxFlags <- function(as_character = FALSE) {
if (dir.exists(Sys.getenv("TBB_INC"))) {
TBB_INC <- normalizePath(Sys.getenv("TBB_INC"))
} else {
TBB_INC <- system.file("include", package = "RcppParallel", mustWork = TRUE)
}
if (file.exists(file.path(TBB_INC, "tbb", "version.h"))) {
CXXFLAGS <- paste0("-I", shQuote(TBB_INC), " -D_REENTRANT -DSTAN_THREADS -DTBB_INTERFACE_NEW")
} else {
CXXFLAGS <- paste0("-I", shQuote(TBB_INC), " -D_REENTRANT -DSTAN_THREADS")
}

@andrjohns
Copy link
Contributor

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
Copy link

RcppParallel autodetects the system tbb, and:

> RcppParallel:::CxxFlags()
-I/usr/include/tbb

@andrjohns
Copy link
Contributor

RcppParallel autodetects the system tbb, and:

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

@Enchufa2
Copy link

Enchufa2 commented May 22, 2024

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

@andrjohns
Copy link
Contributor

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
Copy link

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

@Enchufa2
Copy link

Enchufa2 commented May 22, 2024

@kevinushey I opened a new issue for this RcppCore/RcppParallel#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
Copy link
Contributor

@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
Copy link

@andrjohns
Copy link
Contributor

Right, that log is setting TBB_INC and TBB_LIB though?

@Enchufa2
Copy link

That particular log it is, yes. So?

@Enchufa2
Copy link

Enchufa2 commented May 22, 2024

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

  • RcppParallel implemented tbb autodetection. That is broken now, which is a regression, and we will fix that. The idea is to build RcppParallel with system tbb where available, which is what I'm doing in cran2copr (with a workaround, which will be removed when this regression is fixed).
  • Now, would it be possible to build packages linking to StanHeaders, using the same tbb version as RcppParallel, without any special workaround too?

@Enchufa2
Copy link

Enchufa2 commented May 23, 2024

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.

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

No branches or pull requests

4 participants