chore: PA Migration From Client #7449
Conversation
| # TODO: PA will rebuild the CC clients since it depends on it. | ||
| # This should be optimized so that we do not have to build | ||
| # the CC clients twice. Similarly, because the SDK expectation is | ||
| # that PA is packaged with the python client, we hold off on building |
There was a problem hiding this comment.
Why it is packaged with Python client?
There was a problem hiding this comment.
I am not sure what the original motivation was (likely convenience). However, this design of packaging PA has been baked into the python client AFAIK starting back in 2022. If we want to break this expectation, that is a separate discussion.
| -DTRITON_ENABLE_GPU=${TRITON_ENABLE_GPU} /workspace/client | ||
| RUN make -j16 cc-clients java-clients && \ | ||
| rm -fr ~/.m2 | ||
|
|
There was a problem hiding this comment.
Looks like a multi-stage build to me: 1. client libs; 2. PA / genai-perf
There was a problem hiding this comment.
I general, yes, however, it's complicated by two things:
- The python client, as discussed above, has functionality to package PA in its pip wheel. Therefore, to guarantee its availability, the python client must be built after PA.
- PA does (in the current PA repo PR) and should be able to build itself (including its client dependencies) without needing the client libs prebuilt.
| -DTRITON_ENABLE_CC_HTTP=ON -DTRITON_ENABLE_CC_GRPC=ON \ | ||
| -DTRITON_ENABLE_PYTHON_HTTP=ON -DTRITON_ENABLE_PYTHON_GRPC=ON \ | ||
| -DTRITON_ENABLE_PYTHON_HTTP=OFF -DTRITON_ENABLE_PYTHON_GRPC=OFF \ |
There was a problem hiding this comment.
I think you can just turn it on and build it as that is what the client stage will look like at the end, avoiding rebuild can leave to the PA stage
There was a problem hiding this comment.
We can build the python client here, however, we need to rebuild it as part of the PA build in order for the pip wheel to contain the PA-associated binaries. For this reason, I figured it was best to skip the python client build at this stage. Am I misunderstanding?
| RUN cmake -DCMAKE_INSTALL_PREFIX=/workspace/install \ | ||
| -DTRITON_VERSION=`cat /workspace/TRITON_VERSION` \ | ||
| -DTRITON_REPO_ORGANIZATION=${TRITON_REPO_ORGANIZATION} \ | ||
| -DTRITON_COMMON_REPO_TAG=${TRITON_COMMON_REPO_TAG} \ | ||
| -DTRITON_CORE_REPO_TAG=${TRITON_CORE_REPO_TAG} \ | ||
| -DTRITON_THIRD_PARTY_REPO_TAG=${TRITON_THIRD_PARTY_REPO_TAG} \ | ||
| -DTRITON_ENABLE_PERF_ANALYZER=OFF \ |
There was a problem hiding this comment.
can remove, this is OFF by default.
There was a problem hiding this comment.
I saw it was off by default, but wanted to explicitly mark it as off for expectation clarity and JIC it changed. Ultimately, this option will be removed entirely from the client CMakeLists.txt. However, I can remove the option as well.
| -DTRITON_ENABLE_CC_HTTP=ON \ | ||
| -DTRITON_ENABLE_CC_GRPC=ON \ | ||
| -DTRITON_ENABLE_PYTHON_HTTP=ON \ | ||
| -DTRITON_ENABLE_PYTHON_GRPC=ON \ |
There was a problem hiding this comment.
Should add some TODOs, I don't think these flags will be removed in the final version.
There was a problem hiding this comment.
Can you clarify what TODOs are needed here? You're correct in that these flags will not be removed in the final version.
There was a problem hiding this comment.
Sorry, I meant "will be".. PA doesn't have dependency on Python client library, right?
There was a problem hiding this comment.
You are correct. PA does not depend on the Python client. However, as per current user expectation, the Python client does depend on PA, since it is generally packaged into the python client pip wheel during the build process (see comment above).
We may need to discuss as an org if we want to break this expectation now that PA will be its own repo.
Goal: Build PA separately from client and ensure that there is no impact on CI.
Client changes: triton-inference-server/client#722