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

Jump to bottom

build: tritonfrontend support for no/partial endpoint builds #7605

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
+62 −30
Open

build: tritonfrontend support for no/partial endpoint builds #7605

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
59d9aa8
conditional http/grpc endpoints
KrishnanPrash Sep 6, 2024
0b66a27
Adding tritonfrontend.whl to build.py instructions
KrishnanPrash Sep 6, 2024
173216e
Setting enable_tracing flag to 0
KrishnanPrash Sep 27, 2024
c9fa783
testing conditional appending of tracing lib
KrishnanPrash Sep 27, 2024
384da14
Merge remote-tracking branch 'origin/main' into kprashanth-conditiona…
KrishnanPrash Oct 2, 2024
40da2be
linker error
KrishnanPrash Oct 3, 2024
f08de59
Working conditional builds w Tracing=ON
KrishnanPrash Oct 4, 2024
f3fc425
Adding comments
KrishnanPrash Oct 4, 2024
a657cae
Making top-level imports more specific
KrishnanPrash Oct 4, 2024
be36c42
Fixing imports
KrishnanPrash Oct 4, 2024
80ee6e5
Catching specfic error
KrishnanPrash Oct 4, 2024
723df84
Merge remote-tracking branch 'origin/main' into kprashanth-conditiona…
KrishnanPrash Oct 9, 2024
48acc3e
remove TODO comment
KrishnanPrash Oct 9, 2024
c6efa9e
Cleaning up build.py and CMake
KrishnanPrash Oct 9, 2024
2932600
Minimal working CMake configuration
KrishnanPrash Oct 9, 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
10 changes: 8 additions & 2 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,15 +521,21 @@ if(${TRITON_ENABLE_TRACING})
if (NOT WIN32)
target_include_directories(
tracing-library
PRIVATE ${OPENTELEMETRY_CPP_INCLUDE_DIRS}
PUBLIC ${OPENTELEMETRY_CPP_INCLUDE_DIRS}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@fpetrini15 any general wisdom on necessity of PUBLIC vs PRIVATE and why it fixes it? or @KrishnanPrash

Copy link
Contributor

@fpetrini15 fpetrini15 Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

IIUC, by specifying tracing-library's dependencies as private, any other targets that depend on tracing-library will not be able to see its dependencies (not inherited)--they will have to link/include them again individually. In this case, it appears that to compile the tracing-library, CMake needs to have access to the open telemetry include directories. From my meetings with @KrishnanPrash, it seems like anything that depends on tracing-library also needs to have access to this directory. In this case, it would make sense to expose this as a public dependency.

Take this with a grain of salt, however, as I am no CMake expert 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing from PRIVATE to PUBLIC allows the directories to be included with any library that links to the tracing-library, which in this case is py-bindings.

)

target_link_libraries(
tracing-library
PRIVATE
PUBLIC
${OPENTELEMETRY_CPP_LIBRARIES})
endif()

set_target_properties(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this modification still required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the -fPIC flag set to ON, the CMake build process errors out.

tracing-library
PROPERTIES
POSITION_INDEPENDENT_CODE ON
)

target_link_libraries(
tracing-library
PUBLIC
Expand Down
10 changes: 0 additions & 10 deletions src/python/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,6 @@

cmake_minimum_required(VERSION 3.18)

message("tritonfrontend python package build skipped when relevant frontends are disabled.")
message("In order to build tritonfrontend, the following flags are needed: -DTRITON_ENABLE_HTTP=ON -DTRITON_ENABLE_GRPC=ON")

# [DLIS-7232] tritonfrontend package expects all supported packages to be
# built, without any check/verification for respective frontend enable flags.
# Support for partial builds(ex: HTTP but not gRPC) will be addressed later.
if(NOT (${TRITON_ENABLE_HTTP} AND ${TRITON_ENABLE_GRPC}))
return()
endif()

add_subdirectory(tritonfrontend)

file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/TRITON_VERSION ${TRITON_VERSION})
Expand Down
37 changes: 19 additions & 18 deletions src/python/tritonfrontend/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@ set(
../../common.h
../../common.cc
../../restricted_features.h
../../tracer.h
$<$<BOOL:${TRITON_ENABLE_TRACING}>:../../tracer.cc>
../../classification.cc
)

Expand Down Expand Up @@ -96,13 +94,10 @@ endif()

if(${TRITON_ENABLE_TRACING})
message("TRACING/STATS IS CURRENTLY NOT SUPPORTED.")
find_package(absl CONFIG REQUIRED)
find_package(CURL CONFIG REQUIRED)
find_package(nlohmann_json CONFIG REQUIRED)
find_package(opentelemetry-cpp CONFIG REQUIRED)
list(APPEND PY_BINDING_DEPENDENCY_LIBS
tracing-library
)
list(
APPEND PY_BINDING_DEPENDENCY_LIBS
$<TARGET_OBJECTS:tracing-library>
)
endif()

# ===================== End of Collection ===================================
Expand All @@ -122,8 +117,6 @@ pybind11_add_module(
${PYTHON_FRONTEND_BINDING_SRCS}
)

