Skip to content

Commit 5eef2cd

Browse files
Dandandanclaude
andcommitted
ci: drop the deprecated tpchgen-cli flags, and cut the workflow's wall clock
tpchgen-cli 3.0.0 warns that `--format` goes away in 4.0.0 and that `--parquet-compression` is deprecated, both in favour of a subcommand per format. `tpchgen-cli parquet --compression=...` produces a byte-identical tree at SF0.01, so this is a rename; `bench.sh` gets the same treatment for its parquet, csv and sort-pushdown calls, and says which version it needs. The step timings of the SF10 run say where the half hour goes, and it is not the part that looks expensive: build base runner 990s (835s cargo, 131s freeing disk space) build head runner 966s (845s cargo, 96s freeing disk space) benchmark 461s (410s measuring, 34s generating the data) So generating the data is 34 seconds and needs nothing done to it, while the builds are two minutes of deleting Android SDKs followed by fourteen minutes of compiling dependencies that neither side changed. The builds now restore a `Swatinem/rust-cache` entry, shared between the two sides because they are a commit or two apart, written only by pushes to `main` so pull request runs do not each save a near-identical copy; and the disk cleanup is skipped when the runner already asked for `disk=large`, which is every run that gets the RunsOn machine. The warmup pass is now a read of the data files rather than a discarded pass of the suite. Each round is a fresh process, so the page cache is the only thing a warmup can carry between rounds, and reading the files fills it in seconds where a pass of the suite cost as much as a measured round. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent af4197b commit 5eef2cd

3 files changed

Lines changed: 41 additions & 15 deletions

File tree

‎.github/workflows/benchmark.yml‎

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,11 @@ jobs:
190190
with:
191191
fetch-depth: 2
192192
- name: Free Disk Space (Ubuntu)
193+
# Two minutes of deleting Android SDKs, which a release build of the
194+
# workspace genuinely needs on `ubuntu-latest`'s 14GB. The RunsOn
195+
# runner above asks for `disk=large` and has no such problem, so only
196+
# the fallback pays for this.
197+
if: vars.USE_RUNS_ON != 'true'
193198
uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be # v1.3.1
194199
- name: Install Rust
195200
run: |
@@ -210,14 +215,28 @@ jobs:
210215
else
211216
git worktree add --detach "$BENCH_ROOT/$SIDE" HEAD
212217
fi
218+
- name: Cache the dependency build
219+
# A cold build of this is fourteen minutes, most of it dependencies
220+
# that neither side changed. The action drops workspace crates from
221+
# what it saves, so a hit rebuilds DataFusion and reuses the rest.
222+
#
223+
# `workspaces` because the build happens in a worktree outside the
224+
# checkout, and one shared key for both sides because they are a commit
225+
# or two apart and share a dependency graph. Only a push to `main`
226+
# writes the cache: pull request runs would each save a near-identical
227+
# copy of it.
228+
uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2
229+
with:
230+
workspaces: ${{ env.BENCH_ROOT }}/${{ matrix.side }}
231+
shared-key: benchmark-${{ env.CARGO_PROFILE }}
232+
save-if: ${{ github.event_name == 'push' }}
213233
- name: Build benchmark_runner
214234
env:
215235
SIDE: ${{ matrix.side }}
216-
CARGO_TARGET_DIR: ${{ runner.temp }}/target
217236
run: |
218237
cd "$BENCH_ROOT/$SIDE"
219238
cargo build --profile "$CARGO_PROFILE" -p datafusion-benchmarks --bin benchmark_runner
220-
cp "$CARGO_TARGET_DIR/$CARGO_PROFILE/benchmark_runner" "$RUNNER_TEMP/benchmark_runner"
239+
cp "target/$CARGO_PROFILE/benchmark_runner" "$RUNNER_TEMP/benchmark_runner"
221240
- name: Upload benchmark_runner
222241
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
223242
with:
@@ -283,12 +302,14 @@ jobs:
283302
# crate, but a ~4MB download instead of a build from source, since the
284303
# project attaches no binaries to its GitHub releases for a prebuilt
285304
# fetch to find. It also means this job needs no Rust toolchain at all.
305+
#
306+
# `parquet` as a subcommand rather than `--format parquet`: 3.0.0
307+
# deprecated the flag form and warns that it goes away in 4.0.0.
286308
run: |
287309
mkdir -p "$DATA_DIR/tpch_sf$SCALE_FACTOR" "$RESULTS_DIR/base" "$RESULTS_DIR/head"
288-
uv tool run --from 'tpchgen-cli==3.0.0' tpchgen-cli \
310+
uv tool run --from 'tpchgen-cli==3.0.0' tpchgen-cli parquet \
289311
--scale-factor "$SCALE_FACTOR" \
290-
--format parquet \
291-
--parquet-compression 'ZSTD(1)' \
312+
--compression 'ZSTD(1)' \
292313
--parts=1 \
293314
--output-dir "$DATA_DIR/tpch_sf$SCALE_FACTOR"
294315
du -sh "$DATA_DIR/tpch_sf$SCALE_FACTOR"
@@ -323,11 +344,13 @@ jobs:
323344
--path "$DATA_DIR" \
324345
--output "$output"
325346
}
326-
# One discarded pass first, so no measured round is the only one
327-
# paying to pull the parquet files into the page cache. It writes
328-
# outside RESULTS_DIR, which is where compare.py looks for rounds.
329-
echo "::group::warmup"
330-
run_side base "$RUNNER_TEMP/warmup.json"
347+
# Read the data once first, so no measured round is the only one
348+
# paying to pull the parquet files into the page cache. Reading the
349+
# files is all a warmup can carry between rounds -- each round is a
350+
# fresh process -- and it takes seconds where a discarded pass of the
351+
# suite took as long as a round.
352+
echo "::group::warm the page cache"
353+
find "$DATA_DIR" -type f -exec cat {} + > /dev/null
331354
echo "::endgroup::"
332355
for round in $(seq 1 "$ROUNDS"); do
333356
# Alternate which side goes first, so with an even ROUNDS

‎benchmarks/README.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,10 @@ JSON plus the table are uploaded as the `tpch-comparison` artifact.
333333

334334
The data is generated with the same `tpchgen-cli` settings `bench.sh data tpch`
335335
uses, from the tool's PyPI wheel rather than a source build, which keeps a Rust
336-
toolchain out of the benchmark job entirely.
336+
toolchain out of the benchmark job entirely. Generating SF10 takes well under a
337+
minute; what the workflow's half hour actually goes on is the two builds, so
338+
they restore a dependency cache and skip the `ubuntu-latest` disk cleanup when
339+
the runner has the disk already.
337340

338341
Runners are shared machines, so treat the numbers as a signal rather than a
339342
measurement. A query has to clear three separate bars to fail the gate, and

‎benchmarks/bench.sh‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -668,7 +668,7 @@ data_tpch() {
668668
# check if tpchgen-cli is installed
669669
if ! command -v tpchgen-cli &> /dev/null
670670
then
671-
echo "tpchgen-cli could not be found, please install it via 'cargo install tpchgen-cli'"
671+
echo "tpchgen-cli could not be found, please install it via 'cargo install tpchgen-cli' (3.0 or newer)"
672672
exit 1
673673
fi
674674

@@ -689,7 +689,7 @@ data_tpch() {
689689
echo " parquet files exist ($FILE exists)."
690690
else
691691
echo " creating parquet files using tpchgen-cli ..."
692-
tpchgen-cli --scale-factor "${SCALE_FACTOR}" --format parquet --parquet-compression='ZSTD(1)' --parts=1 --output-dir "${TPCH_DIR}"
692+
tpchgen-cli parquet --scale-factor "${SCALE_FACTOR}" --compression='ZSTD(1)' --parts=1 --output-dir "${TPCH_DIR}"
693693
fi
694694
return
695695
fi
@@ -701,7 +701,7 @@ data_tpch() {
701701
echo " csv files exist ($FILE exists)."
702702
else
703703
echo " creating csv files using tpchgen-cli binary ..."
704-
tpchgen-cli --scale-factor "${SCALE_FACTOR}" --format csv --parts=1 --output-dir "${TPCH_DIR}/csv"
704+
tpchgen-cli csv --scale-factor "${SCALE_FACTOR}" --parts=1 --output-dir "${TPCH_DIR}/csv"
705705
fi
706706
return
707707
fi
@@ -1308,7 +1308,7 @@ data_sort_pushdown() {
13081308
TEMP_DIR="${DATA_DIR}/sort_pushdown_temp"
13091309
mkdir -p "${TEMP_DIR}" "${SORT_PUSHDOWN_DIR}"
13101310

1311-
tpchgen-cli --scale-factor 1 --format parquet --parquet-compression='ZSTD(1)' --parts=3 --output-dir "${TEMP_DIR}"
1311+
tpchgen-cli parquet --scale-factor 1 --compression='ZSTD(1)' --parts=3 --output-dir "${TEMP_DIR}"
13121312

13131313
# Rename: reverse alphabetical order vs key order
13141314
mv "${TEMP_DIR}/lineitem/lineitem.3.parquet" "${SORT_PUSHDOWN_DIR}/a_part3.parquet"

0 commit comments

Comments
 (0)