[CI] add pre-commit integration
#955
Conversation
|
👋 Hi! Thank you for contributing to the TileLang project. Please remember to run We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀 |
WalkthroughAdds repository-wide formatting, linting, and pre-commit configurations; updates lint tool versions; adjusts editor and Git attributes; minor CMake formatting; expands contributing docs; adds ignore patterns; fixes comment typos; introduces a new Python example entry point; renames a C++ class with a compatibility alias; and adds noqa comments in a JIT adapter. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
XuehaiPan
force-pushed
the
add-pre-commit-config
branch
from
830dcb9 to
f6d2cc7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.pre-commit-config.yaml (2)
16-24: Clarify or schedule the disabled hooks.Several hooks (
trailing-whitespace,end-of-file-fixer,check-executables-have-shebangs,check-shebang-scripts-are-executable) are commented out with FIXME but lack context about why they're disabled or when they'll be enabled. Consider either enabling them now if the codebase is ready, or documenting the blocker (e.g., "need to fix 200+ existing violations first").
31-38: Bump clang-format to v20.1.0
Update therevin.pre-commit-config.yamlfromv15.0.7tov20.1.0(LLVM 20.1.0, released Mar 11 2025) to gain the latest bug fixes, performance improvements, and full C++20/23 support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.clang-format(1 hunks).editorconfig(1 hunks).pre-commit-config.yaml(1 hunks)CMakeLists.txt(2 hunks)docs/deeplearning_operators/matmul.md(2 hunks)examples/deepseek_v32/fp8_lighting_indexer.py(2 hunks)pyproject.toml(2 hunks)requirements-lint.txt(1 hunks)setup.py(3 hunks)tilelang/jit/adapter/libgen.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.13.3)
tilelang/jit/adapter/libgen.py
67-67: Unused noqa directive (non-enabled: SIM115)
Remove unused noqa directive
(RUF100)
114-114: Unused noqa directive (non-enabled: SIM115)
Remove unused noqa directive
(RUF100)
131-131: Unused noqa directive (non-enabled: SIM115)
Remove unused noqa directive
(RUF100)
231-231: Unused noqa directive (non-enabled: SIM115)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-metal
- GitHub Check: format-check
🔇 Additional comments (10)
docs/deeplearning_operators/matmul.md (1)
11-11: LGTM!The formatting cleanup is appropriate:
- Removed trailing whitespace for consistency
- Added
codespell:ignorecomment to prevent false positives on the PyCUDA documentation URLAlso applies to: 259-259
.editorconfig (1)
17-20: LGTM!The indentation adjustments align well with LLVM-style formatting conventions typically used with clang-format. The explicit CMake rules ensure consistent 2-space indentation across build configuration files.
CMakeLists.txt (1)
59-59: LGTM!Minor formatting improvements that enhance readability without changing functionality.
Also applies to: 202-202
examples/deepseek_v32/fp8_lighting_indexer.py (1)
262-308: LGTM!Excellent addition of a runnable entry point for the FP8 lighting indexer example. The implementation includes:
- Configurable test parameters
- Correctness validation against reference implementation
- Performance profiling and metrics
- Proper script entry point with main guard
This makes the example more accessible and easier to run standalone.
pyproject.toml (1)
27-30: LGTM!The Ruff configuration additions are well-considered:
- Target Python 3.8 matches your project requirements
- Line length of 100 is reasonable and consistent with YAPF config
- Ignoring SIM108 (ternary enforcement) is a sensible readability preference
- Ignoring B905 (zip strict parameter) is necessary for Python 3.8 compatibility since
strictwas added in Python 3.10Note: SIM115 is not in your enabled rule set, which is why the noqa comments in
libgen.pyare flagged as unused.Also applies to: 56-63
setup.py (1)
420-420: LGTM!Excellent corrections that improve code quality:
- Fixed typo:
TileLangBuilPydCommand→TileLangBuildPyCommand- Improved consistency:
TilelangExtensionBuild→TileLangExtensionBuildThe class names now follow consistent naming conventions with proper capitalization.
Also applies to: 646-646, 932-934
requirements-lint.txt (1)
2-4: Linting tool versions validated
All specified versions exist on PyPI and no security advisories were found..clang-format (1)
1-8: LGTM!The clang-format configuration is well-structured with reasonable defaults: LLVM style, 2-space indentation, 80-character line limit, and C++17 standard. This will help maintain consistent C++ code formatting across the codebase.
.pre-commit-config.yaml (2)
1-9: LGTM!The CI configuration and defaults are well-structured. The monthly autoupdate schedule, autofix on PRs, and exclusion of build/3rdparty directories are all sensible choices for a pre-commit setup.
39-59: LGTM!The ruff, yapf, and codespell configurations look appropriate. The version sync comments with requirements-lint.txt, exclusion patterns, and hook arguments are all well-chosen for a Python/C++ mixed codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/op/parallel.cc (1)
218-222: Consider fixing "complicated replicated" to "completely replicated".The typo corrections look good. However, line 218 uses "complicated replicated" which appears inconsistent with the terminology used throughout the codebase (e.g.,
buffer_is_completed_replicatedat line 269,IsCompletedReplicated()at line 281).Apply this diff if "completely replicated" was the intended term:
- // Deduce buffers that should be complicated replicated. + // Deduce buffers that should be completely replicated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/layout/gemm_layouts.cc(1 hunks)src/op/parallel.cc(1 hunks)src/target/codegen_cuda.cc(1 hunks)src/target/ptx.h(1 hunks)src/transform/inject_assumes.cc(1 hunks)src/transform/loop_vectorize_dynamic.cc(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/layout/gemm_layouts.cc
- src/target/codegen_cuda.cc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-metal
- GitHub Check: format-check
🔇 Additional comments (3)
src/target/ptx.h (1)
151-154: Nice doc polish.Thanks for correcting the typo in the comment; keeping these precise helps maintainers.
src/transform/inject_assumes.cc (1)
36-37: Comment typo fix looks good.Clarifies the wording without touching behavior.
src/transform/loop_vectorize_dynamic.cc (1)
246-249: Rename plus alias maintains compatibility.Correcting the spelling while providing a
usingalias keeps existing callers building and improves readability.Also applies to: 271-273, 397-399
|
@youkaichao vLLM also uses pre-commit for formatting. Based on vllm's experience, is this a good design? |
|
would be better for us to add a message for users who doesn't want to enable pre-commit when they were working on a draft commit. |
|
@LeiWang1999 You can install the For how to teach contributors to use this, I will update the contribution docs. That would be very easy: git clone git@github.com:your-fork/tilelang.git
cd tilelang
# install pre-commit hooks before contributing
pre-commit installWith |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/minference/ops/vertical_slash_index.cu (1)
6-6: Remove unnecessary numpy include In examples/minference/ops/vertical_slash_index.cu (line 6), remove the#include <pybind11/numpy.h>—no numpy-specific pybind11 types are used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitattributes(1 hunks)examples/minference/ops/vertical_slash_index.cu(1 hunks)maint/precision/cuda_ops.cu(1 hunks)src/layout/gemm_layouts.cc(1 hunks)src/target/codegen_cuda.cc(1 hunks)src/target/ptx.h(1 hunks)src/tl_templates/cuda/compress_sm90.cu(3 hunks)src/tl_templates/cuda/cuda_bf16_fallbacks.cuh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/target/ptx.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/layout/gemm_layouts.cc
- src/target/codegen_cuda.cc
🔇 Additional comments (7)
.gitattributes (1)
1-8: Good call on line-ending and binary rules.The LF/CRLF split and binary flags match common tooling expectations; this should keep formatters and diffs stable.
maint/precision/cuda_ops.cu (1)
1-294: LGTM! Formatting applied consistently across the file.The changes are purely cosmetic—consistent indentation, spacing, and line-break adjustments with no functional modifications. All function signatures, logic, and public bindings remain identical.
examples/minference/ops/vertical_slash_index.cu (1)
13-144: LGTM! Formatting changes align with pre-commit integration.The reformatting of function signatures, parameter lists, and code blocks throughout the file is consistent with the PR objective of adding pre-commit hooks. The changes improve readability through consistent spacing and alignment without altering the functional logic.
src/tl_templates/cuda/compress_sm90.cu (4)
16-34: LGTM! Formatting improves readability.The error-checking macros have been reformatted for consistent alignment and indentation. The logic remains unchanged and correct.
35-131: LGTM! Consistent formatting throughout the function.The formatting changes improve code readability:
- Type aliases are properly aligned and split across lines
- Comments are reflowed for clarity
- The clarifying comment on line 75 for variable
Nis helpful- Tensor initialization and hardware info query are consistently formatted
No functional changes; the sparse tensor compression logic remains correct.
136-169: LGTM! Macro formatting is now consistent.The dispatch macros have been reformatted with consistent indentation and alignment across all case statements. The compile-time branching logic for block sizes and data types remains correct and unchanged.
171-175: LGTM! Function signature formatting improved.The return type has been moved to a separate line for better readability. The function logic remains unchanged and correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
requirements-lint.txt (1)
2-5: Pinpre-committo keep lint environments reproducible.All other tooling here is version-pinned; leaving
pre-commitfloating means CI/local lint can suddenly start failing as upstream releases new major hooks. Please lock it to a tested version (e.g.,pre-commit==<version>) so the lint stack remains deterministic.CONTRIBUTING.md (1)
104-108: Remove italic placeholders to satisfy markdownlint.Markdownlint’s MD036 warns against treating emphasis as a heading. Replace
_TBA_with plain text (e.g.,TBDorTo be announced) to clear the lint noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CONTRIBUTING.md(2 hunks)requirements-lint.txt(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
CONTRIBUTING.md
104-104: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
108-108: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
XuehaiPan
force-pushed
the
add-pre-commit-config
branch
from
6ae08e9 to
3ca61ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
CONTRIBUTING.md (1)
55-60: Document Windows activation + pip fallback.The current snippet still only shows
source venv/bin/activate, which fails on Windows, and it requiresuv pip ...even after you tell users they can create the venv withpython3 -m venv. Please restore the Windows activation commands and add a plainpip install -r requirements-dev.txtfallback so the workflow succeeds for contributors withoutuv. Example update:uv venv --seed venv # use `python3 -m venv venv` if you don't have `uv` - source venv/bin/activate + # Activate the virtual environment + source venv/bin/activate # Linux/macOS + .\venv\Scripts\Activate.ps1 # Windows PowerShell + venv\Scripts\activate.bat # Windows Command Prompt + python3 -m pip install --upgrade pip setuptools wheel "build[uv]" uv pip install --requirements requirements-dev.txt + # If `uv` is unavailable, fall back to: + python3 -m pip install -r requirements-dev.txt
🧹 Nitpick comments (1)
requirements-lint.txt (1)
2-5: Pinpre-commitfor deterministic lint installs.
Every other lint dependency here is pinned; leavingpre-commitfloating means contributor environments and CI can silently shift under us. Please lock it to a vetted version (or narrow compatible range) so the new workflow stays reproducible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CONTRIBUTING.md(2 hunks)requirements-lint.txt(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
CONTRIBUTING.md
104-104: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
108-108: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test-metal
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.pre-commit-config.yaml (4)
8-8: Limit heavy hooks to pre-commit; keep only quick guards on pre-push.Running all hooks at pre-push slows pushes. Recommend scoping heavy format/lint to pre-commit and reserving pre-push for quick checks (merge-conflict, large-files).
Apply:
-default_stages: [pre-commit, pre-push, manual] +default_stages: [pre-commit, manual] @@ - - id: check-added-large-files + - id: check-added-large-files + stages: [pre-commit, pre-push] @@ - - id: check-merge-conflict + - id: check-merge-conflict fail_fast: true + stages: [pre-commit, pre-push] @@ - - id: clang-format + - id: clang-format + stages: [pre-commit] @@ - - id: ruff-check + - id: ruff-check args: [--fix, --exit-non-zero-on-fix] + stages: [pre-commit] @@ - - id: yapf + - id: yapf args: [--recursive, --in-place] + stages: [pre-commit] @@ - - id: codespell + - id: codespell additional_dependencies: [".[toml]"] + stages: [pre-commit]Also applies to: 19-21, 34-39, 41-49, 51-60
16-18: Enable whitespace hygiene hooks.These catch common nits cheaply; safe to enable.
- # FIXME: enable these hooks - # - id: trailing-whitespace - # - id: end-of-file-fixer + - id: trailing-whitespace + - id: end-of-file-fixer @@ - # FIXME: enable these hooks - # - id: check-executables-have-shebangs - # - id: check-shebang-scripts-are-executable + # Consider enabling later: + # - id: check-executables-have-shebangs + # - id: check-shebang-scripts-are-executableAlso applies to: 22-24
9-9: Broaden global excludes for common generated/vendor dirs.Reduces noise and runtime.
-exclude: '^(build|3rdparty)/.*$' # exclude build and 3rdparty directories +exclude: '^(build|3rdparty|dist|out|_out|_build|docs/_build|node_modules|\.venv|venv|env|site-packages)/.*$'
51-60: Codespell scope and extras.
- Extras: using additional_dependencies: ".[toml]" is okay; alternatively install "codespell[toml]".
- Scope: excluding all C/C++/CUDA files avoids many typos in comments/strings. Consider scanning them and maintaining an ignore-words list instead.
Example to reduce false positives while scanning C/C++:
- exclude: | - (?x)( - ^.+\.(cpp|hpp|cxx|cc|c|h|cu|cuh)$| - ^.+\.svg$| - ^.*\brequirements\b.*\.txt$ - ) + exclude: | + (?x)( + ^.+\.svg$| + ^.*\brequirements\b.*\.txt$ + ) + args: [--ignore-words=.codespell-ignore-words]And add a .codespell-ignore-words file at repo root as needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.pre-commit-config.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-test-metal
- GitHub Check: build-test-amd
- GitHub Check: format-check
🔇 Additional comments (3)
.pre-commit-config.yaml (3)
31-39: Confirm clang-format version alignment.Mirror v15.0.7 is older; ensure it matches your CI/toolchain and .clang-format expectations. Consider bumping to the toolchain’s major version to avoid formatting drift.
Would you verify the clang-format version used in CI and developer environments and align this mirror accordingly?
3-7: Pre-commit CI config looks good.Autofix and monthly autoupdate are sensible defaults.
41-45: Confirm use ofruff-check
Theruff-pre-commitREADME recommendsruff-checkfor linting (with --fix) andruff-formatfor formatting; the legacy aliasruffremains supported.
|
LGTM, thanks @XuehaiPan ！ |
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores
Style
Refactor