Skip to content

Add nightly Presto benchmark script for NVL72 Slurm cluster - #382

Open
misiugodfrey wants to merge 6 commits into
mainfrom
nightlyPresto
Open

Add nightly Presto benchmark script for NVL72 Slurm cluster#382
misiugodfrey wants to merge 6 commits into
mainfrom
nightlyPresto

Conversation

@misiugodfrey

@misiugodfrey misiugodfrey commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds presto/slurm/presto-nvl72/nightly-benchmark.sh, a daemon script for running automated nightly Presto benchmarks on the NVL72 Slurm cluster
  • Pulls the latest nightly-pinned coordinator and gpu-cuda13.1 worker images from GHCR into a temporary directory (leaves the cluster's main IMAGE_DIR untouched), then removes them after both runs complete
  • Runs two benchmark configurations each night:
    • 1-node / 4-GPU / TPC-H sf1000
    • 2-node / 8-GPU (4 per node) / TPC-H sf3000
  • Posts results to the benchmarking DB via post_results.py after each run
  • Designed to run in a persistent tmux/screen session; sleeps until 3 AM EST (08:00 UTC) each night using date arithmetic

Test plan

  • Verify syntax: bash -n nightly-benchmark.sh
  • Test immediate trigger: RUN_HOUR_UTC=$(date -u +%H) ./nightly-benchmark.sh — should fire within seconds
  • Confirm .sqsh files appear in ${IMAGE_DIR}/nightly-temp/ during a run and are removed on completion
  • Confirm post_results.py is invoked with correct --benchmark-name, --cache-state warm, and storage config per run
  • Kill one benchmark mid-run; verify cleanup still executes and the loop continues to the next night

Adds presto/slurm/presto-nvl72/nightly-benchmark.sh, a daemon script
intended to run in a persistent tmux/screen session. It sleeps until
3 AM EST (08:00 UTC) each night, then:

  1. Pulls the latest nightly-pinned coordinator and gpu-cuda13.1 worker
     images from GHCR into ${IMAGE_DIR}/nightly-temp (leaving the
     cluster's main IMAGE_DIR untouched).
  2. Runs a 1-node / 4-GPU TPC-H sf1000 benchmark via launch-run.sh.
  3. Runs a 2-node / 8-GPU TPC-H sf3000 benchmark via launch-run.sh.
  4. Posts both results to the benchmarking DB via post_results.py.
  5. Cleans up the temporary images.

A failed night's run prints a warning and retries the next night without
killing the daemon. All output is tee'd to a persistent log file under
RESULTS_BASE.
@copy-pr-bot

copy-pr-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

These will be auto-populated from image metadata by the time this script
is in use.
…ning

Set RESULTS_BASE to ${VT_ROOT}/results before sourcing defaults.env so the
workspace derivation logic (which converts path separators to dashes) doesn't
produce a non-existent path when the repo is nested more than one level under
$HOME.
@copy-pr-bot

copy-pr-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

BENCHMARK_TEST_DIR=${TEST_DIR}/performance_benchmarks
pytest -q -s ${BENCHMARK_TEST_DIR}/${BENCHMARK_TYPE}_test.py ${PYTEST_ARGS[*]}
pytest_exit=0
pytest -q -s ${BENCHMARK_TEST_DIR}/${BENCHMARK_TYPE}_test.py ${PYTEST_ARGS[*]} || pytest_exit=$?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Needed to avoid an early exit here on failure since that prevents the logs from being moved. This is to enable the results to be pushed even if there was a pytest failure when running the benchmarks.

[[ "${JOB_STATE}" == "COMPLETED" ]] || exit 1

if [[ -n "${OUTPUT_PATH}" ]]; then
if [[ -n "${OUTPUT_PATH}" && -f "result_dir/benchmark_result.json" ]]; then

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Only copy the results over if there are results to copy - this is instead of checking if the job state was "COMPLETED" (which means without failures).

@misiugodfrey
misiugodfrey marked this pull request as ready for review July 22, 2026 18:21
@misiugodfrey
misiugodfrey requested a review from a team as a code owner July 22, 2026 18:21
@misiugodfrey
misiugodfrey requested a review from mattgara July 22, 2026 18:21
@misiugodfrey

Copy link
Copy Markdown
Contributor Author

This is designed to run with PR #338 so it will need re-testing once that lands.

@patdevinwilson patdevinwilson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Useful daemon for NVL72 nightlies. A couple of correctness issues around failure handling and LOGS_DIR (esp. relative to #338) before this should land. Details inline.

Comment thread presto/slurm/presto-nvl72/nightly-benchmark.sh Outdated
Comment thread presto/slurm/presto-nvl72/nightly-benchmark.sh
export PRESTO_DATA_DIR=/var/lib/presto/data/hive/data/user_data; \
export MINIFORGE_HOME=/workspace/miniforge3; \
export HOME=/workspace; \
export LOGS_DIR=/var/log/nsys; \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This conflicts with the direction in #338, which mounts ${LOGS}:/opt/presto-server/logs and exports LOGS_DIR=/opt/presto-server/logs so provenance + server logs are visible to pytest.

/var/log/nsys is the nsys token/report mount, not the general logs dir used by launch_presto_servers.sh / run_context.py. If this merges before/without #338's /opt/presto-server/logs mount on the CLI container, log snapshotting / GPU detection / provenance may miss files depending on which run_coord_image path is used.

Prefer aligning with #338 (LOGS_DIR=/opt/presto-server/logs + the matching mount), or land #338 first and rebase this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we should wait for #338 to land and then I'll rebase this.

BENCHMARK_TEST_DIR=${TEST_DIR}/performance_benchmarks
pytest -q -s ${BENCHMARK_TEST_DIR}/${BENCHMARK_TYPE}_test.py ${PYTEST_ARGS[*]}
pytest_exit=0
pytest -q -s ${BENCHMARK_TEST_DIR}/${BENCHMARK_TYPE}_test.py ${PYTEST_ARGS[*]} || pytest_exit=$?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Preserving pytest_exit so logs/validation still run on failure is the right call for nightlies that want to post partial results. Please keep this if/when rebasing onto #373 (that PR currently leaves bare pytest under set -e).

Comment thread presto/slurm/presto-nvl72/nightly-benchmark.sh
Comment thread presto/slurm/presto-nvl72/nightly-benchmark.sh
- Add || echo_warning to both post_results calls so a DB posting failure
  (API down, bad SKU, etc.) warns but does not abort the remaining run
- Add trap to run cleanup_images on SIGINT/SIGTERM so temp .sqsh files
  are removed when the daemon is stopped manually in tmux/screen
- Log active COORD_TAG and WORKER_TAG at startup so stale image tags
  are visible in the log
@patdevinwilson

Copy link
Copy Markdown
Contributor

Went through 555869a — replied inline, but summarizing:

  • post_results guard and SIGINT/TERM trap: both fixed and look correct. Left one non-blocking testing note on the trap re: subshell signal propagation, matches your still-unchecked "kill mid-run" test plan item.
  • echo_error nit: that one was my mistake — it does exit 1, no change needed. Sorry for the noise.
  • Worker tag hardcoding: the startup logging is a reasonable call, agreed.
  • functions.sh LOGS_DIR conflict with Bake provenance JSON into images and auto-populate post_results.py args #338: this is the one remaining item, and per your comment it's intentionally deferred until Bake provenance JSON into images and auto-populate post_results.py args #338 lands and you rebase. Makes sense to me — I'll take another pass once that's in. Everything else here looks good to merge as-is if you'd rather land this first and handle the rebase separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants