Miretpl commented on code in PR #71548:
URL: https://github.com/apache/airflow/pull/71548#discussion_r3824727609
##########
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:
As breeze duration is part of image work, I think we could delete that.
##########
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:
A similar function already exists for `Prepare breeze & CI image`. We should
rather not hold duplicates when there is almost one difference in the function
itself.
##########
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:
Is it realistic to have always the same value for `prepare_breeze_duration`
and `image_work_duration`?
##########
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:
Image work consists of the `prepare_breeze_duration` inside. Looking at the
changes only, I don't see the change which would change the check logic, which
would include time for `image_work_duration` instead of/next to
`prepare_breeze_duration`.
--
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]
