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 Sun, 30 Sep 2018 01:47:58 GMT
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