build: tritonfrontend support for no/partial endpoint builds
#7605
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
build.py
Outdated
| @@ -1857,6 +1857,12 @@ def core_build( | |||
| os.path.join(install_dir, "python"), | |||
| ) | |||
|
|
|||
| cmake_script.mkdir(os.path.join(install_dir, "python")) | |||
| cmake_script.cp( | |||
| os.path.join(repo_install_dir, "python", "tritonfrontend*.whl"), | |||
There was a problem hiding this comment.
Is this needed? triton*.whl right above this
There was a problem hiding this comment.
Thank you for the catch. Just removed it.
src/CMakeLists.txt
Outdated
| @@ -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} | |||
There was a problem hiding this comment.
@fpetrini15 any general wisdom on necessity of PUBLIC vs PRIVATE and why it fixes it? or @KrishnanPrash
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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.
| ${OPENTELEMETRY_CPP_LIBRARIES}) | ||
| endif() | ||
|
|
||
| set_target_properties( |
There was a problem hiding this comment.
Is this modification still required?
There was a problem hiding this comment.
Without the -fPIC flag set to ON, the CMake build process errors out.
| target_link_libraries( | ||
| py-bindings | ||
| PUBLIC | ||
| ${OPENTELEMETRY_CPP_LIBRARIES} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| py-bindings | ||
| PRIVATE ${OPENTELEMETRY_CPP_INCLUDE_DIRS} | ||
| PUBLIC ${OPENTELEMETRY_CPP_INCLUDE_DIRS} |
There was a problem hiding this comment.
I think this may also be unnecessary now for the same reason as described below.
There was a problem hiding this comment.
Addressed in comment above.
rmccorm4
changed the title
tritonfrontend support for no/partial endpoint buildstritonfrontend support for no/partial endpoint builds
What does the PR do?
Currently, for the
tritonfrontendpython package only works for builds where all the supported endpoints are enabled. And for builds with partial endpoints enabled, the building of thetritonfrontendwheel is skipped.This PR is meant to add support for partial endpoint builds for
tritonfrontend. With these changes,tritonfrontendwheel will build in all cases and match the functionality of thetritonserverbinary.Checklist
<commit_type>: <Title>Commit Type:
Check the conventional commit type
box here and add the label to the github PR.
Where should the reviewer start?
src/python/tritonfrontend/_c/tritonfrontend_pybind.ccrestricts portions of C++ code that is binded.src/python/tritonfrontend/_api/__init__.pyrestricts which python apis are included in the tritonfrontend wheel.build.pychanges allow for wheel to be tested in CITest plan:
Added testing for
tritonfrontendto variant builds test.