Copilot commented on code in PR #64238:
URL: https://github.com/apache/airflow/pull/64238#discussion_r3025329717
##########
task-sdk/src/airflow/sdk/execution_time/secrets/execution_api.py:
##########
@@ -66,25 +66,26 @@ def get_connection(self, conn_id: str, team_name: str |
None = None) -> Connecti
# Convert ExecutionAPI response to SDK Connection
return _process_connection_result_conn(msg)
except RuntimeError as e:
- # TriggerCommsDecoder.send() uses async_to_sync internally, which
raises RuntimeError
- # when called within an async event loop. In greenback portal
contexts (triggerer),
- # we catch this and use greenback to call the async version
instead.
- if str(e).startswith("You cannot use AsyncToSync in the same
thread as an async event loop"):
- import asyncio
+ import asyncio
+ import greenback
- import greenback
+ msg = str(e)
+ if (
+ "You cannot use AsyncToSync in the same thread as an async
event loop" in msg
+ or "attached to a different loop" in msg
+ ):
task = asyncio.current_task()
- if greenback.has_portal(task):
+ if task and greenback.has_portal(task):
import warnings
warnings.warn(
- "You should not use sync calls here -- use `await
aget_connection` instead",
+ "Sync connection access failed due to event loop
mismatch. "
+ "Falling back to async connection retrieval.",
Review Comment:
The new warning message explains the fallback, but it no longer tells
callers what to do instead (previously it suggested using `await
aget_connection`). Consider including that actionable guidance so users hitting
this in triggerer/async contexts know how to avoid the sync call entirely.
```suggestion
"If you are running in an async or triggerer
context, use "
"`await aget_connection(conn_id)` instead of
`get_connection` "
"to avoid this sync call. Falling back to async
connection retrieval.",
```
##########
task-sdk/src/airflow/sdk/execution_time/secrets/execution_api.py:
##########
@@ -66,25 +66,26 @@ def get_connection(self, conn_id: str, team_name: str |
None = None) -> Connecti
# Convert ExecutionAPI response to SDK Connection
return _process_connection_result_conn(msg)
except RuntimeError as e:
- # TriggerCommsDecoder.send() uses async_to_sync internally, which
raises RuntimeError
- # when called within an async event loop. In greenback portal
contexts (triggerer),
- # we catch this and use greenback to call the async version
instead.
- if str(e).startswith("You cannot use AsyncToSync in the same
thread as an async event loop"):
- import asyncio
+ import asyncio
+ import greenback
- import greenback
+ msg = str(e)
+ if (
+ "You cannot use AsyncToSync in the same thread as an async
event loop" in msg
+ or "attached to a different loop" in msg
+ ):
Review Comment:
This change introduces a second RuntimeError message trigger ("attached to a
different loop"), but there is no regression test covering that new branch.
Please add a test analogous to `test_runtime_error_triggers_greenback_fallback`
that raises the cross-loop RuntimeError and asserts the greenback fallback path
is used.
##########
task-sdk/src/airflow/sdk/execution_time/secrets/execution_api.py:
##########
@@ -66,25 +66,26 @@ def get_connection(self, conn_id: str, team_name: str |
None = None) -> Connecti
# Convert ExecutionAPI response to SDK Connection
return _process_connection_result_conn(msg)
except RuntimeError as e:
- # TriggerCommsDecoder.send() uses async_to_sync internally, which
raises RuntimeError
- # when called within an async event loop. In greenback portal
contexts (triggerer),
- # we catch this and use greenback to call the async version
instead.
- if str(e).startswith("You cannot use AsyncToSync in the same
thread as an async event loop"):
- import asyncio
+ import asyncio
+ import greenback
Review Comment:
`asyncio`/`greenback` are imported for every `RuntimeError`, even when the
message doesn't match the event-loop cases. This adds unnecessary work in the
common path for unrelated runtime errors and also keeps imports inside a method
body. Consider moving these imports to module scope, or at least deferring them
until after the message check so they only happen when the greenback fallback
branch is actually taken.
--
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]
