-
Notifications
You must be signed in to change notification settings - Fork 35
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
Enable custom triton-rapids repo and tag #409
Conversation
ops/Dockerfile
Outdated
@@ -123,16 +123,18 @@ ENV BUILD_TESTS=$BUILD_TESTS | |||
ARG BUILD_EXAMPLE | |||
ENV BUILD_EXAMPLE=$BUILD_EXAMPLE | |||
|
|||
ARG TRITON_REPO_ORGANIZATION=https://github.com/triton-inference-server | |||
ARG TRITON_REPO_ORGANIZATION=$TRITON_REPO_ORGANIZATION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not valid, since TRITON_REPO_ORGANIZATION
is being used here without first being defined elsewhere. Is TRITON_REPO_ORGANIZATION
somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, https://github.com/triton-inference-server
is the default value for the argument TRITON_REPO_ORGANIZATION
. You can override it by passing -DTRITON_REPO_ORGANIZATION=...
when invoking CMake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm following the format specified here: https://github.com/triton-inference-server/fil_backend/pull/409/files#diff-d2e93d579aa110eb1f1fb1526ea0d7ca3fe7cbb3ed700ad8826aae45341a43beL128. I could change it back to the default case if you like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That line is different, since TRITON_VERSION
was defined earlier in the Dockerfile. On the other hand, TRITON_REPO_ORGANIZATION
is not defined earlier in the Dockerfile, so a default value needs to be supplied here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The referenced line constructs a new string from a defined variable by adding an r
in front, whereas the new changes are equivalent to setting the variables to empty strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for my mistake. Changed
@@ -281,7 +274,7 @@ buildpy () { | |||
then | |||
rm -rf "$server_repo" | |||
fi | |||
git clone $REPO_ORGANIZATION/server.git \ | |||
git clone https://github.com/triton-inference-server/server.git \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we hardcoding this here now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it back from before #404. I was not sure where build.sh is used in the pipeline so I let it be hardcoded, like before
…erence-server/fil_backend into kyang-variable-rapids-include
I will monitor the CI and see if the tests pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. The CI is all green too.
Follow up of #404 and #405
Let upstream builds pass rapids and triton variables through fil backend