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:53:47 GMT
We can also request Infra directly to execute an override action on our
behalf - that way, they don't have to change the configuration and it
creates less work for them. That's something they did for us back then when
we switched CI. If we are all aligned, it shouldn't be a big deal for them
to just execute it.

Could we maybe have a dry run on a test repository to show everyone what
the outcome would look like?

Best regards,
Marco

Naveen Swamy <mnnaveen@gmail.com> schrieb am So., 30. Sep. 2018, 03:49:

> yes, AFAIK I think Apache Infra has disabled the merge option.
>
> If there is a valid reason(and this is one), we could ask our Mentors to
> help us create a INFRA ticket to temporarily enable this option and once we
> are done merging we can request to disable it again.
>
> On Sat, Sep 29, 2018 at 9:44 PM Chiyuan Zhang <pluskid@gmail.com> wrote:
>
> > 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