Remove the mixture of async endpoints with a sync db connection

[shangyian]

Summary

We should not mix async API endpoints with a synchronous db connection (psycopg3). Because our database client does not support async, every database call will introduce blocking calls to the event loop, eventually resulting in the uvicorn workers timing out.

The issue is very similar to the one described here (in their case it was sync API call vs sync database query): tiangolo/uvicorn-gunicorn-fastapi-docker#145 (comment)

Test Plan

N/A

• PR has an associated issue: #
• make check passes
• make test shows 100% unit test coverage

Deployment Plan

Ideally we deploy this right away. Otherwise over time the deployed DJ servers will experience more and more blocking calls, gradually causing workers to time out.

[netlify]

✅ Deploy Preview for thriving-cassata-78ae72 canceled.

Name	Link
🔨 Latest commit	1245f28
🔍 Latest deploy log	https://app.netlify.com/sites/thriving-cassata-78ae72/deploys/65b2fadc1cf89c000842b1a2

[agorajek]

Thanks @shangyian for catching this. @betodealmeida have you seen something like this before?

[CircArgs]

@shangyian seeing as the bottleneck has been the orm and db calls, should DJ move to async engines and sessions?

@samredai and I were working on something and came out to approximately:

def get_engine() -> AsyncEngine:
    """
    Create the metadata engine.
    """
    ...

    def getconn():
        conn = connector.connect(instance_connection_name, "pg8000", user=db_user, password=db_pass, db=db_name)
        return conn

    engine = sqlalchemy.create_engine("postgresql+pg8000://", creator=getconn, future=True)
    return engine

as opposed to

dj/datajunction-server/datajunction_server/utils.py

Line 57 in 2df3427

def get_engine() -> Engine:

[shangyian]

@CircArgs yeah, I was investigating this in December and started working on a PR for it, but the necessary changes are actually huge: main...shangyian:dj:use-asyncpg

The part where we just create an async engine is pretty easy, i.e., main...shangyian:dj:use-asyncpg#diff-5ebc477598017d1697e464ee79c28d2cdd36c55b2b904a358a8e5918ab4b2109R13.

However, we also have to prevent implicit io which SQLAlchemy does whenever we dynamically pull in different objects on a given model: sqlalchemy/sqlalchemy#6165 (for example if we call node.parents). Accounting for that across all our endpoints is... not that easy. It's definitely not impossible though and I think I got to a point where 50% of tests were passing. If you want to take a stab at it, go for it!

Also fwiw I don't think bottleneck was database calls, but rather database calls that block the eventloop, which only happens when we issue sync calls from an async endpoint. We might eventually get to a point where database throughput is a problem, but we're not there yet (at least internally at Netflix, that is).

[CircArgs]

@shangyian hmm sqlalchemy strikes again

necessary changes are actually huge
...
if you want to take a stab at it, go for it!

😨

[shangyian]

hmm sqlalchemy strikes again

@CircArgs yeah it's probably also that we weren't using it in the most efficient way... but nonetheless, here we are :(

😨

Yeaaaah unfun