Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout the project

[Chris Olivier]

-0.5 if keeping it as a warning.


On Thu, Oct 4, 2018 at 1:23 AM kellen sunderland <
kellen.sunderl...@gmail.com> wrote:

> I agree that active C++ developers should be the ones making these
> choices.  The downside to that is that many of these people are already
> pretty busy.  To make the best use possible of their time it would probably
> make sense to create a concise doc with proposed style changes and ETAs
> rather than focusing on a single change (modernized range loops).  We've
> got the infrastructure in place now to support any decisions made, and I
> think it's in the best interest of the project as it will (1) unify coding
> style to help readability and (2) make the lifes easier for reviewers which
> are a critical resource on this project.
>
> I've update my PR such that it still modernizes all existing loops, but it
> will now only issue a warning in the case that someone commits an older or
> slower version of a loop.  Of course as we've mentioned a few times this
> only applies to cases where loops can be drop-in replaces with range
> loops.  I.e. no loops indexes are actively used, etc.
>
> Would you be alright with merging this change with warnings instead of
> errors for the time being Chris?
>
> On Wed, Oct 3, 2018 at 7:20 PM Pedro Larroy 
> wrote:
>
> > +1
> >
> > @Chris: do you have data on the performance difference? As far as I know
> > there's a "rewrite rule" like the one between lambdas and C++ functors,
> so
> > performance should be very well defined, but maybe there's something that
> > you are point out that we are missing.
> >
> > Having a consistent and concise code base is beneficial, I think what
> > Kellen is advocating is to use range loops whenever possible, not
> > prescribing its usage on every case, if you have to iterate backwards
> there
> > are other ways such as backwards iterator or others.
> >
> > On Fri, Sep 28, 2018 at 6:54 AM Chris Olivier 
> > wrote:
> >
> > > -1
> > >
> > > Range loops aren’t always the most performant way. In addition,
> sometimes
> > > you want the index. Or maybe you want to iterate backwards, or not
> start
> > > from the first, etc. Maybe you want the iterator because you remove it
> > from
> > > the list at the bottom of the loop Seems like a rule for the sake
> of
> > > having a rule.
> > >
> > > On Fri, Sep 28, 2018 at 2:12 AM kellen sunderland <
> > > kellen.sunderl...@gmail.com> wrote:
> > >
> > > > Hello MXNet devs,
> > > >
> > > > I'd like to discuss uniformly adopting C++11 range loops in the MXNet
> > > > project.  The benefits I see are:
> > > >
> > > > *  Improved C++ readability (examples below).
> > > > *  Consistency with other languages.  The range-loops are quite
> similar
> > > to
> > > > loops almost all other programming languages.  Given we're a project
> > that
> > > > supports many languages this language consistency could be positive
> for
> > > our
> > > > community.
> > > > * Consistency within the same project.  Currently different authors
> > have
> > > > different loops styles which hurts codebase readability.
> > > > *  Best available performance.  There are often multiple ways to
> write
> > > > loops in C++ with subtle differences in performance and memory usage
> > > > between loop methods.  Using range-loops ensures we get the best
> > possible
> > > > perf using an intuitive loop pattern.
> > > > *  Slightly lower chance for bugs / OOB accesses when dealing with
> > > indexing
> > > > in an array for example.
> > > >
> > > > If we decide to enable this uniformly throughout the project we can
> > > enable
> > > > this policy with a simple clang-tidy configuration change.  There
> would
> > > be
> > > > no need for reviewers to have to manually provide feedback when
> someone
> > > > uses an older C++ loops style.
> > > >
> > > > -Kellen
> > > >
> > > > Reference PR:  https://github.com/apache/incubator-mxnet/pull/12356/
> > > > Previous clang-tidy discussion on the list:
> > > >
> > > >
> > >
> >
> https://lists.apache.org/thread.html/b0ae5a9df5dfe0d9074cb2ebe432264db4fa2175b89fa43a5f6e36be@%3Cdev.mxnet.apache.org%3E
> > > >
> > > > -
> > > > Examples:
> > > > for (auto axis_iter = param.axis.begin() ; axis_iter!=
> > param.axis.end();
> > > > ++axis_iter) {
> > > > CHECK_LT(*axis_iter, static_cast(ishape.ndim()));
> > > > stride_[reverse_index] = ishape[*axis_iter];
> > > > ...
> > > > -->
> > > > for (int axis : param.axis) {
> > > > CHECK_LT(axis, static_cast(ishape.ndim()));
> > > > stride_[reverse_index] = ishape[axis];
> > > > ...
> > > > --
> > > > for (size_t i = 0; i < in_array.size(); i++) {
> > > > auto &nd = in_array[i];
> > > > pre_temp_buf_.emplace_back(nd.shape(), nd.ctx(), true,
> nd.dtype());
> > > > }
> > > > -->
> > > > for (auto & nd : in_array) {
> > > > pre_temp_buf_.emplace_back(nd.shape(), nd.ctx(), true,
> nd.dtype());
> > > > }
> > > >
> > >
> >
>

Re: Which merge option to use on the Import Julia binding PR?

[Michael Wall]

Great, glad it worked.  I learned something too.

On Thu, Oct 4, 2018, 22:09 Marco de Abreu
 wrote:

> Oh nice, great catch Michael and Carin! I just learned something new,
> thanks :)
>
> I guess we're all set then, right? Thanks a lot, everyone!
>
> -Marco
>
> Carin Meier  schrieb am Fr., 5. Okt. 2018, 02:47:
>
> > Micheal,
> >
> > Thanks. You were right! I could merge.
> >
> > The PR shows up now as merged
> > https://github.com/apache/incubator-mxnet/pull/10149
> > My merge commit is here
> > https://github.com/apache/incubator-mxnet/commits/master
> >
> > Thanks again for the help.
> >
> > - Carin
> >
> >
> >
> > On Thu, Oct 4, 2018 at 8:09 PM Michael Wall  wrote:
> >
> > > 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 
> 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 
> 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 
> > > wrote:
> > > > >
> > > > > > Thank you Mike!
> > > > > >
> > > > > > On Thu, Oct 4, 2018 at 8:54 AM Michael Wall 
> > > 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 <
> carinme...@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 <
> > carinme...@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
> > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > 

Jira and Github

[Chris Olivier]

Jira has new tight integration with github. I think we should look into
enabling it.

https://blog.github.com/2018-10-04-announcing-the-new-github-and-jira-software-cloud-integration/

Re: Which merge option to use on the Import Julia binding PR?

[Marco de Abreu]

Oh nice, great catch Michael and Carin! I just learned something new,
thanks :)

I guess we're all set then, right? Thanks a lot, everyone!

-Marco

Carin Meier  schrieb am Fr., 5. Okt. 2018, 02:47:

> Micheal,
>
> Thanks. You were right! I could merge.
>
> The PR shows up now as merged
> https://github.com/apache/incubator-mxnet/pull/10149
> My merge commit is here
> https://github.com/apache/incubator-mxnet/commits/master
>
> Thanks again for the help.
>
> - Carin
>
>
>
> On Thu, Oct 4, 2018 at 8:09 PM Michael Wall  wrote:
>
> > 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  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  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 
> > wrote:
> > > >
> > > > > Thank you Mike!
> > > > >
> > > > > On Thu, Oct 4, 2018 at 8:54 AM Michael Wall 
> > 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  >
> > > > 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 <
> carinme...@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 <
> > > plus...@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/
> > >

Re: Maturity Model and Graduation

[Steffen Rochel]

I started a draft assessment -
https://cwiki.apache.org/confluence/display/MXNET/Apache+Maturity+Model+Assessment+for+MXNet
based
on my personal view. Please keep in mind I'm new to the project and Apache
(just attended my first ApacheCon!!). The items I was not sure myself I
marked as "???".

Jim et all - looking for your guidance if the assessment should be
discussed further within PPMC, on private@ and next steps.

Steffen

On Fri, Sep 28, 2018 at 1:53 PM Pedro Larroy 
wrote:

> So Isabel, are you saying that if we publish a clearer TODO list or
> contributions needed material we might get more contribution there?
>
> One thing that I like from other projects is to make a list of low-hanging
> fruit issues or easy contributions that newcomers can pick to get familiar
> with the project, especially in projects like MXNet in which some
> contributions might require significant ramp up time, technical and
> mathematical skills or domain knowledge.
>
> Pedro.
>
> On Fri, Sep 28, 2018 at 3:06 AM Isabel Drost-Fromm 
> wrote:
>
> >
> >
> > On 28/09/18 11:27, kellen sunderland wrote:
> > > I'd love to see some more
> > > sustained contribution from other open source communities to help us
> out
> > in
> > > this area
> >
> > That's not exactly the model I have seen to work. What I have seen works
> > really well at other projects is pulling users in as committers in a
> > scratch your own itch kind of way. For that to work you need to make it
> > clear what contributions you need, you need to make time to coach people
> > to become developers, you need to make your users accustomed to the way
> > you work as early as possible. It also helps to ask users for
> > contributions and offer mentoring help from your side along the way.
> >
> > I know that this is tedious work that needs a lot of motivating people,
> > mentoring people, explaining to people, however it makes for a
> > sustainable community of people that do the work out of self interest.
> >
> >
> >
> http://blog.isabel-drost.de/posts/open-development-and-inner-source-for-fun-and-profit.html
> >
> >
> > Isabel
> >
>

Re: Which merge option to use on the Import Julia binding PR?

[Carin Meier]

Micheal,

Thanks. You were right! I could merge.

The PR shows up now as merged
https://github.com/apache/incubator-mxnet/pull/10149
My merge commit is here
https://github.com/apache/incubator-mxnet/commits/master

Thanks again for the help.

- Carin



On Thu, Oct 4, 2018 at 8:09 PM Michael Wall  wrote:

> 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  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  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 
> wrote:
> > >
> > > > Thank you Mike!
> > > >
> > > > On Thu, Oct 4, 2018 at 8:54 AM Michael Wall 
> 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 
> > > 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  >
> > > > 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 <
> > plus...@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
> > > > > > >>  wrote:
> > > > > > >>
> > > > > > >> > Could you upload the picture somewhere please? HTML is being
> > > > > stripped
> > > > > > >> out
> > > > > > >> > on email lists.
> > > > > > >> >
> > > > > > >> > Chiyuan Zhang  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 scre

Re: MXNet Podling Report - October

[Haibin Lin]

Hi Justin and Michael,

I updated the report with the links to the tutorial summaries:
June -
https://lists.apache.org/thread.html/52f88e9dc7a6a2a1dfa5ad41c469fe2cdd1209a0be2eb345bc2f9a96@%3Cuser.mxnet.apache.org%3E
July -
https://lists.apache.org/thread.html/dea9184350f2fe87ce450722ead28072f763196045f39859190f83f8@%3Cuser.mxnet.apache.org%3E
August - https://discuss.mxnet.io/t/apache-mxnet-digest-august-2018/1863

