Skip to content

Conversation

@dchigarev
Copy link
Contributor

@dchigarev dchigarev commented Jan 23, 2025

Closes #3147

The main goal of this PR was to get rid of icpx dependency while discovering sycl lib and sycl headers, and to also align this logic with AMD and Nvidia backends.

The flow of discovering libsycl.so in driver.py is now the following:

  1. Check TRITON_LIBSYCL_PATH env variable
  2. Check ldconfig -p for libsycl.so
  3. Check LD_LIBRARY_PATH (usually sycl would be found here if oneapi vars are set)
  4. Check $ONEAPI_ROOT/compiler/latest/lib
  5. Check intel-sycl-rt python package
  6. Check $CONDA_PREFIX/lib path
  7. If libsycl.so wasn't found in any of these locations, assume that icpx will be used that already knows the proper sycl location.

The sycl headers are now supposed to always be located in third_party/intel/backend/include/sycl/.... There's a new logic in setup.py that searches for the sycl headers in the system and copies them to the backend folder. If headers were not found it downloads intel-sycl-rt package from conda-forge and extracts sycl headers to the backend folder.

There are a few problems with downloading headers from conda-forge:

  1. Unlike dev packages that nvidia downloads in setup.py the intel-sycl-rt packages also contains compiled libraries (and not only headers). Which makes the downloaded package to be ~6.5mb in size instead of light ~400kb for nvidia packages (we may put up with it though).
  2. The nvidia packages are packed into tar.bz2 that can be unpacked only using standard python libraries. The intel-sycl-rt package is a .conda package, that is a zst packed archive. This type of archive cannot be unpacked using standard python libraries and requires zstandard python package to be installed. I've added this requirement to the scripts/compile-triton.sh script, but I'm not sure if it's okay to add one more dependency for setup.py

Alternatives to solve the 2nd problem are:

  1. Place sycl headers inside the repo so that we don't need to download them in setup.py. The downside of this approach is that we will need to manually reupload new headers once the new version comes up.
  2. Do not rely on prepared headers in third_party/intel/backend/include/sycl and rely on that, that they exist somewhere in the system and are easy to find.

run: |
cd python
pip install wheel pybind11
pip install wheel pybind11 zstandard
Copy link
Contributor Author

@dchigarev dchigarev Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we'll agree to add zstandard as a new build dependency, shouldn't we create a single build_requirements.txt file that would contain all build dependencies and that will be used in all these pip install statements that I've changed in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be great to separate the build and the dependencies installation. Ideally, if the project could be built with cmake only, it would improve integration with IDEs.

Signed-off-by: dchigarev <[email protected]>
@dchigarev dchigarev marked this pull request as ready for review January 23, 2025 15:29
@anmyachev
Copy link
Contributor

Hi @alexbaden, @etiotto, @vlad-penkin, I can review the implementation details, but I'm not sure we're heading in the direction you originally intended. Please confirm that this doesn't conflict with the plans.

@dchigarev
Copy link
Contributor Author

I'm not sure we're heading in the direction you originally intended

@anmyachev What are you doubts about? Do you think we shouldn't prepare headers at all (and rely that they already exist somewhere in the system) or we should prepare them, but avoid adding additional build-time dependencies (by either finding another download source or by placing the headers directly in the repo)?

@dchigarev dchigarev marked this pull request as draft February 3, 2025 17:11
@vlad-penkin vlad-penkin self-requested a review February 3, 2025 17:13
@dchigarev
Copy link
Contributor Author

The original issue was closed #3147

@dchigarev dchigarev closed this Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Align method of sycl headers/libs finding in third_parth/intel/backend/driver.py

4 participants