TBB and 2.21

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

Yes. I don’t expect anyone to build C++ to know how to integrate our makefiles into their build process.

Yes! But only if it dramatically changes how things are built and are required.

Prior to this, it used to be header-only with an optional runtime Sundials.

With this, that old procedure does not work. You can’t get it to build that way. You now have to go and build a library and link against it to make it work.

That is a break.

I agree that this is confusing. I tried to bring this up in the licensing discussions and tried to make it a TWG thing. This is a cross-cutting issue and I brought this up a few times and essentially told not to care about it. For Math, I feel like we have an obligation to follow software best practices.

My opinion: everything goes to 3. On the basis of licensing. If someone had depended on this AND wrote GPLv2 code AND distributed it, after this release, that person is in violation of the license just by upgrading.

I don’t think it’s sufficient to have it in release notes. We can talk about this later today.

I recalled that we discussed a bit of the versioning at last months meeting. I was ok then with naming it v2.21, but that was based on the info at the time.

The implementation has changed a ton since last month. It was still being discussed how TBB would be optional at the time. So I believe we’re evaluating this under different conditions. If I’m mistaken, please let me know.

This is really something that we’re doing for our users and what they would expect to happen. I think we can come to a common understanding and make a recommendation that we’re all comfortable with at the meeting. The software itself doesn’t change due to what we call the version.

I will not be able to attend the Math meeting so here is my thoughts on this.

I dont see how the implementation changed. We agreed that we will merge TBB for the next release if Sebastian is also able to add TBB to map_rect. Which was done and proved to be very efficient.

Sebastian at first wanted to implement it so that its only linked when STAN_THREADS is on, but then changed that based on feedback from Bob and Daniel on Discourse to just link it always. The fact that TBB is dynamically linked was known before, as we discussed that users will need to add TBB to their PATH variables on windows on the August Math meeting. And decided that was fine if the users get clear instructions. Which was done, there are now multiple convenience features added for Windows users that make this as easy as possible.

In terms of versioning I dont really have a strong opinion, whatever works and gets us moving forward. I dont feel that this warrants a Stan 3 release, but that is not my call to make. Its also probably a bit late for this discussion.

1 Like

Thanks, @rok_cesnovar! It’ll either be v2.21 or v3.0.

Thanks for confirming. That’s exactly what changed; to me, that’s substantial. It changes it from an optional process to a mandatory process.

And yes, the linking has always been the same.

Sorry if this is frustrating to you. If it makes you feel any better, I’m frustrated too. For me, I feel like I’ve been saying the same thing and even brought it up when we had to discuss the licensing issue with the rest of the development team. I could let what was described in the issue and in August go because it wasn’t required, but now it affects all users.

From a software perspective, using semantic versioning, backwards incompatible changes mean that the major version gets bumped. There’s been a discussion that’s happening about what’s in the “API” and @seantalts has tried to codify that. I see these particular changes as backwards incompatible:

  1. the requirement that a user must link something in. (We have been a header-only library with the exception of Sundials, which is optional. Now, anyone trying to use Math as a header-only library will not have that option.)
  2. the licensing dependencies that we have introduced are backwards incompatible with GPLv2 under the condition when it is distributed as a binary.

So… if you agree that either one of those conditions is backwards incompatible with Math v2.20 (all the way down to v2.0), then technically we should just move to v3. If you can let me know why these things are backwards incompatible or how it doesn’t affect users, then we should stick with v2.21. I don’t view this as something that’s opinionated. I just view it as the rule that we’ve decided to follow and we should follow it.

2 Likes

I am not really that frustrated as I dont feel too strongly about either option. Hope it didnt read that way. I am on my phone and writing quickly. Sorry if it appears that way!

Had I known that non-conditional linking would warrant Stan 3 at the time when we decided to go from optional to mandatory I would not have approved a mandatory use of TBB. I was not aware of that at the time of that discussion. Not because I dont want too bump a major version but just to postpone this to the next release that will more likely have to be 3 for other reasons also.

1 Like

Thanks for letting me know! It’s really hard to get emotion through text.

=). That’s ok. This was the exact use-case I brought forward in August and at the time, we had discussed a conditional TBB. It’s all good… we discussed this at the meeting today (I’ll post notes later today) and we clarified a few things (once again… minutes to come later): technically, this is a non-backwards compatible change for the users of the Math library. So using semantic versioning, we should move to v3. This is not a breaking change for interfaces, so they can stick with a minor version change (unless they treat the licensing thing enough of a breaking change). And we’ll just have to deal with different versioning schemes for the time being.

1 Like

Thanks, Daniel. I appreciate the quick summary. I agree with this reasoning and the conclusion.

And yes, text is bad for judging emotion. Especially from non-native speakers like me. Glad we sorted it out.

1 Like