Skip to content
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.

Sign up for GitHub

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

RHEL SBSA Build #99

Merged
merged 3 commits into from
Sep 11, 2024
Merged

RHEL SBSA Build #99

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
75c25f7
RHEL SBSA Build
fpetrini15 Aug 30, 2024
4609a9e
Use CMAKE_PREFIX_PATH
fpetrini15 Sep 4, 2024
c8cc448
Review comments
fpetrini15 Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ endif()
set(TRITON_TENSORRT_BACKEND_LIBNAME triton_tensorrt)
set(TRITON_TENSORRT_BACKEND_INSTALLDIR ${CMAKE_INSTALL_PREFIX}/backends/tensorrt)

set(CMAKE_PREFIX_PATH "/usr/local/cuda/targets/sbsa-linux/lib;/usr/local/cuda/targets/x86_64-linux/lib")
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Seems like an anti-pattern to have multiple arches in a PATH variable but this is also concise. Maybe add a comment making note that if any other paths need to be added, this should be split.

  2. Does this overwrite any CMAKE_PREFIX_PATHs which are added on the command line?

Copy link
Contributor Author

@fpetrini15 fpetrini15 Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Circling back on this, I don't like the approach either. I'll set it conditionally.
  2. Good catch!
# results when running with command line arg "-DCMAKE_PREFIX_PATH:STRING=/hello;/world"
foreach(PATH ${CMAKE_PREFIX_PATH})
  message("PATH: ${PATH}")
endforeach()

# before:
PATH: /usr/local/cuda/targets/x86_64-linux/lib

# after:
PATH: /hello
PATH: /world
PATH: /usr/local/cuda/targets/x86_64-linux/lib

Copy link
Contributor

Choose a reason for hiding this comment

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

Semicolon-separated list of directories specifying installation prefixes to be searched by the find_package(), find_program(), find_library(), find_file(), and find_path() commands. Each command will add appropriate subdirectories (like bin, lib, or include) as specified in its own documentation.

  1. In other words if you don't mess with providers, and not planing to provide different architecture CMake will not mess up with dependencies.

    There why suppose condition will be redundant by serving same purpose.

  2. Given argument from command line should extend not override configuration within CMAKE.

Copy link
Contributor Author

@fpetrini15 fpetrini15 Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Are you suggesting that we disallow users from setting this variable at all and go back to overwriting it?

e.g., aarch64: set(CMAKE_PREFIX_PATH ${CMAKE_PREFIX_PATH} "/usr/local/cuda/targets/sbsa-linux/lib")

I can't say that if feels right to do that. If a user is building in their own environment, they should be able to set this variable as-needed.


#
# Dependencies
#
Expand Down Expand Up @@ -247,8 +249,8 @@ ENDFOREACH(p)

# NOTE: TRT 10 for Windows added the version suffix to the library names. See the release notes:
# https://docs.nvidia.com/deeplearning/tensorrt/release-notes/index.html#tensorrt-10
find_library(NVINFER_LIBRARY NAMES nvinfer nvinfer_10 PATHS "/usr/local/cuda/targets/x86_64-linux/lib")
find_library(NVINFER_PLUGIN_LIBRARY NAMES nvinfer_plugin nvinfer_plugin_10 PATHS "/usr/local/cuda/targets/x86_64-linux/lib")
find_library(NVINFER_LIBRARY NAMES nvinfer nvinfer_10)
find_library(NVINFER_PLUGIN_LIBRARY NAMES nvinfer_plugin nvinfer_plugin_10)
target_link_libraries(
triton-tensorrt-backend
PRIVATE
Expand Down
Loading