Dev-iL commented on issue #67799: URL: https://github.com/apache/airflow/issues/67799#issuecomment-5912466212
@zach-overflow Thank you for your willingness to contribute! > Is the intent here to reduce the scope of alterations for async onboarding? Or are there other reasons to use `AsyncSession.run_sync` instead of defining an async version of the `@provide_session`-decorated methods? It isn't meant to limit scope. The line guards against two specific traps: - Passing an `AsyncSession` to a sync `@provide_session` method doesn't fail at the call site. A body that calls `session.scalar(...)` gets back an un-awaited coroutine. Bodies that chain `.scalars(...).all()` or use `.query` fail with `AttributeError` instead. - `run_sync` is fine for a unit that is only SQL. A 0.6s server-side wait under `run_sync` kept the event loop responsive (max gap ~11ms, same as a native await) on all backends. But a `time.sleep(0.4)` inside the same unit stalled the loop for ~400ms on every one of them. So `run_sync` doesn't make network, filesystem or secrets-backend calls non-blocking. The skill doesn't prefer `run_sync`; native async is the first choice. You're right that it doesn't say async twins are welcome, or that `run_sync` is OK for pure-SQL units. I'll clarify that in #73405. > I found it was not difficult to define a @provide_session_async decorator, and modify / add coroutines as needed without having to do any sync thread offloading calls. > > If there is a need for the @provide_session_async decorator I can contribute that as a standalone PR. Let me know if I'm misunderstanding the constraints though. I asked the AI to test this approach. TL;DR: it agrees that the decorator is easy, but `/task-instances/states` doesn't need it, so the recommendation is to wait with it until something does. <details><summary><h4>Full analysis report by Claude</h4></summary> <p> > I wrote a ~13-line prototype on top of `find_session_idx`. It passes the session through when given and creates one when omitted, so it works as you described. > > For `/task-instances/states`, every call on the path already passes `session`. Only the `task_group_id` branch goes beyond the route: `_get_group_tasks` → `get_latest_version_of_dag` → `DBDagBag.get_latest_version_of_dag` → `SerializedDagModel.get`. `latest_item_select_object` is already a plain `select` and the data columns aren't deferred, so the async version can be: > > ```python > serdag = await session.scalar(SerializedDagModel.latest_item_select_object(dag_id)) > dag = dag_bag._read_dag(serdag) # existing sync, session-free > ``` > > Against a seeded 500-task Dag on SQLite this returns the same Dag as the sync path, with no `MissingGreenlet`. No twin, decorator or `run_sync` is needed. > > One open point: `_read_dag` deserializes the whole Dag on every call and never hits the cache on this path. A 500-task Dag took ~32ms, which would now run on the event loop instead of a threadpool worker. Probably fine, but worth a decision (offload or accept) in the PR. > > My one worry about the decorator: if a caller forgets `session`, it silently opens a second connection. With a pool of one, I reproduced a route timing out waiting on its own pool. Routes should pass the `AsyncSessionDep` session down; `dag.clear(...)` in `clear_dag_run` currently doesn't. The legitimate users are entry points with no caller-owned session, like secrets backends. My async-secrets WIP hand-rolls `if session is None: async with create_session_async()` twice. > > So I'd convert `/states` without the decorator. If you want to propose it, I'd tie it to its first consumers (async secrets) so reviewers can see it in use, and get a committer's view first, since `create_session_async` is `:meta private:` today. </p> </details> -- 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]
