Added benchmarking script by misiugodfrey · Pull Request #33 · rapidsai/velox-testing

misiugodfrey · 2025-09-04T05:38:36Z

Add a simple benchmarking script that will run benchmarks based on what is in the testing data directory. Offers optional profiling using nsys.

NOTE: There is ongoing work to perform this kind of benchmarking using python scripts (that will re-use a lot of our integration test work - de-duplicating a lot of code), so this item will likely stand as a placeholder until it can be replaced with the preferred python option once we verify they behave the same.

misiugodfrey · 2025-09-04T05:40:21Z

presto/docker/native_build.dockerfile

 ENV EXTRA_CMAKE_FLAGS=${EXTRA_CMAKE_FLAGS}
 ENV NUM_THREADS=${NUM_THREADS}

+RUN rpm --import https://developer.download.nvidia.com/compute/cuda/repos/ubuntu1804/x86_64/7fa2af80.pub && dnf config-manager --add-repo "https://developer.download.nvidia.com/devtools/repos/rhel$(source /etc/os-release; echo ${VERSION_ID%%.*})/$(rpm --eval '%{_arch}' | sed s/aarch/arm/)/" && dnf install -y nsight-systems-cli-2025.5.1.121


Install a pinned version of nsys to the container.

presto/scripts/run_benchmarks.sh

devavret · 2025-09-04T15:36:40Z

presto/docker/native_build.dockerfile

    echo "/usr/lib64/presto-native-libs" > /etc/ld.so.conf.d/presto_native.conf

-CMD bash -c "ldconfig && presto_server --etc-dir=/opt/presto-server/etc"
+CMD bash -c "ldconfig && nsys launch presto_server --etc-dir=/opt/presto-server/etc"


did this work? I was just assuming this would work but @karthikeyann tried it last night and couldn't get it to work until he started the container in interactive mode and manually ran nsys launch presto_server ...

This has been working for me - at least in so far as it generates profiles. What issue was he running into when he attempted this approach?

some of the arguments need to be given during nsys launch itself.
Please add required arguments for nsys here. I got this from our old velox benchmark scripts.

CMD bash -c "ldconfig && nsys launch -t nvtx,cuda,osrt \ --cuda-memory-usage=true \ --cuda-um-cpu-page-faults=true \ --cuda-um-gpu-page-faults=true \ presto_server --etc-dir=/opt/presto-server/etc"

add --gpu-metrics-devices, --cudabacktrace

devavret · 2025-09-05T12:27:35Z

May I request options to include certain nsys options? Personally, I'm using --gpu-metrics-devices, --cudabacktrace, --cuda-memory-usage. The latter two are nsys launch params and the first is specified at nsys start

… misiug/Benchmarking

misiugodfrey · 2025-09-10T23:49:28Z

May I request options to include certain nsys options? Personally, I'm using --gpu-metrics-devices, --cudabacktrace, --cuda-memory-usage. The latter two are nsys launch params and the first is specified at nsys start

I've added the launch params, but --gpu-metrics-devices requires the container to be run with SYS_ADMIN capabilities. Not sure if we want to require that as part of the base changes, or if we should make that a custom option.

tmostak · 2025-09-15T22:01:37Z

@misiugodfrey thanks for this. I think we should:

Always do a cold run of each query. We may want to capture timing of that separtely. We may want to allow an optional cache flush before each cold run to simulate cold/un-cached reads from disk is requested.
Allow a user-specified number of hot runs, and take the average of those.

misiugodfrey · 2025-09-15T22:12:53Z

Always do a cold run of each query. We may want to capture timing of that separtely. We may want to allow an optional cache flush before each cold run to simulate cold/un-cached reads from disk is requested.

Allow a user-specified number of hot runs, and take the average of those.

@tmostak these are good ideas, but I think it would be better to add them as new features in a subsequent PR. I would rather this focus on how we want runs to occur, and then we can add further tuning and hot/cold profiling once we have settled on the basics.

presto/scripts/run_benchmarks.sh

karthikeyann · 2025-09-17T02:31:09Z

presto/scripts/run_benchmarks.sh

+    if echo "$images" | grep -q "presto-native-worker-cpu"; then
+        [[ -n $WORKER ]] & echo_error "mismatch in worker types" && exit 1


Why does it disallow running with cpu or java?

It's not disallowing those runs (or if it is, that's a bug); it's checking to see if we already have workers of other types running. It's to make sure we aren't running java workers alongside cpu or gpu workers as the profiling expects us to only be running a single worker type (and a single worker for that matter).

@misiugodfrey I think there is an issue with & echo_error, it should be && echo_error.

karthikeyann

I recreated velox-testing

with this PR #33,
with presto PR (prestodb/presto#25899),
with velox branch (https://github.com/rapidsai/velox/tree/merged-prs)

I added the changes in this PR review to make it work on a fress system (arm).

velox-testing/presto/scripts$ ./run_integ_test.sh -b tpch passed (Q15 unstable due to know issue: floating point join key)

presto/docker/native_build.dockerfile

… misiug/Benchmarking

karthikeyann · 2025-09-22T21:02:42Z

presto/docker/native_build.dockerfile

 ENV NUM_THREADS=${NUM_THREADS}

+RUN rpm --import https://developer.download.nvidia.com/compute/cuda/repos/ubuntu1804/x86_64/7fa2af80.pub && dnf config-manager --add-repo "https://developer.download.nvidia.com/devtools/repos/rhel$(source /etc/os-release; echo ${VERSION_ID%%.*})/$(rpm --eval '%{_arch}' | sed s/aarch/arm/)/" && dnf install -y nsight-systems-cli-2025.5.1.121
+RUN RUN dnf install -y -q libnvjitlink-devel-12-8


Suggested change

RUN RUN dnf install -y -q libnvjitlink-devel-12-8

RUN dnf install -y -q libnvjitlink-devel-12-8

karthikeyann · 2025-09-22T21:17:52Z

presto/scripts/run_benchmarks.sh

+        local table_dir="/var/lib/presto/data/hive/data/integration_test/tpch/$table_name"
+        local create_table=$(cat $sql_file | sed "s+{file_path}+$table_dir+g")


please make the table dir configurable to register existing data (for example existing SF100 data in a directory)

This should now be functional using the --data and/or --schema options.

karthikeyann · 2025-09-22T21:18:57Z

presto/scripts/run_benchmarks.sh

+    -p, --profile	    Profile queries with nsys.
+    -q, --queries           Set of benchmark queries to run. This should be a comma separate list of query numbers.
+                            By default, all benchmark queries are run.
+    -l, --command-line      Run queries via presto-cli instead of curl.


please add --schema to help

Added --schema to the help options, as well as new options for --data and --coordinator.

karthikeyann · 2025-09-23T01:33:07Z

presto/scripts/run_benchmarks.sh

+
+parse_args "$@"
+detect_containers
+mkdir -p "$BASE_DIR/benchmark_output/tpch"


This has permission issue for the first time because benchmark_output might have been created by docker by root user

I've changed it so that we make sure benchmark_output exists before we mount it (so docker is not responsible for creating it). This should fix the permissions issues.

karthikeyann · 2025-09-23T01:33:17Z

presto/scripts/run_benchmarks.sh

+
+parse_args "$@"
+detect_containers
+mkdir -p "$BASE_DIR/benchmark_output/tpch"


This has permission issue because benchmark_output might have been created by docker by root user

…ox-testing into misiug/Benchmarking

misiugodfrey · 2025-09-24T00:47:06Z

presto/scripts/run_benchmarks.sh

+            run_outputs+=("$output_json")
+            echo "$output_json" > "$OUTPUT_DIR/Q$query.I$i.summary.json"
+        done
+        [ -z "$CREATE_PROFILES" ] || stop_profile


Right now we generate one profile that covers all non-warmup iterations for a query. Not sure if we want that, or only profiles on some (one) iteration(s)? Up for debate...

karthikeyann · 2025-09-24T22:29:53Z

presto/scripts/run_benchmarks.sh

+        local processed_bytes=$(echo "$stats" | jq -r '.processedBytes // 0')
+        local cpu_time_ms=$(echo "$stats" | jq -r '.cpuTimeMillis // 0')
+        local wall_time_ms=$(echo "$stats" | jq -r '.wallTimeMillis // 0')
+        local elapsed_time_ms=$(echo "$stats" | jq -r '.elapsedTimeMillis // 0')


elapsedTimeMillis is the accurate query execution time.
other numbers are wrong.

karthikeyann · 2025-09-24T22:31:37Z

presto/scripts/run_benchmarks.sh

+            local start_time=$(date +%s.%N)
+            run_query "$sql" "$query"
+            local end_time=$(date +%s.%N)
+            [ -n "$FINAL_RESPONSE" ] && echo "$FINAL_RESPONSE" > "$OUTPUT_DIR/Q$query.I$i.out.json"
+            local execution_time=$(echo "$end_time - $start_time" | bc -l)
+            local output_json=$(filter_output "$query" "$execution_time" "$FINAL_RESPONSE")


please don't use this method to calculate the runtime. This will be vary a lot.

elapsedTimeMillis for curl method.
Try running time inside docker for presto-cli to get better estimate. (even that will be more than curl method)

or get elapsedTime and executionTime from

http://localhost:8080/v1/query/${id}

I've changed this so that the cli path obtains the elapsed time via the web UI, so we are no longer performing any timing ourselves (just relying on the times reported by presto).

karthikeyann · 2025-09-27T06:40:49Z

As part of the PR, please create the queries.json files too and checkin

misiugodfrey · 2025-10-03T22:21:41Z

Changed this to draft PR to reflect that it is no longer intended to land. It has been replaced with PR #56 which uses the python interface.

simoneves · 2025-10-18T01:15:26Z

Changed this to draft PR to reflect that it is no longer intended to land. It has been replaced with PR #56 which uses the python interface.

Can we just close it, then? :)

Added benchmarking script

f7dbd01

misiugodfrey commented Sep 4, 2025

View reviewed changes

devavret reviewed Sep 4, 2025

View reviewed changes

presto/scripts/run_benchmarks.sh Outdated Show resolved Hide resolved

devavret reviewed Sep 4, 2025

View reviewed changes

misiugodfrey added 2 commits September 4, 2025 08:40

Fixed misleading comment

0f0e70b

force overwrite on nsys profile files

c9b6e8e

misiugodfrey added 5 commits September 8, 2025 11:20

testing new timings method

ce247b2

Merge branch 'main' of https://github.com/rapidsai/velox-testing into…

edb79af

… misiug/Benchmarking

Merge branch 'main' of https://github.com/rapidsai/velox-testing into…

f9152bf

… misiug/Benchmarking

Added support for runs via curl

2b4bb22

Merge branch 'main' of https://github.com/rapidsai/velox-testing into…

2a39c8c

… misiug/Benchmarking

misiugodfrey marked this pull request as ready for review September 10, 2025 23:48

misiugodfrey requested review from Avinash-Raj, devavret, karthikeyann, mattgara, paul-aiyedun and simoneves September 10, 2025 23:49

Merge branch 'main' into misiug/Benchmarking

f8a481d

karthikeyann reviewed Sep 17, 2025

View reviewed changes

presto/scripts/run_benchmarks.sh Outdated Show resolved Hide resolved

karthikeyann reviewed Sep 17, 2025

View reviewed changes

presto/scripts/run_benchmarks.sh Show resolved Hide resolved

karthikeyann reviewed Sep 17, 2025

View reviewed changes

added nsys launch options and added a schema parameter

a28a0b9

karthikeyann reviewed Sep 17, 2025

View reviewed changes

presto/docker/native_build.dockerfile Outdated Show resolved Hide resolved

presto/docker/native_build.dockerfile Outdated Show resolved Hide resolved

presto/docker/native_build.dockerfile Show resolved Hide resolved

misiugodfrey added 2 commits September 17, 2025 16:32

Additional requirements for fresh arm build

58a1671

Merge branch 'main' of https://github.com/rapidsai/velox-testing into…

897b6a7

… misiug/Benchmarking

karthikeyann reviewed Sep 22, 2025

View reviewed changes

karthikeyann reviewed Sep 23, 2025

View reviewed changes

misiugodfrey and others added 7 commits September 23, 2025 09:56

Removed limits and added custom data/schema support

04a797f

Removed limits and added custom data/schema support

8b0bd83

Merge branch 'misiug/Benchmarking' of https://github.com/rapidsai/vel…

01d927d

…ox-testing into misiug/Benchmarking

Fix profiling and permissions with output directory

78b9845

Merge branch 'main' into misiug/Benchmarking

1aa86c7

Added warmup queries and some other cleanup

8396e03

Merge branch 'misiug/Benchmarking' of https://github.com/rapidsai/vel…

67b8430

…ox-testing into misiug/Benchmarking

misiugodfrey requested review from karthikeyann and tmostak September 23, 2025 19:25

misiugodfrey added 5 commits September 23, 2025 13:34

minor adjustment

5b140bc

formatting

bea0b45

formatting

7762ad6

fixed typo

bae3e63

Added number of iterations to benchmarking script

289b59a

misiugodfrey commented Sep 24, 2025

View reviewed changes

fixed avg parsing to handle the command-line case

85f1fc2

karthikeyann reviewed Sep 24, 2025

View reviewed changes

Changed cli to get elapsed time from web UI rather than time it.

9c9a6b1

misiugodfrey requested a review from karthikeyann September 26, 2025 16:40

karthikeyann approved these changes Sep 27, 2025

View reviewed changes

Merge branch 'main' into misiug/Benchmarking

a42b376

misiugodfrey marked this pull request as draft October 3, 2025 22:20

misiugodfrey closed this Oct 23, 2025

		if echo "$images" \| grep -q "presto-native-worker-cpu"; then
		[[ -n $WORKER ]] & echo_error "mismatch in worker types" && exit 1

	RUN RUN dnf install -y -q libnvjitlink-devel-12-8
	RUN dnf install -y -q libnvjitlink-devel-12-8

		local table_dir="/var/lib/presto/data/hive/data/integration_test/tpch/$table_name"
		local create_table=$(cat $sql_file \| sed "s+{file_path}+$table_dir+g")

Conversation

misiugodfrey commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devavret commented Sep 5, 2025

Uh oh!

misiugodfrey commented Sep 10, 2025

Uh oh!

tmostak commented Sep 15, 2025

Uh oh!

misiugodfrey commented Sep 15, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karthikeyann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karthikeyann commented Sep 27, 2025

Uh oh!

misiugodfrey commented Oct 3, 2025

Uh oh!

simoneves commented Oct 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

misiugodfrey commented Sep 4, 2025 •

edited

Loading