Thanks for your notes Mu. The tutorial on both nnvm and onnx-mxnet is referenced from ONNX PyTorch tutorial. And both code repos ack that, as appropriate, and without including any specific names. When I looked at the nnvm onnx code, the commit history shows more than just Tianqi and Zhi, which made it seem more appropriate to refer to the entire dmlc/nnvm community.
Hagay On 11/16/17, 19:49, "Mu Li" <[email protected] on behalf of [email protected]> wrote: Thanks for pointing it out, Hagay. I actually missed the "Special thanks to dmlc/nnvm team" sentence. I was looking for Zhi and Tianqi's names, because the onnx converter was mainly done by both of them. However I do not entirely agree that I "had no comments". I actually pointed out the initial draft is similar to the nnvm tutorial two days ago. Hagay made a few updates, but unfortunately, I didn't have a chance to have a look at the updated version before the announcement. Otherwise, I should leave a comment to put individual contributors names on it. On Thu, Nov 16, 2017 at 7:03 PM, Lupesko, Hagay <[email protected]> wrote: > Chiming in as well. > > First and foremost, I agree wholeheartedly that acknowledgments are due > when deserved. In fact, we took care to add acknowledgments in the code, > and in the blog post for that precise reason! > I also personally talked with Mu, to make sure these are in order and > appropriate, and he had no comments. > Have we missed acknowledgments? Maybe (more on that below). But why assume > this was done on intention? > > Addressing specific points (I won’t repeat Henri’s points): > - I’m happy to take another look and see whether more files need to have > the “ack” statement. But looking into it again, import_onnx.py [1] is the > only one that seem to have been missed, and ack was already added. Sheng – > I’ll grab some time with you Monday to discuss in details. > - The tutorial itself was actually referenced from PyTorch, not nnvm. This > is acknowledged by onnx-mxnet code, as well as the nnvm code. > - We intentionally ack-ed an open source community (dmlc/nnvm) and not > individuals. There’s more than Tianqi and Zhi that worked on nnvm and onnx, > it is a whole community that we thank to. > - “I was wondering why your below email didn't include such > acknowledgement?” – as noted by Hen, the email did include the ack. > > One last thing, quoting Sheng: “In general, to have a healthy community, I > believe the right things to do would be…” > I would stress out that in order to have a healthy community, we should > always assume others have best intentions – this will make us a stronger > community, one that works together, and one that if fun to be part of. > > Hagay > > [1] https://github.com/onnx/onnx-mxnet/blob/master/onnx_mxnet/ > import_onnx.py > > On 11/16/17, 18:06, "Hen" <[email protected]> wrote: > > On Thu, Nov 16, 2017 at 4:32 PM, Sheng Zha <[email protected]> wrote: > > > Hi Hagay, > > > > (cc'd Zhi, Tianqi to make sure real authors are aware) > > > > > > > > At first glance the code in the repo you shared (i.e. > > https://github.com/onnx/onnx-mxnet) looks very > > > > familiar, so I did some searching. It looks like *almost all* the > code > > are adopted from the *nnvm onnx* > > > > frontend, but the main contributor (*Zhi Zhang*, committer of mxnet, > and > > intern at AWS) from this same > > > > community was not given his due credit in your email. To elaborate > on why > > I think almost all the > > > > onnx-mxnet code is from nnvm onnx frontend: > > > > > > > > The following is the content of this repo: > > > > ├── LICENSE.txt > > > > ├── README.md > > > > ├── onnx_mxnet > > > > │ ├── __init__.py > > > > │ ├── common.py > > > > │ ├── import_helper.py > > > > │ ├── import_onnx.py > > > > │ └── tests > > > > │ ├── test_models.py > > > > │ └── test_super_resolution.py > > > > ├── setup.py > > > > ├── super_res_input.jpg > > > > └── super_res_output.jpg > > > > (Also attached a screenshot of the commit history of onnx_mxnet at > the > > moment, as well as a copy of the git package, in case commit hash > mismatch > > happens) > > > > > > > > - Out of the 6 files under onnx_mxnet package > > - the following two files are marked as being derived from > nnvm: > > - common.py > > <https://github.com/onnx/onnx-mxnet/blob/master/onnx_mxnet/ > common.py#L12> > > - import_helper.py > > <https://github.com/onnx/onnx-mxnet/blob/master/onnx_mxnet/ > import_helper.py#L12> > > - the rest four files that are not marked as being derived from > > nnvm: > > - __init__.py > > <https://github.com/onnx/onnx-mxnet/blob/ > 4ebc02e8f1cc523049f0928b6dbc566a93dd2f47/onnx_mxnet/__init__.py#L15>: > > looks like a copy from nnvm/frontend/onnx.py > > <https://github.com/dmlc/nnvm/blob/ > 3da53e46db57c438b05fbebe8aa332ee8c5994d1/python/nnvm/frontend/onnx.py#L392 > > > > - import_onnx.py > > <https://github.com/onnx/onnx-mxnet/blob/ > 4ebc02e8f1cc523049f0928b6dbc566a93dd2f47/onnx_mxnet/import_onnx.py#L18> > looks > > like a copy from nnvm/frontend/onnx.py > > <https://github.com/dmlc/nnvm/blob/ > 3da53e46db57c438b05fbebe8aa332ee8c5994d1/python/nnvm/frontend/onnx.py#L345 > > > > - tests/test_models.py > > <https://github.com/onnx/onnx-mxnet/blob/ > 4ebc02e8f1cc523049f0928b6dbc566a93dd2f47/onnx_mxnet/tests/test_models.py> > > - this part > > <https://github.com/onnx/onnx-mxnet/blob/ > 4ebc02e8f1cc523049f0928b6dbc566a93dd2f47/onnx_mxnet/tests/ > test_models.py#L35> looks > > like a copy from nnvm frontend/onnx/model_zoo/__ > init__.py > > <https://github.com/dmlc/nnvm/blob/ > 3da53e46db57c438b05fbebe8aa332ee8c5994d1/tests/python/ > frontend/onnx/model_zoo/__init__.py#L7> > > - this part > > <https://github.com/onnx/onnx-mxnet/blob/ > 4ebc02e8f1cc523049f0928b6dbc566a93dd2f47/onnx_mxnet/tests/ > test_models.py#L63> looks > > like a copy from nnvm frontend/onnx/test_forward.py > > <https://github.com/dmlc/nnvm/blob/ > 3da53e46db57c438b05fbebe8aa332ee8c5994d1/tests/python/ > frontend/onnx/test_forward.py#L9> > > - tests/test_super_resolution.py > > <https://github.com/onnx/onnx-mxnet/blob/ > 4ebc02e8f1cc523049f0928b6dbc566a93dd2f47/onnx_mxnet/tests/ > test_super_resolution.py#L24> looks > > like a copy from nnvm tutorials/from_onnx.py > > <https://github.com/dmlc/nnvm/blob/ > 3da53e46db57c438b05fbebe8aa332ee8c5994d1/tutorials/from_onnx.py#L22> > > > > > > > > > > > > Looks like in the blog post here > > <https://aws.amazon.com/blogs/ai/announcing-onnx-support- > for-apache-mxnet/>, > > there is one line giving credit to nnvm authors: > > > > *"Special thanks to the dmlc/nnvm community, whose ONNX code was > used as a > > reference for this implementation."* > > > > > > > > I was wondering why your below email didn't include such > acknowledgement? > > In general, to have a healthy community, I believe the right things > to do > > would be: > > > > 1. Give the due credit to the original authors in communications. > > > > 2. Keep original authors' names in the file licenses, which is a > required > > for apache 2.0 license <http://www.apache.org/licenses/LICENSE-2.0> > that > > is used in both packages. > > > > 3. For code that you reorganized, keep a reference of the source from > > which it’s adopted, which is good practice when contributing > open-source. > > > > On #2 - the Apache license does not require that author's names be > kept in > files (I presume you're referring to the Author comment in the tutorial > file as I didn't see any others). > > The Apache license requires that (section 4): > > - You must give any other recipients of the Work or Derivative Works a > copy > of this License; and > - You must cause any modified files to carry prominent notices stating > that > You changed the files; and > - You must retain, in the Source form of any Derivative Works that You > distribute, all copyright, patent, trademark, and attribution notices > from > the Source form of the Work, excluding those notices that do not > pertain to > any part of the Derivative Works; and > - If the Work includes a "NOTICE" text file as part of its > distribution, > then any Derivative Works that You distribute must include a readable > copy > of the attribution notices contained within such NOTICE file, > > Going through each in turn: > > 1) The repo contains a copy of the Apache 2.0 license, > 2) 2 of the 6 files contain prominent notices; I agree that the other 4 > should contain a prominent notice if they're derived (I haven't > compared > the files, but I don't doubt your research), > 3) There are no additional copyright, patent, trademark or attribution > notices in the files copied (this is why Apache requires a source > header > for its projects; DMLC does not appear to have source headers on their > files, its equivalent of Apache's legal committee should strongly think > about that standardizing on that for all its projects imo); > 4) There is no NOTICE file (again, imo the DMLC legal thinkers should > strongly think about having NOTICE files imo). > > So agreed that there's a change to make, but there's no requirement to > keep > author's names in files. Note that there is a practice (and it's a > recommended or even mandatory legal practice, I'm not sure which) to > keep a > Copyright Owner's name in a file, but author and copyright owner are > different concepts. > > Hen > > > >
