Reduce assumptions about xrt-smi path #2685
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.
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.
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.
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.
I can see this one backfiring with venvs.
There was a problem hiding this comment.
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.
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.
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.
@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.
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. |
hunhoffe
changed the title
hunhoffe
requested review from
AndraBisca,
andrej,
erwei-xilinx,
jackl-xilinx,
pvasireddy-amd and
stephenneuendorffer
as code owners
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Small follow on to #2683