-
Notifications
You must be signed in to change notification settings - Fork 160
Reduce assumptions about xrt-smi path #2685
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: main
Are you sure you want to change the base?
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
utils/env_setup.sh
Outdated
|
|
||
| XRTSMI=`which xrt-smi` | ||
| if ! test -f "$XRTSMI"; then | ||
| # TODO(erika): should this be removed? |
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.
yes. as long as it is clear in the README that xrt-smi needs to be available on the user's PATH
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.
The test command for xrt-smi is already in both the main README and the building from scratch page. I tried to make it look a little more explicit too!
utils/env_setup.sh
Outdated
| export PATH=${MLIR_AIE_INSTALL_DIR}/bin:${PATH} | ||
| export PYTHONPATH=${MLIR_AIE_INSTALL_DIR}/python:${PYTHONPATH} | ||
| export LD_LIBRARY_PATH=${MLIR_AIE_INSTALL_DIR}/lib:${LD_LIBRARY_PATH} | ||
| export LD_LIBRARY_PATH=${MLIR_AIE_INSTALL_DIR}/lib:${LD_LIBRARY_PATH}:/usr/lib/python3 |
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.
Why?
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.
When you install xrt from the debs, a .so gets put in site packages. Python needs the site packages in the LD_LIBRARY_PATH to find this shared object file, as far as I could tell.
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.
I can see this one backfiring with venvs.
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.
Yes, I had to change the command for creating the venv in the README to allow the venv to see/access site packages. I don't think this is a great solution, so I'm happy to hear other suggestions!
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 makes me want to never use the debs and continue building it myself.
The ideal solution would be to fix the upstream packaging so that one could 'apt install xdna-driver' then 'pip install pyxrt' for their venv and python version, with a looser coupling between the driver and xrt layers. The next best solution might be for mlir-aie to include the bindings as part of its build/wheel.
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.
Yes, I had to change the command for creating the venv in the README to allow the venv to see/access site packages. I don't think this is a great solution, so I'm happy to hear other suggestions!
This can break a lot of other things. mlir-aie is just a part of a toolchain and assuming site-packages AFAIK can force specific package versions which other parts of the toolchain may not like. That was the case in the past with cmake being part of the requirements.txt for example.
I think the the right thing here is to allow local package installation in editable mode via pip install -e . but I haven't used it when libs are needed.
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.
@ypapadop-amd I didn't pursue the pip install method because I'm not sure we assume a user would know the location of the debs? I also haven't used debs in this manner, so I'd need to check it. I could look into how we could migrate just this one site package into the venv... but that still sounds a little messy.
@fifield I think those solutions both sound better. I'm not sure what steps need to be taken for either one.
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.
Maybe I'm missing something. Why would the user need to know the location? The XRT setup script is already taking care of PYTHONPATH and LD_LIBRARY_PATH for XRT and pyxrt.
env_setup.sh only manipulates LD_LIBRARY_PATH and PYTHONPATH because mlir-aie is not installed in the vevn; if you pip install mlir-aie there's no need for either.
|
I appreciate the feedback, all! I think what I'm going to do for now is revert the changes to the venv and LD_LIBRARY_PATH. This means that this solution will NOT work out of the box with the new install method. I think a good solution will be more complex, and need more thought. This PR will then just include changes avoid hard-coded paths. |
Small follow on to #2683