shahar1 commented on code in PR #76836:
URL: https://github.com/apache/airflow/pull/76836#discussion_r4553922916
##########
dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py:
##########
@@ -1445,6 +1465,48 @@ def build_job_statuses(jobs: Iterable[dict[str, Any]])
-> dict[str, tuple[str, s
return statuses
+def find_failed_jobs(jobs: Iterable[dict[str, Any]]) -> list[FailedJob]:
+ """The finished jobs of a fork run that did not pass, by name, leaving out
the jobs every run has."""
+ failed: dict[str, FailedJob] = {}
+ for job in jobs:
+ if job["name"] in UNREPORTED_JOB_NAMES or job.get("status") in
RUNNING_JOB_STATUSES:
+ continue
+ conclusion = job.get("conclusion") or ""
+ if JOB_STATUSES.get(conclusion, ("",))[0] == "failure":
+ failed.setdefault(job["name"], FailedJob(job["name"],
job["html_url"], conclusion))
+ return sorted(failed.values())
Review Comment:
**nit**: with alphabetical order and the cap of 10, the job that actually
failed can end up under `and N more, see the run`, behind cancelled or
timed-out jobs. Job statuses already post failures first. Listing those first
here too would match:
```suggestion
return sorted(failed.values(), key=lambda job: (job.conclusion !=
"failure", job.name))
```
##########
dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py:
##########
@@ -1445,6 +1465,48 @@ def build_job_statuses(jobs: Iterable[dict[str, Any]])
-> dict[str, tuple[str, s
return statuses
+def find_failed_jobs(jobs: Iterable[dict[str, Any]]) -> list[FailedJob]:
+ """The finished jobs of a fork run that did not pass, by name, leaving out
the jobs every run has."""
+ failed: dict[str, FailedJob] = {}
+ for job in jobs:
+ if job["name"] in UNREPORTED_JOB_NAMES or job.get("status") in
RUNNING_JOB_STATUSES:
+ continue
+ conclusion = job.get("conclusion") or ""
+ if JOB_STATUSES.get(conclusion, ("",))[0] == "failure":
+ failed.setdefault(job["name"], FailedJob(job["name"],
job["html_url"], conclusion))
+ return sorted(failed.values())
+
+
+# The jobs of fork runs read in this reconciler run, by run id. None records a
read that failed after
+# its retries, so the next reader gives up at once instead of repeating them;
the next run reads again.
+ReadJobs = dict[int, list[dict[str, Any]] | None]
+
+
+def read_failed_jobs(
+ client: GitHubClient,
+ pr: PullRequestSnapshot,
+ fork: ForkObservation,
+ read_jobs: ReadJobs,
+) -> list[FailedJob] | None:
+ """
+ ``find_failed_jobs`` of the pull request's fork run; None when its jobs
cannot be read.
+
+ :param read_jobs: the jobs already read in this reconciler run; a read
made here is recorded too.
+ """
+ if fork.run_id is None or not pr.head_repo:
+ return []
+ if fork.run_id in read_jobs:
Review Comment:
**nit / follow-up**: a PR whose fork run failed reads its jobs on every
monitor run (12 an hour). That happens even when the comment already lists them
and the run is unchanged: the job-status path skips via `jobs_reported`, but
this one doesn't. This is the ~500 to ~740 row in the description, and the
per-PR read growth that the description already calls tight at ~500 gated PRs.
A finished run's jobs don't change (a re-run sends the PR back to `running`),
so this could be skipped when the marker already records `failed` for this head
and run. Fine to leave for the batching follow-up.
##########
dev/AGENTS.md:
##########
@@ -54,9 +54,9 @@ workflow of `apache/airflow` shares (see "Write budget" in
If the budget is exceeded, turn the job statuses off: set `job_statuses =
false` in
`.github/fork-ci-gate.toml`. Signs that it is exceeded are the monitor's Slack
note that job
-statuses "have not fit in the budget" (posted only after three runs in a row
deferred them)
+statuses "have not fit in the budget" (posted only after about 45 minutes of
runs in a row deferred them)
Review Comment:
**nit**: this reads garbled after the edit.
```suggestion
statuses "have not fit in the budget" (posted only after job statuses were
deferred for about 45 minutes in a row)
```
##########
dev/AGENTS.md:
##########
@@ -54,9 +54,9 @@ workflow of `apache/airflow` shares (see "Write budget" in
If the budget is exceeded, turn the job statuses off: set `job_statuses =
false` in
`.github/fork-ci-gate.toml`. Signs that it is exceeded are the monitor's Slack
note that job
-statuses "have not fit in the budget" (posted only after three runs in a row
deferred them)
+statuses "have not fit in the budget" (posted only after about 45 minutes of
runs in a row deferred them)
staying for hours, pull requests listed because their fork run finished more
than an hour ago with
-job statuses still not posted, its Slack note that GitHub rate-limited it in
two runs in a row, the
+job statuses still not posted, its Slack note that GitHub rate-limited it in
runs in a row for about 30 minutes, the
Review Comment:
**nit**: same here.
```suggestion
job statuses still not posted, its Slack note that GitHub rate-limited it
for about 30 minutes in a row, the
```
##########
dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py:
##########
@@ -828,6 +844,9 @@ def release(state: ForkState, comment: Callable[[Marker],
str]) -> Plan:
_set_ci_passed(plan, pr, passed=state == ForkState.GREEN,
target_url=target_url)
_set_ci_run_started(plan, pr, fork)
green = state in (ForkState.GREEN, ForkState.GREEN_UNVERIFIED)
+ failed_jobs = find_failed_jobs() if state == ForkState.FAILED and not
startup_failure else []
+ if failed_jobs is None:
+ return plan
Review Comment:
**minor**: if the jobs read keeps failing, this holds the state comment
indefinitely. With job statuses on, the reporter's own failure counter alerts
after ~45 minutes. But with `job_statuses = false` (the escape hatch
`dev/AGENTS.md` points to) or when the reporter deferred because the budget was
spent, nothing alerts. The comment then keeps saying "running" (or describing
an earlier head) while `Fork CI / Tests (AMD)` is red.
Holding only protects something when the comment already shows `failed` with
a list. Otherwise, posting the failed state without the list seems better than
leaving the old state up:
```python
if failed_jobs is None:
if marker and marker.state == ForkState.FAILED.value:
return plan
failed_jobs = []
```
--
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]
