-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Feature(call-back): Enhance DAG-level callback context with enriched metadata and test coverage #51949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature(call-back): Enhance DAG-level callback context with enriched metadata and test coverage #51949
Changes from all commits
768e77e
6f8c529
6bbfc34
6830c6f
0e3eca3
dbadcf4
9493126
887eec2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1352,21 +1352,48 @@ def notify_dagrun_state_changed(self, msg: str = ""): | |
|
|
||
| def handle_dag_callback(self, dag: SDKDAG, success: bool = True, reason: str = "success"): | ||
| """Only needed for `dag.test` where `execute_callbacks=True` is passed to `update_state`.""" | ||
| task_instances = self.get_task_instances() | ||
|
|
||
| # Identify the most relevant task instance | ||
| last_relevant_ti = None | ||
| if not success: | ||
| failed_tis = [ti for ti in task_instances if ti.state in State.failed_states and ti.end_date] | ||
| failed_tis.sort(key=lambda x: x.end_date, reverse=True) | ||
| last_relevant_ti = failed_tis[0] if failed_tis else None | ||
| else: | ||
| success_tis = [ti for ti in task_instances if ti.state in State.success_states and ti.end_date] | ||
| success_tis.sort(key=lambda x: x.end_date, reverse=True) | ||
| last_relevant_ti = success_tis[0] if success_tis else None | ||
|
Comment on lines
+1365
to
+1366
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be something like last_relevant_ti = max(success_tis, ...) if success_tis else Noneinstead. I kind of wonder if we can even avoid building the list at all. Also is simply sorting by end_date correct? Especially with trigger_rule, the last success/failed ti might not necessarily be the ti that causes the dag run to be marked as success/failed. Can you check the logic in 2.x to see how the ti is selected? |
||
|
|
||
| # Enrich DAG-level callback context | ||
| context: Context = { # type: ignore[assignment] | ||
| "dag": dag, | ||
| "run_id": str(self.run_id), | ||
| "start_date": self.start_date, | ||
| "end_date": self.end_date, | ||
| "data_interval_start": self.data_interval_start, | ||
| "data_interval_end": self.data_interval_end, | ||
| "reason": reason, | ||
| } | ||
|
|
||
| # Add task-level metadata if available | ||
| if last_relevant_ti: | ||
| context.update( | ||
| { | ||
| "task_instance": last_relevant_ti, | ||
| "ti": last_relevant_ti, | ||
| } | ||
| ) | ||
|
|
||
| callbacks = dag.on_success_callback if success else dag.on_failure_callback | ||
| if not callbacks: | ||
| self.log.warning("Callback requested, but dag didn't have any for DAG: %s.", dag.dag_id) | ||
| self.log.warning("Callback requested, but DAG didn't have any for DAG: %s.", dag.dag_id) | ||
| return | ||
| callbacks = callbacks if isinstance(callbacks, list) else [callbacks] | ||
|
|
||
| for callback in callbacks: | ||
| self.log.info( | ||
| "Executing on_%s dag callback: %s", | ||
| "Executing on_%s DAG callback: %s", | ||
| "success" if success else "failure", | ||
| callback.__name__ if hasattr(callback, "__name__") else repr(callback), | ||
| ) | ||
|
|
@@ -2042,3 +2069,5 @@ def __repr__(self): | |
| if self.map_index != -1: | ||
| prefix += f" map_index={self.map_index}" | ||
| return prefix + ">" | ||
|
|
||
| return prefix + ">" | ||
|
Comment on lines
+2072
to
+2073
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accidental? |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,6 +86,11 @@ | |
| "ts_nodash_with_tz", | ||
| "try_number", | ||
| "var", | ||
| "dag_run_url", | ||
| "end_date", | ||
| "log_url", | ||
| "mark_success_url", | ||
| "max_tries", | ||
|
Comment on lines
+89
to
+93
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same, should be removed |
||
| } | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be removed now they are not being added