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

[v1.9.x][License] Fix redundant ASF and Microsoft Apache 2.0 license files #20520

Merged
merged 8 commits into from
Aug 17, 2021

Conversation

sandeep-krishnamurthy
Copy link
Contributor

@sandeep-krishnamurthy sandeep-krishnamurthy commented Aug 12, 2021

Description

This PR files Item 6 in this issue - #20475

  1. Remove redundant ASF license header when there is Microsoft Apache 2.0 license and copyright header.
  2. Remove stale copyright by authors.
  3. Add relevant files with Apache 2.0 license + Microsoft copyright in LICENSE file
  4. Add MIT license files (deformable conv operators) in LICENSE file. [Source repo - https://github.com/msracver/Deformable-ConvNets/tree/master/DCNv2_op/nn]

Checklist

Essentials

  • PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
  • Changes are complete (i.e. I finished coding on this PR)

@mxnet-bot
Copy link

Hey @sandeep-krishnamurthy , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [unix-gpu, unix-cpu, centos-cpu, clang, website, edge, windows-gpu, centos-gpu, miscellaneous, windows-cpu, sanity]


Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@mseth10 mseth10 added the pr-awaiting-testing PR is reviewed and waiting CI build and test label Aug 12, 2021
@mseth10 mseth10 added pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Aug 12, 2021
LICENSE Outdated
Comment on lines 287 to 289
=======================================================================================
Apache-2.0 license + Microsoft Copyright
=======================================================================================
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The License file should not contain information about copyright. Copyright notices go to NOTICE file but only when legally required: https://infra.apache.org/licensing-howto.html#mod-notice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @leezu so I don't have to call these files out specially right as it is Apache 2.0 licensed?

Also, since we have left copyright of MSFT in these files as is, I don't have call it out in NOTICE file per the link you shared. let me know if this accurate.

From https://infra.apache.org/licensing-howto.html#mod-notice
"Copyright notifications which have been relocated, rather than removed, from source files must be preserved in NOTICE. However, elements such as the copyright notifications embedded within BSD and MIT licenses do not need to be duplicated in NOTICE. You can leave those notices in their original locations."

Copy link
Contributor

@leezu leezu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright notices shouldn't go to LICENSE AFAIK. At least it's not required and for consistency we may want to keep them out. WDYT?

Can you confirm that the reason for removing the ASF headers is that you think the files only contain minor modifications compared to the original Microsoft AL2 implementation? For major modifications, it's totally fine to add ASF header. https://www.apache.org/legal/src-headers.html#3party

I also want to clarify that the ASF header is not redundant to the Microsoft AL2 header. But it only makes sense for major modifications.

@mseth10 mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-work-in-progress PR is still work in progress labels Aug 12, 2021
@sandeep-krishnamurthy
Copy link
Contributor Author

sandeep-krishnamurthy commented Aug 12, 2021

As an additional step, I reached out to contributors, if they can file ICLA, if yes, we can have common ASF header and remove copyright, which is not necessary (since these files are Apache 2.0 license) but nice to have.

@sandeep-krishnamurthy
Copy link
Contributor Author

@leezu

Copyright notices shouldn't go to LICENSE AFAIK. At least it's not required and for consistency we may want to keep them out. WDYT?

Agreed. They are Apache 2.0 license. Copyright need not come here in License file.

Can you confirm that the reason for removing the ASF headers is that you think the files only contain minor modifications compared to the original Microsoft AL2 implementation? For major modifications, it's totally fine to add ASF header. https://www.apache.org/legal/src-headers.html#3party

That is correct.

@mseth10 mseth10 added pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Aug 12, 2021
@sandeep-krishnamurthy
Copy link
Contributor Author

@mxnet-bot run ci [unix-cpu, windows-gpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-cpu, windows-gpu]

@mseth10 mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Aug 13, 2021
@sandeep-krishnamurthy
Copy link
Contributor Author

@mxnet-bot run ci [unix-cpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-cpu]

@mseth10 mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Aug 13, 2021
@sandeep-krishnamurthy
Copy link
Contributor Author

@leezu CI is green. Requesting your review/merge.

1 similar comment
@sandeep-krishnamurthy
Copy link
Contributor Author

@leezu CI is green. Requesting your review/merge.

Copy link
Contributor

@leezu leezu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @sandeep-krishnamurthy. I noticed that 5 of the files are licensed under MIT license by Microsoft, but they are not listed in the LICENSE file

@sandeep-krishnamurthy
Copy link
Contributor Author

Thank you @sandeep-krishnamurthy. I noticed that 5 of the files are licensed under MIT license by Microsoft, but they are not listed in the LICENSE file

I don't see any MIT license files in this PR. Are you referring to other files outside this PR?

All the files in this PR are Caffe license (verified they are in LICENSE) and rest of Apache 2.0 license with Microsoft copyright (since they are Apache 2.0 license, did not explicitly specified in LICENSE file).

@leezu
Copy link
Contributor

leezu commented Aug 14, 2021

You removed * Licensed under The MIT License from 5 files in this PR. You can search the string in https://github.com/apache/incubator-mxnet/pull/20520/files

@sandeep-krishnamurthy
Copy link
Contributor Author

You removed * Licensed under The MIT License from 5 files in this PR. You can search the string in https://github.com/apache/incubator-mxnet/pull/20520/files

Yes. But those were not listed in License file earlier so no change w.r.t it in License file in this PR.

@leezu
Copy link
Contributor

leezu commented Aug 15, 2021

That may be a mistake in LICENSE rather than mistaken license header in the file. The original code is at https://github.com/msracver/Deformable-ConvNets/blob/6aeda878a95bcb55eadffbe125804e730574de8d/DCNv2_op/modulated_deformable_convolution.cc and under MIT license. "Do not modify or remove any copyright notices or licenses within third-party works." https://www.apache.org/legal/src-headers.html#3party

@mseth10 mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-awaiting-review PR is waiting for code review labels Aug 16, 2021
@sandeep-krishnamurthy
Copy link
Contributor Author

That may be a mistake in LICENSE rather than mistaken license header in the file. The original code is at https://github.com/msracver/Deformable-ConvNets/blob/6aeda878a95bcb55eadffbe125804e730574de8d/DCNv2_op/modulated_deformable_convolution.cc and under MIT license. "Do not modify or remove any copyright notices or licenses within third-party works." https://www.apache.org/legal/src-headers.html#3party

Ah! thanks for source repo. Fixed issues as below:

  1. There were 6 files (deformable_, modulated_deformable_ ops) under MIT per source repo. They are not substantially modified. Removed ASF 2.0 header in them. Maintained original MIT. Updated list of these files in LICENSE file under MIT license section.

@mseth10 mseth10 added pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Aug 16, 2021
@mseth10 mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Aug 16, 2021
@szha szha requested a review from leezu August 17, 2021 00:43
@leezu leezu merged commit ffb764f into apache:v1.9.x Aug 17, 2021
barry-jin pushed a commit to barry-jin/incubator-mxnet that referenced this pull request Sep 24, 2021
@barry-jin barry-jin mentioned this pull request Sep 24, 2021
4 tasks
barry-jin added a commit that referenced this pull request Sep 28, 2021
Co-authored-by: Sandeep Krishnamurthy <sandeep.krishna98@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants