Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add copyright hook #7666

Merged

Conversation

pranavm-nvidia
Copy link
Contributor

@pranavm-nvidia pranavm-nvidia commented Sep 27, 2024

What does the PR do?

Adds a copyright pre-commit hook (see below sections for details)

Checklist

  • I have read the Contribution guidelines and signed the Contributor License
    Agreement
  • PR title reflects the change and is of format <commit_type>: <Title>
  • Changes are described in the pull request.
  • Related issues are referenced.
  • Populated github labels field
  • Added test plan and verified test passes.
  • Verified that the PR passes existing CI.
  • I ran pre-commit locally (pre-commit install, pre-commit run --all)
  • Verified copyright is correct on all changed files. (that's the idea)
  • Added succinct git squash message before merging ref.
  • All template sections are filled out.
  • Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

Check the conventional commit type
box here and add the label to the github PR.

  • build
  • ci
  • docs
  • feat
  • fix
  • perf
  • refactor
  • revert
  • style
  • test

Where should the reviewer start?

The main files to be reviewed here are tools/add_copyright.py, which provides a new pre-commit hook that will update or add copyright headers to new or modified files, and .pre-commit-config.yaml. The logic is more complicated than one might expect because the repository includes many different types of files that require slightly different handling.

Caveats:

I've only added support for the most common file types that I saw in the repo. Note that it is usually trivially simple to add support for new file types using the building blocks I've provided in the hook implementation.

Background

Right now, there is no automatic way of updating/inserting copyright headers in the various source files in the repository. Consequently, some files have incorrect copyright years and many don't include headers at all. With this hook, the header no longer needs to be added or changed manually and will automatically be updated when the file is touched.

@pranavm-nvidia pranavm-nvidia force-pushed the add-copyright-hook branch 4 times, most recently from fb8269e to e4f9dfe Compare September 27, 2024 22:17
@rmccorm4 rmccorm4 self-requested a review September 30, 2024 21:10
@pranavm-nvidia
Copy link
Contributor Author

@rmccorm4 I can revert the changes to the copyright headers if it makes it easier to review. Let me know what the rules are for copyrights and I can implement those. My understanding right now is that it should be of the format Copyright <optional_first_modified_year>-<last_modified_year>

@rmccorm4
Copy link
Collaborator

rmccorm4 commented Sep 30, 2024

I believe you will find the following formats throughout the code:

  1. Copyright (c) YYYY - (YYYY - creation year == last modified year) never touched in a new year after creation year
  2. Copyright YYYY - same as (1), but I don't know if these should be considered invalid or not yet. Ideally we would standardize to one or the other for simplicity - but need legal confirmation most likely.
  3. Copyright YYYY-ZZZZ - (YYYY - creation year), (ZZZZ - last modified year), Intentionally removed (c) to avoid line wrapping with multi-year

I think supporting all of these (if possible) would provide the least number of files changed unnecessarily for review with max utility - and then I can separately follow-up to see if we standardize on (1) or (2) after confirmation with legal and not allow both.

@rmccorm4
Copy link
Collaborator

CC @GuanLuo for viz

Updates the copyright year in the LICENSE file if necessary and then
returns its contents.
"""
# TODO: Check if this is right - if the license file needs to have a range,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want the license file to contain a range? Right now it only has a single year.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good question, I'll need to check.

@pranavm-nvidia
Copy link
Contributor Author

@rmccorm4 I think this is ready for review now. See the included tests for what the expected behavior is with the various copyright formats.

@pranavm-nvidia pranavm-nvidia force-pushed the add-copyright-hook branch 3 times, most recently from 4e072f7 to 103c197 Compare October 1, 2024 21:29
Copy link
Collaborator

@rmccorm4 rmccorm4 left a comment

Choose a reason for hiding this comment

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

PR looks great to me! Let me run it by the team to make sure everyone is good with it 👍

I think this will save us from a lot of human error and extra commits saying "fix copyright", and remove a step from our PR template checklist!

@oandreeva-nv
Copy link
Contributor

lgtm in general, left some questions/suggestions

tools/add_copyright.py Fixed Show resolved Hide resolved
@pranavm-nvidia pranavm-nvidia force-pushed the add-copyright-hook branch 2 times, most recently from 3a4f2d4 to 6b41de6 Compare October 3, 2024 22:50
rmccorm4
rmccorm4 previously approved these changes Oct 3, 2024
@rmccorm4
Copy link
Collaborator

rmccorm4 commented Oct 4, 2024

@oandreeva-nv good to ship it?

oandreeva-nv
oandreeva-nv previously approved these changes Oct 4, 2024
Copy link
Contributor

@oandreeva-nv oandreeva-nv left a comment

Choose a reason for hiding this comment

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

LGTM

Adds a pre-commit hook that will automatically update or add copyright
headers to new or modified files.
@rmccorm4 rmccorm4 merged commit da05094 into triton-inference-server:main Oct 8, 2024
3 checks passed
@pranavm-nvidia pranavm-nvidia deleted the add-copyright-hook branch October 9, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants