Repository navigation
Add cmake as the default for building stan math's tests - #3463
Open
SteveBronder wants to merge 17 commits into
Open
SteveBronder wants to merge 17 commits into
SteveBronder wants to merge 17 commits into
Conversation
Import and modernize the CMake setup for current vendored dependencies. Use CMake by default in runTests.py, group directory tests into executables, reuse per-source objects and PCH, and cap build parallelism at six jobs. Migrate Jenkins test stages, document the workflow, and ignore CMake build trees. Fix shared test helper linkage, Eigen fixture initialization, and the Bernoulli GLM invalid-label fixture; suppress GCC column-tracking notes. Validation: 21 runner, selection, and CMake build regression checks passed. Previously validated reverse and primitive function groups, generated probability and expression tests, and object reuse across test selections.
Collaborator
Author
|
Note this is not ready for a review yet. I think once all of the jenkins passes it will be good to look at |
Member
|
Exciting! |
Collaborator
Author
|
Yes I was fingers crossed those would not be necessary, but it is much much less than the last time 😆 . So I'll make a PR for those once all the tests pass here |
Keep grouped thread-pool tests independent of singleton initialization and restore thread-count environments. Match Windows UCRT and ARM definitions across TBB and consumers, disable broken Clang 9/10 PCH use, and validate varmat build setup before classifying signatures. Scope the old GoogleTest policy minimum and allow OpenCL header checks without a loader while retaining executable link requirements. Move CMake runner commands into runTests.py and update Jenkins and documentation.
Remove unused declarations exposed by the Jenkins -Werror build and group conditional callback captures without changing the derivative branches. Forward empty softmax inputs and exercise reverse-mode segment exceptions. Remove discarded reduction copies that consumed temporary matrix inputs. Add regressions covering temporary inputs to all six affected factories.
Collaborator
Author
|
I'll have to make a stacked pr here for all of the math changes. There are a few little places that I missed. |
Remove all 23 tuple captures from the OpenCL and CPU reverse callbacks. Use direct value captures to avoid Clang 10 unused explicit-capture warnings while keeping the original derivative bodies and callback ownership. Verify trivial callback destruction across 23 affected test translation units using validation-only compile-time assertions.
Link ARKODE for the new ODE wrappers and mirror Make's Windows math constant definition. Pair symmetric finite differences before weighting to avoid FMA cancellation noise without relaxing test tolerances. Keep MPI command registration objects in every test executable and use Stan's test flags for its customized GoogleTest main. Validated on the exact CI merge: 11 numerical/ODE executables and five MPI executables pass. The gradient suite also passes with Clang 10 and -Werror; Windows definition propagation and CMake 4 configuration checked.
Remove the unused capture and two profiling-test variables rejected by Clang 10. Pair symmetric fixed-step finite differences before weighting to cancel constant terms, with constant-function and analytic-polynomial regressions. Use absolute bounds for zero-valued matrix sums and solver finite differences, and verify the degenerate solver's analytic gradient independently. Preserve public APIs and thread-pool initialization. Validated all 20 Jenkins threading executables with Clang 10 and -Werror, seven numerical suites with FMA enabled for C and C++, and ten shuffled grouped-core repetitions each with threading on and off.
runPod supplies PARALLEL in the container environment, while the Groovy env lookup fell back to one. Resolve it through sh and reuse that count for CMake builds and CTest. Apply the same lookup to header and doxygen builds, retaining the default of one and explicit serial test overrides. Validated the old failure and repaired commands with a Groovy harness that separates the controller binding from the container shell environment. Checked unset, empty, four, six, and nonpositive values, plus serial test overrides. All eight CMake command-planning tests pass.
Update standalone header checks, test executables, PCH producers, and compile-only PCH producers to use the clearer source filename. Verified before renaming that header targets compile ordinary ELF objects with PCH enabled on Clang 18, without producing PCH artifacts. After renaming, rebuilt three header checks, a test executable, and the unit/compile-only PCH paths. The test and CMake planning checks pass.
Capture the nested arena count explicitly across GCC and Clang. Preserve value categories when returning unit_vector_free results and remove the remaining unused OpenCL and normal-sufficient variables. Match Make's ARM floating-point options for Stan and SUNDIALS, honor OpenCL header overrides, and select existing complete test aggregates so Make can use the requested parallelism. Split Jenkins expression generation into 24 batches to fit Clang 10 compilation within its 64 GiB pod.
Collaborator
|
Would it be worth updating the vendored TBB instead of cmake-ing around it? |
Member
Doing this eventually is one of the main reasons we’d like CMake, but it will be hard to do for the period of time where we’d like to still support the make-only build system. I think it’s better off doing the upgrade separately (and, unfortunately, a good bit later) |
Collaborator
Author
|
@andrjohns yes. To clarify the steps here we will be doing the following
So it is a bit of a long road, but there is a clear path. We can also start the cmake stuff in the other repos now. There is not really a stan math dependency to using cmake in the cmdstan and stan repos. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is my new take on having a cmake scheme. It imports a lot of what I did in #3068. This is the nth time we have attempted to make a cmake scheme for our build setup, but this time, I feel good about it! I've made many smaller PRs to try to get the math library ready for this, so now instead of 100k+ changes we have about 1K+ changes.
Since this is rather new for stan developers the readme.md has a big intro for how to use the new cmake backend. After a few releases I will put that in the normal docs page. But while we are moving to using this new tool I think it is better we are very verbose in showing people how to use it.
Tests
./runTests.pyand jenkins now use the cmake backend.The cmake creates folder level test executables and individual test executables.
The folder level tests are much faster to compile now. For example the
test/unit/math/mix/funtests all compile into one object file which took about 30 minutes on my machine using 24 cores (and at max about 65 gb of memory). On develop these take about an hour and a half on my machine.Side Effects
Yes. I mean this is a pretty big change.
One really nice change is that we have precompiled headers with this. That should speed up test compilation. In another PR I am planning to do some tricks with the distribution tests which should speed compilation for those by a good bit.
Release notes
Add cmake as the build backend
Checklist
Copyright holder: Simons Foundation
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested