-
Notifications
You must be signed in to change notification settings - Fork 754
Mlc thor support Thor and Orin #1320
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
base: dev
Are you sure you want to change the base?
Conversation
|
LGTM (GH200) |
…CUDA 13 updates
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.
Pull Request Overview
This PR updates the environment variable setup for MLC and TVM packages to ensure build completion on Thor and Orin platforms. It introduces a new Thor-specific configuration and refactors the TVM build system to use external TVM dependencies.
- Refactors TVM package to support both pip installation and source builds with proper environment setup
- Updates MLC to use external TVM dependencies instead of bundled TVM for Thor platform
- Adds Thor-specific MLC configuration with SM110 CUDA architecture support
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ml/tvm/config.py | Introduces TVM package configuration function with commit/version support |
| packages/ml/tvm/install.sh | New script for attempting pip install before falling back to source build |
| packages/ml/tvm/build.sh | New source build script with CUDA architecture configuration |
| packages/ml/tvm/Dockerfile | Refactored to use new install/build scripts and proper environment setup |
| packages/ml/tvm/test.py | Added success confirmation message |
| packages/llm/mlc/config.py | Added Thor-specific configuration and TVM dependency |
| packages/llm/mlc/build.sh | Updated to use external TVM and simplified build process |
| packages/llm/mlc/test.sh | Fixed module invocation and benchmark path |
| packages/llm/mlc/benchmark.py | Improved API compatibility detection |
| packages/llm/mlc/Dockerfile | Updated environment variables for external TVM |
| packages/llm/mlc/patches/empty.diff | Empty patch file for Thor configuration |
| packages/cuda/cuda-python/utils.py | Added compatibility for cuda-python 13.x layout |
| packages/cuda/cuda-python/test_runtime.py | Updated version detection and import handling |
| packages/cuda/cuda-python/test_driver.py | Fixed context management and improved compatibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
|
|
||
|
|
||
|
|
Copilot
AI
Aug 24, 2025
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.
[nitpick] Remove the trailing blank lines at the end of the file to improve code cleanliness.
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
Copilot
AI
Aug 24, 2025
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.
[nitpick] This file contains only empty lines. Consider using a truly empty file or adding a comment explaining why this empty patch is needed for the Thor configuration.
| # This empty patch file is required for Thor configuration. No changes are needed. |
|
@johnnynunez did you try running on Orin? If so, what configurations? |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
|
@OriNachum I ran the build on Orin with this command |
Flashinfer is not compatible > 0.2.9 with cuda 12.6. |
7b784f1 to
b31c062
Compare
|
@kbenkhaled i just upload new flashinfer version |
|
@kbenkhaled i added recently cuda 13 support in tvm. It is working with cutlass 4.2. Also i fixed flash attention builds for cuda 13. This PR should work now @OriNachum |
|
@johnnynunez Thank you, I opened this PR because it was not working on orin, but I think it should work now as per my previous fix from #1315 |
Yes, i will try it. It can be cleaned with new flash infer 3.1 |
Follow-up to #1315: Ensure build env vars are set correctly
Summary
This PR is a follow-up to #1315 and updates the environment-variable setup to ensure the build completes successfully.
Testing
Thor: Build completes successfully.
Orin: Not yet tested.