You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2023/05/23 08:01:20 UTC
[GitHub] [airflow] potiuk opened a new pull request, #31469: Bring back min-airflow-version for preinstalled providers
potiuk opened a new pull request, #31469:
URL: https://github.com/apache/airflow/pull/31469
In the last wave of providers #31416 we bumped min-airlfow-version to 2.4 and added mechanism to verify min-airflow version is ok while importing, but it turned out that there are cases where installing just old version of airflow (with no constraints) brings the latest version of those providers and causes new installation of airflow to fail. This is far too common to ignore or require to use constraints, unfortunately We do not have min-airlfow-version in the preinstalled providers for one reason only. For some tools that are NOT conforming to standards (such as bazel), having min-airflow-version for those providers causes circular dependency (even though technically dependencies in PyPI can - and often are - circular, because dependencies in Python are not DAG and can contain cycles.
This was in response to this issue in 2021 #17795.
However, this is really bazel issue, and on top of it - it's recognized as such and being fixed very recently
in https://github.com/bazelbuild/rules_python/issues/1188 because there are other packages that have similar problems (pytorch and triton being popular couple). Also Bazel is not that popular in the Python world.
Therefore, rather than trying to workaround the problem of bazel, we encourage them to merge and release the fix
https://github.com/bazelbuild/rules_python/pull/1166#issuecomment-1558690218 and call it out in our installation instructions, that bazel installation might lead to problems like that.
If bazel does not fix it, this will only be a problem for Future installations of airflow in a few months most likely. It will not impact current bazel users installing old versions of Airflow (actually they might start having problems now if we do not fix it and yank the 5 providers released yesterday)
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #31469: Bring back min-airflow-version for preinstalled providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #31469:
URL: https://github.com/apache/airflow/pull/31469#discussion_r1201822475
##########
dev/provider_packages/PROVIDER_INDEX_TEMPLATE.rst.jinja2:
##########
@@ -56,20 +56,6 @@ Requirements
The minimum Apache Airflow version supported by this provider package is ``{{ MIN_AIRFLOW_VERSION }}``.
-{%- if PREINSTALLED_PROVIDER %}
-
-This provider package is preinstalled by default when Apache Airflow is installed. You do not need to
-install it separately. You can upgrade and downgrade it independently of Apache Airflow package though.
-
-.. note::
-
- The minimum Apache Airflow version for this package is {{ MIN_AIRFLOW_VERSION }} and it will fail
- import at runtime if the version of Airflow is lower even if there is no requirement specified in
- the dependencies - this is because the provider is preinstalled and specifying minimum Apache
- Airflow version would create a dependency cycle, which confuses dependency tools.
-
-{%- endif %}
-
Review Comment:
It is part of the release process: https://github.com/apache/airflow/blob/main/dev/README_RELEASE_PROVIDER_PACKAGES.md#generate-release-notes
There Release Manager will regenerate the docs and increase provider versions.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pankajkoti commented on a diff in pull request #31469: Bring back min-airflow-version for preinstalled providers
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #31469:
URL: https://github.com/apache/airflow/pull/31469#discussion_r1201811958
##########
dev/provider_packages/PROVIDER_INDEX_TEMPLATE.rst.jinja2:
##########
@@ -56,20 +56,6 @@ Requirements
The minimum Apache Airflow version supported by this provider package is ``{{ MIN_AIRFLOW_VERSION }}``.
-{%- if PREINSTALLED_PROVIDER %}
-
-This provider package is preinstalled by default when Apache Airflow is installed. You do not need to
-install it separately. You can upgrade and downgrade it independently of Apache Airflow package though.
-
-.. note::
-
- The minimum Apache Airflow version for this package is {{ MIN_AIRFLOW_VERSION }} and it will fail
- import at runtime if the version of Airflow is lower even if there is no requirement specified in
- the dependencies - this is because the provider is preinstalled and specifying minimum Apache
- Airflow version would create a dependency cycle, which confuses dependency tools.
-
-{%- endif %}
-
Review Comment:
Do we need to regenerate the docs to remove this block from the existing docs of those 5 providers? Or it happens (by itself?) as some later step (perhaps during releasing the providers 🤔 )?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pankajkoti commented on a diff in pull request #31469: Bring back min-airflow-version for preinstalled providers
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #31469:
URL: https://github.com/apache/airflow/pull/31469#discussion_r1201811958
##########
dev/provider_packages/PROVIDER_INDEX_TEMPLATE.rst.jinja2:
##########
@@ -56,20 +56,6 @@ Requirements
The minimum Apache Airflow version supported by this provider package is ``{{ MIN_AIRFLOW_VERSION }}``.
-{%- if PREINSTALLED_PROVIDER %}
-
-This provider package is preinstalled by default when Apache Airflow is installed. You do not need to
-install it separately. You can upgrade and downgrade it independently of Apache Airflow package though.
-
-.. note::
-
- The minimum Apache Airflow version for this package is {{ MIN_AIRFLOW_VERSION }} and it will fail
- import at runtime if the version of Airflow is lower even if there is no requirement specified in
- the dependencies - this is because the provider is preinstalled and specifying minimum Apache
- Airflow version would create a dependency cycle, which confuses dependency tools.
-
-{%- endif %}
-
Review Comment:
Do we need to regenerate the docs to remove this block from the existing docs of those 5 providers? Or it happens by itself as some later step?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pankajkoti commented on a diff in pull request #31469: Bring back min-airflow-version for preinstalled providers
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #31469:
URL: https://github.com/apache/airflow/pull/31469#discussion_r1201811958
##########
dev/provider_packages/PROVIDER_INDEX_TEMPLATE.rst.jinja2:
##########
@@ -56,20 +56,6 @@ Requirements
The minimum Apache Airflow version supported by this provider package is ``{{ MIN_AIRFLOW_VERSION }}``.
-{%- if PREINSTALLED_PROVIDER %}
-
-This provider package is preinstalled by default when Apache Airflow is installed. You do not need to
-install it separately. You can upgrade and downgrade it independently of Apache Airflow package though.
-
-.. note::
-
- The minimum Apache Airflow version for this package is {{ MIN_AIRFLOW_VERSION }} and it will fail
- import at runtime if the version of Airflow is lower even if there is no requirement specified in
- the dependencies - this is because the provider is preinstalled and specifying minimum Apache
- Airflow version would create a dependency cycle, which confuses dependency tools.
-
-{%- endif %}
-
Review Comment:
Do we need to regenerate the docs to remove this block from the existing docs of those 5 providers? Or it happens by itself as some later step (perhaps during releasing the providers 🤔 )?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pankajkoti commented on a diff in pull request #31469: Bring back min-airflow-version for preinstalled providers
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #31469:
URL: https://github.com/apache/airflow/pull/31469#discussion_r1201831074
##########
dev/provider_packages/PROVIDER_INDEX_TEMPLATE.rst.jinja2:
##########
@@ -56,20 +56,6 @@ Requirements
The minimum Apache Airflow version supported by this provider package is ``{{ MIN_AIRFLOW_VERSION }}``.
-{%- if PREINSTALLED_PROVIDER %}
-
-This provider package is preinstalled by default when Apache Airflow is installed. You do not need to
-install it separately. You can upgrade and downgrade it independently of Apache Airflow package though.
-
-.. note::
-
- The minimum Apache Airflow version for this package is {{ MIN_AIRFLOW_VERSION }} and it will fail
- import at runtime if the version of Airflow is lower even if there is no requirement specified in
- the dependencies - this is because the provider is preinstalled and specifying minimum Apache
- Airflow version would create a dependency cycle, which confuses dependency tools.
-
-{%- endif %}
-
Review Comment:
understood was trying to find the usage of the script and guessed that & edited my comment but did not refresh the page to see your comment before. Thank you!
##########
dev/provider_packages/PROVIDER_INDEX_TEMPLATE.rst.jinja2:
##########
@@ -56,20 +56,6 @@ Requirements
The minimum Apache Airflow version supported by this provider package is ``{{ MIN_AIRFLOW_VERSION }}``.
-{%- if PREINSTALLED_PROVIDER %}
-
-This provider package is preinstalled by default when Apache Airflow is installed. You do not need to
-install it separately. You can upgrade and downgrade it independently of Apache Airflow package though.
-
-.. note::
-
- The minimum Apache Airflow version for this package is {{ MIN_AIRFLOW_VERSION }} and it will fail
- import at runtime if the version of Airflow is lower even if there is no requirement specified in
- the dependencies - this is because the provider is preinstalled and specifying minimum Apache
- Airflow version would create a dependency cycle, which confuses dependency tools.
-
-{%- endif %}
-
Review Comment:
Understood. I was trying to find the usage of the script and guessed that & edited my comment but did not refresh the page to see your comment before. Thank you!
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #31469: Bring back min-airflow-version for preinstalled providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #31469:
URL: https://github.com/apache/airflow/pull/31469
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org