You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mxnet.apache.org by Qing Lan <la...@live.com> on 2018/12/12 00:18:29 UTC
[DISCUSS] About the PR merging policy
Hi all,
Recently I self-merged my PR without getting approvals from other committers https://github.com/apache/incubator-mxnet/pull/13617 and only contributors approval. I apologize to the community and thank Marco for pointing out the problem. I took a lesson that we should at least have one committer’s approval to merge the code. However, I just found this section is missing in the CWiki https://cwiki.apache.org/confluence/display/MXNET/Become+an+Apache+MXNet+%28incubating%29+Committer+and+PPMC+Member. So I would like to discuss in here:
How to conduct the PR reviewing/merging. How many approvals (Committers and Contributors) we should get in order to merge?
How to deal with disagreement in the discussion (e.g a contributor/committer request a change)?
Please don’t hesitate to share your thoughts!
Thanks,
Qing
Re: [DISCUSS] About the PR merging policy
Posted by Anton Chernov <me...@gmail.com>.
There was no policy setup before for this as far as I know.
Personally, I don't see any value in introducing another blocker for enough
slow process of PR merges. The best value / burden ratio one gets from
lightweight 'another pair of eyes' approach.
Anton
ср, 12 дек. 2018 г. в 01:24, Tianqi Chen <tq...@cs.washington.edu>:
> I think it is fine as long as we act on good faith. I will normally respect
> code review comments from anyone who might be able to give reasonable
> comments, and beg to differ with good technical reasoning. Normally
> contributions happen in a way that things won't get blocked in small
> features.
>
> For major changes, RFC discussion would be helpful to resolve the case
>
> Tianqi
>
> On Tue, Dec 11, 2018 at 4:18 PM Qing Lan <la...@live.com> wrote:
>
> > Hi all,
> >
> > Recently I self-merged my PR without getting approvals from other
> > committers https://github.com/apache/incubator-mxnet/pull/13617 and only
> > contributors approval. I apologize to the community and thank Marco for
> > pointing out the problem. I took a lesson that we should at least have
> one
> > committer’s approval to merge the code. However, I just found this
> section
> > is missing in the CWiki
> >
> https://cwiki.apache.org/confluence/display/MXNET/Become+an+Apache+MXNet+%28incubating%29+Committer+and+PPMC+Member
> .
> > So I would like to discuss in here:
> >
> > How to conduct the PR reviewing/merging. How many approvals (Committers
> > and Contributors) we should get in order to merge?
> >
> > How to deal with disagreement in the discussion (e.g a
> > contributor/committer request a change)?
> >
> > Please don’t hesitate to share your thoughts!
> >
> > Thanks,
> > Qing
> >
>
Re: [DISCUSS] About the PR merging policy
Posted by Tianqi Chen <tq...@cs.washington.edu>.
I think it is fine as long as we act on good faith. I will normally respect
code review comments from anyone who might be able to give reasonable
comments, and beg to differ with good technical reasoning. Normally
contributions happen in a way that things won't get blocked in small
features.
For major changes, RFC discussion would be helpful to resolve the case
Tianqi
On Tue, Dec 11, 2018 at 4:18 PM Qing Lan <la...@live.com> wrote:
> Hi all,
>
> Recently I self-merged my PR without getting approvals from other
> committers https://github.com/apache/incubator-mxnet/pull/13617 and only
> contributors approval. I apologize to the community and thank Marco for
> pointing out the problem. I took a lesson that we should at least have one
> committer’s approval to merge the code. However, I just found this section
> is missing in the CWiki
> https://cwiki.apache.org/confluence/display/MXNET/Become+an+Apache+MXNet+%28incubating%29+Committer+and+PPMC+Member.
> So I would like to discuss in here:
>
> How to conduct the PR reviewing/merging. How many approvals (Committers
> and Contributors) we should get in order to merge?
>
> How to deal with disagreement in the discussion (e.g a
> contributor/committer request a change)?
>
> Please don’t hesitate to share your thoughts!
>
> Thanks,
> Qing
>
Re: [DISCUSS] About the PR merging policy
Posted by Anirudh Acharya <an...@gmail.com>.
Thanks Qing for bringing this up. I think the cwiki can contain pointers to
apache guidelines -
- https://www.apache.org/dev/committers.html
- https://www.apache.org/dev/new-committers-guide.html
- And a few thumb rules( not hard and fast rules, we should trust the
committers to act in good faith in most cases) on how many approvals to
wait for before merging would be good.
And having these in one place in the cwiki would be convenient.
Thanks
Anirudh
On Fri, Dec 14, 2018 at 11:59 AM Carin Meier <ca...@gmail.com> wrote:
> Thanks Steffen,
>
> I had remembered reading that but couldn't find it again :)
>
> So yes - maybe we can duplicate that section and/or provide a link to a new
> committers guide.
>
> I'm thinking it should go on the community page here
> https://cwiki.apache.org/confluence/display/MXNET/Community
>
> Eventually, some of information collected there could migrate out the
> webpage as well.
>
> - Carin
>
> On Thu, Dec 13, 2018 at 7:30 AM Steffen Rochel <st...@gmail.com>
> wrote:
>
> > We do have already a guide which covers the issue:
> >
> >
> https://cwiki.apache.org/confluence/display/MXNET/Development+Process#DevelopmentProcess-GuidelinesforReviewers/Committers
> > <
> >
> https://cwiki.apache.org/confluence/display/MXNET/Development+Process#DevelopmentProcess-GuidelinesforReviewers/Committers
> > >,
> > but it probably needs to become more prominent. Any suggestion for a good
> > place?
> > Steffen
> >
> > On Wed, Dec 12, 2018 at 5:23 PM Carin Meier <ca...@gmail.com>
> wrote:
> >
> > > Qing - thanks for bringing this up.
> > >
> > > I think it would be a good thing to have a document on the wiki to help
> > > with these sorts of questions.
> > >
> > > In fact, since the project is growing with more new committers, maybe
> we
> > > could use a "New Committer Guide" with the process of how to get going
> > and
> > > any FAQ like this one ...
> > >
> > > Would you be interested in getting a rough draft going of your recent
> > > experience? Then others can help collaborate on it.
> > >
> > > It would be nice to make the path smoother for other new committers to
> > the
> > > project.
> > >
> > > Best,
> > > Carin
> > >
> > > On Tue, Dec 11, 2018 at 7:18 PM Qing Lan <la...@live.com> wrote:
> > >
> > > > Hi all,
> > > >
> > > > Recently I self-merged my PR without getting approvals from other
> > > > committers https://github.com/apache/incubator-mxnet/pull/13617 and
> > only
> > > > contributors approval. I apologize to the community and thank Marco
> for
> > > > pointing out the problem. I took a lesson that we should at least
> have
> > > one
> > > > committer’s approval to merge the code. However, I just found this
> > > section
> > > > is missing in the CWiki
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/MXNET/Become+an+Apache+MXNet+%28incubating%29+Committer+and+PPMC+Member
> > > .
> > > > So I would like to discuss in here:
> > > >
> > > > How to conduct the PR reviewing/merging. How many approvals
> (Committers
> > > > and Contributors) we should get in order to merge?
> > > >
> > > > How to deal with disagreement in the discussion (e.g a
> > > > contributor/committer request a change)?
> > > >
> > > > Please don’t hesitate to share your thoughts!
> > > >
> > > > Thanks,
> > > > Qing
> > > >
> > >
> >
>
Re: [DISCUSS] About the PR merging policy
Posted by Carin Meier <ca...@gmail.com>.
Thanks Steffen,
I had remembered reading that but couldn't find it again :)
So yes - maybe we can duplicate that section and/or provide a link to a new
committers guide.
I'm thinking it should go on the community page here
https://cwiki.apache.org/confluence/display/MXNET/Community
Eventually, some of information collected there could migrate out the
webpage as well.
- Carin
On Thu, Dec 13, 2018 at 7:30 AM Steffen Rochel <st...@gmail.com>
wrote:
> We do have already a guide which covers the issue:
>
> https://cwiki.apache.org/confluence/display/MXNET/Development+Process#DevelopmentProcess-GuidelinesforReviewers/Committers
> <
> https://cwiki.apache.org/confluence/display/MXNET/Development+Process#DevelopmentProcess-GuidelinesforReviewers/Committers
> >,
> but it probably needs to become more prominent. Any suggestion for a good
> place?
> Steffen
>
> On Wed, Dec 12, 2018 at 5:23 PM Carin Meier <ca...@gmail.com> wrote:
>
> > Qing - thanks for bringing this up.
> >
> > I think it would be a good thing to have a document on the wiki to help
> > with these sorts of questions.
> >
> > In fact, since the project is growing with more new committers, maybe we
> > could use a "New Committer Guide" with the process of how to get going
> and
> > any FAQ like this one ...
> >
> > Would you be interested in getting a rough draft going of your recent
> > experience? Then others can help collaborate on it.
> >
> > It would be nice to make the path smoother for other new committers to
> the
> > project.
> >
> > Best,
> > Carin
> >
> > On Tue, Dec 11, 2018 at 7:18 PM Qing Lan <la...@live.com> wrote:
> >
> > > Hi all,
> > >
> > > Recently I self-merged my PR without getting approvals from other
> > > committers https://github.com/apache/incubator-mxnet/pull/13617 and
> only
> > > contributors approval. I apologize to the community and thank Marco for
> > > pointing out the problem. I took a lesson that we should at least have
> > one
> > > committer’s approval to merge the code. However, I just found this
> > section
> > > is missing in the CWiki
> > >
> >
> https://cwiki.apache.org/confluence/display/MXNET/Become+an+Apache+MXNet+%28incubating%29+Committer+and+PPMC+Member
> > .
> > > So I would like to discuss in here:
> > >
> > > How to conduct the PR reviewing/merging. How many approvals (Committers
> > > and Contributors) we should get in order to merge?
> > >
> > > How to deal with disagreement in the discussion (e.g a
> > > contributor/committer request a change)?
> > >
> > > Please don’t hesitate to share your thoughts!
> > >
> > > Thanks,
> > > Qing
> > >
> >
>
Re: [DISCUSS] About the PR merging policy
Posted by Steffen Rochel <st...@gmail.com>.
We do have already a guide which covers the issue:
https://cwiki.apache.org/confluence/display/MXNET/Development+Process#DevelopmentProcess-GuidelinesforReviewers/Committers
<https://cwiki.apache.org/confluence/display/MXNET/Development+Process#DevelopmentProcess-GuidelinesforReviewers/Committers>,
but it probably needs to become more prominent. Any suggestion for a good
place?
Steffen
On Wed, Dec 12, 2018 at 5:23 PM Carin Meier <ca...@gmail.com> wrote:
> Qing - thanks for bringing this up.
>
> I think it would be a good thing to have a document on the wiki to help
> with these sorts of questions.
>
> In fact, since the project is growing with more new committers, maybe we
> could use a "New Committer Guide" with the process of how to get going and
> any FAQ like this one ...
>
> Would you be interested in getting a rough draft going of your recent
> experience? Then others can help collaborate on it.
>
> It would be nice to make the path smoother for other new committers to the
> project.
>
> Best,
> Carin
>
> On Tue, Dec 11, 2018 at 7:18 PM Qing Lan <la...@live.com> wrote:
>
> > Hi all,
> >
> > Recently I self-merged my PR without getting approvals from other
> > committers https://github.com/apache/incubator-mxnet/pull/13617 and only
> > contributors approval. I apologize to the community and thank Marco for
> > pointing out the problem. I took a lesson that we should at least have
> one
> > committer’s approval to merge the code. However, I just found this
> section
> > is missing in the CWiki
> >
> https://cwiki.apache.org/confluence/display/MXNET/Become+an+Apache+MXNet+%28incubating%29+Committer+and+PPMC+Member
> .
> > So I would like to discuss in here:
> >
> > How to conduct the PR reviewing/merging. How many approvals (Committers
> > and Contributors) we should get in order to merge?
> >
> > How to deal with disagreement in the discussion (e.g a
> > contributor/committer request a change)?
> >
> > Please don’t hesitate to share your thoughts!
> >
> > Thanks,
> > Qing
> >
>
Re: [DISCUSS] About the PR merging policy
Posted by Carin Meier <ca...@gmail.com>.
Qing - thanks for bringing this up.
I think it would be a good thing to have a document on the wiki to help
with these sorts of questions.
In fact, since the project is growing with more new committers, maybe we
could use a "New Committer Guide" with the process of how to get going and
any FAQ like this one ...
Would you be interested in getting a rough draft going of your recent
experience? Then others can help collaborate on it.
It would be nice to make the path smoother for other new committers to the
project.
Best,
Carin
On Tue, Dec 11, 2018 at 7:18 PM Qing Lan <la...@live.com> wrote:
> Hi all,
>
> Recently I self-merged my PR without getting approvals from other
> committers https://github.com/apache/incubator-mxnet/pull/13617 and only
> contributors approval. I apologize to the community and thank Marco for
> pointing out the problem. I took a lesson that we should at least have one
> committer’s approval to merge the code. However, I just found this section
> is missing in the CWiki
> https://cwiki.apache.org/confluence/display/MXNET/Become+an+Apache+MXNet+%28incubating%29+Committer+and+PPMC+Member.
> So I would like to discuss in here:
>
> How to conduct the PR reviewing/merging. How many approvals (Committers
> and Contributors) we should get in order to merge?
>
> How to deal with disagreement in the discussion (e.g a
> contributor/committer request a change)?
>
> Please don’t hesitate to share your thoughts!
>
> Thanks,
> Qing
>