Justin, the length of the permanent link is longer than 76 characters.
Would this be an issue?

Best,
Haibin



On Thu, Oct 4, 2018 at 1:16 PM Haibin Lin  wrote:

> Hi Justin,
>
> Thanks for the notice. I've reformatted the MXNet section to have at most
> 76 characters per line. Sorry about the last minute update.
>
> Best,
> Haibin
>
> On Thu, Oct 4, 2018 at 12:59 PM Justin Mclean  wrote:
>
>> Hi,
>>
>> I noticed you have edited the report after the due date and have broken
>> the formatting a little after I formatted it. Each line must have a maximum
>> of 76 characters, would you mind fixing your section of the report?
>>
>> Thanks,
>> Justin
>>
>

Re: Which merge option to use on the Import Julia binding PR?

[Michael Wall]

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  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  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  wrote:
> >
> > > Thank you Mike!
> > >
> > > On Thu, Oct 4, 2018 at 8:54 AM Michael Wall  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 
> > 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 
> > > 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 <
> plus...@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
> > > > > >>  wrote:
> > > > > >>
> > > > > >> > Could you upload the picture somewhere please? HTML is being
> > > > stripped
> > > > > >> out
> > > > > >> > on email lists.
> > > > > >> >
> > > > > >> > Chiyuan Zhang  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 <
> 

Re: Which merge option to use on the Import Julia binding PR?

[Marco de Abreu]

You won't be able to push to master. The commit will be declined
automatically, so command line is not an option.

-Marco

Carin Meier  schrieb am Fr., 5. Okt. 2018, 01:29:

> 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  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  wrote:
> >
> > > Thank you Mike!
> > >
> > > On Thu, Oct 4, 2018 at 8:54 AM Michael Wall  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 
> > 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 
> > > 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 <
> plus...@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
> > > > > >>  wrote:
> > > > > >>
> > > > > >> > Could you upload the picture somewhere please? HTML is being
> > > > stripped
> > > > > >> out
> > > > > >> > on email lists.
> > > > > >> >
> > > > > >> > Chiyuan Zhang  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 <
> > > carinme...@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
> > > > > >> > >>  wrote:
> > > > > >> > >>
> > > > > >> > >> > Are we sure that this is due to lacking permissions and
> not
> > > > > >> because of
> > > > > >> > >> some
> > > > > >> > >> > technical limitation? If we are certai

Re: Which merge option to use on the Import Julia binding PR?

[Carin Meier]

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  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  wrote:
>
> > Thank you Mike!
> >
> > On Thu, Oct 4, 2018 at 8:54 AM Michael Wall  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 
> 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 
> > 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 
> > > > 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
> > > > >>  wrote:
> > > > >>
> > > > >> > Could you upload the picture somewhere please? HTML is being
> > > stripped
> > > > >> out
> > > > >> > on email lists.
> > > > >> >
> > > > >> > Chiyuan Zhang  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 <
> > carinme...@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
> > > > >> > >>  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  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 l

Re: Which merge option to use on the Import Julia binding PR?

[Michael Wall]

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  wrote:

> Thank you Mike!
>
> On Thu, Oct 4, 2018 at 8:54 AM Michael Wall  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  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 
> 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
> > > >
> > > >  >
> > > >
> > > >
> > > >
> > > > On Sat, Sep 29, 2018 at 10:02 PM Chiyuan Zhang 
> > > 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
> > > >>  wrote:
> > > >>
> > > >> > Could you upload the picture somewhere please? HTML is being
> > stripped
> > > >> out
> > > >> > on email lists.
> > > >> >
> > > >> > Chiyuan Zhang  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 <
> carinme...@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
> > > >> > >>  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  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?
> > > >> > >>

Re: CUDNN algorithm selection failure

[kellen sunderland]

"I ran a similar test(test_slice_batchnorm) for 5K times and I couldn't
reproduce the issue."

One thing to keep in mind is that the SelectAlgo call will cache results in
a registry that is in static scope.  To repro you'd likely have to create a
new process each time you run the test.  (Apologies if this is already how
you're reproducing).

SelectAlgo call:
https://github.com/apache/incubator-mxnet/blob/403831ace46eab4447794df9411351e439e8983e/src/operator/nn/cudnn/cudnn_convolution-inl.h#L609

Static local / singleton registry pattern here:
https://github.com/apache/incubator-mxnet/blob/024b5a916dd3a39a39031ce5e6565cd7d9d60fe2/src/operator/nn/cudnn/cudnn_algoreg.cc#L37

On Thu, Oct 4, 2018 at 8:58 PM Marco de Abreu
 wrote:

> For GPU, we don't run any tests in parallel.
>
> -Marco
>
> Naveen Swamy  schrieb am Do., 4. Okt. 2018, 19:54:
>
> > Looking at the error raised, you can see that the workspace size(GPU mem
> > size) of 1GB isn't sufficient. I am wondering if it is due to tests
> running
> > in parallel on CI, if this is true(tests running in parallel) is it
> > possible to reduce the parallelism ?
> > Error:
> > "mxnet.base.MXNetError: [05:40:12]
> > src/operator/nn/./cudnn/cudnn_convolution-inl.h:870: Failed to find any
> > forward convolution algorithm.  with workspace size of 1073741824 bytes,
> > please consider reducing batch/model size or increasing the workspace
> size"
> >
> > I ran a similar test(test_slice_batchnorm) for 5K times and I couldn't
> > reproduce the issue. I will look into it further to see if there are
> other
> > alternatives.
> >
> >
> > On Thu, Oct 4, 2018 at 10:48 AM Piyush Ghai 
> wrote:
> >
> > > Another build where test_slice_batchnorm_reshape_batchnorm fails :
> > >
> > >
> >
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12721/7/pipeline
> > > <
> > >
> >
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12721/7/pipeline
> > > >
> > >
> > > —
> > > Piyush
> > >
> > > > On Oct 3, 2018, at 9:32 AM, Pedro Larroy <
> pedro.larroy.li...@gmail.com
> > >
> > > wrote:
> > > >
> > > > Seems is not the only test:
> > > >
> > >
> >
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12726/5/pipeline
> > > >
> > > > test_slice_batchnorm_reshape_batchnorm is also failing and hasn't
> been
> > > > touched for a while. It doesn't look like a problem with the test to
> > me,
> > > > (not a flaky test). Looks to me that should find and address the root
> > > cause
> > > > instead of disabling the test in this case.
> > > >
> > > > Pedro.
> > > >
> > > > On Tue, Oct 2, 2018 at 2:39 AM Marco de Abreu
> > > >  wrote:
> > > >
> > > >> I have created an issue at
> > > >> https://github.com/apache/incubator-mxnet/issues/12715 and a PR to
> > > disable
> > > >> the test at https://github.com/apache/incubator-mxnet/pull/12716.
> > > >>
> > > >> This test is pretty new and was submitted with a number of other
> > > >> problematic (and disabled) tests:
> > > >> https://github.com/apache/incubator-mxnet/issues/11164 It could be
> > > >> possible
> > > >> that the test is simply not stable enough. The PR that introduced
> that
> > > test
> > > >> is https://github.com/apache/incubator-mxnet/pull/10921 - it was
> > merged
> > > >> two
> > > >> days ago.
> > > >>
> > > >> Best regards,
> > > >> Marco
> > > >>
> > > >> On Tue, Oct 2, 2018 at 8:43 AM Pedro Larroy <
> > > pedro.larroy.li...@gmail.com>
> > > >> wrote:
> > > >>
> > > >>> Thanks for checking Lin. If it happens again we will have to dig
> > > deeper.
> > > >> We
> > > >>> have just one executor in GPU so I wonder what could be the root
> > cause
> > > of
> > > >>> this.
> > > >>>
> > > >>> On Mon, Oct 1, 2018 at 10:57 PM Lin Yuan 
> > wrote:
> > > >>>
> > >  I could not reproduce the error on an EC2 g3x8 instance making it
> > hard
> > > >> to
> > >  debug. I also suspect it was due to resource usage limit on ci
> > > >>> Instance.
> > > 
> > >  On Mon, Oct 1, 2018 at 10:40 PM Pedro Larroy <
> > > >>> pedro.larroy.li...@gmail.com
> > > >
> > >  wrote:
> > > 
> > > > It doesn't look like flakiness to me at first sight. I think it
> > might
> > > >>> be
> > > > related to resource usage / allocation / leak in the worst case.
> > > >
> > > > Could be that there was not enough memory GPU memory at the time
> of
> > > >>> test
> > > > execution. But I'm just speculating, hence my original question.
> > > >
> > > > Pedro.
> > > >
> > > > On Mon, Oct 1, 2018 at 8:16 PM Lin Yuan 
> > wrote:
> > > >
> > > >> Hi Pedro,
> > > >>
> > > >> I also got this failure in my PR
> > > >>
> > > >>
> > > >
> > > 
> > > >>>
> > > >>
> > >
> >
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-11742/27/pipeline
> > > >>
> > > >> I was not able to identify the root cause of it from changeli

Re: MXNet Podling Report - October

[Haibin Lin]

Hi Justin,

Thanks for the notice. I've reformatted the MXNet section to have at most
76 characters per line. Sorry about the last minute update.

Best,
Haibin

On Thu, Oct 4, 2018 at 12:59 PM Justin Mclean  wrote:

> Hi,
>
> I noticed you have edited the report after the due date and have broken
> the formatting a little after I formatted it. Each line must have a maximum
> of 76 characters, would you mind fixing your section of the report?
>
> Thanks,
> Justin
>

Re: MXNet Podling Report - October

[Justin Mclean]

Hi,

I noticed you have edited the report after the due date and have broken the 
formatting a little after I formatted it. Each line must have a maximum of 76 
characters, would you mind fixing your section of the report?

Thanks,
Justin

Re: CUDNN algorithm selection failure

[Marco de Abreu]

For GPU, we don't run any tests in parallel.

-Marco

Naveen Swamy  schrieb am Do., 4. Okt. 2018, 19:54:

> Looking at the error raised, you can see that the workspace size(GPU mem
> size) of 1GB isn't sufficient. I am wondering if it is due to tests running
> in parallel on CI, if this is true(tests running in parallel) is it
> possible to reduce the parallelism ?
> Error:
> "mxnet.base.MXNetError: [05:40:12]
> src/operator/nn/./cudnn/cudnn_convolution-inl.h:870: Failed to find any
> forward convolution algorithm.  with workspace size of 1073741824 bytes,
> please consider reducing batch/model size or increasing the workspace size"
>
> I ran a similar test(test_slice_batchnorm) for 5K times and I couldn't
> reproduce the issue. I will look into it further to see if there are other
> alternatives.
>
>
> On Thu, Oct 4, 2018 at 10:48 AM Piyush Ghai  wrote:
>
> > Another build where test_slice_batchnorm_reshape_batchnorm fails :
> >
> >
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12721/7/pipeline
> > <
> >
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12721/7/pipeline
> > >
> >
> > —
> > Piyush
> >
> > > On Oct 3, 2018, at 9:32 AM, Pedro Larroy  >
> > wrote:
> > >
> > > Seems is not the only test:
> > >
> >
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12726/5/pipeline
> > >
> > > test_slice_batchnorm_reshape_batchnorm is also failing and hasn't been
> > > touched for a while. It doesn't look like a problem with the test to
> me,
> > > (not a flaky test). Looks to me that should find and address the root
> > cause
> > > instead of disabling the test in this case.
> > >
> > > Pedro.
> > >
> > > On Tue, Oct 2, 2018 at 2:39 AM Marco de Abreu
> > >  wrote:
> > >
> > >> I have created an issue at
> > >> https://github.com/apache/incubator-mxnet/issues/12715 and a PR to
> > disable
> > >> the test at https://github.com/apache/incubator-mxnet/pull/12716.
> > >>
> > >> This test is pretty new and was submitted with a number of other
> > >> problematic (and disabled) tests:
> > >> https://github.com/apache/incubator-mxnet/issues/11164 It could be
> > >> possible
> > >> that the test is simply not stable enough. The PR that introduced that
> > test
> > >> is https://github.com/apache/incubator-mxnet/pull/10921 - it was
> merged
> > >> two
> > >> days ago.
> > >>
> > >> Best regards,
> > >> Marco
> > >>
> > >> On Tue, Oct 2, 2018 at 8:43 AM Pedro Larroy <
> > pedro.larroy.li...@gmail.com>
> > >> wrote:
> > >>
> > >>> Thanks for checking Lin. If it happens again we will have to dig
> > deeper.
> > >> We
> > >>> have just one executor in GPU so I wonder what could be the root
> cause
> > of
> > >>> this.
> > >>>
> > >>> On Mon, Oct 1, 2018 at 10:57 PM Lin Yuan 
> wrote:
> > >>>
> >  I could not reproduce the error on an EC2 g3x8 instance making it
> hard
> > >> to
> >  debug. I also suspect it was due to resource usage limit on ci
> > >>> Instance.
> > 
> >  On Mon, Oct 1, 2018 at 10:40 PM Pedro Larroy <
> > >>> pedro.larroy.li...@gmail.com
> > >
> >  wrote:
> > 
> > > It doesn't look like flakiness to me at first sight. I think it
> might
> > >>> be
> > > related to resource usage / allocation / leak in the worst case.
> > >
> > > Could be that there was not enough memory GPU memory at the time of
> > >>> test
> > > execution. But I'm just speculating, hence my original question.
> > >
> > > Pedro.
> > >
> > > On Mon, Oct 1, 2018 at 8:16 PM Lin Yuan 
> wrote:
> > >
> > >> Hi Pedro,
> > >>
> > >> I also got this failure in my PR
> > >>
> > >>
> > >
> > 
> > >>>
> > >>
> >
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-11742/27/pipeline
> > >>
> > >> I was not able to identify the root cause of it from changelist.
> > >> Are
> >  you
> > >> suggesting there is some flakiness in the master branch too?
> > >>
> > >> Thanks,
> > >>
> > >> Lin
> > >>
> > >> On Mon, Oct 1, 2018 at 4:55 PM Pedro Larroy <
> > > pedro.larroy.li...@gmail.com>
> > >> wrote:
> > >>
> > >>> Hi
> > >>>
> > >>> I saw this failure on CI:
> > >>>
> > >>>
> > >>
> > >
> > 
> > >>>
> > >>
> >
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/master/1697/pipeline
> > >>>
> > >>> Have you seen other cases where we fail to select the best CUDNN
> > >> algorithm?
> > >>> In which circumstances this could happen, and do you think is a
> > >>> good
> > > idea
> > >>> to have one selected by default as a last resort?
> > >>>
> > >>>
> > >>> Pedro.
> > >>>
> > >>
> > >
> > 
> > >>>
> > >>
> >
> >
>

Re: CUDNN algorithm selection failure

[Naveen Swamy]

Looking at the error raised, you can see that the workspace size(GPU mem
size) of 1GB isn't sufficient. I am wondering if it is due to tests running
in parallel on CI, if this is true(tests running in parallel) is it
possible to reduce the parallelism ?
Error:
"mxnet.base.MXNetError: [05:40:12]
src/operator/nn/./cudnn/cudnn_convolution-inl.h:870: Failed to find any
forward convolution algorithm.  with workspace size of 1073741824 bytes,
please consider reducing batch/model size or increasing the workspace size"

I ran a similar test(test_slice_batchnorm) for 5K times and I couldn't
reproduce the issue. I will look into it further to see if there are other
alternatives.


On Thu, Oct 4, 2018 at 10:48 AM Piyush Ghai  wrote:

> Another build where test_slice_batchnorm_reshape_batchnorm fails :
>
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12721/7/pipeline
> <
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12721/7/pipeline
> >
>
> —
> Piyush
>
> > On Oct 3, 2018, at 9:32 AM, Pedro Larroy 
> wrote:
> >
> > Seems is not the only test:
> >
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12726/5/pipeline
> >
> > test_slice_batchnorm_reshape_batchnorm is also failing and hasn't been
> > touched for a while. It doesn't look like a problem with the test to me,
> > (not a flaky test). Looks to me that should find and address the root
> cause
> > instead of disabling the test in this case.
> >
> > Pedro.
> >
> > On Tue, Oct 2, 2018 at 2:39 AM Marco de Abreu
> >  wrote:
> >
> >> I have created an issue at
> >> https://github.com/apache/incubator-mxnet/issues/12715 and a PR to
> disable
> >> the test at https://github.com/apache/incubator-mxnet/pull/12716.
> >>
> >> This test is pretty new and was submitted with a number of other
> >> problematic (and disabled) tests:
> >> https://github.com/apache/incubator-mxnet/issues/11164 It could be
> >> possible
> >> that the test is simply not stable enough. The PR that introduced that
> test
> >> is https://github.com/apache/incubator-mxnet/pull/10921 - it was merged
> >> two
> >> days ago.
> >>
> >> Best regards,
> >> Marco
> >>
> >> On Tue, Oct 2, 2018 at 8:43 AM Pedro Larroy <
> pedro.larroy.li...@gmail.com>
> >> wrote:
> >>
> >>> Thanks for checking Lin. If it happens again we will have to dig
> deeper.
> >> We
> >>> have just one executor in GPU so I wonder what could be the root cause
> of
> >>> this.
> >>>
> >>> On Mon, Oct 1, 2018 at 10:57 PM Lin Yuan  wrote:
> >>>
>  I could not reproduce the error on an EC2 g3x8 instance making it hard
> >> to
>  debug. I also suspect it was due to resource usage limit on ci
> >>> Instance.
> 
>  On Mon, Oct 1, 2018 at 10:40 PM Pedro Larroy <
> >>> pedro.larroy.li...@gmail.com
> >
>  wrote:
> 
> > It doesn't look like flakiness to me at first sight. I think it might
> >>> be
> > related to resource usage / allocation / leak in the worst case.
> >
> > Could be that there was not enough memory GPU memory at the time of
> >>> test
> > execution. But I'm just speculating, hence my original question.
> >
> > Pedro.
> >
> > On Mon, Oct 1, 2018 at 8:16 PM Lin Yuan  wrote:
> >
> >> Hi Pedro,
> >>
> >> I also got this failure in my PR
> >>
> >>
> >
> 
> >>>
> >>
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-11742/27/pipeline
> >>
> >> I was not able to identify the root cause of it from changelist.
> >> Are
>  you
> >> suggesting there is some flakiness in the master branch too?
> >>
> >> Thanks,
> >>
> >> Lin
> >>
> >> On Mon, Oct 1, 2018 at 4:55 PM Pedro Larroy <
> > pedro.larroy.li...@gmail.com>
> >> wrote:
> >>
> >>> Hi
> >>>
> >>> I saw this failure on CI:
> >>>
> >>>
> >>
> >
> 
> >>>
> >>
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/master/1697/pipeline
> >>>
> >>> Have you seen other cases where we fail to select the best CUDNN
> >> algorithm?
> >>> In which circumstances this could happen, and do you think is a
> >>> good
> > idea
> >>> to have one selected by default as a last resort?
> >>>
> >>>
> >>> Pedro.
> >>>
> >>
> >
> 
> >>>
> >>
>
>

Re: CUDNN algorithm selection failure

[Piyush Ghai]

Another build where test_slice_batchnorm_reshape_batchnorm fails : 
http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12721/7/pipeline
 


—
Piyush 

> On Oct 3, 2018, at 9:32 AM, Pedro Larroy  wrote:
> 
> Seems is not the only test:
> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-12726/5/pipeline
> 
> test_slice_batchnorm_reshape_batchnorm is also failing and hasn't been
> touched for a while. It doesn't look like a problem with the test to me,
> (not a flaky test). Looks to me that should find and address the root cause
> instead of disabling the test in this case.
> 
> Pedro.
> 
> On Tue, Oct 2, 2018 at 2:39 AM Marco de Abreu
>  wrote:
> 
>> I have created an issue at
>> https://github.com/apache/incubator-mxnet/issues/12715 and a PR to disable
>> the test at https://github.com/apache/incubator-mxnet/pull/12716.
>> 
>> This test is pretty new and was submitted with a number of other
>> problematic (and disabled) tests:
>> https://github.com/apache/incubator-mxnet/issues/11164 It could be
>> possible
>> that the test is simply not stable enough. The PR that introduced that test
>> is https://github.com/apache/incubator-mxnet/pull/10921 - it was merged
>> two
>> days ago.
>> 
>> Best regards,
>> Marco
>> 
>> On Tue, Oct 2, 2018 at 8:43 AM Pedro Larroy 
>> wrote:
>> 
>>> Thanks for checking Lin. If it happens again we will have to dig deeper.
>> We
>>> have just one executor in GPU so I wonder what could be the root cause of
>>> this.
>>> 
>>> On Mon, Oct 1, 2018 at 10:57 PM Lin Yuan  wrote:
>>> 
 I could not reproduce the error on an EC2 g3x8 instance making it hard
>> to
 debug. I also suspect it was due to resource usage limit on ci
>>> Instance.
 
 On Mon, Oct 1, 2018 at 10:40 PM Pedro Larroy <
>>> pedro.larroy.li...@gmail.com
> 
 wrote:
 
> It doesn't look like flakiness to me at first sight. I think it might
>>> be
> related to resource usage / allocation / leak in the worst case.
> 
> Could be that there was not enough memory GPU memory at the time of
>>> test
> execution. But I'm just speculating, hence my original question.
> 
> Pedro.
> 
> On Mon, Oct 1, 2018 at 8:16 PM Lin Yuan  wrote:
> 
>> Hi Pedro,
>> 
>> I also got this failure in my PR
>> 
>> 
> 
 
>>> 
>> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-11742/27/pipeline
>> 
>> I was not able to identify the root cause of it from changelist.
>> Are
 you
>> suggesting there is some flakiness in the master branch too?
>> 
>> Thanks,
>> 
>> Lin
>> 
>> On Mon, Oct 1, 2018 at 4:55 PM Pedro Larroy <
> pedro.larroy.li...@gmail.com>
>> wrote:
>> 
>>> Hi
>>> 
>>> I saw this failure on CI:
>>> 
>>> 
>> 
> 
 
>>> 
>> http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/master/1697/pipeline
>>> 
>>> Have you seen other cases where we fail to select the best CUDNN
>> algorithm?
>>> In which circumstances this could happen, and do you think is a
>>> good
> idea
>>> to have one selected by default as a last resort?
>>> 
>>> 
>>> Pedro.
>>> 
>> 
> 
 
>>> 
>> 

Re: MXNet Podling Report - October

[Haibin Lin]

Hi Michael,

Thanks for reviewing the report! I've so far updated the report based on
question 1,2, 4 and 5.

1. None.
2. Links:

https://medium.com/apache-mxnet

https://twitter.com/ApacheMXNet

https://www.youtube.com/apachemxnet

https://www.slideshare.net/apachemxnet

https://www.reddit.com/r/mxnet/
3. I'll send out the link to the collection of tutorials shortly.
4. The numbers have been captured from
https://github.com/apache/incubator-mxnet/pulse/monthly at the end of each
months.
Today, covering 9/4-10/4 the summary shows:
Excluding merges, 48 authors have pushed 126commits to master and 134
commits to all branches. On master, 429 files have changed and there have
been 13,458 additions and 5,975 deletions.
Unfortunately, Github doesn’t provide an API to capture information for
specific time frames. If there is a preferred or standard way in Apache to
capture the changes, we would like to learn more about it..
As in previous reports we only captured changes from the incubator-mxnet
repo. incubator-mxnet-site is generated through CI from the incubator-mxnet
repo and contains static content for http://mxnet.incubator.apache.org/

5 - I added the statement from the top of the report: MXNext had several
new mentors added to help with diversity among it's mentors.

Best,
Haibin

On Tue, Oct 2, 2018 at 6:44 PM Michael Wall  wrote:

> Hi Haibin,
>
> A couple of things I thought of when reviewing the report
> 1 - No answer for the question "Any issues that the Incubator PMC (IPMC) or
> ASF Board wish/need to be aware of?"
> 2 - What are the links to the medium blog, the youtube channel and the
> twitter account?
> 3 - Where did the 62 tutorials come from?  Mostly my own interest to see
> them.
> 4 - How were the github stats calculated?  For Sep 2018 issues created, the
> report read 87 but I come up with 112 (the sum of open and closed  from
>
> https://github.com/apache/incubator-mxnet/issues?utf8=%E2%9C%93&q=is%3Aissue+created%3A2018-09-01..2018-09-30+
> ).
> For Sep 2018 issues closed the report read 124 where I get 110 (
>
> https://github.com/apache/incubator-mxnet/issues?utf8=%E2%9C%93&q=is%3Aissue+closed%3A2018-09-01..2018-09-30+
> ).
> But that is only for the incubator-mxnet repo, it doesn't include
> incubator-mxnet-site or incubator-mxnet-test.  The links could be included
> in the report as well which might help to generate the numbers next time.
> 5 - You mention new mentors were added, but not why.  If the board asks, it
> will be a follow up task.
>
> Mike
>
> On Mon, Oct 1, 2018 at 8:33 PM Haibin Lin 
> wrote:
>
> > Hi MXNet community,
> >
> > The podling report for MXNet is due on October 3rd. The report covers
> > MXNet's progress on community development and project development (the
> > previous one can be found here <
> https://wiki.apache.org/incubator/July2018
> > >).
> > You can search "MXNet" at https://wiki.apache.org/incubator/October2018
> > for
> > MXNet's draft report for October. Please help review and contribute to
> the
> > report before it's due.
> >
> > If you have any suggestions on improving the report, please let me know
> and
> > I'm happy to update the report based on the feedback. Thanks!
> >
> > Best regards,
> > Haibin
> >
>

