mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chiyuan Zhang <plus...@gmail.com>
Subject Re: Which merge option to use on the Import Julia binding PR?
Date Sun, 30 Sep 2018 02:01:29 GMT
Sorry, here is the image: https://imgur.com/V5wd2XB

And here is the github document on the 3 different merge options for the
web UI button: https://help.github.com/articles/about-pull-request-merges/

On Sat, Sep 29, 2018 at 6:48 PM Marco de Abreu
<marco.g.abreu@googlemail.com.invalid> wrote:

> Could you upload the picture somewhere please? HTML is being stripped out
> on email lists.
>
> Chiyuan Zhang <pluskid@gmail.com> schrieb am So., 30. Sep. 2018, 03:44:
>
> > There is an option in the repo settings menu to disable or enable
> > merge-commit for PR, see a screenshot below (from a different github
> > project):
> >
> > [image: image.png]
> >
> > My guess is that this is disabled for the reason to avoid creating
> > non-linear history for standard PRs (as oppose to technical problem). But
> > this is only my guess, it would be great if someone could confirm.
> >
> > Best,
> > Chiyuan
> >
> > On Sat, Sep 29, 2018 at 3:50 AM Carin Meier <carinmeier@gmail.com>
> wrote:
> >
> >> I believe so, but if someone wants to confirm it would be great.
> >> Unfortunately, I just came down with a cold/flu so I will be out of
> >> communication for a bit
> >>
> >> On Fri, Sep 28, 2018 at 9:51 PM Marco de Abreu
> >> <marco.g.abreu@googlemail.com.invalid> wrote:
> >>
> >> > Are we sure that this is due to lacking permissions and not because of
> >> some
> >> > technical limitation? If we are certain, we can ask out mentors to
> >> create a
> >> > ticket with Apache Infra to make that switch.
> >> >
> >> > -Marco
> >> >
> >> > Carin Meier <carinmeier@gmail.com> schrieb am Sa., 29. Sep. 2018,
> >> 01:17:
> >> >
> >> > > I made a test regular merge commit into a copy of master. It seemed
> >> to go
> >> > > fine. Here is a listing of what it will look like for everyone.
> >> > >
> >> > >
> >> >
> >>
> https://github.com/apache/incubator-mxnet/commits/test-merge-julia-import
> >> > >
> >> > > Although, I would be happy to push the merge button. I think the
> most
> >> > > important thing is to get the PR merged, so whatever way is the best
> >> to
> >> > > make that happen, let's do it.
> >> > >
> >> > > So - Does the regular merge seem like a good option?
> >> > > If so, what is the best way to make that happen?
> >> > >
> >> > >
> >> > > On Fri, Sep 28, 2018 at 6:05 PM Chiyuan Zhang <pluskid@gmail.com>
> >> wrote:
> >> > >
> >> > > > Agreed with Pedro. Maybe the merge-commit option from the github
> >> > > interface
> >> > > > was disabled for a reason. But as Pedro said, maybe it is good
to
> >> > > > temporarily enable it for this PR and merge using that.
> >> > > >
> >> > > >
> >> > > >    - It should be technically easier than rebasing due to the
> >> > > >    git-subtree-import issue we are currently having
> >> > > >    - It also avoid stacking a huge commit history on *top* of
> >> current
> >> > > >    history
> >> > > >    - The downside is probably the history of the project is not
> >> linear
> >> > > >    anymore, but I think this is actually what we would like to
> have
> >> for
> >> > > > this
> >> > > >    particular case, because the contents of the main repo and
the
> >> julia
> >> > > > branch
> >> > > >    actually does not overlap. So it makes sense to have two tails
> >> with
> >> > > > their
> >> > > >    own history.
> >> > > >
> >> > > > Carin: I guess if someone with admin permission on the github
> could
> >> > > > temporarily enable the merge-commit option, then pushing the
> button
> >> on
> >> > > the
> >> > > > web might simply work.
> >> > > >
> >> > > > Best,
> >> > > > Chiyuan
> >> > > >
> >> > > > On Fri, Sep 28, 2018 at 2:53 PM Carin Meier <carinmeier@gmail.com
> >
> >> > > wrote:
> >> > > >
> >> > > > > Pedro - Maybe a merge commit is a better answer in this
case. I
> >> > > > originally
> >> > > > > ruled it out since it wasn't an option in the github web
> >> interface,
> >> > but
> >> > > > > since this looks like it is going to have to be done outside
it
> >> > because
> >> > > > of
> >> > > > > the subtrees anyway, it might be a better fit.
> >> > > > >
> >> > > > > On Fri, Sep 28, 2018 at 5:07 PM Carin Meier <
> carinmeier@gmail.com
> >> >
> >> > > > wrote:
> >> > > > >
> >> > > > > > We are actually running into troubles with using the
subtree
> and
> >> > the
> >> > > > > > rebase. Since it looks like this is not going to be
a simple,
> >> > "click
> >> > > > the
> >> > > > > > button" through the web page merge, I rather hand this
task
> off
> >> to
> >> > > > > someone
> >> > > > > > with more context in making sure it gets in there correctly.
> >> > > > > >
> >> > > > > > Chiyuan or any others, would you be willing to take
this over?
> >> > > > > >
> >> > > > > > Thanks,
> >> > > > > > Carin
> >> > > > > >
> >> > > > > > On Fri, Sep 28, 2018 at 5:00 PM Naveen Swamy <
> >> mnnaveen@gmail.com>
> >> > > > wrote:
> >> > > > > >
> >> > > > > >> Should we try to first being into a branch and
then try merge
> >> that
> >> > > > > >> branch?
> >> > > > > >>
> >> > > > > >> > On Sep 28, 2018, at 4:40 PM, Pedro Larroy
<
> >> > > > > pedro.larroy.lists@gmail.com>
> >> > > > > >> wrote:
> >> > > > > >> >
> >> > > > > >> > I'm not familiar with the specifics of this
contribution,
> as
> >> a
> >> > > > general
> >> > > > > >> > approach my understanding is that if the list
of commits is
> >> big
> >> > > and
> >> > > > > you
> >> > > > > >> > want to preserve history, usually merging
is better so you
> >> keep
> >> > > > > history
> >> > > > > >> and
> >> > > > > >> > causality, if you rebase all the commits on
top of master
> you
> >> > are
> >> > > > > >> changing
> >> > > > > >> > the history of these commits which can't be
individually
> >> > reverted
> >> > > as
> >> > > > > >> some
> >> > > > > >> > have suggested before. Maybe is because I
come from a
> >> mercurial
> >> > > > > >> background,
> >> > > > > >> > but my initial impression would be either
to:
> >> > > > > >> > 1. squash everything and rebase
> >> > > > > >> > 2. or merge without rebasing or squashing.
> >> > > > > >> >
> >> > > > > >> > Pedro.
> >> > > > > >> >
> >> > > > > >> >> On Thu, Sep 27, 2018 at 3:10 PM Carin
Meier <
> >> > > carinmeier@gmail.com>
> >> > > > > >> wrote:
> >> > > > > >> >>
> >> > > > > >> >> Thanks everyone for the input. I'll try
to summarize the
> >> > feedback
> >> > > > > from
> >> > > > > >> the
> >> > > > > >> >> responses:
> >> > > > > >> >>
> >> > > > > >> >> Using Squash-Merge is the project standard
for very good
> >> > reasons.
> >> > > > > >> However,
> >> > > > > >> >> in the case of this PR to bring in the
Julia language from
> >> its
> >> > > > > sibling
> >> > > > > >> >> repo, we want to preserve all the individual
commits of
> the
> >> > many
> >> > > > > >> >> contributors that have worked over multiple
years to make
> >> this
> >> > a
> >> > > > > great
> >> > > > > >> >> language binding. We will use Rebase-Merge
for it.
> >> > > > > >> >>
> >> > > > > >> >> Chiyuan - thanks for the suggestion of
using a tag. I
> think
> >> we
> >> > > can
> >> > > > > try
> >> > > > > >> it
> >> > > > > >> >> initially without it since there are other
ways to browse
> >> the
> >> > > > commit
> >> > > > > >> >> history, like looking at the PRs. But,
we can add the tag
> >> > > > > >> retroactively if
> >> > > > > >> >> people start having trouble.
> >> > > > > >> >>
> >> > > > > >> >> If there no objections, I will merge the
PR using the
> above
> >> > > method
> >> > > > in
> >> > > > > >> my
> >> > > > > >> >> morning (EST).
> >> > > > > >> >>
> >> > > > > >> >> Thanks everyone! I'm looking forward to
having the Julia
> >> > > community
> >> > > > > >> join the
> >> > > > > >> >> main repo and increasing our collaboration
with them.
> >> > > > > >> >>
> >> > > > > >> >> Best,
> >> > > > > >> >> Carin
> >> > > > > >> >>
> >> > > > > >> >>> On Thu, Sep 27, 2018 at 1:37 PM Chiyuan
Zhang <
> >> > > pluskid@gmail.com>
> >> > > > > >> wrote:
> >> > > > > >> >>>
> >> > > > > >> >>> +1 for rebase and merge. As a workaround
for the
> >> > aforementioned
> >> > > > > issue,
> >> > > > > >> >>> maybe we can create a tag for the
commit before the
> merge,
> >> so
> >> > > that
> >> > > > > in
> >> > > > > >> >> case
> >> > > > > >> >>> people want to browse the recent main-repo
commits by
> >> skipping
> >> > > > this
> >> > > > > >> big
> >> > > > > >> >>> chunk of rebased commits, there is
a pointer to take his
> or
> >> > her
> >> > > > hand
> >> > > > > >> on.
> >> > > > > >> >>>
> >> > > > > >> >>> Best,
> >> > > > > >> >>> Chiyuan
> >> > > > > >> >>>
> >> > > > > >> >>>> On Thu, Sep 27, 2018 at 7:34 AM
Jason Dai <
> >> > jason.dai@gmail.com
> >> > > >
> >> > > > > >> wrote:
> >> > > > > >> >>>>
> >> > > > > >> >>>> +1 to rebase and merge to preserve
and track the
> >> > contributions.
> >> > > > > >> >>>>
> >> > > > > >> >>>> Thanks,
> >> > > > > >> >>>> -Jason
> >> > > > > >> >>>>
> >> > > > > >> >>>> On Thu, Sep 27, 2018 at 12:27
PM Aaron Markham <
> >> > > > > >> >>> aaron.s.markham@gmail.com>
> >> > > > > >> >>>> wrote:
> >> > > > > >> >>>>
> >> > > > > >> >>>>> +1 to rebase and merge to
retain the efforts of all of
> >> the
> >> > > > > >> >>> contributors.
> >> > > > > >> >>>> If
> >> > > > > >> >>>>> there's some git maintenance
that can trim it down from
> >> 700+
> >> > > > > commits
> >> > > > > >> >>> then
> >> > > > > >> >>>>> maybe that's a compromise.
> >> > > > > >> >>>>>
> >> > > > > >> >>>>>> On Wed, Sep 26, 2018,
21:23 Naveen Swamy <
> >> > mnnaveen@gmail.com
> >> > > >
> >> > > > > >> wrote:
> >> > > > > >> >>>>>>
> >> > > > > >> >>>>>> this PR comes from more
than 1 individual, if we
> squash
> >> > merge
> >> > > > > we'll
> >> > > > > >> >>> not
> >> > > > > >> >>>>> be
> >> > > > > >> >>>>>> able to attribute the
contribution of those
> individuals.
> >> > > > > >> >>>>>>
> >> > > > > >> >>>>>> +1 to rebase merge to
preserve history
> >> > > > > >> >>>>>>
> >> > > > > >> >>>>>> On Thu, Sep 27, 2018 at
12:04 AM, Tianqi Chen <
> >> > > > > >> >>>> tqchen@cs.washington.edu>
> >> > > > > >> >>>>>> wrote:
> >> > > > > >> >>>>>>
> >> > > > > >> >>>>>>> One of the main reason
for a rebase merge is that it
> >> > > preserves
> >> > > > > >> >> the
> >> > > > > >> >>>>> commit
> >> > > > > >> >>>>>>> history of the MXNet.jl
package contributors, and
> given
> >> > that
> >> > > > the
> >> > > > > >> >>>>> project
> >> > > > > >> >>>>>>> has been evolved since
2015 and has always been a
> >> > > high-quality
> >> > > > > >> >>>> language
> >> > > > > >> >>>>>>> module for MXNet.
> >> > > > > >> >>>>>>>
> >> > > > > >> >>>>>>> I think we should
take an exception here to preserve
> >> the
> >> > > > commit
> >> > > > > >> >>>> history
> >> > > > > >> >>>>>> of
> >> > > > > >> >>>>>>> each individual contributors
to the Julia binding and
> >> > > welcome
> >> > > > > >> >> them
> >> > > > > >> >>> to
> >> > > > > >> >>>>> the
> >> > > > > >> >>>>>>> community.
> >> > > > > >> >>>>>>>
> >> > > > > >> >>>>>>> Tianqi
> >> > > > > >> >>>>>>>
> >> > > > > >> >>>>>>> On Wed, Sep 26, 2018
at 8:55 PM Tianqi Chen <
> >> > > > > >> >>>> tqchen@cs.washington.edu>
> >> > > > > >> >>>>>>> wrote:
> >> > > > > >> >>>>>>>
> >> > > > > >> >>>>>>>> In this particular
case, I would suggest rebase and
> >> > merge.
> >> > > > > >> >>>>>>>>
> >> > > > > >> >>>>>>>> The main reasoning
is that the commit log of the
> Julia
> >> > > > binding
> >> > > > > >> >> is
> >> > > > > >> >>>> not
> >> > > > > >> >>>>>>>> simple WIP commits,
every commit there has been done
> >> > > through
> >> > > > > >> >>>>> testcases
> >> > > > > >> >>>>>>> and
> >> > > > > >> >>>>>>>> it is important
for us to respect the developer of
> the
> >> > > > effort.
> >> > > > > >> >> It
> >> > > > > >> >>>> is
> >> > > > > >> >>>>>> also
> >> > > > > >> >>>>>>>> good to trace
back the history of the commits more
> >> > easily.
> >> > > > > >> >>>>>>>>
> >> > > > > >> >>>>>>>> Tianqi
> >> > > > > >> >>>>>>>>
> >> > > > > >> >>>>>>>>
> >> > > > > >> >>>>>>>> Tianqi
> >> > > > > >> >>>>>>>>
> >> > > > > >> >>>>>>>> On Wed, Sep 26,
2018 at 5:34 PM Carin Meier <
> >> > > > > >> >>> carinmeier@gmail.com>
> >> > > > > >> >>>>>>> wrote:
> >> > > > > >> >>>>>>>>
> >> > > > > >> >>>>>>>>> Chiyuan,
> >> > > > > >> >>>>>>>>>
> >> > > > > >> >>>>>>>>> Thanks for
the prompt to find some clarity of the
> >> pros
> >> > and
> >> > > > > >> >> cons
> >> > > > > >> >>> of
> >> > > > > >> >>>>>>> each. I
> >> > > > > >> >>>>>>>>> think that
will help drive us to the right
> decision.
> >> I
> >> > > think
> >> > > > > >> >>> some
> >> > > > > >> >>>> of
> >> > > > > >> >>>>>>> those
> >> > > > > >> >>>>>>>>> reasons are
the ones you listed. I will take a stab
> >> > below
> >> > > at
> >> > > > > >> >>>>> outlining
> >> > > > > >> >>>>>>>>> what
> >> > > > > >> >>>>>>>>> I see. Feel
free to chime in if I missed any.
> >> > > > > >> >>>>>>>>>
> >> > > > > >> >>>>>>>>> *Squash and
Merge*
> >> > > > > >> >>>>>>>>>  *Pros* -
It is the project standard
> >> > > > > >> >>>>>>>>>          -
It will provide one commit for the
> feature
> >> > and
> >> > > > > >> >>> lessen
> >> > > > > >> >>>>> the
> >> > > > > >> >>>>>>> need
> >> > > > > >> >>>>>>>>> for 700+ commits
rebased on top of master.
> >> > > > > >> >>>>>>>>>         -
It is easier for a user to do git log to
> >> > browse
> >> > > > > >> >>> commits
> >> > > > > >> >>>>> and
> >> > > > > >> >>>>>>> see
> >> > > > > >> >>>>>>>>> what was features
were added.
> >> > > > > >> >>>>>>>>>  *Cons* -
I don't know how github would handle
> >> squashing
> >> > > all
> >> > > > > >> >>>> those
> >> > > > > >> >>>>>>> commit
> >> > > > > >> >>>>>>>>> messages into
one. Will it be too much?
> >> > > > > >> >>>>>>>>>          
 - You lose the granularity of the
