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

Add support for sqlalchemy 2 #77

Merged
merged 10 commits into from
Mar 21, 2023

Conversation

filias
Copy link
Contributor

@filias filias commented Mar 14, 2023

This PR adds support for sqlalchemy 2.

I followed the docs for migrating to version 2:

First I made my local environment ready by being able to run the tests (I used only sqlite).

  • For that I had to upgrade poetry (I also tried to make a PR for that but it always failed in ci and I couldn't figure out the reason yet...). I also had to change the syntax of the toml file for github dependencies and bump the python version to >=3.7.

Then I turned on RemovedIn20Warnings and fixed those warnings (here is a list):

  • MovedIn20Warning: The declarative_base() function is now available as sqlalchemy.orm.declarative_base(). (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
  • RemovedIn20Warning: The legacy calling style of select() is deprecated and will be removed in SQLAlchemy 2.0. Please use the new calling style described at select(). (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
  • RemovedIn20Warning: This Session located a target engine via bound metadata; as this functionality will be removed in SQLAlchemy 2.0, an Engine object should be passed to the Session() constructor directly. (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
  • RemovedIn20Warning: The Query.from_self() method is considered legacy as of the 1.x series of SQLAlchemy and will be removed in 2.0. The new approach is to use the orm.aliased() construct in conjunction with a subquery. See the section "Selecting from the query itself as a subquery" in the 2.0 migration notes for an example. (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)

I added future=True to engine and sessionmaker. This was only in tests.

Finally I upgraded to sqlachemy 2.

@djrobstep I think I need your help here. I don't seem to be able to upgrade poetry. I tried also in a separate PR but did not manage. The output of poetry install does not show me any errors but the step fails in ci. Locally I have everything working fine 🤔 I did try a lot of ideas but nothing helped me understand why install fails. Can you help me? I think it would be great if sqlakeyset support sqlalchemy 2.0 :) Thanks!

Update 1: I suspect an out of memory error in circleci 🤔
Update 2: the problem was python-poetry/poetry#7184 and was solved by passing --no-ansi to the poetry install command.

Note: we might need to change the tests if we want to support sqlalchemy < 1.4

@filias filias marked this pull request as ready for review March 14, 2023 11:49
@filias filias force-pushed the add-support-for-sqlalchemy-2 branch from 631a609 to 790946a Compare March 17, 2023 09:46
@acarapetis
Copy link
Collaborator

Thanks for your effort!
We need to consider the best way forward regarding sqlalchemy-1 compatibility: I thought we'd need to bump our major version and drop support entirely, but from a quick look at your changes it seems we can pretty easily keep the same select_page API with backwards compatibility. I'll have a go at updating the tests to work with both versions (and perhaps add some new SQLAlchemy2-style ORM tests using select_page).

@acarapetis acarapetis force-pushed the add-support-for-sqlalchemy-2 branch from 790946a to ae5c2ae Compare March 19, 2023 05:17
@acarapetis
Copy link
Collaborator

I've merged #78 to master, rebased this branch on master and squashed a bunch of commits.
There is much less change than I was expecting to be necessary. Having trouble getting tests to run, but looks like it might be an issue with sqlbag and sqlalchemy2, working on it.
To be sure I understand, is the only reason for the sqla14/sqla20 split the fact that sqlalchemy.engine.row.LegacyRow no longer exists in sqlalchemy==2? If so, we can simplify this quite a bit.

filias and others added 3 commits March 19, 2023 18:05
- Updated result coercion logic to support sqlalchemy2
- Modified tests to use future=True and sqlalchemy2 style, including the
  following fixes:
  - Fix "The ``declarative_base()`` function is now available as sqlalchemy.orm.declarative_base()"
  - Fix "The legacy calling style of select() is deprecated and will be removed in SQLAlchemy 2.0.  Please use the new calling style described at select()"
  - Fix "This Session located a target engine via bound metadata; as this functionality will be removed in SQLAlchemy 2.0, an Engine object should be passed to the Session() constructor directly"
  - Fix "The legacy calling style of select() is deprecated and will be removed in SQLAlchemy 2.0.  Please use the new calling style described at select()"
  - Fix "RemovedIn20Warning: The Query.from_self() method is considered legacy as of the 1.x series of SQLAlchemy and will be removed in 2.0. The new approach is to use the orm.aliased() construct in conjunction with a subquery."
@acarapetis acarapetis force-pushed the add-support-for-sqlalchemy-2 branch from 3ff3c2e to e7b3192 Compare March 19, 2023 07:05
@filias
Copy link
Contributor Author

filias commented Mar 19, 2023

I've merged #78 to master, rebased this branch on master and squashed a bunch of commits. There is much less change than I was expecting to be necessary. Having trouble getting tests to run, but looks like it might be an issue with sqlbag and sqlalchemy2, working on it.

That's really great :) Thank you for following up on this :)

To be sure I understand, is the only reason for the sqla14/sqla20 split the fact that sqlalchemy.engine.row.LegacyRow no longer exists in sqlalchemy==2? If so, we can simplify this quite a bit.

Yes, that was the reason. Great that it can be simplified 👍

@acarapetis acarapetis force-pushed the add-support-for-sqlalchemy-2 branch from f3ba0eb to e2f6215 Compare March 20, 2023 05:21
@acarapetis acarapetis force-pushed the add-support-for-sqlalchemy-2 branch from e2f6215 to ca18b60 Compare March 20, 2023 06:00
- Use Python 3.7 (our new minimum supported version)
- Test with SQLAlchemy 2.0
- Use large resource class for more CPU cores
- Use pytest-xdist to run tests on 4 cores
@acarapetis acarapetis force-pushed the add-support-for-sqlalchemy-2 branch from ca18b60 to 2d89bf5 Compare March 20, 2023 06:05
@acarapetis
Copy link
Collaborator

As you can see I left the sqla14/sqla20 split more or less as you had it, just reorganized the files a little and made all imports explicit. Tests are passing with all sqlalchemy versions so I think we're good to go. I'm working on some additional stuff (type hints, asyncio support) on another branch but I'm going to go ahead and release this chunk now. Thanks again for your contribution!

@filias
Copy link
Contributor Author

filias commented Mar 21, 2023

As you can see I left the sqla14/sqla20 split more or less as you had it, just reorganized the files a little and made all imports explicit. Tests are passing with all sqlalchemy versions so I think we're good to go. I'm working on some additional stuff (type hints, asyncio support) on another branch but I'm going to go ahead and release this chunk now. Thanks again for your contribution!

Awesome 🎉
Thank you so much for following up on this 👏 🚀

@jokull
Copy link

jokull commented Mar 21, 2023

Wow - amazing effort!

@reinhardt1053
Copy link

Great job, thank you

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.

4 participants