Simple Commit & PR rules

[yuanming-hu]

As the Taichi developers community is growing, I suggest we make some simple rules just to make our life easier. The very high-level guidelines are

• All rules have their own overhead, and should be designed to make us work more effectively. Avoid complex rules that are hard to follow.
• Unit/integration tests are our friends.
• Changesets should be small so that sources of bugs can be easily pinpointed.
• No overkills: always use the easiest solutions to solve easy problems so that you have time and energy for real hard problems.
• ...

Commits:

• No commits with local (i.e. the contributor's local environment) compilation errors should be made.
• Commit messages should be concise and meaningful.
• The master branch is required to have a linear history.
• ...

PRs:

• PRs with small changesets are preferred. A PR should ideally address only one issue, except for trivial fixes like typo/doc/README.
• If you are making multiple PRs
  • Independent PRs should be rebased on different branches forking from master.
  • PRs with dependencies should be raised only after all prerequisites are merged.
• If possible, cover your contribution with tests
• All PRs should always be rebased onto the HEAD of master before merging (as suggested by k-ye)
• All PRs should pass continuous integration tests (build + testing for Mac/Windows) before they get merged
• Merging PR:
  • Error-prone commits such as IR passes & codegen will be rebased on master (without squashing) once approved
    • Pro: Doing so makes it possible to trace back, when we found bugs later at commit granularity instead of PR granularity
    • Con: master gets a bit more commits and might be a little messy
    • Decision: I personally tend to not squash them to simplify debugging, but inputs are welcome
  • Other commits with more trivial correctness (e.g. Taichi examples, GUI, benchmark cases) will first get squashed into a single commit and then rebased on master, for a cleaner master commit log
  • PR authors should not squash commits. Whether squashing all commits in a PR or not, will be decided by the reviewer who merges the PR into the master branch, depending on how trivially correct the commits are. (as suggested by k-ye)
• ...

Please feel free to add your inputs! Thanks! :-) @k-ye @KLozes @archibate @Psycho7 @masahi

After we reach a consensus, the rules above will be put into the Contributor Guideline.

[masahi]

I think CI should also test on cuda backend, if it hasn't been set up that way.

[yuanming-hu]

It's sad that online CI servies (Travis/AppVeyor) does not actually provide CUDA. The Jenkins buildbot (http://f11.csail.mit.edu:8080/job/taichi/) is borrowed from the other lab so I don't dare to abuse it for too many builds.

However, I recently got an NVIDIA Jetson TX2 which I have exclusive access, so we can at least set that up for testing ARM64 + CUDA.

[k-ye]

Looks good!

I think CI should also test on cuda backend.

More thoughts on CI, I noticed that AppVeyor is very slow, and sometimes the tests can stay in the queue for hours. I guess all these CI issues can be resolved just by spending more money... 😂

Merging PR

+1. I think PRs should always be rebased onto the HEAD of master before merging. It's probably also worth mentioning that the author should not squash the commits (or at least not squash beyond the commit of the last review) during the process of a review, so that reviewers can easily track the diffs. For trivial PRs, squash the commits only after the PR is approved?

[yuanming-hu]

I guess all these CI issues can be resolved just by spending more money...

Yeah, money talks :-) (although it takes $745/year to get three parallel jobs on AppVeyor, with which we can reduce the build time from 40min to 13min). On the other hand, I don't know any CI services that support CUDA.

PRs should always be rebased onto the HEAD of master before merging

Good point. Added to the list.

It's probably also worth mentioning that the author should not squash the commits (or at least not squash beyond the commit of the last review) during the process of a review, so that reviewers can easily track the diffs. For trivial PRs, squash the commits only after the PR is approved?

Yeah, let's just say PR authors should not squash commits. Whether squashing all commits in a PR or not, will be decided by the reviewer who merges the PR into the master branch, depending on how trivially correct the commits are.

[archibate]

More thoughts on CI, I noticed that AppVeyor is very slow, and sometimes the tests can stay in the queue for hours. I guess all these CI issues can be resolved just by spending more money... 😂

Yes, and sometimes Travis is far more fast than AppVeyor. I clicked into Details and see, jobs are queueing.

