-
Notifications
You must be signed in to change notification settings - Fork 23
Bake provenance JSON into images and auto-populate post_results.py args #338
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
56ba2d8
7f2f3d0
c1421c8
1c12a14
31b826c
fdc686a
bd8831d
59527a9
849d39c
6aa3055
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| name: Apply provenance labels | ||
| description: >- | ||
| Re-tag an already-built image through provenance_labels.dockerfile so it | ||
| carries the velox-testing provenance labels and baked /opt/velox-testing/provenance.json. | ||
|
|
||
| inputs: | ||
| base_image: | ||
| description: Fully-qualified reference of the already-built image to layer provenance onto. | ||
| required: true | ||
| tags: | ||
| description: Tag(s) to push the provenance-labelled image under (newline-separated). | ||
| required: true | ||
| presto_sha: | ||
| description: Presto commit SHA. | ||
| required: true | ||
| presto_repository: | ||
| description: Presto repository. | ||
| required: false | ||
| default: "" | ||
| presto_branch: | ||
| description: Presto branch (empty on CI, which knows only the SHA). | ||
| required: false | ||
| default: "" | ||
| velox_sha: | ||
| description: Velox commit SHA (empty for coordinator images, which do not build Velox). | ||
| required: false | ||
| default: "" | ||
| velox_repository: | ||
| description: Velox repository. | ||
| required: false | ||
| default: "" | ||
| velox_branch: | ||
| description: Velox branch (empty on CI). | ||
| required: false | ||
| default: "" | ||
|
|
||
| outputs: | ||
| digest: | ||
| description: Digest of the pushed provenance-labelled image. | ||
| value: ${{ steps.push.outputs.digest }} | ||
|
|
||
| runs: | ||
| using: composite | ||
| steps: | ||
| - name: Build and push provenance-labelled image | ||
| id: push | ||
| uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 # v6.18.0 | ||
| with: | ||
| context: velox-testing/presto/docker | ||
| file: velox-testing/presto/docker/provenance_labels.dockerfile | ||
| build-args: | | ||
| BASE_IMAGE=${{ inputs.base_image }} | ||
| PRESTO_SHA=${{ inputs.presto_sha }} | ||
| PRESTO_BRANCH=${{ inputs.presto_branch }} | ||
| PRESTO_REPOSITORY=${{ inputs.presto_repository }} | ||
| VELOX_SHA=${{ inputs.velox_sha }} | ||
| VELOX_BRANCH=${{ inputs.velox_branch }} | ||
| VELOX_REPOSITORY=${{ inputs.velox_repository }} | ||
| outputs: type=registry,compression=zstd,force-compression=true,compression-level=15 | ||
| push: true | ||
| tags: ${{ inputs.tags }} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,28 +118,31 @@ jobs: | |
| cp -r velox/CMake presto-deps-context/velox/CMake | ||
|
|
||
| - name: Build and push Presto deps image | ||
| id: push | ||
| uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 # v6.18.0 | ||
| with: | ||
| context: presto-deps-context | ||
| file: presto/presto-native-execution/scripts/dockerfiles/centos-dependency.dockerfile | ||
| build-args: | | ||
| CUDA_VERSION=${{ matrix.cuda_version }} | ||
| ARM_BUILD_TARGET=generic | ||
| labels: | | ||
| velox-testing.presto.sha=${{ inputs.presto_sha }} | ||
| velox-testing.presto.branch= | ||
| velox-testing.presto.repository=${{ inputs.presto_repository }} | ||
| velox-testing.velox.sha=${{ inputs.velox_sha }} | ||
| velox-testing.velox.branch= | ||
| velox-testing.velox.repository=${{ inputs.velox_repository }} | ||
| cache-from: type=registry,ref=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:cache-presto-deps-cuda${{ matrix.cuda_version }}-${{ matrix.arch }} | ||
| cache-to: type=registry,ref=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:cache-presto-deps-cuda${{ matrix.cuda_version }}-${{ matrix.arch }},mode=max | ||
| outputs: type=registry,compression=zstd,force-compression=true,compression-level=15 | ||
| push: true | ||
| 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 }} | ||
|
|
||
| - name: Apply provenance labels to Presto deps image | ||
| id: push | ||
| 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 }} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| presto_sha: ${{ inputs.presto_sha }} | ||
| presto_repository: ${{ inputs.presto_repository }} | ||
| velox_sha: ${{ inputs.velox_sha }} | ||
| velox_repository: ${{ inputs.velox_repository }} | ||
|
|
||
| - name: Generate artifact attestation | ||
| uses: actions/attest-build-provenance@96278af6caaf10aea03fd8d33a09a777ca52d62f # v3.2.0 | ||
| with: | ||
|
|
@@ -187,25 +190,26 @@ jobs: | |
| echo "tag=presto-coordinator-${PRESTO_SHORT_SHA}-${BUILD_DATE}-${BUILD_VARIANT}-${GITHUB_RUN_ID}-${ARCH}" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Build and push Presto coordinator image | ||
| id: push | ||
| uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 # v6.18.0 | ||
| with: | ||
| context: presto/docker | ||
| file: presto/docker/Dockerfile | ||
| build-args: | | ||
| PRESTO_VERSION=${{ inputs.presto_short_sha }} | ||
| labels: | | ||
| velox-testing.presto.sha=${{ inputs.presto_sha }} | ||
| velox-testing.presto.branch= | ||
| velox-testing.presto.repository=${{ inputs.presto_repository }} | ||
| velox-testing.velox.sha= | ||
| velox-testing.velox.branch= | ||
| velox-testing.velox.repository= | ||
| outputs: type=registry,compression=zstd,force-compression=true,compression-level=15 | ||
| push: true | ||
| tags: | | ||
| ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:${{ steps.tag.outputs.tag }} | ||
|
|
||
| - name: Apply provenance labels to Presto coordinator image | ||
| id: push | ||
| uses: ./velox-testing/.github/actions/apply-provenance-labels | ||
| with: | ||
| base_image: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:${{ steps.tag.outputs.tag }} | ||
| tags: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:${{ steps.tag.outputs.tag }} | ||
| presto_sha: ${{ inputs.presto_sha }} | ||
| presto_repository: ${{ inputs.presto_repository }} | ||
|
|
||
| - name: Generate artifact attestation | ||
| uses: actions/attest-build-provenance@96278af6caaf10aea03fd8d33a09a777ca52d62f # v3.2.0 | ||
| with: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,12 @@ class BenchmarkMetadata: | |
| num_drivers: int | None = None | ||
| gpu_name: str | None = None | ||
| image_digest: str | None = None | ||
| presto_sha: str | None = None | ||
| presto_branch: str | None = None | ||
| presto_repo: str | None = None | ||
| velox_sha: str | None = None | ||
| velox_branch: str | None = None | ||
| velox_repo: str | None = None | ||
|
|
||
| @classmethod | ||
| def from_parsed(cls, raw: dict) -> "BenchmarkMetadata": | ||
|
|
@@ -298,22 +304,22 @@ def _parse_args() -> argparse.Namespace: | |
| parser.add_argument( | ||
| "--velox-branch", | ||
| default=None, | ||
| help="Velox branch used to build the worker image.", | ||
| help="Velox branch used to build the worker image. Defaults to value from image labels in benchmark_result.json.", | ||
| ) | ||
| parser.add_argument( | ||
| "--velox-repo", | ||
| default=None, | ||
| help="Velox repository used to build the worker image.", | ||
| help="Velox repository used to build the worker image. Defaults to value from image labels in benchmark_result.json.", | ||
| ) | ||
| parser.add_argument( | ||
| "--presto-branch", | ||
| default=None, | ||
| help="Presto branch used to build the worker image.", | ||
| help="Presto branch used to build the worker image. Defaults to value from image labels in benchmark_result.json.", | ||
| ) | ||
| parser.add_argument( | ||
| "--presto-repo", | ||
| default=None, | ||
| help="Presto repository used to build the worker image.", | ||
| help="Presto repository used to build the worker image. Defaults to value from image labels in benchmark_result.json.", | ||
| ) | ||
| parser.add_argument( | ||
| "--concurrency-streams", | ||
|
|
@@ -387,8 +393,10 @@ def _build_submission_payload( | |
| validation_results: dict | None = None, | ||
| velox_branch: str | None = None, | ||
| velox_repo: str | None = None, | ||
| velox_sha: str | None = None, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| presto_branch: str | None = None, | ||
| presto_repo: str | None = None, | ||
| presto_sha: str | None = None, | ||
| labels: list[str] | None = None, | ||
| notes: str | None = None, | ||
| ) -> dict: | ||
|
|
@@ -413,8 +421,15 @@ def _build_submission_payload( | |
| # Use placeholders for version info if not provided | ||
| if version is None: | ||
| version = "unknown" | ||
| # 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" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| commit_parts = [] | ||
| if presto_sha: | ||
| 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" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with your note that |
||
|
|
||
| # Build query logs from results | ||
| query_logs = [] | ||
|
|
@@ -504,14 +519,19 @@ def _get_validation_result(query_name): | |
| } | ||
|
|
||
| engine_config_payload = engine_config.serialize() if engine_config else {} | ||
| if velox_branch or velox_repo or presto_branch or presto_repo: | ||
| engine_config_payload = { | ||
| **engine_config_payload, | ||
| "velox_branch": velox_branch, | ||
| "velox_repo": velox_repo, | ||
| "presto_branch": presto_branch, | ||
| "presto_repo": presto_repo, | ||
| } | ||
| # Add image provenance as separate entries; omit empties so a coordinator-only | ||
| # image (no velox fields) contributes just its presto entries. | ||
| provenance_fields = { | ||
| "presto_branch": presto_branch, | ||
| "presto_sha": presto_sha, | ||
| "presto_repo": presto_repo, | ||
| "velox_branch": velox_branch, | ||
| "velox_sha": velox_sha, | ||
| "velox_repo": velox_repo, | ||
| } | ||
| provenance_fields = {k: v for k, v in provenance_fields.items() if v} | ||
| if provenance_fields: | ||
| engine_config_payload = {**engine_config_payload, **provenance_fields} | ||
|
|
||
| payload: dict = { | ||
| "sku_name": sku_name, | ||
|
|
@@ -746,6 +766,15 @@ async def _process_benchmark_dir( | |
| ) | ||
| return 1 | ||
|
|
||
| # 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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should normalize these repository values before persisting them. Local builds populate them from the raw output of Could we strip URL userinfo in |
||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Asymmetry is intentional but slightly surprising: branch/repo allow CLI override, SHAs do not. Worth a sentence in the |
||
| presto_sha = benchmark_metadata.presto_sha | ||
|
|
||
| # Resolve config directory: explicit override → auto-detect from variant | ||
| effective_config_dir = config_dir | ||
| variant = _ENGINE_TO_VARIANT.get(benchmark_metadata.engine) | ||
|
|
@@ -849,8 +878,10 @@ async def _process_benchmark_dir( | |
| validation_results=validation_results, | ||
| velox_branch=velox_branch, | ||
| velox_repo=velox_repo, | ||
| velox_sha=velox_sha, | ||
| presto_branch=presto_branch, | ||
| presto_repo=presto_repo, | ||
| presto_sha=presto_sha, | ||
| labels=labels, | ||
| notes=notes, | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,13 @@ set -e | |
| LOGS_DIR="/opt/presto-server/logs" | ||
| mkdir -p "${LOGS_DIR}" | ||
| : "${SERVER_START_TIMESTAMP:?SERVER_START_TIMESTAMP must be set before starting the container}" | ||
|
|
||
| # 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" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we mirror this in |
||
| fi | ||
|
|
||
| log_file="${LOGS_DIR}/coordinator_${SERVER_START_TIMESTAMP}.log" | ||
|
|
||
| exec /opt/presto-server/bin/launcher run >> "${log_file}" 2>&1 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| ARG BASE_IMAGE | ||
| ARG BASE_IMAGE=invalid | ||
| FROM ${BASE_IMAGE} | ||
|
|
||
| ARG PRESTO_SHA | ||
|
|
@@ -13,3 +13,8 @@ LABEL velox-testing.presto.sha=${PRESTO_SHA} \ | |
| velox-testing.velox.sha=${VELOX_SHA} \ | ||
| 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' \ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor robustness nit: building the JSON with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note, the same printf pattern in |
||
| "${PRESTO_SHA}" "${PRESTO_BRANCH}" "${PRESTO_REPOSITORY}" \ | ||
| "${VELOX_SHA}" "${VELOX_BRANCH}" "${VELOX_REPOSITORY}" \ | ||
| > /opt/velox-testing/provenance.json | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,7 +135,8 @@ ${CONFIGS}/etc_coordinator/node.properties:/opt/presto-server/etc/node.propertie | |
| ${CONFIGS}/etc_coordinator/config_native.properties:/opt/presto-server/etc/config.properties,\ | ||
| ${CONFIGS}/etc_coordinator/catalog/hive.properties:/opt/presto-server/etc/catalog/hive.properties,\ | ||
| ${DATA}:/var/lib/presto/data/hive/data/user_data,\ | ||
| ${VT_ROOT}/.hive_metastore:/var/lib/presto/data/hive/metastore${extra_mounts} \ | ||
| ${VT_ROOT}/.hive_metastore:/var/lib/presto/data/hive/metastore,\ | ||
| ${LOGS}:/opt/presto-server/logs${extra_mounts} \ | ||
| -- bash -lc "unset JAVA_HOME; export JAVA_HOME=/usr/lib/jvm/jre-17-openjdk; export PATH=/usr/lib/jvm/jre-17-openjdk/bin:\$PATH; ${script}" >> ${LOGS}/${log_file} 2>&1 & | ||
| else | ||
| srun -w $COORD --ntasks=1 --overlap \ | ||
|
|
@@ -149,8 +150,9 @@ ${CONFIGS}/etc_coordinator/node.properties:/opt/presto-server/etc/node.propertie | |
| ${CONFIGS}/etc_coordinator/config_native.properties:/opt/presto-server/etc/config.properties,\ | ||
| ${CONFIGS}/etc_coordinator/catalog/hive.properties:/opt/presto-server/etc/catalog/hive.properties,\ | ||
| ${DATA}:/var/lib/presto/data/hive/data/user_data,\ | ||
| ${VT_ROOT}/.hive_metastore:/var/lib/presto/data/hive/metastore,\ | ||
| ${LOGS}:/var/log/nsys,\ | ||
| ${VT_ROOT}/.hive_metastore:/var/lib/presto/data/hive/metastore${extra_mounts} \ | ||
| ${LOGS}:/opt/presto-server/logs${extra_mounts} \ | ||
| -- bash -lc "unset JAVA_HOME; export JAVA_HOME=/usr/lib/jvm/jre-17-openjdk; export PATH=/usr/lib/jvm/jre-17-openjdk/bin:\$PATH; ${script}" >> ${LOGS}/${log_file} 2>&1 | ||
| fi | ||
| } | ||
|
|
@@ -173,6 +175,13 @@ unset CONFIG NODE_CONFIG PRESTO_ETC JAVA_TOOL_OPTIONS JDK_JAVA_OPTIONS _JAVA_OPT | |
|
|
||
| export JAVA_HOME=/usr | ||
| export PATH=/usr/bin:$PATH | ||
|
|
||
| # Surface baked-in image provenance into the shared logs dir for the in-container | ||
| # pytest in run_benchmark.sh to read via LOGS_DIR. | ||
| if [ -f /opt/velox-testing/provenance.json ]; then | ||
| cp /opt/velox-testing/provenance.json "/opt/presto-server/logs/coordinator_provenance.json" | ||
| fi | ||
|
|
||
| /opt/presto-server/bin/launcher run & srv=$! | ||
|
|
||
| # wait for JVM to appear | ||
|
|
@@ -345,6 +354,7 @@ ${worker_hive}:/opt/presto-server/etc/catalog/hive.properties,\ | |
| ${worker_data}:/var/lib/presto/data,\ | ||
| ${DATA}:/var/lib/presto/data/hive/data/user_data,\ | ||
| ${VT_ROOT}/.hive_metastore:/var/lib/presto/data/hive/metastore,\ | ||
| ${LOGS}:/opt/presto-server/logs,\ | ||
| ${WORKER_ENV_FILE}:${vt_worker_env_file},\ | ||
| ${LOGS}:${vt_cufile_log_dir},\ | ||
| ${LOGS}:${vt_nsys_report_dir}${driver_mounts}${gds_mounts:+,${gds_mounts}}${worker_extra_mounts} \ | ||
|
|
@@ -373,6 +383,10 @@ echo \"Worker ${worker_id}: CUFILE_LOGFILE_PATH=\${CUFILE_LOGFILE_PATH:-unset}\" | |
| echo \"Worker ${worker_id}: KVIKIO_TASK_SIZE=\${KVIKIO_TASK_SIZE:-unset}\" | ||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With multi-worker runs, every worker writes the same path |
||
| fi | ||
|
|
||
| if [[ -n '${nsys_bin}' ]]; then | ||
| ( | ||
| echo \"Worker ${worker_id}: nsys subshell started\" | ||
|
|
@@ -573,6 +587,7 @@ function run_queries { | |
| export PRESTO_DATA_DIR=/var/lib/presto/data/hive/data/user_data; \ | ||
| export MINIFORGE_HOME=/workspace/miniforge3; \ | ||
| export HOME=/workspace; \ | ||
| export LOGS_DIR=/opt/presto-server/logs; \ | ||
| cd /workspace/presto/scripts; \ | ||
| ./run_benchmark.sh -b tpch -s tpchsf${scale_factor} -i ${num_iterations} ${extra_args[*]} \ | ||
| --hostname ${COORD} --port $PORT -o ${container_script_dir}/result_dir --skip-drop-cache" "cli" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.