felipepessoto commented on code in PR #12388: URL: https://github.com/apache/gluten/pull/12388#discussion_r3669855867
########## .github/workflows/util/delta-spark-ut/run-delta-tests.sh: ########## @@ -0,0 +1,275 @@ +#!/usr/bin/env bash + +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +# Runs the Delta `spark` module tests for one shard under the Gluten bundle: +# arms a hang watchdog (thread-dumps + kills a wedged fork), invokes sbt +# spark/test with the tuned JVM/heap flags, prints cgroup memory forensics, and +# then gates the results against the baseline (compare-test-results.py). Extracted +# from delta_spark_ut.yml so the workflow step stays readable. +# +# Driven by environment (set by the workflow step / job): +# SHARD_ID - this shard's id (matrix.shard) +# SPARK_VERSION - Delta -DsparkVersion value +# UPDATE_BASELINE - 'true' -> gate seed mode; else enforce +# FAIL_ON_FIXED - passed through to the gate +# DELTA_SCALA_VERSION, NUM_SHARDS, TEST_PARALLELISM_COUNT, DELTA_TESTING +# - test env (see the workflow step's `env:` block) +# GITHUB_WORKSPACE - repo root (holds the Delta clone + util scripts) +# +# JAVA_TOOL_OPTIONS is set by sourcing java-test-args.sh (below), not the caller. + +set -euo pipefail +export JAVA_HOME=/usr/lib/jvm/java-17-openjdk +export PATH=$JAVA_HOME/bin:$PATH +# Gluten/JDK17 test JVM flags (--add-opens + Netty property), shared with local +# dev runs. Sets JAVA_TOOL_OPTIONS so it reaches the sbt launcher + forked JVMs. +# shellcheck source=./java-test-args.sh +source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/java-test-args.sh" +cd "$GITHUB_WORKSPACE/delta" +chmod +x build/sbt +# Only run the unified `spark` sbt project, NOT `sparkGroup/test` -- +# `sparkGroup` aggregates many other projects (sparkV2, contribs, +# sharing, connect*, ...) that are out of scope for this pipeline. +# +# JVM heap layout -- two memory consumers on the ~16G runner: +# * sbt launcher JVM: -J-Xmx4G for the test compile, then forced to +# return idle memory during the (long) test phase via G1 periodic GC +# (G1PeriodicGCInterval=10s; G1PeriodicGCSystemLoadThreshold=0 so the +# busy fork doesn't suppress it; -XX:-G1PeriodicGCInvokesConcurrent +# forces each periodic GC to a full STW collection) that uncommits to a +# tight free ratio (Min/MaxHeapFreeRatio 5/15, JEP 346) above a low +# -Xms512m floor. +# Without this the idle launcher holds ~5.3G for the whole run; with +# it, it drops back to ~1-2G. These flags touch no Gluten/Spark runtime +# config, so they cannot affect the measured pass/fail signal. +# * Forked test JVM: -Xmx2G via the `set ... Test / javaOptions` command +# below. Delta caps its fork at -Xmx1024m in build.sbt; `++=` appends +# so our -Xmx2G comes last and wins. Gluten offloads data to Velox +# off-heap (capped at 2g via spark.memory.offHeap.size in the patched +# DeltaSQLCommandTest), so the fork's heap need is modest. A larger +# fork heap pushed the cgroup peak past the ~16G OOM threshold and the +# kernel OOM-killed the fork mid-shard (no hs_err), wedging sbt -- 2G +# keeps headroom. Keep heap-dump-on-OOM so a real >2G heap OOM is +# analyzable. +# `-u target/test-reports` enables ScalaTest's JUnit XML reporter so +# every suite writes per-test results. Delta itself only configures +# the console reporter (-oDF), so without this we'd have no machine- +# readable results to gate on. The path is relative to the forked +# test JVM's working dir (Test / baseDirectory = spark/), i.e. +# delta/spark/target/test-reports/TEST-*.xml. +# +# We deliberately do NOT let an sbt non-zero exit (which fires on the +# MANY expected Delta-on-Gluten failures) fail this step directly. +# Instead the known-failures gate below decides pass/fail: the build +# is green when the only failures are ones already recorded in the +# baseline, and red on a genuine regression. +set +e +# --- hang watchdog --------------------------------------------------- +# Shard 2 (and occasionally others) hangs indefinitely after a suite's +# last test with no further output. ScalaTest's failAfter only wraps +# individual test BODIES, so a wedge in suite teardown/afterAll -- or in +# a non-interruptible native Velox/JNI call that ignores +# Thread.interrupt() -- has no timeout and stalls until the 350-min job +# limit with zero diagnostics. This watchdog dumps the forked test JVM's +# threads (to the job log, and to a file for the artifact) once the test +# output has been silent for too long, so the deadlock is diagnosable. +SBT_LOG="/tmp/sbt-spark-test-shard-${SHARD_ID}.log" +# Marker the watchdog touches when it KILLS a wedged test fork. The killed fork's +# running suite plus every suite queued behind it never run and never write a +# report, and since we ignore sbt's exit code the gate would only judge the +# suites that DID report -- so the main flow fails the shard when this exists. +WATCHDOG_KILL_MARKER="/tmp/sbt-watchdog-killed-shard-${SHARD_ID}" +: > "$SBT_LOG" +rm -f /tmp/sbt-done "$WATCHDOG_KILL_MARKER" Review Comment: Valid — fixed in 15f06bad5. You're right that it was inconsistent: the script already scopes `SBT_LOG` and `WATCHDOG_KILL_MARKER` by `SHARD_ID`, and the done marker was the odd one out. It's now `/tmp/sbt-done-shard-${SHARD_ID}` via a named `SBT_DONE_MARKER`, covering all three sites you flagged (the `rm -f`, the watchdog's wait loop, and the `touch` after sbt returns). I reproduced the arm/disarm handshake with a fast shard running alongside a slow one to confirm the impact was real rather than cosmetic: | done marker | slow shard's watchdog | |---|---| | `/tmp/sbt-done` (global) | disarmed after **3** ticks — hang detection lost | | `/tmp/sbt-done-shard-N` | stayed armed all **10** ticks — correct | In CI each shard gets its own container, so this was latent there; it matters for parallel local runs, which is exactly the case you described. ########## .github/workflows/velox_backend_x86.yml: ########## @@ -101,6 +106,80 @@ jobs: path: ./cpp/build/ if-no-files-found: error + # Gate the (expensive) Delta Spark UT suite so per-PR it runs only when the PR + # touches high-signal Delta paths -- the Delta integration code + # (backends-velox/src-delta*), the gluten-delta module, or this pipeline's own + # files -- or carries the `run-delta-ci` opt-in label. Changes to general + # Velox/core/native code can also affect Delta offload but are touched + # constantly, so per-PR they skip it; the nightly full run (delta_spark_ut.yml + # `schedule`) and the opt-in label are the safety nets. This keeps GHA usage + # down. NOTE: the label is read from the event that triggered this run, so add + # it before/with a push; labeling an already-finished run needs a new push. + delta-changes: + runs-on: ubuntu-22.04 + outputs: + run_delta: ${{ steps.filter.outputs.run_delta }} + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Detect Delta-relevant changes / opt-in label + id: filter + env: + HAS_LABEL: ${{ contains(github.event.pull_request.labels.*.name, 'run-delta-ci') }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} Review Comment: Checked again, but I don't think this applies — no change. (Same underlying concern as [#discussion_r3659868584](https://github.com/apache/gluten/pull/12388#discussion_r3659868584) and [#discussion_r3660828861](https://github.com/apache/gluten/pull/12388#discussion_r3660828861).) Two independent reasons: 1. **The premise can't occur here.** Parsing this workflow's triggers yields exactly `['pull_request']` — there is no `schedule`, `push` or `workflow_dispatch` entry — so `github.event.pull_request` is always present. (The reusable `delta_spark_ut.yml` has the other triggers, but it doesn't evaluate this expression.) 2. **Even on a non-PR event it wouldn't error.** Actions coerces a missing context to a falsy empty value rather than failing evaluation — per the [expressions reference](https://docs.github.com/en/actions/reference/workflows-and-actions/expressions), `Null` casts to `''` and falsy values coerce to `false`. Concretely, [`n8n-io/n8n`'s `release-storybook.yml`](https://github.com/n8n-io/n8n/blob/master/.github/workflows/release-storybook.yml) runs on a nightly `schedule` and evaluates `!contains(github.event.pull_request.labels.*.name, 'community')` with no event guard at all — it would break every night if this failed evaluation. The pattern is paired with `schedule:` in well over a thousand public workflows. So an `event_name == 'pull_request'` guard would be unconditionally true here and would only add noise. The shell-level fail-open on missing SHAs stays as belt-and-suspenders. ########## .github/workflows/velox_backend_x86.yml: ########## @@ -101,6 +106,80 @@ jobs: path: ./cpp/build/ if-no-files-found: error + # Gate the (expensive) Delta Spark UT suite so per-PR it runs only when the PR + # touches high-signal Delta paths -- the Delta integration code + # (backends-velox/src-delta*), the gluten-delta module, or this pipeline's own + # files -- or carries the `run-delta-ci` opt-in label. Changes to general + # Velox/core/native code can also affect Delta offload but are touched + # constantly, so per-PR they skip it; the nightly full run (delta_spark_ut.yml + # `schedule`) and the opt-in label are the safety nets. This keeps GHA usage + # down. NOTE: the label is read from the event that triggered this run, so add + # it before/with a push; labeling an already-finished run needs a new push. + delta-changes: + runs-on: ubuntu-22.04 + outputs: + run_delta: ${{ steps.filter.outputs.run_delta }} + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Detect Delta-relevant changes / opt-in label + id: filter + env: + HAS_LABEL: ${{ contains(github.event.pull_request.labels.*.name, 'run-delta-ci') }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -euo pipefail + # Opt-in label forces the suite even with no Delta-relevant path change. + if [ "$HAS_LABEL" = "true" ]; then + echo "run-delta-ci label present -> running Delta suite" + echo "run_delta=true" >> "$GITHUB_OUTPUT"; exit 0 + fi + # Fail open if we can't determine the PR range (e.g. a non-PR trigger): + # never silently skip coverage. + if [ -z "${BASE_SHA:-}" ] || [ -z "${HEAD_SHA:-}" ]; then + echo "no PR base/head sha -> running Delta suite (fail-open)" + echo "run_delta=true" >> "$GITHUB_OUTPUT"; exit 0 + fi + BASE=$(git merge-base "$BASE_SHA" "$HEAD_SHA" 2>/dev/null || echo "$BASE_SHA") + echo "diff base=$BASE head=$HEAD_SHA" + # Fail open if the diff itself can't be computed (missing objects after a + # force-push race, an unfetched fork head, ...). Piping straight into + # `grep -q` inside an `if` would hide that: git's failure leaves grep with + # empty input, so the pipeline exits non-zero exactly as it does for "no + # match" -- and `set -e`/`pipefail` can't help, since a tested command is + # allowed to fail. Capture the diff first so the two cases stay distinct. + if ! CHANGED=$(git diff --name-only "$BASE" "$HEAD_SHA"); then Review Comment: Good hypothesis, but it's empirically not what happens — no change. This PR is itself a fork PR (`felipepessoto/gluten` → `apache/gluten`), so it exercises exactly the path in question. Across every run of the gate so far, `git diff` succeeded and the gate reached a real decision — it never hit the fail-open branch: ``` diff base=405e2b67e... head=3e5d4947b... Delta-relevant paths changed -> running Delta suite ``` (4/4 runs printed `Delta-relevant paths changed`; none printed `git diff failed -> ... (fail-open)`.) The reason it works is that the job checks out `refs/pull/<n>/merge` with `fetch-depth: 0`, and that merge commit's **second parent is the fork head commit** — so `github.event.pull_request.head.sha` is present in the local object store, along with the base. Switching to `github.sha` would actually diff the *merge* commit, which also pulls in base-branch changes since the fork point and would make the gate fire more often, not less. The fail-open branch stays as the safety net for genuine diff failures, which is what it was added for. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
