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

Add a connector for SQLAlchemy with Psycopg2 #453

Merged
merged 6 commits into from
Sep 27, 2021
Merged

Conversation

elemoine
Copy link
Contributor

@elemoine elemoine commented Sep 24, 2021

This PR adds a connector for SQLAlchemy with Psycopg2. The goal is for synchronous applications that use SQLAlchemy (e.g. a Flask application with the Flask-SQLAlchemy extension) to use just one SQLAlchemy engine, so just one connection pool, thereby minimizing the number of connections opened against the PostgreSQL server.

It works like this:

from slqlalchemy import create_engine

from procrastinate import App
from procrastinate.contrib.sqlalchemy import SQLAlchemyPsycopg2Connector

engine = create_engine("postgresql+psycopg2://")

app = App(connector=SQLAlchemyPsycopg2Connector())
app.open(engine)

Closes #445

Successful PR Checklist:

  • Tests
    • (not applicable?)
  • Documentation
    • (not applicable?)

@elemoine elemoine force-pushed the sqlalchemy-connector branch 3 times, most recently from 0bb0f7b to b94c893 Compare September 24, 2021 14:59
@elemoine elemoine force-pushed the sqlalchemy-connector branch 4 times, most recently from e75674b to e38bbaa Compare September 24, 2021 16:17
@@ -0,0 +1,176 @@
import functools
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in contrib ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, could be. I have no problem with moving it to the contrib folder. But do we agree that it's just a folder change, and that contribs have nothing else specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the connector to the contrib.sqlalchemy module.

Comment on lines +38 to +43
django = {version = ">=2.2", optional = true}
sqlalchemy = {version = "^1.4", optional = true}

[tool.poetry.extras]
django = ["django"]
sqlalchemy = ["sqlalchemy"]
Copy link
Member

Choose a reason for hiding this comment

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

woo, new syntax ?

Copy link
Contributor Author

@elemoine elemoine Sep 24, 2021

Choose a reason for hiding this comment

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

I think the previous syntax did not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Proof is that we didn't have to use poetry install --extras django in the CI.

Copy link
Member

Choose a reason for hiding this comment

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

oh. Damn :/

@elemoine elemoine force-pushed the sqlalchemy-connector branch 6 times, most recently from d9dd35b to 0d0f407 Compare September 26, 2021 14:36
@elemoine
Copy link
Contributor Author

@ewjoachim, we get the following in the codecov report, and I don't know what to do to fix the problem:

Current head 17c25ba differs from pull request most recent head 0d0f407. Consider uploading reports for the commit 0d0f407 to get more accurate results

Do you know what we can do about it?

@elemoine elemoine force-pushed the sqlalchemy-connector branch 2 times, most recently from 96599e3 to 5fd82dc Compare September 27, 2021 09:24
@procrastinate-org procrastinate-org deleted a comment from codecov bot Sep 27, 2021
@elemoine elemoine force-pushed the sqlalchemy-connector branch from 5fd82dc to 8d7bc54 Compare September 27, 2021 09:55
@github-actions
Copy link

github-actions bot commented Sep 27, 2021

Coverage report

The coverage rate went from 99.0% to 100.0% ⬆️

The branch rate is 99.72999999999999%

100% of new lines are covered.

@procrastinate-org procrastinate-org deleted a comment from codecov bot Sep 27, 2021
@elemoine elemoine marked this pull request as ready for review September 27, 2021 09:57
@elemoine
Copy link
Contributor Author

This is ready for review again.

@ewjoachim
Copy link
Member

ewjoachim commented Sep 27, 2021

The branch rate is 99.72999999999999%

Woops :D I'll have to fix that in the github action :D

Comment on lines +10 to +11
sqlalchemy>=1.4,<2.0.0
django>=2.2
Copy link
Member

Choose a reason for hiding this comment

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

Can we rather change . at the top of the file to .[django,sqlalchemy] ?

Copy link
Member

Choose a reason for hiding this comment

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

Me, Galaxy brain, makes a comment requesting a change, then merges immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll create a PR for that if you haven't done it already.

Comment on lines +3 to +13
from typing import (
TYPE_CHECKING,
Any,
Callable,
Dict,
Iterable,
List,
Optional,
Set,
Union,
)
Copy link
Member

Choose a reason for hiding this comment

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

Wow, I'm not used to having so many imports from a module they don't fit on a single line :D. Well pixel real estate is cheap and if Black wants to spend a bazillion lines, it may well.

@ewjoachim ewjoachim merged commit 3bcf84f into master Sep 27, 2021
@ewjoachim ewjoachim deleted the sqlalchemy-connector branch September 27, 2021 20:05
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.

Integration with SQLAlchemy
2 participants