Possible solution to make it faster:

When we push commits frequent, CI build restarts frequently too. This may cause the CI scheduler (I guess) postpone our follow-up build requests. But some commits are really not necessary for a CI test. For example, a contributor make a little change in a unsiginificant file and push that commit, CI restarts. That's a waste of computation resources.
So, I suggest contributors not to push commit(s) until he think it's time for a CI test or review.

Also, there won't be this problem if we can disable restarting CI for each commit: we could run CI when we want to. For example, PRs with trivial correctness (eg. GUI, examples, docs) can do CI test only after merge is approved.
May save a lot of time and money after our PRs & commits gets very frequent due to the rapid grow of developers community.

[archibate]

The Contributor Guideline should be placed in a eye-catching position so that every newcomers who are willing to contribute can see, for fear of disorder.
Maybe we want to add

<!-- Thank for your issue/PR! Make sure you have viewed the Contributor Guideline (https://taichi.readthedocs.io/en/latest/contributor_guide.html), that helps us a lot! -->

to issue/PR template?

[k-ye]

Also, there won't be this problem if we can disable restarting CI for each commit: we could run CI when we want to. For example, PRs with trivial correctness (eg. GUI, examples, docs) can do CI test only after merge is approved.

+1, I think this is the most efficient way to utilize CI resources. Maybe trigger the CI automatically only right before the merge? Devs should be allowed to run CI if they need to verify the correctness, but they should also be mindful...

[archibate]

Possible dirty hack to control CI:

before_install:
- test -f $TRAVIS_BUILD_DIR/.enable_ci || exit

When creating a new fork, run rm .enable_ci.
When we want to run CI test, just touch .enable_ci.

Or we can make this hack more advanced - detect base commit id:

before_install:
- [ `cat $TRAVIS_BUILD_DIR/.ci_base_commit` == $TRAVIS_THIS_COMMIT_ID ] || exit

# enable_ci.sh
`git get-this-commit-id` > .ci_base_commit

... where $TRAVIS_THIS_COMMIT_ID and get-this-commit-id should be replaced by proper code.

If we want to test the current commit, run ./enable_ci.sh. The next commit will not be enabled with CI. People who make PRs will not be enabled with CI by default.
Also, we can add some code in .git/hooks/pre-commit, to detect if this commit need CI test or not.

[yuanming-hu]

Thanks for all the valuable inputs! I'm summarizing them up.

Yes, and sometimes Travis is far more fast than AppVeyor. I clicked into Details and see, jobs are queueing.

Yeah, Travis currently only build for OS X with Python 3.7, while AppVeyor builds for all 3.6/7/8.

One more property we can possibly make use of to accelerate AppVeyor: currently AppVeyor builds Taichi with Python 3.6/3.7/3.8 in serial, each taking 13 min, and 40 min in total. For most commits, we only need to build with Python 3.6. For releasing commits (e.g. those with message [release] v0.5.1), we build with all three versions of Python and upload the artifacts to PyPI.

[Psycho7]

They all look good to me. 👍

One more thing I could think of is code style. Do we want to adopt some linter?

[masahi]

One more thing I could think of is code style. Do we want to adopt some linter?

I also want to ask something about code style... Is there any reason to use 2 space indenting for python code? I think 4 space is the standard, and I get many warning from my editor when I open python files.

If we adopt any python linter, I expect we get indentation warning everywhere.

[Psycho7]

Is there any reason to use 2 space indenting for python code? I think 4 space is the standard, and I get many warning from my editor when I open python files.

I would assume 2-space is just personal preference. Modify the your editor setsing (probably PEP-8 related) could make it clean.

If we adopt any python linter, I expect we get indentation warning everywhere.

We could modify the linter's setting, or just simply adopting the 4-space common practice.

[k-ye]

We also need a coding style for CPP. Personally I've been following the Google coding style, except for function naming for code-base consistency.

If we adopt any python linter, I expect we get indentation warning everywhere.

We may want to do a few large-scale changes to fix all the styles issue. We can also install some github hooks to check the styles before submitting?

[yuanming-hu]

Currently we heavily rely on clang-format (https://github.com/taichi-dev/taichi/blob/master/.clang-format) and yapf (https://github.com/taichi-dev/taichi/blob/master/misc/.style.yapf).

We may want to do a few large-scale changes to fix all the styles issue. We can also install some github hooks to check the styles before submitting?

Exactly. It would be good to have a git hook that automatically formats all the modified code to enforce the styles.

[Psycho7]

We also need a coding style for CPP. Personally I've been following the Google coding style, except for function naming for code-base consistency.

TBH Google's cpp style is not up to my aesthetic taste. LOL
Joking aside, I would suggest we create linter rules that fit our current codebase, both Python and C++, only for less modification.
Anyway, the rule of thumb about coding style is just keeping consistency.

[yuanming-hu]

I used to have a git hook: https://github.com/taichi-dev/taichi/blob/legacy/githooks/pre-commit.py but for some reason, I abandoned it...

[k-ye]

TBH Google's cpp style is not up to my aesthetic taste. LOL

Well, i don't have much of a choice... ¯_(ツ)_/¯ (But i think it works OK for me) Kind of curious what's your favorite style..

[Psycho7]

Well, i don't have much of a choice... ¯_(ツ)_/¯ (But i think it works OK for me) Kind of curious what's your favorite style..

One thing I really don't like about Google's style is that they put an extra space before access modifiers... That odd number makes me feel bad somehow...
But actually I'm pretty flexible about the coding style as long as they work for my colleagues. XD
Let's end it here and focus back to the main topic :)

[yuanming-hu]

A big thank you to everybody for the valuable inputs! The discussions above on commits and PRs have been compiled into the Contributor Guideline in the documentation. Let's continue the code format discussions in Issue #559 :-)

[k-ye]

@yuanming-hu

I noticed that our commit logs can get pretty long, because Github by default record all the intermediate commits in the commit message. E.g.

Author: Ye Kuang <[email protected]>
Date:   Wed Apr 8 12:28:45 2020 +0900

    [opengl] [refactor] Remove the global `no_gc` in `opengl_codegen.cpp` (#723)
    
    * [opengl] Refactor to remove the global `no_gc`
    
    * [skip ci] enforce code format
    
    Co-authored-by: Taichi Gardener <[email protected]>

commit d28533c503a1e4fd1101b549d63c6fb540618be2
Author: Ye Kuang <[email protected]>
Date:   Tue Apr 7 19:12:48 2020 +0900

    [Lang] Support `continue` on all backends (#716)
    
    * [Lang] Support `continue` on all backends
    
    * [skip ci] enforce code format
    
    * move
    
    * [skip ci] assert != nullptr
    
    * [skip ci] print scope
    
    * [skip ci] more asserts
    
    * comments
    
    Co-authored-by: Taichi Gardener <[email protected]>

commit 213533d5af01f057d1d65ae41cbdc14dc0c1f291
Author: xumingkuan <[email protected]>
Date:   Tue Apr 7 00:39:05 2020 -0400

    [ir] Add assertions of `Alloca`s in the constructors of LocalAddress and LocalStoreStmt

commit 0c1bb390ef79008f6c7c44a039665622ef546bba
Author: Yuanming Hu <[email protected]>
Date:   Mon Apr 6 21:08:30 2020 -0400

    [CUDA] Improve CUDA build portability with run-time driver loading (#714)
    
    * partially replace cuda runtime API
    
    * more CUDA driver APIs
    
    * more CUDA driver API
    
    * more CUDA driver API
    
    * more CUDA driver API
    
    * more CUDA driver API
    
    * clean up
    
    * refactor dynamic loader
    
    * CUDADriver class
    
    * CUDADriverFunction
    
    * create cuda context for each thread
    
...

Should we just remove these intermediate commits before merging?

[yuanming-hu]

Good point. I usually use

[alias]
    lg = log --graph --pretty=mt:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr) %C(bold blue)<%an>%Creset' --abbrev-commit --

in my .gitconfig no I don't personally have this problem (due to --abbrev-commit). But you are right, these lines can be found in the GitHub PR pages (e.g. #716). If we trust github, then we can remove the intermediate commit logs.