target_include_directories(py-bindings PRIVATE ${CMAKE_SOURCE_DIR}/src)

target_link_libraries(
py-bindings
PRIVATE
Expand Down Expand Up @@ -153,14 +146,21 @@ if(${TRITON_ENABLE_GPU})
endif()

if(${TRITON_ENABLE_TRACING})
target_include_directories(
target_include_directories(
py-bindings
PRIVATE ${OPENTELEMETRY_CPP_INCLUDE_DIRS}
PUBLIC ${OPENTELEMETRY_CPP_INCLUDE_DIRS}
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 this may also be unnecessary now for the same reason as described below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in comment above.

)

target_link_libraries(
py-bindings
PUBLIC
${OPENTELEMETRY_CPP_LIBRARIES}
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that these libs are exposed publicly by the tracing-library that is already linked to the py-bindings target, do we need to do it again here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without linking and including these libraries/directories to py-bindings, the build fail which points to the src/CMakeLists.txt linking change from PRIVATE to PUBLIC not having the intended consequence. Hence, will revert the src/CMakeLists.txt level changes and keep these link/include operations.

)

target_compile_definitions(
py-bindings
PRIVATE TRITON_ENABLE_TRACING=1
)
target_compile_definitions(
py-bindings
PRIVATE TRITON_ENABLE_TRACING=1
)
endif()

if(${TRITON_ENABLE_STATS})
Expand All @@ -177,5 +177,6 @@ set_target_properties(
py-bindings
PROPERTIES
BUILD_RPATH "$ORIGIN:/opt/tritonserver/lib"
POSITION_INDEPENDENT_CODE ON
)
# ===================== End of Python Bindings ==============================
# ===================== End of Python Bindings ==============================
13 changes: 11 additions & 2 deletions src/python/tritonfrontend/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,14 @@
import builtins
from importlib.metadata import PackageNotFoundError, version

from tritonfrontend._api._kservegrpc import KServeGrpc
from tritonfrontend._api._kservehttp import KServeHttp
try:
from tritonfrontend._api._kservehttp import KServeHttp
except ImportError:
# TRITON_ENABLE_HTTP=OFF
pass

try:
from tritonfrontend._api._kservegrpc import KServeGrpc
except ImportError:
# TRITON_ENABLE_GRPC=OFF
pass
14 changes: 14 additions & 0 deletions src/python/tritonfrontend/_api/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,17 @@
# OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

try:
from ._kservehttp import KServeHttp
except ImportError:
Fixed Show fixed Hide fixed
# TRITON_ENABLE_HTTP=OFF
# TritonFrontendHttp Package was not present
pass

try:
from ._kservegrpc import KServeGrpc
except ImportError:
github-advanced-security[bot] marked this conversation as resolved.
Fixed Show resolved Hide resolved
# TRITON_ENABLE_GRPC=OFF
# TritonFrontendGrpc Package was not present
pass
1 change: 1 addition & 0 deletions src/python/tritonfrontend/_api/_error_mapping.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
UnsupportedError,
)

# ERROR_MAPPING takes in tritonfrontend Error and maps to respective tritonserver Error
ERROR_MAPPING = {
TritonError: tritonserver.TritonError,
NotFoundError: tritonserver.NotFoundError,
Expand Down
13 changes: 12 additions & 1 deletion src/python/tritonfrontend/_c/tritonfrontend_pybind.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,16 @@
#include <pybind11/pybind11.h>
#include <pybind11/stl.h>

#ifdef TRITON_ENABLE_GRPC
#include "../../../grpc/grpc_server.h"
#endif


#ifdef TRITON_ENABLE_HTTP
#include "../../../http_server.h"
#endif


#include "triton/core/tritonserver.h"
#include "tritonfrontend.h"

Expand All @@ -53,12 +61,14 @@ PYBIND11_MODULE(tritonfrontend_bindings, m)
py::register_exception<AlreadyExistsError>(
m, "AlreadyExistsError", tfe.ptr());


#ifdef TRITON_ENABLE_HTTP
py::class_<TritonFrontend<HTTPServer, HTTPAPIServer>>(m, "TritonFrontendHttp")
.def(py::init<uintptr_t, UnorderedMapType>())
.def("start", &TritonFrontend<HTTPServer, HTTPAPIServer>::StartService)
.def("stop", &TritonFrontend<HTTPServer, HTTPAPIServer>::StopService);
#endif // TRITON_ENABLE_HTTP

#ifdef TRITON_ENABLE_GRPC
py::class_<TritonFrontend<
triton::server::grpc::Server, triton::server::grpc::Server>>(
m, "TritonFrontendGrpc")
Expand All @@ -71,6 +81,7 @@ PYBIND11_MODULE(tritonfrontend_bindings, m)
"stop", &TritonFrontend<
triton::server::grpc::Server,
triton::server::grpc::Server>::StopService);
#endif // TRITON_ENABLE_GRPC
}

}}} // namespace triton::server::python
Loading