mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kellen sunderland <kellen.sunderl...@gmail.com>
Subject Re: Switch PR validation to PR-merge
Date Wed, 10 Jan 2018 20:26:51 GMT
+1

On Wed, Jan 10, 2018 at 6:51 PM, Gautam <gautamnitc@gmail.com> wrote:

> +1
>
> On Jan 10, 2018 1:25 AM, "Marco de Abreu" <marco.g.abreu@googlemail.com>
> wrote:
>
> > 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/92ca1942d67a87ee6a2b4d448c621e
> > 433f2f8aca81e4d913d8b2537e@%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