[rocprofiler-compute] Make pre-commit a superset of CI formatting#5938
[rocprofiler-compute] Make pre-commit a superset of CI formatting#5938vedithal-amd wants to merge 4 commits into
Conversation
- Add gersemi 0.22.1 and clang-format 22.1.0 hooks to the project pre-commit config, mirroring the CMake and C/C++ formatting checks in rocprofiler-compute-formatting.yml. - Add a hook that rejects staged Python bytecode, mirroring the python-bytecode CI job. - Replace the four ad-hoc CI jobs (python, cmake, cxx, python-bytecode) with a single job that runs pre-commit/action against the project config, so the local hook chain and CI stay in lockstep. Co-Authored-By: Claude Opus 4 (1M context)
github-actions
Bot
added
project: rocprofiler-compute
github actions
|
@jbonnell-amd and @xuchen-amd can you review it |
There was a problem hiding this comment.
Pull request overview
This PR aligns rocprofiler-compute’s local pre-commit formatting hooks with the CI formatting workflow by making the pre-commit configuration a superset of CI’s format checks, then simplifying CI to run pre-commit directly (so “local hooks pass” implies “CI passes”).
Changes:
- Extend
projects/rocprofiler-compute/.pre-commit-config.yamlto include CMake formatting (gersemi), C/C++ formatting (clang-format), and a Python bytecode rejection hook. - Replace the multi-job
rocprofiler-compute-formatting.ymlworkflow with a single SHA-pinned pre-commit job that runs on PR/push diffs via--from-ref/--to-ref. - Update workflow permissions/concurrency conventions to match other repo workflows (read-only contents, improved concurrency grouping, full history checkout).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| projects/rocprofiler-compute/.pre-commit-config.yaml | Adds gersemi + clang-format hooks and a local hook to reject staged Python bytecode. |
| .github/workflows/rocprofiler-compute-formatting.yml | Collapses formatting CI into a single pre-commit-based job with SHA-pinned actions and updated concurrency/permissions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Move pyyaml dep into the hash-check hook via additional_dependencies and switch it to language: python, so CI no longer needs a separate pip install step. - Switch copyright-header-check to language: python and let pre-commit pass file paths as args (no more bash + cd wrapper). Script falls back to git diff --cached for direct dev runs without args. - Make EXCLUDED_DIRS in copyright_header_check.py repo-root-relative to match the paths pre-commit hands in. Co-Authored-By: Claude Opus 4 (1M context)
The references to the removed CI jobs no longer have anything to point at. Mirroring rationale lives in the PR description instead. Co-Authored-By: Claude Opus 4 (1M context)
Add files: ^projects/rocprofiler-compute/ to every hook that wasn't already path-scoped (check-yaml, end-of-file-fixer, trailing-whitespace, ruff-check, ruff-format, no-python-bytecode, copyright-header-check, hash-check). Prevents the workflow from enforcing rocprofiler-compute formatting rules on unrelated projects when a PR touches both. Co-Authored-By: Claude Opus 4 (1M context)
xuchen-amd
left a comment
xuchen-amd
left a comment
There was a problem hiding this comment.
Approving the changes for pre-commit uses, but please note that currently, 1. the script is not working manually 2. no auto sync for pyyaml versions
| language: system | ||
| entry: python projects/rocprofiler-compute/src/utils/hash_checker.py | ||
| language: python | ||
| additional_dependencies: [pyyaml==6.0.3] |
There was a problem hiding this comment.
flagging here that if requirements.txt is updated, this file would need to be updated as well. Potential drift in this case.
There was a problem hiding this comment.
FYI, the removal in this script would mean this script would not work if manually invoked, producing error like:
projects/rocprofiler-compute/<staged file>:
could not read file
| # Use the rocprofiler-compute project pre-commit config and only check | ||
| # files changed in this push/PR. | ||
| extra_args: >- | ||
| --config projects/rocprofiler-compute/.pre-commit-config.yaml | ||
| --from-ref ${{ env.FROM_REF }} | ||
| --to-ref HEAD |
There was a problem hiding this comment.
just flagging that while this is the standard practice, pushing in this change means the team MUST ALL use pre-commit. Otherwise the reduction in scope means wrongly formatted code might not be noticed if slipped (I believe formatting is still not a hard requirement for CI).
Motivation
Make the project pre-commit config a strict superset of the
formatting CI workflow, then collapse the workflow to a single job
that runs pre-commit. If local hooks pass, CI passes.
Technical Details
Add gersemi 0.22.1, clang-format 22.1.0, and a staged-bytecode
rejection hook to projects/rocprofiler-compute/.pre-commit-config.yaml,
pinned to the same versions the CI workflow used.
Replace the four jobs (python, cmake, cxx, python-bytecode) in
rocprofiler-compute-formatting.yml with a single pre-commit job that
runs pre-commit/action@v3.0.1 against the project config, scoped to
changed files via --from-ref/--to-ref.
Adopt rocm-libraries pre-commit workflow conventions (https://github.com/ROCm/rocm-libraries/blob/develop/.github/workflows/pre-commit.yml): SHA-pinned
actions, permissions: contents: read, PR-vs-push concurrency
grouping, fetch-depth: 0 for diff resolution.
Self-contain Python deps for the local hooks (hash-check,
copyright-header-check) via language: python and
additional_dependencies, so CI no longer needs a separate pip
install step. copyright-header-check reads file paths from
pre-commit args, with a git-staged fallback for direct dev runs.
JIRA ID
Test Plan
Open this PR and watch the new pre-commit job run successfully
against the changed files.
Locally: pre-commit run --all-files --config
projects/rocprofiler-compute/.pre-commit-config.yaml reports
gersemi/clang-format clean on rocprofiler-compute sources.
Test Result
Pending CI.
Submission Checklist