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 Fri, 28 Sep 2018 22:05:12 GMT
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