I am not sure if I understand your comment. How do you suggest should STAN_NUM_THREADS be honored if you do not want to use getenv? Should the interfaces do that?
Yeah, never mind, its a environment variable not a define like STAN_THREADS. My brain just jumps to the conclusion that its a compile time define whenever I see STAN_*. I will call it a day. Start with fresh eyes tomorrow.
Ah…ok…will open a or tomorrow…maybe some fresh us eyes look while we sleep…
WIP PR is open, see below.
@rok_cesnovar I did some simplifications compared to yesterday.
Things to consider design wise from my view are listed in the Todo section of the PR. More discussion there, I suppose.
Thanks for letting me know—I hadn’t seen that. I wasn’t trying to open an already closed decision.
1 Like
Here are the notes from the 9/19/2019 meeting.
There are a few good fixes in this release. But no, there wasn’t a real big value in a final pre-GPLv2 release, which was why we were ok either going forward with this or not. @wds15, I don’t think the decision was properly reflected: we are ok including TBB with the release if we can make use of it by the code freeze date. If we can not, then we’ll have it with part of the next release. (We don’t need a final pre-GPLv2 release.)
Ok. I think we are basically there. I will then take away those additional if-def guards which right now make the TBB only mandatory with STAN_THREADS. There are a few if-defs which can go away then.
It looks like we have it close to ready - with 9 more days we are good by the time relatively sure.
Hi @syclik!
Just to make sure this doesn’t pass by without notice: As the TBB is now fully integrated into Stan-math, this turns Stan-math into a non header-only library! The TBB must be linked against in any circumstances (with/without threading). Doing so allows for the cleanest design.
The doc for this updated in the places where I came across.
It may appear unnecessary at the moment, but we can only by always linking in the TBB we can get around pouring quite some ifdefs into the code base which we decided against. Since we will almost for sure end-up with threaded warmup in the very near future, I would anyway expect that we will be in a threading-is-the-default world very soon. Once we are there, the TBB facilities are in heavy demand for any Stan program.
Sebastian
Sundials beat the TBB to the punch in making Stan math require linking.
And like Sundials, it looks like I didn’t need to do anything on my end on OS X—just pulling the latest seems to work.
Well…the difference to sundials is that if you do not use sundials functionality in your Stan program, then things still work even without linking to sundials. This is why pystan can get by without linking the sundials libraries. With the tbb this is different. For any Stan program you must link the tbb or it won’t work. That’s new.
And I spend a lot of time on making everything smooth.
Why would it require to run a job that doesn’t utilize threads? Is that necessary?
Yes…you need the tbb for all programs. It makes life for us developers a lot easier.
Edit: and as we will likely have threaded warmup in the very near future, I very much hope so, we will anyway live in a thread only world once we are there…
1 Like
@wds15, thanks for tagging me. I think this means we bump the major version. I think @seantalts is supposed to make the call, but after we make a suggestion.
What do you think? This seems like it changes what we used to be as the Math library. We’re no longer a header-only with optional link library for certain things. And… we have a new library dependency. If you’re using Math and are GPLv2 and distribute the code, then you will be in violation after this new release.
This is the API we’re using for semantic versioning: https://github.com/stan-dev/math/wiki/Supported-external-API
It doesn’t seem to say much about header only vs. requiring libraries, but if you want to bump Math to 3.0 or rename it to something else, that’s fine with me.
Thanks. If that’s it, then I believe we’ve broken the first bullet: “The Math Library header accessed via stan/math.hpp.” It’s implicit there, but at the time of writing (and up until this release), the header was all you needed to access the Math library. Now it’s not going to be true. Keeping it at v2.x indicates that it’s easy to upgrade, but in this case, it won’t be. Old code will work, but whatever scripts used to build will have to change in order to keep up. So… I think that’s enough to justify a bump in version.
Fortunately, I think this coincides with the new compiler, right? Unless there’s an objection, I think we should bump it forward to v3.0.0. I’ll make sure we get a sense of what the rest of the Math developers think when we meet tomorrow.
New compiler is staying at the 2 level.
Ok. Thanks for the update.
This is only technically true for Windows Stan Math users. For those the scripts would break. But in order to fix them you would only have to edit your PATH variable and things would still work as they did before. No changes to the scripts.
On the Cmdstan level nothing actually breaks. You dont even need to set the PATH variable if you dont want to. There is a backfall mechanism in place to copy the 1MB .dll to the .exe file folder if the user does not set the Path variable. And that keeps things working with no change.
I think @syclik considers the math library without its makefiles - but you are right in that if you use our makefiles then you are all set.
We can discuss later - I do understand the thought to flag that this Stan-math is special. However, I do also think that a general version mix would be super confusing to users. I mean would the idea be to go with Stan-math 3.0, but have stan 2.21 and cmdStan 2.21? So either everything is 3.0 or we stay with 2.21 and just make a bigger note in the release notes to flag this.
Given that “Stan 3” is a fixed term for our next big release which breaks backward compatibility, I think the better thing is to leave it with 2.21. That’s better than the other options from my view - this gives the least user confusion and devs with the Stan-math will have to read our release notes (which they should anyway do).
but let’s touch base on it.
But in that case things “break” each time we add any kind of new dependency (statically or dynamically linked) or upgrade the minimum required version of any dependency.
+1