Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
mohit7705 closed pull request #59940: Add sqlalchemy as optional dependency for Postgres provider URL: https://github.com/apache/airflow/pull/59940 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
mohit7705 commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2668917049 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -27,25 +27,14 @@ import psycopg2.extras from more_itertools import chunked from psycopg2.extras import DictCursor, NamedTupleCursor, RealDictCursor, execute_batch -from sqlalchemy.engine import URL from airflow.exceptions import AirflowOptionalProviderFeatureException from airflow.providers.common.compat.sdk import AirflowException, Connection, conf from airflow.providers.common.sql.hooks.sql import DbApiHook from airflow.providers.postgres.dialects.postgres import PostgresDialect -USE_PSYCOPG3: bool Review Comment: 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
potiuk commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2668906645 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -167,14 +157,23 @@ def __cast_nullable(value, dst_type: type) -> Any: @property def sqlalchemy_url(self) -> URL: +try: +from sqlalchemy.engine import URL Review Comment: This will likely fail with MyPy issues. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
potiuk commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2668904667 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -27,25 +27,14 @@ import psycopg2.extras from more_itertools import chunked from psycopg2.extras import DictCursor, NamedTupleCursor, RealDictCursor, execute_batch -from sqlalchemy.engine import URL from airflow.exceptions import AirflowOptionalProviderFeatureException from airflow.providers.common.compat.sdk import AirflowException, Connection, conf from airflow.providers.common.sql.hooks.sql import DbApiHook from airflow.providers.postgres.dialects.postgres import PostgresDialect -USE_PSYCOPG3: bool Review Comment: Actusally deleting it is wrong. The conditional logic should still be used. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
mohit7705 commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2668810738 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -27,7 +27,12 @@ import psycopg2.extras from more_itertools import chunked from psycopg2.extras import DictCursor, NamedTupleCursor, RealDictCursor, execute_batch -from sqlalchemy.engine import URL + +try: Review Comment: 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
jscheffl commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2666505815 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -27,7 +27,12 @@ import psycopg2.extras from more_itertools import chunked from psycopg2.extras import DictCursor, NamedTupleCursor, RealDictCursor, execute_batch -from sqlalchemy.engine import URL + +try: Review Comment: 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
mohit7705 commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2666487478 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -27,7 +27,12 @@ import psycopg2.extras from more_itertools import chunked from psycopg2.extras import DictCursor, NamedTupleCursor, RealDictCursor, execute_batch -from sqlalchemy.engine import URL + +try: Review Comment: can you verify once again @potiuk -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
potiuk commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2659966558 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -27,7 +27,12 @@ import psycopg2.extras from more_itertools import chunked from psycopg2.extras import DictCursor, NamedTupleCursor, RealDictCursor, execute_batch -from sqlalchemy.engine import URL + +try: Review Comment: It's the type hinting - not setting "Any" `URL: Any` baically. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
potiuk commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2659890342 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -27,7 +27,12 @@ import psycopg2.extras from more_itertools import chunked from psycopg2.extras import DictCursor, NamedTupleCursor, RealDictCursor, execute_batch -from sqlalchemy.engine import URL + +try: Review Comment: Type-hinting will be needed of the URL (making it Any). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
mohit7705 commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2659692775 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -27,7 +27,12 @@ import psycopg2.extras from more_itertools import chunked from psycopg2.extras import DictCursor, NamedTupleCursor, RealDictCursor, execute_batch -from sqlalchemy.engine import URL + +try: +from sqlalchemy.engine import URL +except ImportError: +URL = None Review Comment: Applied the suggested typing change for the optional SQLAlchemy URL. Thanks for the review! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
potiuk commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2659685141 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -27,7 +27,12 @@ import psycopg2.extras from more_itertools import chunked from psycopg2.extras import DictCursor, NamedTupleCursor, RealDictCursor, execute_batch -from sqlalchemy.engine import URL + +try: +from sqlalchemy.engine import URL +except ImportError: +URL = None Review Comment: Right right -> I will leave it to the author then :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
Dev-iL commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2659679686 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -27,7 +27,12 @@ import psycopg2.extras from more_itertools import chunked from psycopg2.extras import DictCursor, NamedTupleCursor, RealDictCursor, execute_batch -from sqlalchemy.engine import URL + +try: +from sqlalchemy.engine import URL +except ImportError: +URL = None Review Comment: @potiuk if you want to hint with `Any`, imho it's better to do it before the `try`, i.e. ```python URL: Any try: from sqlalchemy.engine import URL except ImportError: URL = None ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
potiuk commented on PR #59940: URL: https://github.com/apache/airflow/pull/59940#issuecomment-3708102285 Indeed, fixing the other static checks needed -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
potiuk commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2659678296 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -27,7 +27,12 @@ import psycopg2.extras from more_itertools import chunked from psycopg2.extras import DictCursor, NamedTupleCursor, RealDictCursor, execute_batch -from sqlalchemy.engine import URL + +try: +from sqlalchemy.engine import URL +except ImportError: +URL = None Review Comment: ```suggestion URL: Any = None ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
Dev-iL commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2659546114 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -27,7 +27,11 @@ import psycopg2.extras from more_itertools import chunked from psycopg2.extras import DictCursor, NamedTupleCursor, RealDictCursor, execute_batch -from sqlalchemy.engine import URL +try: +from sqlalchemy.engine import URL +except ImportError: +URL = None Review Comment: https://github.com/apache/airflow/pull/59941#discussion_r2659530876 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
Srabasti commented on PR #59940: URL: https://github.com/apache/airflow/pull/59940#issuecomment-3707590820 Thanks for including the changes for Line 92 @mohit7705! However, looks like static checks are failing as below. https://github.com/user-attachments/assets/78b3d6db-7723-4602-a713-8e92f12322f9"; /> As Jens and Jarek advised above, have you reproduced locally by running `prek run --all-files` in your branch? Prek will fix any formatting errors, and then you can push the commit from your branch. To run prek as part of git workflow, use `prek install` to set up git hooks. https://github.com/user-attachments/assets/d6b99971-f5f7-46f6-950f-31084bd7241c"; /> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
potiuk commented on PR #59940: URL: https://github.com/apache/airflow/pull/59940#issuecomment-3703054723 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
jscheffl commented on PR #59940: URL: https://github.com/apache/airflow/pull/59940#issuecomment-3702981362 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
mohit7705 commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2655876605 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -389,8 +400,17 @@ def get_uri(self) -> str: :return: the extracted URI in Sqlalchemy URI format. """ +try: + import sqlalchemy # noqa: F401 Review Comment: can you check once again. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
mohit7705 commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2655861395 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -389,8 +400,17 @@ def get_uri(self) -> str: :return: the extracted URI in Sqlalchemy URI format. """ +try: + import sqlalchemy # noqa: F401 Review Comment: i have made the correction. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
jscheffl commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2655853850 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -389,8 +400,17 @@ def get_uri(self) -> str: :return: the extracted URI in Sqlalchemy URI format. """ +try: + import sqlalchemy # noqa: F401 Review Comment: Why not checking like above? `if URL is None:` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
mohit7705 commented on PR #59940: URL: https://github.com/apache/airflow/pull/59940#issuecomment-3702816032 Thanks for the review! I’ve fixed the indentation issue and updated the branch with latest main. Could you please take another look when you have time? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
mohit7705 commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2655818972 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -389,8 +400,14 @@ def get_uri(self) -> str: :return: the extracted URI in Sqlalchemy URI format. """ +if URL is None: + raise AirflowOptionalProviderFeatureException( +"The 'sqlalchemy' library is required to render the connection URI." +) Review Comment: Fixed the indentation — thanks for catching that 👍 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
jscheffl commented on code in PR #59940: URL: https://github.com/apache/airflow/pull/59940#discussion_r2655797433 ## providers/postgres/src/airflow/providers/postgres/hooks/postgres.py: ## @@ -389,8 +400,14 @@ def get_uri(self) -> str: :return: the extracted URI in Sqlalchemy URI format. """ +if URL is None: + raise AirflowOptionalProviderFeatureException( +"The 'sqlalchemy' library is required to render the connection URI." +) Review Comment: Whitespace is not correct here. Can you please fix the indent? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
mohit7705 commented on PR #59940: URL: https://github.com/apache/airflow/pull/59940#issuecomment-3702633434 Thanks for the guidance! I’ve aligned the Postgres provider changes with the conditional import pattern and optional SQLAlchemy handling used in sibling provider PRs. Please let me know if you’d like me to adjust anything further. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
potiuk commented on PR #59940: URL: https://github.com/apache/airflow/pull/59940#issuecomment-3700920301 It's quite a bit more than that - see at other PRs in sibling issues (conditional handling of imports and sqlalchemy version difference) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add sqlalchemy as optional dependency for Postgres provider [airflow]
boring-cyborg[bot] commented on PR #59940: URL: https://github.com/apache/airflow/pull/59940#issuecomment-3700047289 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) Here are some useful points: - Pay attention to the quality of your code (ruff, mypy and type annotations). Our [prek-hooks]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-prek-hooks) will help you with that. - In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/airflow-core/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it. - Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations. - Be patient and persistent. It might take some time to get a review or get the final approval from Committers. - Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack. - Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices). - Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits. Apache Airflow is a community-driven project and together we are making it better 🚀. In case of doubts contact the developers at: Mailing List: [email protected] Slack: https://s.apache.org/airflow-slack -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
