Re: [PR] changing import path to airflow.sdk [airflow]
vincbeck merged PR #50659: URL: https://github.com/apache/airflow/pull/50659 -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
Bowrna closed pull request #50659: changing import path to airflow.sdk URL: https://github.com/apache/airflow/pull/50659 -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
Bowrna commented on PR #50659: URL: https://github.com/apache/airflow/pull/50659#issuecomment-3019715149 Could this PR be verfied and merged? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
jscheffl commented on PR #50659: URL: https://github.com/apache/airflow/pull/50659#issuecomment-3006217395 > @eladkal @amoghrajesh @o-nikolas @jscheffl When you get time, could you verify this PR and share your feedback? I am not an expert of the Amazon provider, I'd leave the review rather for others. I just stumbled over it and left some comments. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
Bowrna commented on code in PR #50659: URL: https://github.com/apache/airflow/pull/50659#discussion_r2167094560 ## providers/amazon/tests/system/amazon/aws/example_appflow.py: ## @@ -29,6 +31,19 @@ ) from airflow.providers.standard.operators.bash import BashOperator +if TYPE_CHECKING: +from airflow.models.baseoperator import chain +from airflow.models.dag import DAG +else: +if packaging.version.parse( Review Comment: Thanks for pointing this one @potiuk . Let me fix it and raise and updated PR. ## providers/amazon/tests/system/amazon/aws/example_appflow.py: ## @@ -29,6 +31,19 @@ ) from airflow.providers.standard.operators.bash import BashOperator +if TYPE_CHECKING: +from airflow.models.baseoperator import chain +from airflow.models.dag import DAG +else: +if packaging.version.parse( Review Comment: Thanks for pointing this one @potiuk . Let me fix it and raise an updated PR. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
potiuk commented on code in PR #50659: URL: https://github.com/apache/airflow/pull/50659#discussion_r2166507585 ## providers/amazon/tests/system/amazon/aws/example_appflow.py: ## @@ -29,6 +31,19 @@ ) from airflow.providers.standard.operators.bash import BashOperator +if TYPE_CHECKING: +from airflow.models.baseoperator import chain +from airflow.models.dag import DAG +else: +if packaging.version.parse( Review Comment: Also 2.10.0 here is not good. `task.sdk` can only be installed on Airflow 3+ and we already have 2.11 - so if `V3_0_PLUS` is a better condition for using `task.sdk` -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
potiuk commented on code in PR #50659: URL: https://github.com/apache/airflow/pull/50659#discussion_r2166504805 ## providers/amazon/tests/system/amazon/aws/example_appflow.py: ## @@ -29,6 +31,19 @@ ) from airflow.providers.standard.operators.bash import BashOperator +if TYPE_CHECKING: +from airflow.models.baseoperator import chain +from airflow.models.dag import DAG +else: +if packaging.version.parse( Review Comment: This should likely be extracted out to `version_compat.py` that you should place in the `system.amazon` pacjage and imported as ``` from system.amazon.version_compat import AIRFLOW_V_3_0_PLUS ``` This is the convention we used in a number of places. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
Bowrna commented on PR #50659: URL: https://github.com/apache/airflow/pull/50659#issuecomment-3003352375 @eladkal @amoghrajesh @o-nikolas @jscheffl When you get time, could you verify this PR and share your feedback? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
ramitkataria commented on PR #50659: URL: https://github.com/apache/airflow/pull/50659#issuecomment-2993227090 @Bowrna Tests are complete and there were no additional failures so we should be good to merge! -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
ramitkataria commented on PR #50659: URL: https://github.com/apache/airflow/pull/50659#issuecomment-2989144588 > > Also, could you please update the branch with main? There have been changes recently that fixed most of the tests > > @ramitkataria This is done. Thanks, I'll run the tests again and let you know -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
Bowrna commented on PR #50659: URL: https://github.com/apache/airflow/pull/50659#issuecomment-2983107379 > Also, could you please update the branch with main? There have been changes recently that fixed most of the tests @ramitkataria This is done. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
ramitkataria commented on PR #50659: URL: https://github.com/apache/airflow/pull/50659#issuecomment-2923209024 Also, could you please update the branch with main? There have been changes recently that fixed most of the tests -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
jscheffl commented on PR #50659: URL: https://github.com/apache/airflow/pull/50659#issuecomment-2913670350 > > I am not sure whether this PR is a good idea. When this is merged you basically break compatibility with Airflow 2.10/2.11 > > Hi @jscheffl , like suggested by @amoghrajesh could i add support for compatibility in import and raise an updated PR. will that work fine? I'd leave it up to you. I just wanted to warn that it breaks compatability. But you can also leave the imports as-is until compatability is dropped. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
Bowrna commented on PR #50659: URL: https://github.com/apache/airflow/pull/50659#issuecomment-2912787029 > I am not sure whether this PR is a good idea. When this is merged you basically break compatibility with Airflow 2.10/2.11 Hi @jscheffl , like suggested by @amoghrajesh could i add support for compatibility in import and raise an updated PR. will that work fine? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
jscheffl commented on PR #50659: URL: https://github.com/apache/airflow/pull/50659#issuecomment-2910626834 I am not sure whether this PR is a good idea. When this is merged you basically break compatibility with Airflow 2.10/2.11 -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
Bowrna commented on PR #50659: URL: https://github.com/apache/airflow/pull/50659#issuecomment-2900174100 Sure @ramitkataria, please let me know if I can support you in any way. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
ramitkataria commented on PR #50659: URL: https://github.com/apache/airflow/pull/50659#issuecomment-2899583850 @Bowrna I ran this through the system tests infra on local executor and most of the tests are passing but there are a few failures, probably not related to this change (there are already some issues in system tests that we're working on fixing). I can't test on remote executor such as ECS executor right now because all the tests are already failing. I don't think this change should cause issues but just to be safe, maybe we should wait until we fix the existing 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
Bowrna commented on code in PR #50659: URL: https://github.com/apache/airflow/pull/50659#discussion_r2095481460 ## providers/amazon/tests/system/amazon/aws/example_appflow.py: ## @@ -18,8 +18,9 @@ from datetime import datetime -from airflow.models.baseoperator import chain -from airflow.models.dag import DAG +# from airflow.models.baseoperator import chain +# from airflow.models.dag import DAG Review Comment: @amoghrajesh ok i will leave this import without supporting compatibility. @o-nikolas I have removed the commented parts. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
Bowrna commented on PR #50659: URL: https://github.com/apache/airflow/pull/50659#issuecomment-2890632411 @ramitkataria, please let me know if this PR works fine with the system tests infra. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
amoghrajesh commented on code in PR #50659: URL: https://github.com/apache/airflow/pull/50659#discussion_r2093948261 ## providers/amazon/tests/system/amazon/aws/example_appflow.py: ## @@ -18,8 +18,9 @@ from datetime import datetime -from airflow.models.baseoperator import chain -from airflow.models.dag import DAG +# from airflow.models.baseoperator import chain +# from airflow.models.dag import DAG Review Comment: AH ok. Shouldnt have to do it in those dags as log as we do not run compat tests with those dags -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
Bowrna commented on code in PR #50659: URL: https://github.com/apache/airflow/pull/50659#discussion_r2093348372 ## providers/amazon/tests/system/amazon/aws/example_appflow.py: ## @@ -18,8 +18,9 @@ from datetime import datetime -from airflow.models.baseoperator import chain -from airflow.models.dag import DAG +# from airflow.models.baseoperator import chain +# from airflow.models.dag import DAG Review Comment: @amoghrajesh https://github.com/apache/airflow/blob/6728656e9166a10e5d4104fdcf568871be039e91/providers/edge3/src/airflow/providers/edge3/example_dags/integration_test.py#L33-L46 I found this specific example where they have handled for compatibility. Can I add it similarly for this example dags too? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
Bowrna commented on code in PR #50659: URL: https://github.com/apache/airflow/pull/50659#discussion_r2093312490 ## providers/amazon/tests/system/amazon/aws/example_appflow.py: ## @@ -18,8 +18,9 @@ from datetime import datetime -from airflow.models.baseoperator import chain -from airflow.models.dag import DAG +# from airflow.models.baseoperator import chain +# from airflow.models.dag import DAG Review Comment: This is an example DAG, and should we support versions below 3.0? I am having this question because the other example dags in the airflow support only latest version and that's why I wanted to know if we have to include support for older version. @amoghrajesh -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] changing import path to airflow.sdk [airflow]
Bowrna commented on code in PR #50659: URL: https://github.com/apache/airflow/pull/50659#discussion_r2093313358 ## providers/amazon/tests/system/amazon/aws/example_appflow.py: ## @@ -18,8 +18,9 @@ from datetime import datetime -from airflow.models.baseoperator import chain -from airflow.models.dag import DAG +# from airflow.models.baseoperator import chain +# from airflow.models.dag import DAG Review Comment: @o-nikolas I will remove the commented out places once I fixed all issues in the CI/CD pipeline. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
