[Release] Unify local build scripts to use cibuildwheel and reduce size of sdist
#1171
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! 🚀 |
|
Warning Rate limit exceeded@oraluben has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughDeletes Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(250,240,230)
Note over CI: Old flow — build-all invoked per-arch Docker buildx scripts
CI->>docker_build_all.sh: run
docker_build_all.sh->>docker_local_distribute.sh: invoke & log
docker_local_distribute.sh->>docker buildx: setup & per-arch builds
docker_build_all.sh->>docker_pypi_distribute.sh: invoke & log
docker_pypi_distribute.sh->>docker buildx: setup & per-arch builds
docker buildx-->>CI: per-arch artifacts (relocated)
end
rect rgb(230,250,240)
Note over CI: New flow — direct cibuildwheel invocation
CI->>cibuildwheel: invoke (CIBW_ARCHS/CIBW_BUILD/NO_VERSION_LABEL)
cibuildwheel->>manylinux container: build wheels (arch-aware)
manylinux container-->>cibuildwheel: produced wheels
cibuildwheel-->>CI: artifacts in dist/
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
maint/scripts/docker_pypi_distribute.sh (1)
4-20: Fix bash syntax error and clarify docker vs. cibuildwheel usage.Line 15 contains a stray
donekeyword with no corresponding loop—this is a syntax error. More critically, the script sets up Docker buildx infrastructure (lines 4–14) but then invokescibuildwheeldirectly (line 20) without using Docker. This is illogical and suggests incomplete refactoring from the multi-architecture Docker build to cibuildwheel-based distribution.Option 1 (Recommended if using cibuildwheel): Remove all docker setup; simplify to:
-if docker buildx version >/dev/null 2>&1; then - if docker info >/dev/null 2>&1; then - docker run --rm --privileged tonistiigi/binfmt --install amd64,arm64 >/dev/null 2>&1 || true - fi - - if ! docker buildx inspect multi >/dev/null 2>&1; then - docker buildx create --name multi --driver docker-container --use >/dev/null 2>&1 || true - else - docker buildx use multi >/dev/null 2>&1 || true - fi - docker buildx inspect --bootstrap >/dev/null 2>&1 || true - done - - export CIBW_ARCHS='x86_64 aarch64' -fi +export CIBW_ARCHS='x86_64 aarch64' NO_VERSION_LABEL=ON CIBW_BUILD='cp38-*' cibuildwheel .Option 2 (if docker buildx is needed): Use
docker buildx buildand remove barecibuildwheelcall; this would require further changes to invoke cibuildwheel inside the build context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
MANIFEST.in(1 hunks)maint/scripts/docker_build_all.sh(0 hunks)maint/scripts/docker_local_distribute.sh(1 hunks)maint/scripts/docker_pypi_distribute.sh(1 hunks)maint/scripts/pypi.manylinux.Dockerfile(2 hunks)pyproject.toml(3 hunks)
💤 Files with no reviewable changes (1)
- maint/scripts/docker_build_all.sh
⏰ 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 wheels for Python 3.9 on ubuntu-latest with CUDA-12.1
- GitHub Check: Build wheels for Python 3.9 on macos-latest with Metal
- GitHub Check: Build wheels for Python 3.9 on ubuntu-24.04-arm with CUDA-12.8
- GitHub Check: Build SDist
- GitHub Check: Quick Lint
🔇 Additional comments (3)
maint/scripts/docker_local_distribute.sh (1)
1-5: ✓ Straightforward simplification.This script correctly delegates to cibuildwheel for the local architecture. The refactoring from multi-arch Docker orchestration to a single invocation aligns well with the PR's goal to unify build scripts.
maint/scripts/pypi.manylinux.Dockerfile (1)
1-29: ✓ Multi-architecture Docker setup with architecture-aware CUDA versioning.The multi-stage build pattern correctly isolates x86_64 and aarch64 configurations with appropriate CUDA repository selections and versions. The dynamic stage selection via
FROM builder_${TARGETARCH}is well-executed. Environment setup properly exposes CUDA tooling via PATH and LD_LIBRARY_PATH for the build process.Note: CUDA repository URLs (
developer.download.nvidia.cnfor x86_64,developer.download.nvidia.comfor aarch64) are external dependencies. Verify these remain stable and accessible; consider documenting failure modes if repositories become unavailable.pyproject.toml (1)
221-238: ✓ Architecture-aware CUDA version selection.The per-architecture DEFAULT_CUDA_VERSION logic (12.1 for x86_64, 12.8 for aarch64) is well-structured and aligns with the Dockerfile setup. The bash substitution correctly prioritizes the CUDA_VERSION environment variable with a sensible fallback, and the version parsing correctly extracts major.minor for YOLOX package selection.
|
is |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pyproject.toml (1)
90-97: Critical: Broad 3rdparty exclude defeats explicit includes.Line 95 adds a blanket
3rdparty/**exclude that will negate all the selective 3rdparty includes on lines 80-86. In scikit-build-core, excludes typically override includes, resulting in an empty 3rdparty in the sdist and defeating the PR's goal to control sdist size through selective vendoring.Replace the broad exclude with specific prune rules:
exclude = [ ".git", ".github", "**/.git", "**/.github", - "3rdparty/**", + "3rdparty/clang*", + "3rdparty/llvm*", "build", ]This ensures only unwanted subdirectories (clang/llvm) are excluded while the paths specified on lines 80-86 remain in the sdist.
🧹 Nitpick comments (4)
pyproject.toml (1)
63-64: Consider documenting the debug flags.These commented build flags can be helpful for troubleshooting. Consider adding a brief note in the repository's build documentation explaining when developers should uncomment them.
version_provider.py (3)
26-41: Consider error handling for file operations.The git_pin mechanism correctly implements caching and persistence, but file operations on lines 36 and 39 lack error handling. If
git_pin.write_text()orgit_pin.read_text()fail due to permissions or I/O errors, the function could raise an unhandled exception during the build.Wrap file operations in try-except blocks:
@lru_cache(maxsize=1) def get_git_commit_id() -> str | None: """Get the current git commit hash by running git in the current file's directory.""" r = subprocess.run(['git', 'rev-parse', 'HEAD'], cwd=ROOT, capture_output=True, encoding='utf-8') if r.returncode == 0: _git = r.stdout.strip() - git_pin.write_text(_git) + try: + git_pin.write_text(_git) + except OSError: + pass # Non-fatal: proceed with git hash even if cache write fails return _git elif git_pin.exists(): - return git_pin.read_text().strip() + try: + return git_pin.read_text().strip() + except OSError: + return None else: return None
52-53: Consider clarifying the side-effect intent.The call to
get_git_commit_id()on line 53 is intentionally triggering a side effect (writing to_git_commit.txtvia line 36) rather than using the return value. While functional, this implicit behavior could be made clearer for maintainability.Consider making the intent more explicit:
- # generate git version for sdist - get_git_commit_id() + # Cache git commit and persist to file for sdist builds + # (The cached value will be reused below on line 82) + get_git_commit_id()Alternatively, refactor to make the side effect explicit:
- # generate git version for sdist - get_git_commit_id() + # Persist git commit to file for sdist builds (where git may be unavailable) + if git_hash := get_git_commit_id(): + # File already written in get_git_commit_id() + pass
84-85: Update Installation.md to document the 'gitunknown' fallback case.The
gitunknownsuffix added at lines 84-85 is PEP 440 compliant and handled correctly by version comparison tools usingpackaging.version.parse(). However, the documentation atdocs/get_started/Installation.md(line 242) only shows the case where git hash is available (<sdk>.git<git_hash>). Users building without git access will see versions like0.1.6.post1+cu116.gitunknown, which should be documented.Update line 242 to mention both cases:
where local version label could look like <sdk>.git<git_hash> when git is available, or <sdk>.gitunknown when unavailable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)MANIFEST.in(0 hunks)pyproject.toml(4 hunks)version_provider.py(5 hunks)
💤 Files with no reviewable changes (1)
- MANIFEST.in
⏰ 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). (4)
- GitHub Check: Build SDist
- GitHub Check: Build wheels for Python 3.9 on macos-latest with Metal
- GitHub Check: Build wheels for Python 3.9 on ubuntu-24.04-arm with CUDA-12.8
- GitHub Check: Build wheels for Python 3.9 on ubuntu-latest with CUDA-12.1
🔇 Additional comments (6)
.gitignore (1)
105-107: LGTM! Git versioning file correctly ignored.The ignore rule for
_git_commit.txtis necessary for the git_pin mechanism introduced inversion_provider.py(line 36), which persists the git commit hash for sdist builds.pyproject.toml (3)
68-89: LGTM! Clear vendored dependency specifications.The explicit 3rdparty includes are well-documented, and the comment clarifying alignment between sdist and wheel contents is helpful.
99-114: LGTM! Explicit wheel mappings align with sdist includes.The wheel package mappings correctly mirror the selective 3rdparty includes from the sdist configuration, and the addition of TVM's
version.pyensures proper runtime versioning.
220-243: LGTM! Architecture-specific CUDA versions are appropriate.The per-architecture CUDA version defaults (12.1 for x86_64, 12.8 for aarch64) correctly align with the different manylinux base images and their compiler requirements. The fallback logic on line 240 properly uses the environment variable or the appropriate default.
version_provider.py (2)
7-7: LGTM! Import correctly supports the caching decorator.
12-17: LGTM! Well-documented git_pin mechanism.The comments clearly explain the pip version matching issue and the rationale for persisting the git hash to a file during sdist builds.
oraluben
changed the title
cibuildwheel and reduce size of sdistcibuildwheel and reduce size of sdist
yes, this should be included |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pyproject.toml (1)
78-95: Verify that selective 3rdparty includes work with the broad exclude.This concern was previously flagged: Line 95's broad
3rdparty/**exclude may negate the selective includes on lines 78-86. In scikit-build-core, excludes are typically processed after includes, which would remove all 3rdparty content despite the explicit includes.The previous review suggested replacing line 95's
"3rdparty/**"with specific excludes like"3rdparty/clang*"and"3rdparty/llvm*"to preserve the selective vendoring.Verify the actual sdist contents to confirm whether the intended 3rdparty subdirectories (tvm, cutlass, composable_kernel) are included:
#!/bin/bash # Description: Build sdist and verify 3rdparty contents are as expected # Build the sdist python -m build --sdist --outdir dist/ # Extract and list 3rdparty contents in the most recent sdist latest_sdist=$(ls -t dist/*.tar.gz | head -1) echo "Examining sdist: $latest_sdist" tar -tzf "$latest_sdist" | grep "3rdparty/" | head -50 echo "---" echo "Checking for expected subdirectories:" tar -tzf "$latest_sdist" | grep -E "3rdparty/(tvm|cutlass|composable_kernel)/" | head -20
🧹 Nitpick comments (3)
pyproject.toml (1)
63-64: Optional: Document when to enable debug flags.These commented debug flags could be helpful for troubleshooting build issues. Consider adding a brief comment explaining when developers should uncomment them (e.g., "Uncomment for debugging build issues").
version_provider.py (2)
7-7: Consider the implications of caching a function with side effects.The
@lru_cachedecorator is applied toget_git_commit_id(), which writes togit_pin(line 36). Typically, cached functions should be pure, but here:
- The git hash shouldn't change during a build, making caching appropriate
- The side effect (writing git_pin) only needs to happen once
- The early call at line 53 ensures git_pin is created when needed
While this works correctly for the use case, the pattern may be unclear to maintainers. Consider adding a comment near the decorator explaining that the side effect is intentional and happens once per process.
Also applies to: 26-26
52-53: Clarify the purpose of this call.The function is called here solely for its side effect (writing
git_pin), not for its return value. The comment "generate git version for sdist" is slightly misleading—it's actually persisting the git hash to a file.Consider renaming the comment to better reflect the purpose:
- # generate git version for sdist + # Persist git commit to .git_commit.txt for sdist builds get_git_commit_id()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyproject.toml(4 hunks)version_provider.py(5 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). (4)
- GitHub Check: Build SDist
- GitHub Check: Build wheels for Python 3.9 on ubuntu-latest with CUDA-12.1
- GitHub Check: Build wheels for Python 3.9 on ubuntu-24.04-arm with CUDA-12.8
- GitHub Check: Build wheels for Python 3.9 on macos-latest with Metal
🔇 Additional comments (4)
version_provider.py (3)
12-17: LGTM! Clear documentation of the git_pin approach.The comment effectively explains why a separate file is needed to store git commit info for sdist builds (to avoid pip version mismatch complaints). This provides valuable context for future maintainers.
35-41: LGTM! Robust fallback mechanism for git commit retrieval.The implementation correctly handles three scenarios:
- Git available: write hash to git_pin for future use
- Git unavailable but git_pin exists: read cached hash (enables sdist builds)
- No git and no cache: return None gracefully
This design allows building from sdist without requiring git.
84-85: Verify the intentional behavior change: adding 'gitunknown' suffix.Previously, when no git hash was available, the version string would omit the git component entirely (e.g.,
0.1.0+cuda). Now it explicitly appendsgitunknown(e.g.,0.1.0+cuda.gitunknown).This change makes missing git info explicit, which aids debugging, but alters the version format. Confirm this is the intended behavior, especially since downstream tooling may parse version strings.
pyproject.toml (1)
228-240: All verification points confirmed.CUDA 12.1 is available in the x86_64/RHEL7 repository (12.1.0-1 and 12.1.1-1) and CUDA 12.8 is available in the aarch64/RHEL8 repository (12.8.0-1 and 12.8.1-1). CUDA 12.8 officially supports GCC 14 as a host compiler, and CUDA 12.1 is officially supported on CentOS 7.x (x86_64). The architecture-specific CUDA defaults are well-founded and appropriately chosen.
| environment.PATH = "/usr/local/cuda/bin:$PATH" | ||
| environment.LD_LIBRARY_PATH = "/usr/local/cuda/lib64:/usr/local/cuda/lib64/stubs:$LD_LIBRARY_PATH" | ||
| # Pin to glibc 2.17 for x86 and 2.28 for aarch64 for now | ||
| manylinux-x86_64-image = "manylinux2014" # CentOS 7 |
There was a problem hiding this comment.
Out of curiosity, is there any reason not to use manylinux_2_28 for x86_64? GLIBC 2.28 was released 7 years ago. The EOL-ed Ubuntu 20.04 has GLIBC 2.31.
There was a problem hiding this comment.
The only reason is that torch migrated from manylinux 2.17 to manylinux 2.28 since 2.6. I guess torch 2.4/2.5 is not that old to be dropped.
There was a problem hiding this comment.
The shared libraries in our wheel are not linked against torch's, right?
There was a problem hiding this comment.
That's correct. We only align with torch's glibc version here.
There was a problem hiding this comment.
That's correct. We only align with torch's glibc version here.
I think there will not be any runtime effects as they are not linked against each other. And there will be only one libc library used in the process, which is very likely 2.28+.
If you persist, could you add a TODO comment in pyproject.toml for this choice?
There was a problem hiding this comment.
I'm fine with that, since that change is quite independent and small, maybe we can open an issue to see if that's ok for other users cc @LeiWang1999
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pyproject.toml (1)
63-64: Commented debugging flags noted.Lines 63–64 contain commented-out verbose build flags. These can be useful during development but should typically be removed before release. Consider documenting their purpose or removing them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/dist.yml(4 hunks)pyproject.toml(5 hunks)
🔇 Additional comments (5)
.github/workflows/dist.yml (2)
47-51: Environment variable rename improves clarity.The rename from
NO_GIT_VERSIONtoNO_VERSION_LABELwith the expanded comment better documents intent. This change aligns well with the PR's goal to persist git info separately.
91-91: Conditional logic consistently enables release PRs to upload artifacts.The three upload/list steps (lines 91, 159, 169) are now consistent, allowing both non-PR events and PRs with "[Release]" in the title to produce artifacts. This enables developers to iterate on release builds without relying on local tools.
Also applies to: 159-159, 169-169
pyproject.toml (3)
66-89: Sdist includes correctly explicit about vendored 3rdparty paths.The explicit inclusion of specific 3rdparty subdirectories (tvm, cutlass, composable_kernel) on lines 80–86 aligns with the PR goal of reducing sdist size. Per scikit-build-core documentation,
sdist.includetakes precedence oversdist.exclude, so the broad3rdparty/**exclude on line 95 will not override these explicit includes.
99-114: Wheel package mappings now mirror sdist vendored content.The explicit mappings for vendored 3rdparty components (lines 106–114) keep wheel and sdist consistent, ensuring runtime access to TVM and other vendored libraries.
215-241: Per-architecture CUDA version defaults improve manylinux consistency.Includes take precedence over excludes, so introducing per-arch
DEFAULT_CUDA_VERSION(line 229 for x86_64, line 233 for aarch64) and computingcudaverfrom it (line 241) is a clear improvement. The TODO comment on line 216 appropriately flags the manylinux version choice for future review.Verify that the
DEFAULT_CUDA_VERSIONvalues (12.1 for x86_64, 12.8 for aarch64) align with the CI runner images and CUDA availability in manylinux_2014 and manylinux_2_28.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/dist.yml(4 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). (5)
- GitHub Check: Quick Lint
- GitHub Check: Build wheels for Python 3.9 on macos-latest with Metal
- GitHub Check: Build SDist
- GitHub Check: Build wheels for Python 3.9 on ubuntu-latest with CUDA-12.1
- GitHub Check: Build wheels for Python 3.9 on ubuntu-24.04-arm with CUDA-12.8
🔇 Additional comments (4)
.github/workflows/dist.yml (4)
47-51: Verify NO_VERSION_LABEL semantics for sdist.The comment states that
NO_VERSION_LABELdisables embedding the git commit hash to prevent version inconsistency between the sdist and installed packages. However, line 51 setsNO_VERSION_LABEL: 'OFF'. If'OFF'means "disable disabling" (i.e., enable embedding), this contradicts the goal stated in the comment. Confirm whether sdist should use'ON'to suppress git hash embedding, or if the naming convention is inverted from what the comment suggests.To clarify the semantics, can you confirm how
NO_VERSION_LABELis interpreted inversion_provider.py? Specifically:
- Does
NO_VERSION_LABEL='ON'suppress version labels (git hash)?- Does
NO_VERSION_LABEL='OFF'allow version labels (git hash)?If so, line 51 should likely be
NO_VERSION_LABEL: 'ON'to match the intent of avoiding git hash suffixes in the sdist version.
91-91: Upload conditions correctly allow PR-triggered releases.The conditions on lines 91, 159, and 169 now properly allow artifact uploads for PRs with
"[Release]"in the title, while still excluding regular PR uploads to save storage. This aligns well with the PR objective to enable downloading wheels/sdist from PR artifacts instead of building locally.Also applies to: 159-159, 169-169
119-119: Clarify NO_VERSION_LABEL logic for wheel builds.Line 119 sets
NO_VERSION_LABELto'OFF'for release events and'ON'otherwise. This appears intentional—release builds get clean versions without git hash suffixes, while PR/dev builds may have them for clarity. However, this should be validated against the actual behavior inversion_provider.pyto ensure consistency with the sdist handling.
91-91: Artifact upload conditions correctly enable PR-based releases.All three upload/list conditions (lines 91, 159, 169) now allow PRs with
"[Release]"in the title to upload artifacts while excluding regular PR uploads to conserve storage. This implementation cleanly supports the PR objective of enabling on-demand wheel/sdist downloads from PR artifacts.Also applies to: 159-159, 169-169
…size of sdist (tile-ai#1171) * update exclude in sdist * reuse cibw workflow in maint * update * fix * fmt * upload artifacts for [Release] PRs * dot-prefix version file * update
3 participants
"[Release]", then we can download built wheel/sdist from artifacts, instead of use qemu on local machine. This also produces macos wheels, example: https://github.com/tile-ai/tilelang/actions/runs/19007774631?pr=1171 @LeiWang1999 ;.git_commit.txtin sdist.pipdo not allow a sdist to have different version with the installed package (we cannot have a sdist namedtilelang-0.1.6.post2+gitxxxx.tar.gzand install a package namedtilelang==0.1.6.post2+cu128.gitxxxx). Since sdist should not contain toolchain info, we need to store its git commit separately. When installed from sdist, the git info will be read from that file.cibuildwheelto generate wheels for linux. However this seems not necessary, I hope Github Actions can eliminated most of local builds.cc @XuehaiPan
Summary by CodeRabbit