Fix asyncpg event loop bug with startup wrapper patch - #23
Conversation
c2c40d7 to
27b0220
Compare
|
/retest |
omkarjoshi0304
changed the title
27b0220 to
3f98be8
Compare
3f98be8 to
378cb0c
Compare
umago
left a comment
umago
left a comment
There was a problem hiding this comment.
Thanks Omkar, tested it locally and it works.
The code does look "black magic"-ish but this is a temporary workaround so, I don't think we need to spend too much time on it.
Thanks! I will not merge it immediately in case someone else from the team wants to take a look as people may have different opinions on this but, if nobody else review this, I'm happy to merge it later too. Let me know!
lpiwowar
left a comment
lpiwowar
left a comment
There was a problem hiding this comment.
Overall LGTM, thank you for fixing this:). It's nice this will get fixed.
I have just two questions I personally need answered before I would feel comfortable with /lgtm. Both feel important to me. So I'm marking this with "Request changes". But once they are answered I'm 💯 OK with merging immediately:).
378cb0c to
25f6848
Compare
|
/retest |
25f6848 to
dafdda9
Compare
dafdda9 to
335b87f
Compare
|
@omkarjoshi0304: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
StackApp.__init__() runs Stack.initialize() in a temporary event loop via ThreadPoolExecutor + asyncio.run(). Any asyncpg connections created during init get bound to this temp loop, causing the first request on uvicorn's real event loop to fail with: RuntimeError: Task got Future attached to a different loop The upstream fix (llama-stack PR #5837) adds reset_engine() to null out _engine and async_session after init, relying on the existing _ensure_engine() method to lazily recreate them on the correct loop. The current container image has neither reset_engine() nor _ensure_engine() — the engine and session are created once in __init__ and never checked again. Setting async_session to None alone crashes with TypeError because there is no lazy recreation path. The SessionMaker class provides that missing lazy recreation externally — on the first call it recreates the engine on the correct event loop, then replaces itself with the real session maker. Remove this workaround when the container image includes PR #5837.
335b87f to
52e77fb
Compare
lpiwowar
left a comment
lpiwowar
left a comment
There was a problem hiding this comment.
/lgtm
Thank you:), let's see what the jobs think.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lpiwowar, omkarjoshi0304 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
903638a
into
openstack-k8s-operators:main
3 participants
The first request to llama-stack after a pod start fails with RuntimeError because asyncpg connections created during StackApp init are bound to a temporary event loop. This adds a Python wrapper script that patches StackApp.init to lazily recreate SQL engines on uvicorn's event loop, matching the upstream fix (PR #5837) that hasn't been built into the container image yet.
Remove when the container image includes the upstream fix.