Update code style for `airflow db` commands to SQLAlchemy 2.0 style by ephraimbuddy · Pull Request #31486 · apache/airflow

ephraimbuddy · 2023-05-23T18:23:04Z

This commit introduces changes to the code styles of airflow db commands to remove 'RemovedIn20Warning' and ensure compatibility with SQLAlchemy 2.0.

To see these warnings, you need to set SQLALCHEMY_WARN_20=True when using the db commands

Part of #28723

uranusjr · 2023-05-24T02:29:52Z

Hmm, db init is failing with

sqlalchemy.exc.IntegrityError: (psycopg2.errors.NotNullViolation) null value in column "created_at" violates not-null constraint
DETAIL:  Failing row contains (1, {{ ti.dag_id }}/{{ ti.task_id }}/{{ ts }}/{{ try_number }}.log, {dag_id}-{task_id}-{execution_date}-{try_number}, null).

But LogTemplate.created_at has default=timezone.utcnow, which should kick in to add a default value on the client side to the insert query, according to documentation. This has been working fine in 1.x and I don’t see any notes indicating anything changed in 2.0. Something is off.

ephraimbuddy · 2023-05-24T06:25:13Z

Hmm, db init is failing with
sqlalchemy.exc.IntegrityError: (psycopg2.errors.NotNullViolation) null value in column "created_at" violates not-null constraint
DETAIL:  Failing row contains (1, {{ ti.dag_id }}/{{ ti.task_id }}/{{ ts }}/{{ try_number }}.log, {dag_id}-{task_id}-{execution_date}-{try_number}, null).
But LogTemplate.created_at has default=timezone.utcnow, which should kick in to add a default value on the client side to the insert query, according to documentation. This has been working fine in 1.x and I don’t see any notes indicating anything changed in 2.0. Something is off.

It should be fine now. I think I went overboard and used the metadata we use in creating DB models to reflect tables

uranusjr · 2023-05-25T07:02:39Z

Is the bind=bind part still needed? You already set metadata.bind above.

Yeah. It was surprising to me too. Here's the warning that it throws:

/opt/airflow/airflow/utils/db.py:1023 RemovedIn20Warning: The ``bind`` argument for schema methods that invoke SQL against an engine or connection will be required in SQLAlchemy 2.0. (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)

Do we need the metadata.bind line also then? Seems odd to need both.

Have removed it. I remember having some issues around the metadata but let's see if the CI passes cause I can't reproduce locally

Here's the error https://github.com/apache/airflow/actions/runs/5083011128/jobs/9133579355?pr=31486#step:5:10813, adding the bind back...

What happened was that the metadata that was returned by the function was not bound to any database. The other binds are for the reflect methods. Despite the session being bound to metadata, they still want the bind to happen on its method

Maybe instead we can add bind=bind explicitly to the method calls instead? Not sure how feasible that is without actually digging into the call stack.

Seeing this now...taking a look

See 8a08c05, let me know what you think

jedcunningham · 2023-05-25T16:17:06Z

Do we need the metadata.bind line also then? Seems odd to need both.

ephraimbuddy · 2023-05-26T06:10:41Z

Please don't merge yet, investigating MSSQL failure locally(it's canceled on CI)

ephraimbuddy · 2023-05-26T09:00:11Z

Please don't merge yet, investigating MSSQL failure locally(it's canceled on CI)

Resolved: 98643ee

This commit introduces changes to the code styles of `airflow db` commands to remove 'RemovedIn20Warning' and ensure compatibility with SQLAlchemy 2.0. To see these warnings, you need to set SQLALCHEMY_WARN_20=True when using the db commands

…style

…my 2.0 style

ephraimbuddy force-pushed the removed-in-20-dbreset branch from 9619d4b to d5943f4 Compare May 24, 2023 06:22

ephraimbuddy force-pushed the removed-in-20-dbreset branch from d5943f4 to 43d43cb Compare May 24, 2023 07:29

ephraimbuddy marked this pull request as ready for review May 24, 2023 07:43

ephraimbuddy requested review from XD-DENG, ashb and kaxil as code owners May 24, 2023 07:43

ephraimbuddy force-pushed the removed-in-20-dbreset branch 2 times, most recently from be4a85a to fdeaf05 Compare May 24, 2023 14:56

uranusjr added use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing) and removed use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing) labels May 24, 2023

ferruzzi added the area:core label May 24, 2023

ephraimbuddy force-pushed the removed-in-20-dbreset branch from fdeaf05 to f7bef4f Compare May 25, 2023 05:42

uranusjr reviewed May 25, 2023

View reviewed changes

Comment thread airflow/utils/db.py Outdated

uranusjr reviewed May 25, 2023

View reviewed changes

jedcunningham approved these changes May 25, 2023

View reviewed changes

Comment thread airflow/utils/db.py Outdated

jedcunningham May 25, 2023

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the metadata.bind line also then? Seems odd to need both.

ephraimbuddy force-pushed the removed-in-20-dbreset branch 2 times, most recently from 35991d5 to c2423ca Compare May 25, 2023 18:17

ephraimbuddy force-pushed the removed-in-20-dbreset branch from c2423ca to 98643ee Compare May 26, 2023 08:58

ephraimbuddy added 4 commits May 26, 2023 11:37

fixup! Update code style for airflow db commands to SQLAlchemy 2.0 …

aebbc00

…style

fixup! fixup! Update code style for airflow db commands to SQLAlche…

4ea473a

…my 2.0 style

Use connection instead of session.get_bind()

edbbb7e

ephraimbuddy force-pushed the removed-in-20-dbreset branch from 98643ee to edbbb7e Compare May 26, 2023 10:37

remove metadata.bind=bind

8a08c05

ephraimbuddy mentioned this pull request May 26, 2023

Refactor sqlalchemy queries to 2.0 style (Part 1) #31569

Merged

dstandish approved these changes May 26, 2023

View reviewed changes

ephraimbuddy merged commit afa9ead into apache:main May 26, 2023

ephraimbuddy deleted the removed-in-20-dbreset branch May 26, 2023 19:30

ephraimbuddy added this to the Airflow 2.7.0 milestone Jul 6, 2023

ephraimbuddy added the type:improvement Changelog: Improvements label Jul 6, 2023

Uh oh!

Conversation

ephraimbuddy commented May 23, 2023

Uh oh!

uranusjr commented May 24, 2023

Uh oh!

ephraimbuddy commented May 24, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ephraimbuddy commented May 26, 2023

Uh oh!

ephraimbuddy commented May 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants