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 Re: Switch PR validation to PR-merge
Date Sat, 13 Jan 2018 00:51:49 GMT
Considering we're approaching the weekend, I'll switch PR-head off now.
Also, I will retrigger all PRs in order to have a clean state below all
PRs. If anybody objects afterwards, we can just flip the switch to
re-enable these builds.

-Marco

On Fri, Jan 12, 2018 at 7:57 PM, Marco de Abreu <
marco.g.abreu@googlemail.com> wrote:

> Hello,
>
> the protected master branch has successfully been switched to PR-merge
> https://issues.apache.org/jira/browse/INFRA-15833. In the next step, I'd
> like to remove PR-head from our CI. This means that in future only PR-merge
> will be executed.
>
> Does anybody object?
>
> -Marco
>
> On Wed, Jan 10, 2018 at 9:40 PM, Marco de Abreu <
> marco.g.abreu@googlemail.com> wrote:
>
>> Thanks for your opinions. Could a committer please contact a mentor in
>> order to create an Apache Infra ticket to change the protected master
>> branch from PR-head to PR-merge?
>>
>> -Marco
>>
>> On Wed, Jan 10, 2018 at 9:26 PM, kellen sunderland <
>> kellen.sunderland@gmail.com> wrote:
>>
>>> +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