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

Upgrade to SQLAlchemy 2 #1269

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pmdevita
Copy link
Contributor

@pmdevita pmdevita commented Feb 21, 2024

#1177

  • Fix issues upstream with Databases Fix some column types being parsed twice encode/databases#582
  • Update dependencies on Databases and SQLAlchemy
  • Update Ormar for SQLAlchemy 2 changes
    • select() now take multiple params rather than a collection
    • SQLAlchemy now compiles some statements differently, but logically equivalent

@pmdevita pmdevita changed the title Add support for SQLAlchemy 2 Upgrade tp SQLAlchemy 2 (#1177) Feb 21, 2024
@pmdevita pmdevita changed the title Upgrade tp SQLAlchemy 2 (#1177) Upgrade to SQLAlchemy 2 (#1177) Feb 21, 2024
@pmdevita pmdevita changed the title Upgrade to SQLAlchemy 2 (#1177) Upgrade to SQLAlchemy 2 Feb 21, 2024
Copy link

codspeed-hq bot commented Feb 21, 2024

CodSpeed Performance Report

Merging #1269 will degrade performances by 38.25%

Comparing pmdevita:feature/sqlalchemy2 (9bf214b) with master (3a206dd)

Summary

⚡ 3 improvements
❌ 16 regressions
✅ 65 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark master pmdevita:feature/sqlalchemy2 Change
test_exists[250] 13.5 ms 17.7 ms -23.73%
test_exists[500] 11.8 ms 14.8 ms -20.41%
test_first[250] 12.8 ms 10.5 ms +22.23%
test_first[500] 11.4 ms 13 ms -12.64%
test_get_one[1000] 14.6 ms 12.4 ms +17.63%
test_get_one[250] 9.7 ms 11.1 ms -12.94%
test_get_one[500] 11.5 ms 13.1 ms -12.14%
test_get_or_create_when_get[1000] 14.6 ms 12.5 ms +17.27%
test_get_or_create_when_get[250] 13 ms 14.5 ms -10.03%
test_get_or_create_when_get[500] 11.7 ms 13.2 ms -11.46%
test_get_or_none[1000] 14.6 ms 16.5 ms -11.61%
test_get_or_none[250] 10 ms 14.5 ms -31.1%
test_get_or_none[500] 11.1 ms 13.3 ms -16.95%
test_values[1000] 110.6 ms 179.1 ms -38.25%
test_values[250] 32.6 ms 50.2 ms -34.93%
test_values[500] 58.7 ms 91.2 ms -35.66%
test_values_list[1000] 115.5 ms 184.3 ms -37.34%
test_values_list[250] 37.4 ms 54.9 ms -31.93%
test_values_list[500] 60.9 ms 95.2 ms -35.98%

@collerek
Copy link
Owner

I know that we use sqlalchemy only in the core part but to be honest it's much fewer changes than I expected in this codebase for v2.0 support😅
Also appreciate you took it and solved it with databases too 🚀
Great job! 🎉
One test is failing now on parenthesis comparison (I think the one you changed).

@pmdevita
Copy link
Contributor Author

Yep, I haven't updated Poetry yet since I'm still waiting on databases to make a release. I can update it to point at their GitHub for now if you just want to see it pass.

Databases has now also dropped support for 3.7, should I do that as well as part of this PR?

It was a lot less changes than I was expecting too lol, I guess databases just bears most of the work for this.

@collerek
Copy link
Owner

collerek commented Mar 2, 2024

I can wait for the release, they just did 0.9.0 bit not sure if your change was included, guess so as it was merged

@pmdevita
Copy link
Contributor Author

pmdevita commented Mar 2, 2024

Oh yeah 0.9 should be good, I'll finish this up soon then

@pmdevita
Copy link
Contributor Author

pmdevita commented Mar 4, 2024

@collerek We need to remove 3.7 support in order to merge this, should I make another PR to do that? We might want to add 3.12 too while we're at it

@collerek
Copy link
Owner

collerek commented Mar 4, 2024

You can remove it here, it's removed in pydantic v2 pr already

@collerek
Copy link
Owner

BTW it seems like a significant performance degradation over the previous version, we need to check if caching is working as described here: https://docs.sqlalchemy.org/en/20/faq/performance.html

@gmekicaxcient
Copy link

How can I and/or others help? I just don't want to step on anyone's toes.

@pmdevita
Copy link
Contributor Author

Sorry, I'm going to try to come back around to this soon. I think there's a single PostgreSQL test that is failing still and there are the performance problems mentioned by collerek earlier. If you want, you can fork my branch and send a PR for me to merge into this one. Let me know if you want to try and tackle one of those, thanks for the help!

@gmekicaxcient
Copy link

I'll take a look, I still don't know if I have enough expertise to actually do something useful. If I find something, you'll get PR, of course.

@mekanix
Copy link
Contributor

mekanix commented Apr 9, 2024

@pmdevita I was at liberty to cherry-pick your changes onto master branch and create feature/sqlalchemy2. On my machine testing signals fails, but I'll leave that for later. I'll try to find what's going on with performance, first.

@mekanix
Copy link
Contributor

mekanix commented Apr 9, 2024

I did get [no key] in the output of benchmarks, but I never got sqlalchemy.ext.SAWarning nor did I find that TypeDecorator is used in ormar or databases. Any tips on where to look next, as this is caching problem. I am uploading output of pytest -s benchmarks just for reference.

@pmdevita
Copy link
Contributor Author

pmdevita commented Oct 20, 2024

I don't just want to sell out on the databases project... but its not getting a lot of support these days, and SQLAlchemy 2.0 does support async on its own now. I don't really know that much about Ormar's internals but is it worth cutting out the middle man and just working with SQLAlchemy directly?

@pmdevita
Copy link
Contributor Author

Well I went and dug a bit myself, looks like we only use the databases library to execute SQLAlchemy expressions asynchronously in QuerySet, which is very possible now in SQLAlchemy 2. There would be some public API changes since we'd no longer use databases to create the DB connection, but it doesn't seem like it would be a huge hassle and we'd benefit by not having to worry about the state of the databases library. Do you have any thoughts @collerek?

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