Model base class & compile time speedup PRs ready to review

I’d very much like to get these two pull requests reviewed in time for the v2.20 release:

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.

Any suggestions for reviewers, @seantalts or @syclik?

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.

There are two reviews we need.

stan

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 hacked the cmdstan makefiles - @syclik would be the best person to review this.

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?

Best,
Sebastian

1 Like

agreed - fixing now.

please add this comment to the CmdStan PR https://github.com/stan-dev/cmdstan/pull/713 and I will try to address it.

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:

  1. 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.

  2. 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.