mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From eric xie <eric.jy....@gmail.com>
Subject Re: Squash/Merge PRs
Date Thu, 12 Jul 2018 23:46:52 GMT
-1

We should stick with always using squash. It maintains PR reference in commit history. It
is also common practice.

If you want to included commits from multiple contributors in a single PR, open a branch and
make PRs to that branch. Then only use rebase when merging that branch to master.

Thanks,
Eric

On 2018/07/12 23:38:54, Marco de Abreu <marco.g.abreu@googlemail.com.INVALID> wrote:

> Excellent :)
> 
> I don't think we need a formal vote for this but rather let this be lazy
> consensus if nobody minds.
> 
> Could we maybe revisit this decision in 1 or 2 months and then assess the
> state of commit history in all PRs (including squashed ones) and with focus
> on rebase merged PRs?
> 
> -Marco
> 
> Naveen Swamy <mnnaveen@gmail.com> schrieb am Fr., 13. Juli 2018, 01:33:
> 
> > You are right, its already enabled :) I saw that option greyed out for one
> > of the PR(for a different reason) and assumed we need INFRA to enable.
> >
> > On Thu, Jul 12, 2018 at 4:22 PM, Marco de Abreu <
> > marco.g.abreu@googlemail.com.invalid> wrote:
> >
> > > Coukd you elaborate why we would need a ticket for that? GitHub supports
> > it
> > > out of the box and it is enabled as far as I can tell. You just have to
> > > press the little arrow besides the merge button.
> > >
> > > -marco
> > >
> > > Naveen Swamy <mnnaveen@gmail.com> schrieb am Fr., 13. Juli 2018, 00:54:
> > >
> > > > Yes to insist on commit hygiene when rebase-merge. We have to open a
> > JIRA
> > > > with Apache Infra to enable rebase-merge on the repo.
> > > >
> > > > On Thu, Jul 12, 2018 at 3:21 PM, Marco de Abreu <
> > > > marco.g.abreu@googlemail.com.invalid> wrote:
> > > >
> > > > > Fully agree, if we can get our commit hygiene up to industry
> > standard,
> > > I
> > > > > don't see any problems in using rebase merge instead. For the short
> > > term
> > > > I
> > > > > agree that it should be fine to rebase merge individual PRs with
> > > multiple
> > > > > contributors. But in my opinion we should then insist on people
> > > amending
> > > > > their commit history if we deem it below our standard. A PR should
> > not
> > > be
> > > > > rebase merged if the history is not proper - verifying that is the
> > > > > responsibility of the merging committer, but ideally, we'd make
> > people
> > > > > aware of problems early on. What do you think?
> > > > >
> > > > > In general, I think we should gradually start taking this into
> > account
> > > > when
> > > > > reviewing with the goal of all PRs having a proper commit history.
> > This
> > > > > would allow us in the long term to completely move away from
> > squashing.
> > > > >
> > > > > -Marco
> > > > >
> > > > > Pedro Larroy <pedro.larroy.lists@gmail.com> schrieb am Fr.,
13. Juli
> > > > 2018,
> > > > > 00:07:
> > > > >
> > > > > > This is a great discussion, thanks for raising the question
Naveen.
> > > > > >
> > > > > > From my experience working in all kinds of software project
of
> > > varying
> > > > > > sizes and flavours:
> > > > > >
> > > > > >    1. People should be aware of git rebase interactive and have
> > more
> > > > > >    hygiene in their PRs. If a PR is hygienic and is separated
in a
> > > set
> > > > of
> > > > > >    semantic commits, it's better not squashed as it helps finding
> > > bugs
> > > > /
> > > > > >    bisecting. A "dirty" PR with WIP commits, is better squashed,
or
> > > > > > requested
> > > > > >    to rebase interactively on CR.
> > > > > >    2. Mixing different semantic changes on a single PR is an
> > > > > anti-pattern,
> > > > > >    for example fixing whitespace or reformatting many lines
and
> > > > changing
> > > > > > some
> > > > > >    code which is hidden in a bunch of trivial changes and could
> > > cause a
> > > > > > bug or
> > > > > >    major change of behaviour.
> > > > > >    3. Agreed what with others have mentioned about small
> > incremental
> > > > > steps
> > > > > >    better than huge PRs. We also have JIRA or issues to relate
a
> > set
> > > of
> > > > > PRs
> > > > > >    together. If not possible, PR with a set of non squashed
commits
> > > is
> > > > > also
> > > > > >    good.
> > > > > >
> > > > > > Hope it helps.
> > > > > >
> > > > > > Pedro.
> > > > > >
> > > > > > On Thu, Jul 12, 2018 at 11:47 PM Naveen Swamy <mnnaveen@gmail.com>
> > > > > wrote:
> > > > > >
> > > > > > > @Aaron, I do not think most contributors(SDE or not) were
even
> > > aware
> > > > > that
> > > > > > > their commits are getting squashed into one and thereby
others
> > > losing
> > > > > > > credit on that work. I would assume no bad intentions there.
> > > > > > >
> > > > > > > @Hao,
> > > > > > > Agree to breaking down into small and reasonable sized
PRs, but I
> > > > think
> > > > > > > very small PRs will overwhelm the CI as it stands since
it runs
> > > > > > > everything(this is a separate topic and needs fixing).
> > > > > > >
> > > > > > > For cases similar to Aaron's he can raise the PR for the
doc
> > > > > > > work(regardless of fork or not) and add other contributors
as
> > > > > co-authors.
> > > > > > >
> > > > > > > @Marco, co-author might not be suitable always suitable
and agree
> > > > with
> > > > > > Hao
> > > > > > > that should be used on exceptions. co-author also makes
it hard
> > to
> > > > see
> > > > > > the
> > > > > > > contributions individually.
> > > > > > >
> > > > > > > @Marco, why can we not have Rebase merge enabled on the
repo and
> > > use
> > > > > that
> > > > > > > when there are multiple contributors. This discussion is
only
> > about
> > > > Not
> > > > > > > Squashing when there are multiple contributors and agree
to
> > > continue
> > > > > the
> > > > > > > practice of Squashing in general.
> > > > > > >
> > > > > > > Until the tooling is fixed, I suggest to use the co-author
> > feature
> > > > when
> > > > > > > collaborating on a fork.
> > > > > > >
> > > > > > > Also, I just want to reiterate and request contributors
to have
> > > > > > meaningful
> > > > > > > and fewer commits on PRs.
> > > > > > >
> > > > > > > Thanks, Naveen
> > > > > > >
> > > > > > >
> > > > > > > On Thu, Jul 12, 2018 at 11:40 AM, Marco de Abreu <
> > > > > > > marco.g.abreu@googlemail.com.invalid> wrote:
> > > > > > >
> > > > > > > > As of now it will require the usage of a merge bot
as far as I
> > > > > > understand
> > > > > > > > this issue: https://github.com/python/miss-islington/issues/16
> > .
> > > > > > Instead
> > > > > > > of
> > > > > > > > using the web interface do merge, we'd then trigger
the bot to
> > do
> > > > the
> > > > > > > merge
> > > > > > > > on our behalf. There are pre-made solutions on the
internet we
> > > > might
> > > > > be
> > > > > > > > able to leverage, but it would take some time to get
into it
> > and
> > > > > adapt
> > > > > > > our
> > > > > > > > process.
> > > > > > > >
> > > > > > > > Additionally, GitHub has this feature request tracked
> > internally.
> > > > > Let's
> > > > > > > see
> > > > > > > > when they get to add it.
> > > > > > > >
> > > > > > > > -Marco
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Aaron Markham <aaron.s.markham@gmail.com> schrieb
am Do., 12.
> > > Juli
> > > > > > 2018,
> > > > > > > > 21:33:
> > > > > > > >
> > > > > > > > > My point was about collaboration, or the lack
thereof, and
> > how
> > > > the
> > > > > > > > tooling
> > > > > > > > > and apparent rewards from contribution statistics
can
> > reinforce
> > > > > lone
> > > > > > > > ranger
> > > > > > > > > behavior. People can and should be proud of their
work, but
> > why
> > > > > does
> > > > > > it
> > > > > > > > > have to be alone? Working within the context
of a team is
> > > > something
> > > > > > to
> > > > > > > be
> > > > > > > > > proud of too, but if your stats and standing
are graded by
> > how
> > > > the
> > > > > > > > commits
> > > > > > > > > and merges land, and counting lines of code,
then incentives
> > of
> > > > the
> > > > > > > > system
> > > > > > > > > are skewed.
> > > > > > > > > How do we go about implementing the co-author
process? It
> > > sounds
> > > > > like
> > > > > > > > > something worth doing!
> > > > > > > > >
> > > > > > > > > On Thu, Jul 12, 2018 at 11:15 AM, Hao Jin <
> > hjjn.amzn@gmail.com
> > > >
> > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi Aaron,
> > > > > > > > > > To be fair, this discussion has nothing
to do with "pride"
> > of
> > > > > SDEs,
> > > > > > > but
> > > > > > > > > > rather a discussion on what is a better
software
> > engineering
> > > > > > practice
> > > > > > > > for
> > > > > > > > > > the project. Breaking a large project into
smaller tasks
> > is a
> > > > > good
> > > > > > > > > software
> > > > > > > > > > engineering practice. This article(https://arxiv.org/pdf/
> > > > > > > > 1702.01715.pdf)
> > > > > > > > > > on
> > > > > > > > > > Google's software engineering practice says:
"Engineers are
> > > > > > > encouraged
> > > > > > > > to
> > > > > > > > > > keep each individual change small​, with
larger changes
> > > > > preferably
> > > > > > > > broken
> > > > > > > > > > into a series of smaller changes that a
reviewer can easily
> > > > > review
> > > > > > in
> > > > > > > > one
> > > > > > > > > > go.". If you have concerns or your comments
on this
> > practice,
> > > > we
> > > > > > can
> > > > > > > > take
> > > > > > > > > > the discussion offline. On the other hand,
an important
> > > spirit
> > > > of
> > > > > > > > Apache
> > > > > > > > > > community is that "one must interact with
others, and share
> > > > > vision
> > > > > > > and
> > > > > > > > > > knowledge"(https://community.apache.org/contributors/),
if
> > > you
> > > > > > > > observed
> > > > > > > > > > that a majority of the contributors have
serious problems
> > > with
> > > > > > their
> > > > > > > > > > writing maybe you can share some tips and
hints on how to
> > > write
> > > > > > > better
> > > > > > > > > > documentations, in that way not only a lot
of us within the
> > > > > > community
> > > > > > > > can
> > > > > > > > > > have some growth but also you can save some
time for
> > writing
> > > > more
> > > > > > > good
> > > > > > > > > > documentations and blogposts for MXNet,
don't you think so?
> > > Or,
> > > > > if
> > > > > > > you
> > > > > > > > > only
> > > > > > > > > > have to fix the doc for someone once in
a while, I
> > definitely
> > > > > agree
> > > > > > > > that
> > > > > > > > > > you should be given the credit for that,
and that's where
> > the
> > > > > > > co-author
> > > > > > > > > > field can be useful, which was exactly what
I was saying in
> > > my
> > > > > > > previous
> > > > > > > > > > email. Thanks!
> > > > > > > > > > Hao
> > > > > > > > > >
> > > > > > > > > > On Thu, Jul 12, 2018 at 8:16 AM, Aaron Markham
<
> > > > > > > > > aaron.s.markham@gmail.com>
> > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > >  This is a great discussion, and close
to my heart,
> > > because I
> > > > > > want
> > > > > > > to
> > > > > > > > > > > include more writers and editors in
our community, and
> > I'm
> > > > > > > struggling
> > > > > > > > > to
> > > > > > > > > > > see how to best manage this. It seems
like being the sole
> > > > > > > contributor
> > > > > > > > > on
> > > > > > > > > > a
> > > > > > > > > > > feature is like an engineer's Lone
Ranger badge of
> > pride! I
> > > > > feel
> > > > > > > like
> > > > > > > > > it
> > > > > > > > > > > should be a red flag.
> > > > > > > > > > >
> > > > > > > > > > > I work in collaboration with dozens
of engineers to
> > improve
> > > > > their
> > > > > > > > > > > documentation, explain their features,
change flows to
> > > > improve
> > > > > > user
> > > > > > > > > > > experience, and sometimes fix bugs
and write code. When
> > the
> > > > > PR's
> > > > > > > docs
> > > > > > > > > has
> > > > > > > > > > > great coverage, is clear, and not loaded
with bad grammar
> > > and
> > > > > > > > spelling
> > > > > > > > > > > mistakes, I will put comments in a
review. But sometimes
> > > > there
> > > > > > > needs
> > > > > > > > to
> > > > > > > > > > be
> > > > > > > > > > > a complete rework such that hundreds
of review comments
> > > isn't
> > > > > > > > feasible,
> > > > > > > > > > and
> > > > > > > > > > > they can't be properly addressed. In
a lot of these
> > cases,
> > > I
> > > > > > commit
> > > > > > > > my
> > > > > > > > > > > changes to someone else's fork. Now
I know the people I
> > > work
> > > > > with
> > > > > > > on
> > > > > > > > > > their
> > > > > > > > > > > PRs appreciate my help, but when all
of this work gets
> > > > squashed
> > > > > > > down
> > > > > > > > to
> > > > > > > > > > one
> > > > > > > > > > > commit, I'm pretty much regularly getting
squashed out.
> > I'm
> > > > not
> > > > > > > sure
> > > > > > > > > who
> > > > > > > > > > > else is in this boat, but does the
community recognize
> > our
> > > > > > > > > contributions
> > > > > > > > > > > when this is the general mode of operation?
> > > > > > > > > > >
> > > > > > > > > > > Regarding co-author - I'm not sure
how people would feel
> > > > about
> > > > > me
> > > > > > > > > being a
> > > > > > > > > > > co-author on their work. Documentation
and clarity in
> > your
> > > > work
> > > > > > > > product
> > > > > > > > > > is
> > > > > > > > > > > important, but people's views on their
personal *code*
> > > > > > contribution
> > > > > > > > > seems
> > > > > > > > > > > to be more important than the overall
code & feature
> > > quality
> > > > > > itself
> > > > > > > > > when
> > > > > > > > > > > docs are part of the package. I feel
that if I do
> > follow-on
> > > > PRs
> > > > > > > > instead
> > > > > > > > > > of
> > > > > > > > > > > fixing things before they are merged,
that we would be
> > > > > releasing
> > > > > > > > > > incorrect,
> > > > > > > > > > > incomplete, and inferior product. But,
in absence of a
> > > better
> > > > > > > > solution,
> > > > > > > > > > > maybe that's the pill we have to swallow.
> > > > > > > > > > >
> > > > > > > > > > > -1 on lots of small PRs (until we expand
our range of
> > > > > committers
> > > > > > > and
> > > > > > > > > > reduce
> > > > > > > > > > > the latency in reviews and merges).
> > > > > > > > > > > +1 on improving the quality of commit
messages, so we
> > don't
> > > > > have
> > > > > > to
> > > > > > > > > > squash
> > > > > > > > > > > & merge all of the time.
> > > > > > > > > > > +1 on improving the practice of better
commit & merge
> > > > > management
> > > > > > so
> > > > > > > > > that
> > > > > > > > > > > the commit history is concise and meaningful.
(I'm guilty
> > > of
> > > > > > this,
> > > > > > > > and
> > > > > > > > > > > certainly need to improve here.)
> > > > > > > > > > > +1 on co-author - assuming that's what
most everyone
> > thinks
> > > > is
> > > > > a
> > > > > > > good
> > > > > > > > > > > solution for now. I'm unclear on how
this gets managed
> > > > though.
> > > > > > > > > > >
> > > > > > > > > > > Cheers,
> > > > > > > > > > > Aaron
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > On Thu, Jul 12, 2018 at 5:19 AM, Anton
Chernov <
> > > > > > > mechernov@gmail.com>
> > > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > Unfortunately there has been significant
push back for
> > > > small
> > > > > > > > > iterative
> > > > > > > > > > > PR's
> > > > > > > > > > > > and as a result they have grown
substantially involving
> > > > > > multiple
> > > > > > > > > > > > contributors, multiple commits,
sometimes multiple
> > > branches
> > > > > to
> > > > > > be
> > > > > > > > > > worked
> > > > > > > > > > > > on.
> > > > > > > > > > > >
> > > > > > > > > > > > I fully agree and support all
points that Jin raised:
> > > > > > > > > > > >
> > > > > > > > > > > > 1) Most contributions should be
broken down into
> > smallest
> > > > > > > possible
> > > > > > > > > > PR's.
> > > > > > > > > > > > 2) If a PR is small enough a single
person can complete
> > > it.
> > > > > > > > > > > > 3) If a majority of PR is done
by someone, and there's
> > > some
> > > > > > minor
> > > > > > > > > issue
> > > > > > > > > > > > he/she needs help with it could
be done in a follow up
> > PR
> > > > by
> > > > > > > > anybody,
> > > > > > > > > > > > including the reviewer.
> > > > > > 
[message truncated...]
Mime
View raw message