ci: share one Linux native build across CI workflows - #5841
Conversation
b08c766 to
b274a2e
Compare
andygrove
left a comment
There was a problem hiding this comment.
LGTM but I would like to merge #5842 first so we can unblock setting up the merge queue to reduce CI resource usage. I reviewed this PR assuming that it will go in after #5842.
Once #5842 is in, two things will need to happen on the rebase here:
-
ci.ymlmerges cleanly between the two PRs, but #5842 adds arequired_checksaggregator whoseneeds:must list everyci.ymljob, and a preflight guard incheck-ci-config.pythat enforces it. The merged file fails that guard untilpr_build_linux_checksandbuild_linux_nativeare added torequired_checks.needs. The guard message names them, so the rebase will tell you.build_linux_nativein particular has to be there and cannot be exempted: if the producer fails, GitHub marks all nine consumersskipped, and the aggregator treatsskippedas pass, so without the producer inneedsa broken native build would produce a greenRequired Checks. -
#5842 hardens
Lint Scala (syntactic)against Maven Central connection resets by splitting it into a retriedcs launch scalafix:0.14.6 -- --versionwarm-up followed by the real check undercs launch --mode offline. Since this PR moves that job intopr_build_linux_checks.yml, that split needs to come along. Theiceberg_spark_test_reusable.ymlchange in #5842 (routing the shard-inventory upload throughupload-artifact-retry) merges cleanly onto yourprepare-shardsversion.
After both land, check-ci-config.py will carry two hand-rolled YAML readers (block_mapping here and the line regexes in #5842), both justified by PyYAML not being on the preflight runner. I will open a follow-up issue to install PyYAML in preflight and collapse them rather than leave both in place.
I verified the two-way compatibility locally: your checker passes against a ci.yml that includes the required_checks job, and the #5842 checker passes against your ci.yml once the two ids are added.
|
One more heads-up from the merge-queue side, following on from my review above. #5843 (the PR that actually enables the queue, stacked on #5842) changes That will break Also FYI, |
232560c to
2b6d68d
Compare
andygrove
left a comment
There was a problem hiding this comment.
Three things, all about the rebase rather than the design. The producer and consumer wiring itself looks right to me, and I checked that build_linux_native is the exact union of the nine consumer outputs on this head.
#5852 landed on main after this branch was last pushed, adding a Bootstrap Maven step calling ./.github/actions/maven-bootstrap to five jobs in pr_build_linux.yml. Three of those five are the jobs this PR moves into pr_build_linux_checks.yml, namely lint-java, build-spark-4-1 and celeborn-reflection-compatibility. A rebase cannot carry that step into a file this branch creates from scratch, and pr_build_linux.yml conflicts, so the natural resolution is to take the deletion and lose the retry for those three. The two TPC jobs stay behind and keep theirs. Nothing in check-ci-config.py verifies that a job calling a bare ./mvnw has the bootstrap step ahead of it, so this would be silent until the next Maven Central blip takes out a queue-gating job. Could you add it back to those three after the rebase, and update the ROUTING_CASES comment that says the composite is called only from pr_build_linux.yml? Given how easily it drops out in a file move, is it worth a guard for it?
NATIVE_CONSUMERS is a flat list of output keys, one per consumer job, and #5871 has already broken that assumption on main. It added spark_4_1_hive as a second output feeding the same spark_4_1 job, which is now gated on needs.changes.outputs.spark_4_1 == 'true' || needs.changes.outputs.spark_4_1_hive == 'true'. On a labeled run with run-spark-4.1-hive-tests, spark_4_1 is false and spark_4_1_hive is true, so the union computes build_linux_native as false, the producer is skipped, and spark_4_1 is then skipped through its needs even though its if is true. The label would quietly do nothing and Required Checks would still go green. compute-changes.py is the one file that auto-merges cleanly here, so the union will miss the new key silently. native_selection_failures will fail preflight on the if: string mismatch, but the tempting fix there is to relax the comparison, which leaves the hole. Would it make sense to let a consumer map to a set of output keys rather than one?
Last one is coverage. The hosted run only selected pr_build_linux, spark_4_1 and iceberg_1_11, so six of the nine rewired consumers have not run against the shared artifact. spark_3_4 is the one I would most want to see, because its build job in spark_sql_test_reusable.yml is the only consumer whose toolchain setup actually changed, swapping ./.github/actions/setup-builder for a bare actions/setup-java@v4 at JDK 11, and it now consumes a libcomet.so linked against JDK 17. The linux-test Spark 3.4 lane already proves the library loads under JDK 11, but the new setup path has no coverage. These are queue-tier jobs, so a break blocks every merge rather than one PR. Could you apply run-spark-3.4-tests, run-spark-3.5-tests, run-spark-4.0-tests and run-iceberg-tests once on the rebased head?
2b6d68d to
79c161b
Compare
|
@andygrove, addressed the three items from your latest review and rebased onto
Local validation passed: 36 configuration regression tests, 21 native-selection tests (299,008 combinations), actionlint, suite/benchmark inventory checks, 15 Iceberg shard tests, 4 PR-label tests, Markdown formatting, and whitespace checks. The PR description now reflects the implementation and pending hosted coverage. |
andygrove
left a comment
There was a problem hiding this comment.
Happy to approve once the following issues are resolved.
Thanks for the quick turnaround on the last round. All three items are addressed: the Maven bootstrap is back in the three moved jobs and guarded, NATIVE_CONSUMERS maps callers to tuples so the Hive-only label starts the producer, and all four label runs came back green, so every one of the nine consumers has now run against the shared artifact, including the Spark 3.4 setup-java-only build path. I also ran the three Python checks locally on 79c161b and they pass in about five seconds total, so the preflight cost is fine.
-
Rebase hazard from #5897.
chore: drop support for JDK 11landed onmainafter this head was pushed. It edits thelint-javamatrix inpr_build_linux.yml(the Spark 3.4 entry becomes JDK 17 andJAVA_TOOL_OPTIONSdrops the version conditional), and flipsjava: 11to17forspark_3_4andiceberg_1_8inci.ymland for preflight. Theci.ymlandlinux-testhunks should auto-merge, but thelint-javahunk conflicts with this PR's deletion of that block, andpr_build_linux_checks.ymlis a new file, so the natural resolution drops the change and the new file keeps JDK 11. Same shape as the bootstrap step last time. It fails loudly this time because the new Maven enforcer rejects JDK 11, so no guard is needed, but could you carry those edits intopr_build_linux_checks.ymlon the rebase? -
Contributor doc pointer.
docs/source/contributor-guide/adding_a_new_spark_version.md(around line 112) tells authors to add the compile-only job topr_build_linux.yml, butbuild-spark-4-1now lives inpr_build_linux_checks.yml. Could you update the file name there? The suite-matrix instructions indevelopment.mdandcheck-suites.pystill point atpr_build_linux.ymlcorrectly sincelinux-teststays put.
Nothing else from me. The wiring, the main-only cache write on the single producer, artifact retention, the permissions blocks, and the README rewrite all look right.
Which issue does this PR close?
Part of #5830. Follow-up to #3249, which shared native artifacts within individual workflows.
Rationale for this change
The umbrella CI workflow compiles the same default Linux native library separately for Linux, each selected Spark version, and each selected Iceberg version. A source-change PR normally builds three copies; a full merge-group or manual run builds nine. Build one copy and share it with those consumers to reduce duplicate compilation and runner usage. Actual time savings depend on cache state and still need measurement in CI.
What changes are included in this PR?
ciprofile, JDK 17, compiler flags, and main-only cache save policy. Linux JVM/TPC, Spark SQL, and Iceberg consumers depend on it and receive its artifact through a required input.build_linux_nativeincompute-changes.pyafter path/event/label selection.NATIVE_CONSUMERSmaps nine caller jobs to ten selecting outputs: Spark 4.1's core and Hive outputs feed the same caller. A Hive-only label event starts the native producer even when the core output is false. Preserve the core/Hive module selection introduced by ci: move the Spark 4.1 sql_hive shards behind the merge queue #5871 and the existing queue policy.actions/setup-java@v4without Rust setup; its container remains to preserve workspace and Maven-cache paths for downstream Spark shards../mvnwcommands in both Linux workflows.contents: read. Artifact retention remains one day.How are these changes tested?
Rebased onto
mainat451c99963206fa6bf0387239aa12887a16255516. Independent comparison against that base verified all six moved Linux jobs, both remaining TPC bootstrap steps, Spark module selection, queue policy, artifact retry behavior, and existing required-check dependencies.Passed locally on this revision:
python3 dev/ci/check-ci-config.py, including routing and Spark module-matrix checks.python3 dev/ci/test-ci-config.py(36 tests): mutations cover missing, late, conditional, and failure-ignoring Maven bootstrap steps, plus omitted/altered Hive gates and exports, missing consumers/dependencies, and shared-artifact wiring.python3 dev/ci/test-native-build-selection.py(21 tests, including 299,008 path/event combinations): real compute and CLI coverage for Hive-only labels, core-only PRs, core+Hive selection, merge groups, main pushes, other opt-ins, and empty-input manual dispatch.git diff --check.Hosted CI is running on head
79c161b293f2147b1d25233a47a15222d7289cf9. The regular PR run is in progress. Appliedrun-spark-3.4-tests,run-spark-3.5-tests,run-spark-4.0-tests, andrun-iceberg-testsafter pushing the rebased head, starting four additional opt-in runs (1, 2, 3, 4). Their results are pending; the local checks do not establish runtime coverage for those six additional consumers.No full native or Spark build was run locally. Apache RAT is left to hosted preflight because the local Maven cache is read-only and Maven Central was unavailable in the earlier local attempt. Failed-job reruns can reuse the successful producer's artifact while retained; after expiry, rerun the full workflow. That rerun scenario remains unverified.