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
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant