Skip to content
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

♻️ Replace aiopg in catalog service #2869

Merged
merged 41 commits into from
Mar 10, 2022

Conversation

sanderegg
Copy link
Member

@sanderegg sanderegg commented Mar 4, 2022

What do these changes do?

This PR is following #2864 that updated SQLalchemy to the latest version and left the catalog service with some functional but suboptimal code such that aiopg could work with the newer SQLalchemy syntax.

This PR replaces aiopg/psycopg2 with asyncpg and allows a full integration with SQLalchemy ONLY in the catalog for now.
This PR also introduces the use of the library aiocache and is currently used to cache the list of services returned by the director-v0 for a small performance boost in listing the services according to service access rights. This library can also be used with redis to have a distributed cache.
Currently we list all the services with all the details, etc. The most time consuming part is creating the ServiceOut objects at the end. There I don't see how to fix that for now, unless we reduce the number of objects?

Using the new async engine in SQLalchemy implies:

  • replace any occurence of acquire() with connect()
  • if using a construction with async for row... then one needs to call conn.stream( instead of conn.execute(
  • SQL calls with a string is not allowed, one needs to pass a sa.DDL type with the string inside

Additional:

  • added a fixture to create a SQLalchemy async engine in postgres_service.py
  • catalog now caches the list of services in the director-v0 for 5 minutes using a TTL cache

Bonus:

  • added pytest-benchmark for benchmarking tests

reference for SQLAlchemy in async mode: https://docs.sqlalchemy.org/en/14/orm/extensions/asyncio.html#synopsis-core

Related issue/s

How to test

Checklist

@sanderegg sanderegg self-assigned this Mar 4, 2022
@codecov
Copy link

codecov bot commented Mar 4, 2022

Codecov Report

Merging #2869 (66fcd7e) into master (6c63bef) will decrease coverage by 6.3%.
The diff coverage is 61.1%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2869     +/-   ##
========================================
- Coverage    79.1%   72.8%   -6.4%     
========================================
  Files         673     674      +1     
  Lines       27602   27620     +18     
  Branches     3218    3220      +2     
========================================
- Hits        21854   20117   -1737     
- Misses       5001    6856   +1855     
+ Partials      747     647    -100     
Flag Coverage Δ
integrationtests ?
unittests 72.7% <61.1%> (-2.0%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...c/simcore_postgres_database/utils_aiosqlalchemy.py 0.0% <0.0%> (ø)
...catalog/src/simcore_service_catalog/core/events.py 90.0% <ø> (ø)
...imcore_service_catalog/db/repositories/projects.py 41.6% <0.0%> (ø)
...rc/simcore_service_catalog/db/repositories/dags.py 44.4% <11.1%> (-4.3%) ⬇️
.../settings-library/src/settings_library/postgres.py 86.8% <50.0%> (-4.4%) ⬇️
...g/src/simcore_service_catalog/services/director.py 76.3% <50.0%> (ø)
.../simcore_service_catalog/db/repositories/groups.py 72.9% <70.0%> (+16.2%) ⬆️
...imcore_service_catalog/db/repositories/services.py 77.7% <76.4%> (+5.5%) ⬆️
...s/catalog/src/simcore_service_catalog/db/events.py 75.0% <87.5%> (-10.8%) ⬇️
...mcore_service_catalog/api/dependencies/database.py 100.0% <100.0%> (+12.5%) ⬆️
... and 113 more

@sanderegg sanderegg force-pushed the enhancement/replace_aiopg branch 2 times, most recently from 1fa0a11 to 8647cbc Compare March 7, 2022 12:08
@sanderegg sanderegg force-pushed the enhancement/replace_aiopg branch from 3ee44cd to 7366d02 Compare March 7, 2022 16:15
@sanderegg sanderegg changed the title WIP: ♻️ Replace aiopg in catalog service ♻️ Replace aiopg in catalog service Mar 7, 2022
@sanderegg sanderegg added this to the R.Schumann+1 milestone Mar 7, 2022
@sanderegg sanderegg marked this pull request as ready for review March 7, 2022 16:17
@sanderegg sanderegg force-pushed the enhancement/replace_aiopg branch from bddc27f to 259e0c2 Compare March 7, 2022 16:41
Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 very nice

from sqlalchemy.ext.asyncio import create_async_engine

engine = create_async_engine(
f"{postgres_db.url}".replace("postgresql", "postgresql+asyncpg")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this will stay in there until we fully remove aiopg. Maybe put a note.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the only location where the async engine is created (pytest-simcore package). but no worries, the PostgresSettings also have a special attribute that does it for you.

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work. Just a few comments!

@@ -21,12 +21,14 @@ fastapi[all]
pydantic[dotenv]

# database
aiopg[sa]
asyncpg
sqlalchemy[asyncio]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two things, we already introduce this "extra"

sqlalchemy[postgresql_psycopg2binary]

should we always extra with both?

sqlalchemy[asyncio,postgresql_psycopg2binary]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no I think once we can fully migrate, the psycopg2binary becomes obsolete. maybe just for testing where we do have a sync engine as well

tests/performance/locust_files/catalog_servicesd.py Outdated Show resolved Hide resolved
@sanderegg sanderegg force-pushed the enhancement/replace_aiopg branch from 17f1eb8 to b5b209b Compare March 10, 2022 07:15
@sanderegg sanderegg merged commit dcad2db into ITISFoundation:master Mar 10, 2022
@sanderegg sanderegg deleted the enhancement/replace_aiopg branch March 10, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants