Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

feat: Upgrade SQLAlchemy v1.4 for native asyncio support #406

Merged
merged 21 commits into from
Mar 24, 2021

Conversation

achimnol
Copy link
Member

@achimnol achimnol commented Mar 22, 2021

Internal ticket: OP#1161

Major changes:

  • Drop references to aiopg and psycopg2 and add asyncpg as our dependency
  • Update imports and aliases for SAConnection, SAEngine (previously SAPool)
    • SQLAlchemy's engine includes the connection pool interface.
    • RowProxy is deprecated and replaced with sa.engine.row.Row
    • Row still supports __getitem__() interface but we need to use Row._mapping if we need other dict-like interfaces such as .get(key, default).
  • async with dbpool.acquire() as conn: --> async with dbpool.connect() as conn:
  • await result.{fetchall,scalar,first}() --> result.{fetchall,scalar,first}() because the result is already buffered by await conn.execute()
    • To asynchoronously stream the result from the server, use await conn.stream(query) instead of await conn.execute(query)
  • Remove deprecated .as_scalar() and use .scalar_subquery() for scalar-valued subqueries
  • Do NOT use the JIT compilation of high-cost queries in PostgreSQL v11+ to prevent excessive delay on connection setup due to asyncpg's type introspection queries (Slow introspection when using multiple custom types in a query MagicStack/asyncpg#530)
  • Reuse a single DB connection (with transaction block) for all GraphQL queries and mutations performed within a single API request
  • Explicitly wrap insert/update/delete queries with transaction blocks (async with conn.begin():) since now the default reset-on-return behavior is "rollback"
  • Rename dbpool to db

@achimnol achimnol added this to the 20.09 milestone Mar 22, 2021
* It is now reproducible reliably with the new SQLAlchemy + asyncpg combination!
* Also fixed a hidden type conversion bug in AgentRegistry.set_kernel_status() due to
  a comma typo....
@codecov
Copy link

codecov bot commented Mar 22, 2021

Codecov Report

Merging #406 (63515e9) into main (b929ba5) will increase coverage by 1.96%.
The diff coverage is 11.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #406      +/-   ##
==========================================
+ Coverage   46.70%   48.66%   +1.96%     
==========================================
  Files          51       52       +1     
  Lines        8250     8267      +17     
==========================================
+ Hits         3853     4023     +170     
+ Misses       4397     4244     -153     
Impacted Files Coverage Δ
src/ai/backend/gateway/events.py 35.96% <0.00%> (ø)
src/ai/backend/gateway/manager.py 38.23% <0.00%> (ø)
src/ai/backend/gateway/types.py 100.00% <ø> (ø)
src/ai/backend/gateway/utils.py 58.50% <0.00%> (+8.50%) ⬆️
src/ai/backend/manager/models/image.py 57.51% <0.00%> (ø)
src/ai/backend/manager/plugin/error_monitor.py 43.75% <0.00%> (ø)
src/ai/backend/manager/models/user.py 31.91% <1.28%> (+0.30%) ⬆️
src/ai/backend/manager/models/keypair.py 48.59% <2.43%> (+0.93%) ⬆️
src/ai/backend/manager/models/group.py 46.03% <5.08%> (+0.48%) ⬆️
src/ai/backend/manager/models/agent.py 50.00% <5.88%> (+1.40%) ⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b929ba5...63515e9. Read the comment docs.

* Eliminate manual primary key checks for duplicate entries by utilizing
  PostgreSQL's "on conflict" (upsert) support.
* Fix up using special characters in database passwords by correctly escaping them
  using `urllib.parse.quote_plus()`.
* rowcount in SQLAlchemy does NOT represent the number of fetched
  rows for SELECT queries, in contrast to the Python's standard DB API.
@achimnol achimnol marked this pull request as ready for review March 24, 2021 03:15
@achimnol achimnol merged commit cd7479a into main Mar 24, 2021
@achimnol achimnol deleted the feature/upgrade-sqlalchemy-with-asyncio-support branch March 24, 2021 09:17
achimnol added a commit that referenced this pull request Mar 24, 2021
* fix: a long-standing transaction error
  - It is now reproducible reliably with the new SQLAlchemy + asyncpg combination!
  - Also fixed a hidden type conversion bug in AgentRegistry.set_kernel_status() due to
    a comma typo....
* fix/test: Update codes for population of DB fixtures
  - Eliminate manual primary key checks for duplicate entries by utilizing
    PostgreSQL's "on conflict" (upsert) support.
  - Fix up using special characters in database passwords by correctly escaping them
    using `urllib.parse.quote_plus()`.
* fix: Do not rely on `rowcount` for SELECT queries
  - rowcount in SQLAlchemy does NOT represent the number of fetched
    rows for SELECT queries, in contrast to the Python's standard DB API.
* fix: excessive DB connection establishment delay and optimize GQL queries
  - refs MagicStack/asyncpg#530: apply "jit: off" option to DB connections
  - It is specified in `ai.backend.manager.models.base.pgsql_connect_opts`
  - Reuse the same single connection for all GraphQL resolvers and mutation methods
* fix: consistently use urlquote for db passwords
* fix: all DB connections are now transactions
* refactor: Finally, rename "dbpool" to "db"

Backported-From: main
Backported-To: 20.09
achimnol added a commit that referenced this pull request Mar 25, 2021
* Follow-up fixes for #406
  - fix: sqlalchemy's Row Proxy's get() method seems to be deprecated
  - fix: typo in referencing unmanaged_path

Co-authored-by: Jonghyun Park <adrysn@gmail.com>
achimnol added a commit that referenced this pull request Mar 25, 2021
* Follow-up fixes for #406
  - fix: sqlalchemy's Row Proxy's get() method seems to be deprecated
  - fix: typo in referencing unmanaged_path

Co-authored-by: Jonghyun Park <adrysn@gmail.com>
Backported-From: main
Backported-To: 20.09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant