They reduce compile time from 35s24s to 7s for a new model.
There’s a hard deadline for testing to get through Jenkins before some time 18 July (next Thursday, 9 days from now), which means getting all change requests done and into Jenkins at least a day before that.
There’s also the issue of whether to ignore the Mac pro performance regression on arK.stan—I don’t see it with clang++ or g++ 6 on my Macbook. @seantalts told me me he’s seen similar regressions before around pointers versus references and ignored them because they didn’t reproduce on any other hardware.
I think I personally ignored the performance difference on the old mac but I’m not sure what @syclik ended up doing - that was in the Math repo, not the Stan repo. But yeah, I’m okay with doing that here as it’s still a huge net win by my metric even if it did reproduce across hardware. Another option would be to run a full slate of all the models we know how to automatically run - @serban-nicusor recently added a feature to let you do this I believe. It will also happen automatically whenever it gets merged to develop. To do this locally it’s as simple as dropping the directory and specifying --tests-file=shotgun_perf_all.tests
What are you looking for in a reviewer? I am probably not aware of anyone on the project that you aren’t aware of, heh. Most of the recent makefile wizardry I’ve seen going by has been by @syclik or @wds15. CRTP C++ could probably be reviewed by anyone who self-nominates, haha.
Side note - I am traveling for the next week with sporadic availability until I arrive in Porto on the 16th.
Mainly someone with the time to do it before the next release.
And the authority to make the decision on the performance regression, which I think is still you since I think you’re still in charge of the language and head of the TWG.
Ready to review, does not break backward compatibility; the PR consists of a change to code generation (trivial) and addition of nested model base classes using OO, templating, and the CRTP.
cmdstan
Has to wait on stan to get merged before tests will pass. Still work being done on makefile. The speedups should all be in place, but even that I’m not sure of. @mitzimorris will know more.
I can help with this. It probably makes most sense if one person looks into both.
From glancing at things a few comments/questions already:
The makefile variable …_O (forgot the name) should be derived from the variable defining the cpp file with a substitution command
What is new with this PR (to me) is that now CmdStan picks up stuff which are defined in the Stan makefiles - at least this is how we should handle it. Right now the additional make targets are defined in both repos - which is not good, I think. The additional targets needed to build models should be defined in Stan and then those definitions get picked up by CmdStan by including it (and not by defining it again).
Are your tests demonstrating the claimed backward compatibility? I mean, can I generate from the current develop a model OR from the new PR and both sources will work with the system proposed in the new PR?
yes, this is backwards compatibile - all the old tests are there, and new tests added to check both existing and new versions - see tests in src/test/unit/model/model_base_test.cpp
Yes, it’s backward compatible in that anything you used to be able to do with a model, you can still do with it.
Concretely, the existing model code only changes in two places:
in the constructor, a model is delcared to extend model_crtp rather than prob_grad; the superclass calls in the constructor are changed to match.
A new, global, generic constructor function is appended to the end of the file.
No, it’s not forward compatible. A model generated in 2.19 won’t work in CmdStan in 2.20 because it won’t implement the appropriate base class or provide an appropriate global construction function.
For the services as defined in stan-dev/stan 2.20, we need the class generated by 2.20. It extends the proper CRTP class and provides the constructor function.
But what do I need to change / add / restore for the opposite thing: Compiling C++ generated by stanc in versions 2.19, 2.18, etc. against Stan 2.20 using headers only?
We should really think about how we make this easier in the future. Maybe versioned model base classes or versioned namespaces… or some pimpl stuff to hide implementation details… whatever, but this is a real misery for RStan which we should try to address by design.
There is no way to run a model class generated by 2.19 in Stan 2.20. Is there some obstacle to just regenerating C++ code?
The model class generated by Stan 2.20 is different in several ways from that generated in 2.19:
rather than defining a class foo that extends prob_grad, it now is declared to extend model_base_crtp<foo>, which in turns extends model_base. The model_base_crtp class adapts the templated methods generated by the transpiler to implement the virtual methods defined in model_base.
the initializers in the constructors change to match.
each generated model file includes this code in addition to the class that’s generated
typedef foo_model_namespace::foo_model stan_model;
stan::model::model_base& new_model(
stan::io::var_context& data_context,
unsigned int seed,
std::ostream* msg_stream) {
stan_model* m = new stan_model(data_context, seed, msg_stream);
return *m;
}
The typedef existed before, but the new_model function is used in command.hpp to create a new model generically without having to know the name of its class. Before, we’d just instante stan_model through a template in command.hpp.
There is also a change in compilation in that command.hpp no longer has template parameters—it just hard codes base_model as the template parameter for the services.
We need to talk to @bgoodri on this one. The problem is CRAN which forces him to make things binary compatible with older releases as I understand. Just regenerating the C++ file from the Stan model doesn’t work for CRAN.
The bad thing for users is that they have a long wait until they get the next RStan because @bgoodri has to find out a way to munge together the last few releases to “make it work”. So users in R have to wait and they also don’t quite get a vanilla stan release in the sense of it being really the same as what is in CmdStan (it’s close, but not the same).
So this should be discussed if we want to resolve the situation or if we want to ignore this yet another CRAN restriction…
This isn’t as much of a problem as I had feared. The RStan 2.19.x on CRAN now has a compiled code generator based on the pre-2.20 behavior (with prob_grad, templated model, etc.). So, you can (with one change to RStan to #include our Eigen plugin thing at the top of the C++ file) compile a previously existing C++ Stan program with Stan Math 2.20 via StanHeaders 2.20. And you can generate a compile fresh Stan programs this way, albeit with the pre-2.20 patterns.
So, the question becomes how can I keep that functionality for packages that come with C++ that was generated pre-2.20 while also putting the post 2.20 functionality in so that it can be used by subsequent Stan programs and also on RStudio Cloud.
In CmdStan 2.20, there is a main.cpp file that just looks like
that gets compiled once and then subsequent Stan programs link to that. There is also a forward declaration in command.hpp
I think I can compile whatever I need to into the StanHeaders shared object even if it is not always being used. Here is the Makefile that StanHeaders uses currently (on !Windows) to compile the C files in SUNDIALS
I guess my question is: What is the equivalent of CmdStan’s forward declaration and a main program in a shared object? Can I just tell it to compile model_header.hpp?
Also, what is the connection with the pre-compiled headers part of the CmdStan makefiles? Can the rest of this be done without pre-compiled headers because I’m not sure CRAN will let me generate objects that are hundreds of MB by default?
You need to compile what corresponds to main.cpp. The model header.hpp contains templates which won’t compile. The pre compiled headers are not required…but CRAN won’t let you create hundreds of mb. That would need to be an optional thing I am afraid…
The precompiled headers cut down on model compile time. Without the precompiled headers, it’s about 12s and with precompiled headers about 7s on my Macbook. The precompiled headers can be generated when the first model’s compiled if that’s easier than distributing it with the package. I don’t know if the generated headers are compiler-specific or not, but if they are, that’d be a strong reason not to include them in binary form.
I am still working on getting the 2.20 stuff to work going forward while still working with pre-2.20 generated C++ code. At the moment, the 2.20 stuff is not working for rstanarm and any packages like it that come with more than 1 Stan program. The compilation via headers-only actually works, but the linking fails due to multiple instances of the new new_model function across C++ files:
g++ -std=gnu++14 -shared -L/usr/local/lib -o rstanarm.so stan_files/polr.o stan_files/lm.o stan_files/binomial.o stan_files/count.o stan_files/jm.o stan_files/bernoulli.o stan_files/mvmer.o stan_files/continuous.o init.o
stan_files/lm.o: In function `new_model(stan::io::var_context&, unsigned int, std::ostream*)':
/tmp/Rtmp7H61lk/Rbuild72577914b691/rstanarm/src/stan_files/lm.hpp:1230: multiple definition of `new_model(stan::io::var_context&, unsigned int, std::ostream*)'
stan_files/polr.o:/tmp/Rtmp7H61lk/Rbuild72577914b691/rstanarm/src/stan_files/polr.hpp:1838: first defined here
stan_files/binomial.o: In function `new_model(stan::io::var_context&, unsigned int, std::ostream*)':
/tmp/Rtmp7H61lk/Rbuild72577914b691/rstanarm/src/stan_files/binomial.hpp:4071: multiple definition of `new_model(stan::io::var_context&, unsigned int, std::ostream*)'
stan_files/polr.o:/tmp/Rtmp7H61lk/Rbuild72577914b691/rstanarm/src/stan_files/polr.hpp:1838: first defined here
stan_files/count.o: In function `new_model(stan::io::var_context&, unsigned int, std::ostream*)':
/tmp/Rtmp7H61lk/Rbuild72577914b691/rstanarm/src/stan_files/count.hpp:4358: multiple definition of `new_model(stan::io::var_context&, unsigned int, std::ostream*)'
stan_files/polr.o:/tmp/Rtmp7H61lk/Rbuild72577914b691/rstanarm/src/stan_files/polr.hpp:1838: first defined here
stan_files/jm.o: In function `new_model(stan::io::var_context&, unsigned int, std::ostream*)':
/tmp/Rtmp7H61lk/Rbuild72577914b691/rstanarm/src/stan_files/jm.hpp:13249: multiple definition of `new_model(stan::io::var_context&, unsigned int, std::ostream*)'
stan_files/polr.o:/tmp/Rtmp7H61lk/Rbuild72577914b691/rstanarm/src/stan_files/polr.hpp:1838: first defined here
stan_files/bernoulli.o: In function `new_model(stan::io::var_context&, unsigned int, std::ostream*)':
/tmp/Rtmp7H61lk/Rbuild72577914b691/rstanarm/src/stan_files/bernoulli.hpp:4625: multiple definition of `new_model(stan::io::var_context&, unsigned int, std::ostream*)'
stan_files/polr.o:/tmp/Rtmp7H61lk/Rbuild72577914b691/rstanarm/src/stan_files/polr.hpp:1838: first defined here
stan_files/mvmer.o: In function `new_model(stan::io::var_context&, unsigned int, std::ostream*)':
/tmp/Rtmp7H61lk/Rbuild72577914b691/rstanarm/src/stan_files/mvmer.hpp:8722: multiple definition of `new_model(stan::io::var_context&, unsigned int, std::ostream*)'
stan_files/polr.o:/tmp/Rtmp7H61lk/Rbuild72577914b691/rstanarm/src/stan_files/polr.hpp:1838: first defined here
stan_files/continuous.o: In function `new_model(stan::io::var_context&, unsigned int, std::ostream*)':
/tmp/Rtmp7H61lk/Rbuild72577914b691/rstanarm/src/stan_files/continuous.hpp:7120: multiple definition of `new_model(stan::io::var_context&, unsigned int, std::ostream*)'
stan_files/polr.o:/tmp/Rtmp7H61lk/Rbuild72577914b691/rstanarm/src/stan_files/polr.hpp:1838: first defined here
collect2: error: ld returned 1 exit status
/home/ben/r-devel/share/make/shlib.mk:6: recipe for target 'rstanarm.so' failed
make: *** [rstanarm.so] Error 1