You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mxnet.apache.org by Bhavin Thaker <bh...@gmail.com> on 2018/01/14 20:21:04 UTC
Re: [DISCUSSION] Adding labels to PRs
While this email thread had 3 proposals earlier and most comments were on
Jira Vs Github issues, I want to bring attention to and request all
committers to NOT merge a code change unless the PR title and description
has sufficient details as required in the PR template.
Both the PR title and the description are useful for Apache MXNet users in
understanding the code changes in a release from the Release Notes,
especially when a user has a problem and wants to know if a particular
release may fix the problem or not.
This email is based on feedback from the Release retrospective checklist
documented here (prepared after the MXNet 1.0 Release):
https://cwiki.apache.org/confluence/display/MXNET/Release+Retrospective+for+Apache+MXNet+%28incubating%29+1.0.0
Any comments or suggestion on the above Release retrospective are most
welcome.
Bhavin Thaker.
On Mon, Nov 13, 2017 at 3:03 PM, Stephen Bull <sb...@gmail.com> wrote:
> As a complete noob here and someone without a JIRA account, it seems I can
> at least view JIRA issues (so a plus for regular users when it comes to
> release notes). And when it comes time to start contributing, I don't see
> having to create a JIRA account to be a big deal, especially if the
> associated overhead between JIRA and PRs is fairly minimal. It seems there
> are many benefits given the replies so far.
>
> Cheers,
> Stephen
>
> On Mon, 13 Nov 2017 at 11:28 Bhavin Thaker <bh...@gmail.com> wrote:
>
> > +1 for better [1] PR Titles. As suggested by Madan and use by Spark, the
> > current PR template seems to be ignored by folks and so we may want to
> > simplify it to:
> >
> > Q1. What changes were proposed in this pull request?
> >
> > Q2. How was this patch tested?
> >
> >
> > +1 to either [2] Jira OR [3] PR labels.
> >
> > Bhavin Thaker.
> >
> > On Fri, Nov 10, 2017 at 10:40 AM, Markus Weimer <ma...@weimo.de> wrote:
> >
> > > Option 2 works for us over in REEF. We are a bit (too) religious about
> it
> > > [0], but it creates really nice commit messages[1].
> > >
> > > We require each commit message to start with a one line summary which
> > names
> > > the JIRA in brackets and describes the change, e.g. `[REEF-1234] Added
> > > integration with mxnet`. The remainder of the commit message is valid
> > > markdown, and they all end in a block which contains explicit
> references
> > to
> > > the JIRA and pull request:
> > >
> > > ```
> > > JIRA:
> > > [REEF-1234](https://issues.apache.org/jira/browse/REEF-1234)
> > >
> > > Pull Request:
> > > This closes #1234
> > > ```
> > >
> > > The hope is that this structure will eventually proof useful in
> automated
> > > analysis. Then again, we haven't done that ever :)
> > >
> > > Markus
> > >
> > > [0]: https://cwiki.apache.org/confluence/display/REEF/Commit+Messages
> > > [1]: https://github.com/apache/reef/commits/master
> > >
> >
>
Re: [DISCUSSION] Adding labels to PRs
Posted by Pedro Larroy <pe...@gmail.com>.
+1
Agree with Bhavin, Marco and Sheng. I would also like to point out
good commit practices such as, keeping each individual commit small
and on-topic, meaning if that you are changing whitespace and a one
liner fix, it's better practice to separate those commits. Or having
two separate commits for unrelated changes. I see that often we are
squashing these kinds of commits, this makes it more difficult to
review changes and then bisect when you need to find problems. I see
many times them squashed, and I wonder if there's a general rule that
we are following.
On Sun, Jan 14, 2018 at 9:55 PM, Marco de Abreu
<ma...@googlemail.com> wrote:
> +1, we have quite a lot of commits and PR titles which don't state the
> content clearly.
>
> On another note, we should strongly encourage PRs serving only one purpose.
> I have seen quite a decent amount of PRs which introduce multiple unrelated
> changes, while only the main change is getting advertised in the title as
> well as in the description.
>
> -Marco
>
> On Sun, Jan 14, 2018 at 9:32 PM, Bhavin Thaker <bh...@gmail.com>
> wrote:
>
>> +1 to having committers act as role models for descriptive PR title and
>> descriptions.
>>
>> I am sorry to say this explicitly because I found a few committers to have
>> no descriptions in their PRs with PR titles that seemed vague to me as a
>> contributor of Apache MXNet.
>>
>> Bhavin Thaker.
>>
>> On Sun, Jan 14, 2018 at 12:27 PM, Sheng Zha <zh...@apache.org> wrote:
>>
>> > +1. Also, I think committers should act as role models in this regard,
>> and
>> > ensure that our own code changes have sufficient details in the PR.
>> >
>> > Since I proposed the PR template, I also want to re-state that any
>> > suggestions to improving the PR template are welcome. By being open about
>> > it, we can build consensus through discussions, and then act accordingly.
>> > Thanks.
>> >
>> > -sz
>> >
>> > On 2018-01-14 12:21, Bhavin Thaker <bh...@gmail.com> wrote:
>> > > While this email thread had 3 proposals earlier and most comments were
>> on
>> > > Jira Vs Github issues, I want to bring attention to and request all
>> > > committers to NOT merge a code change unless the PR title and
>> description
>> > > has sufficient details as required in the PR template.
>> > >
>> > > Both the PR title and the description are useful for Apache MXNet users
>> > in
>> > > understanding the code changes in a release from the Release Notes,
>> > > especially when a user has a problem and wants to know if a particular
>> > > release may fix the problem or not.
>> > >
>> > > This email is based on feedback from the Release retrospective
>> checklist
>> > > documented here (prepared after the MXNet 1.0 Release):
>> > > https://cwiki.apache.org/confluence/display/MXNET/
>> > Release+Retrospective+for+Apache+MXNet+%28incubating%29+1.0.0
>> > >
>> > > Any comments or suggestion on the above Release retrospective are most
>> > > welcome.
>> > >
>> > > Bhavin Thaker.
>> > >
>> > > On Mon, Nov 13, 2017 at 3:03 PM, Stephen Bull <sb...@gmail.com>
>> wrote:
>> > >
>> > > > As a complete noob here and someone without a JIRA account, it seems
>> I
>> > can
>> > > > at least view JIRA issues (so a plus for regular users when it comes
>> to
>> > > > release notes). And when it comes time to start contributing, I don't
>> > see
>> > > > having to create a JIRA account to be a big deal, especially if the
>> > > > associated overhead between JIRA and PRs is fairly minimal. It seems
>> > there
>> > > > are many benefits given the replies so far.
>> > > >
>> > > > Cheers,
>> > > > Stephen
>> > > >
>> > > > On Mon, 13 Nov 2017 at 11:28 Bhavin Thaker <bh...@gmail.com>
>> > wrote:
>> > > >
>> > > > > +1 for better [1] PR Titles. As suggested by Madan and use by
>> Spark,
>> > the
>> > > > > current PR template seems to be ignored by folks and so we may want
>> > to
>> > > > > simplify it to:
>> > > > >
>> > > > > Q1. What changes were proposed in this pull request?
>> > > > >
>> > > > > Q2. How was this patch tested?
>> > > > >
>> > > > >
>> > > > > +1 to either [2] Jira OR [3] PR labels.
>> > > > >
>> > > > > Bhavin Thaker.
>> > > > >
>> > > > > On Fri, Nov 10, 2017 at 10:40 AM, Markus Weimer <ma...@weimo.de>
>> > wrote:
>> > > > >
>> > > > > > Option 2 works for us over in REEF. We are a bit (too) religious
>> > about
>> > > > it
>> > > > > > [0], but it creates really nice commit messages[1].
>> > > > > >
>> > > > > > We require each commit message to start with a one line summary
>> > which
>> > > > > names
>> > > > > > the JIRA in brackets and describes the change, e.g. `[REEF-1234]
>> > Added
>> > > > > > integration with mxnet`. The remainder of the commit message is
>> > valid
>> > > > > > markdown, and they all end in a block which contains explicit
>> > > > references
>> > > > > to
>> > > > > > the JIRA and pull request:
>> > > > > >
>> > > > > > ```
>> > > > > > JIRA:
>> > > > > > [REEF-1234](https://issues.apache.org/jira/browse/REEF-1234)
>> > > > > >
>> > > > > > Pull Request:
>> > > > > > This closes #1234
>> > > > > > ```
>> > > > > >
>> > > > > > The hope is that this structure will eventually proof useful in
>> > > > automated
>> > > > > > analysis. Then again, we haven't done that ever :)
>> > > > > >
>> > > > > > Markus
>> > > > > >
>> > > > > > [0]: https://cwiki.apache.org/confluence/display/REEF/
>> > Commit+Messages
>> > > > > > [1]: https://github.com/apache/reef/commits/master
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
Re: [DISCUSSION] Adding labels to PRs
Posted by Marco de Abreu <ma...@googlemail.com>.
+1, we have quite a lot of commits and PR titles which don't state the
content clearly.
On another note, we should strongly encourage PRs serving only one purpose.
I have seen quite a decent amount of PRs which introduce multiple unrelated
changes, while only the main change is getting advertised in the title as
well as in the description.
-Marco
On Sun, Jan 14, 2018 at 9:32 PM, Bhavin Thaker <bh...@gmail.com>
wrote:
> +1 to having committers act as role models for descriptive PR title and
> descriptions.
>
> I am sorry to say this explicitly because I found a few committers to have
> no descriptions in their PRs with PR titles that seemed vague to me as a
> contributor of Apache MXNet.
>
> Bhavin Thaker.
>
> On Sun, Jan 14, 2018 at 12:27 PM, Sheng Zha <zh...@apache.org> wrote:
>
> > +1. Also, I think committers should act as role models in this regard,
> and
> > ensure that our own code changes have sufficient details in the PR.
> >
> > Since I proposed the PR template, I also want to re-state that any
> > suggestions to improving the PR template are welcome. By being open about
> > it, we can build consensus through discussions, and then act accordingly.
> > Thanks.
> >
> > -sz
> >
> > On 2018-01-14 12:21, Bhavin Thaker <bh...@gmail.com> wrote:
> > > While this email thread had 3 proposals earlier and most comments were
> on
> > > Jira Vs Github issues, I want to bring attention to and request all
> > > committers to NOT merge a code change unless the PR title and
> description
> > > has sufficient details as required in the PR template.
> > >
> > > Both the PR title and the description are useful for Apache MXNet users
> > in
> > > understanding the code changes in a release from the Release Notes,
> > > especially when a user has a problem and wants to know if a particular
> > > release may fix the problem or not.
> > >
> > > This email is based on feedback from the Release retrospective
> checklist
> > > documented here (prepared after the MXNet 1.0 Release):
> > > https://cwiki.apache.org/confluence/display/MXNET/
> > Release+Retrospective+for+Apache+MXNet+%28incubating%29+1.0.0
> > >
> > > Any comments or suggestion on the above Release retrospective are most
> > > welcome.
> > >
> > > Bhavin Thaker.
> > >
> > > On Mon, Nov 13, 2017 at 3:03 PM, Stephen Bull <sb...@gmail.com>
> wrote:
> > >
> > > > As a complete noob here and someone without a JIRA account, it seems
> I
> > can
> > > > at least view JIRA issues (so a plus for regular users when it comes
> to
> > > > release notes). And when it comes time to start contributing, I don't
> > see
> > > > having to create a JIRA account to be a big deal, especially if the
> > > > associated overhead between JIRA and PRs is fairly minimal. It seems
> > there
> > > > are many benefits given the replies so far.
> > > >
> > > > Cheers,
> > > > Stephen
> > > >
> > > > On Mon, 13 Nov 2017 at 11:28 Bhavin Thaker <bh...@gmail.com>
> > wrote:
> > > >
> > > > > +1 for better [1] PR Titles. As suggested by Madan and use by
> Spark,
> > the
> > > > > current PR template seems to be ignored by folks and so we may want
> > to
> > > > > simplify it to:
> > > > >
> > > > > Q1. What changes were proposed in this pull request?
> > > > >
> > > > > Q2. How was this patch tested?
> > > > >
> > > > >
> > > > > +1 to either [2] Jira OR [3] PR labels.
> > > > >
> > > > > Bhavin Thaker.
> > > > >
> > > > > On Fri, Nov 10, 2017 at 10:40 AM, Markus Weimer <ma...@weimo.de>
> > wrote:
> > > > >
> > > > > > Option 2 works for us over in REEF. We are a bit (too) religious
> > about
> > > > it
> > > > > > [0], but it creates really nice commit messages[1].
> > > > > >
> > > > > > We require each commit message to start with a one line summary
> > which
> > > > > names
> > > > > > the JIRA in brackets and describes the change, e.g. `[REEF-1234]
> > Added
> > > > > > integration with mxnet`. The remainder of the commit message is
> > valid
> > > > > > markdown, and they all end in a block which contains explicit
> > > > references
> > > > > to
> > > > > > the JIRA and pull request:
> > > > > >
> > > > > > ```
> > > > > > JIRA:
> > > > > > [REEF-1234](https://issues.apache.org/jira/browse/REEF-1234)
> > > > > >
> > > > > > Pull Request:
> > > > > > This closes #1234
> > > > > > ```
> > > > > >
> > > > > > The hope is that this structure will eventually proof useful in
> > > > automated
> > > > > > analysis. Then again, we haven't done that ever :)
> > > > > >
> > > > > > Markus
> > > > > >
> > > > > > [0]: https://cwiki.apache.org/confluence/display/REEF/
> > Commit+Messages
> > > > > > [1]: https://github.com/apache/reef/commits/master
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSSION] Adding labels to PRs
Posted by Bhavin Thaker <bh...@gmail.com>.
+1 to having committers act as role models for descriptive PR title and
descriptions.
I am sorry to say this explicitly because I found a few committers to have
no descriptions in their PRs with PR titles that seemed vague to me as a
contributor of Apache MXNet.
Bhavin Thaker.
On Sun, Jan 14, 2018 at 12:27 PM, Sheng Zha <zh...@apache.org> wrote:
> +1. Also, I think committers should act as role models in this regard, and
> ensure that our own code changes have sufficient details in the PR.
>
> Since I proposed the PR template, I also want to re-state that any
> suggestions to improving the PR template are welcome. By being open about
> it, we can build consensus through discussions, and then act accordingly.
> Thanks.
>
> -sz
>
> On 2018-01-14 12:21, Bhavin Thaker <bh...@gmail.com> wrote:
> > While this email thread had 3 proposals earlier and most comments were on
> > Jira Vs Github issues, I want to bring attention to and request all
> > committers to NOT merge a code change unless the PR title and description
> > has sufficient details as required in the PR template.
> >
> > Both the PR title and the description are useful for Apache MXNet users
> in
> > understanding the code changes in a release from the Release Notes,
> > especially when a user has a problem and wants to know if a particular
> > release may fix the problem or not.
> >
> > This email is based on feedback from the Release retrospective checklist
> > documented here (prepared after the MXNet 1.0 Release):
> > https://cwiki.apache.org/confluence/display/MXNET/
> Release+Retrospective+for+Apache+MXNet+%28incubating%29+1.0.0
> >
> > Any comments or suggestion on the above Release retrospective are most
> > welcome.
> >
> > Bhavin Thaker.
> >
> > On Mon, Nov 13, 2017 at 3:03 PM, Stephen Bull <sb...@gmail.com> wrote:
> >
> > > As a complete noob here and someone without a JIRA account, it seems I
> can
> > > at least view JIRA issues (so a plus for regular users when it comes to
> > > release notes). And when it comes time to start contributing, I don't
> see
> > > having to create a JIRA account to be a big deal, especially if the
> > > associated overhead between JIRA and PRs is fairly minimal. It seems
> there
> > > are many benefits given the replies so far.
> > >
> > > Cheers,
> > > Stephen
> > >
> > > On Mon, 13 Nov 2017 at 11:28 Bhavin Thaker <bh...@gmail.com>
> wrote:
> > >
> > > > +1 for better [1] PR Titles. As suggested by Madan and use by Spark,
> the
> > > > current PR template seems to be ignored by folks and so we may want
> to
> > > > simplify it to:
> > > >
> > > > Q1. What changes were proposed in this pull request?
> > > >
> > > > Q2. How was this patch tested?
> > > >
> > > >
> > > > +1 to either [2] Jira OR [3] PR labels.
> > > >
> > > > Bhavin Thaker.
> > > >
> > > > On Fri, Nov 10, 2017 at 10:40 AM, Markus Weimer <ma...@weimo.de>
> wrote:
> > > >
> > > > > Option 2 works for us over in REEF. We are a bit (too) religious
> about
> > > it
> > > > > [0], but it creates really nice commit messages[1].
> > > > >
> > > > > We require each commit message to start with a one line summary
> which
> > > > names
> > > > > the JIRA in brackets and describes the change, e.g. `[REEF-1234]
> Added
> > > > > integration with mxnet`. The remainder of the commit message is
> valid
> > > > > markdown, and they all end in a block which contains explicit
> > > references
> > > > to
> > > > > the JIRA and pull request:
> > > > >
> > > > > ```
> > > > > JIRA:
> > > > > [REEF-1234](https://issues.apache.org/jira/browse/REEF-1234)
> > > > >
> > > > > Pull Request:
> > > > > This closes #1234
> > > > > ```
> > > > >
> > > > > The hope is that this structure will eventually proof useful in
> > > automated
> > > > > analysis. Then again, we haven't done that ever :)
> > > > >
> > > > > Markus
> > > > >
> > > > > [0]: https://cwiki.apache.org/confluence/display/REEF/
> Commit+Messages
> > > > > [1]: https://github.com/apache/reef/commits/master
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSSION] Adding labels to PRs
Posted by Sheng Zha <zh...@apache.org>.
+1. Also, I think committers should act as role models in this regard, and ensure that our own code changes have sufficient details in the PR.
Since I proposed the PR template, I also want to re-state that any suggestions to improving the PR template are welcome. By being open about it, we can build consensus through discussions, and then act accordingly. Thanks.
-sz
On 2018-01-14 12:21, Bhavin Thaker <bh...@gmail.com> wrote:
> While this email thread had 3 proposals earlier and most comments were on
> Jira Vs Github issues, I want to bring attention to and request all
> committers to NOT merge a code change unless the PR title and description
> has sufficient details as required in the PR template.
>
> Both the PR title and the description are useful for Apache MXNet users in
> understanding the code changes in a release from the Release Notes,
> especially when a user has a problem and wants to know if a particular
> release may fix the problem or not.
>
> This email is based on feedback from the Release retrospective checklist
> documented here (prepared after the MXNet 1.0 Release):
> https://cwiki.apache.org/confluence/display/MXNET/Release+Retrospective+for+Apache+MXNet+%28incubating%29+1.0.0
>
> Any comments or suggestion on the above Release retrospective are most
> welcome.
>
> Bhavin Thaker.
>
> On Mon, Nov 13, 2017 at 3:03 PM, Stephen Bull <sb...@gmail.com> wrote:
>
> > As a complete noob here and someone without a JIRA account, it seems I can
> > at least view JIRA issues (so a plus for regular users when it comes to
> > release notes). And when it comes time to start contributing, I don't see
> > having to create a JIRA account to be a big deal, especially if the
> > associated overhead between JIRA and PRs is fairly minimal. It seems there
> > are many benefits given the replies so far.
> >
> > Cheers,
> > Stephen
> >
> > On Mon, 13 Nov 2017 at 11:28 Bhavin Thaker <bh...@gmail.com> wrote:
> >
> > > +1 for better [1] PR Titles. As suggested by Madan and use by Spark, the
> > > current PR template seems to be ignored by folks and so we may want to
> > > simplify it to:
> > >
> > > Q1. What changes were proposed in this pull request?
> > >
> > > Q2. How was this patch tested?
> > >
> > >
> > > +1 to either [2] Jira OR [3] PR labels.
> > >
> > > Bhavin Thaker.
> > >
> > > On Fri, Nov 10, 2017 at 10:40 AM, Markus Weimer <ma...@weimo.de> wrote:
> > >
> > > > Option 2 works for us over in REEF. We are a bit (too) religious about
> > it
> > > > [0], but it creates really nice commit messages[1].
> > > >
> > > > We require each commit message to start with a one line summary which
> > > names
> > > > the JIRA in brackets and describes the change, e.g. `[REEF-1234] Added
> > > > integration with mxnet`. The remainder of the commit message is valid
> > > > markdown, and they all end in a block which contains explicit
> > references
> > > to
> > > > the JIRA and pull request:
> > > >
> > > > ```
> > > > JIRA:
> > > > [REEF-1234](https://issues.apache.org/jira/browse/REEF-1234)
> > > >
> > > > Pull Request:
> > > > This closes #1234
> > > > ```
> > > >
> > > > The hope is that this structure will eventually proof useful in
> > automated
> > > > analysis. Then again, we haven't done that ever :)
> > > >
> > > > Markus
> > > >
> > > > [0]: https://cwiki.apache.org/confluence/display/REEF/Commit+Messages
> > > > [1]: https://github.com/apache/reef/commits/master
> > > >
> > >
> >
>