[Build][CI] Build and test SDist in release CI #1098
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! 🚀 |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a gated Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Runner as Job Runner
participant Storage as Artifact Storage
participant List as list-artifacts Job
GH->>Runner: trigger build-sdist (gated: owners & non-draft & non-PR)
Runner->>Runner: checkout\nsetup Python & uv (cache)\nsetup ccache\nbuild SDist -> dist/\ncreate temp venv & pip install dist (validate)
alt gated upload allowed
Runner->>Storage: upload SDist artifact
else upload skipped
Note right of Runner: SDist upload skipped (PR/draft/non-owner)
end
GH->>List: start list-artifacts (depends on build-sdist & build-wheels)
List->>Storage: download SDist artifact
List->>Storage: download wheel artifacts
List->>List: enumerate distributions and list artifacts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/dist.yml (1)
68-76: Minor: Consider usingpythonafter venv activation instead ofpython3.After sourcing the venv, using
python(rather thanpython3) is the standard practice and avoids any potential environment mismatch. Additionally, the test could be slightly more robust by verifying the pip install succeeded before assuming the package is installed.Apply this diff for consistency and clarity:
- name: Test SDist buildable run: | TEMP_DIR="$(mktemp -d -t tilelang-sdist-test)" cp -r dist "${TEMP_DIR}/dist" uv run --no-project -m -- venv "${TEMP_DIR}/venv" source "${TEMP_DIR}/venv/bin/activate" cd "${TEMP_DIR}" - python3 -m pip install --upgrade pip setuptools wheel - python3 -m pip install -v dist/*.tar.gz + python -m pip install --upgrade pip setuptools wheel + python -m pip install -v dist/*.tar.gz
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/dist.yml(2 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: Test for Python 3.12 with Metal (on macos-latest)
- GitHub Check: Build wheels for Python 3.8 on ubuntu-latest with CUDA-12.1
- GitHub Check: Build wheels for Python 3.8 on macos-latest with Metal
- GitHub Check: Build wheels for Python 3.8 on ubuntu-24.04-arm with CUDA-12.8
- GitHub Check: Build SDist
🔇 Additional comments (3)
.github/workflows/dist.yml (3)
32-86: Overall structure and setup look solid.The new
build-sdistjob follows a sensible pattern: checkout → setup environment → build → validate → upload. The conditions, timeouts, and platform choice (macOS for ccache) are all reasonable. The test step that validates the SDist can be installed is a valuable safety check.
39-40: Confirm theNO_VERSION_LABELlogic is intentional.The environment variable is set to
'OFF'for release events and'ON'for all other events (PRs, scheduled runs). This appears intentional—disabling version labeling only for official releases. Please confirm this semantic is correct for your versioning scheme.Also applies to: 110-111
163-163: Dependency ordering and artifact download logic are correct.The
list-artifactsjob properly depends on bothbuild-sdistandbuild-wheels, ensuring they complete before attempting to download artifacts. Downloading the SDist first (before wheels) is a sensible ordering. The artifact structure looks correct (SDist intodist/, wheels merged intodist/).Also applies to: 166-172
XuehaiPan
force-pushed
the
sdist
branch
2 times, most recently
from
2750f12 to
9f3c856
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/dist.yml (2)
67-76: Consider using standard venv creation and add cleanup.The SDist validation has a couple of areas for improvement:
Line 71: The command
uv run --no-project -m -- venvis unconventional. Consider using the standard approach:uv venv "${TEMP_DIR}/venv"orpython3 -m venv "${TEMP_DIR}/venv".Temp directory cleanup: The temporary directory is never explicitly cleaned up. While the OS will eventually clean it, consider adding a cleanup step or using a trap to ensure cleanup on exit.
Validation depth: The test only imports the module and prints the version. Consider running a basic smoke test or quick test suite to validate the SDist more thoroughly.
Example improvement:
- name: Test SDist buildable run: | TEMP_DIR="$(mktemp -d -t tilelang-sdist-test)" + trap "rm -rf '${TEMP_DIR}'" EXIT cp -r dist "${TEMP_DIR}/dist" - uv run --no-project -m -- venv "${TEMP_DIR}/venv" + python3 -m venv "${TEMP_DIR}/venv" source "${TEMP_DIR}/venv/bin/activate" cd "${TEMP_DIR}" python3 -m pip install --upgrade pip setuptools wheel python3 -m pip install -v dist/*.tar.gz python3 -c "import tilelang; print(tilelang.__version__)" + # Optional: Add basic smoke test + # python3 -m pytest --version
166-172: Clarify comment about artifact behavior.The comment on lines 169-170 states "if
name: artifactis omitted, the action will create extra parent dir", but thename: sdistparameter IS specified in this step. This comment might be confusing for future maintainers.Consider clarifying:
- name: Download built SDist uses: actions/download-artifact@v5 with: - # unpacks default artifact into dist/ - # if `name: artifact` is omitted, the action will create extra parent dir + # Downloads sdist artifact and unpacks into dist/ name: sdist path: distMANIFEST.in (1)
35-35: Remove*.pxdfrom the compiled file exclusion pattern.Cython
.pxdfiles are source definition files (declaration/header-like files) — not build artifacts. The currentglobal-excludepattern on line 35 incorrectly excludes them. While no.pxdfiles currently exist in the repository, removing this pattern prevents potential issues if declaration files are added in the future to support your Cython module attilelang/jit/adapter/cython/cython_wrapper.pyx.Update line 35 to:
global-exclude *~ *.py[cod] *.so *.a *.dylib *.dll *.lib *.o *.obj
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/dist.yml(2 hunks)MANIFEST.in(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). (5)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
- GitHub Check: Build wheels for Python 3.8 on macos-latest with Metal
- GitHub Check: Build wheels for Python 3.8 on ubuntu-24.04-arm with CUDA-12.8
- GitHub Check: Build wheels for Python 3.8 on ubuntu-latest with CUDA-12.1
- GitHub Check: Build SDist
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)
pyproject.toml(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). (5)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
- GitHub Check: Build wheels for Python 3.8 on ubuntu-24.04-arm with CUDA-12.8
- GitHub Check: Build wheels for Python 3.8 on ubuntu-latest with CUDA-12.1
- GitHub Check: Build wheels for Python 3.8 on macos-latest with Metal
- GitHub Check: Build SDist
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pyproject.toml (1)
77-85: Exclude patterns for clang and llvm will not take effect due to include precedence.The exclude patterns on lines 82–83 use non-standard glob syntax (
3rdparty/clang**instead of3rdparty/clang/**), but more importantly, they won't exclude the intended directories because include rules (line 73:3rdparty/**) take precedence in scikit-build-core. The patterns should either:
- Remove
3rdparty/**from includes and explicitly list only needed subdirectories, or- Use negation patterns like
!3rdparty/clang/**within the include list itself.Additionally, correct the glob patterns to use the gitignore-style syntax:
3rdparty/clang/**instead of3rdparty/clang**.Suggested fix:
exclude = [ ".git", ".github", "**/.git", "**/.github", - "3rdparty/clang**", - "3rdparty/llvm**", + "3rdparty/clang/**", + "3rdparty/llvm/**", "build", ]However, note that this alone won't exclude clang/llvm if
3rdparty/**remains in the include list (line 73). Consider refactoring the include strategy to explicitly list only the 3rdparty subdirectories you want included.
🧹 Nitpick comments (1)
pyproject.toml (1)
92-95: Clarify or defer the TODO regarding wheel package inclusions.The TODO on line 92 signals uncertainty about whether benchmark, examples, and testing should be included in the wheel distribution. This should either be:
- Resolved with a documented decision (if intentional, add a brief comment explaining why), or
- Deferred to a follow-up issue/PR with clear acceptance criteria.
For now, if there's no strong rationale to include these in the wheel, consider removing them from the mappings to keep the distribution lean for end users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyproject.toml(2 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). (6)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
- GitHub Check: Test for Python 3.12 with CUDA-12.8 (on self-hosted-nvidia)
- GitHub Check: Build wheels for Python 3.8 on macos-latest with Metal
- GitHub Check: Build wheels for Python 3.8 on ubuntu-latest with CUDA-12.1
- GitHub Check: Build wheels for Python 3.8 on ubuntu-24.04-arm with CUDA-12.8
- GitHub Check: Build SDist
🔇 Additional comments (1)
pyproject.toml (1)
176-178: Smoke test for package import and version looks good.The test-command is a simple but effective validation step to catch import errors and ensure version metadata is accessible during CI wheel builds. This is a good practice.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pyproject.toml (1)
61-85:⚠️ Unresolved critical issue from past review: include precedence over exclude patterns.This flagged issue persists from the previous review. In scikit-build-core,
sdist.includepatterns take precedence oversdist.exclude. Since line 73 includes3rdparty/**, the exclude patterns on lines 82-83 (3rdparty/clang**and3rdparty/llvm**) won't exclude those directories.Additionally, the glob pattern syntax is unconventional; should use
3rdparty/clang/**(not3rdparty/clang**) to match directory contents under gitignore-style patterns.To actually exclude these large third-party build artifacts, either:
- Remove
3rdparty/**from the include list and explicitly include only needed subdirectories, or- Use negation in the include list:
!3rdparty/clang/**and!3rdparty/llvm/**Please confirm whether clang and llvm directories should be excluded from the SDist, and if so, update the configuration to use one of the approaches above.
🧹 Nitpick comments (1)
pyproject.toml (1)
87-95: TODO comment suggests design uncertainty—confirm benchmark/examples/testing in wheels.The expanded package mappings include benchmark, examples, and testing directories in the wheel. The TODO comment on line 92 indicates the team is uncertain whether these should be included.
Consider documenting the reasoning: Are these needed for end-users? If not, removing them would reduce wheel size.
Would you like me to help document the design decision or generate a configuration to conditionally exclude these from the wheel?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyproject.toml(3 hunks)
🔇 Additional comments (3)
pyproject.toml (3)
6-7: Author and maintainer metadata updates look good.Clean reformatting with proper project metadata structure.
171-176: CI configuration additions look appropriate.The skip pattern, environment setup, and before-build debugging are standard for CUDA-aware projects. The environment-pass for CUDA_VERSION aligns with the conditional CUDA setup below.
182-182: Linux CUDA PATH configuration is appropriate.Adding
/usr/local/cuda/binto PATH aligns with the CUDA driver installation and build requirements.
| "CMakeLists.txt", | ||
| "src/**", | ||
| "cmake/**", | ||
| "3rdparty/**", |
There was a problem hiding this comment.
scikit-build needs to include 3rdparty into the SDist explicitly. By default, it only glob for files according to .gitignore. cc @LeiWang1999 @oraluben
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the build and CI pipeline to properly build, test, and validate source distributions (SDist) alongside existing wheel builds. It introduces a dedicated SDist build job, improves packaging metadata, and refines build tool configurations.
Key Changes:
- Adds a new GitHub Actions job to build and test SDist on macOS with artifact upload
- Expands MANIFEST.in with proper include/exclude rules for licenses, tests, and examples
- Updates pyproject.toml with SDist configuration and improved cibuildwheel settings
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| .github/workflows/dist.yml | Adds build-sdist job with versioning controls, buildability test in isolated venv, artifact upload, and updates artifact listing to download SDist before wheels |
| MANIFEST.in | Restructures with comments, explicit license/version includes, graft/prune directives for organized SDist content, and global excludes for build artifacts |
| pyproject.toml | Adds [tool.scikit-build.sdist] section mirroring MANIFEST.in rules, expands package mappings, and consolidates cibuildwheel environment settings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/dist.yml (1)
79-79: Comment inconsistency: "only needed for releases" vs. actual condition logic.Lines 79, 155, and 166 use comments stating artifacts are "only needed for releases," but the condition
github.event_name != 'pull_request'also matches scheduled (cron) runs on line 5. Either update the comments to reflect both scheduled and release events, or restrict the condition to releases only if that's the intended behavior.- # Not PR to save artifact storage, as SDist is only needed for releases. + # Not PR to save artifact storage, as SDists are only needed for releases and scheduled builds.Can you clarify whether artifacts should be uploaded on both scheduled and release events, or only on releases?
Also applies to: 155-155, 166-166
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/dist.yml(3 hunks)
🔇 Additional comments (2)
.github/workflows/dist.yml (2)
112-116: Well-documented versioning environment variables.The detailed comment explaining
NO_GIT_VERSIONand its relationship to version consistency between SDist and built packages is clear and helpful. This addresses a prior concern about documenting these configuration options.
71-71: Invaliduv venvflag:--seeddoes not exist.The
uv venvcommand supports flags like--pythonto specify versions, but--seedis not a documented flag. This will cause the venv creation to fail. Remove the flag or replace it with a valid one if intended.- uv venv --seed "${TEMP_DIR}/venv" + uv venv "${TEMP_DIR}/venv"Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 (3)
.github/workflows/ci.yml(1 hunks).github/workflows/dist.yml(2 hunks)pyproject.toml(3 hunks)
🔇 Additional comments (6)
pyproject.toml (1)
170-176: Verify environment variable alignment between ci.yml and dist.yml.The new
environment-passlist on lines 171–176 passes COLUMNS, CUDA_VERSION, FORCE_COLOR, and CLICOLOR_FORCE to cibuildwheel. Ensure this aligns with the environment settings in.github/workflows/ci.yml(which now includesCOLUMNS: "100"at line 25) and.github/workflows/dist.yml(which defines similar variables at lines 31–36).Confirm that the environment variables needed by cibuildwheel are consistently defined across all workflow files and that there are no missing or conflicting settings.
.github/workflows/ci.yml (1)
25-25: ✓ LGTM – environment variable addition for consistent log formatting.The addition of
COLUMNS: "100"aligns with the same setting added todist.yml(line 34) and supports structured output formatting for CI logs..github/workflows/dist.yml (4)
39-97: ✓ Well-structured SDist build job with proper gating and validation.The new
build-sdistjob is well-designed:
- Properly gated to the tile-ai org and non-draft PRs
- Includes clear documentation for NO_GIT_VERSION (lines 48–51) explaining version consistency issues
- Uses standard
python -m build --sdistpattern (line 70)- Validates SDist buildability in an isolated temp venv (lines 79–88)
- Correctly uses
uv venv --seedfor reproducible venv setup- Conditionally uploads only for non-PR events to save artifact storage
31-36: Environment variables properly set up for SDist and wheel builds.The new env section (lines 31–36) aligns with ci.yml and includes COLUMNS for consistent log formatting. The NO_VERSION_LABEL and NO_GIT_VERSION settings in the build-sdist job environment (lines 46–52) are well-documented and address version consistency concerns.
172-175: Updated job dependency and comment are accurate.The
list-artifactsjob now correctly:
- Depends on both
build-sdistandbuild-wheels(line 175)- Has an updated comment (line 172) that accurately reflects artifact-level reasoning for PR skipping
178-184: Version mismatch is intentional and appropriate for different use cases.The SDist download step uses v5 (with
name: sdistfor a single named artifact), while the wheels download step uses v6 (withpattern: wheels-*andmerge-multiple: true). The v6merge-multiplefeature is required for downloading multiple patterned artifacts; v5 does not support this parameter. Both versions are compatible with their respective artifact configurations, and the breaking changes in v6 (Node v24.x requirement, internal dependency bumps) do not affect artifact naming or path handling. The version mismatch is intentional design, not an oversight.Likely an incorrect or invalid review comment.
2 participants
Summary by CodeRabbit
Chores
Documentation / Packaging