-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Add sqlalchemy as optional dependency for Postgres provider #59940
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 sqlalchemy as optional dependency for Postgres provider #59940
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
|
It's quite a bit more than that - see at other PRs in sibling issues (conditional handling of imports and sqlalchemy version difference) |
|
Thanks for the guidance! |
providers/postgres/src/airflow/providers/postgres/hooks/postgres.py
Outdated
Show resolved
Hide resolved
|
Thanks for the review! |
providers/postgres/src/airflow/providers/postgres/hooks/postgres.py
Outdated
Show resolved
Hide resolved
|
Can you please - without just shooting code as a PR - run basic tests locally via prek? Basic indent is not fitting again and again. All would be fixed with local checks before pushing. |
|
Yes. It adds a lot of overhead if you dont follow basic contributing guides - this makes us loose more time to review and react to it than if we did it ourselves, and you do not seem to be self-reliant on fixing even those issues that are fixable automatically by prek Not very helpful |
Srabasti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 92 should be referencing "sqlalchemy>=1.4.49" instead of "sqlalchemy>=1.4".
Happy New Year!
|
Thanks for including the changes for Line 92 @mohit7705! However, looks like static checks are failing as below. As Jens and Jarek advised above, have you reproduced locally by running To run prek as part of git workflow, use |
providers/postgres/src/airflow/providers/postgres/hooks/postgres.py
Outdated
Show resolved
Hide resolved
cf27cf4 to
e280e2c
Compare
providers/postgres/src/airflow/providers/postgres/hooks/postgres.py
Outdated
Show resolved
Hide resolved
|
Indeed, fixing the other static checks needed |
109bef9 to
07d41e9
Compare
| from psycopg2.extras import DictCursor, NamedTupleCursor, RealDictCursor, execute_batch | ||
| from sqlalchemy.engine import URL | ||
|
|
||
| try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type-hinting will be needed of the URL (making it Any).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the type hinting - not setting "Any"
URL: Any baically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you verify once again @potiuk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can take a look here where corrections were made: https://github.com/apache/airflow/pull/60094/changes#diff-d2d44a709cc687a2a6c390995289cba7689683c39519b81bab4bdd50f00c9bafR28
One other option (which I'd prefer actually) is also making it with lazy imports like in: https://github.com/apache/airflow/pull/60110/changes#diff-60967e11945bf5adce44c15269d657d1ff09ed8916f91e82ba1cf1beddf1e9a7R183
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification, this makes sense 👍
I see the earlier approach and the alternative with lazy imports you linked. I’m happy to align with the preferred direction — please let me know if you’d like me to update this PR to use lazy imports instead, or if the current approach is acceptable as-is.
| from airflow.providers.common.sql.hooks.sql import DbApiHook | ||
| from airflow.providers.postgres.dialects.postgres import PostgresDialect | ||
|
|
||
| USE_PSYCOPG3: bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actusally deleting it is wrong. The conditional logic should still be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification — that makes sense 👍
I’ll restore the conditional logic and adjust the SQLAlchemy import to be MyPy-safe while keeping it optional.
I’ll push an updated commit shortly.
| @property | ||
| def sqlalchemy_url(self) -> URL: | ||
| try: | ||
| from sqlalchemy.engine import URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will likely fail with MyPy issues.


Closes #59905
What does this PR do?
Adds SQLAlchemy as an optional dependency for the Postgres provider.
Why is this needed?
This aligns the Postgres provider with task isolation requirements, where SQLAlchemy should not be a mandatory dependency unless explicitly required.