Skip to content

ci: align pre-commit hooks and GitHub templates - #127

Closed
mc-nv wants to merge 16 commits into
mainfrom
mchornyi/TRI-1100/github-align-hooks-and-templates
Closed

ci: align pre-commit hooks and GitHub templates#127
mc-nv wants to merge 16 commits into
mainfrom
mchornyi/TRI-1100/github-align-hooks-and-templates

Conversation

@mc-nv

@mc-nv mc-nv commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

What does the PR do?

Aligns this repository with the org-wide setup consolidated in triton-inference-server/.github:

  • .pre-commit-config.yaml: shared baseline hooks, conventional-commit message validation (commit-msg stage), and the centralized add-license hook from the org .github repository (rev: v1.4.3 — excludes .github/ templates, never rewrites LICENSE files).
  • Pre-commit CI runs only on files modified by the PR.
  • Caller stub for the org-wide reusable conventional-pr workflow (@v1.4.3): validates the PR title against Conventional Commits (hard gate — it becomes the squash-merge commit), derives one human-readable label per distinct type found in the title and all conforming commit subjects (e.g. ci:CI/CD, feat:feature, fix:fix), enforces org-wide label colors/descriptions, detects cherry-picks, and fails if no type is derivable and no type label is assigned.
  • No per-repo templates: PR templates and issue routing are inherited from the org-wide defaults in triton-inference-server/.github (issues route to the server repository).

Repo-specific: keeps this repo's deliberate Ruff formatter/linter + pyupgrade toolchain (per review feedback); the centralized add-license hook is NOT enabled because this repository has no LICENSE file — follow-up: add a LICENSE, then enable it.

Depends on triton-inference-server/.github#5 (pinned tags current: v1.4.3 — already exist, CI is green).

Pros / Cons

Pros

  • Single home (org .github repo) for hooks, templates, issue routing, and the PR-title workflow — one change propagates everywhere.
  • Repos carry only a config file and two small workflow stubs; no template copies to drift.
  • Commit/PR title format enforced both locally (commit-msg hook) and in CI (reusable workflow), with automatic type labels.
  • License hooks never modify LICENSE files.

Cons / risks

  • Version-pinned dependency on the org .github repository (tags are write-once; changes ship as a new tag + rev bump).
  • Template inheritance requires the org .github repository to remain public.

Related Issues / PRs

  • Resolves: TRI-1100

Related PRs:

Test plan

  • pre-commit validate-config passes; pre-commit run --files <PR diff> passes locally with the centralized hooks pinned to the .github branch SHA.
  • After .github#5 merges and v1.4.3 exists: the conventional-pr check validates this PR's own title and applies the ci label.
  • No LICENSE file content is changed by this PR.

Caveats

  • None beyond the merge-order note above.

Checklist

  • PR title follows <commit_type>: <Title> (conventional commit)
  • I ran pre-commit locally on all files changed by this PR and it passes
  • Copyright header is correct on all changed files
  • External contributors: I have read the Contribution guidelines and signed the Contributor License Agreement

Adopt the shared pre-commit baseline: two-line SPDX header, conventional
commit message validation (commit-msg stage), and the centralized
add-license / add-spdx-license hooks from developer_tools v0.2.0.
Run pre-commit CI only on files modified by the PR, and roll out the
standard issue templates and the simplified single PR template.

TRI-1100
@mc-nv mc-nv self-assigned this Jul 18, 2026
This was referenced Jul 18, 2026
Legal's Copyright / License Header Guidance specifies the SPDX form
without a comma after the year.

TRI-1100
@mc-nv
mc-nv marked this pull request as ready for review July 18, 2026 02:05
@mc-nv
mc-nv requested a review from nvda-mesharma as a code owner July 18, 2026 02:05
@mc-nv
mc-nv requested a review from mudit-eng July 18, 2026 02:19
@github-actions github-actions Bot added the chore Maintenance work, no production code change (chore: PRs) label Jul 20, 2026
@Vinya567

Copy link
Copy Markdown
Contributor

Was the Ruff → black/isort/flake8 swap intentional for TRI-1100? Has pre-commit run --all-files been run to confirm no surprise formatting diffs?

Review feedback on #127: the swap to isort/black/flake8 was not
intentional - this repository's Ruff formatter/linter and pyupgrade
setup is deliberate and stays. Only the shared alignment additions
apply: commit-msg conventional-commit validation and
default_install_hook_types.

TRI-1100
@mc-nv

mc-nv commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Good catch — the Ruff → black/isort/flake8 swap was not intentional. This repo's Ruff formatter/linter + pyupgrade setup is a deliberate toolchain choice (with the trt_llm excludes), and the alignment should have preserved it the same way it preserved perf_analyzer's newer revs and client's mypy. Fixed in ab760e4: the config is now main's Ruff/pyupgrade toolchain verbatim, plus only the shared additions (commit-msg conventional-commit validation, default_install_hook_types).

On --all-files: it was not run repo-wide, by design — CI checks only the files a PR modifies, and the fleet-wide policy avoids mass reformatting. For the record I did measure the impact of the (now-reverted) swap before answering: black/isort would have wanted changes in roughly half a dozen files, i.e. real churn — which the revert makes moot. With Ruff kept, the PR diff passes all hooks cleanly and no repo-wide formatting change is introduced.

@Vinya567

Copy link
Copy Markdown
Contributor

Good catch — the Ruff → black/isort/flake8 swap was not intentional. This repo's Ruff formatter/linter + pyupgrade setup is a deliberate toolchain choice (with the trt_llm excludes), and the alignment should have preserved it the same way it preserved perf_analyzer's newer revs and client's mypy. Fixed in ab760e4: the config is now main's Ruff/pyupgrade toolchain verbatim, plus only the shared additions (commit-msg conventional-commit validation, default_install_hook_types).

