Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update min-sqlalchemy version to account for latest features used #34293

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Sep 12, 2023

Some of the recent sqlalchemy changes are not working with minimum version of sqlalchemy of ours - for example where syntax does not allow moe than one clause and we are already passing more in _do_delete_old_records (added in #33527). This syntax however was added in SQL Alchemy 1.4.28 and our minimum version was 1.4.27.

This change bumps the minimum SQLAlchemy version to 1.4.28 but it also adds a special test job that only runs on Postgres that downgrades the SQLAlchemy to the minimum supported version (retrieved from setup.cfg). This way, we will be able to detect such incompatible changes at the PR time. This is a new flag --downgrade-sqlalchemy on test command that works similar to earlier --upgrade-boto.

We also enable the --upgrade-boto and --downgrade-sqlalchemy flags to be used for breeze shell command - thanks to that we can easily test both flags with breeze shell command.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
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.

@potiuk
Copy link
Member Author

potiuk commented Sep 12, 2023

I am not sure if this test will pass here - we might have other functionality used from even later versions of sqlalchemy - but once this PR gets merged we should be protected against making similar accidental SQLAlchemy new features used without bumping the min version.

SQLAlchemy - unfortunately - does not follow SEMVER and they are adding features in patchlevel versions, so we might accidentally use features (like it was in this case) added at any release. This one should protect us.

@potiuk
Copy link
Member Author

potiuk commented Sep 12, 2023

I am also thinking about having a separate test where we downgrade EVERYTHING possible to the lowest possible versions and run tests for it. But this one wil likely require reconciliation and revisiting MANY min-versions we have and some smart way of figuring what should be min-versions for some of our dependencies.

#34296

Some of the recent sqlalchemy changes are not working with minimum
version of sqlalchemy of ours - for example `where` syntax does
not allow moe than one clause and we are already passing more
in _do_delete_old_records (added in apache#33527). This syntax however
was added in SQL Alchemy 1.4.28 and our minimum version was
1.4.27.

This change bumps the minimum SQLAlchemy version to 1.4.28 but it also
adds a special test job that only runs on Postgres that downgrades
the SQLAlchemy to the minimum supported version (retrieved from
setup.cfg). This way, we will be able to detect such incompatible
changes at the PR time. This is a new flag `--downgrade-sqlalchemy`
on test command that works similar to earlier `--upgrade-boto`.

We also enable the `--upgrade-boto` and `--downgrade-sqlalchemy` flags
to be used for `breeze shell` command - thanks to that we can
easily test both flags with `breeze shell` command.
@Taragolis
Copy link
Contributor

With every step we are getting closer to pin to 1.4.49 😩

@uranusjr
Copy link
Member

I think eventually we want to allow the entire 2.x series anyway, at which point disallowing everything under 1.4.49 would not be a big problem.

@potiuk
Copy link
Member Author

potiuk commented Sep 12, 2023

I think eventually we want to allow the entire 2.x series anyway, at which point disallowing everything under 1.4.49 would not be a big problem.

Yep. I am all for bumping min requirements pretty much every time we have a good reason for that and risk for conflicting with other dependencies is low. This only benefits the entire ecosystem and makes our users less vulnerable to security problems, and leads to far less number of strange issues that are difficult to reproduce.

If we do not have a good mechanism (like this one) to check if a min-version still works, basically we are trading a potential that user will not be able to install newer version of airflow without upgrading something else (or sometimes won't be able to upgrade at all) with the risk that some of the things for some of the features the user needs will fail with random and difficult to explain issues.

If you ask me - I very much prefer the former where we explicitly tell the user "you need to upgrade to use the new version" rather than silently ignoring the fact that it may or might not work if we have not updated min-requirement for a while (this is basically what we do now).

We are trading explicitness of communicating to user with implicitness of sometimes randomly things failing.

@potiuk potiuk merged commit efbead9 into apache:main Sep 12, 2023
65 checks passed
@potiuk potiuk deleted the min-sql-alchemy branch September 12, 2023 09:49
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.2 milestone Oct 3, 2023
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Oct 3, 2023
ephraimbuddy pushed a commit that referenced this pull request Oct 5, 2023
…4293)

Some of the recent sqlalchemy changes are not working with minimum
version of sqlalchemy of ours - for example `where` syntax does
not allow moe than one clause and we are already passing more
in _do_delete_old_records (added in #33527). This syntax however
was added in SQL Alchemy 1.4.28 and our minimum version was
1.4.27.

This change bumps the minimum SQLAlchemy version to 1.4.28 but it also
adds a special test job that only runs on Postgres that downgrades
the SQLAlchemy to the minimum supported version (retrieved from
setup.cfg). This way, we will be able to detect such incompatible
changes at the PR time. This is a new flag `--downgrade-sqlalchemy`
on test command that works similar to earlier `--upgrade-boto`.

We also enable the `--upgrade-boto` and `--downgrade-sqlalchemy` flags
to be used for `breeze shell` command - thanks to that we can
easily test both flags with `breeze shell` command.

(cherry picked from commit efbead9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants