ci: add dev/local-ci.sh to run the Spark SQL and Iceberg suites locally - #5974
Conversation
github-actions
Bot
added
build
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
I checked this out and exercised the script rather than just reading it. All six config readers parse today's files correctly, and with the expensive steps stubbed the emitted sbt and Gradle command lines match the workflow steps for all 7 Spark rows and all 6 Iceberg targets, including the args2 quoting. git apply on the non-git tarball tree works, and so does the reverse-check idempotence guard. The approach holds up. A few things below before this comes out of draft.
One thing that doesn't fit on a line: the description says clones default to ~/comet-local-ci, but the script and the docs both say /tmp/comet-local-ci. Looks like it didn't get updated after the "stage sources under /tmp" commit.
| java="$(input "$job" java)" | ||
| # ci.yml leaves `scala` unset, so the reusable workflow default applies. | ||
| scala="$(awk '/^ scala:/ { f = 1 } f && /^ default:/ { gsub(/[^0-9.]/, "", $2); print $2; exit }' "$ICEBERG_YML")" | ||
| shards="$(sed -n 's/^SHARD_COUNT = //p' "$REPO/dev/ci/check-iceberg-shards.py")" |
There was a problem hiding this comment.
Could shards get the same | grep . || die guard that default_version and input have? I tried it with the parse returning empty and the failure mode is worse than I expected. while [ "$i" -le "$shards" ] prints one stderr line, falls through, and the target list ends up as just extensions runtime, so the script prints done and exits 0 having skipped every sharded core test. A false green is the one outcome worth ruling out for a tool people will use to decide whether something is safe to queue.
scala on the line above has the same gap and would quietly give you iceberg-spark-4.1_:test, and gated at line 224 would drop the 4.0 process isolation without saying so.
| # .github/workflows/spark_sql_test_reusable.yml and | ||
| # .github/workflows/iceberg_spark_test_reusable.yml. | ||
| # | ||
| # Versions, matrix rows and the shard count are read from ci.yml and dev/ci/ at |
There was a problem hiding this comment.
This adds six new readers of CI config, and check-ci-config.py opens by saying it guards six invariants that are silent when broken, with five more drift-guards beside it in preflight. Requoting spark-full: '4.1.3' or reindenting a with: block breaks this script, and we don't find out until someone actually needs it, which is the worst possible moment.
I agree a new CI job isn't warranted. Would a --print-config mode plus an assertion inside one of the existing preflight checks work? That's seconds of CI time. Worth noting too that actionlint runs with --shellcheck=off and only covers workflow files, so nothing lints this script either.
| } | ||
|
|
||
| # git apply works fine outside a repository, so this covers both. | ||
| apply_diff() { |
There was a problem hiding this comment.
I think editing a diff and re-running gets stuck here. First run applies it, you edit dev/diffs/4.1.3.diff, and the second run's reverse check fails and then git apply fails with "patch does not apply". Since the Spark tree has no .git there's nothing to reset it to, so the only way forward is deleting the tree and paying for the download and the full sbt precompile again.
AGENTS.md lists dev/diffs/ as a reason to run this, so that's the main use case landing on the worst path. Iceberg is a real clone, so git checkout . && git clean -fd before applying would handle that half. For Spark, could we re-extract when the reverse check fails, or at minimum print which directory to delete?
| # Coursier then calls the artifact found-locally rather than falling back to | ||
| # Maven Central. Both workflows drop the tree for that reason. | ||
| purge_parquet() { | ||
| dir="${MAVEN_REPO_LOCAL:-$HOME/.m2/repository}/org/apache/parquet" |
There was a problem hiding this comment.
Is MAVEN_REPO_LOCAL something Maven actually reads? I can't find it anywhere else in the repo, and the real overrides are <localRepository> in settings.xml and -Dmaven.repo.local. For anyone who has relocated their repo, the [ -d "$dir" ] || return 0 below makes both this and purge_partial_poms no-ops, and they then hit the exact Coursier "found locally, no JAR" failure the purge exists to prevent. ./mvnw help:evaluate -Dexpression=settings.localRepository would give you the real path.
| of the same name runs. It prepares first (native build, Comet install, patched Spark or Iceberg | ||
| source under `$COMET_LOCAL_CI_HOME`, default `/tmp/comet-local-ci`), then runs the tests. Only the | ||
| sbt projects the selected rows need are compiled, so asking for `catalyst` does not build the sql | ||
| or hive test classes. `SKIP_PREPARE=1` skips to the tests when the sandbox is already current. |
There was a problem hiding this comment.
"When the sandbox is already current" is doing a lot of work here. SKIP_PREPARE=1 skips install_comet too, so someone who just changed the serde and re-ran a shard would be testing the previously installed JAR and getting a green that means nothing. Could that be spelled out, something like "skips the Comet install too, so don't use it after changing Comet"?
| or hive test classes. `SKIP_PREPARE=1` skips to the tests when the sandbox is already current. | ||
|
|
||
| Three caveats. The sandbox lives under `/tmp`, so a reboot or a tmp reaper means downloading and | ||
| compiling again. Preparing runs `rm -rf ~/.m2/repository/org/apache/parquet`, as the workflows do. |
There was a problem hiding this comment.
This names the parquet rm -rf but not purge_partial_poms, which walks all of ~/.m2/repository and deletes POMs. That's 8,818 POMs on my machine. It's recoverable since Maven refetches, but AGENTS.md tells agents to run this unattended, so could the caveat mention that the sweep covers the whole shared repository and not just Comet and Spark artifacts?
|
Thanks for exercising it rather than just reading it. All six points were sound and are fixed in f34a1ae. I reproduced each one first, and two of them turned out slightly different from the description, noted below. The One stderr line, all four core shards dropped, exit 0. The edited-diff wedge. Reproduced:
Now resolved through
One thing worth flagging, because it changed the design: comparing two parsers is not sufficient on its own. My first version passed a requoted
Both doc points. The description. Fixed, along with a stale reference to cloning Spark: it is fetched as a tag archive now, while Iceberg still needs a real clone because its build derives the version from the Still true and unchanged from the description: no full green suite run end to end. The furthest I have got locally is the |
|
Depends on #5987 |
a44ed27 to
187428a
Compare
|
Rebased onto Worth flagging since #5973 landed in the meantime: I independently hit the same cargo-cache eviction while measuring where CI time goes, and the numbers matched yours. Good to see it fixed on Changes since your review:
On testing, so the state is clear. A real |
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Correctness
The existing guides require contributors to reproduce the Spark SQL and Iceberg workflow commands manually. This change provides one local runner, reads versions and row definitions from CI, prepares the native and JVM artifacts, and documents when agents should request heavier validation. I reviewed all seven authored files at 187428a5 against 58ab5f61, including the existing review and the author's subsequent revisions.
The current config resolves four Spark versions and four Iceberg versions, with Spark 4.1 and Iceberg 1.11 as defaults. Focused subprocess-double checks cover all seven Spark rows at every configured version and all six Iceberg targets at every configured version. The emitted test selectors, core heap caps, Spark 4.0 dedicated-JVM gate and Iceberg shard properties match the workflow definitions. Ordinary native-build and test failures propagate, and the recorded-diff recovery handles an initial apply, an unchanged rerun and an edited patch.
Three P2 findings remain inline. Failed config parsing can return success without tests on Bash 3.2. Overlapping row selectors can replace a running row's directory. A later tree-copy failure exits without awaiting an earlier row. The first is reproduced against the unmodified script. The concurrency checks use bounded doubles and establish shell behavior, not Spark test results.
The earlier discussion's config parser, Maven-repository lookup, edited-diff handling and documentation changes are present. The shared-cache sweep and stale-artifact risk of SKIP_PREPARE=1 are now explicit. No expression or operator implementation changes, so this review makes no new Spark semantic compatibility claim.
Current CI has 23 successful and 14 skipped checks. Logs confirm merge 1d982a81 has the pinned base and head as parents, and its tree equals the reviewed head. Preflight passed formatting and CI config checks. The Comet exec group ran 916 tests successfully. The heavy Spark SQL and Iceberg jobs were skipped, so this does not validate the new runner end to end. The author reports a real concurrent Spark run with disk-space failures. I did not run the full suites locally.
Performance
Preparing once and compiling only the selected sbt projects avoids repeating the initial build for each requested row. Concurrent rows retain CI's per-row JVM settings, while the documentation explains the combined memory and disk cost and permits selecting fewer rows. Duplicate selectors currently defeat that isolation and repeat work, as covered inline. There is no measured speedup established by the local probes, and no runtime kernel change requiring a microbenchmark.
Design
The prepare-then-run structure and shared CI configuration make the commands easier to reproduce. Single-row execution avoids an extra tree copy, and retaining row reports supports investigation. The lifecycle needs to account for preparation failures after concurrency begins. Preparing every row directory before spawning, or reliably draining tracked children on exit, gives callers a clear point at which the run has stopped.
Abstraction & complexity
Moving the configuration parsing into one Python module removes the competing shell and Python parsers from the earlier revision. Reading the existing matrix and shard definitions avoids another version table to maintain. The shell-to-Python boundary should preserve failure status explicitly, and selector expansion should produce a unique row list before execution. Focused negative-path checks at those boundaries would cover behavior that the current syntax and config-shape checks do not exercise.
| # validates shapes and fails loudly, so nothing below has to re-guard a parse. | ||
| # shellcheck disable=SC2153 # VERSION/FULL/JAVA/ROWS/... all come from here | ||
| load_config() { | ||
| eval "$(cd "$REPO" && python3 dev/ci/local-ci-config.py --shell "$@")" || |
There was a problem hiding this comment.
Correctness
[P2] Check the parser status before evaluating its output
Could the command substitution be assigned and checked separately before calling eval? eval "$(...)" returns the status of eval, so an empty result from a failed parser succeeds and bypasses die. On the explicitly supported macOS Bash 3.2, I ran the unmodified /bin/bash dev/local-ci.sh spark nonexistent and spark 9.9. Both report the parser error, hit DEFAULTED: unbound variable, print the green runtime message and exit 0 without running a test. The parser itself returns 1. Checking the assignment first makes both cases exit 1. This needs a negative-path check because callers use the exit status to decide whether local validation passed.
| picked = [] | ||
| for want in selectors or ["all"]: | ||
| if want in ("all", "core", "hive"): | ||
| picked += [r for r in rows if want == "all" or r["group"] == want] |
There was a problem hiding this comment.
Correctness
[P2] Deduplicate selected rows before launching them concurrently
Could selection preserve each row only once, or reject overlaps before preparation? spark core sql_core-1 currently selects sql_core-1 twice. run_spark_rows gives both copies the same tree and log path, and the second clone_tree removes that directory while the first process is still using it. With bounded test doubles I observed the first row's directory inode change during execution, and another run failed to open build/sbt after the deletion. Both invocations also write the same log. The disjoint core hive control launches seven distinct rows without replacement. Deduplicating by row name preserves the promised per-row isolation and avoids repeating the expensive suite.
| while IFS=$'\037' read -r name args1 args2 heap metaspace; do | ||
| [ -n "$name" ] || continue | ||
| tree="$dest-$name" | ||
| clone_tree "$dest" "$tree" |
There was a problem hiding this comment.
Correctness
[P2] Reap launched rows when a later tree copy fails
Could every exit path clean up or await the rows already launched? This loop starts a row before preparing the next tree. If a later copy fails, set -e exits before await_rows, and the EXIT trap only prints timing. In a focused probe using this control flow, an injected second-copy failure made the parent exit 73 while the first bounded child was still running. I then awaited that child separately. With real sbt rows, a disk-full copy failure can leave expensive JVMs and report writers running after the command has failed, and a retry removes their directories. Tracking the children in an exit cleanup path, or preparing all selected trees before launching any row, would close this failure path.
The Spark SQL and Iceberg suites do not run on an unlabeled pull request. Spark
4.1 and Iceberg 1.11 first report in the merge queue, where a failure evicts the
pull request and blocks everyone else's merges. Spark 3.5, 4.0 and Iceberg
1.8/1.9/1.10 first report in the nightly run, after the change has landed. Those
two workflows are 522 of the 891 runner-minutes a queue pipeline costs.
The commands are documented but nothing executes them, so every local run is
retyped. The parts that are easy to get wrong are the parts that decide whether
a local pass means anything: the -l/-n tag splits, the per-row heap caps, the
Iceberg shard init script, and the Maven cache purges the workflows depend on.
dev/local-ci.sh spark every Spark SQL matrix row
dev/local-ci.sh spark sql_core-1 one row, or all/core/hive
dev/local-ci.sh iceberg every Iceberg target
dev/local-ci.sh iceberg shard-2 one shard, or extensions/runtime
Selecting more than one Spark row runs them all at once, each in its own copy of
the prepared tree. That is what CI does -- one runner and one extracted
apache-spark/ per matrix row -- so the per-row settings stay byte-identical to
the workflow's rather than diverging to get parallelism. Budget a full tree per
row: the copies are copy-on-write at first but diverge almost completely as each
row recompiles and writes its own reports.
dev/ci/local-ci-config.py is the single parser for everything the script reads:
versions and the JDK from ci.yml, the matrix rows from spark-sql-modules.py, the
shard count from check-iceberg-shards.py, the Iceberg Scala default and the
DEDICATED_JVM_SBT_TESTS gate from the reusable workflows, and the default version
from POLICY in compute-changes.py. It validates every value against a version
shape, so a requoted or reindented input fails loudly instead of producing a URL
with quotes in it. check_local_ci_config imports it during preflight and adds
bash -n on the script, because actionlint runs with --shellcheck=off and only
looks at workflow files.
Three failure modes found while using it, each guarded now: Comet's install
leaves POMs whose JARs it never fetched, which Coursier reports as a missing JAR
it can see a POM for; purging those invalidates any resolution sbt has cached,
which a plugin like sbt-antlr4 reads instead of the filesystem; and editing a
diff used to wedge the tree until it was deleted, so the applied diff is recorded
and reverted before a new one goes on.
Three findings from review, each reproduced first. `eval "$(...)"` reports
eval's own status, so a parser that failed and printed nothing looked like
success and the run exited 0 having tested nothing; the assignment is captured
and checked before evaluating. Overlapping selectors such as `core sql_core-1`
named a row twice, and because a row's tree and log are keyed on its name the
duplicate deleted the tree the first copy was running in; selection now
deduplicates by name. A tree copy failing after earlier rows had launched exited
on set -e without awaiting them, leaving sbt processes behind; every tree is
copied before any row starts.
Rewrite "Reproducing a suite failure locally" in the CI guide around dev/local-ci.sh: the commands, SKIP_PREPARE and what it skips, how row-level concurrency works, and the caveats that matter. The sandbox lives under /tmp so a reboot costs a recompile, preparing sweeps the whole shared Maven repository rather than just Comet and Spark artifacts, running every row at once wants a full tree each, and CI is x86_64 so a pass elsewhere does not cover x86-specific native codegen. Point the Spark SQL and Iceberg test guides at it, keeping their manual steps as the reference since those are also the diff-regeneration workflow. AGENTS.md gains "Checking a change against CI": a green pull request only covers the PR tier, so a change touching the serde, the planner, a native operator, a Spark shim, an Iceberg path or dev/diffs/ needs either a local run or the matching run-* label before it is queued. It names the cost of each so the choice is deliberate, and flags the two things that matter when an agent runs it unattended.
187428a to
ed904dc
Compare
|
Thanks — all three were real and are fixed in Failed parser exited 0. Confirmed exactly as you describe on
Duplicate rows. Orphaned rows on a copy failure. I took the "prepare all trees before launching any row" option rather than tracking children in an exit path — there is no bookkeeping to get wrong, and the likely trigger, a full disk, then surfaces before anything expensive starts. With the second copy injected to fail: exit 1, zero rows launched, zero strays, against a running orphan before. The happy path is still concurrent, three rows of 2s finishing in 2s wall across three distinct trees. On your note that the heavy jobs were skipped so this is not validated end to end: agreed, and it still is not. The closest I have is the real Spark 4.1 run in my previous comment, where |
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed ed904dc4 against 23a9ff64, following 187428a5. All three P2 findings are addressed:
- Failed configuration now exits 1 before
eval. The original invalid-target and invalid-version cases no longer return success on Bash 3.2. - Overlapping selectors run each row once. Both selector orders and a repeated single row retain distinct, stable working directories.
- Every row tree is prepared before launching tests. Injected failures at the first, second and third copy launch zero rows. Ordinary test failures still propagate after the started rows are awaited.
I found no new or remaining P1/P2 issues. The lifecycle checks used bounded test doubles, and every started child completed and was awaited. No real product builds or Spark/Iceberg suites ran locally.
Current CI has 23 successful and 14 skipped checks. Logs confirm the assigned head/base merge, with both updated files byte-identical to the reviewed head. Preflight passed, and the exec group passed 926 Scala tests. The heavy Spark SQL and Iceberg jobs were skipped, so full end-to-end validation of the local runner remains unverified.
3 participants
Which issue does this PR close?
Closes #5966.
That issue asks for two things: a way to run the heavy suites locally, and a way for agents to run
local checks before a merge. Both are here.
Rationale for this change
The Spark SQL and Iceberg suites do not run on an unlabeled pull request. Spark 4.1 and Iceberg
1.11 first report in the merge queue, where a failure evicts the pull request and stalls everyone
else's merges. Spark 3.5, Spark 4.0 and Iceberg 1.8/1.9/1.10 first report in the nightly run,
after the change has already landed.
Measured from queue run
35022044196, those two
workflows are 522 of the 891 runner-minutes a queue pipeline costs:
sql_hive-157 minextensions55 min (unsharded)The commands to run these locally are already documented in
docs/source/contributor-guide/spark-sql-tests.mdandiceberg-spark-tests.md, but nothingexecutes them, so every local invocation is retyped by hand. The parts that are easy to get wrong
are exactly the parts that decide whether a local pass means anything: the
-l/-ntag splitsbetween the
sql_coreandsql_hiveshards, the per-shardHEAP_SIZEandMETASPACE_SIZEcaps,the Iceberg shard init script and its index/count properties, the
DEDICATED_JVM_SBT_TESTSgate,and the Parquet cache purge both workflows depend on.
What changes are included in this PR?
One script,
dev/local-ci.sh, that builds the same sandbox a runner builds and then runs eitherworkflow:
Prepare runs first (
cargo build --profile ci, stage the library where-Preleaselooks for it,fetch and patch,
mvnw install -Prelease -DskipTests, and for Spark only theTest/compileofthe sbt projects the selected rows actually need), then the tests.
SKIP_PREPARE=1goes straightto the tests, and skips the Comet install with them, so it must not be used after changing Comet.
Sources live under
$COMET_LOCAL_CI_HOME, default/tmp/comet-local-ci, outside the repositorybecause
apache-spark/is not in.gitignore. Spark is fetched as a tag archive rather thancloned; Iceberg still needs a clone, because its build derives the project version from the
apache-iceberg-*tag viacom.palantir.git-version.Nothing version-shaped or row-shaped is restated in the script. It is read from the existing
sources at run time:
POLICYindev/ci/compute-changes.pywith:blocks of.github/workflows/ci.ymldev/ci/spark-sql-modules.pydev/ci/check-iceberg-shards.pyDEDICATED_JVM_SBT_TESTSand its gate.github/workflows/spark_sql_test_reusable.ymldev/local-ci.sh --print-configreports all of it, andcheck_local_ci_configindev/ci/check-ci-config.pyasserts it against the same sources parsed independently in Python,plus
bash -non the script. Preflight therefore fails on a requoted value, a reindentedwith:block or a syntax error, rather than the next time somebody needs the script.
So a version bump, a new matrix row or a change to the shard count needs no edit here. Older
versions are still reachable for reproducing a nightly failure, with
dev/local-ci.sh spark 3.5 sql_core-1.Docs: a "Reproducing a suite failure locally" rewrite in
contributor-guide/ci.md, and a pointerat the top of the Spark SQL and Iceberg test guides. The manual steps in those guides stay as the
reference, since they are also the diff-regeneration workflow.
For the agent half of the issue,
AGENTS.mdgains a "Checking a change against CI" section. Itstates that a green pull request only covers the PR tier, lists what a change has to touch to
warrant more (serde, planner, a native operator, a Spark shim, an Iceberg path,
dev/diffs/), andsays to pick either a local
dev/local-ci.shrun or the matchingrun-*label. It also names thecost, hours of compute and tens of GB of disk, so the choice is deliberate rather than an agent
running the full matrix for a docs change.
How are these changes tested?
No new CI job. This is a developer script, and
dev/ci/compute-changes.pyconfirms a PR touchingonly
dev/local-ci.shanddocs/**triggers nothing beyond preflight, which is intended.Checked locally:
shellcheckandbash -nclean. Written for bash 3.2, which is what/bin/bashis on macOS.dev/ci/check-ci-config.py,dev/ci/check-suites.py,prettier --check "**/*.md"andapache-rat:checkall pass.versus target disambiguation, all 7 Spark rows and the
all/core/hivegroups against thecounts
spark-sql-modules.pyreports, all 4 Iceberg shards plusextensionsandruntime, andthe
DEDICATED_JVM_SBT_TESTSgate firing on 4.0 and not 4.1.What is not verified end to end is a full green suite run. I could not complete one: a Spark
sandbox needs more free disk than the machine I wrote this on has, and the environment could not
reach Maven Central to finish the Comet install. The clone, patch and sbt compile path is
therefore verified by construction and by inspection of the emitted commands, not by execution.
Worth a second pair of eyes on an actual run before this is relied on.
Local machine (Mac M3 Max) results
