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.

Sign up for GitHub

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

Jump to bottom

Replace local copyright check with pre-commit-hooks verify-copyright #14917

Merged
merged 23 commits into from
Mar 4, 2024
Merged

Replace local copyright check with pre-commit-hooks verify-copyright #14917

merged 23 commits into from
Mar 4, 2024

Conversation

KyleFromNVIDIA
Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA commented Jan 29, 2024 •
edited
Loading

Description

The local copyright.py script is bug-prone. Replace it with a more robust centralized script from pre-commit-hooks.

Checklist

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

@KyleFromNVIDIA
Replace local copyright check with pre-commit-hooks verify-copyright
0af7a37 
And get all existing files up to date with --batch.
@KyleFromNVIDIA KyleFromNVIDIA requested review from a team as code owners January 29, 2024 14:02
@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. CMake CMake build issue Java Affects Java cuDF API. ci labels Jan 29, 2024
@KyleFromNVIDIA KyleFromNVIDIA added 2 - In Progress Currently a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. CMake CMake build issue Java Affects Java cuDF API. ci labels Jan 29, 2024
@karthikeyann karthikeyann added feature request New feature or request and removed feature request New feature or request labels Jan 29, 2024
@karthikeyann
Copy link
Contributor

karthikeyann commented Jan 29, 2024 •
edited
Loading

Are the verify-copyright failures due to the changes to files currently in this PR?
Eg. join.pxd is updated to 2021, but it expect it to be 2024.

@KyleFromNVIDIA
Copy link
Contributor Author

KyleFromNVIDIA commented Jan 29, 2024 •
edited
Loading

Yes. It appears there's more work to do on the new verify-copyright check. Closing for now.

Edit: I forgot to remove the --batch argument after running it in batch mode.

@github-actions github-actions bot added the Python Affects Python cuDF API. label Jan 30, 2024
@github-actions github-actions bot removed the Python Affects Python cuDF API. label Jan 30, 2024
@bdice bdice mentioned this pull request Jan 30, 2024
Open
bdice
bdice reviewed Jan 30, 2024
View reviewed changes
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@ttnghia ttnghia removed the request for review from a team January 30, 2024 19:50
@charlesbluca charlesbluca removed the request for review from a team February 21, 2024 19:43
@bdice
Copy link
Contributor

bdice commented Feb 26, 2024

@KyleFromNVIDIA Is anything more needed here? Please ping ops for a review in Slack if that's the only missing piece.

@harrism harrism mentioned this pull request Feb 27, 2024
Merged
3 tasks
@karthikeyann
Copy link
Contributor

Will the new pre-commit hook update correctly for PRs created in past year, but still open this year? I had a similar issue with old copyright script in local pre-commit runs. Old script updated a few files which are not part of the PR.
If it is not possible to test or deduce it, it may be ignored.

@KyleFromNVIDIA
Copy link
Contributor Author

The new script will only update files that have actually been changed from the target branch. Not only that, but if a file has its copyright notice updated but has no other changes, the new script will detect the false update and revert it.

karthikeyann
karthikeyann approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

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

Great work @KyleFromNVIDIA 👍

@harrism
Copy link
Member

harrism commented Feb 28, 2024

Any idea why this is failing?

@KyleFromNVIDIA
Copy link
Contributor Author

CI failed due to the ongoing Python 3.11 effort (rapidsai/build-planning#3). We've changed shared-workflows to always pass the 3.11 jobs for now. I'm running CI again.

@bdice
Copy link
Contributor

bdice commented Feb 29, 2024

I'm running CI again.

@KyleFromNVIDIA This failed again because clicking the "Rerun failed jobs" button does not update the shared-workflows. It uses the shared-workflows as of the time of that commit. To get the changes from rapidsai/shared-workflows#182 that are needed to make CI pass, you have to make a new commit (merge the upstream or push an empty commit). I'll merge the upstream.

@KyleFromNVIDIA
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 0ff5a2c into rapidsai:branch-24.04 Mar 4, 2024
73 checks passed
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Mar 6, 2024
@harrism
Automate C++ include file grouping and ordering using clang-format (#…
baa1bbb 
…1349)

This uses the IncludeCategories settings in .clang-format to attempt to enforce our documented #include order in libcuspatial. For discussion, see rapidsai/cudf#15063. This PR uses a subset of the header grouping categories used in that PR.

Note that this also updates the pre-commit configuration to check and correct file copyright years. This comes from rapidsai/cudf#14917.  @KyleFromNVIDIA can you confirm whether or not it's OK to do this? It seems to be working fine.

Closes #1345

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Michael Wang (https://github.com/isVoid)

URL: #1349
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice approved these changes

@AyodeAwe AyodeAwe AyodeAwe approved these changes

@karthikeyann karthikeyann karthikeyann approved these changes

@wence- wence- wence- approved these changes

@mroeschke mroeschke Awaiting requested review from mroeschke mroeschke was automatically assigned from rapidsai/cudf-python-codeowners

@charlesbluca charlesbluca Awaiting requested review from charlesbluca charlesbluca was automatically assigned from rapidsai/cudf-python-codeowners

@robertmaynard robertmaynard Awaiting requested review from robertmaynard robertmaynard was automatically assigned from rapidsai/cudf-cpp-codeowners

@PointKernel PointKernel Awaiting requested review from PointKernel PointKernel was automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
cuDF/Dask/Numba/UCX
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@KyleFromNVIDIA @karthikeyann @PointKernel @bdice @gerashegalov @harrism @wence- @AyodeAwe