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

ci: Add copyright-check workflow #369

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

ko3n1g
Copy link
Collaborator

@ko3n1g ko3n1g commented Nov 14, 2024

Description

Usage

# Add snippet demonstrating usage

Checklist

  • I am familiar with the Contributing Guide.
  • New or Existing tests cover these changes.
  • The documentation is up to date with these changes.

@ko3n1g ko3n1g requested a review from sarahyurick November 14, 2024 16:22
Signed-off-by: Oliver Koenig <[email protected]>
Copy link
Collaborator

@sarahyurick sarahyurick left a comment

Choose a reason for hiding this comment

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

I have a few questions about how this is supposed to work. Some are a bit nitpicky but want to make sure:

  • It looks like it is only supposed to check Python files. However, I see some places where init.py files are skipped. Is this intentional?
  • Can we also have it check YAML/YML files and shell scripts?
  • There are a couple Python files where the year is 2023 (see example) instead of 2024. Is there a way we can check for this too? I imagine when 2025 hits we will want to update too?
  • There are other instances, such as here, where the copyright is even more out of date.
  • Sometimes the files say Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved. and sometimes they just say Copyright (c) 2024, NVIDIA CORPORATION.. Is there any way we can do an exact string match to ensure that all copyrights are identical?
  • Should we make sure the Dockerfile has the copyright?

@ko3n1g
Copy link
Collaborator Author

ko3n1g commented Nov 14, 2024

I have a few questions about how this is supposed to work. Some are a bit nitpicky but want to make sure:

  • It looks like it is only supposed to check Python files. However, I see some places where init.py files are skipped. Is this intentional?
  • Can we also have it check YAML/YML files and shell scripts?
  • There are a couple Python files where the year is 2023 (see example) instead of 2024. Is there a way we can check for this too? I imagine when 2025 hits we will want to update too?
  • There are other instances, such as here, where the copyright is even more out of date.
  • Sometimes the files say Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved. and sometimes they just say Copyright (c) 2024, NVIDIA CORPORATION.. Is there any way we can do an exact string match to ensure that all copyrights are identical?
  • Should we make sure the Dockerfile has the copyright?

Maybe best to sync with Pablo on this, he's in charge of the copyright script.


jobs:
copyright-check:
uses: NVIDIA/NeMo-FW-CI-templates/.github/workflows/[email protected]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
uses: NVIDIA/NeMo-FW-CI-templates/.github/workflows/_copyright_check.yml@v0.2.0
uses: NVIDIA/NeMo-FW-CI-templates/.github/workflows/_copyright_check.yml@v0.14.0

This version allows to check other file types too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! From NVIDIA/NeMo-FW-CI-templates#57, I am thinking we should set additional-find-args to .yml, .yaml, and .sh.

@pablo-garay
Copy link
Collaborator

Sorry i was writing reply for this & got into something else & missed it

It looks like it is only supposed to check Python files. However, I see some places where init.py files are skipped. Is this intentional?
Yes, init.py files are empty so we skip them
Can we also have it check YAML/YML files and shell scripts?
Just created a PR to allow this
There are a couple Python files where the year is 2023 (see example) instead of 2024. Is there a way we can check for this too? I imagine when 2025 hits we will want to update too? There are other instances, such as here, where the copyright is even more out of date.
I am not sure about this actually . I had a few talks and we didnt see this a needed requirement so far
Sometimes the files say Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved. and sometimes they just say Copyright (c) 2024, NVIDIA CORPORATION.. Is there any way we can do an exact string match to ensure that all copyrights are identical?
I guess we can do this / it's doable
Should we make sure the Dockerfile has the copyright?
Not sure, need to check on this / if this needed. And which exact Dockerfile are we talking about?

@sarahyurick
Copy link
Collaborator

Thanks @pablo-garay ! A couple more comments:

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

Successfully merging this pull request may close these issues.

3 participants