[BUILD FAILED] Branch master build 697

2017-11-29 Thread Apache Jenkins Server 
Build for MXNet branch master has broken. Please view the build at 
https://builds.apache.org/job/incubator-mxnet/job/master/697/

Re: [VOTE] Apache MXNet (incubating) 1.0.0 release RC0

2017-11-29 Thread Meghna Baijal 
Hello Justin, Henri,
Thank you for your input.

Justin,
Chris ran Apache Rat on MXNet this morning. Several issues found here, in
addition to your comments that needed a fix have been addressed in the
following 2 PRS -
1. https://github.com/apache/incubator-mxnet/pull/8873/
2. https://github.com/apache/incubator-mxnet/pull/8876/

It would be helpful if you could review these changes.

Thanks,
Meghna Baijal



On Wed, Nov 29, 2017 at 3:18 PM, Justin Mclean  wrote:

> Hi,
>
> >> - A number of source file are missing license headers e.g. [15][16] [18]
> >> [19] and many others
> >>
> >
> > Many of these are not Apache MXNet files but from dependencies. I'll
> > suggest on dev@ that these submodules be moved into a third-party/
> > directory.
>
> Having that clearly identified would certainly make the release a lot
> easier to review.
>
> > Why would it be? We only have to include the LICENSE from TVM, we don't
> > need to name them.
>
> In general all bundled software need to be added. [1]
>
> > If TVM want to be identified, they should add a NOTICE file.
>
> Licenses of permissively bundled software go in LICENSE with a few
> exceptions. [2] Apache licensed (v2) doesn't have to me listed [3] but is
> useful to list and you're listing other Apache licensed software in LICENSE
> so it seemed odd to omit it.
>
> Again I suggest you run rat over the release and see if you can fix up
> what it finds. An annotated rat exclusion file would also be a lot of help.
> Just try not to make the exclusions too wide as you may miss something.
>
> Thanks,
> Justin
>
> 1. http://www.apache.org/dev/licensing-howto.html#guiding-principle
> 2. http://www.apache.org/dev/licensing-howto.html#permissive-deps
> 3. http://www.apache.org/dev/licensing-howto.html#alv2-dep

FYI: [VOTE] Apache MXNet (incubating) 1.0.0 release RC0

2017-11-29 Thread Hen 
Sorry, I should have cc'd dev@ on this.

-- Forwarded message --
From: Hen 
Date: Wed, Nov 29, 2017 at 1:59 PM
Subject: Re: [VOTE] Apache MXNet (incubating) 1.0.0 release RC0
To: gene...@incubator.apache.org


Thanks Justin.

Some comments inline on ones I don't think need fixing; but afaict from
MXNet dev@ activity the plan is to produce a new release and restart the
vote.

On Tue, Nov 28, 2017 at 3:54 PM, Justin Mclean 
wrote:

> Hi,
>
> -1 binding due to license, header issues and having a compiled jar in a
> source release.
>
> I checked:
> - incubating in name
> - signatures and hashes correct
> - DISCLAIMER exists
> - LICENSE has issues (see below) I also note that license issues brought
> up last time have not all been addressed. [22]
> - NOTICE seem rather brief considering the number of Apache licensed
> inclusion do any of them have NOTICE files?
>

We can double check, but I know the ones I've looked at didn't use NOTICE
files (the various dmlc/ projects). Plus as these are GitHub submodules, if
they did the NOTICE would automatically come over.


> - A number of source file are missing license headers e.g. [15][16] [18]
> [19] and many others
>

Many of these are not Apache MXNet files but from dependencies. I'll
suggest on dev@ that these submodules be moved into a third-party/
directory. Given the impact of that to builds etc, I'm hoping that can go
in a subsequent release.



> - A number of source look to have had the ASF header incorrectly added.
> - Binary included in source release [20] Note there’s an unresolved legal
> issue about this [21]
>
> Have you run rat on this release it would of help pick up most of these
> issues?
>
> In this file [1] there’s a copyright notice but it also has an ASF header
> which is a little odd. This also occurs in a number of other places.
>

This came up in the previous release (but the opposite way).

When the code came in from MXNet, it had "Copyright Contributors". We
removed that because 'what a useless statement' :), however as there are
400+ contributors and not everyone has signed a CLA, the previous release
vote asked the project to put it back in again, so they did. It's an
eyesore, but we're a bit stuck unless we're prepared to move away from our
rules of "Never remove or move a Copyright statement without approval" and
"Never edit a copyright statement".

We could (I guess) put a comment above it of "# Original MXNet copyright
statement" or some such.


>
> This file [2] also look to incorrectly have an ASF header and it’s unclear
> how the original code is licensed. From a quick like their seems to be many
> files that incorrectly have ASF headers on them e.g. [5][6][7]
> [10][12][13][14] and others.
>
> This file [3] (and others) looks to come from the TVM project which is not
> mentioned in license.
>

Why would it be? We only have to include the LICENSE from TVM, we don't
need to name them.  If TVM want to be identified, they should add a NOTICE
file.


>
> The license for this file [4] is missing from license.
>
> The link for JQuery [8] is missing from the license. Also missing license
> for these files [9][11][17] and probably others.
>
> At this point I gave up so there may be other issues.
>

Understood. A lot of these are systematic characteristics of the project
rather than flat out issues, but definitely some good finds here too.


>
> It also a good idea to publish your keys:
> gpg: assuming signed data in 'apache-mxnet-src-1.0.0.rc0-in
> cubating.tar.gz'
> gpg: Signature made Sat 25 Nov 07:48:02 2017 AEDT
> gpg:using RSA key 80FD81D7703DF31B
> gpg: requesting key 80FD81D7703DF31B from hkps server
> hkps.pool.sks-keyservers.net
> gpg: Can't check signature: No public key
>
> It’s also a good idea to sign with an apache email address rather than a
> gmail one.
>
> I’m also curious about “CODEOWNERS” file as that doesn’t seem to fit with
> any Apache model I’m aware of.
>

This is a GitHub feature. The name is unfortunate and perhaps we could add
a comment explaining what it is. It allows for automatic addition of
reviewers to a pull request. Kind of like a watch on the code.

The important thing is that a project should never tell a committer they
can't put their name in as a codeowner nee automatic-reviewer.


>
> In “CONTRIBUTORS” there’s a long list of contributors - are their plan to
> make any of these people committers?
>
> Thanks,
> Justin
>
> 1. ./apache-mxnet-src-1.0.0.rc0-incubating/perl-package/AI-MXNe
> t/lib/AI/MXNet.pm
> 2. ./apache-mxnet-src-1.0.0.rc0-incubating/example/image-classi
> fication/predict-cpp/image-classification-predict.cc
> 3. ./apache-mxnet-src-1.0.0.rc0-incubating/nnvm/tvm/src/op/op_util.cc
> 4. ./apache-mxnet-src-1.0.0.rc0-incubating/docs/_static/searcht
> ools_custom.js
> 5. ./apache-mxnet-src-1.0.0.rc0-incubating/src/operator/nn/pool.h
> 6. ./apache-mxnet-src-1.0.0.rc0-incubating/src/operator/contrib
> /nn/deformable_im2col.h
> 7. ./apache-mxnet-src-1.0.0.rc0-incub

Re: CODEOWNERS file being removed

2017-11-29 Thread Chris Olivier 
ok

On Wed, Nov 29, 2017 at 1:42 PM, Tianqi Chen 
wrote:

> Codeowner feature is used to automatically trigger the review request for
> the people. All the committers are already listed as global owners, which
> is consistent with Apache's policy
>
> Tianqi
>
> On Wed, Nov 29, 2017 at 1:37 PM, Hen  wrote:
>
> > Was there more discussion than Justin's question about it on general@?
> >
> > My memory of CODEOWNERS was that it was related to some code review tool,
> > but looking at the history of dev@ I only see:
> >
> > "Can't have changes merged into it until changes to files that have a
> > designated code owner  articles/about-codeowners/>
> > have been approved by that owner"
> >
> > Reading GitHub:
> >
> > "Code owners are automatically requested for review when someone opens a
> > pull request that modifies code that they own. When someone with admin or
> > owner permissions has enabled required reviews
> >  > reviews-for-pull-requests/>,
> > they also can optionally require approval from a code owner before the
> > author can merge a pull request in the repository."
> >
> > I think that having a notion where anyone can put their name down as an
> > automatic reviewer is a good one. Apart from the unfortunate name of the
> > file, this should fit with Apache's culture (as long as more than one
> > committer can, and as long as we never say a committer can't decide to
> put
> > themselves as a codeowner). A comment in the file explaining this would
> be
> > good.
> >
> > We should however, never enable the code-owner option of required
> reviews.
> > As that's an Admin/Owner feature, and I don't think Apache Infra would do
> > that, I think we're good there.
> >
> > So my suggestion would be to add a comment that this is an
> > automatically-listed-on-reviews tool.
> >
> > Hen
> >
> >
> > On Wed, Nov 29, 2017 at 12:35 PM, Chris Olivier 
> > wrote:
> >
> > > Per suggestion from Apache, we are removing CODEOWNERS file from root
> of
> > > mxnet.
> > > If there are any objections, please voice them:
> > >
> > > Here are the contents of rht file:
> > >
> > > # Owners of Apache MXNet
> > >
> > > # Global owners
> > > *@apache/mxnet-committers
> > >
> > > # Owners of language bindings
> > > R-package/*   @thirdwing
> > > scala-package/*   @javelinjs
> > > perl-package/*@sergeykolychev
> > >
> > > # CMake owners
> > > CMakeLists.txt@cjolivier01
> > > cmake/*  @cjolivier01
> > >
> >
>

Re: CODEOWNERS file being removed

2017-11-29 Thread Chris Olivier 
If you can push back on Justin, I can keep it.  Just not looking forward to
going through all of this again just to have it bounced back.

On Wed, Nov 29, 2017 at 1:37 PM, Hen  wrote:

> Was there more discussion than Justin's question about it on general@?
>
> My memory of CODEOWNERS was that it was related to some code review tool,
> but looking at the history of dev@ I only see:
>
> "Can't have changes merged into it until changes to files that have a
> designated code owner 
> have been approved by that owner"
>
> Reading GitHub:
>
> "Code owners are automatically requested for review when someone opens a
> pull request that modifies code that they own. When someone with admin or
> owner permissions has enabled required reviews
>  reviews-for-pull-requests/>,
> they also can optionally require approval from a code owner before the
> author can merge a pull request in the repository."
>
> I think that having a notion where anyone can put their name down as an
> automatic reviewer is a good one. Apart from the unfortunate name of the
> file, this should fit with Apache's culture (as long as more than one
> committer can, and as long as we never say a committer can't decide to put
> themselves as a codeowner). A comment in the file explaining this would be
> good.
>
> We should however, never enable the code-owner option of required reviews.
> As that's an Admin/Owner feature, and I don't think Apache Infra would do
> that, I think we're good there.
>
> So my suggestion would be to add a comment that this is an
> automatically-listed-on-reviews tool.
>
> Hen
>
>
> On Wed, Nov 29, 2017 at 12:35 PM, Chris Olivier 
> wrote:
>
> > Per suggestion from Apache, we are removing CODEOWNERS file from root of
> > mxnet.
> > If there are any objections, please voice them:
> >
> > Here are the contents of rht file:
> >
> > # Owners of Apache MXNet
> >
> > # Global owners
> > *@apache/mxnet-committers
> >
> > # Owners of language bindings
> > R-package/*   @thirdwing
> > scala-package/*   @javelinjs
> > perl-package/*@sergeykolychev
> >
> > # CMake owners
> > CMakeLists.txt@cjolivier01
> > cmake/*  @cjolivier01
> >
>

Re: CODEOWNERS file being removed

2017-11-29 Thread Tianqi Chen 
Codeowner feature is used to automatically trigger the review request for
the people. All the committers are already listed as global owners, which
is consistent with Apache's policy

Tianqi

On Wed, Nov 29, 2017 at 1:37 PM, Hen  wrote:

> Was there more discussion than Justin's question about it on general@?
>
> My memory of CODEOWNERS was that it was related to some code review tool,
> but looking at the history of dev@ I only see:
>
> "Can't have changes merged into it until changes to files that have a
> designated code owner 
> have been approved by that owner"
>
> Reading GitHub:
>
> "Code owners are automatically requested for review when someone opens a
> pull request that modifies code that they own. When someone with admin or
> owner permissions has enabled required reviews
>  reviews-for-pull-requests/>,
> they also can optionally require approval from a code owner before the
> author can merge a pull request in the repository."
>
> I think that having a notion where anyone can put their name down as an
> automatic reviewer is a good one. Apart from the unfortunate name of the
> file, this should fit with Apache's culture (as long as more than one
> committer can, and as long as we never say a committer can't decide to put
> themselves as a codeowner). A comment in the file explaining this would be
> good.
>
> We should however, never enable the code-owner option of required reviews.
> As that's an Admin/Owner feature, and I don't think Apache Infra would do
> that, I think we're good there.
>
> So my suggestion would be to add a comment that this is an
> automatically-listed-on-reviews tool.
>
> Hen
>
>
> On Wed, Nov 29, 2017 at 12:35 PM, Chris Olivier 
> wrote:
>
> > Per suggestion from Apache, we are removing CODEOWNERS file from root of
> > mxnet.
> > If there are any objections, please voice them:
> >
> > Here are the contents of rht file:
> >
> > # Owners of Apache MXNet
> >
> > # Global owners
> > *@apache/mxnet-committers
> >
> > # Owners of language bindings
> > R-package/*   @thirdwing
> > scala-package/*   @javelinjs
> > perl-package/*@sergeykolychev
> >
> > # CMake owners
> > CMakeLists.txt@cjolivier01
> > cmake/*  @cjolivier01
> >
>

Re: CODEOWNERS file being removed

2017-11-29 Thread Hen 
Was there more discussion than Justin's question about it on general@?

My memory of CODEOWNERS was that it was related to some code review tool,
but looking at the history of dev@ I only see:

"Can't have changes merged into it until changes to files that have a
designated code owner 
have been approved by that owner"

Reading GitHub:

"Code owners are automatically requested for review when someone opens a
pull request that modifies code that they own. When someone with admin or
owner permissions has enabled required reviews
,
they also can optionally require approval from a code owner before the
author can merge a pull request in the repository."

I think that having a notion where anyone can put their name down as an
automatic reviewer is a good one. Apart from the unfortunate name of the
file, this should fit with Apache's culture (as long as more than one
committer can, and as long as we never say a committer can't decide to put
themselves as a codeowner). A comment in the file explaining this would be
good.

We should however, never enable the code-owner option of required reviews.
As that's an Admin/Owner feature, and I don't think Apache Infra would do
that, I think we're good there.

So my suggestion would be to add a comment that this is an
automatically-listed-on-reviews tool.

Hen


On Wed, Nov 29, 2017 at 12:35 PM, Chris Olivier 
wrote:

> Per suggestion from Apache, we are removing CODEOWNERS file from root of
> mxnet.
> If there are any objections, please voice them:
>
> Here are the contents of rht file:
>
> # Owners of Apache MXNet
>
> # Global owners
> *@apache/mxnet-committers
>
> # Owners of language bindings
> R-package/*   @thirdwing
> scala-package/*   @javelinjs
> perl-package/*@sergeykolychev
>
> # CMake owners
> CMakeLists.txt@cjolivier01
> cmake/*  @cjolivier01
>

CODEOWNERS file being removed

2017-11-29 Thread Chris Olivier 
Per suggestion from Apache, we are removing CODEOWNERS file from root of
mxnet.
If there are any objections, please voice them:

Here are the contents of rht file:

# Owners of Apache MXNet

# Global owners
*@apache/mxnet-committers

# Owners of language bindings
R-package/*   @thirdwing
scala-package/*   @javelinjs
perl-package/*@sergeykolychev

# CMake owners
CMakeLists.txt@cjolivier01
cmake/*  @cjolivier01