mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From YiZhi Liu <javeli...@gmail.com>
Subject Re: Protected master needs to be turned off
Date Mon, 20 Nov 2017 20:18:25 GMT
+1

2017-11-20 11:47 GMT-08:00 Tianqi Chen <tqchen@cs.washington.edu>:
> +1 until new CI is implemented.
>
> Tianqi
>
> On Mon, Nov 20, 2017 at 11:11 AM, Eric Xie <jxie@apache.org> wrote:
>
>> A lot of people seems to be confused, so let's clarify the separation of
>> roles/responsibilities:
>>
>> 1. The committers that merge code are responsible for code quality and
>> tests passing on master.
>>
>> 2. The CI / infra maintainers are responsible for keeping CI running
>> properly and honestly reporting bugs.
>> If test fails because Jenkins is faulty or slow, you need to fix Jenkins.
>> If test fails because there is a bug in the code, then you did a good job
>> catching bugs. It is not your responsibility to fix the bug. You merely
>> should report it to committers.
>>
>> Thanks,
>> Junyuan Xie
>>
>>
>> On 2017-11-19 16:07, Gautam <gautamnitc@gmail.com> wrote:
>> > -1
>> >
>> > Please see inline.
>> >
>> > On Nov 19, 2017 12:51 PM, "Eric Xie" <jxie@apache.org> wrote:
>> >
>> > Hi all,
>> > I'm starting this thread to vote on turning off protected master. The
>> > reasons are:
>> >
>> > 1. Since we turned on protected master pending PRs has grown from 40 to
>> 80.
>> > It is severely slowing down development.
>> >
>> >
>> >      Turning protection off, will give you ability to merge the code
>> which
>> > has build failure. How and more importantly who will be best judge to
>> > figure out what's is wrong ? If CI is a problem then we have people from
>> > infra team, who are sort of 'on call" day and night trying to fix.
>> > Including me trying to fix the slave at 2 am in night. I have seen
>> > commiters sending PRs and if it fails without even looking at the
>> > reason,which could be just a temporary error, they reach out to infra
>> team
>> > immediately. So I don't agree that we should turn off the protected
>> branch
>> > just for the sake of speed. We should care more on quality than speed.
>> >
>> >
>> > 2. Committers, not CI, are ultimately responsible for the code they
>> merge.
>> > You should only override the CI when you are very confident that CI is
>> the
>> > problem, not your code. If it turns out you are wrong, you should fix it
>> > ASAP. This is the bare minimum requirement for all committers: BE
>> > RESPONSIBLE.
>> >
>> > How do we make sure that this in deed happens?
>> >
>> > I'm aware of the argument for using protected master: It make sure that
>>
>>
>>
>> > master is stable.
>> >
>> > Well, master will be most stable if we stop adding any commits to it. But
>> > that's not what we want is it?
>> >
>> > No we are not saying don't add anything in master, we are just saying
>> > please don't add bad code to the master. And yes I have seen bad code has
>> > been merged to the master when protected branch was not enabled.
>> >
>> > Protected master hardly adds any stability. The faulty tests that breaks
>> > master at random got merged into master because they happened to succeed
>> > once.
>> >
>> > That's not true, it filter out one of the important aspect that at least
>> > when code was merged it completed the whole cycle of build and test. Sure
>> > flaky test we can track down.
>> >
>> > Thanks,
>> > Junyuan Xie
>> >
>>



-- 
Yizhi Liu
DMLC member

Mime
View raw message