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

Dependencies: Update requirement to psycopg~=3.0 #6362

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Apr 21, 2024

Fixes #5864

@sphuber sphuber force-pushed the fix/5864/psycopg3 branch from 4a52882 to e2b8b8e Compare April 21, 2024 11:30
@sphuber
Copy link
Contributor Author

sphuber commented Apr 22, 2024

Before merging, I want to see if I can perform some meaningful benchmarks to see if performance is affected at all.

@sphuber sphuber force-pushed the fix/5864/psycopg3 branch from e2b8b8e to 7e9f36d Compare May 30, 2024 09:01
@sphuber sphuber requested a review from mbercx May 30, 2024 09:09
@sphuber sphuber force-pushed the fix/5864/psycopg3 branch 5 times, most recently from 8a38816 to ed0499f Compare May 30, 2024 14:10
@sphuber sphuber force-pushed the fix/5864/psycopg3 branch 2 times, most recently from 8515fcb to d554e9b Compare May 30, 2024 19:21
@sphuber
Copy link
Contributor Author

sphuber commented May 30, 2024

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.

@sphuber
Copy link
Contributor Author

sphuber commented May 30, 2024

Another interesting thing is that since the very beginning of AiiDA (I remember this being there pre-v1.0) profiles (core.psql_dos ones) 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 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 postgres:// which is the default psycopg dialect and maps to postgres+psycopg2://. So we are currently targeting psycopg. This is why this PR updates the hardcoded protocol to postgres+psycopg:// to target the new version of psycopg.

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 verdi setup and verdi quicksetup are deprecated now anyway, we don't have to remove the options there I would say. The field is still present in the pydantic model of the PsqlDosStorage, but since it is not needed anyway, we might as well get rid of it.

Copy link

codecov bot commented May 30, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 77.81%. Comparing base (ef60b66) to head (423d2b8).
Report is 116 commits behind head on main.

Files with missing lines Patch % Lines
src/aiida/manage/tests/pytest_fixtures.py 0.00% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@sphuber sphuber force-pushed the fix/5864/psycopg3 branch from d554e9b to 4eb1cd1 Compare July 5, 2024 11:41
Copy link
Member

@mbercx mbercx left a 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.
@sphuber sphuber force-pushed the fix/5864/psycopg3 branch from 4eb1cd1 to 423d2b8 Compare July 11, 2024 13:24
@sphuber sphuber merged commit cba6e7c into aiidateam:main Jul 11, 2024
34 checks passed
@sphuber sphuber deleted the fix/5864/psycopg3 branch July 11, 2024 13:54
mikibonacci pushed a commit to mikibonacci/aiida-core that referenced this pull request Sep 3, 2024
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.
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.

Would there be any benefit in migrating from psycopg2 to psycopg3?
2 participants