mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Carin Meier <carinme...@gmail.com>
Subject Re: Which merge option to use on the Import Julia binding PR?
Date Fri, 28 Sep 2018 23:16:50 GMT
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