README needs to be updated to reflect TBB installation instructions

[syclik]

Description

The README.md at the top level needs to be updated to provide instructions for building the Math library properly.

This is the file that needs to be updated: https://github.com/stan-dev/math/blob/develop/README.md#required-libraries

Right now, it doesn't indicate how Intel TBB must be built in order for this to work. The current instructions are adapted instructions from header-only libraries and we should provide clearer instructions for our users.

Example

For "Required Libraries," there's no indication that Intel TBB must be built and that it's a different class of library than Boost or Eigen. For each of the libraries, all the source is included in Math, so it's missing the key information that it needs to be linked in.

For "Installation," it doesn't say how to build the Intel TBB library. There should at least be a note stating how it's supposed to be built from Math. Or a note on where to follow for instructions.

Current Version:

v3.0.0

[wds15]

We do not say that Sundials things are built either in the readme. The things you ask for are probably found in the CmdStan manual. If you think they should go in here as well, sure; but then the Sundials things should be mentioned as well, I think.

I am actually fine with the README as is - I mean if I would approach this afresh, then I read library XY is needed. Ok... libraries are usually something which need to be build by default.

If you approach this as "Stan-math used to be header-only"... ok, then flagging more verbosely that this changed is helpful.

Maybe we add a high-level note somewhere adequate that says "with Stan-math 3.0 changed from header-only to require building the Intel TBB"?

[syclik]

I get where you're coming from, but the simple question is: can you follow those instructions and build an executable with the Stan Math library that uses autodiff? The current answer is no. It used to be yes. The README should really get back up to the standard where someone coming at the project new could follow and build an executable.

Re: Sundials. That's a different category than TBB. TBB is now required. In the old directions, it was ok because we didn't set up an example that required Sundials. So the instructions were correct. In the current directions, just following those steps is not enough.

If you want to link to a page that will tell people how to build TBB, that's fine.

Btw, this is not approaching this as "Stan-math used to be header-only." This is approaching this as "I want to use Stan Math. What are the steps I should follow to build an executable?" If it's at CmdStan, that's nice, but it also must be here. Or linked from here. Or something... a user just needs to know how to build and run it.

On Wed, Nov 27, 2019 at 3:07 AM wds15 notifications@github.com wrote:

We do not say that Sundials things are built either in the readme. The things you ask for are probably found in the CmdStan manual. If you think they should go in here as well, sure; but then the Sundials things should be mentioned as well, I think.

I am actually fine with the README as is - I mean if I would approach this afresh, then I read library XY is needed. Ok... libraries are usually something which need to be build by default.

If you approach this as "Stan-math used to be header-only"... ok, then flagging more verbosely that this changed is helpful.

Maybe we add a high-level note somewhere adequate that says "with Stan-math 3.0 changed from header-only to require building the Intel TBB"?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/math/issues/1465?email_source=notifications&email_token=AADH6F6FO6VEG7OBAOMLLALQVYTCVA5CNFSM4JR6MC62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFIU3PA#issuecomment-558976444, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADH6F5BQAVTBKNJGWKG4X3QVYTCVANCNFSM4JR6MC6Q .

[bob-carpenter]

On Nov 27, 2019, at 1:42 PM, Daniel Lee notifications@github.com wrote:

I get where you're coming from, but the simple question is: can you follow those instructions and build an executable with the Stan Math library that uses autodiff? The current answer is no. It used to be yes. The README should really get back up to the standard where someone coming at the project new could follow and build an executable.

I never did anything special for Sundials or TBB and I can still run autodiff just by downloading stan-math from GitHub.

What kind of instructions do you want to see?

[syclik]

If you start from a clean git clone of Math, the instructions on the README don't work. That's what should be there.

To be more specific, I'm reading from the top of the README.md down:

1. the first code is to put that example c++ into a file. That works.
2. I see directions saying what's supposed to happen (and that specifically says that the paths need to be filled in with something else):
3. This is where it fails: it says this is "an example of a real instantiation" What I see:

