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 2021/06/24 10:51:58 UTC
[GitHub] [airflow] uranusjr opened a new pull request #16630: Remove SQLAlchemy <1.4 constraint
uranusjr opened a new pull request #16630:
URL: https://github.com/apache/airflow/pull/16630
This was added due to flask-sqlalchemy and sqlalchemy-utils not declaring the upper bounds. They have since released sqlalchemy 1.4-compatible versions, so we can remove that hack.
Note that this does *not* actually make we run on sqlalchemy 1.4 since flask-appbuilder still have a <1.4 pin. But that's for flask-appbuilder to worry about—code in Airflow is compatible, so we can remove the constraint now, and get sqlalchemy 1.4 as soon as flask-appbuilder allows us to.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #16630: Remove SQLAlchemy <1.4 constraint
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16630:
URL: https://github.com/apache/airflow/pull/16630#issuecomment-867539785
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #16630: Remove SQLAlchemy <1.4 constraint
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #16630:
URL: https://github.com/apache/airflow/pull/16630
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16630: Remove SQLAlchemy <1.4 constraint
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16630:
URL: https://github.com/apache/airflow/pull/16630#discussion_r657932570
##########
File path: setup.cfg
##########
@@ -149,8 +149,7 @@ install_requires =
pyyaml>=5.1
rich>=9.2.0
setproctitle>=1.1.8, <2
- # SQLAlchemy 1.4 breaks sqlalchemy-utils https://github.com/kvesteri/sqlalchemy-utils/issues/505
- sqlalchemy>=1.3.18, <1.4
+ sqlalchemy>=1.3.18
Review comment:
I'd generally say don't bump minimum unless we know we need that fix.
As a library we want to be as permissive as we can with the _requirements_.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16630: Remove SQLAlchemy <1.4 constraint
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16630:
URL: https://github.com/apache/airflow/pull/16630#discussion_r657935521
##########
File path: setup.cfg
##########
@@ -149,8 +149,7 @@ install_requires =
pyyaml>=5.1
rich>=9.2.0
setproctitle>=1.1.8, <2
- # SQLAlchemy 1.4 breaks sqlalchemy-utils https://github.com/kvesteri/sqlalchemy-utils/issues/505
- sqlalchemy>=1.3.18, <1.4
+ sqlalchemy>=1.3.18
Review comment:
I don’t have a preference, in practice the difference is minimal. The only behavioural difference I can think of is if the user `pip install --upgrade airflow` without specifying the constraints, but from I know most users don’t upgrade an existing setup anyway...
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #16630: Remove SQLAlchemy <1.4 constraint
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #16630:
URL: https://github.com/apache/airflow/pull/16630#discussion_r657926149
##########
File path: setup.cfg
##########
@@ -149,8 +149,7 @@ install_requires =
pyyaml>=5.1
rich>=9.2.0
setproctitle>=1.1.8, <2
- # SQLAlchemy 1.4 breaks sqlalchemy-utils https://github.com/kvesteri/sqlalchemy-utils/issues/505
- sqlalchemy>=1.3.18, <1.4
+ sqlalchemy>=1.3.18
Review comment:
WDYT about bumping it to 1.3.24 ? last version in 1.3 series?
```suggestion
sqlalchemy>=1.3.24
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16630: Remove SQLAlchemy <1.4 constraint
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16630:
URL: https://github.com/apache/airflow/pull/16630#discussion_r657933585
##########
File path: setup.cfg
##########
@@ -149,8 +149,7 @@ install_requires =
pyyaml>=5.1
rich>=9.2.0
setproctitle>=1.1.8, <2
- # SQLAlchemy 1.4 breaks sqlalchemy-utils https://github.com/kvesteri/sqlalchemy-utils/issues/505
- sqlalchemy>=1.3.18, <1.4
+ sqlalchemy>=1.3.18
Review comment:
Exactly 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #16630: Remove SQLAlchemy <1.4 constraint
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #16630:
URL: https://github.com/apache/airflow/pull/16630#discussion_r657931447
##########
File path: setup.cfg
##########
@@ -149,8 +149,7 @@ install_requires =
pyyaml>=5.1
rich>=9.2.0
setproctitle>=1.1.8, <2
- # SQLAlchemy 1.4 breaks sqlalchemy-utils https://github.com/kvesteri/sqlalchemy-utils/issues/505
- sqlalchemy>=1.3.18, <1.4
+ sqlalchemy>=1.3.18
Review comment:
If someone has 1.3.19 installed, this won't bump it up as it satisfies the version. Like you said not really needed, but hey a good number of bug fixes between 1.3.18 and 1.3.24: https://docs.sqlalchemy.org/en/14/changelog/changelog_13.html
and we already use 1.3.24 in constraints :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #16630: Remove SQLAlchemy <1.4 constraint
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #16630:
URL: https://github.com/apache/airflow/pull/16630#discussion_r657932668
##########
File path: setup.cfg
##########
@@ -149,8 +149,7 @@ install_requires =
pyyaml>=5.1
rich>=9.2.0
setproctitle>=1.1.8, <2
- # SQLAlchemy 1.4 breaks sqlalchemy-utils https://github.com/kvesteri/sqlalchemy-utils/issues/505
- sqlalchemy>=1.3.18, <1.4
+ sqlalchemy>=1.3.18
Review comment:
Actually nevermind, lower has an advantage to cause less conflicts with other dependencies that depend on an older version of `sqlalchemy`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16630: Remove SQLAlchemy <1.4 constraint
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16630:
URL: https://github.com/apache/airflow/pull/16630#discussion_r657928052
##########
File path: setup.cfg
##########
@@ -149,8 +149,7 @@ install_requires =
pyyaml>=5.1
rich>=9.2.0
setproctitle>=1.1.8, <2
- # SQLAlchemy 1.4 breaks sqlalchemy-utils https://github.com/kvesteri/sqlalchemy-utils/issues/505
- sqlalchemy>=1.3.18, <1.4
+ sqlalchemy>=1.3.18
Review comment:
I think lower bounds are pretty meaningless unless we know we need some feature. They are really only used if someone has a version below that - in which case it will automatically upgrade the dependency when they upgrade airflow. I ten'd to leave the lower bounds unchanged unless there is a good reason to do so.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org