In this particular case I require CMake 3.15 because it better supports automatically detecting and using Boost 1.70+. This became an issue for me when I started compiling Boost from source in order to test Boost MPI, where I prefer using 1.70 since Boost started officially supporting CMake in this release. This is even more true in Boost 1.71 which just came out.
I agree manually installing dependencies is a pain. I hope people try Conan out and find it easy to use - you can use it to install CMake.
EDIT: But yes, I also use CheckPIESupported. If I require a lesser version of CMake, then I have to backport this functionality. I don’t like putting compiler-specific functionality in the build system since it’s more maintenance work. For example right now you can compile with Visual Studio, I think (not 100% on this - wasn’t interested in doing a full test, since it’s not supported officially, but it is nice to know that you probably could use it if you wanted to.)
Yes. Although it isn’t in the README this is one of my to-do items, just haven’t gotten around to it yet. It is mandatory because RTools doesn’t ship CMake at all.
I am afraid I do not know enough about how RStan is packaged to comment on whether or not they will need to use CMake. Could you elaborate more?
This monorepo does not change how the library source files are implemented. What is currently header-only remains header-only, so theoretically you don’t even need a Makefile to use it.
Hey @stevebronder or @rok_cesnovar - do you know why we switched the opencl include from < CL/cl.hpp> to <cl.hpp>? And why we include the opencl sources in Math? @alashworth was trying to get conan to download vanilla opencl and use that but had some issues with it that seem like they could be related, especially if we have some custom code in our cl headers.
That was me, it was just a thing when I updated the OpenCL headers (we hadn’t updated them for 2 years or so and there were a lot of bug fixes since them.
Not sure what you mean here, like why do we include the OpenCL headers? That header gives the C++ wrappers around the OpenCL C API that’s in the users OpenCL Installable Client Driver (ICD)
The only ‘custom’ thing for our header is that on github the file is called cl2.hpp and I switched it to cl.hpp
@alashworth can you link me to where you are having trouble? I don’t have much cmake/conan experience but can probably get something together
Yeah, I guess sometimes they come with the OS or the drivers, but now I’m seeing that even so you’d probably still want to vendor them if you don’t want to rely on a system install and the headaches involved in locating and versioning that… I mostly wanted to know if we had non-vanilla changes. Hopefully you and @alashworth can get something worked out :D Thank you both :D
The OpenCL headers that reside in lib/opencl_* were updated. But this will be in the next cmdstan release and since Rstan is not on 2.20 this isnt an issue right now.
Will the monorepo try to install boost, eigen, gtest, etc. from conan?
We have non-vanilla changes in Boost (there wont be any after the next upgrade) and GTest (something MPI related). I dont think we touch Eigen other than extend it in Stan Math for .val(),.adj().
Yes. Where possible I would like to avoid maintaining changes to packages we use unless necessary. It’s not particularly about Conan. Actually when I’m just testing my own changes I do not use Conan packages because it’s unnecessary with the distro I use except for Sundials. I would like to shrink the amount of code in the repository and let people use their system packages if possible.
I did not realize there were custom changes to Boost. Can you elaborate more on them? The tests are passing (mostly) and I would like to make sure I am not missing anything.
In that spirit I have also gotten rid of the MPI changes to GTest, and now there’s a custom test runner for MPI tests.
@stevebronder@seantalts
I emailed Steve about this, sorry, maybe it should have gone on Discourse. Anyway the problem on my end was that the distro OpenCL header package I was testing against was not updated since 2017. I uninstalled it and started using my vendor (nVidia) headers and everything is fine.
I realize there are ups and downs to packaging the OpenCL headers. It’s not a big deal either way. However, if we use the vendor OpenCL headers, then I don’t have to write custom OpenCL header handling code. I can instead use https://cmake.org/cmake/help/latest/module/FindOpenCL.html which is maintained by the CMake developers.
The CMake find module adds the include directory path such that you need to do <CL/cl.hpp>. This is the way nVidia, system distros, Conan, AMD, etc. package their headers and if you want to interoperate with them, that’s what you should do. Originally I guessed that the changes to the include path were meant to forbid or make it difficult to use vendor headers.
If all things were equal I’d just link against the vendor headers. Because in the end, they are the ones that provide the runtime DLL that needs to be linked against to run code. If the DLL doesn’t implement header functionality, you have a problem, regardless if morally they are supposed to follow the Khronos implementation. Still I want to emphasize it’s not a big deal either way, I can write code to force linkage against the Khronos headers, and there’s a Conan package that will automatically download them.
I completely understand that, dont get me wrong. I am pro using a package manager, 100%. We would probably all prefer that.
But the current fact is that we do have modifications to the dependencies and until we get rid of them, we need to find a solution to do that while still using the package manager.
Sure. The changes to boost:
commented out the deprecated header warning, see commit. This has been fixed in Boost 1.70+ so if you those versions there is no need for this. Otherwise it keeps spitting out the deprecated warnings. But runtime is fine. This was the PR the last upgrade happened in: https://github.com/stan-dev/math/pull/1223#issuecomment-489302445
the other one is a bugfix that has since been applied to their develop branch but has not been released yet. We did want to have it since it allows some performance boost (no pun intended :) ), see here for the explanation and link to the changes (you can also seem them on the PR changes). I am not sure if this got into Boost 1.71 or not, but will definitely be in 1.72.
This PR was merged recently so you probably were not aware of this. The test test/unit/math/prim/scal/fun/lgamma_test.cpp fails if there is no patch applied.
We use the updated header but specify to only use API for 1.2 for maximum compatibility. So there is no issue on runtime not supporting anything. The main reasons we want to use the new headers is that for the old headers we had to resort to some ugly template trickery to make the interface look nice. And they now support std::string kernels which is also nice. See points 4 and 5 in this PR, @stevebronder gave a good explanation there.
I would personally like to avoid the support issues similar to what you had to do:
Especially since the cost of is actually including a single file. But realize that would set a precedent so its up to someone more senior with the project to call the shot here.
Stan has pinned dependency versions that it tests against and supports. What’s the reason for wanting to switch to system headers and libraries? I could imagine scenarios where using system headers and libs would make it much harder to test and replicate bugs.
idt I do? Or maybe I’m not following. Again my assumption is that we have very specific pinned versions of our dependencies we use.
The cl.hpp file we have in math is really nothing more than a bunch of helper classes around the OpenCL C API which has 1.2 contformant iterfaces for Nvidia, AMD, Intel, etc. I’d much prefer we use the cl.hpp in the math lib. tbh I think having a pinned version and not whatever version the user has preinstalled would avoid a lot of headaches for all of our dependencies