andygrove commented on code in PR #7311:
URL: https://github.com/apache/datafusion-comet/pull/7311#discussion_r4424008561
##########
.github/workflows/ci.yml:
##########
@@ -73,9 +80,13 @@ name: Comet CI
# each other. Every other run is keyed on its event name: opened, synchronize
# and reopened all map to `pull_request`, so a new push still supersedes its
# predecessor, while a scheduled run at the tip of main never cancels that
-# commit's push run.
+# commit's push run. A pull request is keyed on its number, and not on its
+# branch name: nearly every pull request comes from a fork, and two forks'
+# branches can share a name, which would have one cancel the other's run. A
push
+# to a fork (ci_fork.yml) is keyed on its branch instead of its commit, so the
+# next push to the branch cancels the run in flight.
concurrency:
- group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{
github.workflow }}-${{ github.event.action == 'labeled' &&
github.event.label.name || github.event_name }}
+ group: ${{ github.repository }}-${{ github.event.pull_request.number ||
(github.event_name == 'push' && github.repository != 'apache/datafusion-comet'
&& github.ref) || github.sha }}-${{ github.workflow }}-${{ github.event.action
== 'labeled' && github.event.label.name || github.event_name }}
Review Comment:
Good catch, a backup branch at the PR's commit is an ordinary thing to do
and it would have cancelled the build. Fixed in 6dc32f21d8: a fork push is
keyed on `github.run_id`, so no other push shares its group, and `supersede`
still cancels the older runs of exactly the same branch. The test now asserts
different groups for the same sha with different run ids, including on a
different branch, and fails if the key goes back to the commit. I also checked
it on a fork: the same commit pushed to two branches started two runs and
neither was cancelled.
##########
.github/workflows/README.md:
##########
@@ -198,6 +200,91 @@ still needs its label, so a `labeled` run there adds
`spark_3_4` or nothing.
`check-ci-config.py` pins that wiring, because a dropped variable reads as an
empty string and would route the pull request as if it targeted `main`.
+## Fork runs
+
+Nearly every pull request comes from a fork, and the PR tier is the
+largest share of the runner-hours CI spends (about 60% in the week to
+2026-10-28, label runs included). A fork pull request therefore does not run
+the PR tier on ASF's runners. The contributor's fork runs it, and the merge
+queue, which stays on ASF, runs the queue tier on the merge result before
+anything lands. That is the PR tier plus Spark SQL on 4.1, Iceberg 1.11, macOS
and
+the other queue-tier jobs, with path filters still applied; the nightly-only
and
+label-only suites are not part of it. The design follows `apache/datafusion`
(#25591) and `apache/arrow-rs`
+(#11788). Three files carry it:
+
+| File | Role
|
+| -------------------- |
----------------------------------------------------------------------------------------------------------------------------------------------------------
|
+| `ci_fork.yml` | Fires on a push to any branch of a fork except
`asf-site` and calls `ci.yml`, after cancelling the branch's older runs. Its
jobs are skipped upstream. |
+| `compute-changes.py` | `is_fork_push` gives a fork the `pr` tier.
`is_fork_pull_request` takes the `pr` tier away upstream, and leaves every
`run-*` label and the `labeled` run. |
+| `fork_ci_status.yml` | Mirrors the fork's run onto the pull request as the
`CI (fork)` check, on `workflow_run` and every 15 minutes.
|
+
+Things that follow from that, and are easy to break:
+
+- **The fork result is advisory.** `Required Checks` does not depend on
+ `CI (fork)`, because a contributor controls the workflow that produced it. A
+ green fork run that never ran the tests costs one queue slot, not a bad
merge.
+- **Every branch of a fork runs through `ci_fork.yml`, `main` included.**
+ `ci.yml` also triggers on push to `main`, in a fork as upstream, so its
+ `Preflight` skips any fork push that did not come through `ci_fork.yml`,
which
+ it tells apart by the caller's `github.workflow`, and `required_checks` skips
+ itself when `Preflight` does. That starts no runner for a fork's schedule or
+ for a second copy of a push, and leaves the bridge one workflow to look up. A
+ push that adds nothing to upstream `main`, such as a sync of the fork,
selects
+ no suite, so it starts only `Preflight` and `Detect changes` and warms no
Review Comment:
Right, it undercounted. The README now says a sync that adds nothing to
upstream `main` selects no build or test suite and writes to no build cache,
without listing the jobs it does start. The PR description says the same.
--
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]
