Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
boring-cyborg[bot] commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2051325773 Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
mobuchowski merged PR #38155: URL: https://github.com/apache/airflow/pull/38155 -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
mobuchowski commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2051323351 @raphaelauv sorry for leaving you like that. The solution is good, we can add some internal runtime checking of Airflow version to the OL provider later if we want to use that field. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
vandonr commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2051250994 @mobuchowski do you want to review the change now ? (also, I need someone to click the button to run the CI plz) -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
vandonr commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2047252150 I didn't change the signature of the existing openlinage listener https://github.com/apache/airflow/blob/a2f5307fd0ec54b34b8c753a53024a2629a56fd8/airflow/providers/openlineage/plugins/listener.py#L196 because pluggy supports having too many parameters, but it crashes when calling a method with too few params, so we have to keep the old signature as not to break compatibility of newer provider package versions with older airflow versions. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
vandonr commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2045733496 ha, that's very nice and solves most issues I didn't know it was handled so well by pluggy ! I'll make that change tomorrow :) -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
mobuchowski commented on PR #38155:
URL: https://github.com/apache/airflow/pull/38155#issuecomment-2044703627
@potiuk to make sure we fully understand: it means that when the plugin hook
gets called and your listener do not implement the argument, it does get
called, but just does not receive the particular argument
```python
import sys
from pluggy import PluginManager, HookspecMarker, HookimplMarker
hookspec = HookspecMarker("myproject")
hookimpl = HookimplMarker("myproject")
@hookspec
def myhook(config, arg):
pass
class Plugin1:
@hookimpl
def myhook(self, arg):
print("hook without config")
class Plugin2:
@hookimpl
def myhook(self, config, arg):
print("hook with config")
pm = PluginManager("myproject")
# load from the local module's namespace
pm.add_hookspecs(sys.modules[__name__])
pm.register(Plugin1())
pm.register(Plugin2())
pm.hook.myhook(config="config", arg="arg")
```
prints both
```
hook with config
hook without config
```
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
potiuk commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2044690786 > @vandonr @potiuk listeners (outside of dataset listeners) are non-experimental as of 2.9: #36376 - however pluggy allows to add arguments without breaking impls: https://pluggy.readthedocs.io/en/latest/#opt-in-arguments - does this fit into _non-experimental_ label? I thin that's the best solution if it can be done. Do I understand, that when you change the spec, and add a new field, the message can be send with a new argument and if your implementation does not accept it, it will not receive it? If so then I think that solves the problem in the best way. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
mobuchowski commented on code in PR #38155:
URL: https://github.com/apache/airflow/pull/38155#discussion_r1557415431
##
airflow/models/taskinstance.py:
##
@@ -1409,7 +1410,9 @@ class TaskInstance(Base, LoggingMixin):
cascade="all, delete, delete-orphan",
)
note = association_proxy("task_instance_note", "content",
creator=_creator_note)
+
task: Operator | None = None
+_thread_local_data = threading.local()
Review Comment:
I don't have any problem with having more isolated api like
`get_last_error_message()` than storing it in TI model. As context where we
could access the error message is pretty isolated - only worker process after
task failure, the method would just return `None` otherwise.
I would decide whether, in general, we want to possibly add arguments to
non-experimental listener APIs, as adding it in this way would be definitely
more ergonomic for everyone.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
mobuchowski commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2044658732 @vandonr listeners (outside of dataset listeners) are non-experimental as of 2.9: https://github.com/apache/airflow/pull/36376 -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
potiuk commented on code in PR #38155:
URL: https://github.com/apache/airflow/pull/38155#discussion_r1557289295
##
airflow/models/taskinstance.py:
##
@@ -1409,7 +1410,9 @@ class TaskInstance(Base, LoggingMixin):
cascade="all, delete, delete-orphan",
)
note = association_proxy("task_instance_note", "content",
creator=_creator_note)
+
task: Operator | None = None
+_thread_local_data = threading.local()
Review Comment:
Yes - thread local variables shoud should be stored in global variable in
"task_instance" in this case (private).
Yes - I know it's not perfect and also a bit hacky - I am not sure if there
is a better way though.
I'd love other opinions on that one. Maybe my reservations on adding to
TaskInstance were just too cautious. @mobuchowski and @ashb - maybe as the
authors of listenrs you have some opinion:
Some more context: as originally explained by @vandonr :
> I was writing a listener, and it seems extremely hard to get some
information on the error in the on_task_instance_failed callback, because the
error is not passed as a parameter to the callback itself
> It's [written to the
context](https://github.com/apache/airflow/blob/9e97433dc3368138431305c5161a007e4fc5f227/airflow/models/taskinstance.py#L2809-L2810)
a bit further down, but we don't have that yet when the callback is called.
> We cannot add an extra parameter now because it'd be a breaking change,
but what do you think about storing the error in the TaskInstance object before
calling on_task_instance_failed ? It'd be a pretty cheap way to solve that
issue (if it's one!). We don't need to persist that in DB or anything, it just
needs to carry the value to the method call that just follows, seems simple
enough ?
> We cannot add an extra parameter now because it'd be a breaking change,
but what do you think about storing the error in the TaskInstance object before
calling on_task_instance_failed ? It'd be a pretty cheap way to solve that
issue (if it's one!). We don't need to persist that in DB or anything, it just
needs to carry the value to the method call that just follows, seems simple
enough ?
My point is that we should not make it a "hack" but simply extend the API to
include the error message.
Originally Raphael proposed to add error to the TaskInstance object - seems
simple, but feels hacky as we are modifying the object that is a data model and
adding dynamically error message to it. Plus Task Instance gets serialized
back/forth in some places so that seems a bit out-of-place.
I proposed to use ThreadLocal - but that also ends up hacky where the error
message will be stored in private global variable in `task_instance.py` - and
accessible by `get_last_error_mesage()`. This also feels hacky. But I have no
other idea how to pass error message to listener in this case. Maybe you could
help?
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
vandonr commented on code in PR #38155:
URL: https://github.com/apache/airflow/pull/38155#discussion_r1557260788
##
airflow/models/taskinstance.py:
##
@@ -1409,7 +1410,9 @@ class TaskInstance(Base, LoggingMixin):
cascade="all, delete, delete-orphan",
)
note = association_proxy("task_instance_note", "content",
creator=_creator_note)
+
task: Operator | None = None
+_thread_local_data = threading.local()
Review Comment:
I'm not very familiar with `threading.local()`, but I'm not sure it works
like this. It's not a shared static storage for whoever is on the same thread,
it still needs a common variable.
For instance, consider the following sample code:
```
import threading
def set():
setdata = threading.local()
setdata.test = "hello"
def get():
getdata = threading.local()
print(getdata.test)
set()
get()
```
this crashes in `get()` because `getdata` is not the same instance as
`setdata`.
So we need somewhere to store our ThreadLocal variable that can be accessed
from a different taskinstance method. I can actually move it outside of
TaskInstance, to the global scope, or to any other classe for that matter, but
I'm not sure it's a good idea.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
potiuk commented on code in PR #38155:
URL: https://github.com/apache/airflow/pull/38155#discussion_r1556148549
##
airflow/models/taskinstance.py:
##
@@ -1409,7 +1410,9 @@ class TaskInstance(Base, LoggingMixin):
cascade="all, delete, delete-orphan",
)
note = association_proxy("task_instance_note", "content",
creator=_creator_note)
+
task: Operator | None = None
+_thread_local_data = threading.local()
Review Comment:
I just look at this more closely. Hmm. I don't think we shouldadd the thread
local data to task instance - why would we need to do it? The whole point of
thread local data - is that you can access is directly whithout setting a
variable to the model object that is passed down the stack.
What I **really** thought about is literally two methods in
`taskinstance.py`:
```python
def set_last_error(error: None | str | BaseException:):
```
```python
def get_last_error() -> None | str | BaseException:
```
Both using ThreadLocal. No passing extra field in TaskInstance.
##
airflow/models/taskinstance.py:
##
@@ -1409,7 +1410,9 @@ class TaskInstance(Base, LoggingMixin):
cascade="all, delete, delete-orphan",
)
note = association_proxy("task_instance_note", "content",
creator=_creator_note)
+
task: Operator | None = None
+_thread_local_data = threading.local()
Review Comment:
I just look at this more closely. Hmm. I don't think we should add the
thread local data to task instance - why would we need to do it? The whole
point of thread local data - is that you can access is directly whithout
setting a variable to the model object that is passed down the stack.
What I **really** thought about is literally two methods in
`taskinstance.py`:
```python
def set_last_error(error: None | str | BaseException:):
```
```python
def get_last_error() -> None | str | BaseException:
```
Both using ThreadLocal. No passing extra field in TaskInstance.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
potiuk commented on code in PR #38155: URL: https://github.com/apache/airflow/pull/38155#discussion_r1555047282 ## docs/apache-airflow/administration-and-deployment/listeners.rst: ## @@ -34,21 +34,63 @@ Lifecycle events allow you to react to start and stop events for an Airflow ``Jo DagRun State Change Events -- +DagRun state change events occur when a :class:`~airflow.models.dagrun.DagRun` changes state. + - ``on_dag_run_running`` + +.. exampleinclude:: /../../airflow/example_dags/plugins/event_listener.py +:language: python +:dedent: 4 +:start-after: [START howto_listen_dagrun_running_task] +:end-before: [END howto_listen_dagrun_running_task] + - ``on_dag_run_success`` + +.. exampleinclude:: /../../airflow/example_dags/plugins/event_listener.py +:language: python +:dedent: 4 +:start-after: [START howto_listen_dagrun_success_task] +:end-before: [END howto_listen_dagrun_success_task] + - ``on_dag_run_failed`` -DagRun state change events occur when a ``DagRun`` changes state. +.. exampleinclude:: /../../airflow/example_dags/plugins/event_listener.py +:language: python +:dedent: 4 +:start-after: [START howto_listen_dagrun_failure_task] +:end-before: [END howto_listen_dagrun_failure_task] + TaskInstance State Change Events +TaskInstance state change events occur when a :class:`~airflow.models.taskinstance.TaskInstance` changes state. +You can use these events to react to ``LocalTaskJob`` state changes. + - ``on_task_instance_running`` + +.. exampleinclude:: /../../airflow/example_dags/plugins/event_listener.py +:language: python +:dedent: 4 +:start-after: [START howto_listen_ti_running_task] +:end-before: [END howto_listen_ti_running_task] + - ``on_task_instance_success`` + +.. exampleinclude:: /../../airflow/example_dags/plugins/event_listener.py +:language: python +:dedent: 4 +:start-after: [START howto_listen_ti_success_task] +:end-before: [END howto_listen_ti_success_task] + - ``on_task_instance_failed`` -TaskInstance state change events occur when a ``TaskInstance`` changes state. -You can use these events to react to ``LocalTaskJob`` state changes. +.. exampleinclude:: /../../airflow/example_dags/plugins/event_listener.py Review Comment: NIT: would be nice to mention that you can retrieve the error in the text. Yes it will be visible in the example but mentioning it explicitly would be great. I'd also love others to review it as well. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
potiuk commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2034367252 Also one other thing - the smaller and the more isolated the change is, the more important it is to have both unit tests and documentation. When you are working on a bigger change, which is split across multiple PRs - those changes are targetting future airflow versions and there tests and documentation are even supposed to be split after the bigger change takes shape. Simply - those changes will never be cherry-picked. This one, on the other hand - has quite a big chance to be cherry-picked to 2.9* - possibly even to 2.9.0 if we merge it before rc2 (there will be an RC) or even to 2.9.1 or later - while technically it adds, a feature, we can classify it as a bugfix that fixes a missing feature in listener. So that's why it's more important to have this one complete with tests and docs, because it's very likely we will cherry-pick it. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
potiuk commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2034351437 This is also why this one should also have a unit test added before we merge - because then when we cherry-pick that one, we should cherry-pick it with the unit test, which is is the only way we can see if there is also a conflict or another reason why the cherry-picked change does not work in the other branch. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
potiuk commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2034347911 I am not a big fan of splitting PRs like that. If we decide to add something, then big part of the change is to tell the users other than author how to use it. That's a big difference between implementing somethign for "me" vs. for "all users". And there is very practical reason to have it in one PR - if we decide to cherry-pick that one to 2.9.0 or 2.9.1 (for example) , we will have to remember about cherry-picking yet another PR with documentation. This is not something that makes release manager's life easy and, really not a release manager's job to track and remember about those related PRs that should also be cherry-picked. If you imagine that release manager has 60 or 70 PRs to cherry-pick, choosing and cherry-picking them is enough of a task to not add a burden to go through all of them look through all the discussions in PR and see "hey there is also that 2nd PR with documentation that I have to cherry-pick as well". This is largely impossible task to do if you put yourself in the release manager shoes. Comparing to that - adding a paragraph or so documentation to the PR while it is being worked on is far more focused work that just "makes sense" to be done together. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
vandonr commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2034008773 > I guess the documentation about listeners https://airflow.apache.org/docs/apache-airflow/stable/administration-and-deployment/listeners.html - it should have a section on "how to retrieve error" - and yes - ideally it should include the relevant excerpt from an example dag Given the fact that this documentation is very minimalistic for now, what do you think about handling this PR as just adding the capability to retrieve the error (which would already be somewhat visible to users through auto-completion), and I can write an other PR to add the `howto_listen_ti_XXX` to the doc you mentioned, which would self-document how to retrieve the error. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
potiuk commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2032715168 > > using a new public API call (`get_last_task_failure`- and add documentation about it). > > by "new API" you meant just a method on the class, right ? Is there more doc to update besides the sample dag and the docstring for the method itself ? I guess the documentation about listeners https://airflow.apache.org/docs/apache-airflow/stable/administration-and-deployment/listeners.html - it should have a section on "how to retrieve error" - and yes - ideally it should include the relevant excerpt from an example dag (which will serve both as a way to test it end-to-end and serve as a documentation 'extract'. Generally everey "feature" in airflow has some documentation that aims (and sometimes fail) to explain the feature for the user in a more "howto" fashion. That's the goal we strive for -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
vandonr commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2025312796 > using a new public API call (`get_last_task_failure`- and add documentation about it). by "new API" you meant just a method on the class, right ? Is there more doc to update besides the sample dag and the docstring for the method itself ? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
potiuk commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2015166039 > Even though this feature is experimental, the openlineage provider relies on it, so I don't think we can just go ahead and change it Agreed. That's the hard nut to crack. But maybe other idea. How about you put the error message in "thread local" variable and allow to retrieve it from there using a new public API call (`get_last_task_failure`- and add documentation about it). Maybe not a nicest API, but it could be backwards compatible and will allow us to extend the callbacks in the future. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
vandonr commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2011722635 Even though this feature is experimental, the openlineage provider [relies on it](https://github.com/apache/airflow/blob/83e83e272fdd359e7b3fd7ef368681c10d825220/airflow/providers/openlineage/plugins/listener.py#L174), so I don't think we can just go ahead and change it -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
ephraimbuddy commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2011336350 > > > This would be rather hacky. Likely better to add a new method `on_task_instanc_failed_with_error` > > > > > > surely possible, but it doesn't look very clean... And even if we obsolete the other method, we'll be stuck with the weird name. > > Would it make it less hacky if the field was added as a proper class field to `TaskInstance` ? > > TaskInstance is a Data Model. We should not add extra fields there which are not related to DataModel. We have done that in the past and it was not good.. That **really** is hacky. I had an idea for this type of stuff but it seemed disruptive - have a state data model that includes a message field in which we can store errors like this https://github.com/apache/airflow/pull/37896/files. I think, that's the best way to capture these kinds of errors seamlessly. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
potiuk commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2009921785 > > This would be rather hacky. Likely better to add a new method `on_task_instanc_failed_with_error` > > surely possible, but it doesn't look very clean... And even if we obsolete the other method, we'll be stuck with the weird name. > > Would it make it less hacky if the field was added as a proper class field to `TaskInstance` ? TaskInstance is a Data Model. We should not add extra fields there which are not related to DataModel. We have done that in the past and it was not good.. That **really** is hacky. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
vandonr commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2009894978 > This would be rather hacky. Likely better to add a new method `on_task_instanc_failed_with_error` surely possible, but it doesn't look very clean... And even if we obsolete the other method, we'll be stuck with the weird name. Would it make it less hacky if the field was added as a proper class field to `TaskInstance` ? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Give `on_task_instance_failed` access to the error that caused the failure [airflow]
potiuk commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-2009022742 This would be rather hacky. Likely better to add a new method `on_task_instacen_failed_with_error` -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] give on_task_instance_failed access to the error that caused the failure [airflow]
boring-cyborg[bot] commented on PR #38155: URL: https://github.com/apache/airflow/pull/38155#issuecomment-1997939548 Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst) Here are some useful points: - Pay attention to the quality of your code (ruff, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-pre-commit-hooks) will help you with that. - In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it. - Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations. - Be patient and persistent. It might take some time to get a review or get the final approval from Committers. - Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack. - Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices). - Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits. Apache Airflow is a community-driven project and together we are making it better . In case of doubts contact the developers at: Mailing List: d...@airflow.apache.org Slack: https://s.apache.org/airflow-slack -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] give on_task_instance_failed access to the error that caused the failure [airflow]
vandonr opened a new pull request, #38155: URL: https://github.com/apache/airflow/pull/38155 I was going to ask this as a question, but decided to do it as a PR instead given how simple the change is. I was writing a listener, and it seems extremely hard to get some information on the error in the `on_task_instance_failed` callback, because the error is not passed as a parameter to the callback itself :disappointed: It's [written to the context](https://github.com/apache/airflow/blob/9e97433dc3368138431305c5161a007e4fc5f227/airflow/models/taskinstance.py#L2809-L2810) a bit further down, but we don't have that yet when the callback is called. We cannot add an extra parameter now because it'd be a breaking change, but what do you think about storing the error in the TaskInstance object before calling `on_task_instance_failed` ? It'd be a pretty cheap way to solve that issue (if it's one!). We don't need to persist that in DB or anything, it just needs to carry the value to the method call that just follows, seems simple enough ? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
