Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[RFC] [Licensing] Address ASF Feedback for 1.9.0.rc7 release #20616

Closed
5 of 6 tasks
josephevans opened this issue Sep 28, 2021 · 12 comments
Closed
5 of 6 tasks

[RFC] [Licensing] Address ASF Feedback for 1.9.0.rc7 release #20616

josephevans opened this issue Sep 28, 2021 · 12 comments
Labels
Licenses RFC Post requesting for comments

Comments

@josephevans
Copy link
Contributor

josephevans commented Sep 28, 2021

Problem statement

Based on the feedback from ASF on our vote to release MXNet v1.9.0.rc7, we still have a few licensing issues that caused a -1 vote, which prevents us from moving forward with the release.

Based on the RC7 voting thread, the following licensing issues are still outstanding:

  • Files incorrectly have ASF headers. These files are all copyrighted by NumPy developers and released under the 3-clause BSD license. We should not have added the ASF header and kept the original license headers intact, and mention this by including it in the "3-clause BSD license" section in LICENSE.

    • src/operator/numpy/np_einsum_op.cc
    • src/operator/numpy/np_einsum_op-inl.h
    • src/operator/numpy/np_einsum_path_op-inl.h
  • Some files are not correctly listed in LICENSE. For example, these files contain a "Copyright Microsoft" line but are released under ASF-2 or MIT license. These need to be properly mentioned in LICENSE:

    • src/operator/contrib/deformable_psroi_pooling.cu
    • src/operator/contrib/deformable_psroi_pooling-inl.h
    • src/operator/contrib/deformable_psroi_pooling.cc
    • src/operator/contrib/multi_proposal-inl.h
    • src/operator/contrib/multi_proposal.cc
    • src/operator/contrib/multi_proposal.cu
    • src/operator/contrib/psroi_pooling.cc
    • src/operator/contrib/psroi_pooling.cu
    • Additional files in the same situation as above (identified by running skywalking-eyes):
    • src/operator/contrib/psroi_pooling-inl.h
    • src/operator/contrib/deformable_convolution-inl.h
    • src/operator/contrib/deformable_convolution.cc
    • src/operator/contrib/deformable_convolution.cu

As ASF noted, not all files were checked and there could be others that are missed. In order to better identify licensing issues in the future, we should extend the use of automated tools (such as skywalking-eyes).

References

@josephevans josephevans added RFC Post requesting for comments Licenses labels Sep 28, 2021
@josephevans
Copy link
Contributor Author

josephevans commented Sep 28, 2021

I've created a PR (#20598) to address the first three items and the last item (and checked as complete.) Please review with any comments/suggestions/concerns. This PR also adds a Github workflow (thanks to @barry-jin for help) to use Apache's latest license checking tool, skywalking-eyes.

@josephevans
Copy link
Contributor Author

josephevans commented Sep 29, 2021

I manually checked all source files in src/operator to ensure they are either:

  1. Licensed under ASF-2.0 license with standard recognizable header. If they do not contain the standard header, they should be listed under the ASF-2.0 section in LICENSE
  2. Licensed under some other compatible license, but clearly mentioned in LICENSE under the compatible license section (ie BSD 2/3-clause, MIT, etc.)

Any help manually auditing the rest of the source tree would be greatly appreciated.

josephevans added a commit that referenced this issue Sep 29, 2021
Fix the first 3 items in #20616 

* Remove ASF2.0 license and properly list under 3-clause BSD files in LICENSE for np_einsum files.

* Fix license category for deformable_convolution files.

* Update list of Apache-2.0 licensed files.

* Add files to LICENSE that fall under Apache 2.0 license.

* Add include symlinks and other ASF2 files to LICENSE.

* Remove extra line.

* Add numpy einsum files (licensed under bsd 3-clause) to rat-excludes.

* Mention include/nnvm in LICENSE file.

* Remove files from LICENSE with clear ASF-2.0 license headers, added by accident based on skywalking-eyes faulty results.

* Move header guards to below license header section, so skywalking-eyes tool recognizes ASF-2.0 header.

* Fix formatting with license header.

* Fix license section for FindJeMalloc.cmake, move protoc-gen-mypy.py to Apache-2.0 licensed files.

* Clearly indicate caffe subdirectory's licensing model in LICENSE.

* Add skywalking-eyes config to repo.

* Add skywalking-eyes config (.licenserv.yaml) to rat-excludes.

* Add skywalking-eyes license checker into Github workflow.

* Remove caffe subdir from LICENSE and license-check config whitelist, since it is no longer in the repo.

* Remove duplicate entries in LICENSE - leave in Caffe license section.
josephevans added a commit to josephevans/mxnet that referenced this issue Sep 29, 2021
Fix the first 3 items in apache#20616

* Remove ASF2.0 license and properly list under 3-clause BSD files in LICENSE for np_einsum files.

* Fix license category for deformable_convolution files.

* Update list of Apache-2.0 licensed files.

* Add files to LICENSE that fall under Apache 2.0 license.

* Add include symlinks and other ASF2 files to LICENSE.

* Remove extra line.

* Add numpy einsum files (licensed under bsd 3-clause) to rat-excludes.

* Mention include/nnvm in LICENSE file.

* Remove files from LICENSE with clear ASF-2.0 license headers, added by accident based on skywalking-eyes faulty results.

* Move header guards to below license header section, so skywalking-eyes tool recognizes ASF-2.0 header.

* Fix formatting with license header.

* Fix license section for FindJeMalloc.cmake, move protoc-gen-mypy.py to Apache-2.0 licensed files.

* Clearly indicate caffe subdirectory's licensing model in LICENSE.

* Add skywalking-eyes config to repo.

* Add skywalking-eyes config (.licenserv.yaml) to rat-excludes.

* Add skywalking-eyes license checker into Github workflow.

* Remove caffe subdir from LICENSE and license-check config whitelist, since it is no longer in the repo.

* Remove duplicate entries in LICENSE - leave in Caffe license section.
josephevans added a commit that referenced this issue Sep 30, 2021
* [v1.9.x] license updates (#20598)

Fix the first 3 items in #20616

* Remove ASF2.0 license and properly list under 3-clause BSD files in LICENSE for np_einsum files.

* Fix license category for deformable_convolution files.

* Update list of Apache-2.0 licensed files.

* Add files to LICENSE that fall under Apache 2.0 license.

* Add include symlinks and other ASF2 files to LICENSE.

* Remove extra line.

* Add numpy einsum files (licensed under bsd 3-clause) to rat-excludes.

* Mention include/nnvm in LICENSE file.

* Remove files from LICENSE with clear ASF-2.0 license headers, added by accident based on skywalking-eyes faulty results.

* Move header guards to below license header section, so skywalking-eyes tool recognizes ASF-2.0 header.

* Fix formatting with license header.

* Fix license section for FindJeMalloc.cmake, move protoc-gen-mypy.py to Apache-2.0 licensed files.

* Clearly indicate caffe subdirectory's licensing model in LICENSE.

* Add skywalking-eyes config to repo.

* Add skywalking-eyes config (.licenserv.yaml) to rat-excludes.

* Add skywalking-eyes license checker into Github workflow.

* Remove caffe subdir from LICENSE and license-check config whitelist, since it is no longer in the repo.

* Remove duplicate entries in LICENSE - leave in Caffe license section.

* Remove extra characters accidentally added to Apache license header.
@barry-jin
Copy link
Contributor

barry-jin commented Oct 1, 2021

I have manually checked all the header files in include/onednn(mkldnn), include/dlpack, include/dmlc, include/mshadow and found one more issue:

  • include/dmlc/concurrentqueue.h and include/dmlc/blockingconcurrentqueue.h are under the Simplified BSD license (2-clause BSD license), which are not correctly listed in LICENSE.

@josephevans
Copy link
Contributor Author

I'm currently having issues getting #20626 to pass CI because of MKLDNN-related tests - opened an issue (#20643) to track.

@josephevans
Copy link
Contributor Author

I looked into the license_header.py script, and it actually will detect when a file has multiple licenses and throw an error. We may need to expand the list of strings it recognizes licenses with here:

https://github.com/apache/incubator-mxnet/blob/v1.9.x/tools/license_header.py#L62-L63

@josephevans
Copy link
Contributor Author

At this point, we have manually reviewed all the source files in src to ensure they are properly ASF-2.0 licensed, or licensed under a difference license and mentioned in LICENSE. We also added another CI pipeline to run the skywalking-eyes license checker and identified a few minor license typos (such as extra characters accidentally added to license headers.)

Since we don't believe there are any additional outstanding license issues, I think we should move forward with the v1.9.0 release. Does anyone have comments or issues?

@szha
Copy link
Member

szha commented Oct 27, 2021

Thanks Joe and Zhenghui and others who helped on the efforts in thoroughly addressing the license issues. Also thanks to @kezhenxu94 for sharing the great license checker tool from the skywalking community. I have a question from reviewing the LICENSE file from the master branch:

  • For the files under "Caffe Licensing Model", I checked the first file in that section and found the license to be under BSD-2 with Caffe's own copyright model. Since "Caffe Licensing Model" isn't listed in the Apache's license categories, should we call it "BSD-2 with Caffe Copyright Notice and Disclaimer" instead?
  • I saw that we still have several sections in LICENSE where they have dual licenses (e.g. files under "Apache-2.0 license + 3-clause BSD license" are adapted from ONNX and NumPy code). From the past votes we know we want to avoid them. What's preventing us from just using original licenses which are permissible? For the numpy files, they appear to be the first item in the checklist of this issue, and their content was updated. Why then are we still listing them in LICENSE under two licenses?

@josephevans
Copy link
Contributor Author

Thanks for the feedback @szha. I addressed the two issues you list above in #20709. I also audited the LICENSE file in v1.x branch and ensured everything listed is in the proper section.

After reading through https://infra.apache.org/licensing-howto.html, I also wanted to ensure that we comply to licensing terms for binary distributions - we need to include any copyright clauses and provide the associated license text in any binary distribution of MXNet. In order to achieve this, I created a number of LICENSE.*.txt files that includes the required text. These files should automatically get included in pip wheels (see https://github.com/apache/incubator-mxnet/blob/v1.x/tools/pip/setup.py#L94-L95) to fulfill terms. This is how the TVM project includes license text, although MXNet has many more files under different licenses.

I would appreciate a review or comments on this approach.

@josephevans
Copy link
Contributor Author

Created a PR for the v1.9.x branch: #20722

@josephevans
Copy link
Contributor Author

Once #20722 is merged, I would like to close this issue and move forward with 1.9.0.rc8. I've converted the single outstanding task of updating the license tool to a new item for tracking (#20723).

@szha
Copy link
Member

szha commented Nov 3, 2021 via email

@josephevans
Copy link
Contributor Author

Merged #20726 into v1.9.x, we should be ready for another release candidate. Closing this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Licenses RFC Post requesting for comments
Projects
None yet
Development

No branches or pull requests

3 participants