Skip to content

Sqlalchemy 2 fix#3429

Merged
dlstadther merged 11 commits into
spotify:masterfrom
pmgast:sqlalchemy_2_fix
Jun 12, 2026
Merged

Sqlalchemy 2 fix#3429
dlstadther merged 11 commits into
spotify:masterfrom
pmgast:sqlalchemy_2_fix

Conversation

@pmgast

@pmgast pmgast commented May 29, 2026

Copy link
Copy Markdown
Contributor

Description

As mentioned in the referenced PR luigi with enabled task_history is not compatible with sqlalchemy version >2 as raw sql is now supposed to be supplied with the text() construct (See https://docs.sqlalchemy.org/en/14/changelog/migration_20.html)
I have merged spotify/luigi master into the sqlalchemy_2_fix branch from @pjaol and updated the syntax changes.

Motivation and Context

#3267

Have you tested this? If so, how?

I tested with tox run -e py310-core locally.

@pmgast pmgast requested review from a team and dlstadther as code owners May 29, 2026 06:31
@pmgast pmgast force-pushed the sqlalchemy_2_fix branch 2 times, most recently from a6d3f14 to 4bbb851 Compare May 29, 2026 07:18
@pmgast pmgast force-pushed the sqlalchemy_2_fix branch from 4bbb851 to 8f0190d Compare May 29, 2026 09:10
@pmgast

pmgast commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

I get strange result during testing with the CI. It reports test/contrib/docker_runner_test.py::TestDockerTask::test_fail_job_container as failed but on my local machine the test passes, so it is hard for me to figure out what is wrong...

@dlstadther

Copy link
Copy Markdown
Collaborator

The docker contrib failure was transient. After rerunning CI, I now see a ruff format error. Please check and address.

@pmgast

pmgast commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

From my point of view this PR is ready to be merged. If more work is required one the coverage I would need some directions...

@dlstadther

Copy link
Copy Markdown
Collaborator

The failing check is CodeCov's patch coverage gate - it requires 50% of the new/changed lines in your PR to be covered by the test suite. This isn't a reflection on the quality of the fix itself.

To get this merged, there are two paths:

  1. Expand the tests in test/db_task_history_test.py to cover more of the changed lines in db_task_history.py - even basic mocking of the DB session can be enough to clear the 50% threshold.
  2. Update .codecov.yml to add the affected files to the ignore list (alongside the other hard-to-test database-backed modules already listed there). That would look something like:
ignore:
    - "luigi/db_task_history.py"

Either approach works. I've had mixed feelings about the codecov patch check and have almost removed it before; however, it does uncover areas of weaker test coverage in the project.

@pmgast pmgast force-pushed the sqlalchemy_2_fix branch from 7a4d299 to 853b9b1 Compare June 8, 2026 14:32
@pmgast pmgast force-pushed the sqlalchemy_2_fix branch from 853b9b1 to c51e989 Compare June 9, 2026 05:53

@dlstadther dlstadther left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for your persistence here! And sorry for my delays!

@dlstadther dlstadther merged commit af1c20f into spotify:master Jun 12, 2026
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants