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

[v1.7.x]License checker enhancement #18478

Merged
merged 5 commits into from
Jun 25, 2020
Merged

Conversation

ciyongch
Copy link
Contributor

@ciyongch ciyongch commented Jun 3, 2020

Description

  1. Enhance the current license checker to cover multiple license header and the *.md files.
  2. Add the statement in top level LICENSE of einsum related files.
  3. Restrict the numpy version to <1.19.0 in CI scripts.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@ciyongch ciyongch requested a review from szha as a code owner June 3, 2020 14:14
@ciyongch ciyongch mentioned this pull request Jun 3, 2020
11 tasks
@ciyongch
Copy link
Contributor Author

Hi @szha @TaoLv @leezu , with the enhancement (check multiple license header and check *.md files) to license_header.py, there's some more files that missed a license header were exposed,

1) files missing a license header
The .github/*.md files were added by @szha , while some files were added in the year of 2015/2016. Inform the original/modified author and ask them help to (or get approval from them to) add the license header, may I have your suggestions to handle this? thanks!

[2020-06-11T06:58:34.163Z] license_header.py: ERROR File .github/ISSUE_TEMPLATE/bug_report.md doesn't have a valid license
[2020-06-11T06:58:34.163Z] license_header.py: ERROR File .github/ISSUE_TEMPLATE/feature_request.md doesn't have a valid license
[2020-06-11T06:58:34.163Z] license_header.py: ERROR File .github/ISSUE_TEMPLATE/flaky_test.md doesn't have a valid license
[2020-06-11T06:58:34.163Z] license_header.py: ERROR File .github/PULL_REQUEST_TEMPLATE.md doesn't have a valid license
[2020-06-11T06:58:34.418Z] license_header.py: ERROR File R-package/README.md doesn't have a valid license
[2020-06-11T06:58:34.418Z] license_header.py: ERROR File example/image-classification/README.md doesn't have a valid license
[2020-06-11T06:58:34.418Z] license_header.py: ERROR File example/image-classification/predict-cpp/README.md doesn't have a valid license
[2020-06-11T06:58:34.418Z] license_header.py: ERROR File example/image-classification/symbols/README.md doesn't have a valid license
[2020-06-11T06:58:34.418Z] license_header.py: ERROR File example/quantization/README.md doesn't have a valid license
[2020-06-11T06:58:34.673Z] license_header.py: ERROR File julia/docs/src/api/optimizer.md doesn't have a valid license
[2020-06-11T06:58:35.183Z] license_header.py: ERROR File tests/python-pytest/onnx/README.md doesn't have a valid license

2) multiple license header issue
They're tracked in #17329 (comment)

[2020-06-11T06:58:34.673Z] license_header.py: ERROR File python/mxnet/contrib/onnx/mx2onnx/_op_translations.py has multiple license
[2020-06-11T06:58:34.673Z] license_header.py: ERROR File python/mxnet/contrib/onnx/mx2onnx/export_onnx.py has multiple license
[2020-06-11T06:58:34.928Z] license_header.py: ERROR File src/operator/contrib/nn/modulated_deformable_im2col.cuh has multiple license
[2020-06-11T06:58:34.928Z] license_header.py: ERROR File src/operator/contrib/nn/modulated_deformable_im2col.h has multiple license
[2020-06-11T06:58:34.928Z] license_header.py: ERROR File src/operator/numpy/np_einsum_op.cc has multiple license
[2020-06-11T06:58:34.928Z] license_header.py: ERROR File src/operator/numpy/np_einsum_path_op-inl.h has multiple license

@szha
Copy link
Member

szha commented Jun 12, 2020

  1. files missing a license header

I think all cases fall under contribution in apache so I think we can add apache licenses to them.

@ciyongch
Copy link
Contributor Author

ciyongch commented Jun 12, 2020

Hi @szha, the md files that missing license header were updated, but the md files under .github/ folder are not added due to the new header will break the github template configuration. Please help to take a review for PR #18541.

@ChaiBapchya
Copy link
Contributor

@ciyongch Can you update the license_header.py to ignore files in /.github folder? So that it passes the sanity & we can fix this PR? Also you'd have to rebase this PR so it includes the merged changes from #18541
Thanks.

@leezu
Copy link
Contributor

leezu commented Jun 19, 2020

@ciyongch Did you try add the header as follows?

<!--- Licensed to the Apache Software Foundation (ASF) under one -->
<!--- or more contributor license agreements.  See the NOTICE file -->
<!--- distributed with this work for additional information -->
<!--- regarding copyright ownership.  The ASF licenses this file -->
<!--- to you under the Apache License, Version 2.0 (the -->
<!--- "License"); you may not use this file except in compliance -->
<!--- with the License.  You may obtain a copy of the License at -->

<!---   http://www.apache.org/licenses/LICENSE-2.0 -->

<!--- Unless required by applicable law or agreed to in writing, -->
<!--- software distributed under the License is distributed on an -->
<!--- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -->
<!--- KIND, either express or implied.  See the License for the -->
<!--- specific language governing permissions and limitations -->
<!--- under the License. -->

@ChaiBapchya
Copy link
Contributor

Right. Instead of my suggestion of skipping the tests; adding it that way is better. Thanks for that one @leezu

@ciyongch
Copy link
Contributor Author

Hi @ChaiBapchya , @leezu , this enhancement has dependencies as below:
1) files missing a license header
I did try to add the license header as above, but this header will invalid the issue template of github (the option disappeared when adding the license header, not sure if it's configurable to skip such header or not), so I only update the rest of md files as in #18541. So if we're going to keep these templates, skip checking of .md files under .github might be more appropriate for now.
2) multiple license header issue
There's a pending item regarding the dual license header files need to be addressed which is discussed in https://lists.apache.org/thread.html/r6d8f62b1ad34f6dbe109a01e715723e8996cd478a01a08498aa02a2e%40%3Cdev.mxnet.apache.org%3E, this kind of issue will be caught by this enhancement.
I need to update the header for these files as well as this enhancement, and resubmit the PR.
As @leezu proposed a 72 hours lazy consensus on 15/6, is that finalized? Or can you help to accelerate the decision making? Thanks!

@ciyongch
Copy link
Contributor Author

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

@mxnet-bot
Copy link

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

@ciyongch
Copy link
Contributor Author

The failure CI cases is caused by numpy version changed which is reported in #18600 (master branch), the current header/tool changed has nothing to do with the failure.
Probably we need another PR to temporary disable these failed cases, what do you suggest @ChaiBapchya @leezu ?

@ChaiBapchya
Copy link
Contributor

Thanks for driving this. Looks like fixing numpy version helped resolve other pipeline issues [barring windows-cpu]
@leezu any thoughts?

@ciyongch
Copy link
Contributor Author

Hi @ChaiBapchya , I've updated the numpy version constraint to < 1.19.0 instead of a fixed version for ci scripts as well as tests/requirements.txt, seems it's used by the [windows-cpu] job. Let's wait for the results.

@ciyongch
Copy link
Contributor Author

@mxnet-bot run ci [sanity]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [sanity]

@ciyongch
Copy link
Contributor Author

@mxnet-bot run ci [all]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [edge, windows-cpu, windows-gpu, unix-gpu, unix-cpu, centos-gpu, miscellaneous, sanity, clang, website, centos-cpu]

@ciyongch
Copy link
Contributor Author

@mxnet-bot run ci [unix-gpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-gpu]

@ciyongch
Copy link
Contributor Author

Hi @szha @leezu @ChaiBapchya , please help to take a review for the new changes/updates.

Copy link
Contributor

@ChaiBapchya ChaiBapchya left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks. LGTM

tools/license_header.py Show resolved Hide resolved
tools/license_header.py Show resolved Hide resolved
@leezu leezu merged commit 9c06894 into apache:v1.7.x Jun 25, 2020
@szha
Copy link
Member

szha commented Jun 25, 2020

Shall we cherry-pick this to the master branch too?

@ciyongch
Copy link
Contributor Author

Sure, I will backport the license checker/modification part to both v1.x and master later.

@szha
Copy link
Member

szha commented Jul 22, 2020

There seems to be two licenses in cmake/Modules/FindJeMalloc.cmake. Is it because we are skipping 3rdparty folder? Since it's included in the release source, we will need to make sure that all parts we include are checked.

@ciyongch
Copy link
Contributor Author

Hi @szha, the license of cmake/Modules/FindJeMalloc.cmake is already claimed at the top level LICENSE file in https://github.com/apache/incubator-mxnet/blob/v1.7.x/LICENSE#L681-L769, so we can keep it as is now.

@szha
Copy link
Member

szha commented Jul 22, 2020

I don't think we discussed the dual license issue for this file and I think it still needs to be discussed even if the original license is declared in the LICENSE file. That said. this is a minor issue and it won't block the release.

@ciyongch
Copy link
Contributor Author

Ok, got it, then let's keep it as is now and try to finalize the fix solution in the next release.
Actually how to handle the dual license issue or re-license the third party work is kind of complicated as it's required to be discussed case-by-case instead of a clear/direct instruction.
Do you think we need to open another issue to track this?

@szha
Copy link
Member

szha commented Jul 22, 2020

Yes sounds good.

@ChaiBapchya
Copy link
Contributor

@ciyongch is an issue opened for dual licensing/relicensing 3rd party? Is it complete? Can you link that here? Thanks.

@ciyongch
Copy link
Contributor Author

Hi @ChaiBapchya , sorry for missing this, I've just created an issue #19198 for this.

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

Successfully merging this pull request may close these issues.

5 participants