ferruzzi commented on code in PR #70714:
URL: https://github.com/apache/airflow/pull/70714#discussion_r3753280509
##########
task-sdk/src/airflow/sdk/definitions/deadline.py:
##########
@@ -362,9 +360,11 @@ def serialize_reference(self) -> dict:
# Optionally, specify when it is calculated by providing a
DeadlineReference.TYPES value.
@deadline_reference(DeadlineReference.TYPES.DAGRUN_QUEUED)
class MyQueuedRef(BaseDeadlineReference):
- def _evaluate_with(self, *, session: Session, **kwargs) ->
datetime:
- # Put your business logic here
- return some_datetime
+ # Optionally, you can specify when you want it calculated by
providing a DeadlineReference.TYPES
Review Comment:
This comment duplicates the one just above the decorator. The one outside
feels like a better place.
##########
airflow-core/docs/howto/deadline-alerts.rst:
##########
@@ -530,8 +525,5 @@ followed by a more urgent escalation if the Dag is still
running.
and give every field a default value.
* **Plugin Placement**: One convenient place for custom references is in the
plugins directory.
* **API Server Restart**: Restart the Airflow API Server after adding or
modifying custom references.
-* **Required Parameters**: ``required_kwargs`` declares which Dag run context
values Airflow should
Review Comment:
Since Required Parameters is still available until 4.0, we should keep it
here and just add a note that it is deprecated
##########
airflow-core/src/airflow/serialization/definitions/deadline.py:
##########
@@ -153,7 +138,7 @@ class
FixedDatetimeDeadline(SerializedBaseDeadlineReference):
_datetime: datetime
- def _evaluate_with(self, *, session: Session, **kwargs: Any) ->
datetime | None:
+ def _evaluate_with(self, *, session: Session, dagrun: Any) -> datetime
| None:
Review Comment:
Same as before; dagrun is never used, so replacing it with `**kwargs` is a
bit cleaner.
##########
airflow-core/src/airflow/models/deadline.py:
##########
@@ -320,34 +343,18 @@ def get_reference_class(cls, reference_name: str) ->
type[BaseDeadlineReference]
class BaseDeadlineReference(LoggingMixin, ABC):
"""Base class for all Deadline implementations."""
- # Set of required kwargs - subclasses should override this.
- required_kwargs: set[str] = set()
-
@classproperty
def reference_name(cls: Any) -> str:
return cls.__name__
def evaluate_with(self, *, session: Session, interval: timedelta,
**kwargs: Any) -> datetime | None:
- """Validate the provided kwargs and evaluate this deadline with
the given conditions."""
- filtered_kwargs = {k: v for k, v in kwargs.items() if k in
self.required_kwargs}
-
- if missing_kwargs := self.required_kwargs - filtered_kwargs.keys():
- raise ValueError(
- f"{self.__class__.__name__} is missing required
parameters: {', '.join(missing_kwargs)}"
- )
-
- if extra_kwargs := kwargs.keys() - filtered_kwargs.keys():
- self.log.debug(
- "%s ignoring unexpected parameters: %s",
- self.reference_name,
- ", ".join(extra_kwargs),
- )
-
- base_time = self._evaluate_with(session=session, **filtered_kwargs)
+ """Evaluate this deadline with the supplied context."""
+ evaluation_kwargs = _get_evaluation_kwargs(self,
self._evaluate_with, kwargs)
+ base_time = self._evaluate_with(session=session,
**evaluation_kwargs)
return base_time + interval if base_time is not None else None
@abstractmethod
- def _evaluate_with(self, *, session: Session, **kwargs: Any) ->
datetime | None:
+ def _evaluate_with(self, *, session: Session, dagrun: Any) -> datetime
| None:
Review Comment:
It doesn't look like this was actually addressed, was it? You would at
east need to add `queued_at` to `DagRunProtocol` for that to work and it's not
showing in the diff.
--
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]