> features
> >> > > > > >> >>> individual
> >> > > > > >> >>>>>>> commits
> >> > > > > >> >>>>>>>>>
> >> > > > > >> >>>>>>>>> *Rebase and
Merge*
> >> > > > > >> >>>>>>>>> * Pros *-
You don't have a huge commit message with
> >> one
> >> > > > > >> >> commit
> >> > > > > >> >>>>>>>>>          -
 You do have the granularity of the
> >> > individual
> >> > > > > >> >>>> features
> >> > > > > >> >>>>> of
> >> > > > > >> >>>>>>> the
> >> > > > > >> >>>>>>>>> commit
> >> > > > > >> >>>>>>>>> * Cons *-
It is not the project standard
> >> > > > > >> >>>>>>>>>          
- You have 700+ commits on top of master
> >> that
> >> > > > might
> >> > > > > >> >>> be
> >> > > > > >> >>>>>> harder
> >> > > > > >> >>>>>>>>> to
> >> > > > > >> >>>>>>>>> see the ones
that went in right before. (like
> someone
> >> > > > browsing
> >> > > > > >> >>>>>> commits)
> >> > > > > >> >>>>>>>>>
> >> > > > > >> >>>>>>>>> On Wed, Sep
26, 2018 at 8:12 PM Chiyuan Zhang <
> >> > > > > >> >>> pluskid@gmail.com>
> >> > > > > >> >>>>>>> wrote:
> >> > > > > >> >>>>>>>>>
> >> > > > > >> >>>>>>>>>> Hi Carin,
> >> > > > > >> >>>>>>>>>>
> >> > > > > >> >>>>>>>>>> Can you
clarify the pros and cons of the two
> >> > approaches?
> >> > > Is
> >> > > > > >> >>> the
> >> > > > > >> >>>>> main
> >> > > > > >> >>>>>>>>>> concern
here about logistics (e.g. preserving the
> >> > history
> >> > > > of
> >> > > > > >> >>> the
> >> > > > > >> >>>>>>>>> original
> >> > > > > >> >>>>>>>>>> repo and
developments) or technical issue (e.g.
> >> using
> >> > > > squash
> >> > > > > >> >>>> might
> >> > > > > >> >>>>>> end
> >> > > > > >> >>>>>>>>> up
> >> > > > > >> >>>>>>>>>> with a
huuuuge commit message that might be
> >> difficult
> >> > or
> >> > > > > >> >> hard
> >> > > > > >> >>> to
> >> > > > > >> >>>>>>>>> handle)?
> >> > > > > >> >>>>>>>>>>
> >> > > > > >> >>>>>>>>>> I think
it might not be very likely that someone
> is
> >> > going
> >> > > > to
> >> > > > > >> >>>>> cherry
> >> > > > > >> >>>>>>> pick
> >> > > > > >> >>>>>>>>>> revert
some of the commits. But preserving the
> >> commit
> >> > > > > >> >> history
> >> > > > > >> >>> is
> >> > > > > >> >>>>>> still
> >> > > > > >> >>>>>>>>>> useful
in case one need to trace the change or
> >> bisect
> >> > for
> >> > > > > >> >> some
> >> > > > > >> >>>>>>>>> regression
> >> > > > > >> >>>>>>>>>> bugs,
etc.
> >> > > > > >> >>>>>>>>>>
> >> > > > > >> >>>>>>>>>> Just to
provide some context: the PR actually
> >> contains
> >> > > 700+
> >> > > > > >> >>>>> commits,
> >> > > > > >> >>>>>>>>> and it
> >> > > > > >> >>>>>>>>>> dates
back to 2015. The development of the Julia
> >> > binding
> >> > > > > >> >>> started
> >> > > > > >> >>>>> in
> >> > > > > >> >>>>>>> the
> >> > > > > >> >>>>>>>>>> early
stage of MXNet. We started with a separate
> >> repo
> >> > due
> >> > > > to
> >> > > > > >> >>> the
> >> > > > > >> >>>>>>>>>> requirement
of the package system of julia.
> >> > > > > >> >>>>>>>>>>
> >> > > > > >> >>>>>>>>>> Best,
> >> > > > > >> >>>>>>>>>> Chiyuan
> >> > > > > >> >>>>>>>>>>
> >> > > > > >> >>>>>>>>>> On Wed,
Sep 26, 2018 at 3:41 PM Carin Meier <
> >> > > > > >> >>>> carinmeier@gmail.com
> >> > > > > >> >>>>>>
> >> > > > > >> >>>>>>>>> wrote:
> >> > > > > >> >>>>>>>>>>
> >> > > > > >> >>>>>>>>>>> The
Import Julia binding PR ,(
> >> > > > > >> >>>>>>>>>>>
> >> https://github.com/apache/incubator-mxnet/pull/10149
> >> > ),
> >> > > is
> >> > > > > >> >>>>> getting
> >> > > > > >> >>>>>>>>> very
> >> > > > > >> >>>>>>>>>>> close
to being merged. Because of the large
> number
> >> of
> >> > > > > >> >>> commits
> >> > > > > >> >>>>>> there
> >> > > > > >> >>>>>>>>> was a
> >> > > > > >> >>>>>>>>>>> suggestion
not to use the usual "Squash and
> Merge".
> >> > The
> >> > > > > >> >>> only
> >> > > > > >> >>>>>> option
> >> > > > > >> >>>>>>>>>> would
> >> > > > > >> >>>>>>>>>>> be
"Rebase and Merge" since merging with a merge
> >> > commit
> >> > > is
> >> > > > > >> >>> not
> >> > > > > >> >>>>>>> enabled
> >> > > > > >> >>>>>>>>>> for
> >> > > > > >> >>>>>>>>>>> the
project.
> >> > > > > >> >>>>>>>>>>>
> >> > > > > >> >>>>>>>>>>> *Squash
and Merge* - The commits from this branch
> >> will
> >> > > be
> >> > > > > >> >>>>> combined
> >> > > > > >> >>>>>>>>> into
> >> > > > > >> >>>>>>>>>> one
> >> > > > > >> >>>>>>>>>>> commit
in the base branch (With all the commit
> >> > messages
> >> > > > > >> >>>>> combined)
> >> > > > > >> >>>>>>>>>>>
> >> > > > > >> >>>>>>>>>>> *Rebase
and Merge* - The commits from this branch
> >> will
> >> > > be
> >> > > > > >> >>>>> rebased
> >> > > > > >> >>>>>>> and
> >> > > > > >> >>>>>>>>>> added
> >> > > > > >> >>>>>>>>>>> to
the base branch
> >> > > > > >> >>>>>>>>>>>
> >> > > > > >> >>>>>>>>>>> The
PR is over 250+ commits (Github won't show
> all
> >> of
> >> > > > > >> >> them)
> >> > > > > >> >>>>>>>>>>>
> >> > > > > >> >>>>>>>>>>> Thoughts
about how we should handle the merge?
> >> > > > > >> >>>>>>>>>>>
> >> > > > > >> >>>>>>>>>>> Thanks,
> >> > > > > >> >>>>>>>>>>> Carin
> >> > > > > >> >>>>>>>>>>>
> >> > > > > >> >>>>>>>>>>
> >> > > > > >> >>>>>>>>>
> >> > > > > >> >>>>>>>>
> >> > > > > >> >>>>>>>
> >> > > > > >> >>>>>>
> >> > > > > >> >>>>>
> >> > > > > >> >>>>
> >> > > > > >> >>>
> >> > > > > >> >>
> >> > > > > >>
> >> > > > > >
> >> > > > >
> >> > > >
> >> > >
> >> >
> >>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message