-
Notifications
You must be signed in to change notification settings - Fork 191
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
Dependencies: Update requirement to psycopg~=3.0
#6362
Conversation
4a52882
to
e2b8b8e
Compare
Before merging, I want to see if I can perform some meaningful benchmarks to see if performance is affected at all. |
e2b8b8e
to
7e9f36d
Compare
8a38816
to
ed0499f
Compare
8515fcb
to
d554e9b
Compare
Although psycopg is supposed to be more efficient, there are a few sources online that suggest this is perhaps not always trivially the case: Not sure if these differences would even be relevant compared to other overhead added by AiiDA's code. I have not found any clear indication on the psycopg2 documentation about its lifetime and if/when there will be an end-of-life. But it is pretty sure that support will stop at some point and we might be forced to move to psycopg v3 regardless. |
Another interesting thing is that since the very beginning of AiiDA (I remember this being there pre-v1.0) profiles (
The funny thing is that this is not actuall used whatsoever. We could have used this in the connection string when creating the sqlalchemy engine. We currently hardcode this to Because it is hardcoded, we don't need a migration for the existing configs, although it would perhaps be nice to clean it up from existing profiles. Since |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6362 +/- ##
==========================================
+ Coverage 77.51% 77.81% +0.31%
==========================================
Files 560 562 +2
Lines 41444 41888 +444
==========================================
+ Hits 32120 32593 +473
+ Misses 9324 9295 -29 ☔ View full report in Codecov by Sentry. |
d554e9b
to
4eb1cd1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed during the AiiDA meeting, let's merge this version bump into main
so we can start testing it in production projects.
The connection string for SQLAlchemy needs to change to `postgres+psycopg://` for it to target psycopg v3.
4eb1cd1
to
423d2b8
Compare
The `psycopg` library is used by `sqlalchemy` to connect to the PostgreSQL server. So far, the `psycopg2` package was used, but the next generation v3 has already been out for a while. The release comes with a number of performance improvements according to the author. Although there is no clear timeline for support of v2, we are not waiting and switching now to the new version. When v2 was released, instead of releasing a new major version, it was released as a new library changing the name from `psycopg` to `psycopg2` but for v3 they are switching back to `psycopg`. This means that users would still be able to run v2 and v3 along side one another in a single Python environment. This supports the decision to move to v3. Note that `aiida-core` will not be supporting both at the same time and from now only supports v3. Interestingly, from the very early versions of AiiDA, the profile would contain the `database_engine` key. This is still an option in `verdi setup` and `verdi quicksetup`, except it is not really an option as it is hardcoded to `postgresql_psycopg2`. So all `core.psql_dos` profiles out there contain: 'main': { 'storage': { 'backend': 'core.psql_dos', 'config': { 'database_engine': 'postgresql_psycopg2', ... } }, } The value is not actually used however as the connection string for sqlalchemy's engine, where it _could_ be used, simply hardcodes this to `postgres://` which is the default psycopg dialect and maps to `postgres+psycopg2://`. This value is now simply updated to `postgres+psycopg://` to target the new version of `psycopg`. Because it is hardcoded, a migration for the existing configs is not required and it can be left as a vestigial attribute. Since `verdi setup` and `verdi quicksetup` are deprecated now anyway, the options don't have to be removed here.
Fixes #5864