As I’m thinking about the standards for pull requests, I’ve had a chance to reflect on what we’re trying to accomplish. First, the Math library needs to be stable. Alone, it’s not much, but the rest of the Stan ecosystem depends on it working: correct and buildable.
Beyond the code, we want the community of developers that support the Math library to treat the project as an open-source project. Individuals contribute to the Math library so it gets better. There isn’t a sense of code ownership – once code makes it into the repository, we can all modify it and are a bit responsible for it. We want discussions to be in the open and in a place where others can follow along or comment. We’ve already had a lot of wonderful contributions from having open dialogs.
Prototypes are great! The default place for discussions on prototypes should be here on discourse. Sometimes there might be an issue already open and discussion can go there. The goal of a prototype is to show a proof of concept. It’s to show that something could possibly work.
Most prototypes aren’t pull request worthy; and that’s ok! There’s a bit more to getting something ready for inclusion in Stan.
New features, bug fixes, refactors, and essentially any code change comes in the form of a pull request. We would really like it to follow this wiki: Developer process overview (we may adjust this for Math):
- Create an issue on GitHub
- Create a branch for the issue
- Fix the issue on the branch
- Create a pull request
- Code review
- Address code review (if necessary)
- Merge into develop
All development is done by individuals that may have different reasons for creating a pull request, but when a pull request is submitted, it’s really about the contribution to the Math library. Once the pull request is in the Math library, the responsibility lies on all the developers (present and future!).
This is why we want:
- code to be styled consistently (it’s not about your preference, but about reducing the time it takes others to read your code)
- contributions to be idiomatic to the language – that’s either C++, doxygen doc, make, C preprocessor macros, etc.
- readability; the code is written once, but will be read by a reviewer and then again some time in the future. It’s not about how it’s written, but how it’s read.
- documentation to accompany the pull request
- tests; we don’t want to add technical debt to the Math library with every pull request
- design to be reasonable. we want someone else in the future to look at the code and understand the design
- it to build. This is tough. Right now, we support Mac, Windows, and Linux operating systems and g++ (4.6+) and clang++. We actually have users in Mac that use g++ 4.6, 4.9, clang++ 3.0 / 9.0, and some older version of clang++, Windows users that use g++ 4.6, 4.9, and Linux users that use g++ and clang++. As we move into GPU and MPI territory, we have to keep the math library building for all our existing users.
I know the test burden is high for the math library. I’ll try to give some motivation why:
- We instantiate the code in all the ways that the Stan library could instantiate the Math library. We do this because we’ve encountered many compiler bugs where things that should work don’t. And this might only show up with a particular version or even on a particular OS. I don’t know of any other way to test it other than to instantiate it.
- To be responsible to other developers and users. We really shouldn’t push code that’s never been exercised in our tests, at least once. You might be good enough to write code that always works, but I’m not. And if it needs to be changed, tests should be there.
- For maintenance. Whenever we encounter a bug, we fix it by first recreating it. It’s a lot easier adding a test case when a function has been instantiated with the right types than trying to figure out how to instantiate it in the first place. It’s the difference between setting up something in a couple minutes vs taking a couple hours in the future.
- For stability. We write a lot of unit tests, but we still don’t have much test coverage. More tests allow us to update the codebase quicker while trusting the results.
- We also check output messages. These actually make it out to the user, so it helps to be consistent and make sure these are ok.
In short, we’re trying to contribute something to the Math library that’s good and will not incur much technical debt. It also helps if the contributor takes time to guide the reviewer by highlighting the design or the major changes.
Code reviews should be taken seriously and is carried out by developers of the math library. One or more developers have to review pull requests; most often, they might review different parts of the pull request. For example, I might look at the changed build instructions while someone else looks at the template metaprogramming.
When evaluating the code, we should be evaluating it from the point of view of the Math library and defending it. I always assume the contributor has a good reason for putting effort into a pull request – the content of the pull request does something good for the project. The review is a chance for us to evaluate whether that’s true and whether it’s going to be maintainable.
We should be thinking about how easy it is to read the code. Once it’s in, the collective developers have the responsibility of maintaining it. We should think about how hard it would be to replace the code later on. We should be asking if there are easier ways to do things so that it’s less complex. We need to be looking at design. We should also ask if there are enough tests – at least to convince yourself that it’s right (enough) for users to push on all the corners of that code. The litmus test is whether you’d be comfortable hunting down a bug in that code if it were to break.
Code breaks. We’ll deal with it. It’s a matter of how easy or hard it is to deal with later on. I’m never a fan of “we’ll document it later” or “we’ll test it in a future pull request” – that technical debt almost never gets paid off. Even with the best intentions, the individual that contributes code isn’t always the one that fixes it. That’s part of the challenge of open-source software.