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/01/31 10:08:50 UTC
[GitHub] [airflow] potiuk opened a new pull request #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
potiuk opened a new pull request #21235:
URL: https://github.com/apache/airflow/pull/21235
The recent release of FAB 3.4.4 has unblocked us from upgrading
SQLAlchemy to 1.4.* version. We wanted to do it for quite some
time however upgrading to 1.4.* of sqlalchemy and allowing our
users to use it for 2.2.4 is a bit risky.
We are fixing resulting "aftermath" in the main branch and as
of this commit there are two fixes merged and remaining MsSQL
problem. The MSSql problem does not affect 2.2.4 as MsSQL will
be available only starting from 2.3.0, however the two other
problems have shown that SQLAlchemy has a potential to break
things and we might want to test it more thoroughly before
releasing 2.3.0.
The problems in question are #21205 and #21228. Both were only
test problems but the indicate that there might be more hidden
issues involved.
In order to limit risks, this PR proposes to limit SQLAlchemy
for 2.2.* to < 1.4.0. This will allow to upgrade FAB and
related dependencies without opening up Airflow to upgrade to
SQLAlchemy 1.4 (yet).
<!--
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 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 change, 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 [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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] ashb commented on pull request #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #21235:
URL: https://github.com/apache/airflow/pull/21235#issuecomment-1025696769
I thought we targeted v2-2-stable in case of a PR (and v2-2-test was only for cherry-picks and is then rebased on top of -stable as 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #21235:
URL: https://github.com/apache/airflow/pull/21235
--
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 #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21235:
URL: https://github.com/apache/airflow/pull/21235#issuecomment-1025571863
NOTE! This PR's target it `v2-2-test` - not `main` and the proposal is to limit SQLAlchemy in 2.2.4 and above (not in main - I am working to fix the last test failure resulting from SQLAlchemy update in main).
--
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 #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21235:
URL: https://github.com/apache/airflow/pull/21235#issuecomment-1025724779
> I thought we targeted v2-2-stable in case of a PR (and v2-2-test was only for cherry-picks and is then rebased on top of -stable as needed?)
Yeah. Usually yes, but we are really close to release and there are many changes cherry-picked to v2-2-stable already - including setup.py changes, so wanted to avoid merge conflicts - we have no changes in stable yet since it was released, so if we make it to v2-2-test, then moving stable will be just "fast-forward".
--
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 edited a comment on pull request #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21235:
URL: https://github.com/apache/airflow/pull/21235#issuecomment-1025724779
> I thought we targeted v2-2-stable in case of a PR (and v2-2-test was only for cherry-picks and is then rebased on top of -stable as needed?)
Yeah. Usually yes, but we are really close to release and there are many changes cherry-picked to `v2-2-test` already - including setup.py/.cfg changes, so wanted to avoid merge conflicts - we have no changes in stable yet since it was released, so if we make it to v2-2-test, then moving stable will be just "fast-forward".
--
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 #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21235:
URL: https://github.com/apache/airflow/pull/21235#issuecomment-1025762256
> Hmm, do we have a mechanism to limit SQLAlchemy < 1.4 _in the constraints file_, but not in the actual package metadata? This way we can “officially” only support <1.4, but someone adventurous enough to want 1.4+ can still have that without the package metadata blocking the possibility. I feel that’d be a better approach.
Yes. We can do that as well. We already do that in few other cases and that's also a possibilitty.
The way we do that we add limitation in the Dockerfile.ci (this image is used in CI to prepare the constraints):
https://github.com/apache/airflow/blob/v2-2-test/Dockerfile.ci#L276
Actually I toyed with that idea too, but I thought that `install_requires` is a bit safer as SQLALchemy is really important piece for us. I think this case is actually a very nice supportive (and not strawman) case to the proposal I made here:
https://lists.apache.org/thread/250pfvogkqb31g2vj5p0yz3ntz5qj1ht
Sqlalchemy for me is really important piece of Airflow and even small change might impact us a lot - for example performance certain queries might change dramaticaly if SqlAlchemy changes the way how they are generated - even if for them this will be a "small change" and results might be ''catastrophic'.
As I explained in my latest proposal there, I think we should identifty all the "really important" dependencies (few of them) and upper-bound them, all the rest we should not. This way we could take a "deliberate" effort to migrate to higher versions (as we do now in `main` trying to fix it and then later release it in 2.3.0 with enough manual testing. Just having a few of those, will make sure that we can manage the upper-bounds "deliberately". All the rest can easily rely on constraints.
My point here is that sqlalchemy is on top of the list of those deps which should be upperbound (if we decide to upperbound dependencies). So if we agreee that sometimes we do upper-bound, IMHO we should do it here.
But if we decide to relax it, I am also fine (but then IMHO we should relax all the upperbound limits in this case).
--
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] jedcunningham commented on pull request #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #21235:
URL: https://github.com/apache/airflow/pull/21235#issuecomment-1025997377
We see enough users not using constraints that I don't think we should rely on it in this case, especially given we are so close to 2.2.4's release.
--
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 edited a comment on pull request #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21235:
URL: https://github.com/apache/airflow/pull/21235#issuecomment-1025724779
> I thought we targeted v2-2-stable in case of a PR (and v2-2-test was only for cherry-picks and is then rebased on top of -stable as needed?)
Yeah. Usually yes, but we are really close to release and there are many changes cherry-picked to `v2-2-test` already - including setup.py/.cfg changes, so wanted to avoid merge conflicts - we have no changes in stable yet since it was released, so if we make it to `v2-2-test`, then moving stable will be just "fast-forward".
--
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 edited a comment on pull request #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21235:
URL: https://github.com/apache/airflow/pull/21235#issuecomment-1025724779
> I thought we targeted v2-2-stable in case of a PR (and v2-2-test was only for cherry-picks and is then rebased on top of -stable as needed?)
Yeah. Usually yes, but we are really close to release and there are many changes cherry-picked to `v2-2-test` already - including setup.py changes, so wanted to avoid merge conflicts - we have no changes in stable yet since it was released, so if we make it to v2-2-test, then moving stable will be just "fast-forward".
--
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 edited a comment on pull request #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21235:
URL: https://github.com/apache/airflow/pull/21235#issuecomment-1025724779
> I thought we targeted v2-2-stable in case of a PR (and v2-2-test was only for cherry-picks and is then rebased on top of -stable as needed?)
Yeah. Usually yes, but we are really close to release and there are many changes cherry-picked to `v2-2-test` already - including setup.py/.cfg changes, so wanted to avoid merge conflicts - we have no changes in `v2-2-stable` yet since it was released, so if we make it to `v2-2-test`, then moving stable will be just "fast-forward".
--
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 edited a comment on pull request #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21235:
URL: https://github.com/apache/airflow/pull/21235#issuecomment-1025762256
> Hmm, do we have a mechanism to limit SQLAlchemy < 1.4 _in the constraints file_, but not in the actual package metadata? This way we can “officially” only support <1.4, but someone adventurous enough to want 1.4+ can still have that without the package metadata blocking the possibility. I feel that’d be a better approach.
Yes. We can do that as well. We already do that in few other cases and that's also a possibilitty.
The way we do that we add limitation in the Dockerfile.ci (this image is used in CI to prepare the constraints):
https://github.com/apache/airflow/blob/v2-2-test/Dockerfile.ci#L276
Actually I toyed with that idea too, but I thought that `install_requires` is a bit safer as SQLALchemy is really important piece for us. I think this case is actually a very nice supportive (and not strawman) case to the proposal I made here:
https://lists.apache.org/thread/250pfvogkqb31g2vj5p0yz3ntz5qj1ht
Sqlalchemy for me is really important piece of Airflow and even small change might impact us a lot - for example performance of certain queries might change dramaticaly if SqlAlchemy changes the way how they are generated - even if for them this will be a "small change" and results might be ''catastrophic'.
As I explained in my latest proposal there, I think we should identifty all the "really important" dependencies (few of them) and upper-bound them, all the rest we should not. This way we could take a "deliberate" effort to migrate to higher versions (as we do now in `main` trying to fix it and then later release it in 2.3.0 with enough manual testing. Just having a few of those, will make sure that we can manage the upper-bounds "deliberately". All the rest can easily rely on constraints.
My point here is that sqlalchemy is on top of the list of those deps which should be upperbound (if we decide to upperbound dependencies). So if we agreee that sometimes we do upper-bound, IMHO we should do it here.
But if we decide to relax it, I am also fine (but then IMHO we should relax all the upperbound limits in this case).
--
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] github-actions[bot] commented on pull request #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #21235:
URL: https://github.com/apache/airflow/pull/21235#issuecomment-1025697439
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.
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 pull request #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #21235:
URL: https://github.com/apache/airflow/pull/21235#issuecomment-1025741502
Hmm, do we have a mechanism to limit SQLAlchemy < 1.4 _in the constraints file_, but not in the actual package metadata? This way we can “officially” only support <1.4, but someone adventurous enough to want 1.4+ can still have that without the package metadata blocking the possibility. I feel that’d be a better approach.
--
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 #21235: Limit SQLAlchemy to < 1.4.0 for 2.2.* line
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21235:
URL: https://github.com/apache/airflow/pull/21235#issuecomment-1026153268
And this one - I thing should be quite proving the point on upper-bounding SQLAlchemy is a good idea:
https://github.com/apache/airflow/pull/21238
This was only MsSQL but I wonder what else will find when we start more thoroughly testing 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