Skip to content

Use correct data directory for non-Jetson no model perf test #5894

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

Merged
merged 1 commit into from
Jun 2, 2023
Merged

Use correct data directory for non-Jetson no model perf test #5894

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion qa/L0_perf_nomodel/run_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,17 @@ TRITON_DIR=${TRITON_DIR:="/opt/tritonserver"}
ARCH=${ARCH:="x86_64"}
SERVER=${TRITON_DIR}/bin/tritonserver
BACKEND_DIR=${TRITON_DIR}/backends
DATADIR=${DATADIR:="/data/inferenceserver/${REPO_VERSION}"}
MODEL_REPO="${PWD}/models"
PERF_CLIENT=../clients/perf_client
TF_VERSION=${TF_VERSION:=2}
SERVER_ARGS="--model-repository=${MODEL_REPO} --backend-directory=${BACKEND_DIR} --backend-config=tensorflow,version=${TF_VERSION}"
source ../common/util.sh

# DATADIR is already set in environment variable for aarch64
if [ "$ARCH" != "aarch64" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

May be it worth to look at L0_infer and reuse it's approach, to keep some sort of consistency:
https://github.com/triton-inference-server/server/blob/main/qa/L0_infer/test.sh#L37-L39

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, it's setting the REPO_VERSION based on whether you're on Jetson or not, then using it to set DATADIR if it's not set already. That approach wouldn't work here, because the non-Jetson benchmark tests don't set the DATADIR, which is why this previous PR change caused an issue (was trying to model it off the regular test we run, instead this returns to the previous style of check done).

There could be an argument made for consistency of Jetson checks, but I think that should be its own ticket because this is consistent with code in other perf tests like here and even L0_infer uses this type of check here. So if we want to standardize these checks, we'd need to pick one and make them all use that style in a ticket.

Copy link
Contributor

@rmccorm4 rmccorm4 Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

because the non-Jetson benchmark tests don't set the DATADIR

I think it's the opposite. I think the x86 tests (infra/CI script) were setting the DATADIR, but to the wrong value for this test. In the non-benchmark tests, the env var gets lost when not passed to the docker container, so it is "unset" and the default value takes over. In the benchmark tests, they currently seem to propogate all env vars through.

Then adding the new behavior of "keep datadir if set, otherwise set it to this default" from your last PR kept it as the incorrect value for the test.

To summarize, I think your previous PR's logic of "keep datadir if set, otherwise set it to this test-specific default" is good and removes the need for ARCH checks in the tests. Instead, it's on us to set the right datadir in the CI scripts.

I'll share some links offline.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually after looking through it, I think it'll be more trouble than its worth. It looks like we'd basically have to explicitly unset DATADIR in the benchmark test template which isn't very intuitive. Probably better to keep the logic in the test script for now.

DATADIR="/data/inferenceserver/${REPO_VERSION}"
fi

# Select the single GPU that will be available to the inference server
export CUDA_VISIBLE_DEVICES=0

Expand Down