On --all-files: it was not run repo-wide, by design — CI checks only the files a PR modifies, and the fleet-wide policy avoids mass reformatting. For the record I did measure the impact of the (now-reverted) swap before answering: black/isort would have wanted changes in roughly half a dozen files, i.e. real churn — which the revert makes moot. With Ruff kept, the PR diff passes all hooks cleanly and no repo-wide formatting change is introduced.

Thanks for the fix. Verified ab760e4 keeps Ruff/pyupgrade verbatim and only adds the shared alignment pieces (commit-msg hook, conventional-pr stub, modified-files CI). LGTM.

Vinya567
Vinya567 previously approved these changes Jul 21, 2026
The add-license hook now fails when the LICENSE copyright year is
stale.

TRI-1100
Workflow files are license-processed again (only templates excluded);
refresh the stale copyright year this repo's pre-commit workflow
carried.

TRI-1100
Grant issues:write to the labeling job (review feedback).

TRI-1100
@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown

Greptile Summary

This PR aligns the repository with the org-wide CI/tooling baseline defined in triton-inference-server/.github: a new thin caller workflow for conventional commit/PR-title validation, a reworked pre-commit workflow that runs only on PR-modified files, and a commit-msg stage hook for local conventional-commit enforcement.

  • conventional-pr.yml: New caller stub delegates to the org reusable workflow (@v1.4.3); uses the dual-event (pull_request / pull_request_target) conditional pattern to give fork PRs write access for labeling while keeping same-repo PRs on the safer event.
  • pre-commit.yml: Replaces the blanket pre-commit/action run with a scoped git diff | xargs pre-commit run --files invocation; --diff-filter=d skips deleted paths and --no-run-if-empty handles deletion-only PRs cleanly.
  • .pre-commit-config.yaml: Activates the commit-msg hook stage via default_install_hook_types and adds the conventional-pre-commit hook to enforce Conventional Commits locally.

Confidence Score: 5/5

Safe to merge — all changes are CI configuration; no application code is touched and the dual-event workflow guard is correctly constructed.

The three changed files are CI/tooling config only. The conventional-pr caller correctly isolates same-repo vs fork triggers. The pre-commit workflow scoping logic (diff-filter, no-run-if-empty, null-delimited xargs) is sound. The one finding — an unpinned major-version tag on actions/cache — is a consistency nit that does not affect correctness.

.github/workflows/pre-commit.yml — the actions/cache@v4 pin is the only item worth a second look for consistency with the fully-pinned neighbouring actions.

Important Files Changed

Filename Overview
.github/workflows/conventional-pr.yml New caller stub for the org-wide conventional-pr reusable workflow; uses the correct dual-event / conditional pattern to gate same-repo vs fork PRs and grants only the minimum permissions needed for label management.
.github/workflows/pre-commit.yml Switched from the pre-commit action to a manual install + scoped run; correctly handles deleted files (--diff-filter=d) and empty diffs (--no-run-if-empty). actions/cache is pinned to a mutable @v4 tag while the other actions use full semver pins.
.pre-commit-config.yaml Adds default_install_hook_types to activate commit-msg stage and installs the conventional-pre-commit hook at that stage; existing hooks are unchanged.

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant GH as GitHub Actions
    participant OrgWF as org .github reusable workflow (v1.4.3)
    participant PC as pre-commit

    Dev->>GH: Opens / updates PR
    GH->>GH: conventional-pr.yml triggers (pull_request or pull_request_target)
    GH->>GH: if-condition selects correct event for same-repo vs fork
    GH->>OrgWF: "uses: conventional-pr.yml@v1.4.3"
    OrgWF->>GH: Validates PR title (Conventional Commits)
    OrgWF->>GH: "Derives & applies type label (ci, feat, fix ...)"

    GH->>GH: pre-commit.yml triggers (pull_request)
    GH->>GH: "git diff --name-only -z --diff-filter=d HEAD^1 HEAD"
    GH->>PC: xargs -0 --no-run-if-empty pre-commit run --files changed-files
    PC-->>GH: Hook results (ruff, pyupgrade, codespell, pre-commit-hooks)

    Dev->>Dev: git commit (local)
    Dev->>PC: commit-msg hook fires (conventional-pre-commit v4.4.0)
    PC-->>Dev: Pass / fail on commit message format
Loading

Reviews (3): Last reviewed commit: "ci: harden CI workflows and configs per ..." | Re-trigger Greptile

Comment thread .github/workflows/conventional-pr.yml Outdated
Comment thread .github/workflows/pre-commit.yml Outdated
- conventional-pr stub: dual pull_request/pull_request_target triggers
  so fork PRs from external contributors get labeled too (the reusable
  workflow never checks out PR code); explicit contents:read; pinned
  v1.4.3.
- pre-commit workflow: robust modified-files runner (null-delimited
  paths, deletion-only PRs handled, deleted paths filtered, no
  undocumented -r flag, cache keyed on config hash).
- flake8 args quoted correctly (the flow-scalar form split at commas
  and silently reduced the select list).
- hooks pinned to .github v1.4.3.

TRI-1100

@Vinya567 Vinya567 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

@mc-nv

mc-nv commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Closing: the team is moving away from centralized org-level configuration in favor of per-repository self-contained setups (see triton-inference-server/server#8897 for the first decentralized implementation). Branch retained for reference. TRI-1100

@mc-nv mc-nv closed this Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance work, no production code change (chore: PRs) CI/CD Continuous integration and workflow changes (ci: PRs)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants