-
-
Notifications
You must be signed in to change notification settings - Fork 93
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
add Intel TBB #710
add Intel TBB #710
Conversation
(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.13) |
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.02) |
@serban-nicusor-toptal How do the performance tests work when I use the "build with parameters" facility to use non-default stan and stan-math refs to build this PR? Moreover, is it possible to instruct the performance suite to test on macOS specifically (and maybe Linux)? |
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.14) |
(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.1) |
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.1) |
@wds15 Hey, It will get the base branch https://github.com/stan-dev/performance-tests-cmdstan/blob/master/compare-git-hashes.sh I am currently working on a custom branch in git and job for Jenkins to allow running the tests on mac/win/linux with more arguments for ease of customization. See:
Still needs a bit of refactoring to work on win. |
is this still status WIP? |
Yes...the tbb still needs to have settlement on the license |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR needs a merge with recent develop. I also have a question regarding the manually set CXX_TYPE for clang and g++.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally the following changes need to be made:
- cmdstan manual instructions need to be updated for the STAN_THREADS case, ming32-make, ...
- cmdstan wiki Licensing section should have the same note on GPL-2 compliance similar to the TBB Stan Math PR.
- update wiki with installation instructions for Windows (PATH and mingw32-make)
See more here: stan-dev/math#1376 (comment)
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0) |
Right now I am outputting on windows a message about adding the tbb library path to the PATH variable... but we could also just create a wrapper |
For reference, here is the windows search policy for dynamic libraries: |
By this you mean stan compiler? That sound like a nice convenience for Windows users and I very much approve. But honestly, I dont feel this convenience .bat file is necessary in this initial TBB PR. If you feel that it will be easy to write then go ahead. If not, we can do this separately after this initial batch of TBB PRs get in. |
Or did you mean that a user would run their models through this .bat file? |
Yes, users would call the .bat file instead of the .exe file. I would generate the .bat with the makefile. So calling the Bernoulli example becomes
that's it. The bernoulli.bat just wraps the exe call which it will precede with a reset PATH variable. |
This means that PATH is set each time a model is run? Or am I misunderstanding. What about just setting PATH on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is almost ready, the doc changes are great, user warnings also.
Please fix the build path issue and remove the changes to the stan submodule reference.
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0) |
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.99) |
I just did
and it did not change anything as it looks to me. So I am not sure what I should update wrt to the submodule. EDIT: or did u mean I update the stan submodule to the respective stan with the changes for the TBB? I don't think that is ok... we always need to point to some stan develop branch in cmdstan - so I was assuming that we update cmdstan / stan / math and then the submodule auto-update will get it straight. |
If you look at the changes in the PR it looks as we are changing the stan submodule.which we should not. |
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Merge once it passes.
Cool. Let's also wait for the pipeline of stan-math PR-1180 to turn green before we merge this. Then we know that all will work out. (I really hope that this time Jenkins is stable, the thing started to give again flaky behavior here and there) |
Did you setup 1180 to the TBB stan and cmdstan branches? Yeah, it would be great if we had that green light before. I would then merge these three in quick succession and then deal with 1376. Then things will get easier. Will handheld Jenkins if needed, no problem :) |
On the bright side we have come to the point where the only thing keeping us from having TBB in is having to wait for the tests to finish. I am psyched :) |
Me too ... looking forward to this. So yes, I started the Jenkins for PR 1180 of stan-math using the "build with parameters" facility. That lets you test the 1180 with the respective upstream branches which match it. I will update the PR test for 1180. If you don't mind I will take out the performance things there as these are documented in 1376. BTW... the very last one to merge is the changes currently on the branch |
You mean the PR text? Yes, please do that. Just reference to 1376 for performance stuff. 1180 doesnt really change anything outside of Mac's use of tbbmalloc in terms of performance.
Yes, I remember. That should be a simple thing. |
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.01) |
…mdstan into feature/intel-tbb-lib
Now the final message looks nicer. |
Great, thanks. Cmstan and Stan PRs are ready and waiting. When/if 1180 passes we can then merge all three. If all goes well and Jenkins plays nicely we should be there in around 3 hours (1h for distribution test and 1h30min for upstream). I will definitely still be at my computer then to merge. Should I just do it then? If all goes green I think we should. And we then handle 1376 tomorrow once that passes? And the final Cmdstan PR to init the number of threads. |
Sure - go ahead once 1180 turns green. Awesome. |
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.06) |
(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.17) |
Submisison Checklist
./runCmdStanTests.py src/test
Summary
This adds the TBB targets to the makefiles. This PR needs to be merged with the Stan-math PR stan-dev/math#1180 and the stan PR stan-dev/stan#2769
Intended Effect
Make the Intel TBB available to stan.
How to Verify
Tests will link against the tbb, tbbmalloc & tbbmalloc_proxy libraries.
Side Effects:
Documentation:
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Sebastian Weber
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: