-
Notifications
You must be signed in to change notification settings - Fork 168
Vendor eudsl in wheels/build (aie.extras) #2811
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
|
This would be amazing if you figure it out! |
…uld prohibit the MLIR AIE Distro action from using a different version
…tion for llvm versioning script
| psutil | ||
| pytest | ||
| pyyaml | ||
| cloudpickle # required by eudsl when it is vendored instead of installed |
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.
This is a super hacky addition that should be able to be deleted in a follow-on PR. It only exists to satisfy the CI. Here's some explanation.
cloudpickle is a dependency of eudsl. Since (in this PR) we start to vendor eudsl, we need to include cloudpickle as a dependency. We do this in requirements.txt. This is not hacky and is correct.
However, the "Build and Test on Ryzen AI" job uses latest wheels to install dependencies. So new dependencies are not reflected in the CI job. So I added cloudpickle here until wheels can be built with the new dependency, at which point it should not be necessary.
I didn't try to fix the workflow dependencies because there is value in testing that dependencies are correctly reflected in the wheels, and I didn't know how to retain that property while fixing this issue.
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 vendors the eudsl-python-extras package into mlir-aie wheels to simplify installation. Previously, eudsl required manual installation with special environment variables, which is now handled automatically during the wheel build process.
Changes:
- Removed manual eudsl installation steps from documentation and CI workflows
- Created vendor_eudsl.py script to download and package eudsl during wheel builds
- Updated build system to ensure consistent MLIR version across workflows
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/mlir_aie_wheels/vendor_eudsl.py | New script to vendor eudsl-python-extras into wheel packages |
| utils/mlir_aie_wheels/setup.py | Integrated eudsl vendoring into the build process and filtered eudsl from install_requires |
| python/requirements.txt | Added eudsl-python-extras with config settings (previously in separate file) |
| python/requirements_extras.txt | Deleted - contents moved to requirements.txt |
| python/requirements_dev.txt | Added cloudpickle dependency required by vendored eudsl |
| python/CMakeLists.txt | Added build step to vendor eudsl for local builds |
| utils/mlir_aie_wheels/scripts/download_mlir.sh | Updated to use consistent MLIR version from clone-llvm.sh |
| Multiple workflow files | Removed manual eudsl installation steps |
| Multiple documentation files | Removed manual eudsl installation instructions |
| utils/setup_python_packages.sh | Removed eudsl installation from setup script |
| utils/quick_setup.sh | Removed eudsl installation from quick setup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I created an alpha release to test the new installation mechanism in projects that build on mlir-aie wheels. I've use this test release in amd/IRON (amd/IRON#68), and it appears to be working well. There is just one thing I've noticed that I am not sure is a feature or a bug. When you run This is an artifact of how I hacked setup.py in the wheels to ignore the extras dependency, but pip still thinks it was installed via pip. This seems like a feature because it accurately reflects the extras version, and it may even be possible to install a different version via normal pip commands (although I haven't tried this as it seems like an edge use case). This seems like a bug because the eudsl-python-extras weren't actually installed via pip. But, seems more useful than not, so I'm leaving it as-is unless I get feedback to do otherwise! |
With this change are do we now use multiple versions of LLVM for MLIR-AIE? |
|
@jgmelber it should be all the same version, as defined in the clone-llvm.sh script |
TL;DR
This PR vendors eudsl into the mlir-aie wheels and fixes some minor flaws in the CI.
Full Explanation
This PR is associated with the desire for a cleaner install of eudsl (Part of this issue: #2805). This should benefit those that use the wheels to install mlir-aie.
This was more difficult than expected. To include eudsl in the normal requirement.txt, I had to ensure pip was updated. This was fairly straightforward.
But what was not straightforward was:
The difficulties were a combination of: