Bake provenance JSON into images and auto-populate post_results.py args by misiugodfrey · Pull Request #338 · rapidsai/velox-testing

misiugodfrey · 2026-05-07T18:48:23Z

Summary

Capture the Presto/Velox source that built an image and thread it into benchmark reports so results are traceable to their build. Follow-up to #316 (image labels).

Writes /opt/velox-testing/provenance.json into each Presto image at build time (both native_build.dockerfile and provenance_labels.dockerfile). Unlike the OCI labels from Add image labels #316, a file is readable from inside a running container.
Each launch script (launch_presto_servers.sh, launch_coordinator.sh) copies that file into the shared LOGS_DIR at container startup as worker_provenance.json / coordinator_provenance.json, so run_context.py running on the host (Docker) or inside the coordinator container (SLURM/Enroot) can read it via the existing logs bind-mount. The in-container /opt/velox-testing/provenance.json path is kept as a fallback for callers running directly inside an image that has the file baked in.
SLURM functions.sh: adds ${LOGS}:/opt/presto-server/logs to both coordinator and worker srun --container-mounts so the same scheme works across the cluster, mirrors the provenance cp in the inline worker bash and COORD_SCRIPT, and exports LOGS_DIR=/opt/presto-server/logs in run_queries so the in-container pytest looks at the mounted path.
run_context.py reads the provenance file and merges presto_sha/branch/repo and velox_sha/branch/repo into the benchmark context dict written to benchmark_result.json. Empty values are dropped, and the resolver prefers worker_provenance.json (full presto+velox fields) over coordinator_provenance.json (presto only on CI builds).
post_results.py submission:
- Auto-populates --velox-branch, --velox-repo, --presto-branch, --presto-repo from the benchmark_result.json context; explicit CLI args still take precedence.
- Emits presto_sha/velox_sha as their own engine_config entries alongside branch/repo. Provenance fields are added individually with empties omitted, so a coordinator-only image contributes just its presto entries.
- Derives query_engine.commit_hash from the SHAs as presto-<sha>-velox-<sha> (or presto-<sha> when velox is absent) when --commit-hash is not given; explicit --commit-hash still wins, and it stays unknown when no SHAs exist.
- Older result files without provenance are handled gracefully (fields absent, commit_hash = unknown, no error).
presto-build.yml:
- Replaces the labels: input on the deps and coordinator build steps with a second provenance_labels.dockerfile wrapper step — buildx labels: applies OCI metadata only and does not execute RUN steps, so the provenance file would not have been written via that path.
- Fixes the deps provenance step to reference the tag the deps image is actually pushed under (it was missing the -<build_variant>-<run_id> segments, so buildx failed with not found).

Notes / follow-ups

On CI-built images the branch fields are empty (CI knows only the SHA), so a SLURM run submits populated repos + SHAs with commit_hash = presto-<sha>-velox-<sha>, and branch fields only appear for locally-built images that baked a branch.
The new engine_config keys (presto_sha/velox_sha, and the existing branch/repo keys) have so far only been exercised via --dry-run; server-side acceptance should be confirmed with a real submission.

Test plan

Closes: #381

Writes /opt/velox-testing/provenance.json into each Presto image at build time (both native_build.dockerfile and provenance_labels.dockerfile), so run_context.py can read it from the container filesystem in both Docker and SLURM/Enroot environments. run_context.py: reads the provenance file and merges presto_sha/branch/repo and velox_sha/branch/repo into the benchmark context dict. post_results.py: auto-populates --velox-branch, --velox-repo, --presto-branch, --presto-repo from the benchmark_result.json context section; CLI args still take precedence. BenchmarkMetadata gains 6 optional provenance fields. presto-build.yml: replaces the labels: input on the deps and coordinator build steps with a second provenance_labels.dockerfile wrapper step, so CI and local builds both write the provenance file (buildx labels: applies OCI metadata only and does not execute RUN steps).

copy-pr-bot · 2026-05-19T21:32:08Z

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.

The provenance.json baked into images at /opt/velox-testing/provenance.json is only visible from inside the container, but pytest runs on the host in the Docker flow (run_benchmark.sh:294) and inside the coord container in SLURM/Enroot. The host has no /opt/velox-testing/provenance.json, so run_context.py's _get_image_provenance() returned {} and the six provenance fields were silently missing from benchmark_result.json's context. Have each launch script copy the baked-in file into LOGS_DIR at startup (worker_provenance.json from launch_presto_servers.sh, coordinator_provenance.json from launch_coordinator.sh). In Docker the dir is bind-mounted to the host, so pytest reads it directly. In SLURM, add ${LOGS}:/opt/presto-server/logs to both coord and worker srun --container-mounts so the same scheme works across the cluster, and export LOGS_DIR=/opt/presto-server/logs in run_queries so the in-container pytest looks in the right place. run_context.py replaces the hardcoded path constant with a resolver that prefers worker_provenance.json (full presto+velox fields) over coordinator_provenance.json (presto only in CI builds), then falls back to the original /opt/velox-testing/provenance.json for callers running directly inside a container that has the file baked in.

…nance # Conflicts: # presto/slurm/presto-nvl72/functions.sh

…nance

The 'Apply provenance labels to Presto deps image' step reconstructed the deps image tag by hand but omitted the build_variant and run_id segments that the build-and-push step includes, so it resolved a tag that was never pushed (buildx: 'not found'). Align BASE_IMAGE and the output tag with the deps build tag.

Add presto_sha/velox_sha as their own engine_config entries alongside the existing branch/repo fields, sourced from the baked image provenance in the benchmark context. Provenance fields are now emitted individually with empties omitted, so a coordinator-only image contributes just its presto entries. Derive query_engine.commit_hash from the SHAs as 'presto-<sha>-velox-<sha>' (or 'presto-<sha>' when velox is absent) when --commit-hash is not given; explicit --commit-hash still wins, and it stays 'unknown' when no SHAs exist. Degrades gracefully on older result files with no provenance.

misiugodfrey · 2026-07-22T01:06:46Z

          build-args: |
            CUDA_VERSION=${{ matrix.cuda_version }}
            ARM_BUILD_TARGET=generic
-          labels: |


Docker labels were lost if we convert docker images to sqsh images, so instead we store that information in a provenance.json file which is preserved inside the image regardless of conversion.

misiugodfrey · 2026-07-22T01:09:26Z

    validation_results: dict | None = None,
    velox_branch: str | None = None,
    velox_repo: str | None = None,
+    velox_sha: str | None = None,


Usually only branch or sha is used, but decided it was better to have separate values than try to overload the meaning of one value.

misiugodfrey · 2026-07-22T01:10:39Z

+    # Derive commit_hash from the image provenance SHAs when not explicitly given:
+    # "presto-<sha>-velox-<sha>" (worker), "presto-<sha>" (coordinator, no velox).
    if commit_hash is None:
-        commit_hash = "unknown"


commit_hash doesn't apply well to presto-velox builds since they are combination, but this is a top-level value in the benchmarking DB, so it seems useful to fill it with a concatenation of both.

The deps and coordinator provenance wrapper steps were near-identical build-push-action invocations differing only in base image, tags, and the velox fields. Extract them into .github/actions/apply-provenance-labels so the pinned action SHA, provenance dockerfile wiring, and outputs live in one place (mirroring the repo's existing container-registry-login/delete-arch-tags composite actions), removing the risk of the two steps drifting. Coordinator omits the velox_* inputs (default empty), preserving its presto-only provenance. Behaviour is unchanged.

ruff format (v0.14.x) expands the frozenset literal to one element per line; run the formatter to satisfy the check-style pre-commit hook.

patdevinwilson

Nice approach — baking a readable provenance.json (instead of relying on OCI labels that disappear in .sqsh conversion) and surfacing it via LOGS_DIR is clean. A few nits below; otherwise LGTM once the real submission path is confirmed.

patdevinwilson · 2026-07-28T16:24:59Z

      velox-testing.velox.branch=${VELOX_BRANCH} \
      velox-testing.velox.repository=${VELOX_REPOSITORY}
+RUN mkdir -p /opt/velox-testing && \
+    printf '{"presto_sha":"%s","presto_branch":"%s","presto_repo":"%s","velox_sha":"%s","velox_branch":"%s","velox_repo":"%s"}\n' \


Minor robustness nit: building the JSON with printf/%s will produce invalid JSON if a branch/repo ever contains ", \, or a newline. SHAs are safe, but branch names aren't guaranteed. Consider writing via Python/jq (jq -n --arg ...) so values are escaped, or document that only plain git refs are supported.

Note, the same printf pattern in native_build.dockerfile would need the corresponding change.

patdevinwilson · 2026-07-28T16:24:59Z

 echo \"Worker ${worker_id}: KVIKIO_NTHREADS=\${KVIKIO_NTHREADS:-unset}\"

+if [ -f /opt/velox-testing/provenance.json ]; then
+    cp /opt/velox-testing/provenance.json /opt/presto-server/logs/worker_provenance.json


With multi-worker runs, every worker writes the same path worker_provenance.json. That's fine if images are identical (last writer wins with the same content), but worth a one-line comment so nobody later tries to make this per-worker without also updating run_context.py.

patdevinwilson · 2026-07-28T16:24:59Z

+    logs_dir = os.environ.get("LOGS_DIR")
+    if logs_dir:
+        ld = Path(logs_dir)
+        for name in ("worker_provenance.json", "coordinator_provenance.json"):


Preferring worker over coordinator provenance here matches the CI shape (worker has presto+velox; coordinator is presto-only). Good call. One edge case: if only the coordinator file is present (e.g. worker copy failed), you still get useful presto fields — nice fallback chain.

I think these fixed filenames can cause us to report provenance from an earlier run. start_presto_helper.sh archives only *.log, so worker_provenance.json survives between starts. If the next run uses Java, which doesn't write this file, or an older worker image without baked provenance, we will still prefer the stale worker file over the current coordinator file.

Could we clear both provenance files when setting up LOGS_DIR, or make the filenames run-specific using SERVER_START_TIMESTAMP? Clearing them at startup seems like the smaller fix.

patdevinwilson · 2026-07-28T16:24:59Z

+            commit_parts.append(f"presto-{presto_sha}")
+        if velox_sha:
+            commit_parts.append(f"velox-{velox_sha}")
+        commit_hash = "-".join(commit_parts) if commit_parts else "unknown"


Agree with your note that commit_hash is awkward for a dual-repo build. The presto-<sha>-velox-<sha> form is clear for humans/DB. One thing to confirm with a real (non-dry-run) submission: does the benchmarking DB impose a max length / charset on commit_hash? Full SHAs make this ~85 chars.

patdevinwilson · 2026-07-28T16:24:59Z

+    presto_branch = presto_branch or benchmark_metadata.presto_branch
+    presto_repo = presto_repo or benchmark_metadata.presto_repo
+    # SHAs come only from the baked image provenance (no CLI override).
+    velox_sha = benchmark_metadata.velox_sha


Asymmetry is intentional but slightly surprising: branch/repo allow CLI override, SHAs do not. Worth a sentence in the --help text (or PR description is enough) so operators don't expect --velox-sha.

patdevinwilson · 2026-07-28T16:24:59Z

+        uses: ./velox-testing/.github/actions/apply-provenance-labels
+        with:
+          base_image: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:presto-deps-${{ inputs.presto_short_sha }}-velox-${{ inputs.velox_short_sha }}-cuda${{ matrix.cuda_version }}-${{ inputs.date }}-${{ inputs.build_variant }}-${{ github.run_id }}-${{ matrix.arch }}
+          tags: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:presto-deps-${{ inputs.presto_short_sha }}-velox-${{ inputs.velox_short_sha }}-cuda${{ matrix.cuda_version }}-${{ inputs.date }}-${{ inputs.build_variant }}-${{ github.run_id }}-${{ matrix.arch }}


Good catch fixing the deps tag reconstruction. Overwriting the same tag with the provenance-wrapped image means the non-provenance digest is briefly published then replaced — attestation correctly uses steps.push after this step. If anything external could pull between the two pushes, consider pushing provenance under a temp tag then moving, but probably fine for this registry flow.

kjmph · 2026-07-29T15:19:55Z


+    # Fall back to image provenance labels captured in the benchmark context.
+    velox_branch = velox_branch or benchmark_metadata.velox_branch
+    velox_repo = velox_repo or benchmark_metadata.velox_repo


I think we should normalize these repository values before persisting them. Local builds populate them from the raw output of git remote get-url origin, and HTTPS remotes can contain credentials such as https://user:TOKEN@github.com/.... This fallback would submit that value in engine_config. It would also already be present in the baked file, shared logs, and benchmark result.. yet, we definitely don't want them in the engine_config.

Could we strip URL userinfo in capture_build_provenance and store either owner/repo or a credential-free URL? Sanitizing at capture time seems preferable because it protects every downstream artifact, not just the API submission.

kjmph · 2026-07-29T15:22:35Z

+# Surface baked-in image provenance in the shared logs dir so the host-side
+# pytest (Docker) or in-container pytest (SLURM) can read it via LOGS_DIR.
+if [ -f /opt/velox-testing/provenance.json ]; then
+  cp /opt/velox-testing/provenance.json "${LOGS_DIR}/coordinator_provenance.json"


Should we mirror this in launch_java_worker.sh? The Java worker image also gets provenance baked into it, but its launcher never copies that file into the shared logs directory, so Java benchmarks always fall back to the coordinator's provenance. This also was noted in my earlier comment in regards to run_context.py.

misiugodfrey requested a review from a team as a code owner May 7, 2026 18:48

Fix InvalidDefaultArgInFrom warning in provenance_labels.dockerfile

7f2f3d0

misiugodfrey marked this pull request as draft May 19, 2026 21:32

misiugodfrey added 6 commits May 19, 2026 15:30

Merge remote-tracking branch 'origin/main' into misiug/benchmarkProve…

1c12a14

…nance # Conflicts: # presto/slurm/presto-nvl72/functions.sh

Merge remote-tracking branch 'origin/main' into misiug/benchmarkProve…

31b826c

…nance

Merge remote-tracking branch 'origin/main' into misiug/benchmarkProve…

fdc686a

…nance

misiugodfrey commented Jul 22, 2026

View reviewed changes

misiugodfrey marked this pull request as ready for review July 22, 2026 01:31

misiugodfrey mentioned this pull request Jul 22, 2026

Add nightly Presto benchmark script for NVL72 Slurm cluster #382

Open

5 tasks

Apply ruff format to _PROVENANCE_FIELDS

6aa3055

ruff format (v0.14.x) expands the frozenset literal to one element per line; run the formatter to satisfy the check-style pre-commit hook.

patdevinwilson reviewed Jul 28, 2026

View reviewed changes

kjmph reviewed Jul 29, 2026

View reviewed changes

Uh oh!

Conversation

misiugodfrey commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Notes / follow-ups

Test plan

Uh oh!

copy-pr-bot Bot commented May 19, 2026

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!

patdevinwilson left a comment

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!

kjmph Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

misiugodfrey commented May 7, 2026 •

edited

Loading

kjmph Jul 29, 2026 •

edited

Loading