Copilot commented on code in PR #63904:
URL: https://github.com/apache/airflow/pull/63904#discussion_r3025335063
##########
task-sdk/src/airflow/sdk/definitions/deadline.py:
##########
@@ -110,27 +110,32 @@ class AverageRuntimeDeadline(BaseDeadlineReference):
DEFAULT_LIMIT = 10
max_runs: int
min_runs: int | None = None
+ multiplier: float = 1.0
def __post_init__(self):
if self.min_runs is None:
self.min_runs = self.max_runs
if self.min_runs < 1:
raise ValueError("min_runs must be at least 1")
+ if self.multiplier <= 0:
+ raise ValueError("multiplier must be positive")
Review Comment:
The `multiplier` validation (`self.multiplier <= 0`) does not reject
non-finite floats (e.g. `nan`, `inf`). `nan` will pass this check and later
cause failures when constructing a `timedelta`, and `inf` can overflow.
Consider validating that `multiplier` is finite and > 0 (e.g.
`math.isfinite(self.multiplier)` in addition to the positivity check), and
extend the validation tests accordingly.
##########
airflow-core/src/airflow/serialization/definitions/deadline.py:
##########
@@ -191,13 +191,16 @@ class
AverageRuntimeDeadline(SerializedBaseDeadlineReference):
DEFAULT_LIMIT = 10
max_runs: int
min_runs: int | None = None
+ multiplier: float = 1.0
required_kwargs = {"dag_id"}
def __post_init__(self):
if self.min_runs is None:
self.min_runs = self.max_runs
if self.min_runs < 1:
raise ValueError("min_runs must be at least 1")
+ if self.multiplier <= 0:
+ raise ValueError("multiplier must be positive")
Review Comment:
The current `multiplier` validation only checks `<= 0`, so `nan`/`inf` are
accepted and can later raise/overflow in `timedelta(seconds=scaled_seconds)`.
Consider validating that the multiplier is finite and > 0 (e.g.
`math.isfinite(self.multiplier)`) to avoid runtime errors from invalid user
inputs.
##########
airflow-core/docs/howto/deadline-alerts.rst:
##########
@@ -118,6 +118,7 @@ Airflow provides several built-in reference points that you
can use with Deadlin
Parameters:
* ``max_runs`` (int, optional): Maximum number of recent Dag runs to
analyze. Defaults to 10.
* ``min_runs`` (int, optional): Minimum number of completed runs
required to calculate average. Defaults to same value as ``max_runs``.
+ * ``multiplier`` (float, optional): Factor to scale the average
runtime by. Defaults to 1.0. For example, a multiplier of 1.5 sets the deadline
at 1.5x the average runtime. Must be positive.
Review Comment:
The description above this parameter list says the deadline is `current time
+ average runtime + interval`. With the new `multiplier` parameter, the
reference becomes `current time + (average runtime × multiplier)`, so it would
be clearer to update that sentence to explicitly mention the scaling behavior.
##########
airflow-core/src/airflow/models/deadline.py:
##########
@@ -402,13 +402,16 @@ class AverageRuntimeDeadline(BaseDeadlineReference):
DEFAULT_LIMIT = 10
max_runs: int
min_runs: int | None = None
+ multiplier: float = 1.0
required_kwargs = {"dag_id"}
def __post_init__(self):
if self.min_runs is None:
self.min_runs = self.max_runs
if self.min_runs < 1:
raise ValueError("min_runs must be at least 1")
+ if self.multiplier <= 0:
+ raise ValueError("multiplier must be positive")
Review Comment:
`multiplier` is only checked with `<= 0`, which allows `float('nan')` and
`float('inf')`. These values can later raise/overflow when used in
`timedelta(seconds=scaled_seconds)`. Consider requiring the multiplier to be
finite and > 0 (e.g. `math.isfinite(self.multiplier)`), and add/adjust tests to
cover `nan`/`inf`.
--
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]
