-
Notifications
You must be signed in to change notification settings - Fork 333
[Build] Prefer libs from local build dir #1027
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! 🚀 |
WalkthroughUpdates build config (disable libbacktrace when USE_METAL; Darwin requires torch>=2.7; add scikit-build-core), expands installation docs with a faster developer rebuild workflow, and changes env.py to derive a development THIRD_PARTY_ROOT with warning and an assertion when libs are missing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant App as Application
participant Env as tilelang.env
participant FS as FileSystem
Dev->>App: Start / import
App->>Env: Load env.py
Env->>FS: Check THIRD_PARTY_ROOT exists?
alt present
Env-->>App: Use THIRD_PARTY_ROOT and TL_LIBS
else missing
rect rgb(230,245,255)
note right of Env: Development-mode resolution (new)
Env->>FS: Derive dev_root from TL_ROOT
Env->>FS: Compute dev_lib_root and tvm subpath
Env-->>Env: Set THIRD_PARTY_ROOT = dev 3rdparty path
Env-->>Env: Define TL_LIBS = [dev_lib_root, dev_lib_root/tvm, ...]
Env->>FS: Assert any(path in TL_LIBS exists)
end
Env-->>App: Emit warning and provide TL_LIBS
end
App-->>Dev: Continue init
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
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 (3)
pyproject.toml (1)
40-41: Consider consolidating the torch dependency.Both
"torch"and"torch>=2.7; platform_system == 'Darwin'"appear in the dependencies list. On Darwin systems, both will be evaluated, making the first line redundant.Consider replacing both lines with a single conditional dependency for clarity:
- "torch", - "torch>=2.7; platform_system == 'Darwin'" + "torch>=2.7; platform_system == 'Darwin'", + "torch; platform_system != 'Darwin'"This makes the platform-specific requirement explicit and avoids redundancy.
docs/get_started/Installation.md (2)
259-264: Add language specifier to the code block.As per coding guidelines (markdownlint MD040), fenced code blocks should specify a language for proper syntax highlighting.
Apply this diff:
-``` +```bash pip install -r requirements-dev.txt pip install -e . -v --no-build-isolation cd build; ninja--- `268-271`: **Add language specifier to the console output block.** As per coding guidelines (markdownlint MD040), fenced code blocks should specify a language. Use `console` or `bash` for command output. Apply this diff: ```diff -``` +```console $ python -c 'import tilelang' 2025-10-14 11:11:29 [TileLang:tilelang.env:WARNING]: Loading tilelang libs from dev root: /Users/yyc/repo/tilelang/build</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 7a5077e4aa8e30533b6fe1f0716b2c28cf6f661b and d944c58a7f44b885f05f63b4ee462bdce633dd76. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `CMakeLists.txt` (1 hunks) * `docs/get_started/Installation.md` (1 hunks) * `pyproject.toml` (1 hunks) * `requirements-dev.txt` (1 hunks) * `tilelang/env.py` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/get_started/Installation.md</summary> 259-259: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 268-268: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ 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)</summary> * GitHub Check: build-test-amd * GitHub Check: format-check </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>requirements-dev.txt (1)</summary><blockquote> `8-8`: **LGTM!** The addition of `scikit-build-core` aligns with the build system requirements defined in `pyproject.toml` and supports the faster developer rebuild workflow. </blockquote></details> <details> <summary>tilelang/env.py (1)</summary><blockquote> `34-41`: **LGTM! Development mode logic is correct.** The code appropriately switches to development mode when the 3rdparty root is missing: 1. Computes the dev library root from the parent directory 2. Sets `TL_LIBS` to prefer local build directories over installed packages 3. Updates `THIRD_PARTY_ROOT` to point to the development 3rdparty location 4. Emits a clear warning message for debugging 5. Asserts that at least one library path exists, preventing silent failures This implementation aligns well with the PR objective of enabling faster recompilation for developers. **Note:** In development mode (line 37), `TL_LIBS` is completely replaced, ignoring any site-packages paths that were set on lines 25-26. This is intentional and correct for the dev workflow. </blockquote></details> <details> <summary>CMakeLists.txt (1)</summary><blockquote> `78-79`: **Confirm disabling libbacktrace is an acceptable fix** The FIXME tags a temporary workaround for CIBW wheel failures; disabling backtrace support may degrade debugging in Metal builds. Investigate why CIBW fails with libbacktrace enabled (missing dependency, Metal toolchain incompatibility, or packaging issue) and restore libbacktrace once resolved. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
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: 0
🧹 Nitpick comments (1)
docs/get_started/Installation.md (1)
259-271: Add languages to the new code fences.markdownlint (MD040) flags these fences for missing language identifiers. Annotating them keeps the docs lint‑clean and improves syntax highlighting.
Apply this diff:
-``` +```bash pip install -r requirements-dev.txt pip install -e . -v --no-build-isolation cd build; ninjaWhen running in editable/developer mode,
you'll see similar logs like below:
-+console
$ python -c 'import tilelang'
2025-10-14 11:11:29 [TileLang:tilelang.env:WARNING]: Loading tilelang libs from dev root: /Users/yyc/repo/tilelang/build
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/get_started/Installation.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/get_started/Installation.md
259-259: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
268-268: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/get_started/Installation.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/get_started/Installation.md
259-259: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
268-268: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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-amd
- GitHub Check: format-check
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/get_started/Installation.md(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-amd
- GitHub Check: format-check
- GitHub Check: format-check
* Load libs from build dir, if present, to support faster rebuild. * typo * upd * refine check * md lint
This enable to run faster recompile without heavy
pip install.Add log to reduce ambiguity:
Also add torch requirement for metal as in #1021 (comment)
Summary by CodeRabbit
New Features
Documentation
Chores