-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add AsyncSession support for non-blocking db operations #4408
Conversation
await session.commit() | ||
return unmasked_api_key | ||
|
||
unmasked_api_key = asyncio.run(aapi_key()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since create_api_key and delete_api_key are now coroutines, we run the command in an event loop.
I think it's OK as it's a CLI and so a kind of entry point.
But please comment if you think there could be issues like conflicts with other event loops.
The other solution would be to duplicate create_api_key and delete_api_key to have both a sync and an async version.
@@ -3,7 +3,7 @@ | |||
|
|||
from fastapi import APIRouter, Depends, HTTPException, Response | |||
|
|||
from langflow.api.utils import CurrentActiveUser, DbSession | |||
from langflow.api.utils import AsyncDbSession, CurrentActiveUser, DbSession |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: we can't replace atm the DbSession by AsyncDbSession in save_store_api_key as it gets the current user with the sync session and so we can't modify it with the async session.
url_components = self.database_url.split("://", maxsplit=1) | ||
if url_components[0].startswith("sqlite"): | ||
database_url = "sqlite+aiosqlite://" | ||
kwargs = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aiosqlite doesn't have a connection pool.
I don't think that's a big issue but please comment if you think it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these changes to the starter projects are wrong because of a bug we had. You should merge with main and run the backend again to remove them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
Is the greenlet dependency missing? |
c046fea
to
28a8c0e
Compare
Oh right, even though there are now built wheels of greenlet for M1, it seems sqlalchemy doesn't pull them by default. Will fix. |
* Add AsyncSession support for non-blocking db operations * Use sqlalchemy extras
It uses:
I tested manually that it works on PostgreSQL