Skip to content

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

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

ci: align pre-commit hooks and GitHub templates#861
mc-nv wants to merge 18 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: the tensorrt_llm submodule is excluded from all hooks; Python toolchain aligns from yapf/autoflake to the shared baseline (files reformat only when touched); adds pyproject.toml; the stale local bug_report.yml is removed (issues route to server); the repo's safety hooks from main (check-added-large-files, check-symlinks, detect-private-key) are preserved.

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
Human-readable type labels with enforced descriptions and colors.

TRI-1100
@github-actions github-actions Bot added CI/CD Continuous integration and workflow changes (ci: PRs) and removed ci labels Jul 20, 2026
@github-actions github-actions Bot added the chore Maintenance work, no production code change (chore: PRs) label Jul 20, 2026
mc-nv added 4 commits July 20, 2026 17:40
check-added-large-files, check-symlinks, and detect-private-key were
deliberately configured on main; the baseline alignment must not drop
them. (The top-level ^tensorrt_llm/ exclude covers the submodule, so
the per-hook exclude from main is no longer needed.)

TRI-1100
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
@mc-nv
mc-nv requested a review from pskiran1 July 22, 2026 15:12
@Vinya567

Copy link
Copy Markdown

@greptileai

@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown

Greptile Summary

This PR aligns the repository's CI and pre-commit configuration with the org-wide setup in triton-inference-server/.github, replacing local copies of hooks, templates, and workflows with thin callers pinned to versioned tags.

  • Pre-commit overhaul: replaces yapf/autoflake with black/flake8, promotes the tensorrt_llm exclusion to a global top-level exclude, adds conventional-commit message validation (commit-msg stage), and introduces the org-wide add-license hook. A new pyproject.toml centralises codespell and isort configuration.
  • CI workflow updates: pre-commit.yml is rewritten to run only on PR-modified files using null-delimited xargs -0 (correctly handles paths with spaces); a new conventional-pr.yml thin caller delegates PR-title validation and auto-labelling to the reusable org-wide workflow at v1.4.3.
  • Template cleanup: the local bug_report.yml is deleted; issue routing is inherited from the org-wide .github defaults.

Confidence Score: 5/5

Safe to merge — all changes are confined to CI configuration and tooling; no production code is touched.

The diff is limited to pre-commit and GitHub Actions configuration. The previously flagged issues (flake8 arg quoting, xargs null-delimiter, version tag) have been addressed. The one remaining finding is a minor regex anchor in the codespell exclude pattern that does not affect hook correctness for the common case. No runtime logic or business-critical paths are affected.

.pre-commit-config.yaml — minor codespell exclude regex issue; all other files are straightforward.

Important Files Changed

Filename Overview
.pre-commit-config.yaml Major overhaul replacing yapf/autoflake with black/flake8, adding conventional-commit and add-license hooks, and promoting the tensorrt_llm exclusion to a global top-level exclude. Flake8 args and the pre-commit stages are correctly configured. One minor regex issue in the codespell exclude pattern.
.github/workflows/pre-commit.yml Rewrites the pre-commit CI to run only on PR-modified files, using null-delimited xargs (-0) to correctly handle paths with spaces, filtering deleted files with --diff-filter=d, and adding pre-commit cache. Logic is sound.
.github/workflows/conventional-pr.yml New thin-caller workflow invoking the org-wide conventional-pr reusable workflow at v1.4.3. The pull_request_target guard (fork PRs only, metadata-only workflow) is correctly conditioned and documented.
.github/ISSUE_TEMPLATE/bug_report.yml Deleted in favour of routing issues to the server repository via the org-wide GitHub defaults. Straightforward removal.
pyproject.toml New file centralising codespell and isort configuration, replacing scattered inline args. Settings are consistent with the pre-commit hook invocations.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[PR opened / pushed] --> B{Same-repo or fork?}
    B -->|Same-repo| C[pull_request event]
    B -->|Fork| D[pull_request_target event]
    C --> E[conventional-pr job]
    D --> E
    E --> F[Calls org reusable workflow\ntriton-inference-server/.github\n@ v1.4.3]
    F --> G[Validates PR title\nConventional Commits format]
    F --> H[Applies type label\nci / feat / fix / ...]

    A --> I[pre-commit job\npull_request event]
    I --> J[actions/checkout\nfetch-depth: 2]
    J --> K[git diff HEAD^1 HEAD\n--diff-filter=d -z]
    K --> L[xargs -0\npre-commit run --files]
    L --> M{Hooks pass?}
    M -->|Yes| N[CI green]
    M -->|No| O[CI fails\nshows diff]

    subgraph pre-commit hooks
        P[isort + black + flake8]
        Q[clang-format]
        R[codespell --toml pyproject.toml]
        S[conventional-pre-commit\ncommit-msg stage]
        T[pre-commit-hooks\ncheck-yaml / detect-private-key / ...]
        U[add-license\norg .github @ v1.4.3]
    end
    L --> P & Q & R & S & T & U
Loading

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

Comment thread .pre-commit-config.yaml Outdated
Comment thread .github/workflows/pre-commit.yml Outdated
Comment on lines +41 to +45
run: echo "modified_files=$(git diff --name-only -r HEAD^1 HEAD | xargs)" >> $GITHUB_OUTPUT
- uses: actions/setup-python@v6.0.0
- uses: pre-commit/action@v3.0.1
with:
extra_args: --files ${{ steps.modified-files.outputs.modified_files }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Space-split filenames break --files for paths containing spaces. xargs converts newlines to spaces, so --files ${{ steps.modified-files.outputs.modified_files }} is shell-word-split by the runner — any file path with a space becomes two separate (not-found) arguments. A safer approach passes the newline-separated list directly via a multi-line env variable or uses xargs -d ' ' to preserve path boundaries.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the v1.4.3 round: the workflow now uses a null-delimited pipeline (git diff --name-only -z --diff-filter=d | xargs -0 --no-run-if-empty pre-commit run --files ...) - paths with spaces survive, deletion-only PRs skip cleanly, deleted paths are filtered, and nothing is interpolated through YAML extra_args anymore. pre-commit env caching is preserved via actions/cache keyed on the config hash.

Comment thread .pre-commit-config.yaml Outdated
Comment on lines +93 to +95
rev: v1.4.2
hooks:
- id: add-license

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Version tag v1.4.2 in the code does not match the PR description. The description consistently states rev: v1.4.1 as the pinned tag that "already exists" and has "CI is green," while both this file and conventional-pr.yml reference v1.4.2. If v1.4.2 is intentional, the description should be updated to avoid confusion; if v1.4.1 was the intended pin, the rev here needs correcting.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed - documentation drift: the descriptions have been refreshed to the current pinned tag v1.4.3 (they lagged behind the tag revisions; the tag exists and CI is green fleet-wide).

- 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

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