You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/07/26 11:32:41 UTC
[GitHub] [airflow] potiuk opened a new pull request, #25306: Strip limits when constructing devel-all
potiuk opened a new pull request, #25306:
URL: https://github.com/apache/airflow/pull/25306
When we are constructing devel-all, we should strip the limits
to account for some shared packages that are not yet released.
Example is a common-sql package that might be limited to >1.1.0
for example as part of the change, but it might not yet be released.
It does not impact our CI, because we are anyhow removing the packages
after installing (we only care about dependencies of their) and we
are using source version of providers - but having a limit coming
from another provider will make `pip` resolver fail if we do not
strip the limits.
<!--
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+Improvements+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 #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r931339462
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
The current version (with limit removal) is also running in #24836 to double check if it works (but I tested it locally and it worked).
--
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 #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r930934207
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
cc: @uranusjr - somethng I missed?
--
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 #25306: Add pre-release limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r932061081
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
Yeah. That woudl explain it. Just checked it and indeed - simply replacing the suffix with the one we are just building with is good. Update coming in a minute.
So really the problem (might not be a "bug" ) with `pip` is that `--pre` does not apply to limit of the dependecies of the packages you are installing - just to the packages that you are installing. Would that be the right "assesment" ?
Do you think is it something that could be changed easiy and accepted? Maybe I could even attempt to find and fix it, but I am a little bit sceptical after the rejections (and the form of those rejections especially) I got before, so I am a little bit sceptical about submitting anything like that without knowing if it is something that maybe has been discussed in the past or maybe there is a long discussion somewhere I am not aware of.
--
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] uranusjr commented on a diff in pull request #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r930588761
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
This is not a bug but in the Python version range specification. A better “fix” would be to conditionally supply `--pre` when pip is invoked.
--
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 #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r930933847
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
I tried to use pre (I will have a nice feature added to docker files out of that), but I am afraid this is indeed the bug and I remembered our earlier discussion well. This is what happened when I tried to install the providers:
```
pip install --find-links=file:///docker-context-files
--root-user-action ignore --upgrade --upgrade-strategy eager
--pre 'apache-airflow[amazon,async,celery,cncf.kubernetes,dask,docker,elasticsearch,ftp,google,google_auth,grpc,hashicorp,http,ldap,microsoft.azure,mysql,odbc,pandas,postgres,redis,sendgrid,sftp,slack,ssh,statsd,virtualenv]==2.4.0.dev0'
/docker-context-files/apache_airflow_providers_amazon-4.1.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_celery-3.0.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_cncf_kubernetes-4.2.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_common_sql-1.1.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_docker-3.1.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_elasticsearch-4.1.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_ftp-3.1.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_google-8.2.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_grpc-3.0.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_hashicorp-3.0.1.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_http-4.0.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_imap-3.0.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_microsoft_azure-4.1.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_mysql-3.1.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_odbc-3.1.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_postgres-5.1.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_redis-3.0.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_sendgrid-3.0.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_sftp-4.0.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_slack-5.1.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_sqlite-3.1.0.dev0-py3-none-any.whl
/docker-context-files/apache_airflow_providers_ssh-3.1.0.dev0-py3-none-any.whl 'dill<0.3.3'
```
This fails with:
```
#56 34.54 ERROR: Cannot install apache-airflow-providers-amazon==4.1.0.dev0, apache-airflow-providers-common-sql 1.1.0.dev0 (from /docker-context-files/apache_airflow_providers_common_sql-1.1.0.dev0-py3-none-any.whl), apache-airflow-providers-elasticsearch==4.1.0.dev0, apache-airflow-providers-google==8.2.0.dev0 and apache-airflow[amazon,async,celery,cncf-kubernetes,dask,docker,elasticsearch,ftp,google,google-auth,grpc,hashicorp,http,ldap,microsoft-azure,mysql,odbc,pandas,postgres,redis,sendgrid,sftp,slack,ssh,statsd,virtualenv]==2.4.0.dev0 because these package versions have conflicting dependencies.
#56 34.54
#56 34.54 The conflict is caused by:
#56 34.54 The user requested apache-airflow-providers-common-sql 1.1.0.dev0 (from /docker-context-files/apache_airflow_providers_common_sql-1.1.0.dev0-py3-none-any.whl)
#56 34.54 apache-airflow[amazon,async,celery,cncf-kubernetes,dask,docker,elasticsearch,ftp,google,google-auth,grpc,hashicorp,http,ldap,microsoft-azure,mysql,odbc,pandas,postgres,redis,sendgrid,sftp,slack,ssh,statsd,virtualenv] 2.4.0.dev0 depends on apache-airflow-providers-common-sql
#56 34.54 apache-airflow-providers-amazon 4.1.0.dev0 depends on apache-airflow-providers-common-sql
#56 34.54 apache-airflow-providers-elasticsearch 4.1.0.dev0 depends on apache-airflow-providers-common-sql
#56 34.54 apache-airflow-providers-google 8.2.0.dev0 depends on apache-airflow-providers-common-sql>=1.1.0
#56 34.54
#56 34.54 To fix this you could try to:
#56 34.54 1. loosen the range of package versions you've specified
#56 34.54 2. remove package versions to allow pip attempt to solve the dependency conflict
#56 34.54
#56 34.54 ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/topics/dependency-resolution/#dealing-with-dependency-conflicts
```
The problem is that
* apache-airflow-providers-google 8.2.0.dev0 depends on apache-airflow-providers-common-sql>=1.1.0
* The user requested apache-airflow-providers-common-sql 1.1.0.dev0 (from /docker-context-files/apache_airflow_providers_common_sql-1.1.0.dev0-py3-none-any.whl
I was using ``--pre`` flag (as you can see above).
Still google's > 1.1.0 conflicts with 1.1.0dev0 - which is the problem
--
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 #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r931324336
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
Actually even setting limits to 1.1.0.a0 does not work:
```
56 28.82 The user requested apache-airflow-providers-common-sql 1.1.0.dev0 (from /docker-context-files/apache_airflow_providers_common_sql-1.1.0.dev0-py3-none-any.whl)
#56 28.82 apache-airflow[amazon,async,celery,cncf-kubernetes,dask,docker,elasticsearch,ftp,google,google-auth,grpc,hashicorp,http,ldap,microsoft-azure,mysql,odbc,pandas,postgres,redis,sendgrid,sftp,slack,ssh,statsd,virtualenv] 2.4.0.dev0 depends on apache-airflow-providers-common-sql
#56 28.82 apache-airflow-providers-amazon 4.1.0.dev0 depends on apache-airflow-providers-common-sql
#56 28.82 apache-airflow-providers-elasticsearch 4.1.0.dev0 depends on apache-airflow-providers-common-sql
#56 28.82 apache-airflow-providers-google 8.2.0.dev0 depends on apache-airflow-providers-common-sql>=1.1.0.a0
#56 28.82
```
--
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] uranusjr commented on a diff in pull request #25306: Add pre-release limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r932063740
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
I think `--pre` does apply to transitive dependencies, but it’s not possible to tell without a reproducible with much less moving 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-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 #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r932021622
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
Yeah, I will experiment a bit with it. I do not like to remove the limits altogether. Still it's a bit surprising it did not work when I put `>=1.1.0.a0` -> I would imagine ".dev0" is higher than ".a0".
BTW. This is a bit experimental with `common.sql`, in the way that we have a chance now to test how this case works - but we need to solve it 'well' for the future - especially if we seriously think about splitting google provider - this will introduce even more complex relations as there will likely be a 'google.common' or even 'alphabet,gcp.common` provider that other google providers will depend on and it will be quite often that we will have to released those together.
And we need to give a chance for our users to be able to test rc's when we release them as well.
I am not sure if that one is because we are using wheel packages from local folders only? Will that work the same when we release rc's in PyPI?. That will make it a bit harder to test if our - in this case - `google8.1.0rc1 has >= common.sql.1.1.0rc1 requirement" - will the user get the rc1 of common sql installed (even if using pre?).
--
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 #25306: Add pre-release limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r932064617
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
Or is `--pre` only valid for pre-releases and we need `--dev` as well? (Just reading PEP 440 and it seems to me that `--pre` would only apply to a/b/rc ones but not dev ones ?
--
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 #25306: Add pre-release limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r932064617
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
Or is `--pre` only valid for pre-releases and we need `--dev` as well? (Just reading PEP 440 and it seems to me that `--pre` would only apply to a/b ones but not dev ones ?
--
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 pull request #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25306:
URL: https://github.com/apache/airflow/pull/25306#issuecomment-1195365538
Extracted from #24836
--
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 #25306: Add pre-release limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r932068377
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
Though the other part of PEP mentions ("Pre-releases of any kind, including developmental releases") - so I am not sure any more whether `--pre`'s pre-release also includes dev or not :)
--
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 #25306: Add pre-release limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r932079700
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
Actually after further reading of the PEP, I tested one more approach. It's not explicitly mentioned in the PEP, but apparently it works.
Seems that converting the "pre-release" depencies to have ".*" at the end even in >= would include all kinds of pre-releases, so at least we have a way to specify "all pre-releases including development ones" in our dependencies. So `apache-airflow-providers-common-sql>=1.1.0.*` works in this case and that's what I am going to settle with I think.
This is the part of meta-data fron .whl in the Google Provider that works:
```
Requires-Dist: PyOpenSSL
Requires-Dist: apache-airflow-providers-common-sql (>=1.1.0.*)
Requires-Dist: apache-airflow (>=2.2.0.*)
Requires-Dist: google-ads (>=15.1.1)
```
I think that's sustainable for our case (but we have to manually modify the requirements). Would be great to add this behaviour to `--pre` flag I guess, but we can definitely live with that even if it is not added.
--
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 #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r930873021
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
Sorry - i was 100% sure we discussed it before and that was the outcome :)
--
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 #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r930872679
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
Ah. I thought you mentioned before that this was a long standing bug that you never got around to fix, but I must have been mistaken. If `--pre` works this way, then it is indeed a much better fix, so let me change it
--
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 closed pull request #25306: Add pre-release limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #25306: Add pre-release limits when constructing devel-all
URL: https://github.com/apache/airflow/pull/25306
--
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 #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r931014868
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
BTW. The possibility of adding `--pre` flag (or any other) added here: https://github.com/apache/airflow/pull/25337 (but as above, it does not seem to solve the problem).
--
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] uranusjr commented on a diff in pull request #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r932030479
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
`dev0` not higher than `a0` (or lower in the same sense) because it’s actually a separate segment altogether. In PEP 440, a `dev0` part belongs to the development segment, while `a0` belongs to the pre-release segment. So adding `a0` to the range specifier does not help identifying dev releases, but `dev0` can.
https://peps.python.org/pep-0440/#pre-releases
--
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 #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r930872679
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
Ah. I thought you mentioned before that this was a long standing bug that you never got around to fix, but I must have been mistaken. If -pre works this way, then it is indeed a much better fix, so let me change it
--
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] uranusjr commented on a diff in pull request #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r931933573
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
I think the problem is that 1.1.0 prereleases are earlier 1.1.0 (because prereleases are logically before the stable one). Adding `dev0` instead would probably work (and is probably an appropriate solution for Airflow in this use case, since we are kind of intentionally breaking the version range for testing).
--
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 #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r930934207
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
cc: @uranusjr - something I missed?
--
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] uranusjr commented on a diff in pull request #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r931933573
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
I think the problem is that 1.1.0 prereleases are “earlier” than 1.1.0 (because prereleases are logically before the stable one). Adding `dev0` instead would probably work (and is probably an appropriate solution for Airflow in this use case, since we are kind of intentionally breaking the version range for testing).
--
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] uranusjr commented on a diff in pull request #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r932030479
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
`dev0` not higher than `a0` (or lower in the same sense) because it’s actually a separate segment altogether. According to [PEP 440](https://peps.python.org/pep-0440/), a `dev0` part belongs to the development segment, while `a0` belongs to the pre-release segment. So adding `a0` to the range specifier does not help identifying dev releases, but `dev0` can.
--
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 #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25306:
URL: https://github.com/apache/airflow/pull/25306#discussion_r931338663
##########
dev/provider_packages/prepare_provider_packages.py:
##########
@@ -314,7 +314,20 @@ def get_install_requirements(provider_package_id: str, version_suffix: str) -> s
:return: install requirements of the package
"""
- install_requires = ALL_DEPENDENCIES[provider_package_id][DEPS]
+
+ def apply_version_suffix(install_clause: str) -> str:
+ if install_clause.startswith("apache-airflow") and ">=" in install_clause and version_suffix != "":
+ # This is workaround for `pip` bug. When you specify dependency as >= X.Y.Z, and you
+ # Have packages X.Y.Zdev0 or X.Y.Zrc1, the "=" part does not include the pre-release versions
+ # you need to explicitly specify >= X.Y.Za0 to include pre-release versions.
+ # Therefore, when we are preparing pre-releases we always add a0 for airflow and other providers
+ # to Allow installing two packages released in rc* that depend on each-other
Review Comment:
The only way I could make it work @uranusjr was to remove the limits altogether. I am not sure if there is another 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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25306: Strip limits when constructing devel-all
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25306:
URL: https://github.com/apache/airflow/pull/25306#issuecomment-1195437088
BTW. Those are exactly the kind of finding I expected to test with common.sql extraction @eladkal @jedcunningham - nice test ground to see all the "interesting" cases when we have providers depending on specific versions of other 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