Repository navigation
Migrate execution API reads to async sessions - #73403
Conversation
d465b8d to
0672157
Compare
|
Related - #73407 so removed the overlap. |
Dev-iL
left a comment
There was a problem hiding this comment.
Thanks for picking up this batch of endpoints! Please forgive my delay in reviewing this.
The route selection is well judged: every converted handler does direct ORM reads with no sync helper, provide_session or commit on its path, which is exactly the kind of route that can switch cleanly. The conversions themselves are careful. Buffered results are consumed correctly ((await session.scalars(...)).one(), .first(), .mappings()), NoResultFound still maps to 404, or 0 keeps its meaning, and nothing touches a lazy attribute after the await (TI.dag_run is lazy="joined", and the response models read only columns).
I confirmed the rebased branch passes on SQLite, PostgreSQL and MySQL. The two comments below are both about test code, mostly because main moved on after the branch was cut.
Regarding the PR description itself
-
once the fixture is gone, the following sentence no longer applies:
The tests for these routes now opt into async engine reconfiguration so the per-test FastAPI event loop uses a matching async DB engine.
-
The migration guidance also asks for backend, driver, command and result on SQLite, PostgreSQL and MySQL; the listed run is a host
uv runon the default SQLite backend. Abreeze run --backend <backend>pass over the touched test classes should be quick; they were green on all three backends for me. -
Finally, one line on the CPU-bound work that moves onto the event loop would close the last gap. Each converted handler does a single small read plus Pydantic validation, so "negligible, accepted" is enough.
P.S. I've been updating the skill in #73405 in light of recent feedback and changes on main. You might find it useful for future migrations.
Signed-off-by: subhramit <subhramit.bb@live.in>
Signed-off-by: subhramit <subhramit.bb@live.in>
Signed-off-by: subhramit <subhramit.bb@live.in>
b04da60 to
5c6450e
Compare
|
@Dev-iL Thanks for the review. have addressed your comments and updated the PR description. |
Migrate a small group of Execution API read endpoints from sync DB access to
AsyncSessionDep.This updates direct ORM read handlers that can safely switch to async without pulling in sync-only helpers, including:
variable key listing(superseded by MigrateGET /execution/variables/keysto an async DB session #73407)Each converted handler does a single small read plus Pydantic validation, so the CPU-bound work that moves onto the event loop is negligible and accepted for this batch.
Tests
Ran
results:
=============== 597 passed, 4 skipped, 2 warnings in 1157.70s (0:19:17) ===============(warnings were unrelated
JWT reissue middleware failed to refresh token: Not enough segments)Was generative AI tooling used to co-author this PR?
Assisted-by: Zed GPT-5.4 following the guidelines. I used it to navigate code and manually instructed it after going through the above linked issue and PR.