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.

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

merged 3 commits into from
Sep 11, 2024

Conversation

fpetrini15
Copy link
Contributor

@fpetrini15 fpetrini15 commented Sep 4, 2024

Goal: Incorporate SBSA builds/tests and extended the smoke tests for x86 and SBSA to include L0_sequence_batcher.

Server changes: triton-inference-server/server#7595
PyTorch Backend changes: triton-inference-server/pytorch_backend#139
TensorFlow Backend changes: triton-inference-server/tensorflow_backend#106

CMakeLists.txt Outdated Show resolved Hide resolved
mc-nv
mc-nv previously approved these changes Sep 4, 2024
CMakeLists.txt Outdated
@@ -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

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

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.

@fpetrini15 fpetrini15 merged commit ffcbe0b into main Sep 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants