Skip to content

Move up to C++17 #1300

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.

Sign up for GitHub

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

Jump to bottom
Closed
SteveBronder opened this issue Aug 5, 2019 · 57 comments
Closed

Move up to C++17 #1300

SteveBronder opened this issue Aug 5, 2019 · 57 comments
Milestone
Future

Comments

@SteveBronder
Copy link
Collaborator

SteveBronder commented Aug 5, 2019 •
edited
Loading

Description

Jw, what are the blockers that stop us from moving to C++17?

There's a couple features that I think would be nice. From here

  • Nested namespace definitions, e.g., namespace X::Y { … } instead of namespace X { namespace Y { … } }
  • Fold expressions, for variadic templates (ala std::is_arithmetic<Args> &&...)
  • A compile-time static if with the form if constexpr(expression) (No more #ifdefs for the OpenCL code!)
  • copy-initialization and direct-initialization of objects of type T from prvalue expressions of type T (ignoring top-level cv-qualifiers) shall result in no copy or move constructors from the prvalue expression.
  • Additional mathematical special functions, including elliptic integrals and Bessel functions see here

From that same wiki page it looks like we would have to bump up the supported compiler versions?

GCC since version 7 has complete support for C++17. GCC C++ standards status
Clang 5 and later implement all the features of C++17. clang C++ standards status

Current Version:

v2.20.0
@SteveBronder
Copy link
Collaborator Author

Another reason is better support for devirtualization

I can run a branch to check the performance, but I think we can put a final over the chain methods

@andrjohns
Copy link
Collaborator

andrjohns commented Aug 5, 2019 •
edited
Loading

The main thing at the moment would probably be Windows, since that's restricted to the gcc version packaged with RTools, which is only 4.9.3 at the moment. Having said that, the next version of RTools will apparently ship with gcc 8 (https://cran.r-project.org/bin/windows/testing/rtools40.html)

@alashworth
Copy link
Member

Devirtualization should be pretty well supported in C++11; there's nothing stopping you from adding final in 4.9.3, AFAIK.

@syclik
Copy link
Member

syclik commented Aug 7, 2019

Jw, what are the blockers that stop us from moving to C++17?

This is really a question about what are the blockers from requiring a higher version C++ compiler. Something to note: different versions of compilers support incomplete parts of the spec. For example, see C++ Standards Support in GCC.

Although, I think technically we (the developers of the Math library) could pick a version that's different than what's currently supported across Stan, we shouldn't do that lightly. Here are some of the things we care about:

  1. users of the Stan interfaces are able to use Stan. That's RStan, PyStan, and CmdStan.
  2. we are able to provide clear instructions on how to get a toolchain installed for these users
  3. we are able to support users that are on Windows, Linux, and Mac.

If we wanted to change our requirements to say something like g++ 7, we would need to verify that rstan, pystan, cmdstan users could get it working on windows, mac, and linux. It'd require us knowing how to install g++ 7 on different platforms: that's been one of the limiting factors.

At some point, we have to leave a subset of users in the dust. For a long time, we supported g++ 4.2, but we had to move on in order to use C++11 and C++14 features. We're stuck on g++ 4.9 because it's hard to install a toolchain under Windows. If you can figure out how to install g++ 7 on Windows reliably and we can use that for RStan and PyStan, we could probably move the minimum requirements at that point.

@SteveBronder
Copy link
Collaborator Author

Devirtualization should be pretty well supported in C++11; there's nothing stopping you from adding final in 4.9.3, AFAIK.

Yep! I made a branch trying this on the chain methods, but sadly I really didn't see any speedups. Probably because we have a small inheritance structure

If we wanted to change our requirements to say something like g++ 7, we would need to verify that rstan, pystan, cmdstan users could get it working on windows, mac, and linux. It'd require us knowing how to install g++ 7 on different platforms: that's been one of the limiting factors.

I think this is blocked until Rtools40 comes out. pystan has conda and cmdstan can probably do whatever it currently does. Do we have a doc somewhere about this? The most I see is Stan maths windows dev notes

It looks like msys2 is the windows solutions?

At some point, we have to leave a subset of users in the dust. For a long time, we supported g++ 4.2, but we had to move on in order to use C++11 and C++14 features. We're stuck on g++ 4.9 because it's hard to install a toolchain under Windows. If you can figure out how to install g++ 7 on Windows reliably and we can use that for RStan and PyStan, we could probably move the minimum requirements at that point.

I think it's best to leave this open until Rtools40 comes out. Then we can make sure we are aligned with Ben on this

At some point, we have to leave a subset of users in the dust.

Tbh I've always wondered who these people are. Who has not updated their compiler in the last 12 years?

@rok-cesnovar
Copy link
Member

Tbh I've always wondered who these people are. Who has not updated their compiler in the last 12 years?

I think that is mostly people that dont use a compiler outside of Stan. And those that run Stan on clusters not maintained by them.

@wds15
Copy link
Contributor

wds15 commented Aug 7, 2019

Yes, we should be kind to those cluster users. On clusters it is hard to impossible to install a compiler. We should do some research on what the big distributions provide. The rhel which we use at work only brings a g++ 4.8 compiler as stock compiler. It was hard to convince IT to install a newer development compiler on our cluster.

So what do the large commercial distributions bring with them and what are common cloud images people can easily get up and running.

If we enforce that people must install compilers, then we will loose (some of) these users which run Stan for large scale problems...and we want to solve big stuff, don’t we?

@SteveBronder
Copy link
Collaborator Author

It's a super bummer that RHEL7 is stuck at gcc 4.8.x, but idt RHEL is a blocker here since we use c++14 and gcc didn't support that fully till gcc-5. So if people are using RHEL with Stan then they already added gcc 5 themselves right? If they can update to 5 then I'm not sure what the blocker is to update to 6, 7, or 8

https://gcc.gnu.org/projects/cxx-status.html

imo I think the only blocker here is waiting for rtools 40 to come out so it's easy to setup on windows

And like, meta, it's not that I care about C++17 that much, but I do care about C++20 and if we are ever going to be able to move up to 20 then moving to 17 first is probably a good idea. I think the nice update path here would be

  1. Wait till rtools 40 comes out
  2. For the Stan release after that tell people that in X (like 2 or something) releases we are going to move up to C++17
  3. We can have the upstream packages check the compiler version of users to see if that will be an issue for them and if so give a little warning message
  4. After X releases do the upgrade

@bob-carpenter
Copy link
Member

bob-carpenter commented Nov 4, 2019 via email

@wds15
Copy link
Contributor

wds15 commented Nov 4, 2019

Rtools is about to do a huge jump in gcc versions. That's quite a change to say now we use the RTools windows as the defacto standard for all platforms (and not just for Windows).

We will loose users if we are too aggressive with bumping C++ standards too quickly.

@bgoodri
Copy link
Contributor

bgoodri commented Nov 24, 2019

I think we'll be fine with RStan in April, but I think @ariddell is reluctant to move PyStan to C++17.

@riddell-stan
Copy link

riddell-stan commented Nov 24, 2019 •
edited
Loading

We lose the ability to ship "binary versions" of PyStan if we move to C++17 now. Binary versions ("wheels") mean that the package contains libraries pre-compiled for the relevant system. For users, having these wheels is really nice. It makes installation as fast as other packages in the same space (e.g., pymc).

Here's the most recent "platform" which we have to target on Linux if we want to build wheels: https://www.python.org/dev/peps/pep-0599/ . GCC 4.8.2 is the compiler.

I suspect that moving to C++17 would be possible within 12-36 months. Python moves relatively quickly on this.

Moreover, I do think keeping things to C++14 for a bit makes it easier for people in academia who have centrally-managed Linux servers. These servers almost always run RHEL or Centos. They tend to have older compilers.

@wds15
Copy link
Contributor

wds15 commented Nov 24, 2019

I can only echo what @riddell-stan says! Asking users on clusters to deploy a compiler is a big call. On these clusters usually RHEL is running (and not necessarily the latest).

Keeping things working with RHEL (now at gcc 4.8.2) is a great plus for Stan. I know that this poses considerable restrictions on C++ and thus on developers, but the jump to more recent gcc versions should absolutely not be taken lightly (I am myself in the fortunate situation that IT is deploying now a g++ 8.2 compiler in the next production, which is great... but not standard at all).

@SteveBronder
Copy link
Collaborator Author

Looking into centos 7, does pystan also work with 2.7? I don't know much about this but from the thread below it looks like yum only works with 2.7?

https://www.centos.org/forums/viewtopic.php?t=70838

I'm fine with waiting a year, that also gives us a good enough amount of time to let users know about the plan to move up. If everyone is fine with the one year tag then we can put a notice up next release saying, "In 4 releases (one year) stan math will be moving up to the C++17 standard"

@bgoodri
Copy link
Contributor

bgoodri commented Nov 24, 2019 via email

@riddell-stan
Copy link

riddell-stan commented Nov 24, 2019 via email

@SteveBronder
Copy link
Collaborator Author

NVIDIA nvcc has not done anything with C++17. Is that going to be an issue
for people who want to use NVIDIA GPUs?

We use OpenCL so we don't need to worry about nvcc

@alashworth
Copy link
Member

I think moving up C++ standards would be helpful for developers, just from an ease-of-use standpoint (also I am unreasonably excited about c++20 modules and compilation time improvements). I do understand the pains of of having to use old compilers though on production servers. In that case, maybe one solution is to distribute fully statically-linked Stan binaries; heterogeneous compute clusters would need some type of fat binary though to keep it easy. Maybe even an automated installer. (I have not checked all the relevant licenses to see if this is possible, this is just me typing my initial thoughts.)

I will think about this more. I know I could get something decent going on Linux and Windows, but I don't know much about Mac package distribution.

@bob-carpenter
Copy link
Member

bob-carpenter commented Nov 27, 2019 via email

@SteveBronder
Copy link
Collaborator Author

SteveBronder commented Jan 10, 2020 •
edited
Loading

From here

But big changes are coming to R with version 4.0.0, which is expected to be released not long after R's official 20th birthday on February 29, 2020. (The CelebRation 2020 conference will mark the occasion in Copenhagen.)

@riddell-stan

I would suggest revisiting this question in 9 months. I would prefer
waiting until Python has a binary wheel format which works with C++17
(likely one based on Centos 8, which uses gcc 8).

Is there a place online with discussion about this? idk much about python/c++stuff? This blog post looks like you can use C++17 stuff but idk if that post makes sense in your context

https://thomastrapp.com/blog/building-a-pypi-package-for-a-modern-cpp-project/

EDIT: Is this the relevant pep?
https://www.python.org/dev/peps/pep-0600/

@riddell-stan
Copy link

riddell-stan commented Jan 10, 2020 via email

@SteveBronder
Copy link
Collaborator Author

Gotcha thanks! Do you have a link to the toolchain / process you use now that you can slam over to me? If it's reasonable, one of the issues on manylinux has a docker file with centos 5 and a gcc9 toolchain compiled from source. If it seems like something worth trying out I'd be fine to sit down and try to compile the wheel / pass tests with c++17 and that dockerfile

@riddell-stan
Copy link

riddell-stan commented Jan 12, 2020 via email

@mcol mcol added this to the Future milestone Feb 15, 2020
@andrjohns andrjohns mentioned this issue Apr 1, 2020
Closed
5 tasks
@SteveBronder
Copy link
Collaborator Author

R 4.0 is coming out on the 24th with gcc-8 support so I wanted to bump this again.

@riddell-stan I was trying to read through the manylinux pep and it looks like manylinux2014 uses centos 7. Would that work for you? I don't understand the first thing about distributing stuff like this, but theres a lot of neat little featueres in C++17 that would let us do more cool stuff

If we can figure out they python stuff we can at least figure out a deprecation schedule to move from 1y to 17

@wds15
Copy link
Contributor

wds15 commented Apr 21, 2020

Not sure how we settle this...my opinion is that we need a longer grace period before we start with c++17...like a year or so.

@bgoodri
Copy link
Contributor

bgoodri commented Apr 21, 2020 via email

@SteveBronder
Copy link
Collaborator Author

I'm fine with saying in 1 year but would like to put out some date so we can start telling users now in our releases so they can also prepare

@riddell-stan
Copy link

@riddell-stan I was trying to read through the manylinux pep and it looks like manylinux2014 uses centos 7. Would that work for you?

Here's what using the Centos 7 build environment means one has to work with:

GLIBC_2.17
CXXABI_1.3.7, CXXABI_TM_1 is also allowed
GLIBCXX_3.4.19
GCC_4.8.0

Is gcc 4.8.0 and GLIBCXX 3.4.19 recent enough for you? That's not C++17 compatible right?

If I had to guess, I would say there will be a newer Linux reference build system available by May 2021. (The C++14 compatible reference build system for Python (Linux) was fully functional only about a week ago.)

If the benefits of using C++17 are really great and folks want to go ahead, leaving PyStan behind for six months or a year would not be the end of the world. But I hope we could be patient about this.

@t4c1
Copy link
Contributor

t4c1 commented Apr 22, 2020

I am also quite exited about if constexpr.

@alashworth
Copy link
Member

I know this is a pretty ignorant question, but I'm not familiar with PyStan. How does it wrap Stan? Theoretically speaking, what stops PyStan from packaging a compiler with it, and statically linking everything it uses?

@riddell-stan
Copy link

riddell-stan commented Apr 22, 2020 via email

@bgoodri
Copy link
Contributor

bgoodri commented Apr 22, 2020

Not for R certainly. @riddell-stan Can you even build anything based on Stan 2.20 or later with that g++-4.8 on CentOS? I think RedHat patched 4.8 so that it would accept the -std=c++1y flag but it does not actually implement any of the C++14 specific features that came with 4.9.

@riddell-stan
Copy link

riddell-stan commented Apr 22, 2020 via email

@SteveBronder
Copy link
Collaborator Author

@riddell-stan have you thought about trying out the solution here that uses centos5 and gcc9?

pypa/manylinux#118 (comment)

@alashworth
Copy link
Member

alashworth commented Apr 22, 2020 •
edited
Loading

It's true that no other python package ships a compiler, but not many other packages are trying to compile things at runtime, and trying to be usable for non-programmers. Is this actually a valid approach that people might want to explore? A gigabyte on the hard-drive in exchange for less headaches for the Stan developers and the Stan packagers?

@riddell-stan
Copy link

riddell-stan commented Apr 23, 2020 via email

@riddell-stan
Copy link

riddell-stan commented Apr 23, 2020 via email

@SteveBronder
Copy link
Collaborator Author

Actually we found a solution for the static matrix stuff that doesn't require c++17 so that's not a reason to bump anymore.

Yes. We actually used this before the official C++11-supporting
reference environment was released/finalized.

Are you referencing my comment above about the custom container?

The problem is that these ad-hoc images are not updated or maintained.
We really want to use the standard images maintained by the Python
community. It's not as if they will not ever come out with
C++17-supporting images. They will. It will just take time.

I agree that would be a bummer. But I think I'm getting confused. I'm looking at the manylinux repo and their docker file pulls from

FROM quay.io/pypa/manylinux2010_centos-6-no-vsyscall

Going there I'm seeing they have manylinux2014_x86_64 etc where manylinux2014 uses devtoolset-8 which supports gcc-8 (gcc-8 has full c++17 support). I don't know much about compiling with these sorts of containers, is it difficult to switch dockerfiles or will the manylinux2014 container not work for your usecase? That seems to be an officially supported container

If we had one or two more PyStan developers, some of these
considerations would look different. I think we should prioritize
getting out timely releases on platforms we fully support. We should
accept being conservative with features in order to achieve this goal.

Agree, resources can be scarce! There's no set timetable for this yet and I agree we shouldn't have it take up too much dev time. We definitely need to wait a bit to give out both a deprecation notice and for people to upgrade Rtools

@riddell-stan
Copy link

riddell-stan commented Apr 25, 2020 via email

@SteveBronder
Copy link
Collaborator Author

@riddell-stan circling around to this because I had a bit of free time, is the wheel build done in .appveyor.yml? If not can you point me to where that goes on?

@riddell-stan
Copy link

pystan 3 / httpstan uses manylinux2014 now without issues. Linux wheels are built by travis at https://github.com/stan-dev/httpstan-wheels.

@rok-cesnovar
Copy link
Member

rok-cesnovar commented Jan 12, 2021 •
edited
Loading

@riddell-stan does that mean that this bump is fine for pystan/httpstan now and it would not cause any issues?

This bump is definitely out of the question for the January release of 2.26. But is moving up to C++17 for the end-of-April release doable? That would mean a year after R4.0 came out. Is 4 months of a heads-up enough? I would say yes.

I know we have been conservative with this due to various reasons (pystan/rstan/windows/clusters), but we have now been spending a lot of resources (mostly dev time) to work around these self-imposed limitation of staying C++1y compliant. Stan Math has evolved a lot in the past year and I think we need to decide when we will bump and just swallow that pill.

@riddell-stan
Copy link

riddell-stan commented Jan 12, 2021 via email

@SteveBronder
Copy link
Collaborator Author

I think what I would like is to move from C++1y to C++14 in the next release and then the release after that move from C++14 to C++17. The move from 1y to 14 would fix the windows issues I think and then that also gives users two quarters of a deprecation notice.

We can do tricks like use -static-libstdc++ -static-libgcc -- but this has drawbacks.

@riddell-stan what are the drawbacks in particular? I'm unfamiliar with compiling with these. If Pystan is our uncertainty spot maybe we should do this from the top down aka have rstan / pystan etc move to C++17 and then the math library can bump without issue

Maybe this is also a good opportunity to have a vote?

@SteveBronder
Copy link
Collaborator Author

Also @riddell-stan is pystan able to use the manylinux2014 container as well?

@riddell-stan
Copy link

riddell-stan commented Jan 12, 2021 via email

@SteveBronder
Copy link
Collaborator Author

That's reasonable

There's a branch below that uses constexpr if in all of the probability distributions and bumps the version to 17.

https://github.com/stan-dev/math/tree/experimental/cpp17-only

@bgoodri
Copy link
Contributor

bgoodri commented Jan 12, 2021 via email

@SteveBronder
Copy link
Collaborator Author

SteveBronder commented Jan 12, 2021 •
edited
Loading

For if constexpr in particular we could use BOOST_IF_CONSTEXPR but personally I'd really like to avoid having macros everywhere (you can see in the example branch we would have to use this everywhere).

But there's a lot of stuff that we can't macro out. There's a list of C++17 niceties here here of new C++17 features (full list here). In particular things like structured bindings, Template argument deduction for class templates, folding expressions, etc. Having these features would clean up a lot of current stuff that's gross we need to do that is simple with 17

@SteveBronder
Copy link
Collaborator Author

@bgoodri does rstan still need to test windows on older gcc versions or is the minimum version of gcc now 8+?

@bgoodri
Copy link
Contributor

bgoodri commented Jan 13, 2021 via email

@SteveBronder
Copy link
Collaborator Author

Looking at the support sheet it seems like gcc8 is the version that finalizes 17

https://gcc.gnu.org/projects/cxx-status.html#cxx17

@bgoodri
Copy link
Contributor

bgoodri commented Jan 13, 2021 via email

@SteveBronder
Copy link
Collaborator Author

ah icic, looking over that it mostly seems like the stuff that didn't get into 8 is related to working with chars, strings, file system, the parallism TS, and the memory_resource header. I think thats fine?

@rok-cesnovar rok-cesnovar mentioned this issue May 17, 2021
Closed
@WardBrian WardBrian mentioned this issue May 6, 2024
Merged
4 tasks
@WardBrian
Copy link
Member

We bumped to C++17 in #3110

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Future
Development

No branches or pull requests