Re: Which merge option to use on the Import Julia binding PR?

[Carin Meier]

Thank you Mike!

On Thu, Oct 4, 2018 at 8:54 AM Michael Wall  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  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  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
> > >
> > > 
> > >
> > >
> > >
> > > On Sat, Sep 29, 2018 at 10:02 PM Chiyuan Zhang 
> > 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
> > >>  wrote:
> > >>
> > >> > Could you upload the picture somewhere please? HTML is being
> stripped
> > >> out
> > >> > on email lists.
> > >> >
> > >> > Chiyuan Zhang  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  >
> > >> > 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
> > >> > >>  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  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 <
> > plus...@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
> > 

Re: Which merge option to use on the Import Julia binding PR?

[Michael Wall]

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  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  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
> >
> > 
> >
> >
> >
> > On Sat, Sep 29, 2018 at 10:02 PM Chiyuan Zhang 
> 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
> >>  wrote:
> >>
> >> > Could you upload the picture somewhere please? HTML is being stripped
> >> out
> >> > on email lists.
> >> >
> >> > Chiyuan Zhang  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 
> >> > 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
> >> > >>  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  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 <
> plus...@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, be

Re: Which merge option to use on the Import Julia binding PR?

[Carin Meier]

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  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
>
> 
>
>
>
> On Sat, Sep 29, 2018 at 10:02 PM Chiyuan Zhang  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
>>  wrote:
>>
>> > Could you upload the picture somewhere please? HTML is being stripped
>> out
>> > on email lists.
>> >
>> > Chiyuan Zhang  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 
>> > 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
>> > >>  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  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 > >
>> > >> 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
>> > >> > > >
>> > >> > > > O

Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout the project

