Feature/intel tbb lib #1180
Feature/intel tbb lib #1180
Conversation
|
Q, is this going to be bad for non-Intel systems? |
|
Is there a restriction on the bitness of the Stan program when linking with an Intel TBB that was build with 32 bit mingw? I guess we are only using 32 bit integers at the moment, but I had started to write a script to convert to 64 bit integers. |
|
@SteveBronder there are a few mentions of changes to support ARM on the Intel TBB change log |
|
Also, Intel's Parallel STL, which is intended to be an implementation of the C++17 standard, uses TBB and Intel gave it to the LLVM repo and LLVM works fine on non-Intel hardware AFAIK. |
|
@bgoodri I am not sure why 64 bit integers are a consideration from your perspective for the Intel TBB. I don't see how that interacts, but I don't know. I guess we are good; not sure though. @SteveBronder No, this is not bad news for AMD, for example. Reasons:
What is certainly correct is that there are some ifdefs in their code make things work super-optimal on intel compilers and moreover the unit tests for the library are executed on Intel hardware only. I asked myself this questions and for the above reasons I came to the conclusion we are good. |
|
@seantalts Does Jenkins wipe out all previous results between commits when retesting? I am asking as I wonder if I need to do a manual clean once in a while. Thanks. |
|
Like if Stan moves to 64 bit integers and someone passes |
It very much should, but I have seen (rare) cases of a job exiting in a really unusual way (maybe the machine drops out of the network or the whole process segfaults) and then there can be stuff left to clean up. That said, I think it should get a new workspace even in that situation. |
|
I really do not see how 64bit integers are a problem for the TBB. So, yes, this will work to my expectation. |
|
Well in that case I didnt have to change the update-script at all and I will just revert back to what it was. |
build the Intel TBB on Windows (and not otherwise)
|
Ok. If you can revert that would be great (EDIT: ah, it's already done - great). Sorry for the confusion. I just filed an important update to the makefile logic with the TBB: The makefile check on windows for the mingw32-make was flawed. The issue was that the check was marked ".PHONY" which implied that the TBB library targets (which do depend on the makefile check) were never clean for make, because they did depend on something which is never clean. This way the make for the tbb libraries were always called even when the TBB was already build. I changed this such that the make check now depends on a file which I simply create by touch after a successful check for mingw32-make on Windows. This new behavior now ensures that you will only need mingw32-make to build the TBB and moreover the makefiles for the TBB are never triggered again once the TBB is build. So the cool thing now is that users only have to remember the mingw32-make thing when they actually build the TBB. They can continue using "make examples/bernoulli/bernoulli.exe" on windows (after the TBB has been build). It will just work, because the TBB makefiles are not touched any longer. |
Done. No problem. I was the one confused :)
Great! I think this simplifies it a lot. I will review the changes later. So to sum up:
Will we need to merge the "integrate TBB" PR to this branch or can you switch the PR base branch? |
What I do not know is how we handle the updates of the submodules. So the stan PR-2769 needs to be bumped to point to the right stan-math commit. Same thing for the cmdstan PR-710 which needs to point to stan PR-2769. Right now I am running the tests for PR-1180, but that run now uses PR-2769 and PR-710 in its upstream tests - so that should come out "green". I will start respective ones for stan and for cmdstan. The update to the makefile wrt to |
But how will that work if we first merge the math one to develop? Wont that break develop then? Develop tests wont have that info? Couldnt we avoid setting submodules by first merging the cmdstan PR? That wont break anything as the tbb targets will just be empty. Then merge the Stan one (the changes there are only for tests). And once that is in we merge this one and then the map rect one? |
|
Hmm... that order could work, yes. |
|
Ok, lets move the discussion to the cmdstan one and go one by one. |
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.99) |
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0) |
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.17) |
|
the Daily Stan Performance build failed and it seems related to this PR - https://jenkins.mc-stan.org/blue/organizations/jenkins/Daily%20Stan%20Performance/detail/Daily%20Stan%20Performance/1703/changes/ Can someone take a look? If we can't fix it by the 12th we should probably revert before the feature freeze. |
|
It seems that its trying to rune the command "Stan". And there are spaces in the path (Daily Stan Performance) which leaves me to believe that its probably some compile/link path not in quotes somewhere. |
|
This looks like the culprit: Specifically I am looking at:
Those nested quotes look ok? seems iffy. How can we be sure this fixed it? Get what we believe to be the fix merged and then manually restart the daily performance test? Hopefully we can replicate this locally otherwise this one is going to be slow to debug. |
|
I just filed a PR on cmdstan which initialises the TBB threadpool. I would assume that the performance tests gets triggered there as well. So if we use the build with parameters there, then we should have a chance to debug in finite time - since this allows us to use any math branch and we jump over the math tests and the Stan tests. I think you are right on and I will locally set this up and see what happens. Spaces should really be disallowed... I had this problem in the past with boost builds. I am tempted to disallow spaces... |
|
Ah, that's good that that's all it was. These "Daily " ones don't get triggered, they just run once a night on develop. It seems like it's just the path thing so if you can fix paths with spaces that should do it. Alternatively you could move to disallow spaces but that seems like a larger change that hinders backwards compatibility somewhat (e.g. if you upgrade your install could stop working) so we might wanna raise it at a meeting or something to be sure. |
|
The problem with space is resolved now. The bits for that are part of PR-1376. There are two Windows specific things which are introduced:
Both of these above things are not yet spread into the last corner of our pipelines. However, the math tests all run just fine with it - on Windows and on any other platform. So we should by all means be able to sort this out. It's by now - I think - a configuration matter. These configuration requirements are well documented and users having to do this are being helped. |
|
Where else do the PATH variable and mingw32-make need to go? CmdStan? |
|
We have that in cmdstan/stan and math. We are just having issues with Windows+Jenkins setup. |
|
If this is a Windows thing, you really should use the system-level (short) path See |
|
I guess my question is - what are the remaining issues? Because I think most of the Jenkins stuff should be purely reliant on CmdStan, so if CmdStan is fixed for e.g. paths with spaces in them, I'm not sure what else would need to change. |
|
The space thing is fixed for me. So the branch of PR-1376 has makefiles which can deal with
with such a weird path you get a working TBB binary. There is more trouble which I believe has to do with PATH stuff. I don't know about this short path thing yet. |
Summary
Adds the Intel TBB 2019 Update 8 to stan-math's makefile system.
The new dependency checklist has been filled out on the wiki.
The Intel TBB is dynamically linked to programs. For robust linkage with the final programs, the
rpathfacility of linkers are used. This hard-codes the absolute path to the TBB library into the final binaries such that no additional user settings have to be made.However, the Windows platform is special in two ways:
rpathlinking logic is not supported. Instead, thePATHvariable of the user must include the directory of the binary TBB libraries.mingw32-makemust be used to build the Intel TBB since only this variant of make is supported by the Intel TBB sources.The Intel TBB is introduced to the C++ stan-math sources in the PR #1376
Tests
The Intel TBB has been added as dependency to the tests such that the test itself is if it builds ok.
Side Effects
On Windows the
mingw32-makehas to be used from the RTools instead of themakecommand. Otherwise the TBB makefiles do not work.On Windows the
PATHvariable must include the directory where TBB binaries are stored which is configured to be by defaultstan-math/lib/tbb.Checklist
Math issue Add Intel TBB library #1179
Copyright holder: Sebastian Weber (copyright for the Intel TBB is with Intel, of course)
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit)make test-headers)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested