mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Steffen Rochel <steffenroc...@gmail.com>
Subject Re: Squash/Merge PRs
Date Fri, 13 Jul 2018 15:51:39 GMT
Thanks Aaron for restarting the discussion. A vote or decision about voting
should not be called in the middle of a discussion either. It is not
reasonable that everybody can follow every discussion. A vote should be a
separate thread with [VOTE] in the subject and concise description what is
the vote about. I suggest to follow such practice as well for lazy votes.
Steffen
On Fri, Jul 13, 2018 at 7:03 AM Aaron Markham <aaron.s.markham@gmail.com>
wrote:

> @naveen - Hanlon's razor applies, so I am not concerned with any
> individual's action or inaction here. Most people don't even realize its
> happening (I didn't until recently), and the common practice doesn't manage
> collaboration well at all. Let's fix that, and turn a less-than-ideal
> common practice into a best practice.
>
> @eric - We talked about several things in this thread, and your -1 seems to
> have stymied the dialogue. So I'm going to try to clarify your vote and
> your proposal to get the discussion going again.
>
> Your vote was to never use rebase-merge on pull requests to master,
> regardless of the number of contributors, even if the commit hygiene is
> good. Your vote was to maintain the status quo with regard to pull
> requests. You didn't comment on usage of the co-author field, so please
> clarify if your vote includes not using this either, or if you agree that
> the co-author option should be utilized.
>
> Your proposal was for contributors to request of a committer, the creation
> of a branch when there are multiple collaborators on a feature, for the
> collaborators to work on this branch, then, and only then, committers would
> use rebase-merge when bringing this branch into master. Contributors who
> are working alone, or don't mind their contributions squashed should
> continue to create pull requests to master.
>
> I can assume that the collaborators guide would need to be updated and the
> process for requesting branches be formalized and expeditious. I can also
> imagine that we might fall into the same discussion about commit hygiene
> here, and there would need to be formalized expectations in the
> collaborators guide.
>
> Did I get that mostly right? I have more questions about your proposal, but
> I'd like to hear what other people think first and their responses may
> provide further clarifications.
>
> Cheers,
> Aaron
>
> On Thu, Jul 12, 2018 at 4:46 PM, eric xie <eric.jy.xie@gmail.com> wrote:
>
> > -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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message