[kellen sunderland]

I agree that active C++ developers should be the ones making these
choices.  The downside to that is that many of these people are already
pretty busy.  To make the best use possible of their time it would probably
make sense to create a concise doc with proposed style changes and ETAs
rather than focusing on a single change (modernized range loops).  We've
got the infrastructure in place now to support any decisions made, and I
think it's in the best interest of the project as it will (1) unify coding
style to help readability and (2) make the lifes easier for reviewers which
are a critical resource on this project.

I've update my PR such that it still modernizes all existing loops, but it
will now only issue a warning in the case that someone commits an older or
slower version of a loop.  Of course as we've mentioned a few times this
only applies to cases where loops can be drop-in replaces with range
loops.  I.e. no loops indexes are actively used, etc.

Would you be alright with merging this change with warnings instead of
errors for the time being Chris?

On Wed, Oct 3, 2018 at 7:20 PM Pedro Larroy 
wrote:

> +1
>
> @Chris: do you have data on the performance difference? As far as I know
> there's a "rewrite rule" like the one between lambdas and C++ functors, so
> performance should be very well defined, but maybe there's something that
> you are point out that we are missing.
>
> Having a consistent and concise code base is beneficial, I think what
> Kellen is advocating is to use range loops whenever possible, not
> prescribing its usage on every case, if you have to iterate backwards there
> are other ways such as backwards iterator or others.
>
> On Fri, Sep 28, 2018 at 6:54 AM Chris Olivier 
> wrote:
>
> > -1
> >
> > Range loops aren’t always the most performant way. In addition, sometimes
> > you want the index. Or maybe you want to iterate backwards, or not start
> > from the first, etc. Maybe you want the iterator because you remove it
> from
> > the list at the bottom of the loop Seems like a rule for the sake of
> > having a rule.
> >
> > On Fri, Sep 28, 2018 at 2:12 AM kellen sunderland <
> > kellen.sunderl...@gmail.com> wrote:
> >
> > > Hello MXNet devs,
> > >
> > > I'd like to discuss uniformly adopting C++11 range loops in the MXNet
> > > project.  The benefits I see are:
> > >
> > > *  Improved C++ readability (examples below).
> > > *  Consistency with other languages.  The range-loops are quite similar
> > to
> > > loops almost all other programming languages.  Given we're a project
> that
> > > supports many languages this language consistency could be positive for
> > our
> > > community.
> > > * Consistency within the same project.  Currently different authors
> have
> > > different loops styles which hurts codebase readability.
> > > *  Best available performance.  There are often multiple ways to write
> > > loops in C++ with subtle differences in performance and memory usage
> > > between loop methods.  Using range-loops ensures we get the best
> possible
> > > perf using an intuitive loop pattern.
> > > *  Slightly lower chance for bugs / OOB accesses when dealing with
> > indexing
> > > in an array for example.
> > >
> > > If we decide to enable this uniformly throughout the project we can
> > enable
> > > this policy with a simple clang-tidy configuration change.  There would
> > be
> > > no need for reviewers to have to manually provide feedback when someone
> > > uses an older C++ loops style.
> > >
> > > -Kellen
> > >
> > > Reference PR:  https://github.com/apache/incubator-mxnet/pull/12356/
> > > Previous clang-tidy discussion on the list:
> > >
> > >
> >
> https://lists.apache.org/thread.html/b0ae5a9df5dfe0d9074cb2ebe432264db4fa2175b89fa43a5f6e36be@%3Cdev.mxnet.apache.org%3E
> > >
> > > -
> > > Examples:
> > > for (auto axis_iter = param.axis.begin() ; axis_iter!=
> param.axis.end();
> > > ++axis_iter) {
> > > CHECK_LT(*axis_iter, static_cast(ishape.ndim()));
> > > stride_[reverse_index] = ishape[*axis_iter];
> > > ...
> > > -->
> > > for (int axis : param.axis) {
> > > CHECK_LT(axis, static_cast(ishape.ndim()));
> > > stride_[reverse_index] = ishape[axis];
> > > ...
> > > --
> > > for (size_t i = 0; i < in_array.size(); i++) {
> > > auto &nd = in_array[i];
> > > pre_temp_buf_.emplace_back(nd.shape(), nd.ctx(), true, nd.dtype());
> > > }
> > > -->
> > > for (auto & nd : in_array) {
> > > pre_temp_buf_.emplace_back(nd.shape(), nd.ctx(), true, nd.dtype());
> > > }
> > >
> >
>