You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "m1racoli (via GitHub)" <gi...@apache.org> on 2025/03/04 00:22:04 UTC
[PR] fix: don't use blocking property access for async purposes [airflow]
m1racoli opened a new pull request, #47326:
URL: https://github.com/apache/airflow/pull/47326
The AwsGenericHook provides the `@property` decorated method `async_conn` for building and accessing the async boto3 client. Unfortunately, this is results in blocking calls to the Airflow Db or secrets backends within async contexts.
This PR provides an async method `get_async_conn` as an alternative that can be awaited in async contexts. This method will call the original sync code in a `sync_to_async` wrapper to avoid any side effects from potentially blocking `@property` or `@cached_property` decorated methods used downstream.
The `async_conn` property is now deprecated and will be removed in future versions.
closes: #47312
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "m1racoli (via GitHub)" <gi...@apache.org>.
m1racoli commented on code in PR #47326:
URL: https://github.com/apache/airflow/pull/47326#discussion_r1981574934
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -747,7 +748,22 @@ def conn(self) -> BaseAwsConnection:
@property
def async_conn(self):
+ """
+ [DEPRECATED] Get an aiobotocore client to use for async operations.
+
+ This property is deprecated. Accessing it in an async context will cause the event loop to block.
+ Use the async method `get_async_conn` instead.
+ """
Review Comment:
I originally did, but pre-commit complained about deprecation warnings in providers.
https://github.com/apache/airflow/pull/47326/commits/7c45860e474bda815275cc2eae30a566209eb9c7
Let me know how I can add it again without disappointing pre-commit. :)
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "m1racoli (via GitHub)" <gi...@apache.org>.
m1racoli commented on PR #47326:
URL: https://github.com/apache/airflow/pull/47326#issuecomment-2701218730
> I'm curious how noticeable the blockage on the async loop was though. Did you have a very noticeable issue that caused you to drill down and find this?
I've been working with a customer. They have a DAG which kicks off up to 300 deferrable `EcsRunTaskOperator` tasks and they noticed significant performance degradation. We observed the log message
```
Triggerer's async thread was blocked for 8.11 seconds, likely by a badly-written trigger. Set PYTHONASYNCIODEBUG=1 to get more information on overrunning coroutines.
```
appearing 10-19 times per minute with values of 2-11 seconds. In particular the TaskDoneTrigger [instantiates 2 clients per run](https://github.com/apache/airflow/blob/4651ccce787b33af9ea03f43a876538d806ac68a/providers/amazon/src/airflow/providers/amazon/aws/triggers/ecs.py#L170-L171) (one each for EcsHook and AwsLogsHook). The triggerer basically was blocked for ten's of minutes while sequentially instantiating those async clients.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #47326:
URL: https://github.com/apache/airflow/pull/47326#issuecomment-2699312039
I think I see where you are going. Thanks for looking into this and for the extra info, what you are saying makes sense. I'm going to step back and let others make the call, but I'll follow along an learn what I can.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "m1racoli (via GitHub)" <gi...@apache.org>.
m1racoli commented on PR #47326:
URL: https://github.com/apache/airflow/pull/47326#issuecomment-2699320385
Sure! :) Let's see what others think about 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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas merged PR #47326:
URL: https://github.com/apache/airflow/pull/47326
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "m1racoli (via GitHub)" <gi...@apache.org>.
m1racoli commented on PR #47326:
URL: https://github.com/apache/airflow/pull/47326#issuecomment-2698535655
> I think your analysis is correct (I haven't checked it, but you make a convincing argument) but I wonder if it's possible to solve this without changing the interface 🤔
Not changing the interface would be ideal, but I believe we need to return something **awaitable** in order to avoid blocking and that breaks the interface either way. To put it differently, the interface is part of the problem. 🤔
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #47326:
URL: https://github.com/apache/airflow/pull/47326#discussion_r1980459621
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -747,7 +748,22 @@ def conn(self) -> BaseAwsConnection:
@property
def async_conn(self):
+ """
+ [DEPRECATED] Get an aiobotocore client to use for async operations.
+
+ This property is deprecated. Accessing it in an async context will cause the event loop to block.
+ Use the async method `get_async_conn` instead.
+ """
Review Comment:
We should log a deprecation warning here 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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "m1racoli (via GitHub)" <gi...@apache.org>.
m1racoli commented on code in PR #47326:
URL: https://github.com/apache/airflow/pull/47326#discussion_r1981642030
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -747,7 +748,22 @@ def conn(self) -> BaseAwsConnection:
@property
def async_conn(self):
+ """
+ [DEPRECATED] Get an aiobotocore client to use for async operations.
+
+ This property is deprecated. Accessing it in an async context will cause the event loop to block.
+ Use the async method `get_async_conn` instead.
+ """
Review Comment:
Done, let me know what you think.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #47326:
URL: https://github.com/apache/airflow/pull/47326#issuecomment-2698429452
I think your analysis is correct (I haven't checked it, but you make a convincing argument) but I wonder if it's possible to solve this without changing the interface 🤔
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "m1racoli (via GitHub)" <gi...@apache.org>.
m1racoli commented on PR #47326:
URL: https://github.com/apache/airflow/pull/47326#issuecomment-2698859281
> Is it possible to do this while keeping the property or cached_property decorator? We're trying to get rid of get_conn() since it's redundant with the conn property, it would be awkward to have two different means of interacting for sync and async like that.
I don't think `@property` or `@cached_property` are compatible with `async` at all. And generally speaking they can be tricky as they obfuscate actual work being done but evaluate immediately on access and don't allow you to pass them around or wrap them.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #47326:
URL: https://github.com/apache/airflow/pull/47326#discussion_r1981907492
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -747,7 +748,22 @@ def conn(self) -> BaseAwsConnection:
@property
def async_conn(self):
+ """
+ [DEPRECATED] Get an aiobotocore client to use for async operations.
+
+ This property is deprecated. Accessing it in an async context will cause the event loop to block.
+ Use the async method `get_async_conn` instead.
+ """
Review Comment:
lgtm!
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "m1racoli (via GitHub)" <gi...@apache.org>.
m1racoli commented on code in PR #47326:
URL: https://github.com/apache/airflow/pull/47326#discussion_r1981574934
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -747,7 +748,22 @@ def conn(self) -> BaseAwsConnection:
@property
def async_conn(self):
+ """
+ [DEPRECATED] Get an aiobotocore client to use for async operations.
+
+ This property is deprecated. Accessing it in an async context will cause the event loop to block.
+ Use the async method `get_async_conn` instead.
+ """
Review Comment:
I originally did, but pre-commit complained about deprecation warnings in providers.
https://github.com/apache/airflow/pull/47326/commits/7c45860e474bda815275cc2eae30a566209eb9c7
Let me know how I can add it again without disappointing pre-commit.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "m1racoli (via GitHub)" <gi...@apache.org>.
m1racoli commented on PR #47326:
URL: https://github.com/apache/airflow/pull/47326#issuecomment-2698875101
If you really want to keep `@property` you might be able to return a coroutine
```python
class AwsGenericHook:
@property
def async_conn(self) -> coroutine[client]:
return sync_to_async(self._get_async_conn)()
```
but you will still need to await it
```python
async def run(self):
client = await self.hook.async_conn
```
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #47326:
URL: https://github.com/apache/airflow/pull/47326#issuecomment-2698991753
I really don't have enough background with python's async implementation to make the call.
IMHO, I'd prefer to see a breaking implementation change that allows users to use `hook.conn.foo()` and `hook.async_conn.foo()` instead of having to remember that sync calls are `hook.conn.foo()`` and async calls are `hook.get_async_conn().foo()`
Let's see what others think. I have the opinions but not the practical experience with this particular issue, so I suppose they aren't worth all that much in the end. :P
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #47326:
URL: https://github.com/apache/airflow/pull/47326#issuecomment-2698673837
Is it possible to do this while keeping the property or cached_property decorator? We're trying to get rid of get_conn() since it's redundant with the conn property, it would be awkward to have two different means of interacting for sync and async like that.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "m1racoli (via GitHub)" <gi...@apache.org>.
m1racoli commented on PR #47326:
URL: https://github.com/apache/airflow/pull/47326#issuecomment-2699304219
> I really don't have enough background with python's async implementation to make the call.
>
> IMHO, I'd prefer to see a breaking implementation change that allows users to use `hook.conn.foo()` and `hook.async_conn.foo()` instead of having to remember that sync calls are `` hook.conn.foo()`` and async calls are ``hook.get_async_conn().foo()`
>
> Let's see what others think. I have the opinions but not the practical experience with this particular issue, so I suppose they aren't worth all that much in the end. :P
I do get your intentions. :) But I fear that this might not be possible. In particular `hook.async_conn.foo()` will be impossible (without some black magic putting an async wrapper around a maybe-init boto3 client), because getting to `async_conn` requires IO which needs to be awaited before you can even get to `foo`.
The issue is `@property` and `@cached_propery` obfuscate a lot of work away from the user. This can be nice, because now you can just access a property and everything falls into place. But it also fails to communicate that the thing we want actually requires some work to be done.
You can get away with it in normal (sync) code, but in `async` this obfuscation becomes a liability, because `async` requires async all they way down to the actual awaitable IO call (or at least some way of awaitable delegation to a different thread, like we do it with `sync_to_async`) and clearly defined breakpoints (await). This makes sync generally not play nice inside async code and also affects how async code usually is being used.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: don't use blocking property access for async purposes [airflow]
Posted by "m1racoli (via GitHub)" <gi...@apache.org>.
m1racoli commented on code in PR #47326:
URL: https://github.com/apache/airflow/pull/47326#discussion_r1981628372
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -747,7 +748,22 @@ def conn(self) -> BaseAwsConnection:
@property
def async_conn(self):
+ """
+ [DEPRECATED] Get an aiobotocore client to use for async operations.
+
+ This property is deprecated. Accessing it in an async context will cause the event loop to block.
+ Use the async method `get_async_conn` instead.
+ """
Review Comment:
Ah, just found it. Need to use the right deprecation warning. 🤦🏼♂️ One sec!
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org