mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marco de Abreu <marco.g.ab...@googlemail.com>
Subject Switch PR validation to PR-merge
Date Wed, 10 Jan 2018 09:24:56 GMT
Hello,

TLDR: We wish to change how PRs are validated, turning off PR-head which
tests PRs in their current branch, and turning on PR-merge, which tests PRs
rebased on the current master branch.  We believe this will catch more
potential errors that would otherwise get merged into master, and it should
not cause any extra work for commiters or reviewers.

as announced in
https://lists.apache.org/thread.html/92ca1942d67a87ee6a2b4d448c621e433f2f8aca81e4d913d8b2537e@%3Cdev.mxnet.apache.org%3E
and as probably most have noticed, we have been running an experiment with
the PR-validation-jobs. During the past month, every PR was checked by the
jobs called PR-head and PR-merge. In the past, only PR-head has been
executed and was the required job to pass in order to merge a PR into the
protected master branch. Before I continue any further, I’d like to explain
the detailed meaning of both jobs:

PR-head: The PR and its commit history is taken as-is and tested in exactly
the same state as in your local fork.

PR-merge: The PR and its commit history are rebased on top of latest master
commit and thus tested as if the PR would be merged at this point in time.

I have noticed that many PRs are rarely rebased before a merge. Considering
the fast development of MXNet, this could cause serious issues: Imagine a
PR is based on a 4 weeks old commit and accesses an API which has been
modified in the meantime. PR-head would report this PR as ready to merge as
the changes, based on the 4 weeks old commit. But as soon as a committer
merges this PR into the master branch, the master branch will suddenly
report errors because this PR tries to access an API which does not exist
anymore.

Using PR-merge will reduce the chance of this happening as the PR is always
getting rebased on top of the master branch before it is getting validated.
But there is one pitfall: CI only runs if a new commit is getting pushed.
If a PR stays untouched for a certain amount of time it still could be
possible that it missed a breaking change due to the fact that CI hasn’t
been triggered for a while, but this happens quite rarely. In order to
solve this problem, we could think about introducing a job which validates
PRs that haven’t been run for a week, but that’s a different discussion.
Also, if multiple PRs get merged at the same time, conflicting changes (in
terms of changes in one part which cause another part to fail) could be
introduced – but the committers who merge the PRs usually notice two
conflicting PRs. Additionally, merge conflicts in terms of changing the
same lines of code on the other hand will fail fast and tell the
contributor in the GitHub-webinterface that they will have to resolve the
merge-conflicts before the PR can be validated – it couldn’t be merged with
merge-conflicts anyways.

PR-merge is a safer choice in terms of health for the master-branch. Thus,
I’d like to put it up for discussion to turn off PR-head and switch the
required check to PR-merge.

Does anybody object?

Best regards,
Marco

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message