mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Wall <mjw...@gmail.com>
Subject Re: Which merge option to use on the Import Julia binding PR?
Date Fri, 05 Oct 2018 00:08:59 GMT
I would try the merge locally and then inspect the result closely to make
sure it looks like what you want.  If it looks good, you could try pushing
to master.  If you can't push, then we know but I "think" protected just
means you can't force push in this case based on
https://issues.apache.org/jira/browse/INFRA-15233 which links to
https://home.apache.org/~pono/mxnet.png.  Maybe I have only tried that with
repo that own though.

I did find at least one ticket where a team asked for merge commits to be
enabled, https://issues.apache.org/jira/browse/INFRA-16690.  But I think
they intend for it to stay that way.  Is that what the community would want
for the MXNet repo?  Or would you want to enable it for this and disable it
again?


On Thu, Oct 4, 2018 at 7:29 PM Carin Meier <carinmeier@gmail.com> wrote:

> Micheal,
>
> Thanks for catching up and helping us with this.
> I do see the "view command line instructions". I just assumed that master
> was a protected branch and I would not be able to push to it.
> Honestly, I'm a bit scared if it isn't :)
>
> What do you suggest? Should I try to merge and push to master?
>
> On Thu, Oct 4, 2018 at 7:19 PM Michael Wall <mjwall@gmail.com> wrote:
>
> > Just now looking at this.  The button is disabled for merge commit as you
> > have mentioned.  Before I go to INFRA, is the command line an option?  Do
> > you see "or view command line instructions" beside the green squash and
> > merge button?
> >
> > On Thu, Oct 4, 2018 at 9:09 AM Carin Meier <carinmeier@gmail.com> wrote:
> >
> > > Thank you Mike!
> > >
> > > On Thu, Oct 4, 2018 at 8:54 AM Michael Wall <mjwall@apache.org> wrote:
> > >
> > > > Hi Carin,
> > > >
> > > > I will take a look at this tonight.  I am not tracking everything,
> so I
> > > > want to go back and make sure I understand what is being asked.
> Then I
> > > am
> > > > happy to submit an INFRA ticket.
> > > >
> > > > Mike
> > > >
> > > > On Thu, Oct 4, 2018 at 8:36 AM Carin Meier <carinmeier@gmail.com>
> > wrote:
> > > >
> > > > > I just found out that since we are a podling, we should route all
> our
> > > > Infra
> > > > > tickets through one of our mentors and link the dev list discussion
> > in
> > > > > JIRA.
> > > > >
> > > > > Is there a mentor that is willing to help us navigate this process
> to
> > > get
> > > > > the PR merged?
> > > > >
> > > > > Thanks,
> > > > > Carin
> > > > >
> > > > > On Tue, Oct 2, 2018 at 8:42 AM Carin Meier <carinmeier@gmail.com>
> > > wrote:
> > > > >
> > > > > > Marco - Thanks for the "dry run" idea. It will give everyone a
> > clear
> > > > idea
> > > > > > of the process and what the expected results will look like.
> > > > > >
> > > > > > - I took my fork of the repo and synced my master branch.
> > > > > > - @iblis17 made a copy of the branch of the Julia import PR and
> > > > submitted
> > > > > > it to my repo
> > > > > > - I merged it with the "Merge" option through the web interface.
> > > > > >
> > > > > > Here is a gif of the process of merging:
> > > > > > http://g.recordit.co/DzBcFtnjmV.gif
> > > > > > Here is the result of the repo:
> > > > > > https://github.com/gigasquid/incubator-mxnet
> > > > > >
> > > > > > Please everyone take a look and validate that this looks ok.
> > > > > >
> > > > > > If there are no objections, Marco - could you please take the
> lead
> > in
> > > > > > requesting the actions from INFRA?
> > > > > >
> > > > > > It will be great to *finally* get this PR in  :)
> > > > > >
> > > > > > Thanks,
> > > > > > Carin
> > > > > >
> > > > > > <
> > https://github.com/gigasquid/incubator-mxnet/commits?author=iblis17
> > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Sat, Sep 29, 2018 at 10:02 PM Chiyuan Zhang <
> pluskid@gmail.com>
> > > > > wrote:
> > > > > >
> > > > > >> 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