[gmx-developers] suggestions for coding workflow to lead to efficient review

[Mark Abraham]

Hi,

We've had one or two discussions recently about how to manage our code
development with minimal friction. Often while tackling some new or changed
functionality, it becomes clear that some piece of infrastructure should
change as well. The cleanup/reorganization/extension is often good to
handle separately from the new functionality, so that other code can start
using it immediately, and the surface area of each patch stays as small as
possible. This makes it easier to reason about each change, when writing
and reviewing.

Ideas for how to get this done:

Be mentally prepared to throw work away - your first experiment, your first
simulation and your first draft of a paper are often horrible, and code is
no different. You often don't know what you need to do until you've tried
to do it and/or have the requisite general experience.

Make sure you have a way to run the code easily - at least so you can know
immediately that what you've changed compiles and doesn't segfault.

Edit code with only one, small objective in mind. Commit code frequently -
saying "WIP adding xyz - compiles!" is a great commit message for your
local development branch. "Extended XSDE to support xcf" is also great. If
you do this, you'll end up with a series of commits and each one is
reasonably clear about whether it is cleanup, preparatory infrastructure
changes, or new functionality. git rebase can then let you move your
cleanup and infrastructure patches early in your history. Your tests will
let you re-visit points in your revised history and see that things compile.

When you realize that stuff needs cleaning up before you can do what you
want to do, stop and think about what that means for the larger objective
that you have - maybe you should go do that cleanup and then re-write much
of your code in that new context.

Otherwise, make a work-in-progress git stash/commit/branch (whatever works
for you), check out an early version of this branch of your development
repo and do the cleanup, test it, etc. Then cherry-pick your old
work-in-progress on top of it, resolve any issues.

If one has failed to do any of the above, there is still the option of
squashing work into a single git commit, looking for some logical
sub-pieces of the work, and using
http://stackoverflow.com/questions/1440050/how-to-split-last-commit-into-two-in-git
and
perhaps
http://stackoverflow.com/questions/1085162/commit-only-part-of-a-file-in-git to
separate things.

Example case: GROMACS OpenCL support

StreamComputing forked GROMACS release-5-0 on github (
https://github.com/StreamComputing/gromacs), and several of their
developers worked on adding OpenCL functionality for several months.
GROMACS master moved on, of course. When things were approximately
complete, we decided that much of the OpenCL development history was of no
interest to the GROMACS repo. I merged gromacs/master into their github
branch, and made a number of cleanup patches (e.g.
https://github.com/StreamComputing/gromacs/commit/2884290bada2f1245ea0c05dfd278a1356a14517)
with a view to ending up with the ability to separate two commits. One that
didn't change GROMACS functionality, but prepared us to move away from GPU
being synonymous with CUDA (https://gerrit.gromacs.org/#/c/4306/) so that
other CUDA development work could proceed without much impact on the OpenCL
review process, and then one commit that added the new functionality (
https://gerrit.gromacs.org/#/c/4314/).

In my perfect world, from the latter commit we'd also have separated some
forcerec.cpp-related cleanup, CMake support, fastgen, JIT caching and
twin-range support into separate commits. The usefulness of the latter
three partitions didn't become clear to me until very late in the piece -
when I realized that fastgen+twin-range would remove most of the need for
JIT caching, which was problematic to implement well.

None of this lunch is free, of course. The lesson to do one thing at a time
comes hard to smart people sometimes :-)

Mark
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://maillist.sys.kth.se/pipermail/gromacs.org_gmx-developers/attachments/20150908/88d3733b/attachment.html>