-
Couldn't load subscription status.
- Fork 286
[Build] Migrate to scikit-build-core #939
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.
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
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! 🚀 |
WalkthroughMigrates packaging to scikit-build-core, modernizes CMake (≥3.26) and TVM integration, removes legacy setup/tox/install scripts, requires a prebuilt cython_wrapper at import, centralizes third‑party discovery and dynamic version metadata, updates CI/Docker to uv/cibuildwheel flows, and adds a Dist GitHub Actions workflow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant GH as GitHub Actions
participant Repo as Repository
participant UV as uv
participant CIBW as cibuildwheel / scikit-build-core
participant CMake as CMake (>=3.26)
participant TVM as TVM (load_tvm.cmake)
participant Repair as auditwheel/delocate
participant Store as Artifact Store
Dev->>GH: push / release
GH->>Repo: checkout + submodules
GH->>UV: setup Python env & install build deps via uv
GH->>CIBW: run cibuildwheel (matrix)
CIBW->>CMake: configure & build (load_tvm.cmake, cython_wrapper)
CMake->>TVM: resolve TVM_SOURCE / INCLUDES
CMake-->>CIBW: produce raw wheel(s)
CIBW->>Repair: repair wheel(s)
GH->>Store: upload artifact(s)
sequenceDiagram
autonumber
participant User as pip / installer
participant SB as scikit-build-core
participant CMake as CMake
participant TVM as TVM
User->>SB: pip install -v .
SB->>CMake: configure & build targets (tilelang_module, cython_wrapper, ...)
CMake->>TVM: load via cmake/load_tvm.cmake
TVM-->>CMake: provide headers/targets
CMake-->>SB: built artifacts
SB-->>User: package installed / wheel produced
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 actionlint (1.7.7).github/workflows/dist.yml20-20: label "macos-16" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file (runner-label) ⏰ 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)
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 |
6009644 to
76e9df7
Compare
3fda5c0 to
e813cec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/dist.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/dist.yml
43-43: property "repair" is not defined in object type {}
(expression)
44-44: property "repair" is not defined in object type {}
(expression)
Co-authored-by: Xuehai Pan <XuehaiPan@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/dist.yml (1)
15-24: ProvideCUDA_VERSIONfor every matrix leg.
env.CUDA_VERSIONreadsmatrix.CUDA_VERSION, but the macOS row never defines it. On that leg GitHub errors when resolving the expression. Add a matrix default (e.g.,CUDA_VERSION: "") or an include entry for macOS so the key always exists; the snippet below sets a default.Apply this diff:
strategy: matrix: + CUDA_VERSION: [""] os: [ubuntu-22.04, ubuntu-22.04-arm, macos-14] include: - os: ubuntu-22.04 CUDA_VERSION: 12.1 - os: ubuntu-22.04-arm CUDA_VERSION: 12.8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/dist.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/dist.yml
26-26: could not parse as YAML: yaml: line 26: did not find expected key
(syntax-check)
🪛 YAMLlint (1.37.1)
.github/workflows/dist.yml
[error] 33-33: syntax error: expected , but found '-'
(syntax)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
version_provider.py (1)
59-61: Consider defensive parsing of CUDA_VERSION.The unpacking assignment
major, minor, *_ = cuda_version.split('.')assumes at least two dot-separated components. While CUDA versions typically follow this format, adding validation would make the code more robust against unexpected input.Apply this diff to add defensive parsing:
if cuda_version := os.environ.get('CUDA_VERSION'): - major, minor, *_ = cuda_version.split('.') - backend = f'cu{major}{minor}' + parts = cuda_version.split('.') + if len(parts) >= 2: + backend = f'cu{parts[0]}{parts[1]}' + else: + backend = 'cuda' else: backend = 'cuda'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/dist.yml(1 hunks)version_provider.py(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/dist.yml
67-67: property "repair" is not defined in object type {ls-whl: {conclusion: string; outcome: string; outputs: {string => string}}}
(expression)
🪛 Ruff (0.13.3)
version_provider.py
23-23: Starting a process with a partial executable path
(S607)
36-36: Unused function argument: settings
(ARG001)
⏰ 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). (5)
- GitHub Check: build-test-metal
- GitHub Check: build-test-amd
- GitHub Check: build-wheels (ubuntu-22.04)
- GitHub Check: build-wheels (macos-14)
- GitHub Check: build-wheels (ubuntu-22.04-arm)
Needed-by: tile-ai/tilelang#939 Currently, tvm will build limited api cython library for 3.12+, even if we're targeting 3.8+ in tilelang. This workaround just relax the version. This issue should be solved by future tvm-ffi integration. This part is no longer in upstream tvm so I submitted here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/dist.yml(1 hunks)3rdparty/tvm(1 hunks)pyproject.toml(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- 3rdparty/tvm
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/dist.yml
52-52: property "repair" is not defined in object type {ls-whl: {conclusion: string; outcome: string; outputs: {string => string}}}
(expression)
⏰ 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). (5)
- GitHub Check: build-test-metal
- GitHub Check: build-test-amd
- GitHub Check: build-wheels (ubuntu-22.04-arm)
- GitHub Check: build-wheels (ubuntu-22.04)
- GitHub Check: build-wheels (macos-14)
|
This should be ready for review. Main parts that this PR touched:
|
Co-authored-by: Xuehai Pan <XuehaiPan@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/dist.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/dist.yml
53-53: property "repair" is not defined in object type {ls-whl: {conclusion: string; outcome: string; outputs: {string => string}}}
(expression)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/dist.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/dist.yml
21-21: label "macos-16" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
⏰ 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-wheels (ubuntu-22.04-arm)
- GitHub Check: build-wheels (ubuntu-22.04)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Resolves #833
Closes #756
Detailed information could be found at the doc
This PR basically migrates to a fully cmake-based build system and eliminates python-based building process, and therefore simplifies the building and installing stage.
User and developers of tilelang could just install from source (without setting
PYTHONPATHand so on).cp38-abi3wheels for >= python 3.8 (needs Workaround limit api too high in tvm tvm#12)Unify CUDA and ROCm wheel? Is that possible?FixRemove in-tree jit compileNow tilelang reads version (for cache, etc.) from the package version.
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores