potiuk commented on code in PR #71548:
URL: https://github.com/apache/airflow/pull/71548#discussion_r3883764709
##########
scripts/ci/analyze_ci_job_durations.py:
##########
@@ -78,11 +79,23 @@
ISO_SUFFIX_Z = "Z"
PREPARE_BREEZE_STEP_PREFIX = "Prepare breeze & CI image"
+# Steps that build, pull or push a Docker image rather than doing test/work. A
change to
+# an early image layer (an apt package, a Dockerfile line) invalidates every
layer after
+# it, so on the first run after such a merge these steps take multiples of
their usual
+# time - a one-off that must not read as a work-time regression. Their names
come from
+# .github/workflows/push-image-cache.yml and
.github/actions/prepare_breeze_and_image.
+IMAGE_WORK_STEP_PREFIXES = (
+ PREPARE_BREEZE_STEP_PREFIX,
+ "Prepare breeze & PROD image",
+ "Push CI ",
+ "Push PROD ",
+)
class JobDuration(TypedDict):
duration: float
prepare_breeze_duration: float | None
Review Comment:
Done — the field is gone, `image_work_duration` is the only one now.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
scripts/ci/analyze_ci_job_durations.py:
##########
@@ -259,17 +272,33 @@ def get_prepare_breeze_step_duration(job: dict) -> float
| None:
return None
+def get_image_work_seconds(job: dict) -> float | None:
Review Comment:
Merged — `get_prepare_breeze_step_duration` is deleted,
`get_image_work_seconds` is the only extractor.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
scripts/ci/analyze_ci_job_durations.py:
##########
@@ -321,6 +350,7 @@ def get_run_jobs(repo: str, run_id: int) -> dict[str,
JobDuration]:
durations[name] = {
"duration": seconds,
"prepare_breeze_duration":
get_prepare_breeze_step_duration(job),
+ "image_work_duration": get_image_work_seconds(job),
Review Comment:
Good catch, and it was more than duplication: the persistence check only
ever saw the prepare-breeze step, so a slow build in a cache-push job — the
case this PR is about — never reached it. `calculate_image_build_seconds` now
medians `image_work_duration`. The Slack and step-summary lines are relabelled
"Image build, pull & push", since they were still naming a single step for a
figure that now covers all of them.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
scripts/tests/ci/test_analyze_ci_job_durations.py:
##########
@@ -266,7 +266,9 @@ def test_parses_successful_jobs(self, durations_module):
completed = subprocess.CompletedProcess(args=[], returncode=0,
stdout=payload, stderr="")
with patch.object(subprocess, "run", return_value=completed):
jobs = durations_module.get_run_jobs("apache/airflow", 2)
- assert jobs == {"Tests": {"duration": 20 * 60,
"prepare_breeze_duration": 5 * 60}}
+ assert jobs == {
+ "Tests": {"duration": 20 * 60, "prepare_breeze_duration": 5 * 60,
"image_work_duration": 5 * 60}
Review Comment:
No, it wasn't realistic — a cache-push job has no prepare-breeze step but
plenty of image work. With one field it can't arise; the fixtures now carry
`image_work_duration` alone.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