$ clang++ -std=c++1y -I ~/stan-dev/math -I ~/stan-dev/math/lib/eigen_3.3.3/ -I ~/stan-dev/math/lib/boost_1.69.0/ -I ~/stan-dev/math/lib/sundials_4.1.0/include  -I ~/stan-dev/math/lib/tbb_2019_U8/include -L ~/stan-dev/math/lib/tbb -ltbb -Wl,-rpath,"~/stan-dev/math/lib/tbb" -D_REENTRANT foo.cpp
ld: warning: directory not found for option '-L/Users/daniel/stan-dev/math/lib/tbb'
ld: library not found for -ltbb
clang: error: linker command failed with exit code 1 (use -v to see invocation)

4. The last bit of the text on the page doesn't indicate how this is supposed to be solved.

So the README doesn't include instructions on how to build the executable. It could either point the user on how to build Intel TBB using the makefile, it could point them to a different page, or it could tell them they have to have it installed already (preferably with some link to how that's done). But as it stands now, the README is just incomplete (as of v3.0.0). It's not enough to build any executable.

[syclik]

@bob-carpenter, could you let me know if you can follow and build an executable by following those instructions from a clean git repo? (one that doesn't have Intel TBB prebuilt)

[wds15]

I see - you mean we need to have someone in mind who wants to use only stan-math to do C++ autodiff. To be clear your are looking for instructions which lead to a working executable - allowing for links to other documents where things may already be explained. Does that have to work on all platforms? Windows is problematic with dynamic linking (but Windows c++ users should know that).

Probably the best is probably to come up with a makefile skeleton which takes care of all the build things. Is that reasonable? I have done this for the performance-cmdstan repo a while ago already.

[syclik]

Yes, exactly. Yes, any solution that leads to a new user following the instructions and building is fine.

On Wed, Nov 27, 2019 at 5:23 PM wds15 notifications@github.com wrote:

I see - you mean we need to have someone in mind who wants to use only stan-math to do C++ autodiff. To be clear your are looking for instructions which lead to a working executable - allowing for links to other documents where things may already be explained. Does that have to work on all platforms? Windows is problematic with dynamic linking (but Windows c++ users should know that).

Probably the best is probably to come up with a makefile skeleton which takes care of all the build things. Is that reasonable? I have done this for the performance-cmdstan repo a while ago already.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/math/issues/1465?email_source=notifications&email_token=AADH6F6KX3KUBY23YTOCJ63QV3XPXA5CNFSM4JR6MC62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFK4ZGQ#issuecomment-559271066, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADH6FYMDQUVD27VJX6O43LQV3XPXANCNFSM4JR6MC6Q .

[john-myers]

Stan Math installation instructions have many opportunities for improvement.

Most Unix C++ programmers are used to using a standard "./configure" followed by "make", sometimes followed by "make install" cycle. Since the math system is mostly headers, you may punt the "make install". However, since you provide a makefile, it is a reasonable assumption that "make" would configure and make the program.

Instead, it prints out a help file.

Please put together a ./configure file for Unix, and make the makefile "make" command actually make the TBB library. Since TBB is a frozen version and is now required for Stan, you are responsible for giving sufficient installation instructions/processes for it.

Currently the configure and install process is hidden under runTests.py, instead of the existing makefile. Although this might make sense to a python programmer, I don't know, it is counter-intuitive to this C++ programmer. Normally I'm used to tests being run only after a successful installation. So it never occurs to me to use runTests to perform the configuration and installation itself. This shows up in the various documentation in a few places; somewhere at the top of something there was another "use runTests to check the program" when it really meant to install it. So I completely blew past it, and was stuck dead in the water for a long time.

The current README Installation instructions also bury this obscurely way, way down in the middle of a paragraph after the C example helloworld. As an immediate patch, please insert something like the following paragraphs up at the top: -------------------------------snip---------- Installation

The Stan Math Library is a C++ library which depends on the Intel TBB library and requires for some functionality (ordinary differential equations and root solving) on the Sundials library.

Frozen tested versions of these libraries have been included underneath the lib directory in your math-develop download, so you should not get the latest versions of these separately and install them yourself.

In order to configure and install the TBB library, the first thing you should do after you unzip the system is to go to your new math-develop directory and run: ./runTests.py test/unit/math/rev/core/agrad_test.cpp This will install TBB using the default settings of [?? 1 | ?? all] core [??s], and run a quick simple test to prove the system has been installed correctly and works.

??This will also install the Sundials library. ??The Sundials library is not needed for everyday functionality.

A simple hello world program using Stan Math is as follows:
etc. etc.----------------------end snip----------------------

I would then revert "make" back to the standard configure/install functionality, and put the current help functionality under "make help". Then always have "make" spit out a "Type "make help" for help on further make commands." message first thing. Since make will only be run once or twice, but is always run first if you're a newbie, having this message appear all the time will not be intrusive but highly useful.

To be clean, have a "make math" or "make stan-math" or "make stan" be the first line on the Common Targets list when make is run. To make the entire system. Documentation is the secondary category I'm interested in, the first category should be how to make the system.

One might think that "make tbb", a necessary subsystem, would also to be listed in Installation second behind "make math" under Common Targets. This would configure, make, and install the TBB system, perhaps printing out a useful "Use -L /path/to/lib -ltbb when linking" message. Although if installing the TBB is accomplished by "make", as it should be, this subcommand would simply redundantly provide peace of mind.

There is a controversy over whether TBB installs as default under Linux configured to use only one processor core, which I tend to believe. Or whether it is smart enough to configure itself with the possibility of using all processor cores in the system. Or whether a thread system is one layer up, running in software, and has nothing to do with the number of cores that the machine has. Or whether the Stan system is strong enough to use some or all cores, or will break obscurely on some commands if more than one core is used. It seems pointless to use a multithreaded system if you're not using multiple hardware cores, but maybe I'm missing something. Anyway, I do not have time to mess with this; so I am punting that rabbit hole, installing a default perhaps 1-core TBB, and trying to get the system to run with what I have. All this should be solved with ./configure.

Somewhere it says you can compile the system with make -j8 to compile it with 8 cores. This is ambiguous between the immediate compilation process running in fast parallel, which only helps save five seconds right now at system build time, which is what I'm guessing this actually means; vs compiling a system that will run in parallel using 8 cores forever after in the future, which is what I would expect from a decent configuration system. Both the program and the verbage should be improved to ensure the latter.

Please document somewhere how to configure TBB so it runs with multiple cores, and whether this will break any particular known commands or not, and whether, when they break, do they print anything out or is it simply silent. Please make the system default to running as many cores as possible, if this will not break too many major routines.

The runTests making process is extremely long and barfs out pages of make feedbacks. It is completely unobvious that it is installing TBB, or that the installation is successful, or where it ends up. Please if possible print in bright bold green "Configuring and installing TBB..." before the TBB portion of the installation starts, and "TBB successfully installed at /path/to/lib/tbb/libtbb.so" after success. So that people know that TBB was put in, and don't try to go get it from Intel themselves.

The makefile help format is confusing, because dashes are apparently used as subheadings although they are appearing in the place of a literal command. So if I see Common targets: Documentation:

• doxygen :Builds the API etc.

I don't know whether to type "make - doxygen" or "make -doxygen" or "make doxygen". I suggest leaving the dash out completely and simply having the command inspaced an additional two spaces. Unless, of course, you really mean "make - doxygen". Ed....looks like this shows up as a bullet under github, but it's a dash any time I print it out on the Linux system.

On your main splash page mc-stan.org/users/interfaces/math you state flatly The library is header-only, so there is nothing to install. This is absolutely incorrect. At the least, it needs a footnote As of Dec 2019, you must configure and install, then link to, the frozen, tested version of the TBB library that has full source included underneath the build. A ./configure and makefile is provided to do this for Linux systems. "

Hope this helps. good luck; it's an important system.

[wds15]

Remember that stan math relies on the free time of volunteers who work on it. Your post is beyond my time budget. Feel free to contribute. Right now there is an open pr for this issue and you can comment on it.

[SteveBronder]

@john-myers see #1552 the pr @wds15 is talking about

[bob-carpenter]

Thansk for the feedback. I revised the out-of-date header-only statement on

https://mc-stan.org/users/interfaces/math

That used to be true before we added the Sundials ODE solvers. We now also require linking against the Intel TBB.

On Dec 28, 2019, at 4:21 PM, Steve Bronder notifications@github.com wrote:

@john-myers see #1552 the pr @wds15 is talking about

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[wds15]

I think we can close this as #1552 did fix this, right?

[rok-cesnovar]

Yes, closing. Thanks!