mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marco de Abreu <marco.g.ab...@googlemail.com.INVALID>
Subject Re: Which merge option to use on the Import Julia binding PR?
Date Sat, 29 Sep 2018 01:51:11 GMT
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