-
Notifications
You must be signed in to change notification settings - Fork 31
Asynced the Database #659 and #649 #660
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
base: main
Are you sure you want to change the base?
Conversation
Pull reviewers statsStats of the last 120 days for UWOrbital:
|
Syzygicality
left a comment
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.
Please fix pytests before Kevin and I can make a proper code review! Most failures are due to incorrect typehinting in the pytests and use of .exec() on asyncsession, which should instead be .execute()
Summary of Changes
|
proprogrammer504
left a comment
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.
slightly terrified, it looks good mostly. there are some inconsistencies with usage of async and await. additionally, there is a change to the ax25 protocol that i do not understand. this may be a dangerous change. finally, i want another evaluation on whether or not the things which we asynced actually NEED to be asynced. i feel that if something doesnt require it, it should be left as is as it is safer to have less moving parts.
tldr, maybe dont touch ax25, fix inconsistencies.
| DATABASE_CONNECTION_STRING: Final[ | ||
| str | ||
| ] = f"postgresql+psycopg2://{GS_DATABASE_USER}:{GS_DATABASE_PASSWORD}@{GS_DATABASE_LOCATION}:{GS_DATABASE_PORT}/{GS_DATABASE_NAME}" | ||
| ] = f"postgresql+asyncpg://{GS_DATABASE_USER}:{GS_DATABASE_PASSWORD}@{GS_DATABASE_LOCATION}:{GS_DATABASE_PORT}/{GS_DATABASE_NAME}" |
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.
does this allow for the db to be asynchronous? i thought it already was asynchronous
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.
the new one is the async driver for psql connection
| with get_db_session() as session: | ||
| async with get_db_session() as session: | ||
| obj = self.model(**data) | ||
| session.add(obj) |
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.
i mean since we're going nuts and awaiting everything, does this need await as well?
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.
yep, though looking at it, we could just add session: AsyncSession = Depends(get_db_session) into the function args of the abstract wrapper
| session.commit() | ||
| return obj | ||
| # Preserve object state before deleting | ||
| obj_copy = self.model(**{c.name: getattr(obj, c.name) for c in obj.__table__.columns}) # type: ignore[attr-defined] |
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.
why not use session.delete(obj)?
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.
yeah cursor bugged out here thinking that db deletion deletes the object from the memory, though it doesnt. pls return obj, not obj_copy
| if auth_token: | ||
| session.delete(auth_token) | ||
| session.commit() | ||
| session.delete(auth_token) # type: ignore[unused-coroutine] |
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.
yea, this seems more logical. also the await seems inconsistent. sometimes session uses await yet other times it does not.
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.
in terms of db connection, awaits should only be used on IO or expensive processes, so usually commit and refresh session methods
| raise ValueError("Packet command not found.") | ||
| session.delete(command) | ||
| session.commit() | ||
| session.delete(command) # type: ignore[unused-coroutine] |
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.
i just realized that there r a bunch of the type: ignore unused coroutine comments. what r those about?
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.
linting error escape most likely
| from contextlib import asynccontextmanager | ||
|
|
||
| from sqlalchemy import text | ||
| from sqlalchemy.ext.asyncio import AsyncEngine, AsyncSession, create_async_engine |
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.
do we understand the side effects of using async engine and async session instead of the regular ones?
| result = session.exec(query).first() | ||
| if not result: | ||
| result = await session.execute(query) | ||
| if not result.scalars().first(): |
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.
what does this check for exactly? would this prevent main commands from being added in some rare cases?
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.
if the db has already been populated with the data, then this conditional ensures that they wont insert it again, so we dont have duplicated datasets in the tables.
| if (data[-1] == 0 and len(data) > RS_ENCODED_DATA_SIZE + AX25_NON_INFO_BYTES) or ( | ||
| data[-1] == 0 and len(data) < RS_ENCODED_DATA_SIZE | ||
| ): | ||
| # Only remove the byte if we're certain it's padding (exactly 1 byte more than expected for I frames) |
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 that this change could be dangerous, it also is unrelated to making stuff asynchronous. my concern is that this is a part of the ax25 protocol, changing anything here could have unforeseen consequences especially when we dont understand how it works.
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.
i agree. this change is out of the scope of the task
| @@ -20,60 +20,97 @@ | |||
| from gs.backend.data.enums.aro_requests import ARORequestStatus | |||
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 that these endpoints are outdated
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.
what should be tested and asynced (if not already) are eddies abstracted data wrappers. also correction, they are not endpoints they are data wrappers.
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.
yeah, this is alr a task -> #635
Syzygicality
left a comment
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.
some of my own changes for now, will check up on pytests later
|
|
||
| if __name__ == "__main__": | ||
|
|
||
| async def main() -> None: |
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.
wrap this whole script in one async with get_db_session() as session: instead of multiple
| obj = await session.get(self.model, obj_id) | ||
| if not obj: | ||
| raise ValueError(f"{self.model.__name__} with ID {obj_id} not found.") | ||
| session.delete(obj) |
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.
revert this change to return obj, not make a obj copy
Purpose
Closes #659 and #649
Converts the entire backend database layer from synchronous to asynchronous operations to improve concurrent request handling and FastAPI compatibility.
New Changes
AsyncEngineandAsyncSessionfrom SQLAlchemypostgresql+asyncpg://driverasyncpgas a dependency for async PostgreSQL supportasyncio.run()for async executionModified Files (22 total):
Core Database Layer:
gs/backend/data/database/engine.pygs/backend/config/config.pyAbstract & Main Wrappers:
gs/backend/data/data_wrappers/abstract_wrapper.pygs/backend/data/data_wrappers/wrappers.pyMCC Wrappers (8 files):
gs/backend/data/data_wrappers/mcc_wrappers/commands_wrapper.pygs/backend/data/data_wrappers/mcc_wrappers/main_command_wrapper.pygs/backend/data/data_wrappers/mcc_wrappers/comms_session_wrapper.pygs/backend/data/data_wrappers/mcc_wrappers/main_telemetry_wrapper.pygs/backend/data/data_wrappers/mcc_wrappers/packet_commands_wrapper.pygs/backend/data/data_wrappers/mcc_wrappers/packet_telemetry_wrapper.pygs/backend/data/data_wrappers/mcc_wrappers/packet_wrapper.pygs/backend/data/data_wrappers/mcc_wrappers/telemetry_wrapper.pyARO Wrappers (4 files):
gs/backend/data/data_wrappers/aro_wrapper/aro_user_login_wrapper.pygs/backend/data/data_wrappers/aro_wrapper/aro_user_auth_token_wrapper.pygs/backend/data/data_wrappers/aro_wrapper/aro_user_data_wrapper.pygs/backend/data/data_wrappers/aro_wrapper/aro_request_wrapper.pyResource Utils & Migration:
gs/backend/data/resources/utils.pygs/backend/migrate.pyAPI Layer (4 files):
gs/backend/api/lifespan.pygs/backend/api/v1/aro/endpoints/user.pygs/backend/api/v1/mcc/endpoints/commands.pygs/backend/api/v1/mcc/endpoints/main_commands.pyTesting
Testing Notes:
fastapi dev gs/backend/main.pypython3 gs/backend/migrate.pyOutstanding Changes